Skip to content

[PM-27010] fix: Resolve tax ID type from country and value, with format guidance - #22388

Draft
amorask-bitwarden wants to merge 8 commits into
mainfrom
billing/PM-27010/users-unable-add-vat-id
Draft

[PM-27010] fix: Resolve tax ID type from country and value, with format guidance#22388
amorask-bitwarden wants to merge 8 commits into
mainfrom
billing/PM-27010/users-unable-add-vat-id

Conversation

@amorask-bitwarden

Copy link
Copy Markdown
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-27010

📔 Objective

The web client resolved a tax ID's Stripe type from the billing country alone, so it submitted the wrong type for countries with more than one format — e.g. a bare Canadian Business Number (987654321) was tagged ca_gst_hst, and UK VAT numbers were tagged eu_vat instead of gb_vat. It also offered the customer no format guidance.

This PR fixes it on the client:

  • Value-aware type resolution. getTaxIdTypeForCountry now derives the type from the country and the entered value, matching against per-type format regexes ported verbatim from the server's TaxService for the 19 multi-format countries. The submit path passes the entered value through.
  • Format guidance. The tax ID field shows a value-reactive hint: single-format countries show an example up front; multi-format countries confirm "Recognized format: {type}" once the entered value matches one. User-facing type names were cleaned up.
  • Stale-value hardening. The tax ID is cleared when the selected country stops supporting one, so a stale value can't be submitted under the wrong country.

For reviewers: the save-blocking symptom was already fixed server-side in #8122 (PM-40694) — please don't redo that server work. This PR is the client-side correctness + UX. The client's no-match fallback intentionally relies on that server-side re-derivation to stay correct, so don't disable it.

Testing

  • Unit tests: resolver spec (value-aware resolution, presence + round-trip guards, Brazil declaration-order, fallbacks) and component spec (hint states, submit mapping, stale-value reset).
  • Manual QA in the Web Vault: Canada (all six formats), United Kingdom (gb_vat incl. bare, eu_vat for XI), Spain, a single-format country (France), the switch-to-US reset, and an upgrade/checkout flow.

Follow-ups (out of scope)

  • Single source of truth for the tax-ID table — it's duplicated across clients, server, and billing-pricing (the root cause behind both PM-27010 and PM-40694).
  • Server logs the tax-ID value on one derivation path.
  • A buggy *-type.spec.ts Jest ignore pattern silently hides *-type.spec.ts specs repo-wide.

📸 Screenshots

The web client resolved a tax ID's Stripe type from the billing country
alone, so multi-format countries like Canada always resolved to the first
tax-impacting type and a bare value (e.g. a Canadian business number) was
tagged with the wrong type.

Add an optional per-type format regex, ported verbatim from the server's
TaxService, and make getTaxIdTypeForCountry match the entered value against
it, mirroring the server's resolution. First piece of PM-27010; the
submit-path wiring and format-guidance UX follow in later commits.
The format hint was an observable whose async pipe subscribes lazily inside the
field's visibility @if, so it missed the country change that revealed the field
and fell back to a stale value, leaking the US EIN example into Canada. Replace
it with a getter that reads the live form values, and exclude the US.

Reword the hint to "Recognized format: {type}" for multi-format countries
(driven by the type description) since it only appears once the entered value
matches a format; clean up terse and markdown type names that now surface.
@amorask-bitwarden amorask-bitwarden added ai-review Request a Claude code review t:bugfix Change Type - Bugfix labels Aug 12, 2026
@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: REQUEST CHANGES

Reviewed the value-aware tax ID type resolution in tax-id-type.ts, the hint/reset behavior in enter-billing-address.component.ts, the two new spec files, and the en locale changes. I hand-verified the format table: all 19 multi-entry countries define a format, every entry's example matches its own regex, and — accounting for declaration order — every multi-entry example resolves back to its own code, so the round-trip and coverage specs should hold. The bit-icon swap matches the existing class + title="{{ }}" pattern in organization-plans.component.html, and the tax-id-type.resolver.spec.ts filename correctly sidesteps the .*.type.spec.ts Jest ignore pattern. One finding on the tax ID reset.

Code Review Details
  • ⚠️ : Tax ID reset fires on the initial emission, dropping an existing tax ID for US addresses and tiers where scenario.supportsTaxId is false — getRawValue() then submits taxId: null where it previously round-tripped the value
    • apps/web/src/app/billing/payment/components/enter-billing-address.component.ts:217-220

Comment on lines 217 to 220
this.group.controls.taxId.disable();
// getRawValue() includes disabled controls, so clear the value rather than just disabling.
this.group.controls.taxId.reset();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ IMPORTANT: The reset also fires on the initial emission, so an existing tax ID is dropped without the user changing anything.

Details and fix

supportsTaxId$ is false for three different reasons: the country has no tax ID types, the country is US, or scenario.supportsTaxId is false (subscriber tier). Because of startWith(...), the subscription emits once during ngOnInit — after patchValue(scenario.existing) — so for an existing record whose country is US, or an org whose tier no longer supports tax IDs, the reset clears the patched value immediately.

Trace for EditBillingAddressDialogComponent with an existing us_ein on file:

  1. Dialog constructor + ngOnInit patch country: "US", taxId: "12-3456789".
  2. supportsTaxId$ emits false on startWith("US").
  3. disable() + reset() → control value is now null.
  4. User edits only the postal code and saves. getBillingAddressFromForm uses getRawValue()taxId: null is PUT to /address.

Before this PR step 3 only disabled the control, so getRawValue() round-tripped the existing value. The same applies to the async patchValue in trial-payment-dialog.component.ts:172 and organization-plans.component.ts:1227, which land after this subscription is live.

Scoping the reset to an actual country change keeps the stale-value protection without touching the initial state:

this.supportsTaxId$
  .pipe(pairwise(), takeUntil(this.destroy$))
  .subscribe(([previouslySupported, supportsTaxId]) => {
    if (supportsTaxId) {
      this.group.controls.taxId.enable();
    } else {
      this.group.controls.taxId.disable();
      // getRawValue() includes disabled controls, so clear the value rather than just disabling.
      if (previouslySupported) {
        this.group.controls.taxId.reset();
      }
    }
  });

(pairwise needs the initial enable()/disable() handled separately, or use startWith on the pair — either shape works as long as the first emission does not reset.)

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

Labels

ai-review Request a Claude code review t:bugfix Change Type - Bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant