-
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
Start accounting for notificationPreferences in the unread indicator for Chat Rooms #3766
Conversation
Hey @yuwenmemon! I can't seem to get this to work on Desktop when testing 'always'. I'm pretty sure I've followed the right steps, but the indicator always shows up as 1 regardless of how many messages I send, am I missing anything? Here's a screen capture: demo.mp4 |
@jasperhuangg yep, unfortunately that's expected behavior for Desktop as the Electron API fails otherwise. See my QA step here:
and this comment here: |
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.
Awesome, thanks for entertaining my questions! Changes look good
Didn't see that you referring to an |
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 great and tests well :)
✋ 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 in version: 1.0.74-1🚀
|
@jasperhuangg @yuwenmemon Not sure if this is testable for us at the moment. I tried creating a workspace and adding members but I'm stuck on this page. It won't let me add members or create the chat rooms. |
@isagoico can you add those folks to a policy in the "normal" Expensify web app? |
@isagoico perhaps just to keep things simple can you create the policy and add the members via the classic app? |
mmm I tried creating the policy in classic app and adding the members there and the workspace does not appear in e.cash. This is what I've tried, let me know if I'm missing something:
Since above didn't work
|
I guess I'm a bit confused as to why you're looking for a workspace. We're not testing workspaces in this PR, we want to test the #announce room. |
OH I totally thought that the rooms like #announce were created when a workspace was set up. Not sure how to access the #announce room then 🤔 should it automatically appear on any account? |
@isagoico You'll need to run that snippet in the classic app. There's no applicable test for this for Android, so that makes sense. |
@jasperhuangg please review
Details
When users have notificationPreferences in E.Chat for a given, room (comes live with https://github.com/Expensify/Expensify/issues/161785 cc @jasperhuangg) - follow them for push notifications within the app.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/161781
Tests/QA
"always"
by running the following JS in E.Web (User 1):4
"daily"
by running the following JS in Web Expensify (OldDot) (User 1):1
or is simply blank but present."mute"
by running the following JS in Web Expensify (OldDot) (User 1):Tested On
Screenshots
Web
Mobile Web
N/A
Desktop
iOS
Android
Not Applicable