Skip to content

[Master]- Production Order - WIP shows incorrect consumption amount.#9062

Open
neeleshsinghal wants to merge 5 commits into
microsoft:mainfrom
neeleshsinghal:bugs/-Bug-641358-Production-Order-WIP-shows-incorrect-consumption-amount
Open

[Master]- Production Order - WIP shows incorrect consumption amount.#9062
neeleshsinghal wants to merge 5 commits into
microsoft:mainfrom
neeleshsinghal:bugs/-Bug-641358-Production-Order-WIP-shows-incorrect-consumption-amount

Conversation

@neeleshsinghal

@neeleshsinghal neeleshsinghal commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Fixes AB#641358

Workitem :-Bug 641358: [master] [ALL-E] Production Order - WIP shows incorrect consumption amount.

@github-actions github-actions Bot added the From Fork Pull request is coming from a fork label Jul 3, 2026
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

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.

@neeleshsinghal neeleshsinghal requested a review from Copilot July 3, 2026 08:42
@github-actions github-actions Bot added the Linked Issue is linked to a Azure Boards work item label Jul 3, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Jul 3, 2026

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.

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 Entry record 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.

@neeleshsinghal neeleshsinghal added the SCM GitHub request for SCM area label Jul 3, 2026
@neeleshsinghal neeleshsinghal marked this pull request as ready for review July 4, 2026 09:54
@neeleshsinghal neeleshsinghal requested a review from a team July 4, 2026 09:54
@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Copilot PR Review

Iteration 7 · Outcome: completed

All applicable sub-skills completed; al-upgrade-review and al-ui-review returned not-applicable (no upgrade objects, no page/UI files in the diff).

Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630

Orchestrator pre-filter (13 file(s) excluded)

  • layer-disabled (knowledge) : 13 file(s)

Findings produced by the Copilot CLI agent against BCQuality at 822cae1b2771ac25f665f73369f69093bd4fd630. Reply 👎 on any inline comment to flag false positives.

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Agent} \quad \color{gray}{\texttt{\small Iteration\ 2}}$

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

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Request Changes

What this PR does

This 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 ValueOfMatConsump values.

Suggestions

S1 - Fix the CZA WIP report too
src/Apps/CZ/AdvancedLocalizationPack/app/Src/Reports/InventoryValuationWIPCZA.Report.al still resets ValueOfMatConsump only inside if not IsNotWIP(). Its totals add ValueOfMatConsump after every value entry, so the same non-WIP trailing entries can still double-count consumption in report 31133.

S2 - Keep the new test OnPrem scoped
Add [Scope('OnPrem')] to the new test. Every other test in codeunit 137310 has this scope, so the new test should not change where this report test is available.

Risk assessment and necessity

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


[AI-PR-REVIEW] version=1 system=github pr=9062 round=1 by=alexei-dobriansky at=2026-07-06T07:59:53Z lastSha=1275ad613aa1c5bfb475c3f9706d641a9b58ad9e suggestions=S1,S2

@alexei-dobriansky alexei-dobriansky 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.

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!

@v-ankitgoyal v-ankitgoyal self-assigned this Jul 6, 2026

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.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 2

Recommendation: Request Changes

What this PR does

The new commit fixes the CZA report too, and it also clears ValueOfCostPstdToGL before each value entry in both reports. This addresses the stale-value pattern for the W1 and CZA report totals.

I do not see ValueOfRevalCostAct and ValueOfRevalCostPstd as the same per-record reset problem. They are accumulated for the production order and used by ValueEntryOnPostDataItem(), so resetting them per value entry would change that calculation.

Status of previous suggestions
ID Title Status Author response
S1 Fix the CZA WIP report too Addressed The same reset pattern was added to report 31133.
S2 Keep the new test OnPrem scoped Not addressed No reply found.
New observations (commits since round 1)

S3 - Consider CZA regression coverage
The CZA report is a separate copy of the report logic. Add a CZA report test if the CZ test app can reuse the same manufacturing setup without too much duplication.

Risk assessment and necessity

Risk: 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 OnPrem scope used by the rest of the codeunit.

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.


[AI-PR-REVIEW] by=alexei-dobriansky round=2 sha=281a4367d60d93d849b56fbc3036bd9ca1fbab59 version=1 system=github pr=9062 at=2026-07-06T14:38:12Z lastSha=281a4367d60d93d849b56fbc3036bd9ca1fbab59 suggestions=S1:addressed,S2:notaddressed,S3:new parentRound=1

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Approved

@neeleshsinghal neeleshsinghal enabled auto-merge July 7, 2026 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

From Fork Pull request is coming from a fork Linked Issue is linked to a Azure Boards work item SCM GitHub request for SCM area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants