Conversation
| foodType: FoodType; | ||
| name!: string; | ||
| quantity!: number; | ||
| foodType?: FoodType; |
There was a problem hiding this comment.
Based on the DB, a donation items foodType can be null. I'm not sure if that makes sense though...
There was a problem hiding this comment.
Yeah, food type should not be nullable in the db...
| @Column({ name: 'fulfilled_at', type: 'timestamp' }) | ||
| fulfilledAt: Date; | ||
| @Column({ name: 'fulfilled_at', type: 'timestamp', nullable: true }) | ||
| fulfilledAt?: Date | null; |
There was a problem hiding this comment.
Can these fields actually be undefined or null, or does it always come back one way or the other if the database column is null?
|
|
||
| return response.UserConfirmed; | ||
| } catch (err: unknown) { | ||
| throw new BadRequestException('Failed to sign up user'); |
There was a problem hiding this comment.
Bad request implies that it's the client's fault, if we don't really know what caused the error then I'd suggest going with an internal server error
| await this.providerClient.send(adminDeleteUserCommand); | ||
| } | ||
|
|
||
| validateAuthenticationResultTokensForSignIn( |
There was a problem hiding this comment.
Should these methods be private?
|
|
||
| const response = await this.providerClient.send(signInCommand); | ||
|
|
||
| this.validateAuthenticationResultTokensForSignIn(response); |
There was a problem hiding this comment.
For both of these functions, can we return the authentication result from the function with a type that will avoid having to do non-null assertions?
| import { Request } from 'express'; | ||
| import { User } from '../users/user.entity'; | ||
|
|
||
| export interface AuthenticatedRequest extends Request { |
There was a problem hiding this comment.
I got confused by this at first - could you add a comment explaining that user does not have to be provided by the client but is instead added automatically by the auth backend?
| ): Promise<FoodRequest> { | ||
| if ( | ||
| !Object.values(RequestSize).includes(body.requestedSize as RequestSize) | ||
| !Object.values(RequestSize).includes( |
There was a problem hiding this comment.
Do we still need this check now that we have the DTO?
| requestedSize!: RequestSize; | ||
|
|
||
| @ArrayNotEmpty() | ||
| requestedItems!: string[]; |
There was a problem hiding this comment.
Shouldn't this be limited to the valid food types?
| @IsOptional() | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| @MaxLength(255) |
There was a problem hiding this comment.
We shouldn't need a max length here since the corresponding database column is of type text (some of ours are varchar(255) and need this for that reason)
| const createBody = { | ||
| pantryId: 1, | ||
| requestedSize: RequestSize.MEDIUM, | ||
| requestedItems: ['Test item 1', 'Test item 2'], |
There was a problem hiding this comment.
Can we change these to valid food types?
| describe('POST /create', () => { | ||
| it('should call requestsService.create and return the created food request', async () => { | ||
| const createBody: Partial<FoodRequest> = { | ||
| const createBody = { |
There was a problem hiding this comment.
Is the cast necessary here or can we just type this object normally?
| status: order.status, | ||
| foodManufacturerName: order.foodManufacturer.foodManufacturerName, | ||
| foodManufacturerName: | ||
| order.foodManufacturer?.foodManufacturerName ?? undefined, |
There was a problem hiding this comment.
I don't think this is necessary
| name: allocation.item.itemName, | ||
| quantity: allocation.allocatedQuantity, | ||
| foodType: allocation.item.foodType, | ||
| foodType: allocation.item.foodType ?? undefined, |
There was a problem hiding this comment.
Noting here for after the migration that we won't need this either
| @IsNotEmpty({ each: true }) | ||
| @MaxLength(255, { each: true }) | ||
| restrictions?: string[]; | ||
| restrictions!: string[]; |
There was a problem hiding this comment.
What was the reason for this change?
| await expect(controller.getCurrentUserPantryId({})).rejects.toThrow( | ||
| new UnauthorizedException('Not authenticated'), | ||
| ); | ||
| await expect( |
There was a problem hiding this comment.
This case seems important enough that it is an expected case for the function to need to deal with, which is making me think that maybe we should be typing the controller function to take in a Request | AuthenticatedRequest instead of assuming it will always be an AuthenticatedRequest?
| "compilerOptions": { | ||
| "esModuleInterop": true | ||
| "esModuleInterop": true, | ||
| "strict": true |
There was a problem hiding this comment.
very epic and awesome line of code right here that I am very excited for !!
| export interface CreateRequestDto { | ||
| pantryId: number; | ||
| requestedSize: RequestSize; | ||
| requestedItems: string[]; |
There was a problem hiding this comment.
Same comment here as in the backend
There was a problem hiding this comment.
I don't see any package.json changes?
3 participants
ℹ️ Issue
Closes https://vidushimisra.atlassian.net/jira/software/projects/SSF/boards/1?selectedIssue=SSF-131 and
https://vidushimisra.atlassian.net/jira/software/projects/SSF/boards/1?selectedIssue=SSF-138
📝 Description
This PR enables typescript strict mode in the backend which enables the compiler to be a lot more picky and strict with types, null and undefined checks, etc. The PR includes changes to make our backend adhere with all the rules that strict mode enforces. This involved work making entity fields optional or required, and also handling null/undefined errors and return values.
✔️ Verification
I made sure all the backend tests pass and npx nx serve backend properly runs without error.
🏕️ (Optional) Future Work / Notes
N/A