Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.53k stars 2.88k forks source link

[HOLD for payment 2024-11-11] [$250] The setting "Mute all sounds from Expensify" keeps turning back off despite turning it on every day. #49087

Open m-natarajan opened 1 month ago

m-natarajan commented 1 month ago

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: 9.0.33-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: @flodnv Slack conversation: https://expensify.slack.com/archives/C05LX9D6E07/p1726129231306989

Action Performed:

  1. Go to Account settings > Preferences
  2. Toggle "Mute all sounds from Expensify" to "ON"
  3. Signout and Singnin

Expected Result:

Preference settings are saved for each platform and remain intact after the user signs out and signs back in.

Actual Result:

Preference settings not saved and toggles back to "OFF"

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/4b4147f8-c7ee-4305-ae1e-a54a3b96c22e

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021834238152691021896
  • Upwork Job ID: 1834238152691021896
  • Last Price Increase: 2024-10-03
  • Automatic offers:
    • jjcoffee | Reviewer | 104324316
    • c3024 | Contributor | 104324317
Issue OwnerCurrent Issue Owner: @isabelastisser
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @johnmlee101 (AutoAssignerNewDotQuality)

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @isabelastisser (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

Nodebrute commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-09-12 14:52:57 UTC.

Proposal

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

The setting "Mute all sounds from Expensify" keeps turning back off despite turning it on every day.

What is the root cause of that problem?

When user mute all sounds we only merge it in onyx and no API calls are made to store the preference in backend https://github.com/Expensify/App/blob/4c424a2b6efec6aef7477adc4e22a1b7460947c5/src/libs/actions/User.ts#L972-L974

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

We should do a api call for setMuteAllSounds just like we do for updateNewsletterSubscription

We can also do the same for usestagingserver

What alternative solutions did you explore? (Optional)

eyramtitiati commented 1 month ago

Problem Statement:

The setting “Mute all sounds from Expensify” keeps turning off, even after being enabled by the user.

Root Cause:

The current implementation merges the isMutedAllSounds only into Onyx (local storage) without making an API call to persist the preference in the backend, causing the setting to reset daily.

Proposed Solution:

We will add an API call in User.ts for thesetMuteAllSounds function, similar to how updateNewsletterSubscription operates. This ensures the setting is stored in the backend, maintaining persistence across app sessions.

Alternative Solutions Explored:

  1. Persist Locally with AsyncStorage: Instead of adding a backend call, we could utilize React Native’s AsyncStorage to persist the mute setting locally across sessions. This would reduce dependency on the backend, making it quicker but potentially less reliable across different devices.
  2. Onyx Sync with Backend at App Load: Another approach is to sync Onyx with the backend on app load to ensure that any changes made to settings (like isMutedAllSounds) are reflected in the backend at regular intervals, reducing the number of API calls but maintaining sync reliability.
melvin-bot[bot] commented 1 month ago

📣 @eyramtitiati! 📣 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>
jjcoffee commented 1 month ago

Discussing on Slack as a similar issue was previously closed as expected.

eyramtitiati commented 1 month ago

Contributor details Your Expensify account email:eyramtitiati@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0182773c2654eddb1f?mp_source=share

melvin-bot[bot] commented 1 month ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

NaveedShaikh78 commented 1 month ago

Proposal

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

The setting "Mute all sounds from Expensify" keeps turning back off despite turning it on every day.

What is the root cause of that problem?

API to update "Mute all sounds from Expensify" is not implemented

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

Need to implement the API and other related changes to respective files app\src\libs\actions\User.ts

function updateIsMutedAllSounds(isMutedAllSounds: boolean) {
    const optimisticData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: ONYXKEYS.USER,
            value: {isMutedAllSounds: isMutedAllSounds},
        },
    ];
    const failureData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: ONYXKEYS.USER,
            value: {isMutedAllSounds: !isMutedAllSounds},
        },
    ];

    const parameters: UpdateIsMutedAllSoundsParams = {isMutedAllSounds};

    API.write(WRITE_COMMANDS.UPDATE_IS_MUTED_ALL_SOUNDS, parameters, {
        optimisticData,
        failureData,
    });
}

What alternative solutions did you explore? (Optional)

jjcoffee commented 1 month ago

Coming from the Slack discussion, the repro steps for this issue are wrong. The current thinking is that it happens when the app is updated, not when signing in/out (though it would also happen there, but that's expected since the setting isn't stored at the account-level).

jjcoffee commented 1 month ago

Bumped on Slack.

c3024 commented 1 month ago

Proposal

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

Muted setting resets on updating app.

What is the root cause of that problem?

After updating the app, I think backend sends ONYXKEYS.RESET_REQUIRED as true. So, this here https://github.com/Expensify/App/blob/b00b9bc8afaa0c359ff7526f4990e81866e8499d/src/libs/actions/App.ts#L80-L102 retains information only those in the keys to preserve and resets the rest.

As muting preference is within ONYXKEYS.USER it gets reset.

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

I tried the following. Then this muting setting does not get reset.

  1. We can move the muting preference to ONYXKEYS.ACCOUNT because it is already one of the keys in the keys to be preserved. (or)
  2. We can add ONYXKEYS.USER to the keys to preserve because it has some other local preferences like clearing focus mode notification etc. so they are not lost when updated, (or)
  3. Extract muting sounds to a separate Onyx key.

    What alternative solutions did you explore? (Optional)

jjcoffee commented 1 month ago

After updating the app, I think backend sends ONYXKEYS.RESET_REQUIRED as true.

@c3024 Thanks for the proposal! It does sound like a plausible RCA, but I'm wondering if this is true. Based on the comment here it sounds more like this should be triggered fairly rarely (the reports are of it happening "2 or 3 times a week"/"every day"):

https://github.com/Expensify/App/blob/19d037b3a900b08de1f8ba2f22624ba445abe3a5/src/ONYXKEYS.ts#L364-L365

Of course it's possible that an "emergency" reset is being triggered more often that we'd think, or even that it's being triggered by mistake on every update.

@johnmlee101 Are you able to check on the BE to see what/if any Onyx keys get set (e.g. RESET_REQUIRED) to require/trigger an update?

jjcoffee commented 1 month ago

Also asking on the PR that introduced RESET_REQUIRED if we'd be safe to add ONYXKEYS_USER to the list as @c3024 suggests.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

@johnmlee101, @jjcoffee, @isabelastisser Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

jjcoffee commented 1 month ago

Got confirmation from the actual original source of those KEYS_TO_PRESERVE that it's most likely okay to add ONYXKEYS.USER there.

I think we can just try out the proposal and see if it helps. It's a bit hard to judge since we don't have reliable repro steps, but it sounds plausible enough!

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @tgolen, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

tgolen commented 1 month ago

I think that we shouldn't be adding any more keys to KEYS_TO_PRESERVE. That feels like it's only working around the problem and there is not really any current guidance for what keys should be put into KEYS_TO_PRESERVE. What is to stop us from ending up with every single Onyx key listed there eventually?

Isn't the real problem that the backend is not sending the correct setting to the frontend after updating?

jjcoffee commented 1 month ago

@tgolen The problem is we don't know for sure if it's an update that causes the issue, we just know that having the RESET_REQUIRED flag set results in the reported behaviour so it seems pretty likely that it's related. Are you able to check if resetRequired is somewhere in the BE code for updates?

My reasoning with adding the user key was that we already have a bunch of preferences in KEYS_TO_PRESERVE and user looks like it also mostly contains params that we wouldn't necessarily want/need to reset.

{
    "isUsingExpensifyCard": false,
    "isFromPublicDomain": true,
    "isSubscribedToNewsletter": true,
    "validated": true,
    "isMutedAllSounds": true
}
tgolen commented 1 month ago

@marcaaron it looks like you might be more familiar with the resetRequired Onyx key. I can't find any reference to it in our backend code, so I am not sure how that logic is supposed to work. How does the resetRequired key get sent from the backend? Also, what is your opinion on the proposal here to add the user key to KEYS_TO_PRESERVE so some of the flags there are not reset?

marcaaron commented 1 month ago

AFAICT that is unimplemented in the backend. It seems possible we've jumped to a wrong conclusion. Neither Auth nor Web-Expensify mention anything about this key.

dangrous commented 1 month ago

Mute settings is fully frontend right now, there's no backend saved setting. So it's fine if it's per device, etc. but the goal would be a way to keep it in there on the same device with no log out.

Alternately we could add it into the backend, but I'm assuming (?) there's a reason we didn't build it that way in the first place?

tgolen commented 1 month ago

Thanks, Marc. I would agree. @jjcoffee for now, I'd like to avoid accepting any proposal without a clear and detailed root cause. It can't be resetRequired that's causing this (since the backend is not doing anything with that), so maybe we should look more closely at trying to figure out how it's reproducible. The original description says it can be reproduced via signout/signin, which seems like the best place to start. I think we should also be looking at this as a parallel to the setting for using the staging sever. Is the staging server setting stored on the backend and is why it persists between signins (I don't think so because I don't see any network requests when I toggle the setting)?

marcaaron commented 1 month ago

Also, what is your opinion on the proposal here to add the user key to KEYS_TO_PRESERVE so some of the flags there are not reset?

To quickly respond to this question. I think it seems fine. Here's all the stuff we use it for. So, we can look at the situations where the KEYS_TO_PRESERVE are used and decide if any of the things in the user key would be bad to persist (took a quick look and nothing stood out).

However, not sure that's the right direction for this issue yet.

jjcoffee commented 1 month ago

The original description says it can be reproduced via signout/signin, which seems like the best place to start.

The reports are of it happening whilst remaining signed in; signout/signin would only repro the issue since the setting is not persisted via the BE.

We had some discussion on Slack previously and agreed to keep this issue focused on the behaviour whilst signed in, leaving the question of whether or not to persist the preference for future discussion (if any!).

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

@tgolen @johnmlee101 @jjcoffee @isabelastisser 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!

isabelastisser commented 1 month ago

@jjcoffee, are we waiting for an updated proposal to address what has been decided here?

jjcoffee commented 1 month ago

@isabelastisser Yes that's right! I've bumped on Slack now.

melvin-bot[bot] commented 1 month ago

@tgolen, @johnmlee101, @jjcoffee, @isabelastisser Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

mallenexpensify commented 1 month ago

@rushatgabhane @MonilBhavsar I created another dupe issue I'm closing. 👀 here in case you have feedback and/or ideas. Thx

jjcoffee commented 1 month ago

@mallenexpensify If we decide that we want the setting to persist across sign out/sign in it would also fix this issue, but the actual bug reported here is without signing in (i.e. the setting just seems to randomly reset).

MonilBhavsar commented 1 month ago

I think Matt faces the similar issue - the mute setting is reset after signout/signin. Agree with the root cause being discussed. I see we store deviceID in the Onyx DB. Have we considered of saving the mute setting at the server side and returning to the device making request via API response?

coleaeason commented 1 month ago

Have we considered of saving the mute setting at the server side and returning to the device making request via API response?

+1000, I've said this for months. The current implementation leads to the toggle being turned back on randomly all the time. I don't see why we would not persist this.

dangrous commented 1 month ago

Yeah I think it might be easiest to save it in the server as setting/deviceID pairs, then we could still have different preferences for different platforms/devices. If we feel like that would be too much of a lift we should probably at least preserve the setting on an OpenApp/ReconnectApp call (which is I assume what's happening? On a reauthentication it's probably getting wiped?)

tgolen commented 1 month ago

Yes, I think we should do this right and save it to the server. I think the only thing we need to decide is if we want to allow the setting to be set for a specific device/platform or across all devices or platforms. I think I would prefer doing it for each specific platform.

mallenexpensify commented 1 month ago

I think I would prefer doing it for each specific platform.

I think this is the correct way to do it. Since it's def possible that people want sounds on some platforms and not others.

MonilBhavsar commented 1 month ago

Platforms specific sounds good too

tgolen commented 1 month ago

Great, what's the plan to move it to a server setting then and who wants to do it?

melvin-bot[bot] commented 1 month ago

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

mallenexpensify commented 1 month ago

I updated the Expected behaviour to inc. the bold text below

Preference settings are saved for each platform and remain intact after the user signs out and signs back in.

Do we expect to only need one PR to fix the bug or will it be multiple? Asking cuz we might want to increase the price of the job.

c3024 commented 1 month ago

I think one PR should be enough for the frontend part. We can change isMutedAllSounds from a boolean to an object with fields, using each platform name as the key and a mute setting boolean as its value.

We can create a WRITE_COMMAND for updating this setting.

melvin-bot[bot] commented 1 month ago

@tgolen, @johnmlee101, @jjcoffee, @isabelastisser Whoops! This issue is 2 days overdue. Let's get this updated quick!

jjcoffee commented 1 month ago

Do we want a contributor to work on the FE part for this?

tgolen commented 1 month ago

I'll work on the backend part of this, and I think yes, I do want a contributor to work on the frontend part of it.

I need to get caught up on a few things this morning, then do a little research on how this will work exactly, then I'll come back here and try to post a plan for how best to move forward and what all the specifics will be (name of API command, Onyx keys, platform values, etc.).

tgolen commented 1 month ago

Proposal

New Command for Muting a Platform

New Command for Unmuting a Platform

How does that look to everyone? Is that all you need to do a frontend implementation?

tgolen commented 1 month ago

Hm, should we also consider the platform mobileweb? I think, maybe yes?