-
Notifications
You must be signed in to change notification settings - Fork 30
Emily/20250104 handle membership payment confirmation #1993
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
Emily/20250104 handle membership payment confirmation #1993
Conversation
| REJECTED: 'rejected', | ||
| }; | ||
|
|
||
| export async function findPayment(confirmationCode) { |
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.
let's make it so that these functions are imported at the bottom using module.exports = { function1, function2, ... } and then the labels can be function instead of export function -> this pattern should be there in /api/main_endpoints/util/OfficeAccessCard.js as a template if you need
| let accessLevel; | ||
| let membershipValidUntil; | ||
|
|
||
| if (amount >= 30) { | ||
| accessLevel = membershipState.MEMBER; | ||
| membershipValidUntil = getMemberExpirationDate(2); | ||
| } else if (amount >= 20) { | ||
| accessLevel = membershipState.MEMBER; | ||
| membershipValidUntil = getMemberExpirationDate(1); |
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.
we can define const accessLevel = membershipState.MEMBER after this if/else block since it's always gonna be member, and the else clause returns early
| ).then(payment => resolve(payment)) | ||
| .catch(() => resolve(null)); |
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.
| ).then(payment => resolve(payment)) | |
| .catch(() => resolve(null)); | |
| , (error, result) => { | |
| if (error) // -> resolve null | |
| if (!result) // -> resolve false since we couldn't find the payment | |
| return resolve(result) // -> return the document since it's successful |
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.
in other words let's use the (error, result) => callback instead of .then() and .catch() , this pattern should be there in the OfficeAccessCard file
| function rejectPayment(paymentId) { | ||
| return new Promise((resolve) => { | ||
| try { | ||
| MembershipPayment.updateOne( | ||
| { _id: paymentId }, | ||
| { $set: { status: status.REJECTED } } | ||
| ).then(result => resolve(result)) | ||
| .catch(() => resolve(null)); | ||
| } catch (err) { | ||
| resolve(null); | ||
| } | ||
| }); | ||
| } | ||
|
|
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.
let's have this function return true if it's successful and false if it's unsuccessful. also, you can use Mongoose's findByIdAndUpdate() if you want to use paymentId to find the document here
| if (!paymentDocument) { | ||
| return res.sendStatus(NOT_FOUND); | ||
| } |
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.
see comment in the util file about findVerifyPayment() returning null or false. we need to check:
if paymentDocument === null -> there was a server error (500 error code)
if paymentDocument === false -> we couldn't find the document so return 404 not found
then if we pass those two checks we were successful in finding and updating
| const User = require('../models/User'); | ||
| const { getMemberExpirationDate } = require('../util/userHelpers'); | ||
| const { findVerifyPayment, rejectPayment } = require('../util/membershipPaymentQueries'); | ||
| const { decodeToken } = require('../../util/auth'); |
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.
| const { decodeToken } = require('../../util/auth'); | |
| const { decodeToken } = require('../util/token-functions.js'); |
| try { | ||
| const paymentDocument = await findVerifyPayment(confirmationCode, userId); | ||
| if (paymentDocument === null){ | ||
| throw new Error('Server error'); |
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.
| throw new Error('Server error'); | |
| return res.sendStatus(SERVER_ERROR); |
| function rejectPayment(paymentId) { | ||
| return new Promise((resolve) => { | ||
| MembershipPayment.findByIdAndUpdate( | ||
| paymentId, | ||
| { $set: { status: status.REJECTED } }, | ||
| (error, result) => { | ||
| if (error) { | ||
| return resolve(null); | ||
| } | ||
| if (!result) { | ||
| return resolve(false); | ||
| } | ||
| return resolve(true); | ||
| } | ||
| ); | ||
| }); | ||
| } | ||
|
|
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.
for both of these functions, let's make the body of the Promise be within a try/catch, where if we catch an error we return resolve(null)
| if (amount < 20){ | ||
| const rejected = await rejectPayment(paymentId); | ||
| if (rejected === null){ | ||
| throw new Error('Server error'); |
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.
| throw new Error('Server error'); | |
| return res.sendStatus(SERVER_ERROR); |
| } | ||
|
|
||
| try { | ||
| const paymentDocument = await findVerifyPayment(confirmationCode, userId); |
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.
we can remove the try catch from here, since it's already being try/catched in the util file
| if (amount < 20){ | ||
| const rejected = await rejectPayment(paymentId); | ||
| if (rejected === null){ | ||
| return res.sendStatus(NOT_FOUND); |
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.
| return res.sendStatus(NOT_FOUND); | |
| return res.sendStatus(SERVER_ERROR); |
adarshm11
left a comment
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.
excellent work
charred-70
left a comment
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.
lgtm Emily you're my hero
issue #1967