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

[$4000] IOS - Timezone - Toggle switch back to online results after enabled connection #15641

Closed
1 of 6 tasks
kbecciv opened this issue Mar 3, 2023 · 50 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

@kbecciv
Copy link

kbecciv commented Mar 3, 2023

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. Go to staging.new.expensify.com
  2. Log in with gmail account
  3. Go to Settings -> Profile -> Timezone.
  4. Toggle OFF for "Automatically determine your location"
  5. Select any timezone
  6. Disable internet Connection
  7. Toggle ON the for "Automatically determine your location"
  8. Turn on the internet connection

Expected Result:

Toggle "Automatically determine your location" should not switch back after recovering internet connection

Actual Result:

Toggle "Automatically determine your location" switches back after recovering internet connection

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.2.78.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

RPReplay_Final1677856847.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/~01aa6eaf96fa7c4cfe
  • Upwork Job ID: 1631759138934312960
  • Last Price Increase: 2023-03-24
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 3, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Mar 3, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

MelvinBot commented Mar 3, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@abekkala abekkala added the External Added to denote the issue can be worked on by a contributor label Mar 3, 2023
@melvin-bot melvin-bot bot unlocked this conversation Mar 3, 2023
@melvin-bot melvin-bot bot changed the title IOS - Timezone - Toggle switch back to online results after enabled connection [$1000] IOS - Timezone - Toggle switch back to online results after enabled connection Mar 3, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

Current assignee @abekkala is eligible for the External assigner, not assigning anyone new.

@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 3, 2023
@MelvinBot
Copy link

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

@osofus
Copy link

osofus commented Mar 3, 2023

I don't understand why this is considered a bug.
If the "automatic time zone" option was enabled during offline mode, why should it get disabled when internet is turned on, just because it was turned off when the app was last online?
The latest state of that option was "automatic" and hence it should be automatic whether or not there's internet connection, unless I am misunderstanding the concern.

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Mar 3, 2023

I think the expected/actual results are poorly described, or at least I don't understand when I read this: "should not switch back to online results". Anyway, from looking at the video, I understand that the problem is that it doesn't keep the settings the user chose while offline, the setting reverts back to detecting automatically the timezone.

@osofus thanks for having a look. I think we may consider it a bug because: The user changes its settings from X to Y, it should stay in Y no matter the connection status. We shouldn't force back on the user to have automatic detection of timezone just because they regained connection.

@osofus
Copy link

osofus commented Mar 3, 2023

Thanks @aldo-expensify, when I watched the video again till the end I understood the problem.
The app gets the status when it was last online and switches to manual time zone briefly before switching back to automatic.

@aldo-expensify
Copy link
Contributor

Yep, I think we should expect that the setting should not change at all when going offline/online/offline/etc

@orkunkarakus
Copy link
Contributor

orkunkarakus commented Mar 4, 2023

Proposal

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

"Automatically Determine Time Zone" switch should not be available in offline mode

What is the root cause of that problem?

Root cause is network status not check in timezone page

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

We need the check network status in timezone page and make switch disabled while network goes offline.

Add "withNetwork" to compose and add networkPropTypes to propTypes

export default compose(
    withLocalize,
    withNetwork(),
    withCurrentUserPersonalDetails,
)(TimezoneInitialPage);

Change switch component isOn prop like this. (If network goes offline then make switch off. We will eliminate onToggle too)
(Or we add disable feature to base switch component.)

<Switch
     isOn={timezone.automatic && !props.network.isOffline} // check network here
     onToggle={updateAutomaticTimezone}
/>

Test Videos

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-03-04.at.12.13.55.mp4

Please watch switch component with the offline status at bottom left !!

@allroundexperts
Copy link
Contributor

allroundexperts commented Mar 4, 2023

Proposal

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

Timezone selection set when network is offline resets when network is back online.

What is the root cause of that problem?

The root cause of this issue is that while offline, network requests for setting timezone fail. Since we have not defined any failureData, the selection is switched back when the connection becomes available again.

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

We need to add the following here and here.

optimisticData: [{
            onyxMethod: CONST.ONYX.METHOD.MERGE,
            key: ONYXKEYS.PERSONAL_DETAILS,
            value: {
                [currentUserEmail]: {
                    timezone,
                    pendingFields: {
                        timezone: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
                    },
                },
            },
        }],
        successData: [{
            onyxMethod: CONST.ONYX.METHOD.MERGE,
            key: ONYXKEYS.PERSONAL_DETAILS,
            value: {
                [currentUserEmail]: {
                    pendingFields: {
                        timezone: null,
                    },
                },
            },
        }],
        failureData: [
            {
                onyxMethod: CONST.ONYX.METHOD.MERGE,
                key: ONYXKEYS.PERSONAL_DETAILS,
                value: {
                    [currentUserEmail]: {
                        timezone: personalDetails[currentUserEmail].timezone,
                        pendingFields: {
                            timezone: null,
                        },
                    },
                },
            },
        ],

For the optimistic data defined above, we can add timezone in the pending section so that we know which field is pending right now. In our UI, we can then show an indication that timezone field is pending update. Once the network is back, we can remove timezone from pending fields. This is inline with how it's done for Avatar upload.

What alternative solutions did you explore? (Optional)

None

@melvin-bot melvin-bot bot added the Overdue label Mar 6, 2023
@aldo-expensify
Copy link
Contributor

No updates on my side, I'll give some time for @mananjadhav to have a first look at the proposals

@melvin-bot melvin-bot bot removed the Overdue label Mar 6, 2023
@mananjadhav
Copy link
Collaborator

I'll review the proposals today.

@abekkala
Copy link
Contributor

abekkala commented Mar 8, 2023

@mananjadhav have you had a moment to review?

@tienifr
Copy link
Contributor

tienifr commented Mar 10, 2023

I think the root cause and the solution for this issue are the same as that I mentioned in this proposal

@melvin-bot melvin-bot bot changed the title [$1000] IOS - Timezone - Toggle switch back to online results after enabled connection [$2000] IOS - Timezone - Toggle switch back to online results after enabled connection Mar 10, 2023
@melvin-bot melvin-bot bot added the Overdue label Mar 10, 2023
@melvin-bot melvin-bot bot added the Overdue label Mar 23, 2023
@abekkala abekkala removed their assignment Mar 23, 2023
@melvin-bot melvin-bot bot removed the Overdue label Mar 23, 2023
@abekkala abekkala added Overdue Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Mar 23, 2023
@MelvinBot
Copy link

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

@MelvinBot

This comment was marked as duplicate.

@abekkala
Copy link
Contributor

abekkala commented Mar 23, 2023

@flaviadefaria
I'm ooo until Tue April 04 so I'm reassigning this.

Current Status:

  • Upwork job is posted
  • Discussion is occurring irt Proposals - A proposal has not been accepted yet, no one has been assigned

when it's time for payments:
Issue reported by: Applause - Internal Team (no payment will be needed)
Fixer: TBD (no proposal has been accepted yet)
Reviewed PR: will be done by @mananjadhav

@aldo-expensify
Copy link
Contributor

@mananjadhav Isn't the code using the Pattern A already?

@allroundexperts yes, I believe that is the case. We don't want to change the pattern, we just want to fix the settings changing the value when we come back online. This seems to only happen in IOUs devices (not the emulator), and I don't have one to debug this further.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 23, 2023
@aldo-expensify
Copy link
Contributor

No overdue!

@melvin-bot melvin-bot bot removed the Overdue label Mar 24, 2023
@aldo-expensify aldo-expensify changed the title [HOLD #12775][$2000] IOS - Timezone - Toggle switch back to online results after enabled connection [$2000] IOS - Timezone - Toggle switch back to online results after enabled connection Mar 24, 2023
@aldo-expensify
Copy link
Contributor

I'll remove the HOLD because there is something to fix (maybe) not related to the replay effect (see this)

@melvin-bot melvin-bot bot changed the title [$2000] IOS - Timezone - Toggle switch back to online results after enabled connection [$4000] IOS - Timezone - Toggle switch back to online results after enabled connection Mar 24, 2023
@MelvinBot
Copy link

@mananjadhav @flaviadefaria @aldo-expensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

@hoangzinh
Copy link
Contributor

Hey @aldo-expensify , I think this bug has been solved by changes from this PR https://github.com/Expensify/App/pull/16093/files#diff-61e10fef9d79a42e783dbaeb95dc420f23738d0d64c8f7d242208b9e36d54076R23-R27

In short, we sort requests and let CONST.NETWORK.COMMAND.RECONNECT_APP to the last.

The only last bug I found is toggle 2 times => it shows incorrect "automatic timezone" setting until we refresh whole page

@melvin-bot melvin-bot bot added the Overdue label Mar 26, 2023
@aldo-expensify
Copy link
Contributor

Thanks for pointing that out @hoangzinh .

@mananjadhav can you check if this is still reproduce for you in IOS?

@melvin-bot melvin-bot bot removed the Overdue label Mar 27, 2023
@aldo-expensify
Copy link
Contributor

The only last bug I found is toggle 2 times => it shows incorrect "automatic timezone" setting until we refresh whole page

@hoangzinh toggle it 2 times while offline and then go online?

@hoangzinh
Copy link
Contributor

yes it is. let me attach the recording on my end

Screen.Recording.2023-03-27.at.22.40.05-1080.mp4

@mananjadhav
Copy link
Collaborator

@aldo-expensify I am not able to reproduce the original bug. But I am able to reproduce the bug @hoangzinh reported.

@priyeshshah11
Copy link
Contributor

priyeshshah11 commented Mar 29, 2023

@mananjadhav This is due to the request/response timing issues. The response from the ReconnectApp command is received before the second request is completed in the backend. So if you refresh the app again you'll get the right value back for the timezone. In one of the previous issues I had recommended to only send one request (the final one) to the server for all toggle-able actions that are added while being offline. I would recommend to do the same here.

Edit: The other option is to call the ReconnectApp again after the last queued request is completed but that will still cause the flicker behaviour.

@hoangzinh
Copy link
Contributor

my point of view is we can close this GH issue because the original bug is solved (or not able to reproduce again with latest Expensify version). I can help to report this new issue base on my finding so we have a fresh GH issue and get more proposals

But if we think we should fix this new bug here. I will send my proposal here too.

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Mar 29, 2023

I'll just put it back on hold and we can track it in the issue tracking all issues related to triggering an action multiple times while offline.

I'll just close this because it is true that the original bug reported was fixed, and I don't think it is right to modify this issue's description to make it about a different bug. The "replay problem" is being worked here: #12775

Thanks again @hoangzinh for finding out it was fixed.

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