Skip to content

[WOOTAX-322] Refactor - Hoist the canonical-id exemption remap #2990

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bartech merged 3 commits into from
Aug 11, 2026
Merged

[WOOTAX-322] Refactor - Hoist the canonical-id exemption remap #2990

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1dc1d31
[WOOTAX-322] Refactor - Hoist the canonical-id exemption remap into t…
bartech Aug 7, 2026
1f01aa1
[WOOTAX-322] Test - Pin exemption-map accumulation at the canonical-i…
bartech Aug 8, 2026
d58221d
Merge remote-tracking branch 'origin/trunk' into wootax-322
bartech Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 21 additions & 23 deletions classes/class-wc-connect-taxjar-integration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1065,19 +1065,9 @@ protected function get_line_items( $wc_cart_object ) {
);
}

// Re-keys $non_taxable_line_items onto the canonical ids as it goes.
$line_items = $this->assign_canonical_line_item_ids( $line_items );

// The exempt record was keyed while ids were still context-specific; move it onto
// the canonical ids, which is what get_itemized_tax_rates() looks up.
$non_taxable = array();
foreach ( array_keys( $this->non_taxable_line_items ) as $legacy_key ) {
$item_key = substr( $legacy_key, (int) strpos( $legacy_key, '-' ) + 1 );
if ( isset( $line_items[ $item_key ] ) ) {
$non_taxable[ $line_items[ $item_key ]['id'] ] = true;
}
}
$this->non_taxable_line_items = $non_taxable;

return $line_items;
}

Expand Down Expand Up @@ -1150,19 +1140,9 @@ protected function get_backend_line_items( $order ) {
}
}

// Re-keys $non_taxable_line_items onto the canonical ids as it goes.
$line_items = $this->assign_canonical_line_item_ids( $line_items );

// The exempt record was keyed while ids were still context-specific; move it onto
// the canonical ids, which is what get_itemized_tax_rates() looks up.
$non_taxable = array();
foreach ( array_keys( $this->non_taxable_line_items ) as $legacy_key ) {
$item_key = substr( $legacy_key, (int) strpos( $legacy_key, '-' ) + 1 );
if ( isset( $line_items[ $item_key ] ) ) {
$non_taxable[ $line_items[ $item_key ]['id'] ] = true;
}
}
$this->non_taxable_line_items = $non_taxable;

return $line_items;
}

