Room notification preference updates after changing the room name

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Go to URL https://staging.new.expensify.com/
2. Create a workspace from the green plus (if needed)
3. Create a workspace room from the green plus (if needed)
4. Click the header of the workspace room to open the room details page
5. Click "Settings"
6. Rename the room and hit save

Expected Result:

Only the name should be changed when you press the save key

Actual Result:

Сhanges the name and status of notifications to immediately when pressing the save key

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.1.88.14

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any applause account

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5692908_Recording__1538.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @thienlnam (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[thienlnam]

Looks related to the recent changes to rename policy room cc @neil-marcellini #10237
From some testing, it seems like pressing the save button triggers the Picker onInputChange
I looked through the API changes and it doesn't touch any of the notification stuff so it seems like a bug in App, making it external

[melvin-bot]

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[trjExpensify]

The title of the issue confused me a bit, I think we should update it to something like: Room notification preference updates after changing the room name - what do you think?

Also to confirm, is this issue actually isolated to Web only like the title suggested? The Platform section of the OP suggests otherwise.

Lastly, user-created rooms are behind a beta still, so can this actually go external?

Thanks, @thienlnam!

[thienlnam]

Room notification preference updates after changing the room name

This issue update sounds good to me

Also to confirm, is this issue actually isolated to Web only like the title suggested? The Platform section of the OP suggests otherwise.

I've just tested this on desktop and it appears to be happening there as well so it doesn't seem to be isolated to web

Lastly, user-created rooms are behind a beta still, so can this actually go external?

Good catch on this, I didn't know this was behind a beta - this will have to be internal then

Thanks!

[trjExpensify]

Cool, sounds good. Updating the issue title and removing my assignment as a CM isn't needed.

[thienlnam]

Been focusing on N7, haven't looked into this yet

[thienlnam]

Same as above

[trjExpensify]

Any luck here, @thienlnam? The Guides are now in #admins being assigned leads through the #121-assigned project, and some of them are trying to update their room notification preference to immediately to make sure they don't have a delay in seeing a customers message in the room.

[trjExpensify]

Hey @neil-marcellini - I don't suppose you could help us out here while Jack is out? It's impacting the Guides agents that are being assigned customer deals 121 in NewDot now as of the go live yesterday.

[neil-marcellini]

Yeah I can take a look.

[neil-marcellini]

I think there are two problems here. I put up a draft PR for the first issue where the notification preference picker is updating when the component updates, which happens after the room name saves. However, I'm still seeing the issue occur even when the UpdateReportNotificationPreference api command is not called, so I think there's a backend problem as well.

[neil-marcellini]

I thought I found the problem on Web, but it looks like there's more to it. I'll keep working on this tomorrow.

[trjExpensify]

Thank you, Neil!

[neil-marcellini]

Oh man, this one is weird. I added one failing test today and did a bunch of digging around in the code.

Here's the summary of the problems on the backend:

1. We don't set a notification preference on policy rooms when creating them. CreatePolicyRoom.cpp calls Report::createChatReport and NVP_NOTIFICATION_PREFERENCES is not set along with other NVPs here.
2. We force the notification preference to 'always' when renaming reports. updatePolicyRoomName calls ReportAPI::rename, which calls ReportUtils::notifyParticipants, which calls buildOnyxDataForNewAction here without passing in a notification preference, meaning that it defaults to 'always'.
3. In OpenApp we default the notification preference to 'daily' for all reports here.

If we fix 1 then 3 won't be a problem for newly created rooms. After that we can pass the room notification preference to fix 2. For already existing rooms we could write a CQ to add the NVP to fix them. It would also be good to align our defaults, I think the default should always be 'always' / immediately. Lots to think about.

[trjExpensify]

Thanks, Neil! On this one:

It would also be good to align our defaults, I think the default should always be 'always' / immediately. Lots to think about.

There's a bit of background here on why policy rooms were designed to start as Daily, mimicking what you would expect of buoyant company channels like #general, #engineering-chat etc where you would opt-in for Immediately.

That said, you'll also see in that linked issue that we're looking to change the default for the #admins room specifically, given that its primary purpose has evolved to be the central place to communicate with your Expensify assigned agent for onboarding (and later, on-going account management).

[neil-marcellini]

Today I worked on an Auth PR for step 1. The test I added is still failing because I'm setting the notification preferences only for the admin. I think it will be a quick fix tomorrow and I hope to get that up for review. I can also probably put the App PR up for review because I don't think anything else has to change on the front end.

[neil-marcellini]

I thought of a new approach. In PHP when we notify participants, if there are no notification preferences set on the room then I'll call a function to set the default for the room and save it in the NVP. Then I will use the NVP notification preferences when notifying participants. I think that will fix all existing rooms when renaming. We won't need a custom query in that case. Then I can put up some other PRs to fix the notification preference for rooms when they get created, such as that Auth PR I worked on yesterday.

[neil-marcellini]

I just added a failing test that proves that room notification preferences are also changed when you send a message (addComment) in a room.

[trjExpensify]

Wow, that's a great catch haha!

[neil-marcellini]

Right now I have passing tests that confirm that the room notification preference is not changed for the acting user when renaming rooms and commenting.

I also wanted to test that the notification preference for other room members remain the same, but when I invited another policy member in the test they didn't get included in the participantAccountIDs here. The member was only a participant in the announce room, but from front end testing it looks like they should be part of the expense chat too. I'll ask for some help with this on Monday.

[neil-marcellini]

After some more digging in the code this morning I learned that the #announce room is shared with a newly invited workspace member here and that the admin's policy expense chat is not shared with the newly invited member. Instead, when a member is added they get their own unique policy expense chat between themself and the policy admins which is created here. Then we also return that chat to the admin here in SharePolicy.cpp.

Now that I have that straightened out I've been able to move forward with the testing. I found two issues that should be fixed as well.

First, in the original design doc we said that the workspace chat notification preference should be 'immediately' but right now it's 'daily'. https://github.com/Expensify/Expensify/issues/230454.

Second, the workspace member's view of the policy expense chat does not show the admin as a member. I plan to fix the first issue in this PR, and I'll leave the second issue for later. https://github.com/Expensify/Expensify/issues/230457

[neil-marcellini]

My Web-PR is in review and I learned that it's unnecessary to save the default notification preference at all, which means the Auth PR won't be necessary. I think it would still be a good idea to update the default notification preference in the optimistic data in App so I'll add that to the App PR tomorrow.

[neil-marcellini]

I made some updates to the Web-PR which fix the default notification preference when the app is opened / reconnected.

[neil-marcellini]

Sorry this is taking so long! I battled against some flaky tests for the last two days, but I finally got that straightened out. I really think we can get the Web-PR merged on Monday, then then the App PR will follow and fix the issue. The App PR is almost ready; I'm working my way through the author checklist.

[trjExpensify]

Thanks for persevering, Neil!

[neil-marcellini]

I'm waiting on one last approval on the Web-PR and the App PR is also up for review now.

[neil-marcellini]

Web-PR is on staging so I'll take the App PR off of hold.

[neil-marcellini]

I created a tiny follow up issue to set the default room notification preference in the optimistic data.

[trjExpensify]

Nice - thanks so much Neil! 🎉

Quick question on the fix just merged. Once deployed, will it update the notification preference to Always for the #admins rooms already created, or just new ones going forward?

[neil-marcellini]

You're welcome 😄

Quick question on the fix just merged. Once deployed, will it update the notification preference to Always for the #admins rooms already created, or just new ones going forward?

Good question. Yes if they refresh / reopen the app the default will be set correctly. Or if there is any action taken on the room the default will also be set properly. They can also manually update the notification preference and their change will be saved properly. Please let me know if there are any other issue with this, after the Web and App PRs are on prod.

[trjExpensify]

Nice, that's handy - the Guides are covered then as they're in app daily.

Or if there is any action taken on the room the default will also be set properly.

If a Guide sends a follow-up message in the room, will that action update the room notification preference for all participants of the room?

[neil-marcellini]

If a Guide sends a follow-up message in the room, will that action update the room notification preference for all participants of the room?

Yep, all users will be sent the correct default or their desired (saved) notification preference.

[trjExpensify]

Awesome, thanks for confirming!