Bug 648535: Reprice subcontracting lines after scheduling - #11292
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
GitHub Actions
Inactive
github-actions
Bot
added
AL: Apps (W1)
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.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 255f96a1-3a67-418c-9b69-ca50ae31e5b0
Chethan Thopaiah (ChethanT)
changed the title
Guard zero expected output, avoid unnecessary record loads, and cover direct no-price fallback calculations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 255f96a1-3a67-418c-9b69-ca50ae31e5b0
There was a problem hiding this comment.
🟡 Changes recommended
There is a confirmed pricing-path correctness issue in TryGetSubcPriceListCostForPurchLine (treating zero as a successful lookup) and a non-deterministic test record reload via PurchaseLine.Find() that should be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Apps/W1/Subcontracting/Test/Tests/SubcPricingTest.Codeunit.al:421
- PurchaseLine.Find() here runs without filters and can reposition the record to an unrelated purchase line, making the test non-deterministic when other purchase lines exist in the test company. Reload the specific line by primary key after releasing the header.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
|
In OnInsertPurchOrderLineOnAfterTransferFromReqLineToPurchLine, the guard Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6 |
|
TryGetSubcPriceListCostForPurchLine() returns false both for the expected 'no matching price list' case and for the unexpected case where TryFindProdOrderRtngLine() cannot find the production routing line. This new early exit treats both outcomes as benign, so a missing routing line is silently swallowed here instead of surfacing as a defect the way GetSubcPriceForPurchLine() still does on its fallback path (which calls the non-Try GetProdOrderRtngLine() that raises an error via FindFirst() if no routing line exists). Consider distinguishing the two failure causes, or raising an explicit error when the routing line is missing, so a data/setup problem is not masked as 'no applicable price'. Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6 |
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
One newly added test has a setup bug (Work Center Direct Unit Cost is created as 0) that can make its assertions unreliable and cause test failures unrelated to the intended behavior.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Apps/W1/Subcontracting/Test/Tests/SubcPricingTest.Codeunit.al:253
- In NoMatchPriceListPreservesCalculatedWorksheetCostForTimeBasedRouting, the Work Center is created via CreateSubcontractingItemWithSingleOperationRouting, which calls LibraryMfgManagement.CreateWorkCenterWithCalendar(WorkCenter, 0) (Direct Unit Cost = 0). With a zero routing rate, both RoutingRate and the calculated worksheet cost can end up 0, making Assert.AreNotEqual(RoutingRate, CalculatedWorksheetCost) unreliable and potentially failing the test for the wrong reason. Set a nonzero Work Center Direct Unit Cost as part of this test’s setup so the routing multiplier produces a distinguishable worksheet cost.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
This test hand-rolls Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6 |
There was a problem hiding this comment.
🟡 Changes recommended
Subc price fallback logic risks runtime/incorrect behavior due to partial-record SetLoadFields use and a possible divide-by-zero in minimum-amount pricing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
Good Sense Reviewer - Round 3Recommendation: Request ChangesWhat this PR doesSince round 2, the change now compares the worksheet value with the requisition line's own automatic cost before repricing the final purchase line. It also stops the requisition-line minimum-quantity miss from writing an automatic zero and adds tests for the two previously open paths. The functional changes fit the reported pricing problem: the final purchase-line date is used when the worksheet value is automatic, and intentional worksheet values are preserved. The remaining issue is merge readiness: the current build fails on a new analyzer warning in the added test helper. Status of previous suggestions
New observations (commits since round 2)S4 (🔴 High): Rename helper parameter to unblock CI Risk assessment and necessityRisk: Necessity: The change is needed because generated subcontracting purchase lines can be backward-scheduled to an Order Date different from the first price lookup date. Without it, users can get the wrong price or must manually correct generated purchase costs.
|
- Guard GetPriceByUOM against divide-by-zero when PriceListQty is 0 while a Minimum Amount is configured. - Extend SetLoadFields on the fallback Prod. Order Routing Line so Type, Unit Cost Calculation, Direct Unit Cost, Expected Operation Cost Amt. and Expected Capacity Ovhd. Cost are available when the no-price-list fallback path runs (previously only Standard Task Code was loaded, which could error or silently zero out on fallback). - Add SetLoadFields to the Prod. Order Line lookup in GetNonPriceListDirectCost to avoid materializing unused columns. - Avoid computing the subcontracting price twice per requisition line during Carry Out / Make Order: GetAutomaticSubcCostForReqLine now has an overload that returns the Prod. Order Routing Line it looked up, and GetSubcPriceForPurchLine accepts that record to skip a redundant Prod. Order Routing Line lookup for the same operation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good Sense Reviewer - Round 4Recommendation: Request ChangesWhat this PR doesThe latest commit reuses the production routing line between the automatic worksheet-cost check and final purchase-line repricing, adds load-field declarations, and avoids a minimum-amount division when the price quantity is zero. These changes are consistent with the existing pricing fix, but the round 3 analyzer blocker is still present in the test helper. Status of previous suggestions
New observations (commits since round 3)None. The routing-line reuse and zero-quantity guard do not introduce a new review issue. Risk assessment and necessityRisk: Subcontracting Necessity: The repricing change is needed to select the correct effective-date cost without overwriting manual values. Rename the helper parameter so the tested fix can pass validation and merge.
|
…ling (#10917) (#11198) ## Backport of #10917 to `releases/29.0` Backports [#10917](#10917) — *Bug 648535: Reprice subcontracting lines after scheduling* — to the `releases/29.0` release branch. ### What & why Subcontracting purchase lines were priced before backward scheduling had finalized the purchase-line Order Date and before routing context was transferred from the requisition line, so a subcontractor price valid for the purchase-header date could be selected instead of the price valid for the final purchase-line date. The review follow-up also: - preserves the worksheet's calculated Units/Time cost when no subcontractor price matches; - preserves manually overridden worksheet costs during carry-out; - keeps date scheduling editable on released purchase orders without changing financial terms; - reprices lead-time-only changes when the resulting Order Date crosses a price boundary. ### Source - Source PR: #10917 - Source squash-merge commit: `049258b463302e36649979d241a6f122c7eb87c0` - Original backport commit: `c1924162a` - Corrective follow-up commit: `327c1d82c5` - Main follow-up PR: #11292 - ADO: [AB#648535](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/648535) ### Cherry-pick / conflict resolution The product changes applied cleanly. The test file conflicted because the release branch predates the `Subc. Management Library` helper refactor. Only the relevant regression tests were applied, using equivalent local helper procedures already required by this backport. No unrelated `main` tests were introduced. The resolution is identical to the `releases/29.x` backport. ### Validation - Static verification: no conflict markers or duplicate procedures; `git diff --check` passes. - Four corrective regressions cover no-price fallback, manual worksheet overrides, released-order date edits, and lead-time-only Order Date changes. - Full AL compile and test execution are deferred to the AL-Go PR build because local symbol packages are unavailable and sandbox policy blocks CoreXT package-state initialization.⚠️ Do not merge until the AL-Go PR build passes. **Not** auto-merged. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 255f96a1-3a67-418c-9b69-ca50ae31e5b0
…ling (#10917) (#11197) ## Backport of #10917 to `releases/29.x` Backports [#10917](#10917) — *Bug 648535: Reprice subcontracting lines after scheduling* — to the `releases/29.x` release branch. ### What & why Subcontracting purchase lines were priced before backward scheduling had finalized the purchase-line Order Date and before routing context was transferred from the requisition line, so a subcontractor price valid for the purchase-header date could be selected instead of the price valid for the final purchase-line date. The review follow-up also: - preserves the worksheet's calculated Units/Time cost when no subcontractor price matches; - preserves manually overridden worksheet costs during carry-out; - keeps date scheduling editable on released purchase orders without changing financial terms; - reprices lead-time-only changes when the resulting Order Date crosses a price boundary. ### Source - Source PR: #10917 - Source squash-merge commit: `049258b463302e36649979d241a6f122c7eb87c0` - Original backport commit: `6a3802a0a` - Corrective follow-up commit: `02e7a0dba2` - Main follow-up PR: #11292 - ADO: [AB#648535](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/648535) ### Cherry-pick / conflict resolution The product changes applied cleanly. The test file conflicted because the release branch predates the `Subc. Management Library` helper refactor. Only the relevant regression tests were applied, using equivalent local helper procedures already required by this backport. No unrelated `main` tests were introduced. ### Validation - Static verification: no conflict markers or duplicate procedures; `git diff --check` passes. - Four corrective regressions cover no-price fallback, manual worksheet overrides, released-order date edits, and lead-time-only Order Date changes. - Full AL compile and test execution are deferred to the AL-Go PR build because local symbol packages are unavailable and sandbox policy blocks CoreXT package-state initialization.⚠️ Do not merge until the AL-Go PR build passes. **Not** auto-merged. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 255f96a1-3a67-418c-9b69-ca50ae31e5b0
- Rename UnitCostCalculation parameter to UnitCostCalculationType in CreateNoPriceSubcontractingPurchaseLine to avoid shadowing the codeunit's global variable of the same name (AA0244). - Reorder var declarations in OnInsertPurchOrderLineOnAfterTransferFromReqLineToPurchLine so the Record var precedes the Codeunit var, per AA0021 ordering rules. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good Sense Reviewer - Round 5Recommendation: AcceptWhat this PR doesThe latest commit fixes the remaining analyzer warnings by renaming the conflicting test-helper parameter and ordering the local variables correctly. These changes are behavior-neutral, preserve the subcontracting pricing fix, and allow the validated code to pass the required build and analyzer checks. Status of previous suggestions
New observations (commits since round 4)None - the latest commit only resolves analyzer warnings and introduces no behavioral change. Risk assessment and necessityRisk: Subcontracting Necessity: The pricing changes are needed to use the final scheduled date while preserving manual and fallback costs. The latest cleanup is needed to clear the analyzer gate and make the PR mergeable.
|
Predrag Maricic (PredragMaricic)
left a comment
Predrag Maricic (PredragMaricic)
left a comment
There was a problem hiding this comment.
The final-date repricing flow and no-price fallback formulas are well covered, but requisition-line recalculation can retain stale price-list metadata when no minimum-quantity tier applies. This is a correctness regression already fixed in the merged 29.x backport. Please clear that state on the no-match path and add focused regression coverage.
…ForReqLine early exit When GetPriceByUOM finds no applicable price tier, GetSubcPriceForReqLine now clears Subc. Pricelist Cost, Subc. UoM for Pricelist, and both conversion ratios (restoring them to 1) instead of leaving stale values from a prior calculation on the requisition line. Adds regression coverage that starts with populated price-list metadata, recalculates with no applicable tier, and verifies all four fields are reset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…SubcCostForReqLine Adds SetLoadFields (Standard Task Code, Type, Unit Cost Calculation, Direct Unit Cost, Expected Operation Cost Amt., Expected Capacity Ovhd. Cost) before fetching the routing line, matching the equivalent restriction already applied in the sibling TryGetSubcPriceListCostForPurchLine, avoiding an inconsistent full-row load on this per-line hot path. Found by local AL review (al-performance-review). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good Sense Reviewer - Round 6Recommendation: Request ChangesWhat this PR doesThe latest commits clear stale price-list fields when no minimum-quantity tier applies and restrict the routing-line fields loaded by the automatic-cost calculation. These product-code changes preserve the earlier pricing corrections, but the new regression test changes a primary-key field with Status of previous suggestions
New observations (commits since round 5)S5 (🔴 High): Recreate the price tier with the new key Risk assessment and necessityRisk: Subcontracting Necessity: Clearing stale price-list state is needed when recalculation no longer finds an applicable quantity tier. Fix the test setup so the financially sensitive fallback behavior is validated before merge.
|
Predrag Maricic (PredragMaricic)
left a comment
Predrag Maricic (PredragMaricic)
left a comment
There was a problem hiding this comment.
The final pricing flow correctly preserves manual worksheet overrides while repricing automatically calculated costs against the resulting purchase-line Order Date. The no-price fallback matches the standard unit/time calculations, guards zero expected output, distinguishes an intentional zero price from a missing tier, and resets stale requisition price-list metadata. Released-order date edits retain their financial terms. The regression coverage exercises each changed path. No blocking issues found.
Use Record.Rename when changing Minimum Quantity because it is part of the Subcontractor Price primary key. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good Sense Reviewer - Round 7Recommendation: AcceptWhat this PR doesThe latest commit fixes the regression test setup by renaming the subcontractor price tier when changing The product changes still match the reported bug: subcontracting purchase lines are repriced from the final line Status of previous suggestions
New observations (commits since round 6)None - the latest commit only addresses the previous test setup blocker. Risk assessment and necessityRisk: Subcontracting Necessity: The latest change is necessary because the regression test must reach the recalculation path instead of failing while changing a primary-key field. The overall fix remains needed to price subcontracting purchase lines from the final
|
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.