-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Priority Mode: Focus - Hide LHN reports that don't have parent access #52533
base: main
Are you sure you want to change the base?
Priority Mode: Focus - Hide LHN reports that don't have parent access #52533
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
@ikevin127, I see there was no C+ assigned as reviewer for the linked issue. Shall I go ahead and review this? |
@c3024 Indeed, this issue wasn't
Given the above, it's not clear to me and I don't want to assume. Let's wait for confirmation from them on whether C+ review is required here or they're going to handle that. Thanks! cc @puneetlath |
🚀 PR is ready for review now after removing the unused function in commit 0b480b8. Note: Failing test was due to timeout (not related to this PR). |
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / SafarionMainParentChrome.mp4onThisBranchParentChrome.mp4onMainSlackTest.mp4onThisBranchSlackTest.mp4MacOS: DesktopparentDesktop.mp4 |
src/libs/SidebarUtils.ts
Outdated
const shouldOverrideHidden = | ||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
hasValidDraftComment(report.reportID) || hasErrorsOtherThanFailedReceipt || isFocused || isSystemChat || report.isPinned || isExpenseReportWithoutParentAccess; | ||
hasValidDraftComment(report.reportID) || hasErrorsOtherThanFailedReceipt || isFocused || isSystemChat || report.isPinned; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasValidDraftComment(report.reportID) || hasErrorsOtherThanFailedReceipt || isFocused || isSystemChat || report.isPinned; | |
hasValidDraftComment(report.reportID) || hasErrorsOtherThanFailedReceipt || isFocused || isSystemChat || report.isPinned || ReportUtils.requiresAttentionFromCurrentUser(report, parentReportAction); |
For this test, the expenses sent for approval for User B before they were removed as an approver should still appear in User B's LHN because the reports have GBRs and are awaiting User B's approval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or perhaps move the early return check inside the shouldReportBeInOptionList with moving the GBR check inside the function to close to top.
@c3024 Thanks for the thorough testing and catching that GBR edge case, just pushed the fix as suggested. |
Explanation of Change
Some reports have
hasParentAccess
equal tofalse
which causedshouldOverrideHidden
to returntrue
because of theisExpenseReportWithoutParentAccess
check which was added in PR #49557 for the following reason:This was discussed in #52396 (comment) and was decided in #52396 (comment) that the check must be removed to fix our issue.
Fixed Issues
$ #52396
PROPOSAL: #52396 (comment)
Tests
Owner
set your Settings > Preferences > Priority mode to #focus.Employee
- submit an expense report.Owner
- Approve and pay the report.Important
Besides the QA Steps which don't actually reproduce the reports that have
hasParentAccess
equal tofalse
, the reviewer must follow the same method detailed in #52396 (comment) using the DIFF (red, before version) in order to verify the fix locally, just as shown in the video below - usingOnyx.set()
in the browser console to create a faux report withhasParentAccess
equal tofalse
to verify that the report won't show up in LHN onceOnyx.set()
is performed.Offline tests
N/A
QA Steps
Owner
set your Settings > Preferences > Priority mode to #focus.Employee
- submit an expense report.Owner
- Approve and pay the report.PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
PR Author / Reviewer Test (local dev):
bad.mov
good.mov
QA Steps Test:
Screen.Recording.2024-11-14.at.01.36.44.mov
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop