diff --git a/.github/workflows/deploy-staging.yml b/.github/workflows/deploy-staging.yml index 86389b27c9..f60eef8aa6 100644 --- a/.github/workflows/deploy-staging.yml +++ b/.github/workflows/deploy-staging.yml @@ -3,7 +3,7 @@ on: workflow_run: workflows: ["Test"] branches: - - release-delete-sketch + - develop types: - completed env: diff --git a/server/controllers/aws.controller.js b/server/controllers/aws.controller.js index 249dbd2310..00ef62a764 100644 --- a/server/controllers/aws.controller.js +++ b/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(); } } diff --git a/server/controllers/project.controller/__test__/deleteProject.test.js b/server/controllers/project.controller/__test__/deleteProject.test.js index fe812e4dd5..ec03debab9 100644 --- a/server/controllers/project.controller/__test__/deleteProject.test.js +++ b/server/controllers/project.controller/__test__/deleteProject.test.js @@ -3,7 +3,10 @@ */ 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'; @@ -11,71 +14,101 @@ 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); + }); }); }); diff --git a/server/controllers/project.controller/deleteProject.js b/server/controllers/project.controller/deleteProject.js index 1b747cc028..e3dace94c4 100644 --- a/server/controllers/project.controller/deleteProject.js +++ b/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); }