Skip to content

[29.x] - Bug 650938: [Expense Agent] Deleting an itemized Expense Category shows misleading missing-subcategories confirmation - #11706

Merged
v-rohangarg20 merged 2 commits into
releases/29.xfrom
bugs/Bug-650938-Deleting-itemized-Exp-Cat-shows-misleading-missing-subcategories
Sep 23, 2026
Merged

v-rohangarg20 merged 2 commits into
releases/29.xfrom
bugs/Bug-650938-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 September 22, 2026 06:27
@v-rohangarg20
v-rohangarg20 requested a review from a team as a code owner September 22, 2026 06:27
@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 29.1 milestone Sep 22, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 1

Recommendation: Accept with Suggestions

What this PR does

This change suppresses close-page setup warnings while deleting Expense Category and Expense User cards. For the category path, setting the delete flag before the page closes matches the reported problem: the missing-subcategory check no longer runs after a successful delete, so the misleading setup confirmation is avoided. The chosen page trigger is narrow and does not change table deletion or cascade behavior.

Problem-solution fit

Fit: Strong

The reported scenario is clear, and the category change targets the exact close-after-delete path that showed the wrong confirmation. The extra Expense User change follows the same pattern, but its canceled-delete path should keep the existing close warning behavior intact.

Suggestions

S1 (🟠 Moderate): Set deletion flag after confirmation
Set IsDeletingExpenseUser only after ConfirmApproverReassignment() returns true. If the user cancels that confirmation, the delete is canceled but the page flag remains true. A later close can then skip the blank-employee warning for a record that was not deleted.

Risk assessment and necessity

Risk: Low. The change is limited to page close confirmation behavior on two setup card pages, with no posting, table schema, or cascade-delete changes. The main regression surface is canceled or failed delete attempts because page-level state now tracks that a delete was attempted.

Necessity: The category change is needed to avoid a misleading warning after a delete that already removed the relevant subcategories. The scope is small, but the Expense User change should preserve the existing warning when deletion is canceled.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11706 round=1 by=alexei-dobriansky at=2026-09-22T07:11:34Z lastSha=4bbfa0387f3af4952800c6b799bb878fc5b56d74 reviewKey=6dcd9d9bcec894223968e95f60fe80e05cd5068df26e336562f8d8d5171d8d95 suggestions=S1@d554de33

@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 Set deletion flag after confirmation 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=11706 round=2 by=alexei-dobriansky at=2026-09-22T19:41:20Z lastSha=5b184b25c0ad3d4e0b8ec0d55e12804974de166e reviewKey=0903d2c497d57945ef8676ae1619fd1512827d7102cce35520660598142b18c2 suggestions=S1@d554de33:notaddressed parentRound=1

@v-rohangarg20
v-rohangarg20 merged commit 0639eb3 into releases/29.x Sep 23, 2026
327 of 332 checks passed
@v-rohangarg20
v-rohangarg20 deleted the bugs/Bug-650938-Deleting-itemized-Exp-Cat-shows-misleading-missing-subcategories branch September 23, 2026 13:25
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.

5 participants