Skip to content

Commit 31f3ffb

Browse files
slash1andyslash1andy
committed
fix(amount-factory): address review — from_wc_order, tests, alias, str round-trip
- Fix from_wc_order() with same breakdown-derived total logic, preserving the free-trial $1.00 override. Removes the now-unnecessary get_total() call for non-free-trial orders. - Add number_format() string round-trip when converting integer cents back to a Money value, making the float→string path explicit and immune to IEEE 754 representation edge cases (e.g. 1001/100 = 10.009999...). - Replace inline FQN with StoreApiMoney import alias for readability. - Add unit tests: - testFromWcCartTotalAlwaysEqualsBreakdownSum() — parametrised invariant across 6 rounding scenarios - testFromWcCartTotalDerivesFromComponentsNotWcGetTotal() — proves get_total() is ignored when it would return a mismatched value - testFromStoreApiCart() — parametrised across 4 scenarios including fees and discount, verifies breakdown invariant and that fees are included in item_total - Remove stale get_total() shouldReceive() mocks from cart and order tests.
1 parent af1315a commit 31f3ffb

2 files changed

Lines changed: 191 additions & 48 deletions

File tree

‎modules/ppcp-api-client/src/Factory/AmountFactory.php‎

Lines changed: 28 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use WooCommerce\PayPalCommerce\ApiClient\Exception\RuntimeException;
1717
use WooCommerce\PayPalCommerce\ApiClient\Helper\CurrencyGetter;
1818
use WooCommerce\PayPalCommerce\WcGateway\StoreApi\Entity\CartTotals;
19+
use WooCommerce\PayPalCommerce\WcGateway\StoreApi\Entity\Money as StoreApiMoney;
1920
use WooCommerce\PayPalCommerce\WcSubscriptions\FreeTrialHandlerTrait;
2021
use WooCommerce\PayPalCommerce\WcGateway\Gateway\CardButtonGateway;
2122
use WooCommerce\PayPalCommerce\WcGateway\Gateway\CreditCardGateway;
@@ -92,11 +93,14 @@ public function from_wc_cart( \WC_Cart $cart ): Amount {
9293
// using get_total(), which can diverge from the component sum by ±$0.01
9394
// due to WooCommerce per-item tax rounding. PayPal requires amount.value to
9495
// exactly equal the sum of its breakdown fields or it rejects the PATCH.
96+
// Formatting through a string avoids floating-point representation issues
97+
// when converting the integer-cent sum back to a decimal (e.g. 1001/100).
9598
$total_cents = (int) round( $item_total_val * 100 )
9699
+ (int) round( $shipping_val * 100 )
97100
+ (int) round( $taxes_val * 100 )
98101
- (int) round( $discount_val * 100 );
99-
$total = new Money( $total_cents / 100, $this->currency->get() );
102+
$total_str = number_format( $total_cents / 100, 2, '.', '' );
103+
$total = new Money( (float) $total_str, $this->currency->get() );
100104

101105
$breakdown = new AmountBreakdown(
102106
$item_total,
@@ -133,11 +137,7 @@ public function from_store_api_cart( CartTotals $cart_totals ): Amount {
133137
$currency = $cart_totals->total_price()->currency_code();
134138
$minor_unit = $cart_totals->total_price()->currency_minor_unit();
135139
$make = static function ( int $minor ) use ( $currency, $minor_unit ): Money {
136-
return ( new \WooCommerce\PayPalCommerce\WcGateway\StoreApi\Entity\Money(
137-
(string) $minor,
138-
$currency,
139-
$minor_unit
140-
) )->to_paypal();
140+
return ( new StoreApiMoney( (string) $minor, $currency, $minor_unit ) )->to_paypal();
141141
};
142142

143143
return new Amount(
@@ -171,37 +171,37 @@ public function from_wc_order( \WC_Order $order ): Amount {
171171
$this->discounts_from_items( $items ),
172172
)
173173
);
174-
$discount = null;
174+
$discount = null;
175175
if ( $discount_value ) {
176-
$discount = new Money(
177-
(float) $discount_value,
178-
$currency
179-
);
176+
$discount = new Money( (float) $discount_value, $currency );
180177
}
181178

182-
$total_value = (float) $order->get_total();
179+
$item_total_val = (float) $order->get_subtotal() + (float) $order->get_total_fees();
180+
$shipping_val = (float) $order->get_shipping_total();
181+
$taxes_val = (float) $order->get_total_tax();
182+
183+
$item_total = new Money( $item_total_val, $currency );
184+
$shipping = new Money( $shipping_val, $currency );
185+
$taxes = new Money( $taxes_val, $currency );
186+
187+
// Free trial orders charge a fixed $1.00 regardless of cart contents —
188+
// preserve that override. For all other orders derive the total from
189+
// breakdown components so amount.value always equals the breakdown sum.
183190
if ( (
184191
in_array( $order->get_payment_method(), array( CreditCardGateway::ID, CardButtonGateway::ID ), true )
185192
|| ( PayPalGateway::ID === $order->get_payment_method() && 'card' === $order->get_meta( PayPalGateway::ORDER_PAYMENT_SOURCE_META_KEY ) )
186193
)
187194
&& $this->is_free_trial_order( $order )
188195
) {
189-
$total_value = 1.0;
196+
$total = new Money( 1.0, $currency );
197+
} else {
198+
$total_cents = (int) round( $item_total_val * 100 )
199+
+ (int) round( $shipping_val * 100 )
200+
+ (int) round( $taxes_val * 100 )
201+
- (int) round( $discount_value * 100 );
202+
$total_str = number_format( $total_cents / 100, 2, '.', '' );
203+
$total = new Money( (float) $total_str, $currency );
190204
}
191-
$total = new Money( $total_value, $currency );
192-
193-
$item_total = new Money(
194-
(float) $order->get_subtotal() + (float) $order->get_total_fees(),
195-
$currency
196-
);
197-
$shipping = new Money(
198-
(float) $order->get_shipping_total(),
199-
$currency
200-
);
201-
$taxes = new Money(
202-
(float) $order->get_total_tax(),
203-
$currency
204-
);
205205

206206
$breakdown = new AmountBreakdown(
207207
$item_total,
@@ -212,11 +212,7 @@ public function from_wc_order( \WC_Order $order ): Amount {
212212
null, // shipping discounts?
213213
$discount
214214
);
215-
$amount = new Amount(
216-
$total,
217-
$breakdown
218-
);
219-
return $amount;
215+
return new Amount( $total, $breakdown );
220216
}
221217

222218
/**

‎tests/PHPUnit/ApiClient/Factory/AmountFactoryTest.php‎

Lines changed: 163 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010
use WooCommerce\PayPalCommerce\TestCase;
1111
use Mockery;
1212
use WooCommerce\PayPalCommerce\WcGateway\Gateway\PayPalGateway;
13+
use WooCommerce\PayPalCommerce\WcGateway\StoreApi\Entity\CartTotals;
14+
use WooCommerce\PayPalCommerce\WcGateway\StoreApi\Entity\Money as StoreApiMoney;
1315
use function Brain\Monkey\Functions\expect;
1416
use function Brain\Monkey\Functions\when;
1517

@@ -35,10 +37,6 @@ public function setUp(): void
3537
public function testFromWcCartDefault()
3638
{
3739
$cart = Mockery::mock(\WC_Cart::class);
38-
$cart
39-
->shouldReceive('get_total')
40-
->withAnyArgs()
41-
->andReturn(13);
4240
$cart
4341
->shouldReceive('get_subtotal')
4442
->andReturn(5);
@@ -78,12 +76,8 @@ public function testFromWcCartNoDiscount()
7876
{
7977
$cart = Mockery::mock(\WC_Cart::class);
8078
$cart
81-
->shouldReceive('get_total')
82-
->withAnyArgs()
83-
->andReturn(11);
84-
$cart
85-
->shouldReceive('get_subtotal')
86-
->andReturn(5);
79+
->shouldReceive('get_subtotal')
80+
->andReturn(5);
8781
$cart
8882
->shouldReceive('get_fee_total')
8983
->andReturn(0);
@@ -110,6 +104,165 @@ public function testFromWcCartNoDiscount()
110104
$this->assertEquals((float) 4, $result->breakdown()->tax_total()->value());
111105
}
112106

107+
/**
108+
* The critical invariant: amount.value must always equal the formatted sum of
109+
* its breakdown parts. This verifies the fix for the FCI PATCH rejection that
110+
* occurred when WC per-item tax rounding caused get_total() to diverge by ±$0.01.
111+
*
112+
* @dataProvider dataBreakdownRoundingCases
113+
*/
114+
public function testFromWcCartTotalAlwaysEqualsBreakdownSum(
115+
float $subtotal,
116+
float $fees,
117+
float $shipping,
118+
float $tax,
119+
float $discount
120+
): void {
121+
$cart = Mockery::mock( \WC_Cart::class );
122+
$cart->shouldReceive( 'get_subtotal' )->andReturn( $subtotal );
123+
$cart->shouldReceive( 'get_fee_total' )->andReturn( $fees );
124+
$cart->shouldReceive( 'get_shipping_total' )->andReturn( $shipping );
125+
$cart->shouldReceive( 'get_total_tax' )->andReturn( $tax );
126+
$cart->shouldReceive( 'get_discount_total' )->andReturn( $discount );
127+
128+
$woocommerce = Mockery::mock( \WooCommerce::class );
129+
$session = Mockery::mock( \WC_Session::class );
130+
$woocommerce->session = $session;
131+
when( 'WC' )->justReturn( $woocommerce );
132+
$session->shouldReceive( 'get' )->andReturn( [] );
133+
134+
$result = $this->testee->from_wc_cart( $cart );
135+
$breakdown = $result->breakdown();
136+
137+
$sum = (float) $breakdown->item_total()->value_str()
138+
+ (float) $breakdown->shipping()->value_str()
139+
+ (float) $breakdown->tax_total()->value_str();
140+
if ( $breakdown->discount() ) {
141+
$sum -= (float) $breakdown->discount()->value_str();
142+
}
143+
$expected_total_str = number_format( $sum, 2, '.', '' );
144+
145+
$this->assertSame(
146+
$expected_total_str,
147+
$result->value_str(),
148+
"amount.value_str() must equal the formatted breakdown sum (PayPal PATCH invariant)"
149+
);
150+
}
151+
152+
public function dataBreakdownRoundingCases(): array
153+
{
154+
return [
155+
// Classic WC tax-rounding edge case: 8% tax on $13.33 = $1.0664.
156+
// WC rounds to $1.07 for tax but get_total() may accumulate differently.
157+
'tax_rounding_edge_case' => [ 13.33, 0.0, 5.00, 1.07, 0.0 ],
158+
// Three items at $3.336 each: item total rounds differently at cart vs item level.
159+
'multi_item_tax_rounding' => [ 10.01, 0.0, 4.99, 1.50, 0.0 ],
160+
// Discount present; all components interact.
161+
'with_discount' => [ 20.00, 2.00, 3.50, 2.25, 1.75 ],
162+
// Fees and no discount.
163+
'with_fees_no_discount' => [ 15.50, 4.50, 6.00, 2.60, 0.0 ],
164+
// Large order with all components.
165+
'large_order' => [ 199.99, 10.00, 12.95, 22.30, 15.00 ],
166+
// Zero-value shipping (digital goods).
167+
'digital_no_shipping' => [ 49.99, 0.0, 0.0, 4.50, 0.0 ],
168+
];
169+
}
170+
171+
/**
172+
* Proves that the total is derived from breakdown components, not get_total().
173+
* When WC's get_total() is off by 1 cent, the PATCH total must still match
174+
* the breakdown so PayPal accepts it.
175+
*/
176+
public function testFromWcCartTotalDerivesFromComponentsNotWcGetTotal(): void
177+
{
178+
$cart = Mockery::mock( \WC_Cart::class );
179+
// Components sum to $19.40 (1333 + 500 + 107 = 1940 cents).
180+
$cart->shouldReceive( 'get_subtotal' )->andReturn( 13.33 );
181+
$cart->shouldReceive( 'get_fee_total' )->andReturn( 0.0 );
182+
$cart->shouldReceive( 'get_shipping_total' )->andReturn( 5.00 );
183+
$cart->shouldReceive( 'get_total_tax' )->andReturn( 1.07 );
184+
$cart->shouldReceive( 'get_discount_total' )->andReturn( 0.0 );
185+
// get_total() deliberately returns a mismatched value to prove it is not used.
186+
$cart->shouldReceive( 'get_total' )->withAnyArgs()->andReturn( 19.39 );
187+
188+
$woocommerce = Mockery::mock( \WooCommerce::class );
189+
$session = Mockery::mock( \WC_Session::class );
190+
$woocommerce->session = $session;
191+
when( 'WC' )->justReturn( $woocommerce );
192+
$session->shouldReceive( 'get' )->andReturn( [] );
193+
194+
$result = $this->testee->from_wc_cart( $cart );
195+
196+
// Total must be $19.40 (from breakdown), not $19.39 (from get_total()).
197+
$this->assertSame( '19.40', $result->value_str() );
198+
$this->assertSame( '13.33', $result->breakdown()->item_total()->value_str() );
199+
$this->assertSame( '5.00', $result->breakdown()->shipping()->value_str() );
200+
$this->assertSame( '1.07', $result->breakdown()->tax_total()->value_str() );
201+
}
202+
203+
/**
204+
* @dataProvider dataFromStoreApiCart
205+
*/
206+
public function testFromStoreApiCart(
207+
int $items,
208+
int $fees,
209+
int $shipping,
210+
int $tax,
211+
int $discount,
212+
string $expected_total,
213+
string $expected_items,
214+
string $expected_shipping,
215+
string $expected_tax,
216+
?string $expected_discount
217+
): void {
218+
$currency = 'USD';
219+
$minor_unit = 2;
220+
$make = fn( int $v ) => new StoreApiMoney( (string) $v, $currency, $minor_unit );
221+
222+
$totals = Mockery::mock( CartTotals::class );
223+
$totals->shouldReceive( 'total_items' )->andReturn( $make( $items ) );
224+
$totals->shouldReceive( 'total_fees' )->andReturn( $make( $fees ) );
225+
$totals->shouldReceive( 'total_shipping' )->andReturn( $make( $shipping ) );
226+
$totals->shouldReceive( 'total_tax' )->andReturn( $make( $tax ) );
227+
$totals->shouldReceive( 'total_discount' )->andReturn( $make( $discount ) );
228+
$totals->shouldReceive( 'total_price' )->andReturn( $make( 0 ) ); // never used for value
229+
230+
$result = $this->testee->from_store_api_cart( $totals );
231+
$breakdown = $result->breakdown();
232+
233+
$this->assertSame( $expected_total, $result->value_str(), 'amount.value_str' );
234+
$this->assertSame( $expected_items, $breakdown->item_total()->value_str(), 'item_total includes fees' );
235+
$this->assertSame( $expected_shipping, $breakdown->shipping()->value_str(), 'shipping' );
236+
$this->assertSame( $expected_tax, $breakdown->tax_total()->value_str(), 'tax_total' );
237+
if ( null === $expected_discount ) {
238+
$this->assertNull( $breakdown->discount(), 'discount should be null' );
239+
} else {
240+
$this->assertSame( $expected_discount, $breakdown->discount()->value_str(), 'discount' );
241+
}
242+
// Core invariant: value must equal breakdown sum.
243+
$sum = (float) $breakdown->item_total()->value_str()
244+
+ (float) $breakdown->shipping()->value_str()
245+
+ (float) $breakdown->tax_total()->value_str();
246+
if ( $breakdown->discount() ) {
247+
$sum -= (float) $breakdown->discount()->value_str();
248+
}
249+
$this->assertSame( $result->value_str(), number_format( $sum, 2, '.', '' ), 'breakdown invariant' );
250+
}
251+
252+
public function dataFromStoreApiCart(): array
253+
{
254+
return [
255+
// items=1000¢, fees=0, shipping=500¢, tax=180¢, discount=0 → total=1680¢=$16.80
256+
'no_fees_no_discount' => [ 1000, 0, 500, 180, 0, '16.80', '10.00', '5.00', '1.80', null ],
257+
// Fees included in item_total: items=1000¢ + fees=200¢ = 1200¢
258+
'with_fees' => [ 1000, 200, 500, 180, 0, '18.80', '12.00', '5.00', '1.80', null ],
259+
// Discount reduces total: items=2000¢ + fees=0, shipping=500¢, tax=225¢, discount=150¢
260+
'with_discount' => [ 2000, 0, 500, 225, 150, '25.75', '20.00', '5.00', '2.25', '1.50' ],
261+
// Fees + discount together.
262+
'fees_and_discount' => [ 1500, 300, 700, 250, 100, '26.50', '18.00', '7.00', '2.50', '1.00' ],
263+
];
264+
}
265+
113266
public function testFromWcOrderDefault()
114267
{
115268
$order = Mockery::mock(\WC_Order::class);
@@ -134,9 +287,6 @@ public function testFromWcOrderDefault()
134287
->shouldReceive('get_payment_method')
135288
->andReturn(PayPalGateway::ID);
136289

137-
$order
138-
->shouldReceive('get_total')
139-
->andReturn(10);
140290
$order
141291
->shouldReceive('get_subtotal')
142292
->andReturn(6);
@@ -195,9 +345,6 @@ public function testFromWcOrderDiscountIsNull()
195345
->shouldReceive('get_payment_method')
196346
->andReturn(PayPalGateway::ID);
197347

198-
$order
199-
->shouldReceive('get_total')
200-
->andReturn(100);
201348
$order
202349
->shouldReceive('get_subtotal')
203350
->andReturn(6);

0 commit comments

Comments
 (0)