Conversation
There was a problem hiding this comment.
last few nits and a note of consideration @sam-schu @Yurika-Kan : the order create method does 4 db operations across 4 different tables but if one of the steps fails then the db could reflect a partially updated state (e.g. reservedQuantities aren't set but orders are made, or donations aren't matched). i looked into it and we could maybe wrap this in a transaction so that all operations succeed or are rolled back/fail. thoughts?
sam-schu
left a comment
sam-schu
left a comment
There was a problem hiding this comment.
Thanks for making separate service functions to better organize the logic not directly related to orders! What was the rationale for making endpoints corresponding to all of those service functions?
Also, agreeing with @amywng that using a transaction to make the database updates all-or-nothing is a great idea
amywng
left a comment
amywng
left a comment
There was a problem hiding this comment.
one small nit but other than that LGTM!!!
sam-schu
left a comment
sam-schu
left a comment
There was a problem hiding this comment.
After discussion with Dalton related to using this PR as an example for using a transaction in another PR, we realized it's a bit hard to keep track of whether we're using the default repo, manager, etc. or the one from the transaction. Can we prefix any of these variables/params (for manager, repo, etc.) that come from the transaction manager (if one is provided) with transaction so it's easier to tell that we didn't accidentally use the default one rather than the transaction one somewhere?
| ); | ||
| const fmDonationIdSet = new Set(fmDonations.map((d) => d.donationId)); | ||
|
|
||
| const donationItemIds = Object.keys(itemAllocations).map(Number); |
There was a problem hiding this comment.
Is the Number conversion necessary anymore?
There was a problem hiding this comment.
I think it is still necessary, it doesn't work when I remove it and I thinnk that Object.keys always returns a string[] so Ii need to map to number
3 participants
ℹ️ Issue
(https://vidushimisra.atlassian.net/browse/SSF-153)
📝 Description
This PR adds an endpoint for volunteers to create a new order. There is a lot of criteria going into this route with lots of checks and side effects, the ticket details what I mean.
There were a decent amount of new routes that had to be added for this functionality to work. Here is the list:
/create-multiple in allocations
/associated-donations, /all, /set-quantities, in donation items
/match-all in donations
/create in order
✔️ Verification
I tested functionality of all backend routes with Postman and I also wrote and made sure all tests passed.
🏕️ (Optional) Future Work / Notes
As a note, there are no tests for allocations, and no service tests yet for donation items. These will be added in a future ticket.