-
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 RBR reason to LHN debugging #50831
Add RBR reason to LHN debugging #50831
Conversation
Todo:
|
…o-lhn-debugging # Conflicts: # src/libs/DebugUtils.ts # src/libs/SidebarUtils.ts # src/pages/Debug/Report/DebugReportPage.tsx # tests/unit/DebugUtilsTest.ts
Today's update:
Todo:
|
@DylanDylann 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] |
The PR is now ready for review! 😄 Go ahead @DylanDylann |
I just requested confirmation for the translations in |
…o-lhn-debugging # Conflicts: # tests/unit/DebugUtilsTest.ts
Discover an error log on Android and IOS app when going to debug report page Screen.Recording.2024-10-18.at.15.10.41.movIt is caused by using 2 scrollView on DebugReportPage |
@DylanDylann I applied your suggestions to the translations and I fixed the VirtualizedLists issue 😄 |
@pac-guerreiro Please ping me when you update Jest UTs |
@DylanDylann I just fixed the unit tests that were failing 😄 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-10-18.at.15.08.39.movAndroid: mWeb ChromeScreen.Recording.2024-10-18.at.15.03.59.moviOS: NativeScreen.Recording.2024-10-18.at.15.06.04.moviOS: mWeb SafariScreen.Recording.2024-10-18.at.15.04.34.movMacOS: Chrome / SafariScreen.Recording.2024-10-18.at.15.05.22.movMacOS: DesktopScreen.Recording.2024-10-18.at.15.02.42.mov |
style={styles.mt5} | ||
contentContainerStyle={styles.pb5} | ||
> | ||
<View style={[styles.mv5, styles.flex1]}> |
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: But we have a point on the checklist
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page
Here, we use Flatlist instead of ScrollView. But I think It still make sense
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.
@DylanDylann Thanks for pointing this out! I just applied added ScrollView again and disabled scroll on the FlatList and the problem is gone 😄
Let me know if you like this solution!
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.
Looks good to me. Thanks for adding the tests. Just one question about the TODO comment.
tests/unit/DebugUtilsTest.ts
Outdated
) ?? {}; | ||
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.
What does this mean?
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.
@puneetlath Sorry, I missed this while resolving a merge conflict with main 😅 I just reverted this change so it matches what we currently have on main!
All feedback from @puneetlath @DylanDylann were applied 😄 I also applied the changes requested by @Gonals to the spanish translations, thank you 🙌 (Sorry for the late update but I had to travel between places) |
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.
@puneetlath All yours
Approved, but there are conflicts @pac-guerreiro |
…o-lhn-debugging # Conflicts: # src/languages/en.ts # src/languages/es.ts
@puneetlath conflicts resolved 😄 |
✋ 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/puneetlath in version: 9.0.52-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.52-5 🚀
|
Details
Fixed Issues
$#50665
PROPOSAL: #46992 (comment)
Tests
Debug Mode
inTroubleshoot
Debug
Has RBR
is shown inVisible in LHN
RBR
section displays the correct reason for why the report has aRBR
If you don't have a report with RBR in LHN, then just create an
expense
and remove a required property (e.g.category
) in order to trigger a missing violation.Offline tests
Same as above
QA Steps
Same as above
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