Skip to content

fix: relax NATIONAL_ID regex to accept short codes like DCC#2167

Merged
bjcoombs merged 2 commits intodevelopfrom
fix-national-id-regex
Apr 7, 2026
Merged

fix: relax NATIONAL_ID regex to accept short codes like DCC#2167
bjcoombs merged 2 commits intodevelopfrom
fix-national-id-regex

Conversation

@bjcoombs
Copy link
Copy Markdown
Collaborator

@bjcoombs bjcoombs commented Apr 7, 2026

Summary

  • Lowered the NATIONAL_ID external reference regex minimum from 5 to 2 characters
  • Short codes like DCC (3 chars), GETW (4 chars), and SEPD (4 chars) are legitimate UK energy industry identifiers used in the PAYG energy manifest
  • This has been causing the nightly Demo Reset workflow to fail on every run since the PAYG tenant was added (feat: PAYG energy billing manifest and demo tenant #1963)

Test plan

  • Added test case for short national ID (DCC) - passes
  • Existing TestValidateExternalReference suite passes
  • CI green
  • Demo Reset workflow succeeds after merge and redeploy

The national ID regex required 5-20 characters, rejecting legitimate
short codes like DCC (3 chars), GETW (4 chars), and SEPD (4 chars)
used in the PAYG energy manifest. This caused the nightly Demo Reset
workflow to fail on every run since the PAYG tenant was added.

Lowered the minimum from 5 to 2 characters.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 7, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 80738ffe-cb91-400f-a573-a85548f4f27d

📥 Commits

Reviewing files that changed from the base of the PR and between 96f0d1d and f8b04d5.

📒 Files selected for processing (1)
  • services/party/domain/party_test.go

📝 Walkthrough

Walkthrough

The national ID external reference validation regex was relaxed to accept 2–20 uppercase alphanumeric characters (was 5–20). Tests were updated: a boundary valid case shortened and a new valid test for "DCC" added.

Changes

Cohort / File(s) Summary
National ID Validation Regex Update
services/party/domain/party.go
Modified the national ID external reference validation regex from ^[A-Z0-9]{5,20}$ to ^[A-Z0-9]{2,20}$.
Test Case Addition / Adjustment
services/party/domain/party_test.go
Adjusted boundary tests for NationalID (valid case shortened) and added a new test case for ExternalReferenceTypeNationalID with reference "DCC" expecting successful validation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: relaxing the NATIONAL_ID regex to accept shorter codes, which directly aligns with the primary purpose of this PR.
Description check ✅ Passed The description clearly explains the regex change, provides legitimate business context (UK energy industry identifiers), and documents the impact on the Demo Reset workflow.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-national-id-regex

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 7, 2026

Claude Code Review

Commit: f8b04d5 | CI: running (several checks still pending)

Summary

Clean, well-scoped bug fix. The NATIONAL_ID regex minimum was 5 characters, rejecting legitimate UK energy industry short codes (DCC=3, GETW=4, SEPD=4) and breaking the nightly Demo Reset workflow since the PAYG tenant was added. Lowering the minimum to 2 is a reasonable fix -- it accommodates known short codes while still rejecting empty/single-character inputs.

Tests are thorough: boundary cases updated for both edges of the new range (AB at min, A below min), and a dedicated DCC case added to TestValidateExternalReference. This satisfies the stated requirement.

Risk Assessment

Area Level Detail
Blast radius Low Single regex in party domain validation
Rollback Safe No schema/data changes, pure code revert
Scale N/A Regex validation, no scaling concern
Cross-system Low Unblocks Demo Reset workflow; no API contract change
Migration N/A No migrations

Findings

No actionable findings. The change is minimal, correct, and well-tested.

Bot Review Notes

  • CodeRabbit flagged that existing TestParty_SetExternalReference_NationalID boundary tests might conflict with the new regex. Already addressed -- the second commit (f8b04d5) updated minimum length from AB123 to AB and too short from AB12 to A, correctly matching the new 2-20 constraint.

Previously Flagged

Severity Location Description Status
Suggestion party_test.go:527 Add boundary tests for new minimum Resolved (second commit added them)

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Low-risk fix with good test coverage. One minor suggestion for boundary tests in the summary comment.

Comment thread services/party/domain/party_test.go
coderabbitai[bot]
coderabbitai Bot previously requested changes Apr 7, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@services/party/domain/party_test.go`:
- Around line 520-525: The existing TestParty_SetExternalReference_NationalID
tests must be updated to match the relaxed regex ({2,20}) used for
ExternalReferenceTypeNationalID: locate the
TestParty_SetExternalReference_NationalID table entries for the cases named
"minimum length" (currently using reference "AB123") and "too short" (currently
using "AB12") and either (A) change the inputs to reflect true minimum/too-short
boundaries (e.g., set "minimum length" to a 2-char string and make "too short" a
1-char string) or (B) update their wantErr expectations so "AB12" no longer
expects an error; ensure any assertions that reference the reference value or
wantErr are adjusted accordingly so tests align with the new {2,20} rule for
SetExternalReference / ExternalReferenceTypeNationalID.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6da54b73-b3ee-4869-94d5-43666650f2a2

📥 Commits

Reviewing files that changed from the base of the PR and between 43dddc7 and 96f0d1d.

📒 Files selected for processing (2)
  • services/party/domain/party.go
  • services/party/domain/party_test.go

Comment thread services/party/domain/party_test.go
Update minimum length test to use 2-char reference and too-short test
to use 1-char reference, matching the new {2,20} constraint.
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

See summary comment. No actionable findings.

@bjcoombs bjcoombs dismissed coderabbitai[bot]’s stale review April 7, 2026 16:19

Stale CR - CodeRabbit re-reviewed and approved after boundary tests were updated

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@bjcoombs bjcoombs merged commit 03de43f into develop Apr 7, 2026
39 checks passed
@bjcoombs bjcoombs deleted the fix-national-id-regex branch April 7, 2026 16:52
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.

1 participant