Fix: unify optimistic managerID with API param in submitReport#84628
Fix: unify optimistic managerID with API param in submitReport#84628
Conversation
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
The failing check is unrelated to this PR. Analysis: The Evidence: This PR only modifies |
|
Merge main @MelvinBot |
|
@MelvinBot Give me comprehensive testing steps for different types of approval and submitter setups. Also, give me steps to set these different test case. |
Co-authored-by: parasharrajat <parasharrajat@users.noreply.github.com>
8d9b6ff to
8b932f2
Compare
|
Merged main into this branch. The branch previously had unrelated history (no common ancestor with |
Comprehensive Testing Steps for Approval & Submitter SetupsThis PR unifies the optimistic
Test Case 1: Basic Approval (Submit & Approve)Setup:
Test:
Test Case 2: Advanced Approval — Simple Chain (submitsTo + forwardsTo)Setup:
Test:
Test Case 3: Advanced Approval — Over-Limit ForwardingSetup:
Test (under limit):
Test (over limit):
Test Case 4: Category-Based Rule ApproverSetup:
Test (category rule matches):
Test (category rule does not match):
Test Case 5: Tag-Based Rule ApproverSetup:
Test:
Test Case 6: Category Rule Takes Priority Over Tag RuleSetup:
Test:
Test Case 7: Multiple Transactions with Different Category ApproversSetup:
Test:
Test Case 8: Rule Approver Is the Submitter (Self-Approval Scenario)Setup:
Test:
Test Case 9: Submit & Close (Optional Approval)Setup:
Test:
Test Case 10: No submitsTo Configured (Fallback to Default Approver)Setup:
Test:
Test Case 11: Offline Submission (Optimistic Verification)
Test:
Test Case 12: isAwaitingFirstLevelApproval Consistency
Setup: Use Test Case 4 setup (category rule approver = Approver D, submitsTo = Approver B) Test:
Quick Reference: Approver Resolution Priority
|
|
The failing check is unrelated to this PR. Failing check: Analysis: The build fails with: This is an Android build environment issue where the expo autolinking script is missing from Evidence: The other Android build ( |
|
The failing checks are unrelated to this PR's changes. Analysis: This PR only modifies
Evidence: Both errors are in Expo build infrastructure, not in any file changed by this PR. The same build failures are also occurring on |
Screenshots🔲 iOS / native🔲 iOS / Safari🔲 MacOS / Chrome🔲 Android / Chrome🔲 Android / native |
|
Tested a few flows. Seems fine. need to test more. I will be away today for a few hours. So will check back in 12 hours. |
|
@MelvinBot merge main. |
…ortManagerID Co-authored-by: parasharrajat <parasharrajat@users.noreply.github.com>
|
Done — merged |
|
The failing check ( Analysis: The iOS build fails with a Swift import access level error in Evidence: This PR only modifies |
Explanation of Change
submitReport()inIOU/index.tsused two different functions to compute the approver:managerID(written to local Onyx state) was derived fromgetApprovalChain(policy, expenseReport).at(0), which prepends category/tag rule approvers before thesubmitsTo→forwardsTochain.managerAccountIDwas derived fromgetSubmitToAccountID(policy, expenseReport), which only checks the first rule approver then falls back togetManagerAccountID().When a policy has rule-based approvers configured, these two functions can diverge — the client shows one approver optimistically while routing to another on the backend. This also caused
isAwaitingFirstLevelApproval()(which comparesgetSubmitToAccountID()withreport.managerID) to return incorrect results, affecting the "Add Expenses" button visibility in Classic.This PR unifies both code paths to use
getSubmitToAccountID(), so the optimistic value matches the API parameter. Both the optimistic update and API parameter now reference a singlemanagerIDvariable.Fixed Issues
$ #83073
PROPOSAL: #83073 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A - Logic-only change, no UI modifications
Android: mWeb Chrome
N/A - Logic-only change, no UI modifications
iOS: Native
N/A - Logic-only change, no UI modifications
iOS: mWeb Safari
N/A - Logic-only change, no UI modifications
MacOS: Chrome / Safari
N/A - Logic-only change, no UI modifications