[SSF-125] Move Food Request Fields by Tarun-Nagesh · Pull Request #100 · Code-4-Community/ssf

Tarun-Nagesh · 2026-02-08T18:53:09Z

ℹ️ Issue

📝 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:

Created TypeORM migration to move dateReceived, feedback, and photos fields from food_requests table to orders table
Added status field to FoodRequest entity with enum values ACTIVE and CLOSED
Moved confirm delivery endpoint from requests to orders: POST /api/orders/:orderId/confirm-delivery (previously POST /api/requests/:requestId/confirm-delivery)
Implemented automatic request status updates - request closes when all its orders are delivered
Fixed TypeScript compilation by defining local MulterFile interface in aws-s3.service.ts (could be changed later with tsconfig fix instead)

✔️ Verification

Ran migration with yarn typeorm:migrate
Tested confirm delivery endpoint via PowerShell
All tests passing (added new tests for confirm delivery logic)

dburkhart07

Some initial comments. Good first pass though! 🥇

apps/backend/src/foodRequests/request.controller.spec.ts

apps/backend/src/foodRequests/request.controller.ts

apps/backend/src/foodRequests/request.service.spec.ts

apps/backend/src/foodRequests/types.ts

apps/backend/src/migrations/1770571145350-MoveRequestFieldsToOrders.ts

apps/backend/src/orders/order.controller.ts

dburkhart07 · 2026-02-10T07:36:45Z

apps/backend/src/orders/order.entity.ts

  })
  deliveredAt: Date | null;

+  @Column({ name: 'date_received', type: 'timestamp', nullable: true })


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.

Unresolving this.

apps/backend/src/orders/order.service.ts

dburkhart07

few more small things. looks great so far!!

apps/backend/src/orders/dtos/confirm-delivery.dto.ts

apps/backend/src/orders/order.controller.spec.ts

apps/backend/src/orders/order.service.spec.ts

apps/backend/src/orders/order.controller.ts

apps/backend/src/orders/order.service.ts

dburkhart07

few more things, and just some questions that i wanted your opinions on. looking beautiful so far ⛱️

apps/backend/src/aws/aws-s3.service.ts

apps/backend/src/migrations/1770571145350-MoveRequestFieldsToOrders.ts

apps/frontend/src/types/types.ts

apps/backend/src/orders/order.service.ts

apps/backend/src/orders/dtos/confirm-delivery.dto.ts

apps/backend/src/orders/order.controller.ts

apps/backend/src/orders/order.entity.ts

apps/backend/src/orders/order.service.spec.ts

dburkhart07

One more thing, but looks good.

dburkhart07 · 2026-02-24T05:12:52Z

apps/backend/src/orders/order.entity.ts

  })
  deliveredAt: Date | null;

+  @Column({ name: 'date_received', type: 'timestamp', nullable: true })


Unresolving this.

sam-schu · 2026-02-24T04:38:39Z

apps/backend/src/orders/order.entity.ts

+  feedback?: string | null;
+
+  @Column({ name: 'photos', type: 'text', array: true, nullable: true })
+  photos?: string[];


Can we add | null here?

another thing tarun had put and dalton asked to be removed, i agree with dalton here as ? implies the value can be undefined so | null isn't necessary, it also wouldn't be consistent with how the rest of the code base handles nullable fields

In Justin's strict mode PR, we're adding | null to entity fields corresponding to nullable database fields because we realized that TypeORM actually returns null, not undefined, for a null database entry. (The undefined option should only be used if you want to call a repo function with a subset of the entity fields)

sam-schu · 2026-02-24T04:42:46Z

apps/backend/src/orders/dtos/confirm-delivery.dto.ts

@@ -0,0 +1,4 @@
+export class ConfirmDeliveryDto {


Can we use class-validator to validate the fields in this DTO, as we have for other endpoint body DTOs?

sam-schu · 2026-02-24T04:45:28Z

apps/backend/src/orders/order.controller.ts

+        dateReceived: {
+          type: 'string',
+          format: 'date-time',
+          nullable: true,


Pending confirmation from Priya, but assuming the date received is required as it is in the DTO, this should not be nullable here

We can assume this for now as it may need to change later based on client updates

sam-schu · 2026-02-24T04:51:17Z

apps/backend/src/aws/aws-s3.service.ts

@@ -1,5 +1,6 @@
 import { Injectable } from '@nestjs/common';
 import { S3Client, PutObjectCommand } from '@aws-sdk/client-s3';
+import 'multer';


Is this necessary since there were no other changes to this file?

sam-schu · 2026-02-24T04:56:31Z

apps/backend/src/orders/order.service.ts


+  async confirmDelivery(
+    orderId: number,
+    dto: ConfirmDeliveryDto,


Can we pass in the individual fields rather than the DTO to limit the DTO to the controller layer?

This is how Tarun originally had it, Dalton said in his review that this is not how ssf does it. I agree with you that it would generally be the case that you would want to limit the DTO to the controller layer, but overall looking at the repository it seems like DTOs are almost always passed into the service (according to Dalton this is true, I briefly looked through most of the controllers and this tends to be true). I think we should just be consistent with how this is done on the project and leave it as is

I'm fine with that for consistency, I think that started with service functions that would need to take in an impractical number of parameters (e.g., for pantry applications) and ended up spreading to almost all of our DTOs

sam-schu · 2026-02-24T05:35:46Z

apps/backend/src/orders/order.service.spec.ts

+          { dateReceived: new Date().toISOString(), feedback: 'test' },
+          [],
+        ),
+      ).rejects.toThrow(`Order ${invalidOrderId} not found`);


Can we check this is a NotFoundException?

sam-schu · 2026-02-24T05:36:23Z

apps/backend/src/orders/order.service.spec.ts

+          { dateReceived: 'invalid-date', feedback: 'test feedback' },
+          [],
+        ),
+      ).rejects.toThrow('Invalid date format for dateReceived');


Can we check this is a BadRequestException?

sam-schu · 2026-02-24T05:37:38Z

apps/backend/src/orders/order.service.spec.ts

+    it('should throw BadRequestException when order is not shipped', async () => {
+      const orderRepo = testDataSource.getRepository(Order);
+
+      const pendingOrder = await orderRepo.findOne({


Can we also check that we can't re-confirm a delivered order?

sam-schu · 2026-02-24T05:42:01Z

apps/backend/src/orders/order.service.spec.ts

+      const orderRepo = testDataSource.getRepository(Order);
+      const requestRepo = testDataSource.getRepository(FoodRequest);
+
+      const shippedOrder = await orderRepo.findOne({


Can we get a specific order here? this implies that any shipped order will work for this test, which is not true since we're checking that the request is being closed

sam-schu · 2026-02-24T05:46:12Z

apps/backend/src/orders/order.service.spec.ts

+      expect(updatedRequest.status).toBe(FoodRequestStatus.ACTIVE);
+    });
+
+    it('should set request status to CLOSED when all orders are delivered', async () => {


I'm not quite sure what the difference is in what we're checking in these two tests compared to the two previous tests?

Yeah looks like 1 and 3 are pretty much the same, 2 is different because it is a case where there are more orders in the request

Isn't 4 ("should set request status to ACTIVE when not all orders are delivered") also checking essentially the same thing as 2?

Tarun-Nagesh added 3 commits February 8, 2026 12:43

Updated logic for moving fields

a3ec50a

Added/changed tests

f4a22c2

Some scuffed workaround, unsure if it has any other implications

bb71e3e

sam-schu added the high priority label Feb 9, 2026

dburkhart07 requested changes Feb 10, 2026

View reviewed changes

Tarun-Nagesh added 4 commits February 18, 2026 23:10

cleanup and comments

4dd7a44

merging main

aee1b48

linting

fda0ece

fixed test

4b16f21

Juwang110 mentioned this pull request Feb 20, 2026

SSF-130 Properly Displaying Request Status on Frontend #112

Open

Added status back to FoodRequest

e1dc7c0

Tarun-Nagesh requested review from dburkhart07 and sam-schu February 20, 2026 23:38

dburkhart07 requested changes Feb 22, 2026

View reviewed changes

maxn990 and others added 5 commits February 23, 2026 13:25

resolve comments

66805a8

fix test

9f2a664

fix test

699b0a5

fix tests

4d5bba1

Merge branch 'main' into TN/SSF-125-move-food-request-fields

53cfb2f

maxn990 requested a review from dburkhart07 February 23, 2026 18:50

dburkhart07 requested changes Feb 23, 2026

View reviewed changes

Juwang110 mentioned this pull request Feb 23, 2026

SSF-143 Reusable Order Details Modal #110

Open

resolve comments

6ed0047

maxn990 requested a review from dburkhart07 February 24, 2026 01:47

dburkhart07 approved these changes Feb 24, 2026

View reviewed changes

fix modules

b332d10

sam-schu requested changes Feb 24, 2026

View reviewed changes

fix order entity

19af4cb

maxn990 self-assigned this Feb 24, 2026

Comments

Conversation

Tarun-Nagesh commented Feb 8, 2026

ℹ️ Issue

📝 Description

✔️ Verification

Uh oh!

dburkhart07 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dburkhart07 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dburkhart07 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dburkhart07 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!