ABN-298 / ABN-356 / ABN-366: chip-summary sync, fee column polish, deferred invoice#83
ABN-298 / ABN-356 / ABN-366: chip-summary sync, fee column polish, deferred invoice#83dgjlindsay wants to merge 6 commits intomainfrom
Conversation
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>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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 refactors the order fulfillment process by centralizing invoice creation through a new createOfflinePaidInvoice method in the order save and shipment observers. It also simplifies the OrderService by removing redundant invoicing logic and enhances the admin surcharge grid with loading indicators and improved fee calculation. Feedback was provided regarding the handling of partial shipments for invoicing, the use of deprecated transaction and save methods in favor of repositories, and inconsistent naming constants for branding.
| $invoice->pay(); | ||
| $invoice->setTransactionId($order->getPayment()->getLastTransId()); | ||
| $invoice->save(); | ||
| $this->createOfflinePaidInvoice($order); |
There was a problem hiding this comment.
The current implementation only creates a Magento invoice when the entire order is shipped. If the fulfill_trigger is set to shipment, Magento should ideally create a partial invoice corresponding to the items in the current shipment, especially since a partial fulfillment request was just sent to the Two API (line 119). Consider removing the isWholeOrderShipped gate and passing the shipment's quantities to prepareInvoice to support partial invoicing.
$this->createOfflinePaidInvoice($order, $shipment);There was a problem hiding this comment.
[Claude Opus 4.7] Out of scope for ABN-366 per the implementation brief: "For partial shipments: keep the current pass-through behaviour (we have no ask to support partial Magento invoicing yet)." Two-side partial fulfilments still post correctly via line 119; only the Magento-side partial invoice is deferred. Will revisit when there is a concrete merchant ask.
| if (!$this->isWholeOrderShipped($order)) { | ||
| $error = __( | ||
| "%1 requires whole order to be shipped before it can be fulfilled.", | ||
| $this->configRepository::PRODUCT_NAME |
There was a problem hiding this comment.
The constant PRODUCT_NAME is used here, but PROVIDER is still used in other parts of the file (lines 200 and 205). For consistency and to ensure the correct branding is displayed to the merchant, please use PRODUCT_NAME for all status history comments.
There was a problem hiding this comment.
[Claude Opus 4.7] Resolved the other way in 1253c42 — reverted my new PRODUCT_NAME usages to PROVIDER, which is the existing repo-wide convention here (10 files use PROVIDER, 0 used PRODUCT_NAME before this PR). PRODUCT_NAME came in via a port brief from the ABN fork where the constant is named differently; both constants are equal strings (Two) so no functional change either way.
| $this->transaction | ||
| ->addObject($invoice) | ||
| ->addObject($invoice->getOrder()) | ||
| ->save(); |
There was a problem hiding this comment.
The use of Magento\Framework\DB\Transaction and the save() method on models is deprecated in Magento 2. It is recommended to use Magento\Sales\Api\InvoiceRepositoryInterface and Magento\Sales\Api\OrderRepositoryInterface to persist these entities. Additionally, Transaction should be replaced with Magento\Framework\Model\ResourceModel\Db\TransactionManagerInterface if atomic operations across multiple resource models are required.
$this->invoiceRepository->save($invoice);
$this->orderRepository->save($invoice->getOrder());There was a problem hiding this comment.
[Claude Opus 4.7] Keeping \Magento\Framework\DB\Transaction for atomicity across the invoice + order saves — switching to 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 (e.g. it is the same shape OrderService::processOrder previously used for the eager-invoice path). No deprecation notice on the class itself in current Magento 2.4.x.
There was a problem hiding this comment.
Yh makes more sense to have atomicity across the invoice + order saves as a grouped action rather than on a per save basis.
- 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>
There was a problem hiding this comment.
The idempotency guard flagged in my earlier review (!$order->hasInvoices() in SalesOrderShipmentAfter) has been applied in 1253c42. Resolving that thread.
Re-reviewed the full diff at this commit — no critical issues. The deferred-invoice refactor, storeId scope fix, and fee-column polish all look correct. Ready for human approval.
| $this->transaction | ||
| ->addObject($invoice) | ||
| ->addObject($invoice->getOrder()) | ||
| ->save(); |
There was a problem hiding this comment.
Yh makes more sense to have atomicity across the invoice + order saves as a grouped action rather than on a per save basis.
2 participants
Summary
Five commits, four tickets, one branch.
storeIdfromModel/Ui/ConfigProviderintoSurchargeCalculator::calculate()so chip values resolve at the same scope as the Total Collector segment. Without this, any merchant config set at website/store scope (typical) made the page-load chip labels diverge from the order-summary segment value. Matches the "sometimes, mysteriously, the chips would show a different fee to the order summary" symptom."x%"if fixed is 0,"y"if percentage is 0,"0.00"if both.loadFees()so non-term-set changes collapse to no-op;.failclears the memo so transient errors are retryable.CAPTURE_ONLINEinvoice creation fromOrderService::processOrder(). Magento invoice now lives behind the actual fulfilment in both observers (SalesOrderShipmentAfterfor trigger=shipment,SalesOrderSaveAfterfor trigger=complete), created withCAPTURE_OFFLINEafter the/v1/order/{id}/fulfillmentsPOST succeeds — soTwo::capture()doesn't double-post. Idempotency on the complete branch via\$order->hasInvoices(). Shipment observer'selseif (trigger == 'complete')removed; SaveAfter is the sole trigger=complete handler.Test plan
fulfill_trigger=shipment: confirm shows Processing, no invoice, cancel button visible. Ship → invoice created and paid, email sends.fulfill_trigger=complete: confirm shows Processing, no invoice. Save with status infulfill_order_statuslist →/fulfillmentsposts, invoice created and paid. Re-save: no second/fulfillments(gated byhasInvoices()).fulfill_trigger=invoice(admin clicks Invoice in UI): unchanged path.🤖 Generated with Claude Code