[Backport 29.x] Bug 648535: Reprice subcontracting lines after scheduling (#10917) - #11197
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>
attilatoury
left a comment
There was a problem hiding this comment.
Critical review
Reviewed commit 6a3802a0a30c9740cd015c7353c60c7fc62ddae9 against releases/29.x. Recommend addressing the following before merging; details and suggested regression coverage are in four inline comments:
- [P1] Carry-out replaces correctly calculated time/UOM-based costs with the raw routing rate when no subcontractor price matches.
- [P1] The new date handlers reject date changes on released subcontracting POs through discount validation's open-status check.
- [P2] Carry-out silently discards manually entered worksheet costs, including when no price-period change occurs.
- [P2] Lead-time-only rescheduling can still leave a stale price because the planned-date equality guard ignores a changed order date. This is an incomplete-fix case, not a new regression.
These findings are based on source-level tracing, not runtime reproductions. Runtime tests were not run. The five added tests cover open orders with matching prices, base UOM, and a one-minute operation, and do not exercise the cases above.
PR Shepherd plan ps-cc34ce23d2666cb9f8cf92bd42ce62692b4290a447a17c6b761b8e18fedc3b4f
Summary{"summary":"Four inline findings are valid from source tracing. The post-transfer repricing call can replace the requisition worksheet's calculated or manually overridden cost; the date subscribers invoke line-discount validation on released orders; and the planned-date equality guard misses lead-time changes that alter Order Date. The fix will preserve worksheet overrides and calculated fallbacks, reprice only automatically selected prices, keep released-order scheduling from changing financial terms, and trigger repricing when the resulting pricing date changes. Regression coverage will include no matching price with nontrivial runtime/UOM conversion, a manual worksheet override, released purchase-order date changes, and lead-time-only movement across a price boundary."}Approved actions
approve ps-cc34ce23d2666cb9f8cf92bd42ce62692b4290a447a17c6b761b8e18fedc3b4f |
|
approve ps-cc34ce23d2666cb9f8cf92bd42ce62692b4290a447a17c6b761b8e18fedc3b4f |
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.
🟡 Changes recommended
The new non-price-list cost calculation can divide by zero on Total Exp. Oper. Output (Qty.), which should be guarded to avoid runtime failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Backport to releases/29.x of the subcontracting pricing fix to ensure subcontracting purchase lines are repriced after scheduling/field transfers finalize the effective Order Date, so date-effective subcontractor prices are selected correctly.
Changes:
- Reapplies subcontractor pricing after requisition-to-purchase-line manufacturing fields are transferred.
- Reprices subcontracting purchase lines when scheduling-related date validations shift the effective Order Date (Planned Receipt Date / Order Date).
- Adds regression tests covering backward-scheduled pricing, rescheduling repricing, and preservation of financial terms on released orders.
File summaries
| File | Description |
|---|---|
| src/Apps/W1/Subcontracting/Test/Tests/SubcPricingTest.Codeunit.al | Adds regression tests and local helper procedures to validate date-effective subcontractor pricing behavior on 29.x. |
| src/Apps/W1/Subcontracting/App/src/Purchase/SubcPurchaseLineExt.Codeunit.al | Subscribes to Purchase Line date validations to trigger subcontracting repricing on schedule changes (with release-status bypass). |
| src/Apps/W1/Subcontracting/App/src/Purchase/SubcPriceManagement.Codeunit.al | Refactors purchase-line price retrieval to support “try price-list cost” logic and adds non-price-list direct-cost calculation. |
| src/Apps/W1/Subcontracting/App/src/Manufacturing/SubcReqWkshMakeOrd.Codeunit.al | Reprices worksheet-created subcontracting purchase lines after requisition fields are transferred, when the original cost was price-list-derived. |
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 doesThis change reprices subcontracting purchase lines after requisition fields are copied and after schedule date changes, so The functional approach matches the reported pricing-date problem. The make-order publisher was verified on the target branch: it runs after requisition fields are transferred and before the purchase line is inserted. The purchase-line date flow also assigns Problem-solution fitFit: Strong The bug says subcontracting pricing can use a date that does not match the final purchase-line SuggestionsS1 (🔴 High): Build fails on variable order Risk assessment and necessityRisk: The functional regression surface is subcontracting purchase-line pricing, including Necessity: The change is required. Without it, a subcontracting purchase line can keep a cost selected for the wrong effective date. The scope is appropriate for a backport once the build-blocking warning is fixed.
|
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.
🟢 Approval recommended
The changes are well-scoped to subcontracting repricing, include safeguards for released documents, and add regression tests covering the key behavioral paths described in the PR.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 255f96a1-3a67-418c-9b69-ca50ae31e5b0
|
Proactively applied the minimum-quantity-tier correction from the main review in |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core purchase-line validation/pricing behavior via new event subscribers and pricing fallback logic, which warrants full CI validation and careful human review of downstream behavioral impact.
Review details
- 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 fix the prior build warning by sorting the test variables, and they extend the no-price fallback so direct purchase-line repricing can fall back when no applicable price tier matches. The main date-effective repricing approach still matches the reported bug, and the target-branch event timing was verified. The new minimum-quantity fallback is incomplete. It handles direct purchase-line repricing, but the worksheet pricing path still ignores the new Status of previous suggestions
New observations (commits since round 1)S2 (🔴 High): Worksheet minimum tier still zeroes cost Risk assessment and necessityRisk: The regression surface is subcontracting purchase-line pricing, especially Necessity: The change is required because subcontracting lines can otherwise keep a price selected for the wrong effective date. The backport scope is appropriate, but the minimum-quantity fallback must cover both direct purchase-line pricing and worksheet pricing before merge.
|
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
A pricing path now exits early without clearing requisition-line subcontracting price-list fields, which can leave stale state when no tier is applicable.
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
| if not GetPriceByUOM(SubcontractorPrice, PriceListQty, PriceListCost) then | ||
| exit; |
Good Sense Reviewer - Round 3Recommendation: Request ChangesWhat this PR doesThe latest commits apply the follow-up fixes for automatic worksheet cost detection, final purchase-line date pricing, minimum-quantity fallback, strict routing lookup, and release-compatible tests. The previous worksheet minimum-tier problem is now covered: requisition pricing consumes the GetPriceByUOM match result, and the new test keeps the calculated fallback when no tier applies. The target-branch publishers were verified. The requisition-to-purchase event runs after requisition fields are transferred and before line insert, and the purchase-line date publisher recalculates Order Date before the after-validate subscribers run. However, the test app does not compile because the new tests call LibraryRandom without declaring that codeunit variable. Status of previous suggestions
New observations (commits since round 2)S3 (🔴 High): Test app misses LibraryRandom variable Risk assessment and necessityRisk: The functional regression surface is subcontracting purchase-line pricing, including worksheet carry-out, minimum-quantity fallback, released-order scheduling, and date-effective prices. The code changes are narrow and guarded by subcontracting, production-order, operation, temporary-record, upgrade-context, and purchase-header status checks. The current concrete risk is merge readiness: the Subcontracting Test app fails compilation because a new test dependency is missing. Necessity: The change is required because subcontracting lines can otherwise use a price selected for a date that does not match the final purchase-line Order Date. The scope is appropriate for a backport once the compile blocker is fixed.
|
Backport of #10917 to
releases/29.xBackports #10917 — Bug 648535: Reprice subcontracting lines after scheduling — to the
releases/29.xrelease 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
049258b463302e36649979d241a6f122c7eb87c06a3802a0a02e7a0dba2Cherry-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.Validation
git diff --checkpasses.