Skip to content

ABN-298: Extended net terms and surcharge configuration#79

Merged
dgjlindsay merged 3 commits intomainfrom
doug/ABN-298-extended-net-terms
Apr 15, 2026
Merged

ABN-298: Extended net terms and surcharge configuration#79
dgjlindsay merged 3 commits intomainfrom
doug/ABN-298-extended-net-terms

Conversation

@dgjlindsay
Copy link
Copy Markdown
Contributor

@dgjlindsay dgjlindsay commented Apr 13, 2026

Summary

Based on #80 (dev tooling).

  • Extended net terms: Multi-term selection (14/30/60/90 + custom), per-term surcharge config via admin grid, surcharge calculator with pricing API integration, checkout term selector, VAT auto-detect
  • Differential surcharge (commit 2, ABN-specific): "Surcharge Calculation Basis" option that charges buyers only for the fee difference vs the default payment term — delegates calculation to the pricing API via buyer_fee_share.reference_terms

Key design decisions

  • Surcharge config stored as flat keys — no migration, per-field scope inheritance
  • Admin grid via custom frontend_model Block replacing 12 sequential fields
  • SurchargeCalculator delegates percentage + differential to /v1/pricing/order/fee API (single call), applies fixed amount and limit cap locally
  • Differential mode (last commit) is cleanly revertable for the standard build

Commit structure

# Commit Scope
1 Extended net terms + surcharge Full feature, 103 tests
2 Differential surcharge ABN-specific, +6 tests = 109 total

Test plan

  • Unit tests pass (make test — 109 tests)
  • E2E pricing tests pass (make test-e2e)
  • Admin: surcharge grid renders, columns toggle per surcharge type
  • Admin: custom payment term adds grid row dynamically
  • Checkout: term selector + surcharge line item on order
  • Differential: default term = zero surcharge, extended term = fee delta

🤖 Generated with Claude Code

Comment thread Service/Order/ComposeOrder.php
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Reviewed the surcharge + extended net terms implementation. One critical issue flagged inline; the rest of the approach looks sound.

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 a comprehensive surcharge and payment terms system for the Two payment gateway, featuring an admin configuration grid, a calculation service, and a frontend term selector. It also modernizes the local development environment with Docker, Xdebug, and FRP proxy support, supported by new unit and E2E tests. Feedback focuses on improving code maintainability by refactoring duplicated logic in the order service and moving inline styles to external CSS files or standard Magento admin classes.

Comment thread Block/Adminhtml/System/Config/Field/SurchargeTaxRate.php
Comment thread Service/Order/ComposeOrder.php
Comment thread view/adminhtml/templates/system/config/field/surcharge-grid.phtml Outdated
@dgjlindsay dgjlindsay changed the title ABN-298: Extended net terms, surcharge configuration, and dev tooling ABN-298: Extended net terms and surcharge configuration Apr 13, 2026
@dgjlindsay dgjlindsay changed the base branch from main to doug/ABN-298-dev-tooling April 13, 2026 12:56
@dgjlindsay dgjlindsay force-pushed the doug/ABN-298-dev-tooling branch from 97044f4 to f8b395c Compare April 13, 2026 13:44
@dgjlindsay dgjlindsay force-pushed the doug/ABN-298-extended-net-terms branch from e316346 to 4405245 Compare April 13, 2026 13:45
Base automatically changed from doug/ABN-298-dev-tooling to main April 13, 2026 15:09
@dgjlindsay dgjlindsay force-pushed the doug/ABN-298-extended-net-terms branch 2 times, most recently from f911874 to 369b21e Compare April 13, 2026 15:15
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

All previously flagged issues are resolved:

  • selectedTerm validation (critical): getSelectedTermDays() now validates against getAllBuyerTerms() and falls back to the default term. Fix matches the suggestion exactly.
  • Inline styles: moved to _surcharge-grid.less (auto-loaded via _module.less).
  • buildTermObject() duplication: extracted into shared helper.

No new critical issues found in the updated commits. Ready for human approval.

Comment thread Model/Config/Backend/SurchargeGrid.php Outdated
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

The selectedTerm validation from the previous review is correctly fixed. One new critical issue flagged inline: SurchargeGrid::afterSave() reads from getValue() which beforeSave() already cleared — surcharge config is never saved to the database.

@dgjlindsay dgjlindsay force-pushed the doug/ABN-298-extended-net-terms branch 2 times, most recently from fe6fbb7 to fb5236c Compare April 13, 2026 16:44
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Both previously flagged critical issues are resolved:

  • selectedTerm validation (Service/Order/ComposeOrder.php): getSelectedTermDays() now validates the buyer-supplied value against getAllBuyerTerms() before use. ✓
  • afterSave() not persisting values (Model/Config/Backend/SurchargeGrid.php): now reads from getData('groups') instead of getValue() (which beforeSave() had cleared). ✓

No new critical issues found in the updated commits. Ready for human approval.

@dgjlindsay dgjlindsay force-pushed the doug/ABN-298-extended-net-terms branch 2 times, most recently from fe6fbb7 to 11974fc Compare April 13, 2026 18:20
@dgjlindsay dgjlindsay changed the base branch from main to doug/fix-dev-tooling April 13, 2026 18:20
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Both issues flagged in my previous review are confirmed fixed:

  • selectedTerm validationgetSelectedTermDays() now validates against getAllBuyerTerms() before trusting the buyer-supplied value; invalid/unconfigured terms fall back to the configured default.
  • SurchargeGrid::afterSave() data source bug — now reads grid values from getData('groups') instead of getValue() (which beforeSave() clears to ''), so surcharge config is actually persisted.

No new critical issues found in the updated diff. Ready for human review.

@dgjlindsay dgjlindsay force-pushed the doug/ABN-298-extended-net-terms branch from 4b47f33 to c1e27a6 Compare April 14, 2026 10:29
dgjlindsay and others added 3 commits April 14, 2026 11:45
…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>
@dgjlindsay dgjlindsay force-pushed the doug/ABN-298-extended-net-terms branch from 22a5d9e to 40ddf19 Compare April 14, 2026 10:46
Base automatically changed from doug/fix-dev-tooling to main April 14, 2026 13:38
@dgjlindsay dgjlindsay merged commit 84d8297 into main Apr 15, 2026
14 checks passed
@dgjlindsay dgjlindsay deleted the doug/ABN-298-extended-net-terms branch April 15, 2026 14:38
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.

2 participants