Conversation
dburkhart07
left a comment
dburkhart07
left a comment
There was a problem hiding this comment.
Some initial comments. Good first pass though! 🥇
| mockRequestsService.find.mockReset(); | ||
| mockRequestsService.create.mockReset(); | ||
| mockRequestsService.updateDeliveryDetails?.mockReset(); | ||
| // mockRequestsService.updateDeliveryDetails?.mockReset(); // Removed - method no longer exists |
There was a problem hiding this comment.
Let's actually delete all of these isntead of commenting them out.
| export class RequestsController { | ||
| constructor( | ||
| private requestsService: RequestsService, | ||
| private awsS3Service: AWSS3Service, |
There was a problem hiding this comment.
Let's clean up the request.module.ts file if we are removing these. We should not need the AWSS3Module anymore, as well as the Multer one I believe.
| dateReceived: null, | ||
| feedback: null, | ||
| photos: null, | ||
| // dateReceived: null, // Removed - no longer on FoodRequest |
There was a problem hiding this comment.
Same thing here, let's just delete everything instead of commenting it out.
| LARGE = 'Large (10+ boxes)', | ||
| } | ||
|
|
||
| export enum FoodRequestStatus { |
There was a problem hiding this comment.
Let's bring this enum into the frontend types as well. While we are at it, can we also rewrite the frontend FoodRequest type, and briefly smoke test the entire app to make sure nothing is broken by us changing the frontend type?
|
|
||
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query(` | ||
| CREATE TYPE food_requests_status_enum AS ENUM ('Active', 'Closed'); |
There was a problem hiding this comment.
small nit: can we ask sam/yurika about the capitalization here? our enums are normally always lowercase, so it seems weird to change it here.
| @UseInterceptors( | ||
| FilesInterceptor('photos', 10, { storage: multer.memoryStorage() }), | ||
| ) | ||
| async confirmDelivery( |
There was a problem hiding this comment.
This migth have a bit more to it, but can we create a DTO for these 3 fields and use it in the confirm delivery here? Not quite sure what the photos would look like in the DTO, but would be much cleaner.
| }) | ||
| deliveredAt: Date | null; | ||
|
|
||
| @Column({ name: 'date_received', type: 'timestamp', nullable: true }) |
There was a problem hiding this comment.
We are trying to enforce strict type, so we need to give everything a ! for required fields, and ? for optional. So let's add 3 ?s here.
| return updatedOrder; | ||
| } | ||
|
|
||
| private async updateRequestStatus(requestId: number): Promise<void> { |
There was a problem hiding this comment.
Let's make sure to validate the requestId first.
| throw new NotFoundException(`Request ${requestId} not found`); | ||
| } | ||
|
|
||
| const orders = request.orders || []; |
There was a problem hiding this comment.
I think if we have no orders for a function like this, we should be throwing an error. This function should never be getting called on a request with no orders.
| order.feedback = feedback; | ||
| order.photos = photos; | ||
| order.status = OrderStatus.DELIVERED; | ||
| order.deliveredAt = dateReceived; |
There was a problem hiding this comment.
This field doesn't make sense to me. Good question for @sam-schu or @Yurika-Kan what is the difference between dateReceived and the deliveredAt date?
3 participants
ℹ️ Issue
Closes SSF-125
📝 Description
Moved delivery confirmation fields from Food Requests to Orders and implemented request status logic. This change reflects where delivery details are specific to individual orders instead of the entire request.
Backend Changes:
✔️ Verification