-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: [IOBP-838] Remove receipt from list endpoint #430
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #430 +/- ##
==========================================
- Coverage 64.28% 64.24% -0.04%
==========================================
Files 167 167
Lines 5364 5372 +8
Branches 731 731
==========================================
+ Hits 3448 3451 +3
- Misses 1915 1920 +5
Partials 1 1
Continue to review full report in Codecov by Sentry.
|
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.
Great work! Just a small note, and it’s good to go from my side.
addNoticesHandler("post", "/paids/:eventId/disable", (req, res) => { | ||
pipe( | ||
req.params.eventId, | ||
O.fromNullable, | ||
O.fold( | ||
() => res.sendStatus(400), | ||
eventId => | ||
pipe( | ||
O.fromNullable(NoticesDB.removeUserNotice(eventId)), | ||
O.fold( | ||
() => res.sendStatus(hideReceiptResponseCode), | ||
() => res.sendStatus(hideReceiptResponseCode) | ||
) | ||
) | ||
) | ||
); | ||
}); |
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.
Since we don't really need two nested fold
that have the same behavior, what do you think of replacing with the following suggestion?
addNoticesHandler("post", "/paids/:eventId/disable", (req, res) => { | |
pipe( | |
req.params.eventId, | |
O.fromNullable, | |
O.fold( | |
() => res.sendStatus(400), | |
eventId => | |
pipe( | |
O.fromNullable(NoticesDB.removeUserNotice(eventId)), | |
O.fold( | |
() => res.sendStatus(hideReceiptResponseCode), | |
() => res.sendStatus(hideReceiptResponseCode) | |
) | |
) | |
) | |
); | |
}); | |
addNoticesHandler("post", "/paids/:eventId/disable", (req, res) => { | |
pipe( | |
req.params.eventId, | |
O.fromNullable, | |
O.fold( | |
() => res.sendStatus(400), | |
eventId => { | |
NoticesDB.removeUserNotice(eventId); | |
return res.sendStatus(hideReceiptResponseCode); | |
} | |
) | |
); | |
}); |
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!
Short description
Include a summary of the changes.
This pull request includes changes to the payment route to support a new endpoint for disabling user notices.
List of changes proposed in this pull request
paids/:eventId/disable
routehideReceiptResponseCode
field