Validate Customer Email Format (Resolves #55) #56
Conversation
- Added override DTOs with @isemail and @IsOptional to ensure correct email format and maintain regeneration safety - Updated CustomerController to use new DTOs - Ensured global ValidationPipe is configured with whitelist option - Implemented unit tests for DTO validation logic - Added E2E tests for invalid email rejection on create and update - Updated README and created CHANGELOG entry documenting the change - Ran all tests to confirm they pass
Completed Working on "Code Review v2.0"✅ Review submitted: COMMENT. Total comments: 5 across 4 files. |
![overcut-ai[bot]](https://avatars.githubusercontent.com/in/1367736?s=60&v=4){kind=small}
There was a problem hiding this comment.
Added 5 focused review comments:
- Missing GraphQL/Swagger decorators in CustomerCreateInput override
- Potential breaking change from
whitelist: true - Missing undefined-email test for CustomerUpdateInput
- E2E routes omit global
apiprefix - Lack of positive-case assertions in E2E tests
| export class CustomerCreateInput extends BaseCustomerCreateInput { | ||
| @IsEmail({}, { message: "Invalid email format" }) | ||
| @IsOptional() | ||
| override email?: string | null = undefined; |
There was a problem hiding this comment.
🛠 Missing GraphQL/Swagger decorators after override
Redefining the email property overrides the metadata that Nest (and @nestjs/swagger/GraphQL) gathered from the base class, so @Field and @ApiProperty are no longer attached. As a result the field disappears from the generated GraphQL schema and Swagger docs, breaking existing clients.
Please re-add the respective decorators, e.g.:
@Field(() => String, { nullable: true })
@ApiProperty({ required: false, type: String })(or avoid redeclaring the property and extend validation via @ValidateIf).
| app.useGlobalPipes( | ||
| new ValidationPipe({ | ||
| transform: true, | ||
| whitelist: true, |
There was a problem hiding this comment.
whitelist: true will silently strip any extra properties from incoming payloads. If existing clients rely on sending unknown fields the requests will now succeed but data will be lost, which can be hard to debug.
Please confirm that no consumers depend on this behaviour and consider adding regression tests to cover the stripping logic.
| }); | ||
| }); | ||
|
|
||
| describe("CustomerUpdateInput", () => { |
There was a problem hiding this comment.
🔬 Edge-case missing – CustomerUpdateInput is marked @IsOptional, so email being undefined should also pass validation. The current suite covers null but not undefined. Adding a test similar to the create-input suite will prevent future regressions.
|
|
||
| it("POST /customers with invalid email should return 400", async () => { | ||
| await request(app.getHttpServer()) | ||
| .post("/customers") |
There was a problem hiding this comment.
🌐 Route does not include global prefix
The production app sets app.setGlobalPrefix("api"), therefore the real endpoint is /api/customers. Exercising /customers here means the test does not cover the deployed path and may give false positives. Please either prepend api to the request paths or import the full AppModule and call setGlobalPrefix in the test setup.
| await app.close(); | ||
| }); | ||
|
|
||
| it("POST /customers with invalid email should return 400", async () => { |
There was a problem hiding this comment.
✅ Add at least one success scenario
The suite only asserts failure cases for invalid emails. Including a test that submits a valid payload and expects 201 Created (POST) / 200 OK (PATCH) will ensure the happy path remains functional when validation or handler logic changes.
There was a problem hiding this comment.
Review Summary
Total comments: 5 across 4 files
Severity: 3 major (blocking), 2 minor (non-blocking)
Key themes:
- Metadata loss and API contract breaks due to DTO overrides (GraphQL/Swagger decorators and global prefix).
- Breaking-change risk from enabling ValidationPipe
whitelist. - Test coverage gaps (edge-case validation and positive E2E scenarios).
Next steps:
• Address major issues before merge: restore decorators, adjust route prefixes, verify whitelist impact.
• Add the suggested tests.
• Re-run full test suite.
| app.useGlobalPipes( | ||
| new ValidationPipe({ | ||
| transform: true, | ||
| whitelist: true, |
There was a problem hiding this comment.
Aggregated review summary will be provided in the overall review message.
Completed Working on "Auto-Changelog Reminder"✅ Changelog check completed for PR #56: changelog entry found; confirmation comment posted. |
|
✅ Changelog entry detected. Thank you! |
Overall goal: enforce proper email validation on customer create/update endpoints.
Checklist of completed tasks:
Notes for reviewers: Discuss regeneration-safe override strategy.