Add LHN debugging to Debug Mode#48442
Conversation
fabioh8010
left a comment
There was a problem hiding this comment.
Partial review for now
blimpich
left a comment
There was a problem hiding this comment.
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
blimpich
left a comment
There was a problem hiding this comment.
one comment, otherwise looks good, will wait on Fabio to approve before giving final approval
fabioh8010
left a comment
There was a problem hiding this comment.
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.
| 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 😄 |
src/libs/SidebarUtils.ts
Outdated
| return LHNReports; | ||
| } | ||
|
|
||
| function isRedBrickRoad(report: Report, reportActions: OnyxEntry<ReportActions>, hasViolations: boolean, transactionViolations?: OnyxCollection<TransactionViolation[]>) { |
There was a problem hiding this comment.
| 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.
| 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 : ''; |
neil-marcellini
left a comment
There was a problem hiding this comment.
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.
| if (action && !isEmptyObject(action.errors)) { | ||
| Object.assign(reportActionErrors, action.errors); | ||
|
|
||
| if (!reportAction) { |
There was a problem hiding this comment.
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.
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.
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.
NAB: Do we have an issue to track this?
There was a problem hiding this comment.
Not that I'm aware of
There was a problem hiding this comment.
Would you please create one or ask in Slack to have an issue created?
There was a problem hiding this comment.
@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
|
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
DebugOffline tests
Same as tests.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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
MacOS: Desktop