Skip to content

ABN-298 / ABN-356 / ABN-366: chip-summary sync, fee column polish, deferred invoice#84

Merged
dgjlindsay merged 50 commits intodevelopfrom
doug/ABN-298-chip-summary-mismatch
Apr 25, 2026
Merged

ABN-298 / ABN-356 / ABN-366: chip-summary sync, fee column polish, deferred invoice#84
dgjlindsay merged 50 commits intodevelopfrom
doug/ABN-298-chip-summary-mismatch

Conversation

@dgjlindsay
Copy link
Copy Markdown
Contributor

Summary

Mirror of #83 (against main). Same five commits, four tickets:

  • ABN-298Model/Ui/ConfigProvider now passes storeId into SurchargeCalculator::calculate() so chip values resolve at the same scope as the Total Collector segment.
  • ABN-356 — Fee column: drop zero components, refetch on term-set change (memoised), animated three-dot placeholder while loading.
  • ABN-366 — defer Magento invoice until fulfilment. OrderService::processOrder() no longer eagerly invoices; both fulfilment observers create a CAPTURE_OFFLINE invoice after the /fulfillments POST. Idempotency via \$order->hasInvoices() on the complete branch.

See #83 for the full test plan.

🤖 Generated with Claude Code

brtkwr and others added 30 commits January 26, 2026 19:26
INF-665/fix: Fix Claude Code Review workflow OIDC authentication
Patch to incorporate return type fix shared by merchant
- Replace ENABLE_CLAUDE_AUTO_REVIEW with CLAUDE_REVIEW_CONFIG org variable
- Remove branch filter (now controlled via config)
- Add GitHub App auth for private plugin marketplace

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Makefile targets for creating, configuring, and managing a local Magento
container with the plugin installed. API and checkout URLs can be
overridden via environment variables for targeting local/staging backends.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses review feedback: TWO_API_BASE_URL and TWO_CHECKOUT_BASE_URL
env vars now only take effect when Magento is in developer mode,
preventing accidental URL overrides in production environments.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The injected Magento\Framework\App\State dependency caused
"Class get does not exist" during setup:di:compile in CI.
Use getenv('MAGE_MODE') instead, which avoids DI entirely and is
set via the container's -e flag in the Makefile.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Magento's setup:di:compile scans all .php files under the module root.
dev/configure.php contained $obj->get() calls that the DI compiler
misinterpreted as class references, causing "Class get does not exist"
on Magento 2.3.7 / PHP 7.3.

Renaming to dev/configure (no .php extension, with shebang) excludes
it from the scanner. Verified on both PHP 7.3/2.3.7 and PHP 8.2/2.4.6.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ABN-287/fix: replace State DI injection with MAGE_MODE env var check
ABN-287/feat: add local development environment setup
Instead of showing generic "Phone Number is not valid." or falling back
to "Something went wrong", now extracts the msg field from pydantic
validation errors and displays it alongside the field name, e.g.
"Phone Number: Invalid phone number for GB: 00123456789."

Refactored getFieldFromLocStr into getFieldNameFromLoc (returns field
name only) and added cleanValidationMessage to strip pydantic prefixes.
Updated nb_NO, sv_SE, and nl_NL translations accordingly.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Strip trailing period from API message before wrapping with format
  string to prevent double periods in user-facing output
- Cache field name translations in static variable to avoid redundant
  __() calls when processing multiple validation errors

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Categorise API errors into user errors (validation, no trace ID) and
  system errors (with trace ID) for cleaner customer-facing messages
- Drop verbose "Your request to Two failed" prefix from user errors
- Restore State DI injection for developer mode check (MAGE_MODE env
  var was not reliably set in the container process environment)
- Add compile make target for DI recompilation
- Restart container after configure to pick up config changes

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Only treat API errors as user-facing (no trace ID) when the response
status is 400. Errors with other status codes (401, 500, etc.) are
always treated as system errors with trace ID included.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ABN-287/fix: surface API validation details in checkout error messages
Bootstrap test infrastructure without composer install — uses a PSR-4
autoloader and Magento stub classes so tests run with just a phpunit
phar on a bare PHP image.

- 38 tests (65 assertions) covering getErrorFromResponse(),
  getFieldNameFromLoc(), getCheckoutApiUrl(), getCheckoutPageUrl()
- Docker-based `make test` target and CI workflow (PHP 7.4/8.1/8.2)
- Fix return type bug: SCHEMA_ERROR/ORDER_INVALID now returns Phrase

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add tests asserting that in developer mode, TWO_API_BASE_URL and
TWO_CHECKOUT_BASE_URL env vars take precedence even when an explicit
$mode parameter is passed to getCheckoutApiUrl/getCheckoutPageUrl.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pin to phpunit-9.6.34 with hardcoded SHA256 hash in both CI workflow
and Makefile to prevent supply chain attacks via compromised CDN.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds Test/E2E/ with a RealCurl HTTP implementation and three tests covering
valid key, invalid key (401), and bad payload. make test runs Unit only
(defaultTestSuite); make test-e2e forwards TWO_API_KEY/TWO_API_BASE_URL
into Docker and runs the E2E suite. Missing key fails naturally at the
assertion rather than skipping.
Stubs for Curl, LocalizedException, BundlePrice, ProductType; unit tests
for Adapter, OrderShouldSkip, and addVersionDataInURL; bootstrap wiring;
session summary and plan docs.
Remove session summaries and implementation plans that were
accidentally committed. Add Session Artifacts section to AGENTS.md
clarifying that session-specific content must not be committed to
this public repository.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rite

- Add FRP reverse proxy (frpc.toml + start-proxy.sh) with GCP Secret
  Manager fallback for auth token
- Patch container's PHP router to trust X-Forwarded-Host for HTTPS
  detection (FRP terminates TLS but sends X-Forwarded-Proto: http)
- Auto-set Magento base URLs to proxy domain via custom-entrypoint.sh
  (survives container restarts, entrypoint base URL resets)
- Install Xdebug automatically (mode=off) during make install
- Add make debug target: activates Xdebug + disables all caches for
  hot reload
- make run/stop now manage the FRP proxy lifecycle and print URLs
- Auto-detect staging/sandbox environment from gcloud account
- Support custom port via make install PORT=<n>
- Rewrite README: document full dev workflow including debugging,
  proxy, and tests

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ABN-298: Dev tooling — FRP proxy, Xdebug, debug mode, README
Clear stale generated code after composer require during install.
Fix proxy base_link_url not being set in "apply immediately" section,
causing navigation links to redirect back to localhost.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dgjlindsay and others added 19 commits April 14, 2026 11:43
Port from magento-abn-plugin PR #10. Adds getBrand() and getBrandVersion()
to Repository, appends brand query params to payment URL, terms link, and
sole trader iframe. Defaults empty (no brand override for Two). Forks
override via TWO_BRAND / TWO_BRAND_VERSION env vars in .env.local.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…oling

Extended net terms with configurable surcharge strategy (fixed, percentage,
or both). Per-term surcharge grid with differential mode. Dev tooling: FRP
proxy, Xdebug, debug mode, proxy auto-start in install target.

Fixes: surcharge field visibility on "Use System Value" toggle, field value
reset on inherit re-check, proxy base_link_url in patch-proxy section 3,
broken validation rule blocking admin config save.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fix: clear generated code after composer require
ABN-298: Extended net terms and surcharge configuration
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>
- 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>
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>
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>
ABN-298: Extended net terms, surcharge config, and dev tooling
Total Collector passes storeId to SurchargeCalculator::calculate() so
surcharge config resolves at the quote's store scope. ConfigProvider
omitted the arg, falling through to default scope. Any merchant config
set at website/store scope (typical) made the page-load chip values
diverge from the segment value computed by the Total Collector — chip
labels disagreed with the order summary.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cell text was always "x% + y" even when one side was zero. Now only
non-zero components render: pct alone if fixed is zero, fixed alone if
pct is zero, "0.00" if both are zero.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
processOrder() previously created and registered a Magento invoice via
CAPTURE_ONLINE at order-confirmation time when fulfill_trigger was set
to 'shipment' or 'complete'. Result: order showed up under Sales →
Invoices immediately, the cancel button on Sales → Orders was hidden,
and the invoice email fired before Two had actually invoiced the buyer.

Drop the eager invoice creation. processOrder() now just promotes the
order to Processing and records the authorisation transaction.

Move invoice creation to the two observers that already post to Two's
/fulfillments endpoint:

- SalesOrderShipmentAfter: after a successful /fulfillments POST for
  the whole order, prepareInvoice → CAPTURE_OFFLINE → register → pay.
  CAPTURE_OFFLINE is critical — CAPTURE_ONLINE would route through
  Two::capture() and re-post /fulfillments.
  The trigger='complete' branch is removed; SalesOrderSaveAfter is the
  sole handler for that trigger.

- SalesOrderSaveAfter: after a successful /fulfillments POST, create
  the invoice with the same CAPTURE_OFFLINE shape. Idempotency gated
  via $order->hasInvoices() — once we've fulfilled, subsequent saves
  of the same order return early before re-posting to Two.

Two::canCapture() / Two::capture() unchanged: trigger='invoice' still
flows admin-clicks-Invoice → Magento::capture() → /fulfillments →
register, which was already correct.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
loadFees() was only called once at module init. Toggling a term
checkbox or typing a new value into the custom-days input updated the
grid rows but left new cells stuck at "—" until a full page reload.

Wire loadFees() into update() so any term-set mutation triggers a
refetch. Memoise the joined terms key inside loadFees() to collapse
repeat fires from update() (every change handler hits update(), most
of them don't actually change the terms set). On AJAX failure the memo
is cleared so a transient error is retryable on the next event; cells
keep whatever they last showed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the static em-dash placeholder with three pulsing dots so the
merchant has visual feedback while the proxy fetches per-term fees from
Two. Dots are pure CSS (keyframed opacity + scale, staggered delays)
and live in initial server-rendered cells, dynamically created rows,
and any cell awaiting its first response. When the response lands,
cells that have a fee render the value; cells the server omitted fall
back to the em-dash so the loading state ends cleanly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces flexible payment terms and a per-term surcharge configuration for the Two payment gateway, featuring a new admin grid, a quote total collector, and a checkout chip selector. It also includes a comprehensive development environment setup and a suite of unit and E2E tests. Feedback identifies a fatal error in the surcharge grid backend model due to an undefined method, recommends adopting PriceCurrencyInterface for standardized currency rounding, and notes the usage of the deprecated Transaction class in event observers.

Comment on lines +153 to +155
return (string)$this->getFieldsetDataValue('currency/options/base')
?: (string)$this->_config->getValue('currency/options/base')
?: 'USD';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The method getFieldsetDataValue does not exist in Magento\Framework\App\Config\Value or its parent classes. This will cause a fatal error when saving the configuration. You should use the injected _config (ScopeConfig) to retrieve the base currency directly.

        return (string)$this->_config->getValue('currency/options/base')
            ?: 'USD';

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Opus 4.7] Out of scope for this PR — this code lands via the develop-forward merge from main (already shipped under PR #82, where it has been running on staging without errors). getFieldsetDataValue() is inherited from Magento\Framework\App\Config\Value and reads from the per-fieldset data populated during admin form submission; it is a documented Magento 2 pattern. Worth a separate ticket if Gemini's analysis turns out to be right after live verification, but should not block this PR.

Comment thread Model/Total/Surcharge.php
Comment on lines +147 to +153
$taxAmount = round($netAmount * $taxRate, 2);
$grossAmount = $netAmount + $taxAmount;

// Convert to base currency for base_* fields (order totals/tax reports)
$baseToQuoteRate = (float)$quote->getBaseToQuoteRate() ?: 1.0;
$baseGrossAmount = round($grossAmount / $baseToQuoteRate, 4);
$baseTaxAmount = round($taxAmount / $baseToQuoteRate, 4);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

It is recommended to use Magento\Framework\Pricing\PriceCurrencyInterface::round instead of the native PHP round function for currency-related calculations (lines 147, 152, 153). This ensures that rounding is performed according to the store's currency configuration and precision settings. You should inject PriceCurrencyInterface into the constructor.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Opus 4.7] Out of scope — that file is from the develop-forward merge of PR #82, not this PR. PriceCurrencyInterface::round() is a stylistic preference; the existing call sites use native round() consistently and PriceCurrencyInterface::round only delegates to native rounding plus a localised currency-code lookup, which we do not need on internal arithmetic. Worth a sweep across the repo as its own ticket if we want consistency, not a one-shot change in this PR.

Comment on lines +152 to +155
$this->transaction
->addObject($invoice)
->addObject($invoice->getOrder())
->save();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The Magento\Framework\DB\Transaction class is deprecated. For saving multiple sales objects atomically, consider using the newer transaction management patterns or repositories. Additionally, saving an order within its own save_after event can be risky, although the idempotency check hasInvoices() prevents infinite recursion here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Opus 4.7] Same justification as on PR #83: keeping \Magento\Framework\DB\Transaction for atomicity across the paired invoice + order saves. Splitting into InvoiceRepositoryInterface::save + OrderRepositoryInterface::save would split into two separate transactions with no rollback if the second fails after the first commits. The Transaction wrapper is the documented Magento 2 pattern for paired entity saves and is used elsewhere in this repo. No deprecation notice on the class itself in current Magento 2.4.x.

Comment on lines +167 to +170
$this->transaction
->addObject($invoice)
->addObject($invoice->getOrder())
->save();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The Magento\Framework\DB\Transaction class is deprecated. Consider using modern transaction management or repositories for saving the invoice and order.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Opus 4.7] Same justification as on PR #83: keeping \Magento\Framework\DB\Transaction for atomicity across the paired invoice + order saves. The Transaction wrapper is the documented Magento 2 pattern for paired entity saves; no deprecation notice on the class itself in 2.4.x.

- Idempotency guard on shipment branch: skip invoice creation if the
  order already has invoices, mirroring the SalesOrderSaveAfter check.
  Without this, a second sales_order_shipment_save_after for the same
  fully-shipped order would call prepareInvoice() on an already-fully-
  invoiced order and throw "Cannot create an invoice without products."

- Revert PRODUCT_NAME usages back to PROVIDER in three files. The repo
  convention here is PROVIDER (10 files); PRODUCT_NAME was carried over
  from the ABN-fork brief where the constant is named differently.
  Restoring consistency within this fork's codebase.

- Pick up an admin-grid header padding tweak that was already on disk.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dgjlindsay dgjlindsay merged commit a07c855 into develop Apr 25, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants