-
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
fix: Inbox showing GBR when there’s a report with RBR #51643
Conversation
@mkhutornyi 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] |
@mkhutornyi I encountered an error when running IOS native: I will test and update the IOS native video when it is fixed. |
@@ -62,6 +62,14 @@ const getBrickRoadForPolicy = (report: Report, altReportActions?: OnyxCollection | |||
const reportErrors = ReportUtils.getAllReportErrors(report, reportActions); | |||
const oneTransactionThreadReportID = ReportActionsUtils.getOneTransactionThreadReportID(report.reportID, reportActions); | |||
let doesReportContainErrors = Object.keys(reportErrors ?? {}).length !== 0 ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined; |
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.
add line break
src/libs/WorkspacesSettingsUtils.ts
Outdated
const parentReportActions = (altReportActions ?? allReportActions)?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report?.parentReportID}`]; | ||
const parentReportAction = parentReportActions?.[report?.parentReportActionID ?? '-1']; | ||
const shouldDisplayViolations = ReportUtils.shouldDisplayTransactionThreadViolations(report, allTransactionViolations, parentReportAction); | ||
const shouldDisplayReportViolations = ReportUtils.isReportOwner(report) && ReportUtils.hasReportViolations(report.reportID); | ||
const hasViolations = shouldDisplayViolations || shouldDisplayReportViolations; | ||
if (hasViolations) { | ||
doesReportContainErrors = CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR; | ||
} |
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.
put this whole code inside if (!doesReportContainErrors) { }
(just for optimization)
src/libs/WorkspacesSettingsUtils.ts
Outdated
const hasViolations = shouldDisplayViolations || shouldDisplayReportViolations; | ||
if (hasViolations) { | ||
doesReportContainErrors = CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR; | ||
} | ||
|
||
if (oneTransactionThreadReportID) { |
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.
Same here
if (oneTransactionThreadReportID) { | |
if (oneTransactionThreadReportID && !doesReportContainErrors) |
src/libs/WorkspacesSettingsUtils.ts
Outdated
const parentReportActions = (altReportActions ?? allReportActions)?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report?.parentReportID}`]; | ||
const parentReportAction = parentReportActions?.[report?.parentReportActionID ?? '-1']; | ||
const shouldDisplayViolations = ReportUtils.shouldDisplayTransactionThreadViolations(report, allTransactionViolations, parentReportAction); | ||
const shouldDisplayReportViolations = ReportUtils.isReportOwner(report) && ReportUtils.hasReportViolations(report.reportID); | ||
const hasViolations = shouldDisplayViolations || shouldDisplayReportViolations; | ||
if (hasViolations && !doesReportContainErrors) { | ||
doesReportContainErrors = CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR; | ||
} |
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.
const parentReportActions = (altReportActions ?? allReportActions)?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report?.parentReportID}`]; | |
const parentReportAction = parentReportActions?.[report?.parentReportActionID ?? '-1']; | |
const shouldDisplayViolations = ReportUtils.shouldDisplayTransactionThreadViolations(report, allTransactionViolations, parentReportAction); | |
const shouldDisplayReportViolations = ReportUtils.isReportOwner(report) && ReportUtils.hasReportViolations(report.reportID); | |
const hasViolations = shouldDisplayViolations || shouldDisplayReportViolations; | |
if (hasViolations && !doesReportContainErrors) { | |
doesReportContainErrors = CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR; | |
} | |
if (!doesReportContainErrors) { | |
const parentReportActions = (altReportActions ?? allReportActions)?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report?.parentReportID}`]; | |
const parentReportAction = parentReportActions?.[report?.parentReportActionID ?? '-1']; | |
const shouldDisplayViolations = ReportUtils.shouldDisplayTransactionThreadViolations(report, allTransactionViolations, parentReportAction); | |
const shouldDisplayReportViolations = ReportUtils.isReportOwner(report) && ReportUtils.hasReportViolations(report.reportID); | |
const hasViolations = shouldDisplayViolations || shouldDisplayReportViolations; | |
if (hasViolations) { | |
doesReportContainErrors = CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR; | |
} | |
} |
Reviewer Checklist
Screenshots/VideosAndroid: mWeb ChromeiOS: mWeb SafariMacOS: Desktop |
@truph01 bump #51643 (comment) |
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.
@tgolen all yours
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.
Can you please add a unit test that confirms the bug is fixed and won't come back?
@tgolen I added unit test |
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.
Thanks for adding the test! Can you please follow the conventions here for the given/when/then comment format? https://github.com/Expensify/App/blob/main/tests/README.md#documenting-tests
Also, I'm not really seeing other tests do this, but it might be nice to move the hard-coded JSON to it's own file called tests/unit/WorkspaceSettingsUtilsTest.json
Also, if you're not used to writing those comments, here is a tip. The best comment is one that explains "why" the logic is doing what it's doing. Many times, comments merely state "what" the logic is doing, which is almost always self-evident. |
// When calling getBrickRoadForPolicy with a report and report actions | ||
const result = getBrickRoadForPolicy(report as Report, reportActions as OnyxCollection<ReportActions>); | ||
|
||
// Then the result should be 'error' |
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.
Why is the result error
in this case? What is it about the report and report actions that produce this result?
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.
I added 2nd test case, which will return undefined
. The getBrickRoadForPolicy
return error
or undefined
based on the transactionsViolation data.
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.
Thanks for the second test case. I still don't think I know the answer to my question though. Can you please update this code comment to explain why it's expected to be "error" and not anything else? Again, what is it about the report and report actions that produce this result?
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.
@truph01 bump on this one. I'm still looking for some more contextual code comments for this.
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.
Apologies, I missed that earlier. I've updated the comment. Let's aim to get this PR merged today.
// When calling getBrickRoadForPolicy with a report and report actions | ||
const result = getBrickRoadForPolicy(report as Report, reportActions as OnyxCollection<ReportActions>); | ||
|
||
// Then the result should be 'error' |
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.
Thanks for the second test case. I still don't think I know the answer to my question though. Can you please update this code comment to explain why it's expected to be "error" and not anything else? Again, what is it about the report and report actions that produce this result?
@truph01 are you working on these changes today? |
@muttmuure I resolved all the comments. In case you miss it: #51643 (comment) |
@tgolen The PR is ready for the final review |
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.
Ah, that's perfect. Thank you!
✋ 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/tgolen in version: 9.0.61-0 🚀
|
@mvtglobally thanks for checking! It looks like running the "test steps" will work in this case, so please do those. |
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.61-3 🚀
|
Details
Fixed Issues
$ #50927
PROPOSAL: #50927 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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
Android: Native
Screen.Recording.2024-10-29.at.17.59.03.mov
Android: mWeb Chrome
Screen.Recording.2024-10-29.at.18.08.24.mov
iOS: Native
iOS: mWeb Safari
Screen.Recording.2024-10-29.at.18.01.03.mov
MacOS: Chrome / Safari
Screen.Recording.2024-10-29.at.17.54.29.mov
MacOS: Desktop
Screen.Recording.2024-10-29.at.17.57.01.mov