Fix subcontracting repricing regressions - #11292
Fix subcontracting repricing regressions#11292Chethan Thopaiah (ChethanT) wants to merge 1 commit into
Conversation
Preserve manual and calculated worksheet costs, retain released-order scheduling, and reprice lead-time-only date changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 255f96a1-3a67-418c-9b69-ca50ae31e5b0
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core subcontracting pricing/scheduling behavior in multiple event-driven paths where subtle functional regressions are hard to rule out without a full CI run.
Pull request overview
This PR is a follow-up to #10917 / AB#648535 to address remaining subcontracting repricing regressions around carry-out cost preservation, released-order date edits, and lead-time-only rescheduling.
Changes:
- Reprices subcontracting lines based on resulting Order Date (not Planned Receipt Date) and skips repricing when the purchase header is not Open.
- Refines no-price fallback to use the standard subcontracting worksheet cost formula, and avoids overwriting manually entered worksheet Direct Unit Cost during carry-out.
- Adds regression tests covering the newly fixed edge paths (no matching price, manual overrides, released-order date edits, lead-time-only boundary crossing).
File summaries
| File | Description |
|---|---|
| src/Apps/W1/Subcontracting/Test/Tests/SubcPricingTest.Codeunit.al | Adds regression tests for the uncovered subcontracting repricing/carry-out scenarios. |
| src/Apps/W1/Subcontracting/App/src/Purchase/SubcPurchaseLineExt.Codeunit.al | Reprices on Order Date changes and bypasses repricing for non-open purchase headers to preserve released-order scheduling behavior. |
| src/Apps/W1/Subcontracting/App/src/Purchase/SubcPriceManagement.Codeunit.al | Splits price-list lookup from fallback cost calculation and uses worksheet-consistent fallback logic when no price matches. |
| src/Apps/W1/Subcontracting/App/src/Manufacturing/SubcReqWkshMakeOrd.Codeunit.al | Avoids repricing during carry-out unless the transferred worksheet value matches the automatically selected price-list value. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ProdOrderRoutingLine."Direct Unit Cost" * ProdOrderLine."Qty. per Unit of Measure", | ||
| GeneralLedgerSetup."Unit-Amount Rounding Precision")); | ||
|
|
||
| ProdOrderLine.CalcFields("Total Exp. Oper. Output (Qty.)"); |
There was a problem hiding this comment.
New procedure GetNonPriceListDirectCost (SubcPriceManagement.Codeunit.al) divides by ProdOrderLine."Total Exp. Oper. Output (Qty.)" whenever the routing operation's Unit Cost Calculation is not Units, with no guard against that field being zero. "Total Exp. Oper. Output (Qty.)" is a FlowField reflecting output reported so far, which is legitimately zero for a production order/operation that has not yet had output posted — exactly the state a fresh subcontracting purchase line without a matching price-list entry can be in. This introduces an unhandled division-by-zero runtime error on the pricing path used by GetSubcPriceForPurchLine. Note this is especially notable because the identical formula is already guarded elsewhere in the same app: both SubcPurchaseOrderCreator.Codeunit.al (around the RequisitionLine.Quantity <> 0 check before the Direct Unit Cost calculation) and SubcCalculateSubcontracts.Report.al wrap the same '(Expected Operation Cost Amt. - Expected Capacity Ovhd. Cost) / Total Exp. Oper. Output (Qty.)' expression in a zero-quantity guard that falls back to a Direct Unit Cost of 0, but the new GetNonPriceListDirectCost path added by this PR omits that guard. If this were not already mirrored by an established guarded pattern elsewhere in the same feature, its impact would be major (an unhandled runtime error blocking pricing/carry-out); it should be promoted to a knowledge-backed rule and guarded to match the sibling call sites before it can gate.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
|
|
||
| local procedure TryFindProdOrderRtngLine(ProdOrderNo: Code[20]; RtngRefNo: Integer; RoutingNo: Code[20]; OperationNo: Code[10]; var ProdOrderRoutingLine: Record "Prod. Order Routing Line"): Boolean | ||
| begin | ||
| ProdOrderRoutingLine.SetFilter(Status, '%1|%2', ProdOrderRoutingLine.Status::Released, ProdOrderRoutingLine.Status::Finished); |
There was a problem hiding this comment.
TryFindProdOrderRtngLine sets every component of the Prod. Order Routing Line primary key (Status, Prod. Order No., Routing Reference No., Routing No., Operation No.) via SetFilter/SetRange and then calls FindFirst, even though this is a full-primary-key lookup. This is called on the pricing hot path (every GetSubcPriceForPurchLine invocation) and should use a direct Get instead of an index-walking Find.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
if ProdOrderRoutingLine.Get(ProdOrderRoutingLine.Status::Released, ProdOrderNo, RtngRefNo, RoutingNo, OperationNo) then
exit(true);
exit(ProdOrderRoutingLine.Get(ProdOrderRoutingLine.Status::Finished, ProdOrderNo, RtngRefNo, RoutingNo, OperationNo));Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
| // and CanChangePlannedReceiptDateOnReleasedPurchOrderLine). Bypass repricing entirely | ||
| // once the header is no longer Open so scheduling still works without silently | ||
| // changing released financial terms. | ||
| PurchaseHeader.SetLoadFields(Status); |
There was a problem hiding this comment.
RepriceSubcPurchLineOnScheduleChange performs PurchaseHeader.Get before checking the same cheap applicability guards that GetSubcontractingPrice already applies (Type = Item, No. <> '', Operation No. <> ''). Only 'Prod. Order No.' is checked before the Get, so every production-linked subcontracting purchase line pays an unnecessary database lookup on every schedule change (Planned Receipt Date / Order Date validation), even when the line will ultimately be excluded from repricing by those other guards.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
if (PurchaseLine.Type <> PurchaseLine.Type::Item) or (PurchaseLine."No." = '') or (PurchaseLine."Operation No." = '') then
exit;
PurchaseHeader.SetLoadFields(Status);
if not PurchaseHeader.Get(PurchaseLine."Document Type", PurchaseLine."Document No.") then
exit;
if PurchaseHeader.Status <> PurchaseHeader.Status::Open then
exit;
GetSubcontractingPrice(PurchaseLine);Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
Good Sense Reviewer - Round 1Recommendation: Request ChangesWhat this PR doesThis change tightens subcontracting repricing so carry-out keeps manual or calculated worksheet costs when it should, avoids repricing released purchase orders, and reprices when Order Date changes even if Planned Receipt Date does not. The pricing logic is now split so callers can detect whether the current worksheet value came from the automatic price lookup before replacing it. The fix matches the main code paths. The requisition-to-purchase event fires after manufacturing fields are transferred and before the line is inserted, so it has the routing context needed to reprice. The purchase-line date logic assigns Order Date directly from Planned Receipt Date in some paths, so gating the Planned Receipt Date subscriber on the resulting Order Date is the right shape. The added tests cover the reported regressions, but a new CodeCop warning currently fails the W1 build. Problem-solution fitFit: Partial The reported regressions are about keeping subcontracting Direct Unit Cost aligned with the correct pricing date without overwriting intentional worksheet or released-order values. The functional diff addresses those paths directly, but the PR cannot pass its required build until the new analyzer warning is fixed. SuggestionsS1 (🔴 High): Fix the new CodeCop warning that fails the build Risk assessment and necessityRisk: Necessity: The functional change is justified. Without it, subcontracting purchase lines can keep a cost selected for the wrong effective date, or a follow-up reprice can overwrite a valid worksheet value. The scope is focused on those regressions, but the build-blocking declaration order must be corrected before merge.
|
What & why
Follow-up to #10917 and AB#648535.
The original date-effective repricing fix exposed four uncovered paths:
This change detects whether the transferred worksheet value is the automatically selected price before repricing it, uses the standard subcontracting worksheet cost formula as the no-price fallback, skips financial repricing for non-open purchase orders, and keys planned-date repricing on the resulting Order Date.
Coverage
Adds regression tests for:
Backports
The same correction is applied to:
releases/29.x: [Backport 29.x] Bug 648535: Reprice subcontracting lines after scheduling (#10917) #11197 (02e7a0dba2)releases/29.0: [Backport 29.0] Bug 648535: Reprice subcontracting lines after scheduling (#10917) #11198 (327c1d82c5)Validation
git diff --checkpasses.