Skip to content

[29.0] - Bug 650480:[Expense Agent] Closed travel requests are editable and are missing approver info - #11594

Merged
v-rohangarg20 merged 10 commits into
releases/29.0from
bugs/Bug-650348-travel-request-editable-approver-290
Sep 23, 2026
Merged

v-rohangarg20 merged 10 commits into
releases/29.0from
bugs/Bug-650348-travel-request-editable-approver-290

Conversation

@v-rohangarg20

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

Copy link
Copy Markdown
Contributor

Fixes AB#650480

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

This change makes travel-request fields editable only while the request is open, stores the selected expense user's name, and records readable approval audit details. The status checks and audit-field updates follow the existing Spend Request flow, but the new unused page variable causes every Apps build to fail. The stored requested-for name can also be edited independently or remain blank on existing records.

Problem-solution fit

Fit: Partial

The change addresses the requested editing and audit-name behavior, but the implementation does not currently build and does not keep the new derived name reliable in all record states.

Suggestions

S1 (🔴 High): Remove the unused open-state variable
The variable is assigned but never used, so AA0206 fails every Apps build. Remove the trigger and variable, or use RequestIsOpen in the page editability expressions.

S2 (🟠 Moderate): Populate names for existing requests
Existing records start with a blank Requested For Name even when Requested For is set. Add upgrade or lazy-population logic so existing requests keep the derived name.

S3 (🟠 Moderate): Keep the derived name read-only
Requested For Name is derived from Requested For, but this page lets users change it independently. Make the name read-only so the stored values cannot become inconsistent.

S4 (🟠 Moderate): Add regression coverage for changed behavior
Existing Spend Request tests can cover these changes. Add cases for open versus closed editing, derived-name updates, and approval or rejection audit names.

Risk assessment and necessity

Risk: The page change currently fails the enforced warning policy in every Apps build. After that is fixed, the stored requested-for name can become inconsistent through direct editing or remain blank on existing records. The approval audit changes are narrow and use the established User lookup pattern; no event-publisher dependency is involved.

Necessity: The editing restrictions and approval audit updates are useful and appropriately scoped. The build failure must be fixed before merge, while name synchronization, upgrade behavior, and regression coverage should be completed to avoid inconsistent travel-request data.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11594 round=1 by=alexei-dobriansky at=2026-09-18T01:15:59.9615026Z lastSha=ab35d6a4e0638ae23bb87da785f13347afff7225 reviewKey=4a0663c075805af6c012a475fcd83ff10c3a2c2013b5ce8e7d5d117552b59125 suggestions=S1@7e5cdde2,S2@4e5fd159,S3@aff8c166,S4@dd3ef861

