3.0.11 — correct the transaction payload sent to Paystack - #72
Merged
Merged
Conversation
Math.ceil(grand_total * 100) overcharges by one subunit whenever the float
product lands just above the integer, which is common:
Math.ceil(8.21 * 100) === 822 // 821.0000000000001 -> 822
Math.ceil(1.10 * 100) === 111 // 110.00000000000001 -> 111
Math.round gives the correct subunit amount for every total. Verified that
PHP round($v * 100) and JS Math.round(v * 100) agree on every value across
two exhaustive sweeps (0-2000 at 0.001 steps, 100000-500000 at 0.07 steps,
zero divergences), including exact .5 cases such as 45.675 -> 4568 -- PHP
round() is half-away-from-zero, matching JS for positive values. So the
server-side amount check added later can compare against this without drift.
This is the inline/popup mirror of the redirect-flow fix in #70.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Magento\Sales\Model\Order has no getCurrency() method. Verified by reflection:
getCurrency declared: NO (magic __call -> getData)
getOrderCurrencyCode declared: YES in Magento\Sales\Model\Order
So the call fell through AbstractModel::__call to getData('currency'), and
sales_order has no `currency` column -- the payload carried currency: null on
every redirect-flow transaction. Paystack accepts a null currency silently and
substitutes the integration's own default:
{"amount":150000,"currency":null} -> {"status":true,"message":"Authorization URL created"}
On a store whose order currency differs from the Paystack integration default
that means the right number is charged in the wrong currency -- a base-NGN /
display-USD store sent a 12.50 USD order as amount 1250 with no currency and
was charged N12.50 instead of ~N19,000.
getOrderCurrencyCode() is the correct accessor, not getBaseCurrencyCode(): the
customer is charged in the display/quote currency, and the inline flow already
sends quote_currency_code, which is the same value.
BEHAVIOUR CHANGE: a merchant whose Paystack integration does not have their
store currency enabled will now see initialize fail with unsupported_currency
where it previously completed (in the wrong currency). That is the correct
outcome, but it is visible -- such merchants must enable the currency on their
Paystack integration.
The test stubbed getCurrency() on a mock of Order, which is how this hid: the
stub made a non-existent method look real. Under PHPUnit 10.5 that stub errors
outright once the suite is actually run, so both cases were dead. Retargeted to
getOrderCurrencyCode(), added a currency assertion to the initialize payload,
and swapped StoreInterface for the concrete Store model since StoreInterface
does not declare getBaseUrl(). Confirmed the test now fails if either the
currency fix or #70's integer-amount fix is reverted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Version bumped in all three places CLAUDE.md requires kept in sync: composer.json, etc/module.xml, README.md. CHANGELOG documents the three outbound-payload fixes and the one behaviour change merchants need to know about (a store currency not enabled on the Paystack integration now fails visibly rather than being charged in the default currency). Also corrected two stale CHANGELOG statements: the "since the last tag" preamble said v3.0.4, and the closing note said 3.0.5-3.0.10 were untagged when v3.0.10 was in fact tagged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…l totals Gate-2 diff review findings on the R1 changes. Setup.php: getOrderCurrencyCode() fixed the value but not the failure mode. Paystack accepts a null currency silently and substitutes the integration's own default, so an order with an empty currency code would still be charged in the wrong currency -- with a success response and no trace. Now throws ApiException before the API call, which the existing catch turns into order history plus the failure page. Reachable for order rows created outside normal checkout (data import, headless/third-party creation, partially migrated stores). SetupTest: the amount assertion used a grand total of 5000.00, whose *100 product is exactly representable, so it did not regression-test the defect #70 fixed -- (int)(19.99*100) truncates to 1998 and would have passed. Added a data provider covering the inexact totals (19.99, 8.21, 1.10, 0.29) and a case asserting initializeTransaction is never called when the order currency is missing. Confirmed both new cases fail when the respective fix is reverted. ConfigProviderTest: the same StoreInterface stub defect fixed in SetupTest was present here too (StoreInterface does not declare getBaseUrl(), so PHPUnit 10 refuses to configure it). Fixing only one of the two would have left the suite unable to run while claiming coverage. build-adobe-zip.sh: `rm -f "$ZIP"` only removed the zip for the version being built, leaving prior-version zips at the repo root to be uploaded to Adobe by mistake. Now removes every "${NAME}"-*.zip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release 3.0.11. Three corrections to what the module sends to Paystack. Deliberately scoped to the outbound payload — nothing here changes how a payment is verified or how an order advances, so none of it can reject a payment that succeeded. The verification gate is a separate release.
Builds on #70 (@iammcoding), which is already on
master.What was wrong
1. Redirect-mode checkout failed outright on many totals. The amount was
grandTotal * 100— a float. Paystack rejects a non-integer amount, confirmed against the live API:So this was never the 1-kobo undercharge #70's description guessed — it was a checkout the customer could not complete. Only inexact products break (19.99, 8.21, 1.10, 0.29); 5000.00 is fine, which is why it looked intermittent. Fixed by #70.
2. Inline mode overcharged by one subunit.
Math.ceil(8.21 * 100)is 822, not 821. NowMath.round.3. Redirect mode sent no currency at all.
Setup.phpcalled$order->getCurrency()— not a method onMagento\Sales\Model\Order:It fell through to
getData('currency');sales_orderhas no such column, so it returnednull. Paystack accepts a null currency silently, substituting the integration's default — so a base-NGN/display-USD store charged ₦12.50 for a $12.50 order with astatus:trueresponse and nothing in the logs. Now sendsgetOrderCurrencyCode()(notgetBaseCurrencyCode()— the customer is charged in the display currency, and the inline flow already sendsquote_currency_code).Also fails closed when the order currency is empty, rather than letting Paystack pick.
A merchant whose Paystack integration does not have their store currency enabled will now see
unsupported_currencywhere checkout previously completed in the wrong currency. That is the correct outcome, but it is visible — such merchants must enable their currency on their Paystack integration. Documented in the CHANGELOG upgrade note.Verified
Runtime, on
dev-repro/(Magento 2.4.9 / PHP 8.5.1 / CSP enforced, production mode) — not just unit tests:amount: 1999,currency: ZAR. Integer, and the order's own currency.amount=821. Same page showsMath.ceilwould have posted822.verify-checkout.jsCSP regression guard: PASS — 0 blocked CSP violations, 0 JS errors, popup opens.Unit suite: 12 errors → 7, 93 → 98 tests, 129 assertions. The 7 remaining are pre-existing and unrelated (observer tests stubbing magic accessors). Note the suite is not runnable from a fresh checkout and no CI runs it — that is why
SetupTesthad been failing since March; wiring it up is tracked separately, deliberately not coupled to this fix.New tests: a data provider over the inexact totals (19.99, 8.21, 1.10, 0.29) — the old assertion used 5000.00, whose
*100is exact, so it did not cover the bug — plus a case assertinginitializeTransactionis never called when the order currency is missing. Both confirmed to fail when the corresponding fix is reverted.Also in here
SetupTest/ConfigProviderTeststubbedgetCurrency()andStoreInterface::getBaseUrl(), neither of which exists. That stub is how defect 3 hid — a PHPUnit stub on a magic accessor makes a non-existent method look real.build-adobe-zip.shremoved only the current version's zip, leaving prior-version artifacts at the repo root to be uploaded to Adobe by mistake.Found while verifying, NOT fixed here
/paystack/payment/setupis invoked twice per redirect order, and I measured it rather than inferring it — temporary logging at the top ofexecute()and immediately beforeinitializeTransaction, one order attempt:Same reference both times. The first call initializes successfully; the second is rejected by Paystack as
duplicate_reference, and it is that second response which reaches the catch — so the customer lands oncheckout/onepage/failurewith "Duplicate Transaction Reference" in order history even though the transaction was created and is payable.The fact of the double invocation is measured; the cause is not yet known — I have not established what re-enters the route, and it should be diagnosed rather than guessed. Pre-existing and independent of this diff (none of these three lines affects invocation count); previously masked because the first call failed anyway. Filed for the next release.
🤖 Generated with Claude Code