[Backport 29.0] Bug 648535: Reprice subcontracting lines after scheduling (#10917) - #11198
Conversation
[AB#648535](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/648535) 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. This could select a subcontractor price valid for the purchase-header date rather than the price valid for the final purchase-line date. This change reapplies subcontractor pricing after requisition-to-purchase-line manufacturing fields are transferred. It also reprices existing subcontracting lines after Planned Receipt Date reschedules Order Date and after Order Date is validated directly. The existing guarded price helper keeps ordinary purchase lines unchanged. Internal ADO bug: https://dev.azure.com/dynamicssmb2/Dynamics%20SMB/_workitems/edit/648535 - [x] I read the full diff and it contains only changes I intended. - [ ] I built the affected app(s) locally with no new analyzer warnings. - [ ] I ran the change in Business Central and confirmed it behaves as expected. - [x] I added or updated tests for the new behavior, or explained below why none are needed. **What I tested and the outcome** *(required - be specific: scenarios, commands, screenshots for UI changes)* - Added regression coverage proving initial purchase-order creation uses the price valid on the final backward-scheduled Order Date. - Added regression coverage proving Expected Receipt Date rescheduling selects the price valid on the newly derived Order Date. - Added regression coverage proving direct Order Date validation reapplies date-effective pricing. - Ran the BCQuality AL branch review at Medium severity: no findings. - Local app build and Business Central execution are pending because Docker Desktop's Windows engine returned HTTP 500 after its service was started. The BCApps AL-Go PR build will provide build, analyzer, and test execution results. No schema, public API, permission, upgrade, or data-migration changes. The new subscribers reuse existing price selection and feature-flag guards. Non-subcontracting purchase lines remain excluded by the existing item, production-order, and operation checks. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GitHub Actions
Inactive
github-actions
Bot
added
AL: Apps (W1)
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
|
Applied the same corrective follow-up as #11197 in 327c1d8. It preserves calculated no-price fallback costs and manual worksheet overrides, keeps released-order scheduling editable without changing financial terms, and handles lead-time-only Order Date shifts. The four corresponding regression tests are included. |
There was a problem hiding this comment.
🟡 Changes recommended
A division-by-zero edge case was introduced in the new non-price-list direct cost calculation (see stored review comment) that can cause runtime failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Backport to releases/29.0 that ensures subcontracting purchase lines are repriced using the final (possibly backward-scheduled) purchase-line Order Date, and that scheduling-related edits don’t unintentionally alter financial terms (especially on released orders).
Changes:
- Reprice subcontracting purchase lines after date scheduling/validation so date-effective vendor pricing is applied to the resulting Order Date.
- Preserve worksheet-derived or manually overridden costs during carry-out when repricing should not override them.
- Add regression tests covering backward-scheduled pricing, rescheduling repricing, released-order date edits, and lead-time-only Order Date shifts.
File summaries
| File | Description |
|---|---|
| src/Apps/W1/Subcontracting/Test/Tests/SubcPricingTest.Codeunit.al | Adds regression tests and local helpers for date-effective pricing and carry-out preservation scenarios. |
| src/Apps/W1/Subcontracting/App/src/Purchase/SubcPurchaseLineExt.Codeunit.al | Adds subscribers to trigger subcontracting repricing on Planned Receipt Date / Order Date changes, with a guard for non-open headers. |
| src/Apps/W1/Subcontracting/App/src/Purchase/SubcPriceManagement.Codeunit.al | Refactors purchase-line pricing to support “no price match” fallback and exposes a helper to compute price-list cost without mutating the line. |
| src/Apps/W1/Subcontracting/App/src/Manufacturing/SubcReqWkshMakeOrd.Codeunit.al | Reapplies subcontractor pricing after requisition→purchase transfer when the transferred value reflects the pre-scheduling price. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Good Sense Reviewer - Round 1Recommendation: Request ChangesWhat this PR doesThe change backports the subcontracting repricing fix to the 29.0 branch. It reprices subcontracting purchase lines after requisition fields are transferred and after schedule-related date changes, so the final purchase-line Order Date drives date-effective subcontractor pricing. It also tries to preserve worksheet and manual costs and keeps released-order date edits from changing financial terms. The BaseApp event publisher for Problem-solution fitFit: Partial The date-based pricing issue is covered for creation and rescheduling, and the tests exercise those paths. The no-price fallback is part of the correction, but one new formula is not safe when expected output quantity is 0, so the solution is not complete yet. SuggestionsS1 (🔴 High): Guard zero expected output before dividing Risk assessment and necessityRisk: Moderate to high because Necessity: Justified. Without the change, a subcontracting purchase line can keep a price that is valid for a different date than its final Order Date. The backport scope matches the bug, but the fallback path should be made safe before merge.
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 255f96a1-3a67-418c-9b69-ca50ae31e5b0
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
|
Applied the accepted #11292 review follow-up in |
There was a problem hiding this comment.
🟡 Changes recommended
A confirmed pricing-path edge case can incorrectly treat “no minimum-quantity price match” as a successful lookup and set Direct Unit Cost to 0, which needs correction 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:1063
- PurchaseLine.Find() here is relying on whatever filters/key state happens to be on the variable after releasing the header. That’s brittle and can become flaky if additional purchase lines exist in the same test run. Re-fetch the exact line by primary key instead.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
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
A new test uses PurchaseLine.Find() without deterministic filters/key after release, which can make the test flaky or target the wrong record.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/Apps/W1/Subcontracting/Test/Tests/SubcPricingTest.Codeunit.al:1120
PurchaseLine.Find();after releasing the header is not deterministic unless filters/key are set; it can move the record pointer to an unrelated line. To reliably reload the same line afterReleasePurchaseDocument, re-GET by primary key.
src/Apps/W1/Subcontracting/App/src/Manufacturing/SubcReqWkshMakeOrd.Codeunit.al:52- In OnInsertPurchOrderLineOnAfterTransferFromReqLineToPurchLine, the variable name
ReqLineDatePriceListCostis misleading:TryGetSubcPriceListCostForPurchLinereturns the Direct Unit Cost (after UOM/currency conversion), not the raw price list cost. This makes the intent of the equality check harder to understand and increases the risk of misuse later.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
Good Sense Reviewer - Round 2Recommendation: Request ChangesWhat this PR doesThe new commits address the previous zero-output fallback issue and add coverage for no-price fallbacks, minimum-quantity tiers, released-line date edits, and lead-time-only rescheduling. The original date-based repricing path is still mostly in place: the creation subscriber compares the transferred cost with the requisition-date price before repricing with the final purchase-line Order Date, and the date subscribers reprice open subcontracting lines when Order Date changes. The zero-output division is now guarded, but the new minimum-quantity false return leaves a carry-out path where final-date repricing can be skipped. If the requisition-date price record has no applicable minimum-quantity tier, the creation subscriber exits before checking the final purchase-line Order Date. Status of previous suggestions
New observations (commits since round 1)S2 (🔴 High): Carry-out can skip final-date pricing after a quantity miss Risk assessment and necessityRisk: High because the affected value is Necessity: The change is justified because subcontracting pricing must use the final purchase-line Order Date after backward scheduling. The scope is right for the reported bug, but the carry-out guard should still allow final-date repricing when the requisition-date lookup fails only because no minimum-quantity tier applies.
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 255f96a1-3a67-418c-9b69-ca50ae31e5b0
|
Synchronized the latest #11292 review fixes in |
There was a problem hiding this comment.
🟡 Changes recommended
There is a confirmed divide-by-zero risk in the updated minimum-amount adjustment logic when repricing with zero quantity (see stored comment).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
Good Sense Reviewer - Round 3Recommendation: Request ChangesWhat this PR doesThe latest commit addresses the previous carry-out pricing gap by comparing the requisition line with the automatic subcontracting cost, then repricing the purchase line with the final line Order Date when the cost was not manually overridden. It also keeps the fallback path for no applicable price and adds more regression coverage for final-date pricing and minimum-quantity fallback. The pricing flow now matches the reported scenario: automatic worksheet costs can be replaced by the price valid on the final purchase-line date, while manual worksheet overrides are preserved. The remaining blocker is not the pricing logic itself; the new test code does not compile on the target branch because a library variable was not backported. Status of previous suggestions
New observations (commits since round 2)S3 (🔴 High): Declare LibraryRandom for new tests Risk assessment and necessityRisk: High until the compile error is fixed, because the Subcontracting test app cannot build. The functional change still touches Necessity: The change is justified because subcontracting pricing must use the final purchase-line Order Date after scheduling. The latest product code is targeted and matches the needed behavior, but the backport is not merge-ready while its tests reference an undeclared library.
|
…declare LibraryRandom - SubcPriceManagement.Codeunit.al: GetPriceByUOM now guards against PriceListQty = 0 before dividing Minimum Amount by it, preventing a runtime error when repricing with zero quantity. - SubcPricingTest.Codeunit.al: declare the missing LibraryRandom global variable used by the new regression tests (fixes AL0118 compile error causing all Build Apps checks to fail). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good Sense Reviewer - Round 4Recommendation: Accept with SuggestionsWhat this PR doesSince round 3, the change declares the missing Status of previous suggestions
New observations (commits since round 3)S4 (🟠 Moderate): Cover zero-quantity minimum amount pricing Risk assessment and necessityRisk: Moderate because the changed code still sets Necessity: Justified. Subcontracting pricing must use the final purchase-line Order Date after scheduling, and the round-4 changes are targeted cleanup for review-found issues.
|
Addresses round-4 Good Sense Reviewer suggestion S4: add coverage for a matched Subcontractor Price with Minimum Amount set repriced at zero purchase quantity, verifying the divide-by-zero guard keeps the price list Direct Unit Cost unadjusted instead of throwing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good Sense Reviewer - Round 5Recommendation: AcceptWhat this PR doesSince round 4, the change adds the missing regression test for zero quantity with Status of previous suggestions
New observations (commits since round 4)None - changes only addressed prior suggestions. Risk assessment and necessityRisk: Moderate because the full change updates Necessity: Justified. Without this change, a subcontracting purchase line can keep a subcontractor price that was selected for a different effective date than the final purchase-line Order Date. Users would need to notice and correct the cost manually.
|
a875420
into
releases/29.0
…11292) ## What & why Follow-up to microsoft#10917 and [AB#648535](https://dynamicssmb2.visualstudio.com/Dynamics%20SMB/_workitems/edit/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: - `releases/29.x`: microsoft#11197 (`02e7a0dba2`) - `releases/29.0`: microsoft#11198 (`327c1d82c5`) ## 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. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 255f96a1-3a67-418c-9b69-ca50ae31e5b0
Backport of #10917 to
releases/29.0Backports #10917 — Bug 648535: Reprice subcontracting lines after scheduling — to the
releases/29.0release 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:
Source
049258b463302e36649979d241a6f122c7eb87c0c1924162a327c1d82c5Cherry-pick / conflict resolution
The product changes applied cleanly. The test file conflicted because the release branch predates the
Subc. Management Libraryhelper refactor. Only the relevant regression tests were applied, using equivalent local helper procedures already required by this backport. No unrelatedmaintests were introduced. The resolution is identical to thereleases/29.xbackport.Validation
git diff --checkpasses.