Skip to content

[Master] Bug 650348: [Expense Agent] Closed travel requests are editable and are missing approver info. - #11595

Open
v-rohangarg20 wants to merge 1 commit into
mainfrom
bugs/Bug-650348-travel-request-editable-approver-main
Open

v-rohangarg20 wants to merge 1 commit into
mainfrom
bugs/Bug-650348-travel-request-editable-approver-main

Conversation

@v-rohangarg20

@v-rohangarg20 v-rohangarg20 commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Fixes AB#650348

Problem

Closed travel requests can still be edited, and automatically approved travel requests do not record approver information. The requested-for name is also not shown on the travel request card.

Changes

  • Restrict editable Travel Request fields to open requests.
  • Record the approving user and timestamp when automatic approval is used.
  • Store and display the requested-for expense user name.

@v-rohangarg20
v-rohangarg20 requested review from a team as code owners September 17, 2026 20:57
@github-actions github-actions Bot added AL: Apps (W1) Add-on apps for W1 Team: Finance GitHub request for Finance area labels Sep 17, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 1

Recommendation: Request Changes

What this PR does

The change limits travel-request editing to open requests, stores the requested-for name, and records display user names when BaseApp spend requests are approved or rejected. The BaseApp helper and status guards are targeted, but the Expense Agent approval path does not use its new helper and the W1 build currently fails.

Problem-solution fit

Fit: Partial

The UI and BaseApp changes address parts of the reported behavior. The Expense Agent approval path still does not record the display user name, so the approver update is not preserved consistently.

Suggestions

S1 (🔴 High): Wire the display name into travel approval
GetCurrentUserName is never called, so CodeCop fails with AA0228 and the travel-request approval path still leaves Approved/Rejected by User Name unchanged. Move or use this helper in TravelRequestApproval.ApproveInternal so both manual and automatic approvals record the name.

S2 (🟠 Moderate): Test the recorded approval display name
Extend the existing manual, automatic, and rejection tests to assert Approved/Rejected by User Name. This will cover the new display-name behavior and its fallback instead of only checking status, IDs, and report creation.

Risk assessment and necessity

Risk: Approval attribution is audit data. BaseApp approval and rejection now record a display name with a safe fallback, but Expense Agent approvals remain inconsistent, and the unused method currently prevents the W1 app from compiling.

Necessity: The change is needed to keep open travel requests editable and make approval attribution readable. The scope is reasonable, but the incomplete approval-path port and missing focused assertions must be corrected.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11595 round=1 by=alexei-dobriansky at=2026-09-18T01:13:44Z lastSha=3623c61942977572921cec2ba707e6bd6df24322 reviewKey=b72f8708d89c3a55d4253ac453712afb0adac9ca7dd46125e8ff62483f33597d suggestions=S1@81cdc954,S2@c384e47a

@github-actions github-actions Bot added this to the Version 30.0 milestone Sep 18, 2026
@v-rohangarg20 v-rohangarg20 changed the title Bug 650348: Make travel request approver editable Bug 650348: [Expense Agent] Closed travel requests are editable and are missing approver info. Sep 18, 2026
@v-rohangarg20 v-rohangarg20 changed the title Bug 650348: [Expense Agent] Closed travel requests are editable and are missing approver info. [Master] Bug 650348: [Expense Agent] Closed travel requests are editable and are missing approver info. Sep 18, 2026
@v-rohangarg20 v-rohangarg20 reopened this Sep 18, 2026
Allow editing the approver on Travel Request cards and preserve the approver update behavior across the Expense Agent and Spend Request objects.

(cherry picked from commit abe7b2a)
@v-rohangarg20
v-rohangarg20 force-pushed the bugs/Bug-650348-travel-request-editable-approver-main branch from 3623c61 to 217e966 Compare September 18, 2026 06:51
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 2

Recommendation: Request Changes

What this PR does

This update removes unused code from the subscriber and routes disabled-agent automatic approval through the shared travel-request approval codeunit. That cleans up the build-warning part of the first-round finding, but the shared approval path still does not set the readable approval user name.

Problem-solution fit

Fit: Partial

The change improves the implementation shape, but it still does not fully match the reported approval behavior. Automatic and manual travel-request approval still update the user ID and approver number without storing the approval user name.

Status of previous suggestions
ID Title Status Author response
S1 Wire the display name into travel approval Not addressed
S2 Test the recorded approval display name Not addressed
New observations (commits since round 1)

None - the latest change moves the auto-approval call and removes unused page state, but it does not add a separate new issue.

Risk assessment and necessity

Risk: Approval attribution is audit data. The current travel-request approval path can still leave Approved/Rejected by User Name blank, so the user sees incomplete approver information even though the request is approved.

Necessity: The change is needed to keep closed travel requests locked and approval attribution readable. It should not merge until the readable approver name is written in the travel-request approval path and covered by a focused test.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11595 round=2 by=alexei-dobriansky at=2026-09-18T07:15:05.3387232Z lastSha=217e966c6cac6ee44ed5719cb0894962f63f33e1 reviewKey=c0c0863674c4d82b4226a3af8e0c87a4e4ab617b0c58635e99f101814a9f6619 suggestions=S1@81cdc954:notaddressed,S2@c384e47a:notaddressed parentRound=1

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.

2 participants