[Master]- Production Order - WIP shows incorrect consumption amount.#9062
Conversation
|
Could not find linked issues in the pull request description. Please make sure the pull request description contains a line that contains 'Fixes #' followed by the issue number being fixed. Use that pattern for every issue you want to link. |
There was a problem hiding this comment.
Pull request overview
This PR addresses incorrect material consumption totals in the “Inventory Valuation - WIP” report by preventing consumption amounts from being carried over and double-counted when subsequent non-WIP value entries are processed. It also adds a regression test to reproduce and validate the scenario.
Changes:
- Add a new automated test covering the “consumption not doubled by trailing non-WIP entries” scenario.
- Fix report logic by resetting WIP-calculation accumulator variables at the start of each
Value Entryrecord processing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Layers/W1/Tests/SCM-Manufacturing/SCMManufacturingReportsII.Codeunit.al | Adds a regression test validating that reported material consumption isn’t double-counted. |
| src/Layers/W1/BaseApp/Manufacturing/Reports/InventoryValuationWIP.Report.al | Resets per-record WIP calculation variables earlier to prevent carry-over into non-WIP entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot PR ReviewIteration 7 · Outcome: completed
Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630 Orchestrator pre-filter (13 file(s) excluded)
Findings produced by the Copilot CLI agent against BCQuality at |
…WIP-shows-incorrect-consumption-amount
The fix in this PR moves ValueOfWIP/ValueOfMatConsump/ValueOfCap/ValueOfOutput/ValueOfInvOutput1/ValueOfExpOutput1/ValueOfExpOutput2 resets to the top of Value Entry.OnAfterGetRecord so they no longer carry over into non-WIP records (correctly fixing the double-counting bug covered by the new test).However, ValueOfRevalCostAct and ValueOfRevalCostPstd sit in the exact same trigger and follow the same accumulate-then-copy pattern (accumulated via '+=' at lines 245/254, then copied into NcValueOfRevalCostAct/NcValueOfRevalCostPstd via ':=' at lines 267-268), but they are only zeroed once in OnPreDataItem (lines 342-343), never per record like the variables that were just fixed. Because the copy at 267-268 overwrites Nc* with the full running total, once NcValueOfRevalCostAct/NcValueOfRevalCostPstd are reset to 0 at a CountRecord = LengthRecord checkpoint (lines 287-288) the next WIP record can immediately re-assign the stale, never-reset cumulative ValueOfRevalCostAct/ValueOfRevalCostPstd back into them, effectively reintroducing revaluation amounts from earlier batches into the current one. This is the same missing-reset root cause the PR just fixed for the sibling variables, left unaddressed for these two. Recommend resetting ValueOfRevalCostAct and ValueOfRevalCostPstd to 0 alongside the other per-record resets added in this PR (and confirming whether NcValueOfRevalCostAct/NcValueOfRevalCostPstd should accumulate via '+=' rather than overwrite via ':=', since every other Nc* variable uses '+=' at lines 260-266). Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why |
Agentic PR Review - Round 1Recommendation: Request ChangesWhat this PR doesThis PR moves the per-entry WIP amount resets before the WIP check in report 5802, so non-WIP value entries after a consumption entry no longer reuse stale consumption values. The new test posts consumption and output, runs "Inventory Valuation - WIP", and checks that the reported material consumption equals the consumption value entries. For the changed W1 report, the root cause is fixed: the amount variables are now cleared before both the WIP and non-WIP paths, and the later total increments see zero for non-WIP entries. The remaining problem is scope: the CZA report with the same caption still has the old reset pattern and can still add stale SuggestionsS1 - Fix the CZA WIP report too S2 - Keep the new test OnPrem scoped Risk assessment and necessityRisk: This is a financial manufacturing report, so incorrect consumption totals are high impact. The W1 code change is narrow and does not change public APIs or events, but the same stale-value pattern remains in the CZA report with caption "Production Order - WIP". Necessity: The bug is concrete and important: the work item shows consumption in the report differs from Value Entries. The reset change is needed, but the PR scope is too narrow while the analogous report in this repo still has the same defect.
|
alexei-dobriansky
left a comment
There was a problem hiding this comment.
Please check if the copilot's suggestions (about resetting ValueOfCostPstdToGL) are valid and also if the CZ report is available to you and contains the same issue.
Thank you!
Agentic PR Review - Round 2Recommendation: Request ChangesWhat this PR doesThe new commit fixes the CZA report too, and it also clears I do not see Status of previous suggestions
New observations (commits since round 1)S3 - Consider CZA regression coverage Risk assessment and necessityRisk: This is a financial manufacturing report, so wrong totals are high impact. The code changes are narrow and do not change public APIs or events, but the new test still lacks the Necessity: The bug is concrete and important because the report consumption value can differ from the Value Entries. The reset change is needed, and the remaining requested change is a small test metadata fix.
|
|
Approved |
…WIP-shows-incorrect-consumption-amount
…WIP-shows-incorrect-consumption-amount
Fixes AB#641358
Workitem :-Bug 641358: [master] [ALL-E] Production Order - WIP shows incorrect consumption amount.