Skip to content
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

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

Closed
1 of 6 tasks
m-natarajan opened this issue Feb 16, 2024 · 41 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@m-natarajan
Copy link

m-natarajan commented Feb 16, 2024

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


Version Number: 1.4.42-1
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: @quinthar
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1708049588944159

Action Performed:

  1. Enable DND in the android phone
  2. Send a message to the expensify account logged in the phone

Expected Result:

No sound notification

Actual Result:

Sound notification is heard

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

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01861dee3b63d29cfc
  • Upwork Job ID: 1758531931545219072
  • Last Price Increase: 2024-03-01
@m-natarajan 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 16, 2024
@melvin-bot melvin-bot bot changed the title User receiving "new message" notification sound even if the phone is on DND [$500] User receiving "new message" notification sound even if the phone is on DND Feb 16, 2024
Copy link

melvin-bot bot commented Feb 16, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01861dee3b63d29cfc

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

melvin-bot bot commented Feb 16, 2024

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

Copy link

melvin-bot bot commented Feb 16, 2024

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

@m-natarajan
Copy link
Author

@shawnborton tagging you as per this comment

@shawnborton
Copy link
Contributor

cc @Julesssss in case you wanna take this one.

@shahinyan11
Copy link
Contributor

shahinyan11 commented Feb 16, 2024

Proposal

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

User receiving "new message" notification sound even if the phone is on DND

What is the root cause of that problem?

Missing code for this functionality

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

  1. Create Native module to get DND status if device. (I searched but did not find a ready package)
  2. Get DND status with created module and wrap this line in if block

What alternative solutions did you explore? (Optional)

@divyanshk443
Copy link

divyanshk443 commented Feb 17, 2024

Proposal

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

Even if phone is on DND , user is receiving new message notification sound.

What is the root cause of that problem?
Need to make changes to the code that handles the notification sounds, specifically in the playSoundForMessageType function. Current implementation doesn't explicitly handle Do Not Disturb (DND) mode

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

function playSoundForMessageType(pushJSON: OnyxServerUpdate[]) {
    // ... (existing code)

    isDNDActive().then((dndActive) => {
        if (!dndActive) {
            // Only play sounds if not in DND mode
            try {
                // ... (existing sound-playing logic)
            } catch (e) {
                // Handle any errors
                // ...
            }
        }
    });
}

To implement a function that checks whether the Do Not Disturb (DND) mode is active on a mobile device, we need to use platform-specific APIs provided by the operating system.

For example in iOS:
we can use the AVAudioSession class from the AVFoundation framework to check the current audio session category. In iOS, the audio session category will change when the device enters Do Not Disturb (DND) mode.

Here's how we can implement the isDNDActive function for iOS:

const isDNDActive = () => {
    return new Promise((resolve, reject) => {
        const session = AVAudioSession.sharedInstance();
        const currentMode = session.mode;

        if (currentMode === AVAudioSessionModeDoNotDisturb) {
            resolve(true); // DND mode is active
        } else {
            resolve(false); // DND mode is not active
        }
    });
};

For Android:
On Android, we can use the NotificationManager class to check whether Do Not Disturb (DND) mode is active. we'll need to use the getNotificationPolicy method to retrieve the current notification policy and check if the DND mode is enabled.

Here's how we can implement the isDNDActive function for Android:

const isDNDActive = () => {
    return new Promise((resolve, reject) => {
        const notificationManager = getSystemService(Context.NOTIFICATION_SERVICE);
        const currentPolicy = notificationManager.getCurrentNotificationPolicy();

        if (currentPolicy.priorityCategories.includes(NotificationManager.PRIORITY_CATEGORY_ALARMS)) {
            resolve(false); // DND mode is not active
        } else {
            resolve(true); // DND mode is active
        }
    });
};


What alternative solutions did you explore? (Optional)
Null

Copy link

melvin-bot bot commented Feb 17, 2024

📣 @divyanshk443! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@melvin-bot melvin-bot bot added the Overdue label Feb 19, 2024
@Julesssss Julesssss self-assigned this Feb 19, 2024
@melvin-bot melvin-bot bot removed the Overdue label Feb 19, 2024
@Julesssss
Copy link
Contributor

Are we not confusing two things here? DND should block/silence native push notifications, but this sound is occurring from a message received by pusher websocket while the app is alive. It's not a push notification, but an in-app event.

Perhaps we should not play the sound if the app is not in the foreground, but there's an argument that the sound should be played in this case:

  • User has media volume above 0
  • User hasn't disabled sounds within our settings
  • This is an in-app event sound

If pusher triggered a push notification then i would agree with us needing to mute it.

@kirillzyusko
Copy link
Contributor

kirillzyusko commented Feb 19, 2024

If we want to detect silent/dnd mode then we can use https://github.com/hirbod/react-native-volume-manager package (for example)

However from my observation we get push notifications only if app is in background or it's killed. So maybe wrapping the function playSoundForMessageType in if (!isInBackground) would be sufficient to solve the problem?

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Feb 19, 2024

When I mute in Desktop, it doesn't change my settings on Web/Chrome, even after refresh. Is that related to this bug or should I report in #expensify-bugs ?

@pecanoro
Copy link
Contributor

@Julesssss I also experienced this, but there are two weird things going on here.

  1. As you mentioned, no way to disable the sounds to send the message, I tried muting the phone, DND, vibration only and nothing, since there are no in-app settings to mute them, the sound when you send a message will be always played.

  2. My phone has all notifications muted and as David experienced, many times I get the notification sound even if I am not using my phone, so somehow is ignoring the phone settings, not sure how.

@Julesssss
Copy link
Contributor

Hey @pecanoro, so on Android our message received sound is controlled by the media volume setting, which I think makes sense for the message SENT sound.

For message received, this should only be coming from the push notification, which will follow the users DND preference. So removing the logic that plays message received sound on Android/iOS should resolve this.

@pecanoro
Copy link
Contributor

Hey @pecanoro, so on Android our message received sound is controlled by the media volume setting, which I think makes sense for the message SENT sound.

Do you know if we are planning to add the functionality to disable the sounds to the app? It feels pretty weird having to disable media sound in the whole phone just to mute an app.

@Julesssss
Copy link
Contributor

It feels pretty weird having to disable media sound in the whole phone just to mute an app.

I agree, but this is pretty standard AFAIK. Users either disable the sounds, or turn down media volume with the volume hardware buttons.

Also btw there's already a 'Mute all sounds from Expensify' option under preferences.

@kirillzyusko
Copy link
Contributor

When I mute in Desktop, it doesn't change my settings on Web/Chrome, even after refresh. Is that related to this bug or should I report in #expensify-bugs ?

@mallenexpensify I think you describe the same problem as in #36487 right?

@mallenexpensify
Copy link
Contributor

Yes, thanks @kirillzyusko , I wasn't sure if it was expected behaviour. Good to know it's working as expected

@nikihatesgh
Copy link

idk if this has been mentioned yet, but the sound also stops music being played on my iphone (in addition to making the sound when on silent). Other apps have a pause in the music or lower the music to play over it, but this just overrides what I am listing to without resuming afterwwards

@Julesssss
Copy link
Contributor

Ah thanks @nikihatesgh, I added that to the tracking issue

@shawnborton
Copy link
Contributor

Works for me, I'll close this now.

@pecanoro
Copy link
Contributor

pecanoro commented Feb 21, 2024

@Julesssss How is this expected behavior? If my phone is muted, it should not play any notification sounds when I am getting new messages 🤔

@Julesssss
Copy link
Contributor

How is this expected behavior?

@pecanoro I should have made it clearer, but that was in response to @mallenexpensify's account-wide preference, not your issue. Your issue (which I 100% agree with) was due to a faulty implementation that has to be rewritten and is being handed separately here.

We can keep this issue open, but there are lots of different issues being reported here and it's getting confusing. I wanted to close this issue to help clear up our list of tasks.

@Julesssss
Copy link
Contributor

I'll re-open as the PR is about to be merged anyway.

But further comments should only be related to the iOS/Android sound cue playing. Please raise other issues on the parent issue tracker

@Julesssss Julesssss reopened this Feb 21, 2024
Copy link

melvin-bot bot commented Feb 23, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@dylanexpensify
Copy link
Contributor

dylanexpensify commented Feb 26, 2024

Question: does this solution cover notification preferences? E.g. notifications set to Daily vs Instant? Context

@melvin-bot melvin-bot bot added the Overdue label Feb 26, 2024
@Julesssss
Copy link
Contributor

Question: does this solution cover notification preferences? E.g. notifications set to Daily vs Instant? Context

If your issue is just the sound playing, then yes I believe so. Right now we're removing the local sound and tying it to mobile push notifications. Once that is done we'll be looking at the room preference.

@Julesssss
Copy link
Contributor

Wait @dylanexpensify, perhaps this is the issue you are looking for?

@dylanexpensify
Copy link
Contributor

Ahh interesting, it looks like mine is similar, but technically earlier than that one linked! However, that one is further along and mine was reported by Applause, so I think we can close mine in favor of that one.

Copy link

melvin-bot bot commented Feb 27, 2024

@Julesssss, @Ollyws, @muttmuure Eep! 4 days overdue now. Issues have feelings too...

@Julesssss
Copy link
Contributor

@muttmuure @dylanexpensify would you mind retesting on the latest staging build, please? This is fixed for me on Android

@Julesssss
Copy link
Contributor

Just waiting on a review, not overdue

@melvin-bot melvin-bot bot removed the Overdue label Feb 28, 2024
Copy link

melvin-bot bot commented Mar 1, 2024

@Julesssss @Ollyws @muttmuure this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Copy link

melvin-bot bot commented Mar 1, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Mar 1, 2024
@Julesssss
Copy link
Contributor

This should be fixed, though i'm unable to verify. My iPhone doesn't play loud notifications for ANY app.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 1, 2024
@kirillzyusko
Copy link
Contributor

@Julesssss should we ask reporter to verify it again?

@Julesssss
Copy link
Contributor

Hi @m-natarajan, could you please retest on the latest builds? Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Mar 4, 2024
@Julesssss
Copy link
Contributor

Verified fixed on Android:

Screenshot_20240304-130505

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests