Skip to content

[Master] - Bug 650785: [Expense Agent] Deleting an itemized Expense Category shows misleading missing-subcategories confirmation - #11705

Merged
v-rohangarg20 merged 2 commits into
mainfrom
bugs/Bug-650785-Deleting-itemized-Exp-Cat-shows-misleading-missing-subcategories
Sep 23, 2026
Merged

v-rohangarg20 merged 2 commits into
mainfrom
bugs/Bug-650785-Deleting-itemized-Exp-Cat-shows-misleading-missing-subcategories

Conversation

@v-rohangarg20

@v-rohangarg20 v-rohangarg20 commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

@v-rohangarg20
v-rohangarg20 requested a review from a team as a code owner September 22, 2026 06:17
@github-actions github-actions Bot added AL: Apps (W1) Add-on apps for W1 Team: Finance GitHub request for Finance area labels Sep 22, 2026
@github-actions github-actions Bot added this to the Version 30.0 milestone Sep 22, 2026
@v-rohangarg20 v-rohangarg20 reopened this Sep 22, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 1

Recommendation: Accept with Suggestions

What this PR does

The change suppresses setup confirmations while a record is being deleted. It stops the missing-subcategories prompt from appearing after related subcategories were already cascade-deleted, and it applies the same guard to the blank employee prompt on Expense User.

The main category scenario is addressed because OnQueryClosePage no longer calls the subcategory check after OnDeleteRecord marks the page as deleting. One edge case remains: the new flags are set before the delete is known to have completed, so a cancelled or failed delete can suppress later close validation on the same page instance.

Problem-solution fit

Fit: Strong

The diff targets the page close path that shows the misleading message after deletion, and it keeps normal close validation for non-delete closes. The extra Expense User change follows the same pattern, but it needs the cancellation guard noted below.

Suggestions

S1 (🟠 Moderate): Delete flag stays set after cancellation
Set the delete flag only after ConfirmApproverReassignment() returns true, or reset it when the delete is cancelled. A No answer leaves IsDeletingExpenseUser true, so the later close check skips the blank-employee confirmation even though the record was not deleted. Use the same reset pattern on the category page so a failed delete does not suppress the missing-subcategories warning.

Risk assessment and necessity

Risk: Low. The change is limited to page close confirmations in Expense Agent master data. The main regression surface is suppressing a valid close warning if a delete is cancelled or blocked, not posting or ledger data.

Necessity: The change is needed because the previous confirmation is shown in a delete flow where the category will not be used again. The scope is small and fits the reported UX bug.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11705 round=1 by=alexei-dobriansky at=2026-09-22T07:12:31Z lastSha=f2aee081ad6881fec9ebbad112e8d40c6117d7f2 reviewKey=e4c72b347c0f12355efc6d0aa4c6bf4cf644bd3960a27eec884a4f166907966f suggestions=S1@d554de33

@t-prda Prangshuman Das (t-prda) 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 review the suggestion in the comment: #11705 (comment)

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 2

Recommendation: Accept with Suggestions

What this PR does

The new commit changes the expense-user delete guard so the page only treats a delete as "in progress" when the confirmation was actually accepted, instead of marking it unconditionally before the confirmation result is known. This matches the earlier suggestion for that page.

Status of previous suggestions
ID Title Status Author response
S1 Delete flag stays set after cancellation Not addressed The expense-user page now sets its guard from the confirmation result, which fixes that half. The expense-category page still sets its guard unconditionally in its delete trigger, before the table's own delete validation runs - so a blocked deletion there can still leave the close-time warning suppressed.
New observations (commits since round 1)

None - the new commit only changes the expense-user guard already covered by S1.

Risk assessment and necessity

Risk: Low. Both the fixed and the still-open path only affect a page close confirmation, not the underlying delete or any posted data - a blocked deletion still correctly leaves the record in place either way.

Necessity: Unchanged from round 1 - the core scenario is fixed; the remaining gap is a smaller, same-shape risk on the sibling page.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11705 round=2 by=alexei-dobriansky at=2026-09-22T19:41:20Z lastSha=9e83e11c479f2013d5544f0679fdbc726921966a reviewKey=8bda876c8470828b949580edbe0a31beb34b86a2a1dfafdeefca77081df736af suggestions=S1@d554de33:notaddressed parentRound=1

@v-rohangarg20
v-rohangarg20 added this pull request to the merge queue Sep 23, 2026
@v-rohangarg20
v-rohangarg20 removed this pull request from the merge queue due to a manual request Sep 23, 2026
@v-rohangarg20
v-rohangarg20 added this pull request to the merge queue Sep 23, 2026
Merged via the queue into main with commit 5811fef Sep 23, 2026
172 checks passed
@v-rohangarg20
v-rohangarg20 deleted the bugs/Bug-650785-Deleting-itemized-Exp-Cat-shows-misleading-missing-subcategories branch September 23, 2026 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AL: Apps (W1) Add-on apps for W1 Team: Finance GitHub request for Finance area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants