-
Notifications
You must be signed in to change notification settings - Fork 0
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
Changes from 10 commits
a4df509
ad8cc48
9b3bcca
cf84701
85e8682
fef8416
64d11c6
724393c
cb8a20c
2a72a7c
ca75126
c34fab6
573404e
b52a64b
b8d0d9a
8065887
e723a5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,26 +42,20 @@ const verify = async (utln: string, code: number) => { | |
|
||
// No email has been sent for this utln | ||
if (result.rowCount === 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! | ||
|
@@ -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, | ||
}); | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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 | ||
|
@@ -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]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(` | ||
|
@@ -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'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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(); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
@@ -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]); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = { | ||
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this happen in the |
||
_.map(photos, photo => photo.id), | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
}), | ||
]; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,9 +38,8 @@ 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, | ||
}); | ||
// $FlowFixMe I'm really not sure what's going on here...it's only HERE | ||
return apiUtils.status(codes.GET_PHOTO__NOT_FOUND).noData(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: 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).
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 | ||
|
@@ -49,9 +48,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, | ||
}); | ||
}; | ||
|
@@ -62,10 +61,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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(` | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we return or log the mismatches? |
||
} | ||
|
||
// Get an updated list of photos for the requesting user | ||
|
@@ -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, | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
}), | ||
]; | ||
|
||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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