-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 filtering to the reports data in the Focus mode switch logic #34624
Conversation
Add some methods to check if the report is valid Fix ts stuff
262d6b6
to
9710b71
Compare
@sobitneupane 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] |
I am having trouble building the native apps so kicked off an adhoc build. |
@sobitneupane Can you please kindly give an ETA on when you will be reviewing this PR? |
Sorry for the delay @marcaaron. I won't be able to review it soon. I will ask someone else to take over. |
Reviewer Checklist
Screenshots/VideosAndroid: NativefocusTest1Android.mp4focusTest2Android.mp4Android: mWeb ChromefocusTest1AndroidChrome.movfocusTest2AndroidChrome.moviOS: NativefocusTest1iOS.mp4focusTest2iOS.mp4iOS: mWeb SafarifocusTest1iOSSafari.mp4focusTest2iOSSafari.mp4MacOS: Chrome / SafarifocusTest1Chrome.mp4focusTest2Chrome.mp4 |
src/libs/ReportUtils.ts
Outdated
} | ||
|
||
// We are not the owner or the manager or a participant | ||
if (report?.ownerAccountID !== accountID && report?.managerID !== accountID && !(report?.participantAccountIDs ?? []).includes(accountID)) { |
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.
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.
Oh I see. In the case of a DM - you would not be in the participantAccountIDs
?
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.
DMs are supposed to have an ownerAccountID
of 0
that is expected.
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.
But the problem is that they won't be in there... have a solution one sec.
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.
Let me know if the latest commit looks better!
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.
Yes, this gets correct valid report length,
In this step here
After logging into a new account and adding chats one by one, at the 30th chat it switches to focus mode immediately. So, logging in with 30+ chats to see the focus mode popup is not possible I think because it is already switched to focus mode. We can verify only that. |
Can you please help me with these?
directLogin.mp4
|
Could add 29 then have someone invite you to a DM to make 30 then leave a comment on it? Also feel free to lower the threshhold to make testing more bearable 😄 |
Oh hmm that is probably not possible - but signing in should have the same effect so maybe fine to skip this until QA. |
Please merge main as the Android build is failing. |
Could not identify the exact steps that are causing this but sometimes I am getting this crash on web when I run both npm run desktop and npm run web one after another. Screen.Recording.2024-01-25.at.10.10.04.PM.mov |
What's the crash? Can you look at the logs or share them? I can't tell from that video. I will merge |
I am not able to reproduce the Chrome crash again. In the crashes in the video it said Perhaps this was caused due to opening both of them quickly or perhaps it got fixed with merging |
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
@cristipaval 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] |
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.
Code looks good. Could you make Typescript style pass?
✋ 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/cristipaval in version: 1.4.33-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.33-5 🚀
|
Coming from https://expensify.slack.com/archives/C049HHMV9SM/p1705439950975899
Details
Seems like there is some number of invalid reports that are persisting from one session to the next.
Long term fix: Figure out what is preventing Onyx data from clearing and fix it
Short term fix: Apply some sane filtering to to the reports since the only thing we are checking is if there is a report key in Onyx. This is not really correct now since you can search for reports (e.g. workspace rooms) where you are not a participant yet - these do not show up in the LHN but are stored in the Onyx reports data. So even if we figure out what is causing this data to exist we still need some front end solution to cover reports where you are not a participant.
cc @AndrewGable
Fixed Issues
$ #34778
Tests
Verify focus mode switch still works and there are no regressions
Verify focus mode modal is not incorrectly showed for brand new account after signing out of large account
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 methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.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 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
Unable to test
Android: mWeb Chrome
iOS: Native
Unable to test
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop