[HOLD for payment 2024-04-03] [$500] In-app sounds play in the background and message received sounds play while not viewing the chat #37098

m-natarajan · 2024-02-22T18:37:15Z

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.43-12
Reproducible in staging?: y
**Reproducible in production?:**y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @arosiclair
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1708621869934919

Action Performed:

Log in
Open a chat
Minimize the window or switch tabs
Receive a message in the chat

Expected Result:

No sounds play for the message

Actual Result:

The message received sound plays

Workaround:

unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~011ab7662a4d0687c6
Upwork Job ID: 1760735530618884096
Last Price Increase: 2024-02-29
Automatic offers:

abdulrahuman5196 | Reviewer | 0
dukenv0307 | Contributor | 0

melvin-bot · 2024-02-22T18:37:19Z

Job added to Upwork: https://www.upwork.com/jobs/~011ab7662a4d0687c6

melvin-bot · 2024-02-22T18:37:23Z

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

melvin-bot · 2024-02-22T18:37:27Z

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

rayane-djouah · 2024-02-22T18:43:40Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

In-app sounds play in the background and message received sounds play while not viewing the chat

What is the root cause of that problem?

Currently, there is no check to determine whether the app is visible or whether we are viewing the chat before triggering the sound.

What changes do you think we should make in order to solve the problem?

To address this problem, we should add a conditions before playing in-app sounds.

Muting in-app sounds while the app is in the background

we should add an early return at the beginning of the function when the app is not visible, here in playSound function:

App/src/libs/Sound/index.ts

Line 60 in a868c1a

