Add notification preference for all the reports and update the design for the group chats #28200
Conversation
melvin-bot
bot
requested review from
mollfpr
and removed request for
a team
|
@mollfpr I am also working from Bali 😄. Let me know if the changes look good to you. I will add the videos by then. If you have questions, you can put them here, and we can resolve them quickly. Looking to get this shipped ASAP as it is a wave8 priority. |
|
Added @arosiclair for review as he is assigned to review the web-e changes. |
|
Take the review now!!! @techievivek Really?? I didn't see you in the 10am spot in Ubud 😅 |
|
@mollfpr I only joined for the last week 😢, so now we are working from Jimbaran. |
|
@techievivek Is the Web-E PR in staging? |
|
Thank you for prioritizing this @mollfpr, this is an important piece of an external deadline project so your help is super appreciated! |
|
@techievivek The selected option is not changed on the profile page. Screen.Recording.2023-09-27.at.10.36.04.mov |
|
@mollfpr Aaah sorry the backend changes are not yet live, do you want me to share my ngrok URL so you can test them? |
|
@techievivek That's would be great! |
|
@mollfpr I am looking at your videos and it looks like this should have worked because it doesn't seem to actually depend on the backend changes. Do you mind sharing the onyx response for the request. Does it include the updated notification preference for the report? |
|
Tested with your ngrok. The response seems fine, but the selected value doesn't change.
Screen.Recording.2023-09-27.at.11.27.56.mov |
|
I think the issue is because the page is not re-render after selecting the preference. |
|
@mollfpr How is it going? Let me know if you are stuck anywhere or need help with anything, thanks. |
There was a problem hiding this comment.
Sorry @techievivek I forgot to approve the PR. The changes looks good and it tests well on Web, Desktop and Android. I still uploading the recordings to the checklist.
|
@hayata-suenaga 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] |
|
@arosiclair Can you please give this a review so we can get this merged today. |
|
🤦 We are once again in the conflict zone, let me resolve them quickly. @mollfpr can you please add the ios recordings for any single test(just for DM). |
|
@techievivek Sure! |
I think the checklist is already done by @mollfpr. @arosiclair is assigned as an internal engineer. melvin, why did you assign me??? Anyway, I'm unassigning myself |
| key: ONYXKEYS.SESSION, | ||
| }, | ||
| }), | ||
| // eslint-disable-next-line rulesdir/no-multiple-onyx-in-file |
There was a problem hiding this comment.
Not sure I understand that comment, but it sounds like there's urgency behind this so I won't be a blocker. However, can we create a follow up issue to clean this up and remove the need for a second Onyx subscription? I'm not sure exactly how it should be cleaned up but it definitely doesn't need to be like this. Thanks!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@arosiclair Created one here for the cleanup/improvements: #29165 |
|
🚀 Deployed to staging by https://github.com/techievivek in version: 1.3.81-0 🚀
|
| if (Number(session.accountID) === accountID || Session.isAnonymousUser()) { | ||
| return null; | ||
| } | ||
| return `${ONYXKEYS.COLLECTION.REPORT}${lodashGet(ReportUtils.getChatByParticipants([accountID]), 'reportID', '')}`; |
There was a problem hiding this comment.
This caused regression when user opens new user profile link first time.
This happens because report_ returning all reports.
We should fix this by setting report_0 or report_null or null
| if (Number(session.accountID) === accountID || Session.isAnonymousUser()) { | |
| return null; | |
| } | |
| return `${ONYXKEYS.COLLECTION.REPORT}${lodashGet(ReportUtils.getChatByParticipants([accountID]), 'reportID', '')}`; | |
| const reportID = lodashGet(ReportUtils.getChatByParticipants([accountID]), 'reportID', ''); | |
| if (Number(session.accountID) === accountID || Session.isAnonymousUser() || !reportID) { | |
| return null; | |
| } | |
| return `${ONYXKEYS.COLLECTION.REPORT}${reportID}`; |
Right now, app crashes and will be a deploy blocker soon
There was a problem hiding this comment.
raised PR for the quick fix
| @@ -61,8 +61,7 @@ const defaultProps = { | |||
| function ReportDetailsPage(props) { | |||
| const policy = useMemo(() => props.policies[`${ONYXKEYS.COLLECTION.POLICY}${props.report.policyID}`], [props.policies, props.report.policyID]); | |||
| const isPolicyAdmin = useMemo(() => PolicyUtils.isPolicyAdmin(policy), [policy]); | |||
| const shouldDisableSettings = useMemo(() => ReportUtils.shouldDisableSettings(props.report), [props.report]); | |||
| const shouldUseFullTitle = !shouldDisableSettings || ReportUtils.isTaskReport(props.report); | |||
| const shouldUseFullTitle = ReportUtils.isTaskReport(props.report); | |||
|
🚀 Deployed to staging by https://github.com/techievivek in version: 1.3.83-0 🚀
|
| const shouldDisableSettings = _.isEmpty(report) || ReportUtils.shouldDisableSettings(report) || ReportUtils.isArchivedRoom(report); | ||
| const shouldDisableSettings = _.isEmpty(report) || ReportUtils.isArchivedRoom(report); |
There was a problem hiding this comment.
After enabling settings for all the reports and revealing access to the notification preference page. We had to test the offline behavior for the newly enabled reports. Coming from #29334 this caused a regression where the creation of a new task missed the optimistic prop notificationPreference.
| if (isChatRoom(report) || isPolicyExpenseChat(report) || isChatThread(report) || isTaskReport(report) || isMoneyRequestReport(report)) { | ||
| Navigation.navigate(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(report.reportID)); | ||
| return; | ||
| } | ||
| if (participantAccountIDs.length === 1) { | ||
| if (isDM(report) && participantAccountIDs.length === 1) { |
There was a problem hiding this comment.
This created a regression which is fixed in #30182
| if (Number(session.accountID) === accountID || Session.isAnonymousUser()) { | ||
| return null; | ||
| } | ||
| return `${ONYXKEYS.COLLECTION.REPORT}${lodashGet(ReportUtils.getChatByParticipants([accountID]), 'reportID', '')}`; |
There was a problem hiding this comment.
You haven't taken into account the dependency of getChatByParticipants on reports which caused #35654
Details
Here are a few things that got updated in this PR.
Fixed Issues
$ #27394
PROPOSAL: N/A
Tests
Needs to be tested with this Web-E PR: https://github.com/Expensify/Web-Expensify/pull/38977
dailyand confirm you no longer see the notification.dailyand confirm you no longer see the notification.dailyand confirm you no longer see the notification.Login to newDot as userA on one device/browser.
Login to newDot as userB on another device/browser.
Create a new workspace and create a new public room.
As userA, write a few message to the room.
As userB, join the above public room.
As userA, write few more new message and confirm userB don't receive a notification.
Update the notification preference for userB to always and confirm they receive a notification.
Verify that no errors appear in the JS console
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android