[WOOTAX-19] Fix - Prevent duplicate tax rate rows for semicolon cities

[bartech]

Description

WooCommerce core treats a ; in a city value asymmetrically between storage and lookup:

• WC_Tax::_update_tax_rate_cities() explode(';', ...)s the input, treating ; as a multi-city separator — so a city like Casse;Berry is stored as two location rows (CASSE and BERRY).
• WC_Tax::find_rates() queries the city column as a single literal (location_code = 'CASSE;BERRY').

Because of this mismatch, find_rates() never matches the stored rows, so create_or_update_tax_rate() believes no rate exists and inserts a fresh wp_woocommerce_tax_rates row on every tax calculation — unbounded table growth for any store whose checkout city contains a semicolon (typo'd or pasted addresses).

This PR adds a normalize_city() helper that strips ; and collapses the resulting whitespace, applied at every point that feeds the tax-rate tables or the TaxJar API:

• the cart/checkout taxable-address entry,
• the admin order POST entry,
• the find_rates() lookup in create_or_update_tax_rate(),
• the _update_tax_rate_cities() storage call.

This restores round-trip symmetry: the value stored is the value looked up, so existing rows are reused instead of duplicated.

Related issue(s)

Closes WOOTAX-19

Steps to reproduce & screenshots/GIFs

Before this fix:

1. Configure TaxJar tax calculation for a US store.
2. Place (or recalculate) an order whose city contains a semicolon, e.g. Casse;Berry.
3. Inspect wp_woocommerce_tax_rates — each recalculation adds a new duplicate row, and tax lookups keep missing the existing rate.

After this fix:

1. Repeat the steps above.
2. The city is normalized to Casse Berry; the rate row is created once and reused on every subsequent calculation — no duplicate rows.

Automated coverage (run composer test):

• test_normalize_city_strips_semicolons_and_normalizes_whitespace — data-provider unit test for the helper (semicolons, leading/trailing/consecutive separators, whitespace).
• test_create_or_update_tax_rate_does_not_duplicate_rows_for_semicolon_city — DB round-trip regression test asserting two consecutive calls with Casse;Berry reuse the same tax_rate_id and add exactly one row.

Checklist

• unit tests

• changelog.txt entry added
• readme.txt entry added

[Abdalsalaam]

🤖 Automated AI pre-flight review — generated by Claude Code (ultra-review skill) on behalf of @Abdalsalaam.
This is NOT an approval and not a substitute for human review; it is an early automated pass to surface likely issues. Treat every finding as a suggestion to verify.

---

Findings

Blockers (must fix before merge)

None.

Majors (should fix before merge)

None.

Minors (nice to fix)

M1 · classes/class-wc-connect-taxjar-integration.php:1791 · correctness/i18n — The whitespace-collapse regex lacks the Unicode flag: preg_replace( '/\s+/', ' ', $city ). Without /u, \s matches only ASCII whitespace, so multibyte whitespace (NBSP U+00A0, ideographic space U+3000) introduced around a stripped ; won't collapse — a stored value could then differ from the looked-up one. Low impact (the ;→space str_replace is the real fix; ASCII whitespace already collapses).
Suggested fix: preg_replace( '/\s+/u', ' ', $city ). But see M4-companion: adding /u makes preg_replace able to return null on malformed UTF-8, so pair it with a null-safe return — return trim( (string) $city ); (or $city = preg_replace(...) ?? ''). As written today (no /u, short bounded input) the trim(null) path is effectively unreachable; it becomes real only if /u is added without the guard.

M2 · classes/class-wc-connect-taxjar-integration.php:703 · consistency — allow_street_address_for_matched_rates() performs the same find_rates() city lookup the PR is hardening ('city' => strtoupper( $city ), where $city comes from WC_Tax::get_tax_location() / the customer session) but was not updated with normalize_city(). For a ;-bearing customer city this secondary lookup stays asymmetric with the now-normalized stored rows. This does not reintroduce the unbounded-growth bug (it only reads, never inserts), so it's a consistency gap against the PR's "restore round-trip symmetry" goal rather than a regression.
Suggested fix: 'city' => strtoupper( self::normalize_city( $city ) ) for parity with :1855.

M3 · tests/php/test-class-wc-connect-taxjar-integration.php · coverage — The core path is well covered, but: the four production call sites are only exercised via create_or_update_tax_rate; get_address (:673) and get_backend_address (:920, which wraps strtoupper(wc_clean())) have no direct assertion, and the outbound TaxJar-API city mutation is untested — a regression that dropped normalize_city() from get_backend_address would not be caught. The ! is_string() early-return branch (reachable in production when city is false) and tab/newline whitespace are absent from the data provider.
Suggested fix: add a focused test invoking get_backend_address() with $_POST['city'] = 'Casse;Berry' asserting no ;; add null/false/tab/newline cases to the provider.

M4 · classes/class-wc-connect-taxjar-integration.php:1790 · silent failure — A city consisting only of separators (e.g. ";", "; ;") normalizes to ''. Such a value passes the ! empty() guards at :673/:920 before normalization, so a city the caller treated as present is silently discarded (rate row created/looked up with no city component) with no log entry. Garbage input, but undebuggable when it happens.
Suggested fix: _log() when normalization empties a previously non-empty city.

Nits (optional)

• N1 · tests/php/…: regression-test cleanup — The DB regression test inserts real woocommerce_tax_rates + _locations rows and never deletes them; it relies on WC_Unit_Test_Case's per-test transaction rollback. Robust in practice (the COUNT(*) is snapshotted before the calls, so pre-existing/leaked rows don't skew it), but not self-contained. Consider explicit cleanup of $first_id.
• N2 · tests/php/…:190 · phpcs comment — phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared sits on a query whose values are properly passed through $wpdb->prepare(); only the prefix-derived table name is interpolated (no injection risk). The ignore is mislabeled vs the genuine COUNT(*) cases at :176/:185. Optionally use the %i identifier placeholder (WP 6.2+).
• N3 · architecture — Three of the four normalization sites are redundant for the bug itself (the lookup+store pair suffices). This is intentional defense-in-depth and acceptable; noting it so reviewers know the breadth is deliberate, not accidental.

Follow-ups (candidates — none required to merge)

1. Apply M1 (/\s+/u + null-safe trim) and M2 (normalize the :703 sibling lookup) for full symmetry.
2. Add the M3 entry-point + non-string + tab/newline test cases.
3. (Optional) One-time cleanup script for stores already carrying duplicate ;-city tax-rate rows.

[CezaryDrewniak]

LGTM with minor suggestions. Verified the fix is correct and well-scoped.

The fix: Adds normalize_city() (strips ; and collapses whitespace) and applies it at every entry point that touches the tax-rate tables or TaxJar API — get_address(), get_backend_address(), allow_street_address_for_matched_rates(), and both arms of create_or_update_tax_rate(). The defensive double-normalization inside create_or_update_tax_rate() is the right call, it makes the storage/lookup contract local to that method.

Tests: The three new tests are well-designed. test_create_or_update_tax_rate_does_not_duplicate_rows_for_semicolon_city is a real DB round-trip regression test that snapshots the row count, asserts idempotency, and verifies the stored location_code is semicolon-free. The tab and newline collapse to space and null/false cases in the data provider are good defensive coverage.

Lint: No new PHPCS errors. (composer run check-all reports 1423 pre-existing errors in the codebase; the new normalize_city() and the five call sites are clean. The test file has 0 errors.)

Non-blockers to consider:

1. get_backend_address() normalizes after strtoupper (line 920); the find_rates call at line 1856 normalizes before. Functionally identical (since normalize_city only touches ; and whitespace), but worth aligning for consistency.
2. No direct test for the frontend get_address() path with a ; city — only the helper and backend path are exercised.
3. test_create_or_update_tax_rate_does_not_duplicate_rows_for_semicolon_city doesn't clean up the inserted tax rate in tear_down — one row will leak per test run. Minor

[Abdalsalaam]

LGTM! I tested it and it work as expected.