feat: validate customer email via @IsEmail decorator (Resolves #39) #40
Conversation
* Added CustomerCreateInput and CustomerUpdateInput overrides with @isemail validation * Wired customer controller to use new DTOs * Added e2e tests for email validation on create/update * Implemented Prisma and ACL testing mocks; all Jest tests pass * Updated README with validation note * Added ESLint config and resolved lint issues
Completed Working on "Code Review v2.0"✅ Review submitted: REQUEST_CHANGES. Total comments: 10 across multiple files. |
|
🚀 Build Reliability – react 18.3.x Please downgrade both packages to |
|
🛠 Build Compatibility – Node engine mismatch "engines": { "node": ">=20.0.0" }Our Dockerfile & CI use Node 18 LTS, so Please either:
Until resolved, the build will fail. |
|
🔐 Security / Image Size – Please move |
![overcut-ai[bot]](https://avatars.githubusercontent.com/in/1367736?s=60&v=4){kind=small}
| this[model] = new Proxy( | ||
| {}, | ||
| { | ||
| get: () => jest.fn(), |
There was a problem hiding this comment.
🛠 Correctness
get returns a new jest.fn() on every property access, meaning repeated calls to the same delegate method (e.g. prisma.customer.findMany) are tracked by different mock instances. Any test that wants to assert call counts or inspect mock .mock data will therefore see zero calls.
get: () => jest.fn(),Please memoise the mock per property, e.g.
const cache = new Map<string, jest.Mock>();
get: (_target, prop: string) => {
if (!cache.has(prop)) cache.set(prop, jest.fn());
return cache.get(prop)!;
},This preserves call history across invocations while keeping the lightweight design.
|
|
||
| // The generator usually also exports `Decimal`. Provide a minimal stub to avoid | ||
| // runtime failures when casting/formatting numeric fields in tests. | ||
| export class Decimal { |
There was a problem hiding this comment.
🛠 Correctness / Maintainability
The Decimal stub only implements toNumber(). Production code often calls toString(), valueOf(), or relies on JSON serialisation. Missing these will cause runtime errors once those paths are exercised in tests.
Consider implementing minimal shims:
class Decimal {
constructor(private readonly value: number | string) {}
toNumber() { return Number(this.value); }
toString() { return String(this.value); }
valueOf() { return this.toNumber(); }
toJSON() { return this.value; }
}This keeps the stub lightweight while covering common use-cases.
| "@types/validator": "^13.11.2", | ||
| "jest": "^29.7.0", | ||
| "jest-mock-extended": "^3.0.5", | ||
| "nest-access-control": "^3.1.0", |
There was a problem hiding this comment.
nest-access-control is imported by production code (e.g., src/auth/acl.module.ts) but is placed in devDependencies. In production environments where only dependencies are installed, this will cause a runtime module-not-found error. Please move the package to the dependencies section.
|
|
||
| @Injectable() | ||
| export class DefaultAuthGuard implements CanActivate { | ||
| canActivate(_context: ExecutionContext): boolean { |
There was a problem hiding this comment.
canActivate() unconditionally returns true, effectively bypassing all authorization checks in every environment. If this stub is only meant for tests, guard its usage with an environment check or provide it via a testing module override so that production builds continue to enforce auth.
| } | ||
|
|
||
| // Override: use validated DTO for create | ||
| @common.Post() |
There was a problem hiding this comment.
Overriding the controller methods without the @common.Override() decorator keeps the route metadata from the base class and registers new metadata for the re-declared methods. This leads to duplicate handlers for POST /customers and PATCH /customers/:id, which NestJS will reject at startup. Add @common.Override() (or remove the base declarations) to ensure only one handler is registered per route.
|
|
||
| @Injectable() | ||
| export class AclValidateRequestInterceptor implements NestInterceptor { | ||
| intercept(_context: ExecutionContext, next: CallHandler): Observable<any> { |
There was a problem hiding this comment.
The interceptor is currently a no-op; it forwards the request without performing any ACL validation. Either remove the interceptor to avoid a false sense of security or implement the validation logic (e.g., using RolesBuilder). At minimum, restrict registration of this stub to test environments.
| "strict": true | ||
| "strict": true, | ||
| "paths": { | ||
| "@prisma/client": ["__mocks__/@prisma/client.ts"] |
There was a problem hiding this comment.
Mapping @prisma/client to a concrete mock implementation means the production build will compile against the mock and may even bundle test-only code. Move this path mapping to a test-specific tsconfig (e.g. tsconfig.spec.json) or map to a .d.ts type-only file to avoid leaking test artifacts into production.
|
|
||
| ### Server scripts | ||
|
|
||
| > Validation note: The **Customer** entity's `email` field is validated using `class-validator`'s `@IsEmail` decorator. API requests to create or update a customer with an invalid email will be rejected with **HTTP 400** and the error message `Invalid email format`. |
There was a problem hiding this comment.
[meta] Starting overall review. Summary will follow in main review comment.
Completed Working on "Auto-Changelog Reminder"✅ User-facing changes detected without changelog entry; suggested entry posted on PR #40, task complete. |
|
Here’s a suggested entry you can copy-paste: - Added email format validation for Customer entity; create or update requests with invalid email now return HTTP 400.Reply with "approve changelog" and I will create a commit on this PR with the suggested entry. |
Overall Goal
Add validation to the Customer
emailfield so that creating or updating a customer with an invalid email returns HTTP 400.Checklist
CustomerCreateInput,CustomerUpdateInput) with@IsEmaildecoratorFollow-up Notes