TWO-24485: rewrite plugin as brand-aware Two_Gateway module#109
Conversation
Imports the cleaned Two_Gateway runtime from the consolidated private branch. The module is brand-aware via Two\Gateway\Api\BrandRegistryInterface, with a default DI binding to Two\Gateway\Brand\TwoBrand. Downstream brand-overlay packages may rebind this interface via their own DI preference. Bumps to 2.0.0-rc.1 to signal the architectural change. See internal ticket for the full design rationale. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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}
Magento dev mode has a long-standing requirejs/mixins-plugin race that keeps the admin grid loading spinner visible for ~100s+ after page load, even though grid data renders correctly underneath. The root cause is in vendor (Magento_RequireJs) and has no clean local fix. This adds a make-install-time script that creates a tiny local-only module (Local_AdminLoaderHide) shipping a single CSS rule to hide the overlay. Local dev only — only runs from the Makefile install target, never affects the shipped Two_Gateway plugin. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Re-reviewed after the second commit (8c88daa).
The new commit adds surcharge block wiring in admin layouts, brand-aware getProductName() substitutions, null-safe maxFixed handling in the surcharge grid, and switches SalesOrderShipmentAfter from a singleton Transaction to TransactionFactory — all correct, no new critical issues.
The two previously flagged issues remain open and unaddressed (see inline threads):
Model/Config/Source/PaymentTermsDurationDays.php— hardcoded 30-day option instead of delegating toBrandRegistryInterface::getAvailablePaymentTerms().github/workflows/release.yml— barepushtrigger has no CI gate; any commit tomaincan cut a release regardless of test status
…present KO subscribables don't replay. On slow stacks the surcharge model can mount after Magento's /totals-information has already fired and called quote.setTotals(), so the subscriber misses the emit and the chip loader hangs forever. Trigger loadFees() once at init when getTotals() is already populated; the snapshotTotals dedup and fetchSeq guard handle the double-emit and out-of-order cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Re-reviewed after the third commit (d47d623).
The new commit introduces InvoiceSurchargeRunningTotal, QuoteToOrderSurcharge, SalesOrderCancelAfter, and Model/Webapi/Surcharges — all look correct. The atomic UPDATE … SET col + delta WHERE col + delta <= cap pattern in InvoiceSurchargeRunningTotal is safe (values are number_format-cast floats and a cast-to-int ID, no injection risk). No new critical issues.
The two previously flagged issues remain open and unaddressed:
Model/Config/Source/PaymentTermsDurationDays.php— still hardcodes['value' => 30, 'label' => __('30 days')]instead of delegating toBrandRegistryInterface::getAvailablePaymentTerms()(see inline thread)..github/workflows/release.yml— barepushtrigger still has no CI gate (see inline thread).
The constructor was updated to take TaxCalculation as arg 5 and BrandRegistryInterface as arg 6, dropping the previous State dependency (developer-mode is now read from app/etc/env.php). The unit tests still passed the old argument list, so every test instantiating Repository errored on setUp. - Drop State usage from both Repository test fixtures. - Add a BrandRegistryInterface mock returning the Two URL template. - For URL tests, introduce a RepositoryUrlTestable subclass that overrides isDeveloperMode() via a public flag, since the real check reads BP/app/etc/env.php which doesn't exist in unit tests. - Make Repository::isDeveloperMode() protected so it can be overridden by the test double. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Re-reviewed after the fourth commit (205a514).
The new commit adds: buyer_fee_share API payload refactoring (omits surcharge key on percentage-only paths, adds differential reference_terms cleanly), response caching on SurchargeCalculator, creditmemo line-items fixes (adjustment reconciliation line, surcharge BUYER_FEE line, tax-rate rounding to 6dp), and frontend layout XMLs wiring the surcharge totals block into order/invoice/creditmemo views. All look correct; no new critical issues.
The two previously flagged issues remain open and unaddressed (see inline threads):
Model/Config/Source/PaymentTermsDurationDays.php— still hardcodes['value' => 30, 'label' => __('30 days')]instead of delegating toBrandRegistryInterface::getAvailablePaymentTerms()..github/workflows/release.yml— barepushtrigger still has no CI gate; any commit landing onmaincan cut a release regardless of test status.
Ready for human approval ✅ (pending the two open threads above, which the team should decide whether to address now or defer to a follow-up).
Three more test files were instantiating runtime classes with the
pre-rewrite constructor signatures or against runtime defaults that
have since changed.
- AdapterTest: add BrandRegistryInterface mock as arg 2 of Adapter.
- TwoErrorHandlingTest: inject a BrandRegistry mock via reflection
so error messages that reference the product name don't NPE.
- SurchargeCalculatorTest: replace the CurrencyFactory/Currency
active-record mock with a CurrencyRatesProviderInterface mock;
use getRate() multiplier + null-rate for the conversion-failure
path. Pass BrandRegistry mock to the inner Adapter constructor.
- RepositoryPaymentTermsTest: align the expected default surcharge
line description with the runtime ('Payment terms fee - %1 days').
Also fix the i18n source string for the organization_number loc
mapping in Model/Two.php — it was hardcoded to the Dutch "KVK
number" rather than the brand-neutral source string "Company ID"
that the i18n files translate per locale.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Re-reviewed after the fifth commit (86862c1).
The new commit adds: NL BV-company order-intent filter (BVCompanyRegex + country gate in placeOrderIntent), single-term chip UI with async loading state, payment title/subtitle block redesign, "KVK number" label for NL, defensive (window.checkoutConfig.payment || {}).two_payment || {} config access, and removal of the stale generalErrorMessage fallback branch. All look correct; no new critical issues.
The two previously flagged issues remain open and unaddressed (see inline threads):
Model/Config/Source/PaymentTermsDurationDays.php— still hardcodes['value' => 30, 'label' => __('30 days')]instead of delegating toBrandRegistryInterface::getAvailablePaymentTerms()..github/workflows/release.yml— barepushtrigger still has no CI gate; any commit landing onmaincan cut a release regardless of test status.
Ready for human approval ✅ (pending the two open threads above, which the team should decide whether to address now or defer to a follow-up).
A handful of unit tests asserted behaviour that the current runtime does not implement, or used payload shapes from an earlier draft of the surcharge calculator. - testReturnsZeroWhenApiOmitsBuyerFeeShare → renamed to testThrowsWhenApiOmitsBuyerFeeShare. The runtime is strict and throws when the pricing API response is missing the required field; the prior "returns zero" behaviour was never implemented. - testPayloadBuyerFeeShareForPercentage: the percentage payload does not include a `surcharge` key; assert its absence instead of an explicit zero value. - testDifferentialModeDefaultTermShortCircuits → renamed to testDifferentialModeDefaultTermDelegatesToApi. Differential math is the API's job; the plugin always calls the API and trusts the response (which will be 0 at the default term). - testReturnsTaxRateAndDescription: surcharge line description is a Magento phrase template, so the merchant-supplied string needs to embed `%1` for the term-days substitution to take effect. - AdapterTest: stub BrandRegistry::getProductName() so error messages that include the brand name don't render as empty. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Re-reviewed after the sixth commit (654403a).
The new commit is test-only — it aligns AdapterTest and SurchargeCalculatorTest with the current runtime behaviour (removing assertions for unimplemented behaviours and outdated payload shapes). No application code was changed; no new critical issues.
The two previously flagged issues remain open and unaddressed (see inline threads):
Model/Config/Source/PaymentTermsDurationDays.php— still hardcodes['value' => 30, 'label' => __('30 days')]instead of delegating toBrandRegistryInterface::getAvailablePaymentTerms()..github/workflows/release.yml— barepushtrigger still has no CI gate; any commit landing onmaincan cut a release regardless of test status.
Ready for human approval ✅ (pending the two open threads above, which the team should decide whether to address now or defer to a follow-up).
The PHPStan and DI-compile jobs were running `composer require two-inc/magento2:@dev` inside the Magento container, which resolved the package from Packagist rather than the working tree that had just been `docker cp`d into /data/extensions/. That meant CI was exercising the already-published version of the plugin on every run, not the code under review. Configure a composer `path` repository pointing at the uploaded directory before requiring the package. Composer reads the local composer.json directly, so the job tests exactly the bytes that were uploaded. Drops the feature-branch / strip-version dance that was only needed to keep Packagist happy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Re-reviewed after the seventh commit (c8b604c).
The new commit refines the admin surcharge-grid JS and CSS: lastFeesKey is moved (with a source-order comment) to just before initInheritCheckboxes()/update() rather than being removed; the fee-cell rendering is improved to handle zero-percentage and zero-fixed cases separately; the degraded-currency note now reads from data-text-* attributes via .attr() instead of .data(); a .fail() fallback replaces loading dots with —; and helper-text paragraphs are wired to the surcharge-type toggle. All correct; no new critical issues.
The two previously flagged issues remain open and unaddressed (see inline threads):
Model/Config/Source/PaymentTermsDurationDays.php— still hardcodes['value' => 30, 'label' => __('30 days')]instead of delegating toBrandRegistryInterface::getAvailablePaymentTerms()..github/workflows/release.yml— barepushtrigger still has no CI gate; any commit landing onmaincan cut a release regardless of test status.
Ready for human approval ✅ (pending the two open threads above, which the team should decide whether to address now or defer to a follow-up).
Two open review threads: 1. .github/workflows/release.yml — the plugin-rewrite commit reset release.yml to its pre-INF-1159 shape, which restored the bare `push: [main]` trigger and dropped the workflow_run CI gate. A broken commit landing on main could therefore produce a tagged Release page regardless of CI status. Restore Bharat's workflow_run-gated version (with the brand-neutralised tag prefix comment kept). 2. Model/Config/Source/PaymentTermsDurationDays.php — hardcoded a single `30 days` option and wasn't referenced from any admin XML, DI binding, or source_model attribute. Functionality is already covered by Model/Config/Source/AvailablePaymentTerms which delegates to BrandRegistryInterface::getAvailablePaymentTerms(). Drop the dead class rather than keeping a stub that would silently produce wrong results if someone wired it later. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Gold Squadron flagged five real issues. All addressed in this commit.
- i18n full-sentence template (Yoda HIGH). Magento's translator
exact-matches on the full template string, so `__('Payment terms
fee - %1 days', 30)` was never hitting the `"%1 days"` row in
nl_NL.csv / nb_NO.csv / sv_SE.csv — the surcharge line stayed
English in non-EN carts. Ship the full sentence as a translation
source in each shipped locale.
- Header.php sign-up + documentation URLs (Leia HIGH). Pulled the
hardcoded `https://portal.two.inc/...` and `https://docs.two.inc/...`
constants up onto BrandRegistryInterface so brand-overlay packages
can redirect them without subclassing the admin block.
- etc/csp_whitelist.xml (Leia HIGH). Magento's CSP whitelist file is
static XML and the BrandRegistryInterface seam cannot rewrite it
at runtime. Add an explicit comment for future overlay authors
noting that overlays whose checkout lives on a non-two.inc host
must ship their own etc/csp_whitelist.xml (additive merge).
- ci.yml docker cp leak (Vader HIGH). `docker cp "$(pwd)"` does not
respect .gitignore or .gitattributes, so untracked dev junk,
dotfiles, and the .git/ directory crossed into the CI container.
Switch to `git archive HEAD | docker exec -i ... tar -x` which
honours export-ignore and ships only tracked files.
- Surcharges.php + TermSelection.php asymmetric exception handling
(Han MEDIUM). Per-term pricing failures were silently zeroed and
shown to the buyer as 0.00 fee chips, but when the buyer picked
that term the cart total collector would throw on the same path.
Add `addErrorLog` calls inside the inner catch so the silent zero
doesn't mask a broken pricing path; inject LogRepository into
TermSelection (Surcharges already had it).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`git archive` honours `.gitattributes export-ignore`, which strips the repo-only build/test files (phpstan.neon, phpunit.xml, Test/, .github/) — phpstan then can't find its config inside the container. Switch to `git ls-files | tar`: ships all tracked files (so CI gets phpstan.neon and friends) but still leaves .git/, dotfiles, and any untracked dev state on the host — same supply-chain win. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Re-reviewed after commits b5dd547 and 949f0de.
b5dd547 ("adversarial review fixes — i18n, brand seam, CI source"):
- i18n full-sentence template fix: correct — Magento's translator requires exact template-string matches, so shipping the full sentence in nb_NO/nl_NL/sv_SE CSVs is the right fix.
BrandRegistryInterface::getSignUpUrl()/getDocumentationUrl(): clean extraction;Header.phpandTwoBrand.phpupdated consistently.etc/csp_whitelist.xmloverlay comment: appropriate guidance for future brand packages.- Exception handling in
Surcharges.phpandTermSelection.php:addErrorLogadded inside the per-term catch blocks so silent 0.0 fallbacks are now visible in logs — correct fix; the 0.0 fallback behaviour itself is preserved. TermSelection:LogRepositoryinjection follows existing DI patterns; no concerns.
949f0de ("ship tracked files via git ls-files, not git archive"):
git ls-files -z | tar --null -cf - -T -correctly ships all tracked files (so CI getsphpstan.neon,phpunit.xml,Test/) while still excluding.git/, dotfiles, and untracked working-tree state. The null-delimiter handling is correct for paths with spaces. This supersedes thegit archiveapproach from b5dd547 which was stripping CI-required files via.gitattributes export-ignore.
Both previously flagged threads (hardcoded 30-day payment term, bare push release trigger) were addressed in 58ba972 and are now resolved.
No new critical issues.
Ready for human approval ✅
Two adversarial iter-2 follow-ups landed: - Yoda: three PHP fallback labels `'Payment terms fee'` in Model/Total/Surcharge, Service/Order/ComposeOrder, and Service/Order/ComposeRefund weren't wrapped in `__()`, so when the upstream description was missing the buyer saw the English string regardless of locale. Wrap each in `__()` (and `(string)`- cast in the two compose paths where the value is sent to the API as a JSON string field). - Leia: tighten the etc/csp_whitelist.xml overlay-author comment. "Additively" was slightly imprecise — within a single policy, Magento merges by `value id`: new ids are appended, but ids reused by an overlay override this module's value. Document the actual semantics so overlay authors aren't surprised. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Two\Gateway\Api\BrandRegistryInterfaceas the brand-customisation seam; the default binding isTwoBrand.Test plan
composer installresolves cleanly on a fresh Magento sandbox.bin/magento setup:di:compile && bin/magento setup:upgradesucceed on a Two-only install.🤖 Generated with Claude Code