Merge pull request #3106 from processing/revert-changes

Revert some changes from previous updates to deleteProject

Author: raclim

.github/workflows/deploy-staging.yml

@@ -3,7 +3,7 @@ on:
   workflow_run:
     workflows: ["Test"]
     branches:
-      - release-delete-sketch
+      - develop
     types:
       - completed
 env:

server/controllers/aws.controller.js

@@ -43,19 +43,6 @@ export function getObjectKey(url) {
 export async function deleteObjectsFromS3(keyList, callback) {
   const objectsToDelete = keyList?.map((key) => ({ Key: key }));
 
-  const sendResponse = (cb, err) => {
-    if (cb && err) {
-      callback(err);
-    } else if (cb) {
-      callback();
-    } else if (!cb && err) {
-      console.error('Failed to delete objects from S3.');
-      throw err;
-    }
-
-    return 'Objects successfully deleted from S3.';
-  };
-
   if (objectsToDelete.length > 0) {
     const params = {
       Bucket: process.env.S3_BUCKET,
@@ -64,12 +51,17 @@ export async function deleteObjectsFromS3(keyList, callback) {
 
     try {
       await s3Client.send(new DeleteObjectsCommand(params));
-      sendResponse();
+      if (callback) {
+        callback();
+      }
     } catch (error) {
-      sendResponse();
+      console.error('Error deleting objects from S3: ', error);
+      if (callback) {
+        callback(error);
+      }
     }
-  } else {
-    sendResponse();
+  } else if (callback) {
+    callback();
   }
 }
 

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

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

server/controllers/project.controller/deleteProject.js

@@ -7,7 +7,7 @@ const ProjectDeletionError = createApplicationErrorClass(
   'ProjectDeletionError'
 );
 
-async function deleteFilesFromS3(files) {
+function deleteFilesFromS3(files) {
   const filteredFiles = files
     .filter((file) => {
       const isValidFile =
@@ -22,27 +22,25 @@ async function deleteFilesFromS3(files) {
     })
     .map((file) => getObjectKey(file.url));
 
-  try {
-    await deleteObjectsFromS3(filteredFiles);
-  } catch (error) {
-    console.error('Failed to delete files from S3:', error);
-  }
+  deleteObjectsFromS3(filteredFiles);
 }
 
-export default async function deleteProject(req, res) {
-  const sendFailure = (error) => {
+export default function deleteProject(req, res) {
+  function sendFailure(error) {
     res.status(error.code).json({ message: error.message });
-  };
+  }
 
-  try {
-    const project = await Project.findById(req.params.project_id);
+  function sendProjectNotFound() {
+    sendFailure(
+      new ProjectDeletionError('Project with that id does not exist', {
+        code: 404
+      })
+    );
+  }
 
-    if (!project) {
-      sendFailure(
-        new ProjectDeletionError('Project with that id does not exist', {
-          code: 404
-        })
-      );
+  function handleProjectDeletion(project) {
+    if (project == null) {
+      sendProjectNotFound();
       return;
     }
 
@@ -56,10 +54,19 @@ export default async function deleteProject(req, res) {
       return;
     }
 
-    await deleteFilesFromS3(project.files);
-    await project.remove();
-    res.status(200).end();
-  } catch (error) {
-    sendFailure(error);
+    deleteFilesFromS3(project.files);
+
+    project.remove((removeProjectError) => {
+      if (removeProjectError) {
+        sendProjectNotFound();
+        return;
+      }
+
+      res.status(200).end();
+    });
   }
+
+  return Project.findById(req.params.project_id)
+    .then(handleProjectDeletion)
+    .catch(sendFailure);
 }