@github-actions github-actions Bot added this to the Version 29.0 milestone Sep 18, 2026
@v-rohangarg20 v-rohangarg20 changed the title Bug 650348: Make travel request approver editable Bug 650480: [29.0] - 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 650480: [29.0] - Bug 650348:[Expense Agent] Closed travel requests are editable and are missing approver info. [29.0] - Bug 650480:[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-290 branch from ab35d6a to ec51e0b Compare September 18, 2026 07:07
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 2

Recommendation: Accept with Suggestions

What this PR does

This update removes the unused page state that caused the first-round build blocker while keeping the travel-request editability, requested-for name, and disabled-agent auto-approval changes. The build failure noted earlier is addressed, but existing records can still keep a blank derived name, the derived name is still editable, and there is no focused regression coverage.

Problem-solution fit

Fit: Partial

The change addresses the build blocker and keeps the main behavior changes. The fit remains partial because existing requests and direct edits can still leave the stored requested-for name inconsistent with the selected expense user.

Status of previous suggestions
ID Title Status Author response
S1 Remove the unused open-state variable Addressed Removed the unused trigger and variable.
S2 Populate names for existing requests Not addressed
S3 Keep the derived name read-only Not addressed
S4 Add regression coverage for changed behavior Not addressed
New observations (commits since round 1)

None - the latest change only removes the unused page state from the first review.

Risk assessment and necessity

Risk: The enforced-warning build risk from the unused page state is gone. The remaining risk is inconsistent requested-for name data on existing or manually edited requests, plus weak coverage for approval and page-editability behavior.

Necessity: The editing restrictions and approval audit fields are useful and scoped to the reported travel-request behavior. The remaining name-synchronization and test gaps are non-blocking but should be fixed before the behavior is relied on.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11594 round=2 by=alexei-dobriansky at=2026-09-18T07:15:00.0274116Z lastSha=ec51e0bbeef00f5e16ead29209ea5307d996eecb reviewKey=b9597b42dee85e796c269d32c312215c7818512f9f0072a0bf427217591213c4 suggestions=S1@7e5cdde2:addressed,S2@4e5fd159:notaddressed,S3@aff8c166:notaddressed,S4@dd3ef861:notaddressed parentRound=1

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 3

Recommendation: Accept with Suggestions

What this PR does

This round restricts travel request edits to open requests, records automatic approval audit details, stores and shows the requested-for expense user name, and adds tests for approval audit data, requested-for name updates, and editability rules.

Status of previous suggestions
ID Title Status Author response
S1 Build and approval attribution Addressed The build blocker is gone and automatic approval metadata is tested.
S2 Populate names for existing requests Not addressed Existing records can still have a blank requested-for name.
S3 Keep derived name read-only Not addressed The card still makes the derived name editable while open.
New observations (commits since round 2)

S4 (🟠 Moderate): Populate names for existing requests
Existing requests can still have Requested For set while Requested For Name stays blank. Add upgrade or lazy-population logic so old travel requests show the same derived name as new ones.

S5 (🟠 Moderate): Keep the derived name read-only
Requested For Name is derived from Requested For, but the card still makes it editable while the request is open. Make this field read-only so users cannot store a name that does not match the selected expense user.

Risk assessment and necessity

Risk: The build and approval-attribution blockers are fixed. Remaining risk is stale or manually inconsistent requested-for display data.

Necessity: The change is needed to prevent editing closed travel requests and to record approval audit information. The derived name should stay trustworthy.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11594 round=3 by=alexei-dobriansky at=2026-09-21T20:34:04.9992134Z lastSha=35e5a2d2a80f4b9dc443a534a92eecd5760979d9 reviewKey=8d8d3a6301609e1f25f895572278606c2ba870f5823785cddebe7e30645c6b6d suggestions=S1@old:addressed,S2@old:notaddressed,S3@old:notaddressed,S4@reqbackfill:new,S5@reqreadonly:new parentRound=2

@v-rohangarg20
v-rohangarg20 requested a review from a team September 22, 2026 04:33
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 4

Recommendation: Accept with Suggestions

What this PR does

This round keeps the travel-request edit locks, automatic approval audit fields, requested-for name storage, and regression coverage. The latest commits add validation on the requested-for name, lock a few remaining card fields to open requests, and keep approval-name storage aligned with the existing user-id pattern. The remaining issue is still the consistency of the stored requested-for name.

Status of previous suggestions
ID Title Status Author response
S1 Build and approval attribution Addressed The build blocker remains gone, automatic approval audit fields are present, and coverage was added.
S2 Populate names for existing requests Not addressed Existing records can still have Requested For set while Requested For Name stays blank.
S3 Keep derived name read-only Not addressed The card still lets users edit the derived name while the request is open.
S4 Populate names for existing requests Not addressed No upgrade or lazy-population path was added for old travel requests.
S5 Keep the derived name read-only Not addressed The table now blocks edits after the status changes, but open requests can still store a manual name.
New observations (commits since round 3)

None - the latest commits do not introduce a new blocker. The previous requested-for name consistency suggestions remain open.

Risk assessment and necessity

Risk: The build and approval-audit risks are covered by the current code and checks. The remaining risk is stale or manually inconsistent requested-for display data on travel requests. No BaseApp event-publisher dependency is involved.

Necessity: The change is needed to prevent edits after a travel request leaves the open state and to record automatic approval audit data. Showing the requested-for name is useful, but the stored name should stay trustworthy for both old and new requests.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11594 round=4 by=alexei-dobriansky at=2026-09-22T07:13:15.3217799Z lastSha=dc51e920017dcf248fbf8a817cc1c845f516c20f reviewKey=47046727594e1f074f12db5304b3063b253dc136573f739df3f40afc92eac9b7 suggestions=S1@old:addressed,S2@old:notaddressed,S3@old:notaddressed,S4@reqbackfill:notaddressed,S5@reqreadonly:notaddressed parentRound=3

@v-rohangarg20
v-rohangarg20 enabled auto-merge (squash) September 23, 2026 13:24
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 5

Recommendation: Accept

What this PR does

This round keeps the travel-request edit locks, automatic approval audit fields, requested-for name display, and regression coverage. The latest commit changes the requested-for name into a calculated value, so existing and new travel requests show the current expense-user name without storing a separate editable copy. The previous name consistency concerns are addressed, and no new issue was found in the latest changed lines.

Status of previous suggestions
ID Title Status Author response
S1 Build and approval attribution Addressed The build blocker remains gone, automatic approval audit fields are present, and coverage was added.
S2 Populate names for existing requests Addressed The name is now calculated from the requester, so existing records no longer need a backfill.
S3 Keep derived name read-only Addressed The name is now derived instead of stored separately, so it cannot become a manual inconsistent value.
S4 Populate names for existing requests Addressed The calculated field resolves old travel requests from the current requester.
S5 Keep the derived name read-only Addressed The stored-name path was removed, and the field is now a calculated display value.
New observations (commits since round 4)

None - the latest commit only addresses the previous requested-for name consistency suggestions.

Risk assessment and necessity

Risk: The change is scoped to travel request editing, automatic approval audit data, and requested-for name display. The remaining regression surface is low because the latest change removes duplicated stored name data instead of adding more write paths.

Necessity: The change is needed to keep non-open travel requests from being edited, to record automatic approval details, and to show the requested-for name consistently for both existing and new requests.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11594 round=5 by=alexei-dobriansky at=2026-09-23T13:05:23.629Z lastSha=bd40bf14a26d1232e830e5f2d0455f9a3cfbfabd reviewKey=ee8a5c0400968c1c22f06b8e17e13489678f7b79630cd989437dfe38565d63a3 suggestions=S1@old:addressed,S2@old:addressed,S3@old:addressed,S4@reqbackfill:addressed,S5@reqreadonly:addressed parentRound=4

@v-rohangarg20
v-rohangarg20 merged commit 9ef1180 into releases/29.0 Sep 23, 2026
330 of 332 checks passed
@v-rohangarg20
v-rohangarg20 deleted the bugs/Bug-650348-travel-request-editable-approver-290 branch September 23, 2026 14:31
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.

6 participants