Skip to content

Conversation

@pachonjcl
Copy link
Collaborator

@pachonjcl pachonjcl requested a review from roguenet December 22, 2025 16:25
@IsOptional()
@IsString()
@ApiProperty({ description: "Comment for the status update", required: false, type: String })
comment: string | null = null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For required: false, this should be comment?: string. If you really want to explicitly accept null, the ApiProperty should have nullable: true.

@ApiOperation({
operationId: "updateSitePolygonStatus",
summary: "Update the status of a site polygon",
description: "Update the status of a site polygon"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These descriptions make it sound like this endpoint operates on a single polygon.

@JsonApiResponse({ data: SitePolygonLightDto, hasMany: true })
@ExceptionResponse(UnauthorizedException, { description: "Authentication failed." })
@ExceptionResponse(BadRequestException, { description: "Invalid request data." })
@ExceptionResponse(NotFoundException, { description: "Site polygon not found." })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be something more like "At least one of the site polygons was not found".

const userId = this.policyService.userId;
if (userId == null) {
throw new UnauthorizedException("User must be authenticated");
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a normal endpoint (one that doesn't opt out of auth), the user id is guaranteed to be non null, so instead of this check, you can do:

const userId = this.policyService.userId as number;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants