[$1000] Settings -Changes "Notifications"/ Priority mode offline aren't saved

[kbecciv]

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

---

Action Performed:

1. Open the app
2. Log in with any account
3. Navigate to Settings > Preferences
4. Disable the internet connection in the device
5. Toggle the Notification option
6. Go back online

Expected Result:

Notifications toggle is still in the same position as it was offline

Actual Result:

Changes "Notifications" made offline aren't saved. The switch returns to its original position.

Workaround:

Unknown

Platform:

Where is this issue occurring?

• iOS
• Mobile Web

Version Number: 1.2.13.3

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

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

Notes/Photos/Videos: Any additional supporting documentation

Bug5774162_2022_10_12_23_49_Img_7908.mp4

2022.10.12.23.41.Img.7907.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c0254925e0e226e4
• Upwork Job ID: 1603370449146945536
• Last Price Increase: 2022-12-15

[melvin-bot]

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

[alex-mechler]

I'm not able to reproduce this on Web, although I'm not on an ios device. If I switch the toggle while offline, it remains in the correct position after going back. Is this only reproducible on iOS @kbecciv?

[kbecciv]

@alex-mechler Checking with team, will update you shortly.

[alex-mechler]

@kbecciv did you hear back from the team?

[kbecciv]

@alex-mechler , we can reproduced in all environments.

RPReplay_Final1665740705.MP4

Recording.2366.mp4

RPReplay_Final1665740705.MP4

Record_2022-10-14-12-50-03.mp4

.

[alex-mechler]

Thanks for checking! Sending this external, since I can't get it to reproduce, I don't think it requires API changes

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[Beamanator]

mooost likely this is due to the fact that Pusher isn't sending messages, due to hitting an API quota limit

[Beamanator]

Putting on hold till we figure out if that's the problem (I can reproduce this error in dev - after coming back online, we didn't get any pusher message from the back-end)

[alexpensify]

For now, I won't take any action and will hold on uploading to Upworks.

[sobitneupane]

@abdulrahuman5196 Thanks for your proposal.

Can you be more specific please. Which are the files you are proposing the changes. Can you link those files and explain what are the changes you are proposing? (Example: #11778 (comment))

[sobitneupane]

@priyeshshah11

the ReconnectApp comes back with the first value which triggers the onChange function

Your proposal might prevent the subsequent requests. But if the ReconnectApp comes back with the previous value, won't it reset the values to the state received in ReconnectApp?

[priyeshshah11]

@priyeshshah11

the ReconnectApp comes back with the first value which triggers the onChange function

Your proposal might prevent the subsequent requests. But if the ReconnectApp comes back with the previous value, won't it reset the values to the state received in ReconnectApp?

@sobitneupane That issue arrises only when multiple requests are sent simultaneously, otherwise ReconnectApp always comes back with the right state (new updated state).

[sobitneupane]

@priyeshshah11
I tested your proposal and it didn't work for me. Still Priority mode changed offline are not being saved.

[priyeshshah11]

@priyeshshah11 I tested your proposal and it didn't work for me. Still Priority mode changed offline are not being saved.

are you able to share a video of the behaviour that you're seeing? I will try to do the same

[priyeshshah11]

@sobitneupane Video of priority mode changed offline working correctly.

Apologies for the blury video, had to reduce it to 480p due to github upload limit. Hopefully, you can still see the behaviour.

Screen.Recording.2022-12-20.at.1.13.03.AM.mov

[sobitneupane]

I was able to reproduce the issue on native devices only not on desktop. So, all my testings and findings are based on native device.

Steps:

1. Open the app
2. Log in with any account
3. Navigate to Settings > Preferences
4. Disable the internet connection in the device
5. Change the priority mode
6. Go back online

Tested on IOS.

RPReplay_Final1671459272.mp4

[priyeshshah11]

@sobitneupane I can reproduce on iOS without my fix but cannot reproduce it with my fix (so I mean to say that it works).

Without fix

not-working.mov

With my fix

working-with-fix.mov

[sobitneupane]

On web, When I change the priority mode quickly for few times (on online), it starts oscillating between the modes.

Screen.Recording.2022-12-19.at.20.30.10.mov

[priyeshshah11]

On web, When I change the priority mode quickly for few times, it starts oscillating between the modes.

Screen.Recording.2022-12-19.at.20.30.10.mov

@sobitneupane The above solution fixes the OG bug, I think "switching between modes quickly several times" is currently showing this odd behaviour due to the fact that the server is running a bit slow right now thus it queues a new request before it has received a response back from the previous one which I think is a very rare edge case i.e How often are users actually going to switch between modes so quickly & frequently along with the server being slow at the same time?

If we still want to fix this particular edge case, then the only option would be to disable the picker until we get a response back, just like we do for SignIn.

[s77rt]

This seems to be another issue related to the Pusher.
I think this needs to be handled internally, at least partly

• The solution should revolve around adding certain fields to the pusher notifications where we use those fields to validate events and ignore those that does not pass the validation e.g. timestamp.
• Another solution is to implement a lock mechanism i.e. a value on onyx should not be modified if it's locked by another "thread" (similarity on how concurrency works on other languages...)

I don't have a proper proposal at the moment, just wanted too share my toughts hopefully we will be heading to the right solution.

---

@priyeshshah11 Your solution won't work if the request has been sent already.

[abdulrahuman5196]

@abdulrahuman5196 Thanks for your proposal.

Can you be more specific please. Which are the files you are proposing the changes. Can you link those files and explain what are the changes you are proposing? (Example: #11778 (comment))

Hi, @sobitneupane my original proposal is here,
#11778 (comment)

For solution
Action item 1

--- a/src/libs/actions/User.js
+++ b/src/libs/actions/User.js
@@ -425,9 +425,16 @@ function updateChatPriorityMode(mode) {
             value: mode,
         },
     ];
+    const successData = [
+        {
+            onyxMethod: CONST.ONYX.METHOD.MERGE,
+            key: ONYXKEYS.NVP_PRIORITY_MODE,
+            value: mode,
+        },
+    ];
     API.write('UpdateChatPriorityMode', {
         value: mode,
-    }, {optimisticData});
+    }, {optimisticData, successData});
 }
 

