SSF-225 FM Deleting & Editing Donation Item Backend#188
Conversation
There was a problem hiding this comment.
changes requested for codebase consistency!
let's have oz/value for donation items stay optional in the backend while the frontend requires them to match the create donation items & confirm item details contracts.
edit: i just saw your new pr up which updates those backend fields to be required & since it is better to match be + fe contracts and it's already implemented...let's keep it required in the be then, thanks for taking care of that :)
dburkhart07
left a comment
dburkhart07
left a comment
There was a problem hiding this comment.
very very clean pr! few small things and one question worth discussing, but aside from that lgtm!
| @Min(1) | ||
| quantity!: number; | ||
|
|
||
| @IsNumber( |
There was a problem hiding this comment.
@Yurika-Kan can you explain the rationale behind making these required? the case im considering:
if i make a new donation with 2 items where i dont fill out the ozPerItem and estimatedValue, the next time i go to update the donation, i cant just make an edit to one of them, i have to update all fields for all values. i think they should be allowed to go in and just update the ozPerItem for one item if thats all they have.
lmk if im missing something though!
There was a problem hiding this comment.
To my understanding we cannot now make new donation items without ozPerItem and estimatedValue?
There was a problem hiding this comment.
They wanted to make all those fields now required upon creation so this was to keep consistency with that request
There was a problem hiding this comment.
ie oz, donation val, food rescue
There was a problem hiding this comment.
okay, then in that case, while it wasnt directly in the ticket, i think we should make an update to the donationItems entity, and write a new migration to fix it in the database and fix the frontend create donations so everything is consistent again
There was a problem hiding this comment.
I asked Yurika specifically about the entity/db and she said it wasn't necessary at the time @Yurika-Kan thoughts?
dburkhart07
left a comment
dburkhart07
left a comment
There was a problem hiding this comment.
2 things, but imma approve since its pretty small.
| ): Promise<void> { | ||
| const itemRepo = transactionManager.getRepository(DonationItem); | ||
|
|
||
| const existingItems = await itemRepo.find({ where: { donationId } }); |
There was a problem hiding this comment.
we only use the ids, so we should just be able to put the itemRepo.find and map into one call and then delete existingItems altogether, and just use existingIds on 163
| @Min(1) | ||
| quantity!: number; | ||
|
|
||
| @IsNumber( |
There was a problem hiding this comment.
okay, then in that case, while it wasnt directly in the ticket, i think we should make an update to the donationItems entity, and write a new migration to fix it in the database and fix the frontend create donations so everything is consistent again
3 participants
ℹ️ Issue
Closes https://vidushimisra.atlassian.net/browse/SSF-225
📝 Description
Added backend support for patch route donations/:donationId/item to allow adding, editing and deleting donation items for a donation.
✔️ Verification
Postman verified and service/controller tests
🏕️ (Optional) Future Work / Notes
N/A