Merge pull request #3098 from processing/fix/updates-address-outage

Updates to earlier fixes to address outage logs

Author: raclim

server/config/passport.js

@@ -22,7 +22,11 @@ function generateUniqueUsername(username) {
 }
 
 passport.serializeUser((user, done) => {
-  done(null, user.id);
+  if (user) {
+    done(null, user.id);
+  } else {
+    done(new Error('User is not available for serialization.'));
+  }
 });
 
 passport.deserializeUser((id, done) => {

server/controllers/aws.controller.js

@@ -39,37 +39,40 @@ export function getObjectKey(url) {
   }
   return objectKey;
 }
-
+// TODO: callback should be removed
 export async function deleteObjectsFromS3(keyList, callback) {
   const objectsToDelete = keyList?.map((key) => ({ Key: key }));
 
-  if (objectsToDelete.length > 0) {
-    const params = {
-      Bucket: process.env.S3_BUCKET,
-      Delete: { Objects: objectsToDelete }
-    };
+  if (!objectsToDelete.length) {
+    callback?.();
+    return;
+  }
 
-    try {
-      await s3Client.send(new DeleteObjectsCommand(params));
-      if (callback) {
-        callback();
-      }
-    } catch (error) {
-      console.error('Error deleting objects from S3: ', error);
-      if (callback) {
-        callback(error);
-      }
-    }
-  } else if (callback) {
-    callback();
+  const params = {
+    Bucket: process.env.S3_BUCKET,
+    Delete: { Objects: objectsToDelete }
+  };
+
+  try {
+    const deleteResult = await s3Client.send(new DeleteObjectsCommand(params));
+    callback?.(null, deleteResult);
+  } catch (error) {
+    callback?.(error);
   }
 }
 
 export function deleteObjectFromS3(req, res) {
   const { objectKey, userId } = req.params;
   const fullObjectKey = userId ? `${userId}/${objectKey}` : objectKey;
-  deleteObjectsFromS3([fullObjectKey], () => {
-    res.json({ success: true });
+
+  deleteObjectsFromS3([fullObjectKey], (error, result) => {
+    if (error) {
+      return res
+        .status(500)
+        .json({ error: 'Failed to delete object from s3.' });
+    }
+
+    return res.json({ success: true, message: 'Object deleted successfully.' });
   });
 }
 

server/controllers/project.controller/__test__/deleteProject.test.js

@@ -3,112 +3,79 @@
  */
 import { Request, Response } from 'jest-express';
 
-import Project, {
-  createMock,
-  createInstanceMock
-} from '../../../models/project';
+import Project from '../../../models/project';
 import User from '../../../models/user';
 import deleteProject from '../../project.controller/deleteProject';
 import { deleteObjectsFromS3 } from '../../aws.controller';
 
 jest.mock('../../../models/project');
 jest.mock('../../aws.controller');
 
-describe('project.controller', () => {
-  describe('deleteProject()', () => {
-    let ProjectMock;
-    let ProjectInstanceMock;
-
-    beforeEach(() => {
-      ProjectMock = createMock();
-      ProjectInstanceMock = createInstanceMock();
-    });
-
-    afterEach(() => {
-      ProjectMock.restore();
-      ProjectInstanceMock.restore();
-    });
+// TODO: incomplete test, 500 response status needs to be added
 
-    it('returns 403 if project is not owned by authenticated user', (done) => {
-      const user = new User();
-      const project = new Project();
-      project.user = user;
+describe('project.controller', () => {
+  let request;
+  let response;
 
-      const request = new Request();
-      request.setParams({ project_id: project._id });
-      request.user = { _id: 'abc123' };
+  beforeEach(() => {
+    request = new Request();
+    response = new Response();
+    Project.findById = jest.fn();
+  });
 
-      const response = new Response();
+  afterEach(() => {
+    request.resetMocked();
+    response.resetMocked();
+  });
 
-      ProjectMock.expects('findById').resolves(project);
+  it('returns 403 if project is not owned by authenticated user', async () => {
+    const user = new User();
+    const project = new Project();
+    project.user = user;
 
-      const promise = deleteProject(request, response);
+    request.setParams({ project_id: project._id });
+    request.user = { _id: 'abc123' };
 
-      function expectations() {
-        expect(response.status).toHaveBeenCalledWith(403);
-        expect(response.json).toHaveBeenCalledWith({
-          message: 'Authenticated user does not match owner of project'
-        });
+    Project.findById.mockResolvedValue(project);
 
-        done();
-      }
+    await deleteProject(request, response);
 
-      promise.then(expectations, expectations).catch(expectations);
+    expect(response.status).toHaveBeenCalledWith(403);
+    expect(response.json).toHaveBeenCalledWith({
+      message: 'Authenticated user does not match owner of project'
     });
+  });
 
-    it('returns 404 if project does not exist', (done) => {
-      const user = new User();
-      const project = new Project();
-      project.user = user;
-
-      const request = new Request();
-      request.setParams({ project_id: project._id });
-      request.user = { _id: 'abc123' };
-
-      const response = new Response();
-
-      ProjectMock.expects('findById').resolves(null);
-
-      const promise = deleteProject(request, response);
+  it('returns 404 if project does not exist', async () => {
+    request.setParams({ project_id: 'random_id' });
+    request.user = { _id: 'abc123' };
 
-      function expectations() {
-        expect(response.status).toHaveBeenCalledWith(404);
-        expect(response.json).toHaveBeenCalledWith({
-          message: 'Project with that id does not exist'
-        });
+    Project.findById.mockResolvedValue(null);
 
-        done();
-      }
+    await deleteProject(request, response);
 
-      promise.then(expectations, expectations).catch(expectations);
+    expect(response.status).toHaveBeenCalledWith(404);
+    expect(response.json).toHaveBeenCalledWith({
+      message: 'Project with that id does not exist'
     });
+  });
 
-    it('deletes project and dependent files from S3 ', (done) => {
-      const user = new User();
-      const project = new Project();
-      project.user = user;
-
-      const request = new Request();
-      request.setParams({ project_id: project._id });
-      request.user = { _id: user._id };
-
-      const response = new Response();
-
-      ProjectMock.expects('findById').resolves(project);
-
-      ProjectInstanceMock.expects('remove').yields();
+  it('delete project and dependent files from S3', async () => {
+    const user = new User();
+    const project = new Project();
+    project.user = user;
+    project.remove = jest.fn().mockResolvedValue();
 
-      const promise = deleteProject(request, response);
+    request.setParams({ project_id: project._id });
+    request.user = { _id: user._id };
 
-      function expectations() {
-        expect(response.status).toHaveBeenCalledWith(200);
-        expect(response.json).not.toHaveBeenCalled();
-        expect(deleteObjectsFromS3).toHaveBeenCalled();
+    Project.findById.mockResolvedValue(project);
+    deleteObjectsFromS3.mockResolvedValue();
 
-        done();
-      }
+    await deleteProject(request, response);
 
-      promise.then(expectations, expectations).catch(expectations);
-    });
+    expect(response.status).toHaveBeenCalledWith(200);
+    expect(deleteObjectsFromS3).toHaveBeenCalled();
+    expect(project.remove).toHaveBeenCalled();
   });
 });

server/controllers/project.controller/deleteProject.js

@@ -7,45 +7,42 @@ const ProjectDeletionError = createApplicationErrorClass(
   'ProjectDeletionError'
 );
 
-function deleteFilesFromS3(files) {
-  deleteObjectsFromS3(
-    files
-      .filter((file) => {
-        if (
-          file.url &&
+async function deleteFilesFromS3(files) {
+  const filteredFiles = files
+    .filter((file) => {
+      const isValidFile =
+        (file.url &&
           (file.url.includes(process.env.S3_BUCKET_URL_BASE) ||
-            file.url.includes(process.env.S3_BUCKET))
-        ) {
-          if (
-            !process.env.S3_DATE ||
-            (process.env.S3_DATE &&
-              isBefore(new Date(process.env.S3_DATE), new Date(file.createdAt)))
-          ) {
-            return true;
-          }
-        }
-        return false;
-      })
-      .map((file) => getObjectKey(file.url))
-  );
+            file.url.includes(process.env.S3_BUCKET)) &&
+          !process.env.S3_DATE) ||
+        (process.env.S3_DATE &&
+          isBefore(new Date(process.env.S3_DATE), new Date(file.createdAt)));
+
+      return isValidFile;
+    })
+    .map((file) => getObjectKey(file.url));
+
+  try {
+    await deleteObjectsFromS3(filteredFiles);
+  } catch (error) {
+    console.error('Failed to delete files from S3:', error);
+  }
 }
 
-export default function deleteProject(req, res) {
-  function sendFailure(error) {
+export default async function deleteProject(req, res) {
+  const sendFailure = (error) => {
     res.status(error.code).json({ message: error.message });
-  }
+  };
 
-  function sendProjectNotFound() {
-    sendFailure(
-      new ProjectDeletionError('Project with that id does not exist', {
-        code: 404
-      })
-    );
-  }
+  try {
+    const project = await Project.findById(req.params.project_id);
 
-  function handleProjectDeletion(project) {
-    if (project == null) {
-      sendProjectNotFound();
+    if (!project) {
+      sendFailure(
+        new ProjectDeletionError('Project with that id does not exist', {
+          code: 404
+        })
+      );
       return;
     }
 
@@ -59,19 +56,20 @@ export default function deleteProject(req, res) {
       return;
     }
 
-    deleteFilesFromS3(project.files);
-
-    project.remove((removeProjectError) => {
-      if (removeProjectError) {
-        sendProjectNotFound();
-        return;
-      }
+    try {
+      await deleteFilesFromS3(project.files);
+    } catch (error) {
+      sendFailure(
+        new ProjectDeletionError('Failed to delete associated project files.', {
+          code: 500
+        })
+      );
+      return;
+    }
 
-      res.status(200).end();
-    });
+    await project.remove();
+    res.status(200).end();
+  } catch (error) {
+    sendFailure(error);
   }
-
-  return Project.findById(req.params.project_id)
-    .then(handleProjectDeletion)
-    .catch(sendFailure);
 }

server/models/user.js

@@ -163,7 +163,6 @@ userSchema.methods.comparePassword = async function comparePassword(
   candidatePassword
 ) {
   if (!this.password) {
-    console.error('No password is set for this user.');
     return false;
   }