Expand Down Expand Up @@ -1192,6 +1172,15 @@ protected function get_line_item( $id, $line_items ) {
* identical lines — an order can legitimately hold the same product twice at the
* same price — distinct, so neither loses its rate.
*
* Rewriting the ids invalidates `$non_taxable_line_items`, which callers record
* while ids are still context-specific but `get_itemized_tax_rates()` reads back
* under the canonical id. So this method re-keys that map itself rather than
* leaving each caller to repair it afterwards: the map is never observable in the
* stale keying, and a future caller inherits the invariant instead of having to
* remember it. Callers reset the map before building, so replacing it wholesale
* here is safe — an entry whose line item did not survive into $line_items (the
* order path skips zero-priced lines) is dropped, exactly as before.
*
* @since 3.6.12
*
* @param array $line_items Line items whose 'id' is currently the bare product ID.
Expand All @@ -1200,6 +1189,7 @@ protected function get_line_item( $id, $line_items ) {
*/
private function assign_canonical_line_item_ids( array $line_items ): array {
$occurrences = array();
$non_taxable = array();

foreach ( $line_items as $key => $line_item ) {
$product_id = $line_item['id'];
Expand All @@ -1220,9 +1210,17 @@ private function assign_canonical_line_item_ids( array $line_items ): array {

$occurrences[ $fingerprint ] = isset( $occurrences[ $fingerprint ] ) ? $occurrences[ $fingerprint ] + 1 : 0;

$line_items[ $key ]['id'] = $product_id . '-' . substr( $fingerprint, 0, 12 ) . '-' . $occurrences[ $fingerprint ];
$canonical_id = $product_id . '-' . substr( $fingerprint, 0, 12 ) . '-' . $occurrences[ $fingerprint ];

if ( isset( $this->non_taxable_line_items[ $product_id . '-' . $key ] ) ) {
$non_taxable[ $canonical_id ] = true;
}

$line_items[ $key ]['id'] = $canonical_id;
}

$this->non_taxable_line_items = $non_taxable;

return $line_items;
}

Expand Down
81 changes: 81 additions & 0 deletions tests/php/test-class-wc-connect-taxjar-integration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4139,6 +4139,87 @@ public function test_canonical_line_item_id_keeps_product_id_prefix() {
$this->assertSame( 0, strpos( $item['id'], $product->get_id() . '-' ) );
}

/**
* Rewriting the ids is what invalidates `$non_taxable_line_items`, so re-keying it
* belongs to the method that does the rewriting — not to each caller afterwards.
*
* Called directly here, with no caller involved, precisely so a future third caller
* of `assign_canonical_line_item_ids()` inherits the remap instead of having to
* remember it. Forgetting it has already caused one silent regression: the map
* stayed keyed by the pre-canonical id while `get_itemized_tax_rates()` looked up
* the canonical one, so the non-taxable guard never fired and no test failed.
*/
public function test_assign_canonical_line_item_ids_rekeys_non_taxable_map() {
$line_items = array(
'cart_key_exempt' => array(
'id' => 101,
'product_tax_code' => '99999',
'quantity' => 1,
'unit_price' => '10.00',
'discount' => '0.00',
'tax_location' => 'shipping',
),
'cart_key_taxable' => array(
'id' => 202,
'product_tax_code' => '',
'quantity' => 1,
'unit_price' => '20.00',
'discount' => '0.00',
'tax_location' => 'shipping',
),
);

// Keyed the way both callers record it: "<product_id>-<context-specific key>".
$this->set_private_property( 'non_taxable_line_items', array( '101-cart_key_exempt' => true ) );

$result = $this->invoke_protected_method( 'assign_canonical_line_item_ids', array( $line_items ) );
$recorded = $this->get_private_property( 'non_taxable_line_items' );

$this->assertSame(
array( $result['cart_key_exempt']['id'] => true ),
$recorded,
'The helper must leave the map keyed by canonical id, since that is what get_itemized_tax_rates() looks up.'
);
$this->assertArrayNotHasKey(
'101-cart_key_exempt',
$recorded,
'The pre-canonical key must not survive, or the stale entry outlives the id it described.'
);
Comment thread
Abdalsalaam marked this conversation as resolved.
}

/**
* Replacing the map wholesale is only safe because an entry with no surviving line
* item is meant to be dropped.
*
* `get_backend_line_items()` records the exempt status before the `if ( $unit_price )`
* guard decides whether the line is sent at all, so a zero-priced exempt item is
* recorded and then omitted. It has no canonical id to be re-keyed onto, and TaxJar
* never echoes a breakdown line for it, so carrying it forward would be dead state.
*/
public function test_assign_canonical_line_item_ids_drops_non_taxable_entries_without_a_line_item() {
$line_items = array(
'item_1' => array(
'id' => 101,
'product_tax_code' => '',
'quantity' => 1,
'unit_price' => '10.00',
'discount' => '0.00',
'tax_location' => 'shipping',
),
);

// 303 was recorded as exempt but never made it into $line_items.
$this->set_private_property( 'non_taxable_line_items', array( '303-item_2' => true ) );

$this->invoke_protected_method( 'assign_canonical_line_item_ids', array( $line_items ) );

$this->assertSame(
array(),
$this->get_private_property( 'non_taxable_line_items' ),
'An exempt record with no line item to re-key onto must be dropped, not carried forward.'
);
}

/**
* Build a representative TaxJar request body for cache-signature tests.
*
Expand Down
Loading