Action item 2:
This might be a more complex step, To implement API priority we have to add API priority logic in 'SequentialQueue' 'process' function. There we have to sort and pick tasks based on priority. This way we can make sure 'ReconnectApp' or other data refresher APIs are executed after 'UpdateChatPriorityMode'. This would be difficult to provide draft working code.

Action item 3:
I have already noted a different comment on different issue

We can remove older UpdateChatPriorityMode requests from the SequentialQueue when user triggers new UpdateChatPriorityMode API call.
I have seen examples of this similar replace proposals by different people in different places on this repository which could indicate this as a common problem. 
https://github.com/Expensify/App/issues/13521#issuecomment-1347700448

[Beamanator]

Thanks y'all for your conversation and investigation so far! We're planning to regroup internally on the optimal next steps, then we will most likely unlock this when we're ready to move forward. Thanks for everyone's effort so far while looking into possible solutions to this issue

[alexpensify]

@Beamanator - It sounds like this one is on hold. Should we move it to Weekly to give more time for the regroup process?

[PauloGasparSv]

I don't think there is a need for that right now @alexpensify

[JmillsExpensify]

Yeah this is not on hold. This should remain a daily.

[alexpensify]

Thanks for the feedback! I'm going OOO until January 3 and will catch up then to identify if there is any movement here.

[JmillsExpensify]

Cool, I will keep at eye out, as this is one of the oldest issues in the repo without a PR.

[PauloGasparSv]

I think we should change those toggles to be Pattern D or C, so we block the UI when the user goes offline.

This request order issue looks generic enough that it could affect other requests though, so I want some more input on this. I'll throw this on slack and see what the team thinks of it.

[JmillsExpensify]

Based on the discussion in the Slack thread, the title of this issue is incorrect. It seems that we are saving the settings correctly, but due to site slows, the two requests hit two different servers – one of which is behind versus the other – so older data gets served up. Refreshing would correctly show the "correct" data.

Again, based on this discussion, we're closing this issue. If we experience it again during site slows, or come up against backend performance more in the future, we'll re-assess.