-
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
Add LHN debugging to Debug Mode #48442
Add LHN debugging to Debug Mode #48442
Conversation
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.
Partial review for now
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 realize some instances won't be possible to change but is it possible to get rid of some of the as
casts we are using in this PR?
cd7391c
to
31a8c94
Compare
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.
one comment, otherwise looks good, will wait on Fabio to approve before giving final 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.
LGTM, what's missing @pac-guerreiro:
- Fix prettier / lint errors
- Add screenshots / videos
@blimpich @fabioh8010 @mountiny PR is ready to be merged! |
@pac-guerreiro The logic to display RBR still be different in LHN Screen.Recording.2024-10-03.at.16.42.28.movIn LHN, we display RBR in these cases. Let's mirror it for this feature Line 323 in 69066f8
Line 335 in 69066f8
|
{ | ||
title: translate('debug.RBR'), | ||
subtitle: translate(`debug.${hasRBR}`), | ||
action: reportActionRBR |
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.
action: reportActionRBR | |
action: hasRBR && reportActionRBR |
@pac-guerreiro This bug still can reproduce. In DebugReportPage, please help to add the third param
|
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-10-04.at.15.19.51.movAndroid: mWeb ChromeScreen.Recording.2024-10-04.at.15.17.36.moviOS: NativeScreen.Recording.2024-10-04.at.18.09.48.moviOS: mWeb SafariScreen.Recording.2024-10-04.at.18.08.02.movMacOS: Chrome / SafariScreen.Recording.2024-10-04.at.15.14.23.movMacOS: DesktopScreen.Recording.2024-10-04.at.15.15.50.mov |
Thank you for the feedback! I just pushed the changes you requested 😄 |
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.
Small comment
src/libs/SidebarUtils.ts
Outdated
@@ -234,6 +234,27 @@ function getOrderedReportIDs( | |||
return LHNReports; | |||
} | |||
|
|||
function isRedBrickRoad(report: Report, reportActions: OnyxEntry<ReportActions>, hasViolations: boolean, transactionViolations?: OnyxCollection<TransactionViolation[]>) { |
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.
function isRedBrickRoad(report: Report, reportActions: OnyxEntry<ReportActions>, hasViolations: boolean, transactionViolations?: OnyxCollection<TransactionViolation[]>) { | |
function shouldShowRedBrickRoad(report: Report, reportActions: OnyxEntry<ReportActions>, hasViolations: boolean, transactionViolations?: OnyxCollection<TransactionViolation[]>) { |
src/libs/SidebarUtils.ts
Outdated
result.brickRoadIndicator = CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR; | ||
} | ||
} | ||
result.brickRoadIndicator = isRedBrickRoad(report, reportActions, hasViolations, transactionViolations) ? 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.
result.brickRoadIndicator = isRedBrickRoad(report, reportActions, hasViolations, transactionViolations) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''; | |
result.brickRoadIndicator = shouldShowRedBrickRoad(report, reportActions, hasViolations, transactionViolations) ? 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.
The code looks great to me. I left some NAB comments that could be addressed in a follow up. I'm excited for this feature, I think things will start to make a lot more sense when bugs happen. Thanks.
|
||
for (const action of reportActionsArray) { | ||
if (action && !isEmptyObject(action.errors)) { | ||
Object.assign(reportActionErrors, action.errors); | ||
|
||
if (!reportAction) { |
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.
NAB: Instead of this check in every iteration, and going through all actions, can we instead assign the reportAction var and then break out of the loop?
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.
The original code iterated over all report actions to get their errors, so if I break the loop it will affect the current app behaviour.
Since there can be multiple report actions with errors, I'm just picking the first report action that shows up
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.
So if we're just picking the first one then I think we can break out of the loop. NAB, please fix in a follow up.
); | ||
expect(reportAction).toBeUndefined(); | ||
}); | ||
// TODO: remove '.failing' once the implementation is fixed |
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.
NAB: Do we have an issue to track 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.
Not that I'm aware of
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.
Would you please create one or ask in Slack to have an issue created?
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.
@neil-marcellini I just checked and it seems there's no implementation issue.
The test is just outdated with the new implementation, so I'll fix it in this PR - #50468 - if that's okay with you? 😄
@pac-guerreiro Could you please resolve this PR? |
@DylanDylann on it, will push changes in a few minutes |
…g-to-debug-mode # Conflicts: # src/libs/actions/IOU.ts
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.
Merging this 🚀
Idk why the reviewer checklist is failing, I can see that it's fully filled out, so merging. |
✋ 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/neil-marcellini in version: 9.0.46-0 🚀
|
This PR is failing for Desktop because of issue #50403 |
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.46-5 🚀
|
Details
Fixed Issues
$#46992
PROPOSAL: N/A
Tests
Debug
Offline tests
Same as tests.
QA Steps
Same as tests.
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop