ABN-298: Extended net terms, surcharge config, and dev tooling#82
ABN-298: Extended net terms, surcharge config, and dev tooling#82dgjlindsay wants to merge 6 commits intomainfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No critical issues found. The currency-aware surcharge logic is sound: fixed fees and limits are converted via Magento's exchange rate API, missing rates throw a LocalizedException (which Magento's checkout layer surfaces as a user-facing error), and the null vs 0 limit distinction is correctly handled end-to-end. Test coverage covers same-currency, cross-currency, zero-limit, legacy-empty-currency, and conversion-failure paths. Ready for human approval.
There was a problem hiding this comment.
Re-reviewed after latest commit. No new critical issues found, but the base-currency surcharge bug flagged in the inline thread on Model/Total/Surcharge.php remains unresolved — setBaseGrandTotal and setBaseTaxAmount are still populated with quote-currency values. Recommend holding merge until that is fixed.
There was a problem hiding this comment.
Re-reviewed after latest commit (22efeba). The base-currency surcharge bug on Model/Total/Surcharge.php (inline thread) remains unresolved — setBaseGrandTotal and setBaseTaxAmount still receive quote-currency values. No new critical issues found in the rest of the diff. Recommend holding merge until the base-currency fix is applied.
There was a problem hiding this comment.
Re-reviewed after latest commit (e2d51d2). The base-currency surcharge bug on Model/Total/Surcharge.php (inline thread) remains unresolved — setBaseGrandTotal and setBaseTaxAmount still receive quote-currency values. No new critical issues found in the rest of the diff. Recommend holding merge until the base-currency fix is applied.
dgjlindsay
force-pushed
the
doug/ABN-298-extended-net-terms
branch
from
0554aec to
463e257
Compare
There was a problem hiding this comment.
Re-reviewed after latest commit (0ed6ada). The previously flagged base-currency bug is now fixed: Model/Total/Surcharge.php correctly divides $grossAmount and $taxAmount by getBaseToQuoteRate() before passing values to setBaseGrandTotal and setBaseTaxAmount. The inline thread on that file has been resolved. No new critical issues found. Ready for human approval.
dgjlindsay
force-pushed
the
doug/ABN-298-extended-net-terms
branch
from
0ed6ada to
dd06c1d
Compare
There was a problem hiding this comment.
Re-reviewed after latest commit (dd06c1d). The previously flagged base-currency bug on Model/Total/Surcharge.php remains fixed. The new changes — admin payment-terms checkbox field (replacing multiselect), fee-response caching in SurchargeCalculator, the frontend chip selector JS model, and TaxCalculation stub refactor for tests — introduce no critical issues. Ready for human approval.
dgjlindsay
force-pushed
the
doug/ABN-298-extended-net-terms
branch
from
da34873 to
26b26b7
Compare
There was a problem hiding this comment.
Re-reviewed after latest commit (26b26b7). The previously flagged LocalizedException swallow on Model/Total/Surcharge.php line 129 remains unresolved — a missing exchange rate silently drops the surcharge instead of rejecting the order. No new critical issues found in the rest of the diff. Recommend holding merge until that catch block is fixed.
There was a problem hiding this comment.
Re-reviewed after latest commit (26b26b7). No new critical issues found. The open inline thread on Model/Total/Surcharge.php (LocalizedException swallowed in catch block) remains unresolved — recommend holding merge until that is addressed.
Adds support for configurable payment terms (14/30/60/90 days + custom duration), per-term merchant surcharges, and buyer-facing term selection in checkout. Admin config: - Payment terms via individual checkboxes (comma-separated storage) - Surcharge strategy: fixed, percentage, fixed+percentage, differential - Per-term surcharge grid with currency-aware fixed fees - Surcharge tax rate configurable (falls back to default product tax rate) Checkout flow: - Term selector chips with real-time surcharge preview - Magento Total Collector adds surcharge to grand total (net + tax) - Surcharge tax flows into Magento's native Tax line - REST endpoint recalculates totals on term change - Base-currency conversion applied to base_grand_total / base_tax_amount Two API integration: - BUYER_FEE line item type for surcharge - Pricing API /v1/pricing/order/fee drives merchant fee lookup - In-memory fee cache per-request to avoid redundant API hits Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dgjlindsay
force-pushed
the
doug/ABN-298-extended-net-terms
branch
from
26b26b7 to
6cb0a73
Compare
There was a problem hiding this comment.
Re-reviewed after latest commit (6cb0a73). Both previously flagged issues are now resolved: the base-currency division (getBaseToQuoteRate()) is in place, and LocalizedException is re-thrown rather than swallowed. No new critical issues found. Ready for human approval.
- Relocate term chips to top of payment method content (above redirect message and form fields) so buyers see term options immediately after picking the payment method. - Filter empty customAttributes in new-customer-address to remove phantom blank lines below billing address details. - Wrap surcharge segment title in Phrase so TotalsConverter's is_object guard passes and the dynamic title reaches the order summary. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ment - Compose surcharge description as "<configured title> - <N> days" to match ABN fork convention, matching buyer expectations of which term the fee corresponds to. - In surcharge.js, sync the selected term's chip value from the authoritative two_surcharge segment on every totals update. Fixes intermittent chip/summary mismatch on page load when the server-side ConfigProvider computed from stale session data before collectTotals ran against fresh quote state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Plugin now maps merchant surcharge config to a buyer_fee_share object on POST /v1/pricing/order/fee. API applies percentage, fixed, cap, and differential (via reference_terms) and returns the authoritative buyer fee. Plugin only FX-converts merchant-config amounts (fixed, cap) into order currency before send. - Removed: delta math, ceil rounding, threshold comparisons - Added: buildBuyerFeeShare() payload mapping - Kept: feeCache (chip precompute still hits API N times per render), FX of merchant-config amounts, type=NONE and differential-default short-circuits Requires checkout-api hotfix/ABN-319-buyer-fee-surcharge-cap live in every env the plugin talks to. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Re-reviewed after latest commits (since 6cb0a73). The previously fixed bugs remain in place. The new additions — checkout surcharge summary component/template, sidebar totals registration, and the new-customer-address-mixin (filters null/empty custom attributes) — introduce no critical issues. No new critical issues found. Ready for human approval.
Magento 2 ships English only — core checkout strings like "Order Summary", "Cart Subtotal", "Ship To:" fall back to English unless a language pack is installed. QA flagged this on Dutch. Bake the five community-engineering language packs into the install step so every fresh dev container renders localised core strings out of the box. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Re-reviewed after latest commit (77e24ed). Both previously flagged bugs (base-currency division and LocalizedException re-throw in Model/Total/Surcharge.php) remain fixed. No new critical issues found in the updated diff — the new additions (chip selector template, surcharge summary sidebar registration, TermSelection webapi, surcharge.js model, and new-customer-address-mixin) are all sound. Ready for human approval.
dgjlindsay
force-pushed
the
doug/ABN-298-extended-net-terms
branch
from
6ad84ad to
5e40ba3
Compare
Adds a read-only "Fee" column to the admin surcharge configuration grid
so merchants can see what Two charges them (fixed + percentage) alongside
what they charge the buyer. Fetched async from the Two API.
- New admin controller Two\Gateway\Controller\Adminhtml\Config\Fees
proxies POST /pricing/v1/merchant/rates server-side so the API key stays
in the backend. Resolves scope (default / website / store) to a storeId,
then FX-converts the response's fixed_fee values into the grid's base
currency. On upstream or FX failure the response is {success: false} and
the JS leaves dashes in the cells — the admin page never breaks on a
Two API outage.
- Service/Api/Adapter::execute() takes an optional ?int $storeId so the
scope-resolved API key and mode are used when the controller fires.
- Column header currency suffixes removed in favour of a single footnote
below the grid ("Amounts are shown in X.") — cleaner when the Fee
column joins Fixed / Limit.
- Bugfix: in differential mode the default-term row is disabled but was
still displaying any previously-saved non-zero values. The row now
zeroes on the client while differential is active (API short-circuits
that row regardless). Original values are snapshotted per input so
toggling differential off — or picking a different default term —
before saving restores them.
- payout_schedule is intentionally omitted from the request — the server
infers it from the merchant's payee accounts. recourse_pricing is
hardcoded false pending a matching admin config field.
buyer_country_code falls back to the Magento store's base country.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dgjlindsay
force-pushed
the
doug/ABN-298-extended-net-terms
branch
from
5e40ba3 to
60b5994
Compare
1 participant
Summary
Test plan
make test)🤖 Generated with Claude Code