Skip to content

[Master]-Chance of success % does not validate correctly when using the action type "Update" in OpportunityChanges#8918

Open
DeepsShukla wants to merge 2 commits into
microsoft:mainfrom
DeepsShukla:bugs/Bug-640318-Chance-of-success%-does-not-validate-Master
Open

[Master]-Chance of success % does not validate correctly when using the action type "Update" in OpportunityChanges#8918
DeepsShukla wants to merge 2 commits into
microsoft:mainfrom
DeepsShukla:bugs/Bug-640318-Chance-of-success%-does-not-validate-Master

Conversation

@DeepsShukla

@DeepsShukla DeepsShukla commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Workitem Bug 640926: [master] [ALL-E] Chance of success % does not validate correctly when using the action type "Update" in Opportunity
AB#640926

@DeepsShukla DeepsShukla requested a review from a team June 30, 2026 11:18
@github-actions github-actions Bot added From Fork Pull request is coming from a fork Linked Issue is linked to a Azure Boards work item labels Jun 30, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Jun 30, 2026
TempSalesCycleStageNext.FindFirst();
"Sales Cycle Stage" := TempSalesCycleStageNext.Stage;
"Sales Cycle Stage Description" := TempSalesCycleStageNext.Description;
"Chances of Success %" := TempSalesCycleStageFirst."Chances of Success %";

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.

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

In the 'Next' branch of WizardActionTypeValidate2, "Chances of Success %" is assigned from TempSalesCycleStageFirst."Chances of Success %" instead of TempSalesCycleStageNext."Chances of Success %".

Every other field in this branch (Stage, Description) correctly uses TempSalesCycleStageNext. The same copy-paste pattern applies to the Previous, Skip, and Jump branches. The new test ChancesOfSuccessRevertsToCurrentStageOnActionTypeUpdate directly demonstrates the bug: it asserts Stage2Chances after a Next action, but the code will return Stage1Chances (from TempSalesCycleStageFirst).

Recommendation:

  • change TempSalesCycleStageFirst to TempSalesCycleStageNext on this line; apply the same correction to the Previous, Skip, and Jump branches (use their respective TempSalesCycleStagePrevious, TempSalesCycleStageSkip, TempSalesCycleStageJump variables). The Update branch intentionally uses TempSalesCycleStageFirst to revert to the current stage and should remain unchanged.
Suggested change
"Chances of Success %" := TempSalesCycleStageFirst."Chances of Success %";
"Chances of Success %" := TempSalesCycleStageNext."Chances of Success %";

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

TempSalesCycleStagePrevious.FindFirst();
"Sales Cycle Stage" := TempSalesCycleStagePrevious.Stage;
"Sales Cycle Stage Description" := TempSalesCycleStagePrevious.Description;
"Chances of Success %" := TempSalesCycleStageFirst."Chances of Success %";

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.

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

In the 'Previous' branch of WizardActionTypeValidate2, "Chances of Success %" is assigned from TempSalesCycleStageFirst."Chances of Success %" instead of TempSalesCycleStagePrevious."Chances of Success %".

The Stage and Description fields in this same branch correctly use TempSalesCycleStagePrevious, making this a copy-paste omission.

Recommendation:

  • replace TempSalesCycleStageFirst with TempSalesCycleStagePrevious on this line so the Chances of Success percentage reflects the target previous stage, consistent with how Stage and Description are set.
Suggested change
"Chances of Success %" := TempSalesCycleStageFirst."Chances of Success %";
"Chances of Success %" := TempSalesCycleStagePrevious."Chances of Success %";

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

TempSalesCycleStageSkip.FindFirst();
"Sales Cycle Stage" := TempSalesCycleStageSkip.Stage;
"Sales Cycle Stage Description" := TempSalesCycleStageSkip.Description;
"Chances of Success %" := TempSalesCycleStageFirst."Chances of Success %";

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.

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

In the 'Skip' branch of WizardActionTypeValidate2, "Chances of Success %" is assigned from TempSalesCycleStageFirst."Chances of Success %" instead of TempSalesCycleStageSkip."Chances of Success %".

Stage and Description in this branch correctly use TempSalesCycleStageSkip.

Recommendation:

  • replace TempSalesCycleStageFirst with TempSalesCycleStageSkip on this line.
Suggested change
"Chances of Success %" := TempSalesCycleStageFirst."Chances of Success %";
"Chances of Success %" := TempSalesCycleStageSkip."Chances of Success %";

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

TempSalesCycleStageJump.FindLast();
"Sales Cycle Stage" := TempSalesCycleStageJump.Stage;
"Sales Cycle Stage Description" := TempSalesCycleStageJump.Description;
"Chances of Success %" := TempSalesCycleStageFirst."Chances of Success %";

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.

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

In the 'Jump' branch of WizardActionTypeValidate2, "Chances of Success %" is assigned from TempSalesCycleStageFirst."Chances of Success %" instead of TempSalesCycleStageJump."Chances of Success %".

Stage and Description in this branch correctly use TempSalesCycleStageJump (via FindLast).

Recommendation:

  • replace TempSalesCycleStageFirst with TempSalesCycleStageJump on this line.
Suggested change
"Chances of Success %" := TempSalesCycleStageFirst."Chances of Success %";
"Chances of Success %" := TempSalesCycleStageJump."Chances of Success %";

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Copilot PR Review

Iteration 2 · Outcome: no-knowledge

The diff (a table field assignment added in 5 branches of an existing case statement plus a new test) touches no performance, security, privacy, upgrade, style, or UI concern with knowledge-file coverage; al-upgrade-review and al-ui-review were not-applicable (no upgrade/install codeunit, table-schema, or page/control-add-in changes) and the remaining leaves ran but found no applicable knowledge files.

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

No findings were posted for this iteration.

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.

@alexei-dobriansky alexei-dobriansky added the SCM GitHub request for SCM area label Jun 30, 2026
@mazhelez

mazhelez commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

@DeepsShukla Please change the name of your branch to exclude the % sign.

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Request Changes

What this PR does

This PR tries to make the Update Opportunity wizard reset Chances of Success % when the user changes Action Type from Next back to Update. The scenario is valid, and the chosen path is the right area: the page calls WizardActionTypeValidate2() when Action Type changes. However, the fix does not fully address the root cause because most branches copy the chance value from TempSalesCycleStageFirst instead of the temp record for the selected stage. For an existing opportunity, CreateStageList() does not populate TempSalesCycleStageFirst, so the Update branch is not guaranteed to restore the current stage chance.

Suggestions

S1 - Wrong stage source for chance value
Use the temp Sales Cycle Stage that matches the selected Action Type when setting Chances of Success %. The Next, Previous, Skip, Update, and Jump branches now read from TempSalesCycleStageFirst, so the Update path does not read the current stage. This means the PR can still save the wrong chance value.

S2 - Random test values can hide regression
Set fixed, different chance values in the new regression test. The helper uses random values for stage 1 and stage 2, so they can be equal and then the test no longer proves that Update changed the value back.

Risk assessment and necessity

Risk: The change is narrow to the Update Opportunity wizard and does not change public APIs or event signatures. The regression surface is CRM opportunity updates: an incorrect chance value changes probability and calculated current value for an opportunity.

Necessity: The change is required because users can save the wrong Chances of Success % after switching from Next to Update. The scope is right, but the assignment must use the selected stage record before this can merge.


[AI-PR-REVIEW] version=1 system=github pr=8918 round=1 by=alexei-dobriansky at=2026-06-30T16:00:35Z lastSha=b9ba2a4b92ca36ff79191cf938971d9222426d91 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 address the suggestions

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.

3 participants