const playSound = (soundFile: ValueOf<typeof SOUNDS>) => {

and here in playSoundForMessageType function:

App/src/libs/actions/User.ts

Line 490 in a868c1a

function playSoundForMessageType(pushJSON: OnyxServerUpdate[]) {

if (!Visibility.isVisible()) {
    // Don't play sounds if the app is not visible
    return;
}

While adding the condition in playSound is sufficient, adding it to playSoundForMessageType as well provides optimization by avoiding unnecessary computations.

Muting in-app message received sounds while not viewing the chat

we should change the logic of the playSoundForMessageType to play the sound only when the route contains the id of the report with updated actions:

App/src/libs/actions/User.ts

Line 490 in a868c1a

function playSoundForMessageType(pushJSON: OnyxServerUpdate[]) {

function playSoundForMessageType(pushJSON: OnyxServerUpdate[]) {
    if (!Visibility.isVisible()) {
        // Don't play sounds if the app is not visible
        return;
    }
    const reportActionsOnly = pushJSON.filter((update) => update.key.includes('reportActions_'));

    const activeRoute = Navigation.getActiveRoute();

    // Use filter to get the report actions of the visible report
    const visibleReportActions = reportActionsOnly.filter((value) => {
        const reportID = value.key.split('_')[1];
        return activeRoute.includes(ROUTES.REPORT_WITH_ID.getRoute(reportID));
    });

    if (!visibleReportActions.length) {
        // No matching reportID found, we are not viewing the updated report
        return;
    }

    // Extract the reportID from the first visible report action
    const reportID = visibleReportActions[0].key.split('_')[1];

    Promise.all([isChannelMuted(reportID)])
        .then(([isMuted]) => {
            if (isMuted) {
                return;
            }
        })
        .then((isSoundMuted) => {
            if (isSoundMuted) {
                return;
            }

            try {
                const flatten = visibleReportActions.flatMap((update) => {

    // Rest of the code remains unchanged...

What alternative solutions did you explore? (Optional)

We can use Navigation.getTopmostReportId() to determine whether we should play the sound, but in this way, we will play the sound even if the user is not viewing the chat (in settings pages...) (Ex: when we are in single pane homepage)

shawnborton · 2024-02-22T20:53:03Z

Hmm actually on second look, I don't think this is a bug. The messaging sounds are about playing sounds each time a message is received. The notification preference is related to push notifications or email notifications about when emails are received.

Curious if you agree @Julesssss @kirillzyusko

arosiclair · 2024-02-22T21:43:32Z

Reached a better understanding here:

I guess in my head there are only

In-app sounds

Notification sounds

These are in-app sounds so the root problem would really be that we're playing in-app sounds while the app is in the background. When they play in the background they act like "notification sounds" and are ignoring my notification preference

So to fix this we should just mute our in-app sounds (like the message sent/received sounds) while the app is in the background. We should be able to check that with Visibility.isVisible()

rayane-djouah · 2024-02-22T22:49:13Z

Proposal

Updated

kirillzyusko · 2024-02-23T11:44:34Z

@shawnborton I think you are right. @Julesssss curious about your opinion 👀

Julesssss · 2024-02-23T11:50:52Z

The notification preference is related to push notifications or email notifications about when emails are received.

Yeah, I agree with this. Currently, it feels very weird on iOS/Android, but we are in the process of removing the in-app sound for mobile and moving the sound to the push notification.

shawnborton · 2024-02-23T13:26:23Z

Also just wanted to note here, I do think that while you are actively viewing a chat message, it makes sense to play the chat sounds even if you have notifications set to Weekly. I think there is a difference between in-app sounds vs when you are notified about new chats.

dukenv0307 · 2024-02-24T17:28:43Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

Message received sounds play for rooms with Daily notification preference

What is the root cause of that problem?

Currently, we always play sound once the message is received without considering whether the app is in foreground or background.
Additional note, currently, we have two types:

In-app sounds
Notification sounds

The setting "Notify me about new message" (with 3 option "Immediately" | "Daily" | "Mute") should belong to "Notification sounds".
And the setting "Mute all sounds from Expensify" (with 2 option "yes" | "no") should belong to in-app sounds.
The case we are need to fix in this issue is "In-app sounds".

What changes do you think we should make in order to solve the problem?

Before going to the solution, we need to list the expected behaviors:

In background, always mute in-app sounds
In foreground, mute in-app sounds in two cases: One case is when "Mute all sounds from Expensify" is set to true and another case is when user is not actively viewing a chat

Based on the above expected behaviors, we need to early return this function if the app is in background or user is not viewing the chat and also remove the condition as I mentioned in the RCA section that the isChannelMuted(reportID) is not related to "In-app sounds".
We can do it by updating function to:

function playSoundForMessageType(pushJSON: OnyxServerUpdate[]) {
    const reportActionsOnly = pushJSON.filter((update) => update.key.includes('reportActions_'));
    // "reportActions_5134363522480668" -> "5134363522480668"
    const reportIDs = reportActionsOnly
+      .map((value) => value.key.split('_')[1])
+      .filter((reportID) => {
+          return reportID === Navigation.getTopmostReportId() && Visibility.isVisible() && Visibility.hasFocus();
+      });
        Promise.all(reportIDs.map((reportID) => isChannelMuted(reportID)))
        .then((muted) => muted.every((isMuted) => isMuted))
        .then((isSoundMuted) => {
-          if (isSoundMuted) {
-              return;
-          }

Along with reportID === Navigation.getTopmostReportId(), we can also add the check if the current focused screen is report screen or not (optional).

What alternative solutions did you explore? (Optional)

We can also create a util function to check should we play a sound or not, named shouldPlayNewMessageSound (similar to shouldShowReportActionNotification):

function shouldPlayNewMessageSound(reportID: string, action: ReportAction | null = null): boolean {
   // If the action was deleted, do not play sound.
    if (action && ReportActionsUtils.isDeletedAction(action)) {
        return false;
    }

    // If the report does not exist or is pending deleted, do not play sound.
    const report = currentReportData?.[reportID];
    if (!report || (report && report.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)) {
        return false;
    }

    // If this play sound was delayed and the user saw the message already, do not play sound.
    if (action && report?.lastReadTime && report.lastReadTime >= action.created) {
        return false;
    }

    // If this is a whisper targeted to someone else, do not play sound.
    if (action && ReportActionsUtils.isWhisperActionTargetedToOthers(action)) {
        return false;
    }

    // If we are currently not viewing this report, do not play sound.
    return reportID === Navigation.getTopmostReportId() && Visibility.isVisible() && Visibility.hasFocus()
}

Then use it to check should we play sound or not

abdulrahuman5196 · 2024-02-26T11:49:30Z

Reviewing now

abdulrahuman5196 · 2024-02-26T11:59:55Z

@dukenv0307 I dont see this in the expectation mentioned above (although I see some discussions on slack related to same)

In foreground, mute in-app sounds in two cases: One case is when "Mute all sounds from Expensify" is set to true and another case is when user is not actively viewing a chat

@arosiclair Could you kindly confirm if below is the only expectation to solve? As mentioned by you here - #37098 (comment)

So to fix this we should just mute our in-app sounds (like the message sent/received sounds) while the app is in the background

dukenv0307 · 2024-02-26T12:55:03Z

@abdulrahuman5196

I dont see this in the expectation mentioned above (although I see some discussions on slack related to same)

"One case is when "Mute all sounds from Expensify" is set to true": This one is my suggestion
"another case is when user is not actively viewing a chat": This one is mentioned in here

arosiclair · 2024-02-26T15:30:32Z

Yeah sorry for the moving target. We had some more discussion in this thread and came to this conclusion:

Muting in-app sounds while the app is in the background

Muting in-app message received sounds while not viewing the chat

I'll update the title and description

melvin-bot · 2024-03-05T09:44:31Z

📣 @abdulrahuman5196 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

melvin-bot · 2024-03-05T09:44:40Z

📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

dukenv0307 · 2024-03-06T12:13:49Z

The draft PR is created, I am testing all the related cases

dukenv0307 · 2024-03-07T09:13:05Z

@Julesssss

In the case below, user is viewing the profile, so should we consider that user is actively viewing the chat?

My suggestion is, we should consider that the user is viewing the chat in this case because: if we play in-app sound, the notification is not displayed, and vice versa. Currently, the condition to check whether we should display notification is:

App/src/libs/actions/Report.ts

Lines 2030 to 2034 in ffa731a

    
           // If we are currently viewing this report do not show a notification. 
        
           if (reportID === Navigation.getTopmostReportId() && Visibility.isVisible() && Visibility.hasFocus()) { 
        
               Log.info(`${tag} No notification because it was a comment for the current report`); 
        
               return false; 
        
           }

With that condition, we will not play notification, and should play the in-app sound

Julesssss · 2024-03-07T09:54:33Z

My suggestion is, we should consider that the user is viewing the chat in this case

Ah good spot. I agree, but we'll need to check that isVisible works and if not maybe handle this differently for Android/iOS, as they aren't going to see the message... We might also need to consider mWeb users

arosiclair · 2024-03-07T15:43:31Z

My suggestion is, we should consider that the user is viewing the chat in this case

Ah good spot. I agree, but we'll need to check that isVisible works and if not maybe handle this differently for Android/iOS, as they aren't going to see the message...

Yeah this is a good point. Looking at the profile page is fairly rare though so maybe we can ignore that use case? If a user is looking at it and they're hearing message received sounds below it, they should understand what's happening.

Julesssss · 2024-03-07T15:50:18Z

@arosiclair do you recall the push notification logic? If we have to modify both to support profile page I agree it's probably unnecessary for now

arosiclair · 2024-03-07T16:15:36Z

Push notifications are currently hidden on web and mobile when viewing the profile page (because the report is under it).

Julesssss · 2024-03-07T16:23:49Z

Let's leave it then.

dukenv0307 · 2024-03-09T03:26:28Z

@abdulrahuman5196 PR #37872 is ready to review

melvin-bot · 2024-03-27T04:42:14Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2024-03-27T04:42:18Z

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.56-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

Fix/37098: In app sound is played if user not viewing chat #37872

If no regressions arise, payment will be issued on 2024-04-03. 🎊

For reference, here are some details about the assignees on this issue:

@abdulrahuman5196 requires payment automatic offer (Reviewer)
@dukenv0307 requires payment automatic offer (Contributor)

melvin-bot · 2024-03-27T04:42:19Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@abdulrahuman5196 / @dukenv0307] The PR that introduced the bug has been identified. Link to the PR:
[@abdulrahuman5196 / @dukenv0307] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[@abdulrahuman5196 / @dukenv0307] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[@abdulrahuman5196 / @dukenv0307] Determine if we should create a regression test for this bug.
[@abdulrahuman5196 / @dukenv0307] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[@zanyrenney] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

abdulrahuman5196 · 2024-04-01T12:49:06Z

The PR that introduced the bug has been identified. Link to the PR:
The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

Not a regression

Determine if we should create a regression test for this bug.
If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

I think in the last sound related GH i saw the mention, that we are going to create common test case for sounds handling. So I don't think we need here separately.

cc: @Julesssss @zanyrenney

Julesssss · 2024-04-02T10:53:30Z

Yeah, I haven't done this yet but I agree.

zanyrenney · 2024-04-03T14:15:08Z

Yep, makes sense to me!

zanyrenney · 2024-04-03T14:16:15Z

payment summary

@abdulrahuman5196 requires payment automatic offer (Reviewer) paid $500 via Upwork
@dukenv0307 requires payment automatic offer (Contributor) paid $500 via Upwork

m-natarajan added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 22, 2024

melvin-bot bot changed the title ~~Message received sounds play for rooms with Daily notification preference~~ [$500] Message received sounds play for rooms with Daily notification preference Feb 22, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 22, 2024

melvin-bot bot assigned abdulrahuman5196 and zanyrenney Feb 22, 2024

shawnborton mentioned this issue Feb 22, 2024

[Tracking issue] Implement sound feature tasks and bugs #36846

Closed

This comment was marked as outdated.

Sign in to view

melvin-bot bot added the Overdue label Feb 26, 2024

melvin-bot bot removed the Overdue label Feb 26, 2024

Julesssss mentioned this issue Feb 26, 2024

[$500] User receiving "new message" notification sound even if the phone is on DND #36714

Closed

6 tasks

Julesssss self-assigned this Feb 26, 2024

dylanexpensify mentioned this issue Feb 26, 2024

Notifications - User receives audio notifications when a message arrives when Daily sets #36875

Closed

6 tasks

arosiclair changed the title ~~[$500] Message received sounds play for rooms with Daily notification preference~~ [$500] In-app sounds play in the background and message received sounds play while not viewing the chat Feb 26, 2024

Julesssss assigned dukenv0307 Mar 5, 2024

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 5, 2024

dukenv0307 mentioned this issue Mar 7, 2024

Fix/37098: In app sound is played if user not viewing chat #37872

Merged

50 tasks

Expensify deleted a comment from melvin-bot bot Mar 8, 2024

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Mar 9, 2024

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 27, 2024

melvin-bot bot changed the title ~~[$500] In-app sounds play in the background and message received sounds play while not viewing the chat~~ [HOLD for payment 2024-04-03] [$500] In-app sounds play in the background and message received sounds play while not viewing the chat Mar 27, 2024

melvin-bot bot removed the Reviewing Has a PR in review label Mar 27, 2024

zanyrenney closed this as completed Apr 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2024-04-03] [$500] In-app sounds play in the background and message received sounds play while not viewing the chat #37098

[HOLD for payment 2024-04-03] [$500] In-app sounds play in the background and message received sounds play while not viewing the chat #37098

m-natarajan commented Feb 22, 2024 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Feb 22, 2024

melvin-bot bot commented Feb 22, 2024

melvin-bot bot commented Feb 22, 2024

rayane-djouah commented Feb 22, 2024 •

edited

Loading

shawnborton commented Feb 22, 2024

arosiclair commented Feb 22, 2024

rayane-djouah commented Feb 22, 2024

This comment was marked as outdated.

kirillzyusko commented Feb 23, 2024

Julesssss commented Feb 23, 2024

shawnborton commented Feb 23, 2024

dukenv0307 commented Feb 24, 2024 •

edited

Loading

abdulrahuman5196 commented Feb 26, 2024

abdulrahuman5196 commented Feb 26, 2024

dukenv0307 commented Feb 26, 2024

arosiclair commented Feb 26, 2024

melvin-bot bot commented Mar 5, 2024

melvin-bot bot commented Mar 5, 2024

dukenv0307 commented Mar 6, 2024

dukenv0307 commented Mar 7, 2024 •

edited

Loading

Julesssss commented Mar 7, 2024 •

edited

Loading

arosiclair commented Mar 7, 2024

Julesssss commented Mar 7, 2024

arosiclair commented Mar 7, 2024

Julesssss commented Mar 7, 2024

dukenv0307 commented Mar 9, 2024

melvin-bot bot commented Mar 27, 2024

melvin-bot bot commented Mar 27, 2024

melvin-bot bot commented Mar 27, 2024

abdulrahuman5196 commented Apr 1, 2024

Julesssss commented Apr 2, 2024

zanyrenney commented Apr 3, 2024

zanyrenney commented Apr 3, 2024

[HOLD for payment 2024-04-03] [$500] In-app sounds play in the background and message received sounds play while not viewing the chat #37098

[HOLD for payment 2024-04-03] [$500] In-app sounds play in the background and message received sounds play while not viewing the chat #37098

Comments

m-natarajan commented Feb 22, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Feb 22, 2024

melvin-bot bot commented Feb 22, 2024

melvin-bot bot commented Feb 22, 2024

rayane-djouah commented Feb 22, 2024 • edited Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

shawnborton commented Feb 22, 2024

arosiclair commented Feb 22, 2024

rayane-djouah commented Feb 22, 2024

Proposal

This comment was marked as outdated.

kirillzyusko commented Feb 23, 2024

Julesssss commented Feb 23, 2024

shawnborton commented Feb 23, 2024

dukenv0307 commented Feb 24, 2024 • edited Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

abdulrahuman5196 commented Feb 26, 2024

abdulrahuman5196 commented Feb 26, 2024

dukenv0307 commented Feb 26, 2024

arosiclair commented Feb 26, 2024

melvin-bot bot commented Mar 5, 2024

melvin-bot bot commented Mar 5, 2024

dukenv0307 commented Mar 6, 2024

dukenv0307 commented Mar 7, 2024 • edited Loading

Julesssss commented Mar 7, 2024 • edited Loading

arosiclair commented Mar 7, 2024

Julesssss commented Mar 7, 2024

arosiclair commented Mar 7, 2024

Julesssss commented Mar 7, 2024

dukenv0307 commented Mar 9, 2024

melvin-bot bot commented Mar 27, 2024

melvin-bot bot commented Mar 27, 2024

melvin-bot bot commented Mar 27, 2024

abdulrahuman5196 commented Apr 1, 2024

Julesssss commented Apr 2, 2024

zanyrenney commented Apr 3, 2024

zanyrenney commented Apr 3, 2024

m-natarajan commented Feb 22, 2024 •

edited by melvin-bot bot

Loading

rayane-djouah commented Feb 22, 2024 •

edited

Loading

dukenv0307 commented Feb 24, 2024 •

edited

Loading

dukenv0307 commented Mar 7, 2024 •

edited

Loading

Julesssss commented Mar 7, 2024 •

edited

Loading