-
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
[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] Wave8: Add notification preferences to all report types #27394
Comments
Current assignee @trjExpensify is eligible for the NewFeature assigner, not assigning anyone new. |
Hmm I'm not sure why there is a difference between groupDMs and single DMs. I would say that we should be consistent here and maybe always put the "Notify me..." row behind Settings in all cases. Otherwise just expose the "Notify me" row at the first level on both Groups and regular DMs. For Group DMs, I see that you have a "Title" row mocked up behind Settings. Any reason for that? I feel like we can leave that out because it can't be edited. |
Oops I thought I pressed send either but just realized that I didn't... I paused briefly on |
Yeah, maybe to elaborate on my thinking there. Single DMs open to a
I thought about that as well, but then my second thought was that GroupDMs are soon getting an overhaul where you can invite members, change the name etc like a workspace room, so I thought it better to foreshadow it by keeping the notifications preference inside the |
Ah, the |
Got it. I don't feel strongly if you want to keep it as you have mocked or wait until the Group DMs get the overhaul and include it then. Seems like given that the title isn't related to notification preferences, we don't need to include it here. |
Donezo! |
Job added to Upwork: https://www.upwork.com/jobs/~0139328089cd0ca439 |
Triggered auto assignment to Contributor Plus for review of internal employee PR - @mollfpr ( |
Triggered auto assignment to @neil-marcellini ( |
Thread here where we're looking for a volunteer! |
Reminder: We will need to make sure to update/add the regression tests for the changes introduced here. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.83-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-10-20. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.83-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-10-20. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.84-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-10-23. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.84-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-10-23. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
@trjExpensify I think we don't need the checklist for this issue, because this a new feature. |
Well, we should still consider regression tests and stuff or if it's new and could benefit from a PR checklist update etc. Anyhow, Mitch has taken care of adding a regression test in this instance. |
Confirming this is a flat $500 to @mollfpr for the internal PR review. |
$500 payment approved to @mollfpr based on post above. |
👋 @MitchExpensify, coming from here.
Problem
Today, only the workspace rooms have notification preferences. That creates a confusing experience whereby some chats you can control how you're notified, whereas others you can't. Needless to say, it's totally against our philosophy of maintaining consistency at every possible turn.
Why is this important?
Inflexible notifications are notoriously a sticking point for people, and we're gearing up to launch chat for prime time!
Solution
Add the three
mute
,daily
andimmediately
notification preferences to all report types:chat
(i.e DMs, GroupDMs, policyExpenseChat, Rooms, Threads)expense
(including “transaction threads”)iou
(including "transaction threads")task
The default setting for all report types should be
immediately
.hidden
prior to joining, but that's out of scope here, and we'll setimmediately
as the default for any thread you've joined.The notification preference setting will be accessed by tapping the chat header on the
Details
>Settings
page like you do today with rooms.expense
andiou
reports, you can't click the header yet to access "Details", but that will soon be possible with this PR close to merging. (CC: @fedirjh).groupDMs
where inDetails
today it's just a current list of members, we'll need move those behind aMembers >
row and add a row forSettings >
. While we're here, let's also include the groupDM avatar and chat title for more consistency with the other report details pages:DMs
where clicking the chat's header navigates toProfile
, we'll add aNotify me about new messages
push row directly for the notification setting:CC: @JmillsExpensify @shawnborton
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: