From f4a8abb0fa804d73b5d1fae1a1054113a3693ca4 Mon Sep 17 00:00:00 2001 From: raclim Date: Tue, 30 Apr 2024 18:00:40 -0400 Subject: [PATCH 1/5] update some methods in user model to async await --- server/models/user.js | 97 +++++++++++++++++++++++-------------------- 1 file changed, 51 insertions(+), 46 deletions(-) diff --git a/server/models/user.js b/server/models/user.js index 1c4704ec5e..c6fe0cfc4c 100644 --- a/server/models/user.js +++ b/server/models/user.js @@ -199,20 +199,17 @@ userSchema.methods.findMatchingKey = function findMatchingKey( * @callback [cb] - Optional error-first callback that passes User document * @return {Promise} - Returns Promise fulfilled by User document */ -userSchema.statics.findByEmail = function findByEmail(email, cb) { - let query; - if (Array.isArray(email)) { - query = { - email: { $in: email } - }; - } else { - query = { - email - }; - } +userSchema.statics.findByEmail = async function findByEmail(email) { + const user = this; + const query = Array.isArray(email) ? { email: { $in: email } } : { email }; + // Email addresses should be case-insensitive unique // In MongoDB, you must use collation in order to do a case-insensitive query - return this.findOne(query).collation({ locale: 'en', strength: 2 }).exec(cb); + const userFoundByEmail = await user + .findOne(query) + .collation({ locale: 'en', strength: 2 }) + .exec(); + return userFoundByEmail; }; /** @@ -220,16 +217,20 @@ userSchema.statics.findByEmail = function findByEmail(email, cb) { * Queries User collection by emails and returns all Users that match. * * @param {string[]} emails - Array of email strings - * @callback [cb] - Optional error-first callback that passes User document * @return {Promise} - Returns Promise fulfilled by User document */ -userSchema.statics.findAllByEmails = function findAllByEmails(emails, cb) { +userSchema.statics.findAllByEmails = async function findAllByEmails(emails) { + const user = this; const query = { email: { $in: emails } }; // Email addresses should be case-insensitive unique // In MongoDB, you must use collation in order to do a case-insensitive query - return this.find(query).collation({ locale: 'en', strength: 2 }).exec(cb); + const usersFoundByEmails = await user + .find(query) + .collation({ locale: 'en', strength: 2 }) + .exec(); + return usersFoundByEmails; }; /** @@ -239,29 +240,31 @@ userSchema.statics.findAllByEmails = function findAllByEmails(emails, cb) { * @param {string} username - Username string * @param {Object} [options] - Optional options * @param {boolean} options.caseInsensitive - Does a caseInsensitive query, defaults to false - * @callback [cb] - Optional error-first callback that passes User document * @return {Promise} - Returns Promise fulfilled by User document */ -userSchema.statics.findByUsername = function findByUsername( +userSchema.statics.findByUsername = async function findByUsername( username, - options, - cb + options ) { + const user = this; const query = { username }; + if ( - (arguments.length === 3 && options.caseInsensitive) || - (arguments.length === 2 && - typeof options === 'object' && - options.caseInsensitive) + arguments.length === 2 && + typeof options === 'object' && + options.caseInsensitive ) { - return this.findOne(query) + const foundUser = await user + .findOne(query) .collation({ locale: 'en', strength: 2 }) - .exec(cb); + .exec(); + return foundUser; } - const callback = typeof options === 'function' ? options : cb; - return this.findOne(query, callback); + + const userFoundByUsername = await user.findOne(query).exec(); + return userFoundByUsername; }; /** @@ -276,37 +279,39 @@ userSchema.statics.findByUsername = function findByUsername( * default query for username or email, defaults * to false * @param {("email"|"username")} options.valueType - Prevents automatic type inferrence - * @callback [cb] - Optional error-first callback that passes User document * @return {Promise} - Returns Promise fulfilled by User document */ -userSchema.statics.findByEmailOrUsername = function findByEmailOrUsername( +userSchema.statics.findByEmailOrUsername = async function findByEmailOrUsername( value, - options, - cb + options ) { - let isEmail; - if (options && options.valueType) { - isEmail = options.valueType === 'email'; - } else { - isEmail = value.indexOf('@') > -1; - } + const user = this; + const isEmail = + options && options.valueType + ? options.valueType === 'email' + : value.indexOf('@') > -1; + // do the case insensitive stuff if ( - (arguments.length === 3 && options.caseInsensitive) || - (arguments.length === 2 && - typeof options === 'object' && - options.caseInsensitive) + arguments.length === 2 && + typeof options === 'object' && + options.caseInsensitive ) { const query = isEmail ? { email: value } : { username: value }; - return this.findOne(query) + const foundUser = await user + .findOne(query) .collation({ locale: 'en', strength: 2 }) - .exec(cb); + .exec(); + + return foundUser; } - const callback = typeof options === 'function' ? options : cb; + if (isEmail) { - return this.findByEmail(value, callback); + const userFoundByEmail = await user.findByEmail(value); + return userFoundByEmail; } - return this.findByUsername(value, callback); + const userFoundByUsername = await user.findByUsername(value); + return userFoundByUsername; }; /** From 12adbd4b2ca2aa2b2f08d30a646f1ba2e2c23ec1 Mon Sep 17 00:00:00 2001 From: raclim Date: Tue, 30 Apr 2024 18:01:07 -0400 Subject: [PATCH 2/5] refactor passport to use async/await --- server/config/passport.js | 314 ++++++++++++++++++-------------------- 1 file changed, 152 insertions(+), 162 deletions(-) diff --git a/server/config/passport.js b/server/config/passport.js index 3ae3dfbe0b..da0abd4d28 100644 --- a/server/config/passport.js +++ b/server/config/passport.js @@ -70,30 +70,30 @@ passport.use( * Authentificate using Basic Auth (Username + Api Key) */ passport.use( - new BasicStrategy((userid, key, done) => { - User.findByUsername(userid, (err, user) => { - if (err) { - done(err); - return; - } + new BasicStrategy(async (userid, key, done) => { + try { + const user = await User.findByUsername(userid); + if (!user) { - done(null, false); - return; + return done(null, false); } + if (user.banned) { - done(null, false, { msg: accountSuspensionMessage }); - return; + return done(null, false, { msg: accountSuspensionMessage }); } - user.findMatchingKey(key, (innerErr, isMatch, keyDocument) => { - if (isMatch) { - keyDocument.lastUsedAt = Date.now(); - user.save(); - done(null, user); - return; - } - done(null, false, { msg: 'Invalid username or API key' }); - }); - }); + + const { isMatch, keyDocument } = await user.findMatchingKey(key); + if (!isMatch) { + return done(null, false, { message: 'Invalid API key' }); + } + + keyDocument.lastUsedAt = Date.now(); + await user.save(); + return done(null, user); + } catch (err) { + console.error(err); + return done(null, false, { msg: err }); + } }) ); @@ -128,20 +128,19 @@ passport.use( scope: ['user:email'], allRawEmails: true }, - (req, accessToken, refreshToken, profile, done) => { - User.findOne({ github: profile.id }, (findByGithubErr, existingUser) => { + async (req, accessToken, refreshToken, profile, done) => { + try { + const existingUser = await User.findOne({ github: profile.id }); + if (existingUser) { if (req.user && req.user.email !== existingUser.email) { - done(null, false, { + return done(null, false, { msg: 'GitHub account is already linked to another account.' }); - return; } else if (existingUser.banned) { - done(null, false, { msg: accountSuspensionMessage }); - return; + return done(null, false, { msg: accountSuspensionMessage }); } - done(null, existingUser); - return; + return done(null, existingUser); } const emails = getVerifiedEmails(profile.emails); @@ -153,58 +152,63 @@ passport.use( req.user.tokens.push({ kind: 'github', accessToken }); req.user.verified = User.EmailConfirmation.Verified; } - req.user.save((saveErr) => done(null, req.user)); - } else { - User.findAllByEmails(emails, (findByEmailErr, existingEmailUsers) => { - if (existingEmailUsers.length) { - let existingEmailUser; - // Handle case where user has made multiple p5.js Editor accounts, - // with emails that are connected to the same GitHub account - if (existingEmailUsers.length > 1) { - existingEmailUser = existingEmailUsers.find( - (u) => (u.email = primaryEmail) - ); - } else { - [existingEmailUser] = existingEmailUsers; - } - if (existingEmailUser.banned) { - done(null, false, { msg: accountSuspensionMessage }); - return; - } - existingEmailUser.email = existingEmailUser.email || primaryEmail; - existingEmailUser.github = profile.id; - existingEmailUser.username = - existingEmailUser.username || profile.username; - existingEmailUser.tokens.push({ kind: 'github', accessToken }); - existingEmailUser.name = - existingEmailUser.name || profile.displayName; - existingEmailUser.verified = User.EmailConfirmation.Verified; - existingEmailUser.save((saveErr) => - done(null, existingEmailUser) - ); - } else { - let { username } = profile; - User.findByUsername( - username, - { caseInsensitive: true }, - (findByUsernameErr, existingUsernameUser) => { - if (existingUsernameUser) { - username = generateUniqueUsername(username); - } - const user = new User(); - user.email = primaryEmail; - user.github = profile.id; - user.username = profile.username; - user.tokens.push({ kind: 'github', accessToken }); - user.name = profile.displayName; - user.verified = User.EmailConfirmation.Verified; - user.save((saveErr) => done(null, user)); - } - ); - } - }); + req.user.save(); + return done(null, req.user); + } + + const existingEmailUsers = await User.findAllByEmails(emails); + + if (existingEmailUsers.length) { + let existingEmailUser; + + // Handle case where user has made multiple p5.js Editor accounts, + // with emails that are connected to the same GitHub account + if (existingEmailUsers.length > 1) { + existingEmailUser = existingEmailUsers.find( + (u) => (u.email = primaryEmail) + ); + } else { + [existingEmailUser] = existingEmailUsers; + } + + if (existingEmailUser.banned) { + return done(null, false, { msg: accountSuspensionMessage }); + } + existingEmailUser.email = existingEmailUser.email || primaryEmail; + existingEmailUser.github = profile.id; + existingEmailUser.username = + existingEmailUser.username || profile.username; + existingEmailUser.tokens.push({ kind: 'github', accessToken }); + existingEmailUser.name = + existingEmailUser.name || profile.displayName; + existingEmailUser.verified = User.EmailConfirmation.Verified; + existingEmailUser.save(); + return done(null, existingEmailUser); } - }); + + let { username } = profile; + + const existingUsernameUser = await User.findByUsername(username, { + caseInsensitive: true + }); + + if (existingUsernameUser) { + username = generateUniqueUsername(username); + } + const user = new User(); + user.email = primaryEmail; + user.github = profile.id; + user.username = profile.username; + user.tokens.push({ kind: 'github', accessToken }); + user.name = profile.displayName; + user.verified = User.EmailConfirmation.Verified; + await user.save(); + + return done(null, user); + } catch (err) { + console.error(err); + return done(null, false, { msg: err }); + } } ) ); @@ -221,92 +225,78 @@ passport.use( passReqToCallback: true, scope: ['openid email'] }, - (req, accessToken, refreshToken, profile, done) => { - User.findOne( - { google: profile._json.emails[0].value }, - (findByGoogleErr, existingUser) => { - if (existingUser) { - if (req.user && req.user.email !== existingUser.email) { - done(null, false, { - msg: 'Google account is already linked to another account.' - }); - return; - } else if (existingUser.banned) { - done(null, false, { msg: accountSuspensionMessage }); - return; - } - done(null, existingUser); - return; + async (req, accessToken, refreshToken, profile, done) => { + try { + const existingUser = await User.findOne({ + google: profile._json.emails[0].value + }); + + if (existingUser) { + if (req.user && req.user.email !== existingUser.email) { + return done(null, false, { + msg: 'Google account is already linked to another account.' + }); + } else if (existingUser.banned) { + return done(null, false, { msg: accountSuspensionMessage }); } - const primaryEmail = profile._json.emails[0].value; - - if (req.user) { - if (!req.user.google) { - req.user.google = profile._json.emails[0].value; - req.user.tokens.push({ kind: 'google', accessToken }); - req.user.verified = User.EmailConfirmation.Verified; - } - req.user.save((saveErr) => done(null, req.user)); - } else { - User.findByEmail( - primaryEmail, - (findByEmailErr, existingEmailUser) => { - let username = profile._json.emails[0].value.split('@')[0]; - User.findByUsername( - username, - { caseInsensitive: true }, - (findByUsernameErr, existingUsernameUser) => { - if (existingUsernameUser) { - username = generateUniqueUsername(username); - } - // what if a username is already taken from the display name too? - // then, append a random friendly word? - if (existingEmailUser) { - if (existingEmailUser.banned) { - done(null, false, { msg: accountSuspensionMessage }); - return; - } - existingEmailUser.email = - existingEmailUser.email || primaryEmail; - existingEmailUser.google = profile._json.emails[0].value; - existingEmailUser.username = - existingEmailUser.username || username; - existingEmailUser.tokens.push({ - kind: 'google', - accessToken - }); - existingEmailUser.name = - existingEmailUser.name || profile._json.displayName; - existingEmailUser.verified = - User.EmailConfirmation.Verified; - existingEmailUser.save((saveErr) => { - if (saveErr) { - console.log(saveErr); - } - done(null, existingEmailUser); - }); - } else { - const user = new User(); - user.email = primaryEmail; - user.google = profile._json.emails[0].value; - user.username = username; - user.tokens.push({ kind: 'google', accessToken }); - user.name = profile._json.displayName; - user.verified = User.EmailConfirmation.Verified; - user.save((saveErr) => { - if (saveErr) { - console.log(saveErr); - } - done(null, user); - }); - } - } - ); - } - ); + return done(null, existingUser); + } + + const primaryEmail = profile._json.emails[0].value; + + if (req.user) { + if (!req.user.google) { + req.user.google = profile._json.emails[0].value; + req.user.tokens.push({ kind: 'google', accessToken }); + req.user.verified = User.EmailConfirmation.Verified; } + req.user.save(); + return done(null, req.user); + } + let username = profile._json.emails[0].value.split('@')[0]; + const existingEmailUser = await User.findByEmail(primaryEmail); + const existingUsernameUser = await User.findByUsername(username, { + caseInsensitive: true + }); + + if (existingUsernameUser) { + username = generateUniqueUsername(username); + } + // what if a username is already taken from the display name too? + // then, append a random friendly word? + if (existingEmailUser) { + if (existingEmailUser.banned) { + return done(null, false, { msg: accountSuspensionMessage }); + } + existingEmailUser.email = existingEmailUser.email || primaryEmail; + existingEmailUser.google = profile._json.emails[0].value; + existingEmailUser.username = existingEmailUser.username || username; + existingEmailUser.tokens.push({ + kind: 'google', + accessToken + }); + existingEmailUser.name = + existingEmailUser.name || profile._json.displayName; + existingEmailUser.verified = User.EmailConfirmation.Verified; + + await existingEmailUser.save(); + return done(null, existingEmailUser); } - ); + + const user = new User(); + user.email = primaryEmail; + user.google = profile._json.emails[0].value; + user.username = username; + user.tokens.push({ kind: 'google', accessToken }); + user.name = profile._json.displayName; + user.verified = User.EmailConfirmation.Verified; + + await user.save(); + return done(null, user); + } catch (err) { + console.error(err); + return done(null, false, { msg: err }); + } } ) ); From f48253d55152e37cc69e925d6ee29f4c3f8105e8 Mon Sep 17 00:00:00 2001 From: raclim Date: Tue, 30 Apr 2024 18:02:24 -0400 Subject: [PATCH 3/5] update errors for aws controller --- server/controllers/aws.controller.js | 80 ++++++++++++++-------------- 1 file changed, 41 insertions(+), 39 deletions(-) diff --git a/server/controllers/aws.controller.js b/server/controllers/aws.controller.js index 00ef62a764..d36c583bc1 100644 --- a/server/controllers/aws.controller.js +++ b/server/controllers/aws.controller.js @@ -39,8 +39,8 @@ export function getObjectKey(url) { } return objectKey; } -// TODO: callbacks should be removed from deleteObjectsFromS3 and deleteObjectFromS3 -export async function deleteObjectsFromS3(keyList, callback) { + +export async function deleteObjectsFromS3(keyList) { const objectsToDelete = keyList?.map((key) => ({ Key: key })); if (objectsToDelete.length > 0) { @@ -51,33 +51,27 @@ export async function deleteObjectsFromS3(keyList, callback) { try { await s3Client.send(new DeleteObjectsCommand(params)); - if (callback) { - callback(); - } } catch (error) { - console.error('Error deleting objects from S3: ', error); - if (callback) { - callback(error); + if (error.name === 'NotFound') { + console.log('Object does not exist:', error); + } else { + console.error('Error deleting objects from S3: ', error); } } - } else if (callback) { - callback(); } } -export function deleteObjectFromS3(req, res) { +export async function deleteObjectFromS3(req, res) { const { objectKey, userId } = req.params; const fullObjectKey = userId ? `${userId}/${objectKey}` : objectKey; - deleteObjectsFromS3([fullObjectKey], (error, result) => { - if (error) { - return res - .status(500) - .json({ error: 'Failed to delete object from s3.' }); - } + try { + await deleteObjectsFromS3([fullObjectKey]); return res.json({ success: true, message: 'Object deleted successfully.' }); - }); + } catch (err) { + return res.status(500).json({ error: 'Failed to delete object from s3.' }); + } } export function signS3(req, res) { @@ -116,8 +110,12 @@ export async function copyObjectInS3(url, userId) { try { await s3Client.send(new HeadObjectCommand(headParams)); } catch (error) { - console.error('Error fetching object metadata: ', error); - throw error; + if (error.name === 'NotFound') { + console.log('Object does not exist:', error); + } else { + console.error('Error fetching object metadata:', error); + throw error; + } } const params = { @@ -131,7 +129,12 @@ export async function copyObjectInS3(url, userId) { await s3Client.send(new CopyObjectCommand(params)); return `${s3Bucket}${userId}/${newFilename}`; } catch (error) { - console.error('Error copying object in S3:', error); + // temporary error handling for sketches with missing assets + if (error.startsWith('TypeError')) { + console.log('Object does not exist:', error); + return null; + } + console.error('Error copying object:', error); throw error; } } @@ -224,31 +227,30 @@ export async function listObjectsInS3ForUser(userId) { return { assets: projectAssets, totalSize }; } catch (error) { - console.log('Got an error:', error); + if (error.name === 'NotFound') { + console.log('Object does not exist:', error); + return null; + } + console.error('Got an error: ', error); throw error; } } -export function listObjectsInS3ForUserRequestHandler(req, res) { +export async function listObjectsInS3ForUserRequestHandler(req, res) { const { username } = req.user; - User.findByUsername(username, (err, user) => { - if (err) { - console.error('Error fetching user:', err.message); - res.status(500).json({ error: 'Failed to fetch user' }); - return; - } + + try { + const user = await User.findByUsername(username); + if (!user) { res.status(404).json({ error: 'User not found' }); return; } - const userId = user.id; - listObjectsInS3ForUser(userId) - .then((objects) => { - res.json(objects); - }) - .catch((error) => { - console.error('Error listing objects in S3:', error.message); - res.status(500).json({ error: 'Failed to list objects in S3' }); - }); - }); + + const objects = await listObjectsInS3ForUser(user.id); + res.json(objects); + } catch (error) { + console.error('Error listing objects in S3:', error.message); + res.status(500).json({ error: 'Failed to list objects in S3' }); + } } From f93132e37ed72cc7278009ca952713cca8654668 Mon Sep 17 00:00:00 2001 From: raclim Date: Tue, 30 Apr 2024 18:02:49 -0400 Subject: [PATCH 4/5] update tests for getProjectsForUser to async/await --- .../__test__/getProjectsForUser.test.js | 116 ++++++------------ 1 file changed, 36 insertions(+), 80 deletions(-) diff --git a/server/controllers/project.controller/__test__/getProjectsForUser.test.js b/server/controllers/project.controller/__test__/getProjectsForUser.test.js index 0160cb1477..9fb4e63f53 100644 --- a/server/controllers/project.controller/__test__/getProjectsForUser.test.js +++ b/server/controllers/project.controller/__test__/getProjectsForUser.test.js @@ -3,7 +3,7 @@ */ import { Request, Response } from 'jest-express'; -import { createMock } from '../../../models/user'; +import User from '../../../models/user'; import getProjectsForUser, { apiGetProjectsForUser } from '../../project.controller/getProjectsForUser'; @@ -12,14 +12,8 @@ jest.mock('../../../models/user'); jest.mock('../../aws.controller'); describe('project.controller', () => { - let UserMock; - beforeEach(() => { - UserMock = createMock(); - }); - - afterEach(() => { - UserMock.restore(); + User.findByUsername = jest.fn(); }); describe('getProjectsForUser()', () => { @@ -42,117 +36,79 @@ describe('project.controller', () => { promise.then(expectations, expectations).catch(expectations); }); - it('returns 404 if user does not exist', (done) => { + it('returns 404 if user does not exist', async () => { const request = new Request(); request.setParams({ username: 'abc123' }); const response = new Response(); - UserMock.expects('findOne') - .withArgs({ username: 'abc123' }) - .resolves(null); - - const promise = getProjectsForUser(request, response); - - function expectations() { - expect(response.status).toHaveBeenCalledWith(404); - expect(response.json).toHaveBeenCalledWith({ - message: 'User with that username does not exist.' - }); + await User.findByUsername.mockResolvedValue(null); - done(); - } + await getProjectsForUser(request, response); - promise.then(expectations, expectations).catch(expectations); + expect(response.status).toHaveBeenCalledWith(404); + expect(response.json).toHaveBeenCalledWith({ + message: 'User with that username does not exist.' + }); }); - it('returns 500 on other errors', (done) => { + it('returns 500 on other errors', async () => { const request = new Request(); request.setParams({ username: 'abc123' }); const response = new Response(); - UserMock.expects('findOne') - .withArgs({ username: 'abc123' }) - .rejects(new Error()); + await User.findByUsername.mockRejectedValue(new Error()); - const promise = getProjectsForUser(request, response); - - function expectations() { - expect(response.json).toHaveBeenCalledWith({ - message: 'Error fetching projects' - }); - expect(response.status).toHaveBeenCalledWith(500); - - done(); - } + await getProjectsForUser(request, response); - promise.then(expectations, expectations).catch(expectations); + expect(response.json).toHaveBeenCalledWith({ + message: 'Error fetching projects' + }); + expect(response.status).toHaveBeenCalledWith(500); }); }); describe('apiGetProjectsForUser()', () => { - it('returns validation error if username not provided', (done) => { + it('returns validation error if username not provided', async () => { const request = new Request(); request.setParams({}); const response = new Response(); - const promise = apiGetProjectsForUser(request, response); - - function expectations() { - expect(response.status).toHaveBeenCalledWith(422); - expect(response.json).toHaveBeenCalledWith({ - message: 'Username not provided' - }); - - done(); - } + await apiGetProjectsForUser(request, response); - promise.then(expectations, expectations).catch(expectations); + expect(response.status).toHaveBeenCalledWith(422); + expect(response.json).toHaveBeenCalledWith({ + message: 'Username not provided' + }); }); - it('returns 404 if user does not exist', (done) => { + it('returns 404 if user does not exist', async () => { const request = new Request(); request.setParams({ username: 'abc123' }); const response = new Response(); - UserMock.expects('findOne') - .withArgs({ username: 'abc123' }) - .resolves(null); - - const promise = apiGetProjectsForUser(request, response); - - function expectations() { - expect(response.status).toHaveBeenCalledWith(404); - expect(response.json).toHaveBeenCalledWith({ - message: 'User with that username does not exist.' - }); + await User.findByUsername.mockResolvedValue(null); - done(); - } + await apiGetProjectsForUser(request, response); - promise.then(expectations, expectations).catch(expectations); + expect(response.status).toHaveBeenCalledWith(404); + expect(response.json).toHaveBeenCalledWith({ + message: 'User with that username does not exist.' + }); }); - it('returns 500 on other errors', (done) => { + it('returns 500 on other errors', async () => { const request = new Request(); request.setParams({ username: 'abc123' }); const response = new Response(); - UserMock.expects('findOne') - .withArgs({ username: 'abc123' }) - .rejects(new Error()); - - const promise = apiGetProjectsForUser(request, response); - - function expectations() { - expect(response.status).toHaveBeenCalledWith(500); - expect(response.json).toHaveBeenCalledWith({ - message: 'Error fetching projects' - }); + await User.findByUsername.mockRejectedValue(new Error()); - done(); - } + await apiGetProjectsForUser(request, response); - promise.then(expectations, expectations).catch(expectations); + expect(response.status).toHaveBeenCalledWith(500); + expect(response.json).toHaveBeenCalledWith({ + message: 'Error fetching projects' + }); }); }); }); From 801cbb13b467312e7cd229192002e9062fcc2916 Mon Sep 17 00:00:00 2001 From: raclim Date: Tue, 30 Apr 2024 18:03:36 -0400 Subject: [PATCH 5/5] reimplement refactor for deleteProjects --- .../__test__/deleteProject.test.js | 129 +++++++----------- .../project.controller/deleteProject.js | 63 ++++----- 2 files changed, 76 insertions(+), 116 deletions(-) diff --git a/server/controllers/project.controller/__test__/deleteProject.test.js b/server/controllers/project.controller/__test__/deleteProject.test.js index ec03debab9..fe812e4dd5 100644 --- a/server/controllers/project.controller/__test__/deleteProject.test.js +++ b/server/controllers/project.controller/__test__/deleteProject.test.js @@ -3,10 +3,7 @@ */ 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'; @@ -14,101 +11,71 @@ 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(deleteObjectsFromS3).toHaveBeenCalled(); + expect(project.remove).toHaveBeenCalled(); + expect(response.status).toHaveBeenCalledWith(200); }); }); diff --git a/server/controllers/project.controller/deleteProject.js b/server/controllers/project.controller/deleteProject.js index e3dace94c4..cd4bbff6f4 100644 --- a/server/controllers/project.controller/deleteProject.js +++ b/server/controllers/project.controller/deleteProject.js @@ -7,40 +7,42 @@ const ProjectDeletionError = createApplicationErrorClass( 'ProjectDeletionError' ); -function deleteFilesFromS3(files) { +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)) && - !process.env.S3_DATE) || - (process.env.S3_DATE && - isBefore(new Date(process.env.S3_DATE), new Date(file.createdAt))); + file.url && + (file.url.includes(process.env.S3_BUCKET_URL_BASE) || + 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)); - deleteObjectsFromS3(filteredFiles); + 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; } @@ -54,19 +56,10 @@ export default function deleteProject(req, res) { return; } - deleteFilesFromS3(project.files); - - project.remove((removeProjectError) => { - if (removeProjectError) { - sendProjectNotFound(); - return; - } - - res.status(200).end(); - }); + await deleteFilesFromS3(project.files); + await project.remove(); + res.status(200).end(); + } catch (error) { + sendFailure(error); } - - return Project.findById(req.params.project_id) - .then(handleProjectDeletion) - .catch(sendFailure); }