Add diagnostic logging to approval workflow resolution#81725
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
Fixed the Prettier check failure by sorting the The other two failing checks are unrelated to the PR changes:
|
Adds Log.info calls to track the approval chain resolution path when submitting expense reports, to validate root cause analysis for issue #591864 where reports skip the first approver. Logging added to: - getManagerAccountID: logs which branch is taken and all inputs - getSubmitToAccountID: logs rule approver path vs fallthrough - submitReport (IOU): logs comparison of optimistic vs API values - submitMoneyRequestOnSearch: same diagnostic for search submit
Co-authored-by: Tim Golen <tgolen@users.noreply.github.com>
dbcb5b7 to
9288605
Compare
|
Fixed the failing |
|
The |
|
The remaining failing check ("Check for unused styles") is unrelated to this PR. It failed during the The check never reached the actual "unused styles" analysis — it couldn't even clone the repo due to a TLS connection drop. This should pass on a re-run. I was unable to trigger the re-run automatically (requires approval), so please re-run the failed job manually. |
|
Thanks @dangrous. I hope it will be enough to figure out what's going on! Once the tests are passing, I'll merge this. |
|
🚧 @tgolen has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Explanation of Change
Adds diagnostic logging to the approval workflow resolution path to validate the root cause analysis from https://github.com/Expensify/Expensify/issues/591864, where expense reports were skipping the first approver in the chain (Kayce) and going directly to the final approver (Jasmin).
The hypothesis is that
getSubmitToAccountID()/getManagerAccountID()in PolicyUtils sometimes falls back topolicy.owner(the final approver) instead of readingemployee.submitsTo(the first approver), likely due to stale or incomplete Onyx data forpolicy.employeeListat the moment of submission.These logs capture the complete decision state at each branch point so we can confirm or disprove this hypothesis from production data before making any code changes.
What's logged
getManagerAccountID: Which code path is taken (non-advanced workflow / no employee found / advanced workflow resolution), plus all inputs:employeeLogin,employeeSubmitsTo,defaultApprover,policyApprover,policyOwner, and the resolvedmanagerAccountID.getSubmitToAccountID: Whether a rule approver was used vs falling through togetManagerAccountID.submitReport(IOU): Side-by-side comparison of the optimisticmanagerID(fromgetApprovalChain) vs themanagerAccountIDsent to the API (fromgetSubmitToAccountID), which can diverge.submitMoneyRequestOnSearch: Same diagnostic for the search-based submit path.Fixed Issues
Part of https://github.com/Expensify/Expensify/issues/591864
Tests
None
Offline tests
None
QA Steps
None - Once this is on production, we need to have a report submitted for the customer and then the logs can be analyzed. I'll do that myself (@tgolen)
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari