Conversation
|
See slack comment |
313c63d to
2577fbe
Compare
2577fbe to
265bad6
Compare
dburkhart07
left a comment
There was a problem hiding this comment.
Make these initial changes, also see message I sent you.
| yarn-error.log | ||
| testem.log | ||
| /typings | ||
| .nx |
There was a problem hiding this comment.
Why is this here? Can we get rid of it?
| pantries?: Pantry[]; | ||
| } | ||
|
|
||
| export interface UserDto { |
There was a problem hiding this comment.
This should not be deleted either I dont think.
…com/Code-4-Community/ssf into sk/SSF-97-pantry-management-backend
…k/SSF-97-pantry-management-backend
…com/Code-4-Community/ssf into sk/SSF-97-pantry-management-backend
dburkhart07
left a comment
There was a problem hiding this comment.
Few initial things. Also, I do not see the second half of the ticket requirement to make a new endpoint for this: Add a new endpoint to overwrite the set of volunteers assigned to a pantry with a new set of volunteers for intended use case
apps/backend/src/pantries/types.ts
Outdated
| email: string; | ||
| phone: string; | ||
| }; | ||
| refrigeratedDonation: string; |
apps/backend/src/pantries/types.ts
Outdated
| }; | ||
| refrigeratedDonation: string; | ||
| allergenClients: string; | ||
| status: string; |
| } | ||
|
|
||
|
|
||
| async getApprovedPantriesWithVolunteers(): Promise<ApprovedPantryResponse[]> { |
There was a problem hiding this comment.
See comment in the type.ts file, this will need to change too.
| return this.pantriesService.getPendingPantries(); | ||
| } | ||
|
|
||
| @Get('/approved') |
There was a problem hiding this comment.
We should write a test for this as well.
…k/SSF-97-pantry-management-backend
…k/SSF-97-pantry-management-backend
|
Final comments:
|
dburkhart07
left a comment
There was a problem hiding this comment.
LGTM! Thanks for responding to so much back-and-forth 🔴🐼
Yurika-Kan
left a comment
There was a problem hiding this comment.
first round of review!
found a field that was missing
done: jest tests ran
pending: testing via postman
litttt
| contactPhone: pantry.pantryUser.phone, | ||
| shipmentAddressLine1: pantry.shipmentAddressLine1, | ||
| shipmentAddressCity: pantry.shipmentAddressCity, | ||
| shipmentAddressZip: pantry.shipmentAddressZip, |
There was a problem hiding this comment.
shipmentAddressState is also missing here!
| contactEmail: string; | ||
| contactPhone: string; | ||
| shipmentAddressLine1: string; | ||
| shipmentAddressCity: string; |
There was a problem hiding this comment.
can zip be after state & country?
Yurika-Kan
left a comment
There was a problem hiding this comment.
hefty second review! some small changes, adding auth gating, testing, and a bit more! thanks for the great work so far <3
(update) tested:
- postman calls
| shipmentAddressLine1: '123 Main Street', | ||
| shipmentAddressCity: 'Boston', | ||
| shipmentAddressZip: '02101', | ||
| shipmentAddressState: 'MA', |
There was a problem hiding this comment.
can you make it so state comes before zip?
| @@ -16,6 +17,7 @@ import { Roles } from '../auth/roles.decorator'; | |||
| import { ValidationPipe } from '@nestjs/common'; | |||
There was a problem hiding this comment.
can we add this to the list of imports above in the first lines?
| import { ValidationPipe } from '@nestjs/common'; | ||
| import { PantryApplicationDto } from './dtos/pantry-application.dto'; | ||
| import { ApiBody } from '@nestjs/swagger'; | ||
| import { ApprovedPantryResponse } from './types'; |
There was a problem hiding this comment.
also here add this import into the list of imports below so that we aren't unnecessarily calling multiple imports of the same file
| @@ -56,6 +58,11 @@ export class PantriesController { | |||
| return this.pantriesService.getPendingPantries(); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
since we began auth gating endpoints by role, let's continue that here by making this only accessible to admin role - aligned with the feature implications of this backend ticket
| attachments, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
same here with annotating this to be accessible only for admin
| name: string; | ||
| email: string; | ||
| phone: string; | ||
| role: string; |
There was a problem hiding this comment.
we have a user enum that we should use instead of string here to determine role!
| }, | ||
| ], | ||
| }).compile(); | ||
|
|
There was a problem hiding this comment.
missing tests for getApprovedPantriesWithVolunteers & updatePantryVolunteers
| import { PantryApplicationDto } from './dtos/pantry-application.dto'; | ||
| import { OrdersService } from '../orders/order.service'; | ||
| import { Order } from '../orders/order.entity'; | ||
| import { ApprovedPantryResponse } from './types'; |
There was a problem hiding this comment.
lets add this to the list of imports below!
| contactEmail: string; | ||
| contactPhone: string; | ||
| shipmentAddressLine1: string; | ||
| shipmentAddressCity: string; |
There was a problem hiding this comment.
can zip be after state & country?
| name: string; | ||
| email: string; | ||
| phone: string; | ||
| role: string; |
There was a problem hiding this comment.
i don't think we need to save role of the volunteer here - they should always be volunteer - could you delete this everywhere in this implementation? @sam-schu lmk thoughts
ℹ️ Issue 97
Closes 97
📝 Description
Added a new endpoint (getApprovedPantries) to return all information necessary for the pantry management frontend about pantries with 'approved' status - includes assigned volunteers.
Added a second new endpoint (updatePantryVolunteers) to overwrite the set of volunteers assigned to a pantry with a new set of volunteers.
Added a type file for pantries and included ApprovedPantryResponse and AssignedVolunteer types.
Added a controller test for each endpoint.
✔️ Verification
Tested both endpoints using curl. Made sure GET endpoint retrieved all approved pantries with assigned volunteers, and that the PUT endpoint overwrites volunteer assignments successfully.