Skip to content

Fix subcontracting repricing regressions - #11292

Open
Chethan Thopaiah (ChethanT) wants to merge 1 commit into
mainfrom
bugs/648535-followup-202609091030
Open

Fix subcontracting repricing regressions#11292
Chethan Thopaiah (ChethanT) wants to merge 1 commit into
mainfrom
bugs/648535-followup-202609091030

Conversation

@ChethanT

@ChethanT Chethan Thopaiah (ChethanT) commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

What & why

Follow-up to #10917 and AB#648535.

The original date-effective repricing fix exposed four uncovered paths:

  • carry-out could replace a calculated time/UOM worksheet cost with the raw routing rate when no price matched;
  • date edits on released subcontracting purchase orders could fail through line-discount validation's open-status check;
  • carry-out could overwrite a manually entered worksheet cost;
  • lead-time-only rescheduling could change Order Date without changing Planned Receipt Date and therefore skip repricing.

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:

  • no matching price with a time-based routing multiplier;
  • manual worksheet Direct Unit Cost preservation;
  • Planned Receipt Date and Order Date edits on a released subcontracting purchase order;
  • lead-time-only Order Date movement across a price boundary.

Backports

The same correction is applied to:

Validation

  • git diff --check passes.
  • Local compile/test execution is blocked because CoreXT initialization cannot write its generated package state under the current sandbox policy. Global AL symbol download is also unavailable. The PR build is the executable validation gate; no local runtime pass is claimed.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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.)");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$\textbf{🟡\ Medium\ Severity\ —\ Agent}$

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$\textbf{🟠\ High\ Severity\ —\ Performance}$

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$\textbf{🟠\ High\ Severity\ —\ Performance}$

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

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 1

Recommendation: Request Changes

What this PR does

This 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 fit

Fit: 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.

Suggestions

S1 (🔴 High): Fix the new CodeCop warning that fails the build
The new NewLeadTime declaration is after Date variables, but CodeCop requires DateFormula variables before the remaining variable types. CI reports this as a new AA0021 warning at line 394, and the W1 build fails because new warnings fail the PR. Move NewLeadTime before the Decimal and Date variables so the build can pass.

Risk assessment and necessity

Risk: Direct Unit Cost drives subcontracting purchase costs, so wrong repricing can affect purchasing amounts. The implementation is narrow: it stays in the Subcontracting app, uses existing price logic, skips temporary and upgrade contexts, and avoids changing released-order financial terms. The current concrete blocker is merge readiness because the PR introduces a warning that fails the W1 build.

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.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11292 round=1 by=alexei-dobriansky at=2026-09-10T00:18:57.9626275Z lastSha=c6acb74ab120e9415ce860a5b15fba9f2abeb06b reviewKey=60d008a5b5ca6b99457fe55a9b3177c69ec87b7af8c53167079b774ca7c97cce suggestions=S1@f03922c3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AL: Apps (W1) Add-on apps for W1 Team: SCM GitHub request for SCM area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants