IMP-424: Extended net terms and surcharge configuration#78
IMP-424: Extended net terms and surcharge configuration#78dgjlindsay wants to merge 7 commits intomainfrom
Conversation
Admin config for multi-term selection (14/30/60/90 days), custom duration, default term, surcharge method (percentage/fixed/both), differential surcharge for term extensions, configurable line item description and tax rate. SurchargeCalculator calls the pricing API to compute buyer fees. ComposeOrder sends available_terms to the API for buyer-facing checkout selection. Configurable limits (AVAILABLE_PAYMENT_TERMS, SURCHARGE_FIXED_MAX, SURCHARGE_PERCENTAGE_MAX) in RepositoryInterface for easy fork override. Translations added for nl_NL, nb_NO, sv_SE. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the payment terms and surcharge capabilities of the system, providing merchants with greater flexibility in configuring payment options for their customers. It introduces a robust framework for managing multiple payment durations and applying various types of surcharges, including a differential mode that intelligently adjusts fees based on term extensions. The changes are supported by a new backend service for surcharge calculation, an updated admin interface for intuitive configuration, and a comprehensive suite of tests to ensure stability and correctness. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive functionality for managing payment terms and surcharges within the system. Key changes include extending the configuration interface and repository to support multiple payment terms, custom durations, default terms, and various surcharge types (percentage, fixed, differential, with tax rates). A new SurchargeCalculator service has been added to determine buyer surcharges based on these configurations and API fees. The admin UI has been updated with new configuration fields and dynamic JavaScript logic for managing payment term and surcharge settings. Additionally, the pull request enhances API error handling by providing more specific and user-friendly messages for validation and system errors, and significantly expands the testing suite with new unit and end-to-end tests, along with necessary Magento stubs and a PHPUnit bootstrap. The review feedback suggests improving the abstraction of the RepositoryInterface by using methods instead of constants for data arrays, replacing a magic number with a named constant in getDefaultPaymentTerm, and dynamically passing the TERM_DAYS array to JavaScript from the backend to prevent hardcoding inconsistencies.
| public const AVAILABLE_PAYMENT_TERMS = [14, 30, 60, 90]; | ||
| public const SURCHARGE_FIXED_MAX = 100; | ||
| public const SURCHARGE_PERCENTAGE_MAX = 100; |
There was a problem hiding this comment.
Defining data arrays like AVAILABLE_PAYMENT_TERMS, SURCHARGE_FIXED_MAX, and SURCHARGE_PERCENTAGE_MAX directly as constants in an interface can make the interface less abstract. While the comment "override in fork" suggests this is the intended mechanism for customization, a more flexible approach might be to define methods that return these values, allowing implementing classes to provide their own logic or data sources. This would keep the interface purely behavioral.
| return $default; | ||
| } | ||
| $terms = $this->getAllBuyerTerms($storeId); | ||
| return $terms ? min($terms) : 30; |
There was a problem hiding this comment.
The fallback value 30 in getDefaultPaymentTerm is a magic number. It would be more maintainable and readable to define this as a named constant, perhaps DEFAULT_PAYMENT_TERM_DAYS, in the RepositoryInterface or Repository class. This improves clarity and makes it easier to change if the default ever needs to be adjusted.
return $terms ? min($terms) : self::DEFAULT_PAYMENT_TERM_DAYS;| 'use strict'; | ||
|
|
||
| // Payment Terms Configuration Handler | ||
| var TERM_DAYS = [14, 30, 60, 90]; |
There was a problem hiding this comment.
The TERM_DAYS array is hardcoded here, mirroring RepositoryInterface::AVAILABLE_PAYMENT_TERMS. If the PHP constant is updated, this JavaScript array would also need manual synchronization. To prevent potential inconsistencies and reduce maintenance effort, consider passing this array dynamically from the backend (e.g., via window.config or a dedicated API endpoint) rather than hardcoding it in the frontend JavaScript.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| public function getDefaultPaymentTerm(?int $storeId = null): int | ||
| { | ||
| $default = (int)$this->getConfig(self::XML_PATH_DEFAULT_PAYMENT_TERM, $storeId); | ||
| if ($default > 0) { | ||
| return $default; | ||
| } | ||
| $terms = $this->getAllBuyerTerms($storeId); | ||
| return $terms ? min($terms) : 30; |
There was a problem hiding this comment.
getDefaultPaymentTerm() does not validate that the stored value is actually in the enabled terms set. If a merchant selects e.g. 14 days as default but has not enabled 14-day terms in the multiselect, this method returns 14. ComposeOrder then sends terms.duration_days = 14 to the API while available_terms (built from getAllBuyerTerms) omits 14 — the default term is absent from its own available-terms list, which could cause order-creation failures.
| public function getDefaultPaymentTerm(?int $storeId = null): int | |
| { | |
| $default = (int)$this->getConfig(self::XML_PATH_DEFAULT_PAYMENT_TERM, $storeId); | |
| if ($default > 0) { | |
| return $default; | |
| } | |
| $terms = $this->getAllBuyerTerms($storeId); | |
| return $terms ? min($terms) : 30; | |
| public function getDefaultPaymentTerm(?int $storeId = null): int | |
| { | |
| $default = (int)$this->getConfig(self::XML_PATH_DEFAULT_PAYMENT_TERM, $storeId); | |
| $terms = $this->getAllBuyerTerms($storeId); | |
| if ($default > 0 && in_array($default, $terms, true)) { | |
| return $default; | |
| } | |
| return $terms ? min($terms) : 30; | |
| } |
The root cause is that the default_payment_term admin dropdown uses AvailablePaymentTerms as its source model (listing all 14/30/60/90 options) rather than a dynamic model reflecting only the currently-enabled terms, so the guard in getDefaultPaymentTerm() is needed regardless.
Checkout integration: - Buyer-facing term selector dropdown (hidden when only one term available) - selectedTerm flows through DataAssignObserver → ComposeOrder - SurchargeCalculator injected into ComposeOrder via constructor - Surcharge added as line item (type: SURCHARGE) with tax calculation - Order totals (gross/net/tax) adjusted to include surcharge - ConfigProvider passes availableBuyerTerms + defaultPaymentTerm to frontend Tests (35 new, 95 total): - RepositoryPaymentTermsTest: 22 tests covering getPaymentTerms, getAllBuyerTerms, getDefaultPaymentTerm, getSurchargeType/Config/TaxRate - SurchargeCalculatorTest: 13 tests covering percentage, fixed, combined, limit cap, cent rounding, differential mode, end-of-month, tax+description Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…hen unconfigured Replace the hidden surcharge_show_tax_rate toggle with auto-detection: - Empty surcharge_tax_rate field falls back to Magento's default tax rate via TaxCalculationInterface::getDefaultCalculatedRate() - Explicit "0" means tax-exempt (merchant override) - New SurchargeTaxRate frontend_model Block shows dynamic comment: green "defaults to X%" when tax rules exist, red warning when not - Remove surcharge_show_tax_rate from system.xml, config.xml, and RepositoryInterface Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Six tests covering the POST /pricing/v1/portal/order/fee endpoint: valid 30/90-day fees, longer-terms-cost-more assertion, end-of-month terms, invalid country code, and zero-duration error handling. Tests skip gracefully with a clear message when the API key lacks pricing endpoint permissions, keeping the suite green. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Re-reviewed after the latest commits. The critical issue flagged in the previous review — getDefaultPaymentTerm() returning a configured value that may not be present in getAllBuyerTerms() — is still unresolved. See the existing inline comment with a suggested fix. No new critical issues found in the updated diff.
…lation Switch from portal-only /pricing/v1/portal/order/fee to the new API-key-authed /v1/pricing/order/fee endpoint. Fix RealCurl header/option accumulation across sequential requests in E2E tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Named arguments are a PHP 8.0 feature. CI runs against PHP 7.4. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Re-reviewed after today's update. The critical issue flagged in the previous review remains unresolved: getDefaultPaymentTerm() in Model/Config/Repository.php does not validate that the stored default value is present in the enabled terms set (see existing inline comment with suggested fix). No new critical issues found in the additional commits (SurchargeCalculator, ComposeOrder integration, test infrastructure). This PR should not be merged until the getDefaultPaymentTerm() guard is applied.
There was a problem hiding this comment.
Re-reviewed after the force-push. The critical bug flagged in the earlier thread (PRRT_kwDOHkfW4s53AhoJ) is still present: getDefaultPaymentTerm() returns a stored value without checking it exists in getAllBuyerTerms(), meaning terms.duration_days sent to the API can be absent from available_terms. The suggestion block in that thread shows the fix. Please address before merging.
1 participant
Summary
SurchargeCalculatorservice callsPOST /pricing/v1/portal/order/feeto compute buyer fees, with differential mode making two API callsAVAILABLE_PAYMENT_TERMS,SURCHARGE_FIXED_MAX,SURCHARGE_PERCENTAGE_MAXinRepositoryInterfacefor easy overrideBranding notes for fork
Test plan
make test— 60 unit tests pass (verified)payment_terms_duration_days=30should see [30] as buyer terms🤖 Generated with Claude Code