[$250] Room -Notify me about new messages is shown as "daily"

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.57-2
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4458530
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Launch app
2. Tap fab --- start chat -- room
3. Enter room name with
4. Make sure visibility is " Workspace "
5. Tap create room
6. Tap header
7. Tap settings

Expected Result:

Notify me about new messages must be shown as "immediately" in room created with visibility " Workspace ".

Actual Result:

Notify me about new messages is shown as "daily" by default instead of "immediately" in room created with visibility " Workspace ".

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6428803_1711551923141.notifications.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f2eeabf999360436
• Upwork Job ID: 1773032769246867456
• Last Price Increase: 2024-03-27

[melvin-bot]

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @blimpich (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[izarutskaya]

@kadiealexander I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

[izarutskaya]

We think that this bug might be related to #vip-vsb.

[izarutskaya]

Production

[blimpich]

Not a blocker. Demoting. Looking into to see what could have caused this regression. Seems like a simple bug.

[blimpich]

Hmmm, okay I spent some time looking at the PRs for this deploy and nothing really stands out to me. Lets open this up to contributors.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01f2eeabf999360436

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

[melvin-bot]

Upwork job price has been updated to $250

[FitseTLT]

It is the BE that is returning notification preference daily and if u use staging server and if not it works well

[sobitneupane]

The response received on creating a new room has "notificationPreference" set as "daily". So, I believe it's a backend issue.

cc: @blimpich

[blimpich]

Unassigning myself since I don't have bandwidth for this. We should assign this a wave / vip or close it as not worth fixing if it doesn't fit into any of them.

[kadiealexander]

Updated the project and priority. I'm half-debating closing this, it doesn't seem to be a big deal.

[melvin-bot]

@kadiealexander this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[tgolen]

@mountiny @aldo-expensify Are either of you aware of any changes that could have lead to this? I saw that you've both done some work on these notification preferences. From my investigation, it looks like it might have coincided with this change but it's not clear to me from looking at the code how the notification preferences get set in the CreatePolicyRoom flow.

This code is the closest thing I could find, but in the CreatePolicyRoom flow, parentReportActionID is null so that block would never execute.

This must have been something that recently changed as opposed to something that was never working (since the problem initially wasn't reproducible on production).

Is this something that needs to be added to Report::getDefaultNotificationPreference() for the CHAT_TYPE_POLICY_ROOM?

[aldo-expensify]

(since the problem initially wasn't reproducible on production).

If this was the case, I think it may have been a change in Web-Expensify since there is no "staging Auth".

Maybe this PR: https://github.com/Expensify/Web-Expensify/pull/41437/files#diff-7a032f7284c8b42392fb9715c5196b05b34ebeb369ee31387f702a26980eb792L1299-R1303

It touches the onyx updates sent after creating the policy room.

It would be good to:

1. Reproduce the bug
2. Log out and log in
3. Check the notification preference

if in step 3. the notification preference is correct, then it is a problem of the pushed onyx updates

[mountiny]

I am not aware of anything recent that I would work on or reviewed, but the PR Aldo linked looks suspicious and aligns timewise to when this bug was raised

[iwiznia]

Maybe this PR: Expensify/Web-Expensify#41437 (files)

I thought so too, but this was open 6 hours before that was deployed to staging...
Anyway, taking a look just in case.

[iwiznia]

ok I am almost sure this has nothing to do with my changes, as that only changes what updates we queue and I just tested and this is wrongly saved in auth

[iwiznia]

Not a blocker. Demoting. Looking into to see what could have caused this regression. Seems like a simple bug.

I disagree with this. This was broken in staging and not prod, so it should've blocked as it is creating bad data in the DB that either willl remain bad forever or we need to fix manually.
It was probably a web-e blocker, not an App blocker (assuming that by the time we looked at it we had not deployed web). cc @blimpich

[blimpich]

Fair, I wrongly assumed this wasn't a backend issue when I demoted it originally. The fact that we're creating bad data does make this worse than I thought. I can take a crack at this this afternoon and see if I can find the issue.

[blimpich]

Hmm, I haven't been able to figure out which PR caused this. Trickier than it seems. I suspected this PR but actually on second look the timing is a little off. Merged after this was reported by less than a day. Also suspicious of this PR but reverting it is rife with merge conflicts that I don't have the brain power to wade through.

Frustrating because the fix is actually pretty easy. I can put up a PR to fix this today. Though we should probably add a unit test to cover regressions as well.

[blimpich]

I have a very hard time believing that this was ever a bug in Auth since we have a test that hasn't changed in 7 months that asserts that a newly created workspace visibility room should have a notification preference set to Daily. See here.

Additionally, the test rail steps linked in the issue description do not specify what the default notification setting should be.

All of this makes me think this is intended behavior.

[blimpich]

Asking in #product about what we intended for this to be link. If we want it to be immediately I can do that, if we want it to stay the same we should update the test rail steps to say so.

[blimpich]

There doesn't seem to be a consensus on whether we intentionally wanted the default notification preference set to daily or immediate, so I'll be airing on the side of doing as little as possible and just updating the QA steps to check for the notification preference being set to Daily. Best to "do nothing" in this case I think.