Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1. BIG server clean & API response schema standardization #266

Merged
merged 17 commits into from
Feb 8, 2019
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/server/api/auth/get-token-utln.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ const getTokenUtln = async (utln: string) => {
// If the token is invalid, it will get caught upstream in the `authorized`
// middleware. If it is valid, the request should include a `user` property
// including the user's utln, which we will return here.
return apiUtils.status(200).json({
status: codes.AUTHORIZED,
return apiUtils.status(codes.AUTHORIZED).data({
utln,
});
};
Expand Down
20 changes: 6 additions & 14 deletions src/server/api/auth/send-verification-email.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,7 @@ const sendVerificationEmail = async (utln: string, forceResend: boolean) => {
// respond that the email has already been sent
if (forceResend !== true && !oldCodeExpired) {
logger.info(`Already sent code: ${code.code}`);
return apiUtils.status(200).json({
status: codes.SEND_VERIFICATION_EMAIL__EMAIL_ALREADY_SENT,
return apiUtils.status(codes.SEND_VERIFICATION_EMAIL__EMAIL_ALREADY_SENT).data({
email: code.email,
});
}
Expand All @@ -67,31 +66,25 @@ const sendVerificationEmail = async (utln: string, forceResend: boolean) => {

// If the member info is null (not found), error that it was not found.
if (!memberInfo) {
return apiUtils.status(400).json({
status: codes.SEND_VERIFICATION_EMAIL__UTLN_NOT_FOUND,
});
return apiUtils.status(codes.SEND_VERIFICATION_EMAIL__UTLN_NOT_FOUND).noData();
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: lots of reuse of .classyear and such, in general on these I think it would be a good idea to use destructure {classyear} = memberinfo in these

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree...I think that's a separate PR. I started doing that for some util functions too and I'd like to do a full sweep

// Ensure the member is a student
if (!memberInfo.classYear) {
return apiUtils.status(400).json({
status: codes.SEND_VERIFICATION_EMAIL__UTLN_NOT_STUDENT,
});
return apiUtils.status(codes.SEND_VERIFICATION_EMAIL__UTLN_NOT_STUDENT).noData();
}

// Check that the student is in A&S or E
if (!_.includes(['A&S', 'E'], memberInfo.college)) {
return apiUtils.status(400).json({
status: codes.SEND_VERIFICATION_EMAIL__UTLN_NOT_UNDERGRAD,
return apiUtils.status(codes.SEND_VERIFICATION_EMAIL__UTLN_NOT_UNDERGRAD).data({
college: memberInfo.college,
classYear: memberInfo.classYear,
});
}

// Ensure user is in the Class of 2019
if (memberInfo.classYear !== '19') {
return apiUtils.status(400).json({
status: codes.SEND_VERIFICATION_EMAIL__UTLN_NOT_2019,
return apiUtils.status(codes.SEND_VERIFICATION_EMAIL__UTLN_NOT_2019).data({
classYear: memberInfo.classYear,
});
}
Expand Down Expand Up @@ -132,8 +125,7 @@ const sendVerificationEmail = async (utln: string, forceResend: boolean) => {
slack.postVerificationCode(verificationCode, utln, memberInfo.email);

// Send a success response to the client
return apiUtils.status(200).json({
status: codes.SEND_VERIFICATION_EMAIL__SUCCESS,
return apiUtils.status(codes.SEND_VERIFICATION_EMAIL__SUCCESS).data({
email: memberInfo.email,
});
};
Expand Down
15 changes: 4 additions & 11 deletions src/server/api/auth/verify.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,26 +42,20 @@ const verify = async (utln: string, code: number) => {

// No email has been sent for this utln
if (result.rowCount === 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more of a syntac standardization idea, but I was thinking we could do
result.rowCount === 0 && return apiUtils...
for these kinda things? It would really clean up lots of code that's lots of conditional chaining

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that cleaner in general? I guess I don't see that as more readable

return apiUtils.status(401).json({
status: codes.VERIFY__NO_EMAIL_SENT,
});
return apiUtils.status(codes.VERIFY__NO_EMAIL_SENT).noData();
}

const verification = result.rows[0];
const expired = new Date(verification.expiration).getTime() < new Date().getTime();

// Check if the code is expired
if (verification.attempts > 3 || expired) {
return apiUtils.status(400).json({
status: codes.VERIFY__EXPIRED_CODE,
});
return apiUtils.status(codes.VERIFY__EXPIRED_CODE).noData();
}

// Check if the code is valid. If not, send a bad code message
if (verification.code !== code) {
return apiUtils.status(400).json({
status: codes.VERIFY__BAD_CODE,
});
return apiUtils.status(codes.VERIFY__BAD_CODE).noData();
}

// Success! The code is verified!
Expand Down Expand Up @@ -97,8 +91,7 @@ const verify = async (utln: string, code: number) => {
});

// Send the response back!
return apiUtils.status(200).json({
status: codes.VERIFY__SUCCESS,
return apiUtils.status(codes.VERIFY__SUCCESS).data({
token,
});
};
Expand Down
46 changes: 26 additions & 20 deletions src/server/api/photos/confirm-upload.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,7 @@ const confirmUpload = async (userId: number) => {
`, [userId]);

if (unconfirmedPhotoRes.rowCount === 0) {
return apiUtils.status(400).json({
status: codes.CONFIRM_UPLOAD__NO_UNCONFIRMED_PHOTO,
});
return apiUtils.status(codes.CONFIRM_UPLOAD__NO_UNCONFIRMED_PHOTO).noData();
}

const [{ uuid }] = unconfirmedPhotoRes.rows;
Expand All @@ -47,9 +45,7 @@ const confirmUpload = async (userId: number) => {
try {
await s3.headObject(s3Params).promise();
} catch (error) {
return apiUtils.status(400).json({
status: codes.CONFIRM_UPLOAD__NO_UPLOAD_FOUND,
});
return apiUtils.status(codes.CONFIRM_UPLOAD__NO_UPLOAD_FOUND).noData();
}

// TRANSACTION: Insert the photo and delete its "unconfirmed" counterpart
Expand All @@ -71,20 +67,33 @@ const confirmUpload = async (userId: number) => {
// Ensure there are only 3 or fewer photos
const [{ photoCount }] = photosRes.rows;
if (photoCount > 3) {
return apiUtils.status(400).json({
status: codes.CONFIRM_UPLOAD__NO_AVAILABLE_SLOT,
});
return apiUtils.status(codes.CONFIRM_UPLOAD__NO_AVAILABLE_SLOT).noData();
}

// Insert the photo in the `photos` table, giving it the "next" index.
const insertRes = await client.query(`
INSERT INTO photos
(user_id, index, uuid)
VALUES ($1, $2, $3)
RETURNING id
`, [userId, photoCount + 1, uuid]);

const photoId = insertRes.rows[0].id;
WITH inserted AS (
INSERT INTO photos
(user_id, index, uuid)
VALUES ($1, $2, $3)
RETURNING id
)
SELECT
array_cat(
ARRAY(
SELECT id
FROM photos
WHERE user_id = $4
ORDER BY index
),
ARRAY(
SELECT id
FROM inserted
)
) AS "photoIds"
`, [userId, photoCount + 1, uuid, userId]);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return the new photoIds!


const [{ photoIds }] = insertRes.rows;

// Delete the unconfirmed photo
await client.query(`
Expand All @@ -96,10 +105,7 @@ const confirmUpload = async (userId: number) => {
await client.query('COMMIT');
client.release();

return apiUtils.status(200).json({
status: codes.CONFIRM_UPLOAD__SUCCESS,
photoId,
});
return apiUtils.status(codes.CONFIRM_UPLOAD__SUCCESS).data(photoIds);
} catch (err) {
// Rollback the transaction and release the client
await client.query('ROLLBACK');
Expand Down
65 changes: 21 additions & 44 deletions src/server/api/photos/delete-photo.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const bucket = config.get('s3_bucket');
* @api {delete} /api/photos/:photoId
*
*/
const deletePhoto = async (photoId: number, userId: number, userHasProfile: boolean) => {
const deletePhoto = async (photoId: number, userId: number) => {
// On error, return a server error.
const photosRes = await db.query(`
SELECT id, uuid, index
Expand All @@ -34,15 +34,7 @@ const deletePhoto = async (photoId: number, userId: number, userHasProfile: bool
const photos = photosRes.rows;
const [photoToDelete] = _.remove(photos, photo => photo.id === photoId);
if (photoToDelete === undefined) {
return apiUtils.status(400).json({
status: codes.DELETE_PHOTO__NOT_FOUND,
});
}

if (photos.length === 0) {
return apiUtils.status(409).json({
status: codes.DELETE_PHOTO__CANNOT_DELETE_LAST_PHOTO,
});
return apiUtils.status(codes.DELETE_PHOTO__NOT_FOUND).noData();
}
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes the server check that it is a user's last photo.


// Transaction to delete the photo:
Expand All @@ -51,37 +43,30 @@ const deletePhoto = async (photoId: number, userId: number, userHasProfile: bool
// 0. Begin the transaction
await client.query('BEGIN');

// If we are deleting the splash photo, update the user's splash photo
// Only do this if the user already has a profile
if (userHasProfile && photoToDelete.index === 1) {
await client.query(`
UPDATE profiles
SET splash_photo_id = $1
WHERE user_id = $2
`, [photos[0].id, userId]);
}
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete "splash photo id"


// 1. Remove the photo from our database
await client.query(`
DELETE FROM photos
WHERE id = $1
WHERE
id = $1
`, [photoToDelete.id]);

// Get an updated list of photos for the requesting user
const updatedPhotos = _.map(photos, (photo, index) => {
return `(${photo.id}, ${index + 1})`;
});

// 2. Update the photos for the requesting user
await client.query(`
UPDATE photos
SET index = updated_photos.index
FROM
(VALUES
${updatedPhotos.join(',')}
) AS updated_photos (id, index)
WHERE photos.id = updated_photos.id
`);
// 2. Update the photos for the requesting user. Only do if some photos exist
if (photos.length > 0) {
await client.query(`
UPDATE photos
SET index = updated_photos.index
FROM
(VALUES
${updatedPhotos.join(',')}
) AS updated_photos (id, index)
WHERE photos.id = updated_photos.id
`);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to reorder the photos if there are no photos! This just wraps the old query in an if statement

}

// 3. Delete the photo from S3
const params = {
Expand All @@ -92,29 +77,21 @@ const deletePhoto = async (photoId: number, userId: number, userHasProfile: bool

// 4. Commit the transaction!
await client.query('COMMIT');

return apiUtils.status(codes.DELETE_PHOTO__SUCCESS).data(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this happen in the finally? It's dangerous to put anything after the s3Delete, because theoretically any additional code could trigger the rollback, but s3 would have been changed.
Actually, for this reason I think step 3 and 4 should also be switched.

_.map(photos, photo => photo.id),
);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return the new photo order

} catch (err) {
await client.query('ROLLBACK');
throw err;
} finally {
client.release();
}

const newOrderRes = await db.query(`
SELECT id
FROM photos
WHERE user_id = $1
ORDER BY index
`, [userId]);

return apiUtils.status(200).json({
status: codes.DELETE_PHOTO__SUCCESS,
photos: _.map(newOrderRes.rows, row => row.id),
});
};

const handler = [
apiUtils.asyncHandler(async (req: $Request) => {
return deletePhoto(Number.parseInt(req.params.photoId, 10), req.user.id, req.user.hasProfile);
return deletePhoto(Number.parseInt(req.params.photoId, 10), req.user.id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the Number.parseInt should be abstracted to a photoUtils function if we pass the same constant base 10 to it every time.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Params.photoId comes in as a string. I am about to refactor this endpoint to be MUCH simpler using a new way of ordering photos using a trigger. This will remove the need for the photoId to come in as a number and instead allow it to come in as a string. Therefore, I'd prefer not to add a utility here as it will soon be removed

}),
];

Expand Down
25 changes: 16 additions & 9 deletions src/server/api/photos/get-photo.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const NODE_ENV = serverUtils.getNodeEnv();
const s3 = new aws.S3({ region: 'us-east-1', signatureVersion: 'v4' });
const bucket = config.get('s3_bucket');

const getSignedUrl = async (params) => {
const getSignedUrl = async (params): Promise<string> => {
return new Promise((resolve, reject) => {
s3.getSignedUrl('getObject', params, (err, url) => {
if (err) return reject(err);
Expand All @@ -38,9 +38,11 @@ const getPhoto = async (photoId: number) => {

// If it does not exist, error.
if (photoRes.rowCount === 0) {
return apiUtils.status(400).json({
status: codes.GET_PHOTO__NOT_FOUND,
});
// Weird flowtype issue requires us to specifically define return type
// Same bug as https://github.com/facebook/flow/issues/5294. Not resolve.
// Should look into this more: Max made a trello ticket 2/4/19
// $FlowFixMe
return apiUtils.status(codes.GET_PHOTO__NOT_FOUND).noData();
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird...I'm not worried about it and I'm pretty sure it's flow getting confused. Will look into it more

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spent a little time debugging this. I think the root of this is the implicit types returned by functions; feels like it should work, perhaps related to:
facebook/flow#5294

I tested out a solution there by explicitly typing the return type (which is safe, as flow does make sure it's a valid return type).

    const foo: { body: { status: string }, statusCode: number } = apiUtils
      .status(codes.GET_PHOTO__NOT_FOUND)
      .noData();
    return foo;

Worth looking into later the deeper cause of this, but how about for now at least keeping this with a TODO comment so that the export here is well typed

}

// Sign a url for the photo and redirect the request to it
Expand All @@ -49,9 +51,9 @@ const getPhoto = async (photoId: number) => {
Bucket: bucket,
Key: `photos/${NODE_ENV}/${uuid}`,
};

const url = await getSignedUrl(params);
return apiUtils.status(200).json({
status: codes.GET_PHOTO__SUCCESS,
return apiUtils.status(codes.GET_PHOTO__SUCCESS).data({
url,
});
};
Expand All @@ -62,10 +64,15 @@ const handler = [
async (req: $Request, res: $Response, next: $Next) => {
try {
const photoRes = await getPhoto(req.params.photoId);
if (photoRes.body.status === codes.GET_PHOTO__SUCCESS) {
return res.redirect(photoRes.body.url);

// If the photo was succesfully retrieved, redirect to it!
// NOTE: This is "abnormal" by the standards of the API
if (photoRes.body.status === codes.GET_PHOTO__SUCCESS.status) {
return res.redirect(photoRes.body.data.url);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This handles the redirect. Internally, we still have a GET_PHOTO__SUCCCES response

}
return res.status(photoRes.status).json(photoRes.body);

// On failure, respond 'normally'
return res.status(photoRes.statusCode).json(photoRes.body);
} catch (err) {
return next(err);
}
Expand Down
22 changes: 4 additions & 18 deletions src/server/api/photos/reorder-photos.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const schema = {
* @api {patch} /api/photos/reorder
*
*/
const reorderPhotos = async (newOrder: number[], userId: number, userHasProfile: boolean) => {
const reorderPhotos = async (newOrder: number[], userId: number) => {
// No worry about SQL Injection here: newOrder is verified to be an
// array of integers/numbers.
const result = await db.query(`
Expand All @@ -41,9 +41,7 @@ const reorderPhotos = async (newOrder: number[], userId: number, userHasProfile:

// If there are photo id mismatches, error
if (mismatchCount > 0) {
return apiUtils.status(400).json({
status: codes.REORDER_PHOTOS__MISMATCHED_IDS,
});
return apiUtils.status(codes.REORDER_PHOTOS__MISMATCHED_IDS).noData();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we return or log the mismatches?
edit: made a task

}

// Get an updated list of photos for the requesting user
Expand All @@ -62,25 +60,13 @@ const reorderPhotos = async (newOrder: number[], userId: number, userHasProfile:
WHERE photos.id = updated_photos.id
`);

// If the user has a profile, set the new first photo to be the splash photo
if (userHasProfile) {
await db.query(`
UPDATE profiles
SET splash_photo_id = $1
WHERE user_id = $2
`, [newOrder[0], userId]);
}

return apiUtils.status(200).json({
status: codes.REORDER_PHOTOS__SUCCESS,
photos: newOrder,
});
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete stuff about splash photo

return apiUtils.status(codes.REORDER_PHOTOS__SUCCESS).data(newOrder);
};

const handler = [
apiUtils.validate(schema),
apiUtils.asyncHandler(async (req: $Request) => {
return reorderPhotos(req.body, req.user.id, req.user.hasProfile);
return reorderPhotos(req.body, req.user.id);
}),
];

Expand Down
Loading