feat: deep link to report preview or message that caused GBR/RBR#85192
feat: deep link to report preview or message that caused GBR/RBR#85192mountiny merged 12 commits intoExpensify:mainfrom
Conversation
🤖 Code Review — PR #85192Overall: The approach is sound — tracking 1. Dead code:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3e58ad55c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
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.
|
|
@hoangzinh Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
melvin-bot
bot
requested review from
joekaufmanexpensify
and removed request for
a team
This comment was marked as resolved.
This comment was marked as resolved.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-03-18.at.08.21.53.movAndroid: mWeb ChromeScreen.Recording.2026-03-18.at.06.28.33.moviOS: HybridAppScreen.Recording.2026-03-18.at.08.39.19.moviOS: mWeb SafariScreen.Recording.2026-03-18.at.08.30.00.movMacOS: Chrome / SafariScreen.Recording.2026-03-18.at.06.22.10.mov |
hoangzinh
left a comment
hoangzinh
left a comment
There was a problem hiding this comment.
I have 2 small reviews. Overall, LGTM
|
When a report has unread messages, should we navigate to unread messages or navigate to message that caused GBR/RBR? Screen.Recording.2026-03-17.at.18.14.16.mov |
More like question for @Expensify/product |
|
Happy to help with the review feedback! Here are the specific changes needed: 1. Remove dead code
|
|
Here are suggested unit tests for Guard clause tests (return
|
| # | Scenario | Setup |
|---|---|---|
| 1 | report is null |
Pass null as report |
| 2 | Report is not a policy expense chat | Set chatType to something other than POLICY_EXPENSE_CHAT |
| 3 | Current user is not the submitter | Set ownerAccountID to a different account than current user |
| 4 | Report has no policyID |
Omit policyID from the report |
| 5 | All reports under policy are invoice reports | Set type: CONST.REPORT.TYPE.INVOICE on the child reports |
| 6 | All reports are closed (approved/reimbursed) | Set stateNum/statusNum to approved state |
Positive path tests (return a report ID)
| # | Scenario | Expected |
|---|---|---|
| 7 | Open report with VIOLATION type violation (showInReview: true) |
Returns that report's ID |
| 8 | Open report with WARNING type violation (showInReview: true) |
Returns that report's ID |
| 9 | Open report with NOTICE type violation (showInReview: true) |
Returns that report's ID |
| 10 | Processing (submitted) report with violations | Returns that report's ID |
Ordering / selection tests
| # | Scenario | Expected |
|---|---|---|
| 11 | Multiple reports with violations, different created dates |
Returns the oldest report's ID (sorted ascending by created) |
| 12 | Mix of reports — some with violations, some without | Returns the first (oldest) report that has violations, skipping clean ones |
Filtering edge cases
| # | Scenario | Expected |
|---|---|---|
| 13 | Violations exist but all have showInReview: false |
Returns null — function passes showInReview: true to hasViolations/hasWarningTypeViolations/hasNoticeTypeViolations |
| 14 | Violations exist but are all dismissed by current user | Returns null — isViolationDismissed filters them out |
| 15 | hasVisibleViolationsForUser returns false but hasViolations would return true |
Returns null — both checks must pass |
Skeleton
Here's a rough structure following the existing test patterns in ReportUtilsTest.ts:
describe('getViolatingReportIDForRBRInLHN', () => {
// Shared setup: create a policy expense chat where current user is submitter
const policyID = 'testPolicy';
const chatReport = {
reportID: 'chatReport1',
chatType: CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT,
ownerAccountID: currentUserAccountID,
policyID,
};
it('returns null when report is null', () => { ... });
it('returns null when report is not a policy expense chat', () => { ... });
it('returns null when current user is not the submitter', () => { ... });
it('returns null when report has no policyID', () => { ... });
it('returns the reportID of an open report with VIOLATION type violations', () => {
// Create an open expense report under the same policy
// Add a transaction with a VIOLATION violation (showInReview: true)
// Set up reportsByPolicyID to include it
// Assert: returns expenseReport.reportID
});
it('returns the reportID for WARNING type violations', () => { ... });
it('returns the reportID for NOTICE type violations', () => { ... });
it('returns the oldest violating report when multiple exist', () => {
// Create two expense reports with violations, different `created` dates
// Assert: returns the one with the earlier `created` date
});
it('returns null when violations exist but showInReview is false', () => { ... });
it('returns null when all violations are dismissed', () => { ... });
it('skips invoice reports', () => { ... });
it('skips closed/approved reports', () => { ... });
});The most valuable additions are tests #7 and #11 — they're the only way to verify the function actually returns a report ID (the positive path) and that the sort order is correct. The existing tests only assert null returns.
|
@MelvinBot can you add unit tests based on your analysis? |
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Excited for this!
| }; | ||
| } | ||
|
|
||
| const {reportAction} = getAllReportActionsErrorsAndReportActionThatRequiresAttention(report, reportActions, isReportArchived); |
There was a problem hiding this comment.
for Follow up: Since we do not care about the errors here, can you use different method that only gets the report action?
| const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${potentialReport.policyID}`]; | ||
| const transactions = getReportTransactions(potentialReport.reportID); | ||
|
|
||
| // Allow both open and processing reports to show RBR for violations | ||
| if (!isOpenOrProcessingReport(potentialReport)) { | ||
| return false; | ||
| } | ||
| // Allow both open and processing reports to show RBR for violations | ||
| if (!isOpenOrProcessingReport(potentialReport)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
we dont neeed to access it so early
| const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${potentialReport.policyID}`]; | |
| const transactions = getReportTransactions(potentialReport.reportID); | |
| // Allow both open and processing reports to show RBR for violations | |
| if (!isOpenOrProcessingReport(potentialReport)) { | |
| return false; | |
| } | |
| // Allow both open and processing reports to show RBR for violations | |
| if (!isOpenOrProcessingReport(potentialReport)) { | |
| return false; | |
| } | |
| // Allow both open and processing reports to show RBR for violations | |
| if (!isOpenOrProcessingReport(potentialReport)) { | |
| return false; | |
| } | |
| const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${potentialReport.policyID}`]; | |
| const transactions = getReportTransactions(potentialReport.reportID); |
|
🚧 @mountiny 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. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.40-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.40-0 🚀
|
|
Deploy Blocker #85659 was identified to be related to this PR. |
|
Deploy Blocker #85665 was identified to be related to this PR. |
|
Deploy Blocker #85677 was identified to be related to this PR. |
|
Deploy Blocker ##85706 was identified to be related to this PR. |
|
Deploy Blocker #85744 was identified to be related to this PR. |
|
Deploy Blocker #85753 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.41-4 🚀
|
8 participants
Explanation of Change
Note: I intentionally disabled this feature in production yet to avoid PR being reverted. This gate will be removed once this passes QA
Fixed Issues
#85436
#84764
Tests
Submit,Approve,Pay) showsFixshowsOffline tests
Same as Tests
QA Steps
Same as Tests
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
web.mov