feat(customer): enforce unique phoneNumber with validation, duplicate handling & cleanup

[overcut-ai]

Resolves #17

Overall goal:
Ensure customer phone numbers remain unique across the system and gracefully handle attempts to create duplicates.

Checklist / Implemented Phases:

• Phase 1 – Prisma schema updated with @unique on Customer.phoneNumber + migration generated.
• Phase 2 – Application-level handling of Prisma P2002 (duplicate) mapped to DuplicatePhoneNumberError returning HTTP 409.
• Phase 3 – DTO validation decorators added to enforce non-empty, numeric phone number pattern.
• Phase 4 – Unit & integration tests added/extended to cover duplicate scenarios (both service & HTTP levels).
• Phase 5 – Documentation updates in CHANGELOG & README noting uniqueness requirement.
• Phase 6 – One-time cleanup script (npm run cleanup:duplicates) + package.json script & README instructions.

Notes for reviewers:

1. Run order for production:
   a. Execute npm run cleanup:duplicates to purge existing duplicate phone numbers.
   b. Apply database migrations (Prisma migrate deploy) to add the unique constraint.
2. Tests pass locally (npm test).
3. Review new docs in README & CHANGELOG for additional context.

[overcut-ai]

Completed Working on "Code Review"

✅ Code review process completed: All 10 items reviewed, inline comments added, and overall summary submitted (COMMENT).

---

👉 View complete log

[overcut-ai]

Summary of review (action required):

1. Schema/DTO inconsistency – phoneNumber is @unique but still nullable in Prisma while DTOs/tests treat it as required. This weakens uniqueness and can cause runtime errors.
2. Error handling – customer.service maps every P2002 to DuplicatePhoneNumberError; check error.meta.target to avoid misclassification and sanitize unexpected errors.
3. Security – DefaultAuthGuard always returns true; ACL interceptors are empty, effectively disabling auth/ACL. Ensure they’re implemented or not registered in production.
4. Cleanup script – Runs findMany then deleteMany outside a transaction, risking race conditions and high memory usage; wrap in transaction or optimize query.
5. Tests – Add negative test confirming non-phone unique violations aren’t turned into DuplicatePhoneNumberError.
6. package.json – Move ts-node and nest-access-control to devDependencies if not used at runtime; update docs to clarify nullable uniqueness caveat and safe script usage.

Please address these points before merging.

[overcut-ai]

🔍 Correctness / Data-Integrity

phoneNumber is now marked @unique, but it remains nullable (String?). In most relational databases (e.g. PostgreSQL) a UNIQUE constraint still allows multiple NULL values, meaning the schema will not prevent inserting several customers without a phone number.

Because the DTO layer was changed to require a non-empty phone number, keeping the column nullable introduces a mismatch between the application contract and the database and leaves room for unexpected NULL values via:

1. Legacy data or direct DB access scripts
2. Future migrations/tests that bypass the validation layer

If every customer must have a phone number and it must be unique, consider dropping the ? to make the column non-nullable:

phoneNumber String  @unique

This guarantees consistency at the storage layer and aligns with the new validation decorators.

If there is a transitional period where NULL values must persist, please document the plan and add a follow-up migration to set NOT NULL once the cleanup script has removed existing gaps.

[overcut-ai]

nullable: false makes the field mandatory at the GraphQL / DTO layer, but the underlying Prisma schema still defines phoneNumber as optional (String? @unique). This mismatch means:

1. The database will still accept NULL values (and multiple rows with NULL), so the uniqueness guarantee you intend to enforce can be silently bypassed by callers that do not go through this DTO (e.g. seeds, scripts, future mutations).
2. Runtime code that relies on the Prisma type may still pass null/undefined, leading to inconsistent data or unexpected P2002 errors only in some paths.

Either make the column non-nullable in Prisma (String @unique) or keep the DTO nullable and enforce validation only when a value is provided, so both layers stay consistent.

[overcut-ai]

🛠️ Correctness & Maintainability

P2002 is Prisma’s generic unique-constraint violation. Multiple columns in the Customer table may be subject to @unique (e.g. id, any future email, etc.).

Relying only on error.code === "P2002" will therefore turn all unique-constraint collisions into a DuplicatePhoneNumberError, potentially leaking the wrong information to callers and hiding other integrity issues.

Prisma includes the conflicting columns in error.meta?.target – you can safely disambiguate with:

if (
  error instanceof Prisma.PrismaClientKnownRequestError &&
  error.code === "P2002" &&
  Array.isArray(error.meta?.target) &&
  error.meta.target.includes("phoneNumber")
) {
  throw new DuplicatePhoneNumberError();
}

This also removes the any cast and keeps strict typing.

Consider updating the guard accordingly so only phoneNumber collisions are mapped to DuplicatePhoneNumberError.

[overcut-ai]

🔐 Security / Information Leakage

Using throw error; re-throws the raw Prisma error back up the call stack, which eventually propagates to Nest’s HTTP exception filter. Unless you have a global filter that sanitises Prisma errors, this may leak internal details (database column names, constraint identifiers) to API consumers.

Consider mapping all unexpected errors to a generic InternalServerErrorException (or re-use an existing domain error) instead of exposing driver specifics.

[overcut-ai]

Consider wrapping the selection + deletion steps inside a single prisma.$transaction([...]) call.
Without a transaction, any concurrent writes between the findMany (lines 55-59) and this deleteMany could introduce race-conditions where:

1. a new customer with the same phone number is inserted after findMany runs, or
2. another process deletes/updates one of the records you plan to keep/remove.
   Enclosing both operations in a transaction guarantees atomicity and prevents accidental data loss under load.

[overcut-ai]

Because this interceptor currently performs no validation and simply forwards the request, registering it globally may give a false sense of security and mask missing ACL checks. Until full logic is implemented, consider:

• guarding registration with a feature flag,
• logging a warning when invoked, or
• at minimum, documenting clearly that it is a no-op.
  Otherwise, reviewers might assume requests are validated when they are not.

[overcut-ai]

Similar to the request-validation interceptor, this response filter currently does nothing. If consumers rely on it to mask restricted fields, sensitive data could leak. Until implemented, you might:

• leave the class unregistered,
• throw NotImplementedException when invoked, or
• include a TODO comment with a link to the tracking ticket to avoid accidental production exposure.

[overcut-ai]

The note accurately states that phoneNumber is now unique, but it does not mention that the underlying Prisma field is still nullable (String? @unique). Because most SQL dialects allow multiple NULL values under a unique constraint, it is still possible to insert many customers whose phoneNumber is null, silently bypassing the uniqueness guarantee you describe here. Either:

1. Update the schema to make phoneNumber non-nullable, or
2. Amend this note to clarify that the uniqueness guarantee only applies to non-null values and highlight the expectation that phoneNumber is always provided at the API level.

Documenting this caveat will prevent confusion and production data inconsistencies.

[overcut-ai]

Consider adding --preview-feature in the command (if required by your Prisma version) or specifying the Node environment (e.g. NODE_ENV=script) so that the script connects to the correct database. Without clear guidance, teams might accidentally run this against production before verifying in staging.

[yuval-hazaz]

@overcut i accept that , please add a commit with this change on this PR

[overcut-ai]

Working on "Interactive Mention Response"

Progress: 🟩⬜⬜⬜⬜⬜⬜⬜⬜⬜ 0%

⏳ Initializing...

---

👉 View live progress & logs

_{I'll update this comment with progress and post final results when complete.}

[overcut-ai]

Minor but important: the error class is named DuplicatePhoneNumberError server-side, but the client receives the standardized error code DUPLICATE_PHONE_NUMBER. Aligning the wording here (or adding the literal string) makes it easier for frontend consumers scanning the changelog to know what to guard for.

[overcut-ai]

Completed Working on "Auto-Changelog Reminder"

✅ Reviewed PR for changelog requirement: user-facing changes detected, changelog modification present. Posted acknowledgment comment.

---

👉 View complete log

[overcut-ai]

✅ Changelog entry detected. Thank you!

[overcut-ai]

📌 Change applied: Customer.phoneNumber is now non-nullable (String @unique).

Updated:

1. schema.prisma
2. Migration SQL – column set NOT NULL.
3. Docs (README & CHANGELOG).

Let me know if you’d like further tweaks – otherwise we’re good to merge. 🙌

I’ll keep this context open in this thread - reply with @overcut to continue.