[$250] Don't set the user's timezone if it's empty

puneetlath commented 3 months ago

It seems like there are some cases where the user's local timezone can be empty. In those cases, if the user has "automatic" set for their timezone preferences, we will call the API with the empty timezone. We recently had an employee experience this while using airplane wifi. Coming from the internal issue:

Although, the ideal solution would be to figure out if automatic timezone selection can ever set the selected timezone to empty. I feel like the ideal experience should be no and it should continue to use the previous timezone until a new timezone is obtained. So with an example, if carlos was flying from iceland to new york, while in flight instead of automatic timezone clearing the timezone it should keep it as iceland throughout the flight instead of setting it empty. So i believe the code change would be here i.e., to if (timezone?.automatic && timezone?.selected !== currentTimezone && currentTimezone !== '') {. I haven't tested nor do i know how to reproduce the issue (i can attempt to reproduce the issue during my flight to savannah 😛). Let me know your thoughts?

Let's try to:

understand what the scenarios are when the currentTimezone would be empty
not call the server to update the timezone in that scenario and instead leave it as-is

cc @rafecolton @chiragsalian

Issue Owner

Current Issue Owner: @

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021841911357137490129
Upwork Job ID: 1841911357137490129
Last Price Increase: 2024-10-03

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

Current assignee @puneetlath is eligible for the Bug assigner, not assigning anyone new.

FitseTLT commented 3 months ago

Proposal

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

Don't set the user's timezone if it's empty

What is the root cause of that problem?

We updateAutomaticTimezone if there is a difference between the local timezone and user preferences if automatic is set without checking if the local timezone is not empty https://github.com/Expensify/App/blob/8b947e3d8d91af218e9d8532c171bd283ecd85f8/src/libs/Navigation/AppNavigator/AuthScreens.tsx#L142

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

We need to add a check that currentTimezone is not empty

 if(timezone?.automatic && timezone?.selected !== currentTimezone && !!currentTimezone)

What alternative solutions did you explore? (Optional)

github-actions[bot] commented 3 months ago

Elliot-Alderson504 Your proposal will be dismissed because you did not follow the proposal template.

daledah commented 3 months ago

Edited by proposal-police: This proposal was edited at 2024-08-15 02:04:16 UTC.

Proposal

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

What is the root cause of that problem?

As mentioned in the OP, Intl.DateTimeFormat().resolvedOptions().timeZone can be empty. That leads to if the user has "automatic" set for their timezone preferences, we will call the API with the empty timezone in: https://github.com/Expensify/App/blob/8b947e3d8d91af218e9d8532c171bd283ecd85f8/src/libs/Navigation/AppNavigator/AuthScreens.tsx#L144 and https://github.com/Expensify/App/blob/8b947e3d8d91af218e9d8532c171bd283ecd85f8/src/pages/settings/Profile/TimezoneInitialPage.tsx#L31

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

We need to make sure we don't call the server to update the timezone if Intl.DateTimeFormat().resolvedOptions().timeZone is empty and instead leave it as-is.
So we need to update: https://github.com/Expensify/App/blob/8b947e3d8d91af218e9d8532c171bd283ecd85f8/src/libs/Navigation/AppNavigator/AuthScreens.tsx#L142 to:
```
    if (!isEmpty(currentTimezone) && timezone?.automatic && timezone?.selected !== currentTimezone) {
```

And https://github.com/Expensify/App/blob/8b947e3d8d91af218e9d8532c171bd283ecd85f8/src/pages/settings/Profile/TimezoneInitialPage.tsx#L33 to:

        selected: isAutomatic && !isEmpty(currentTimezone) ? currentTimezone : timezone.selected,

with:

    const currentTimezone = Intl.DateTimeFormat().resolvedOptions().timeZone as SelectedTimezone;

What alternative solutions did you explore? (Optional)

We can early return in here: https://github.com/Expensify/App/blob/8b947e3d8d91af218e9d8532c171bd283ecd85f8/src/libs/actions/PersonalDetails.ts#L207 if formatedTimezone.automatic && isEmpty(formatedTimezone.selected)

daledah commented 3 months ago

Proposal Updated

wildan-m commented 3 months ago

Proposal

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

Do not assign a timezone to the user if it is empty.

What is the root cause of that problem?

Intl.DateTimeFormat().resolvedOptions().timeZone can return empty timezone depending on each platform implementation, it might also specific browser bug e.g. this chrome issue.

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

In updateAutomaticTimezone function, early return.

src/libs/actions/PersonalDetails.ts

function updateAutomaticTimezone(timezone: Timezone) {
    if (!currentUserAccountID) {
        return;
    }

    if(isEmpty(timezone) || isEmpty(timezone.selected)) {
        return;
    }

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 2 months ago

@puneetlath, @chiragsalian, @abdulrahuman5196 Huh... This is 4 days overdue. Who can take care of this?

puneetlath commented 2 months ago

@abdulrahuman5196 thoughts on the proposals?

abdulrahuman5196 commented 2 months ago

Checking now

abdulrahuman5196 commented 2 months ago

Contributors, I don't see solid information on the first point in all the proposals.

understand what the scenarios are when the currentTimezone would be empty

Furthermore currentTimezone is not optional value and doesn't have empty options - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/resolvedOptions#timezone.

Could you folks kindly check and update the proposal on Then why are we getting empty value? Or is there some other issue?

And @puneetlath Do we have any way to repro this issue normally?

puneetlath commented 2 months ago

The person who experienced it was on airplane wifi when they experienced it. I don't know a way to reliably reproduce it.

melvin-bot[bot] commented 2 months ago

@puneetlath, @chiragsalian, @abdulrahuman5196 Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 2 months ago

@puneetlath @chiragsalian @abdulrahuman5196 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!

melvin-bot[bot] commented 2 months ago

@puneetlath, @chiragsalian, @abdulrahuman5196 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

chiragsalian commented 2 months ago

Not overdue, waiting for proposals. Demoting to weekly.

abdulrahuman5196 commented 2 months ago

@puneetlath I was unable to repro the issue. Seems it would be hard to repro. i don't see a solid reproducibility/RCA from the proposals as well.

But we can pro-actively, check if the current timezone is empty and skip server call as multiple proposals here suggested and hope it works. What is your suggestion? @puneetlath / @chiragsalian

puneetlath commented 2 months ago

That seems reasonable to me. What do you think @chiragsalian?

chiragsalian commented 2 months ago

Yeah that makes sense to me. Lets do that.

puneetlath commented 2 months ago

Whose solution do you recommend @abdulrahuman5196?

abdulrahuman5196 commented 2 months ago

Checking now

abdulrahuman5196 commented 2 months ago

@daledah 's proposal here https://github.com/Expensify/App/issues/47452#issuecomment-2290356632 looks good. Though it was 2nd proposal here, it covered more cases and the core solution is already available as part of the issue OP so there is not much to copy from other proposals.

🎀 👀 🎀 C+ Reviewed

melvin-bot[bot] commented 2 months ago

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

Beamanator commented 2 months ago

too many engineers assigned, i'll unassign 😅

puneetlath commented 2 months ago

@chiragsalian you're the CME here, so I'll let you decide whether to go forward with the recommended proposal.

chiragsalian commented 2 months ago

Proposal LGTM, feel free to create the PR @daledah.

daledah commented 2 months ago

@abdulrahuman5196 PR is ready.

melvin-bot[bot] commented 1 month ago

This issue has not been updated in over 15 days. @puneetlath, @Beamanator, @chiragsalian, @abdulrahuman5196, @daledah eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

daledah commented 1 month ago

PR has been on prod since 18/9/2024

abdulrahuman5196 commented 1 month ago

@chiragsalian / @puneetlath Seems the automation had failed here. The PR has been in production for more than a week.

The issue should be due for payment.

abdulrahuman5196 commented 1 month ago

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.

Not a consistently reproducible bug, so regression test wont be useful here.

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @strepanier03 (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/~021841911357137490129

melvin-bot[bot] commented 1 month ago

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

mallenexpensify commented 1 month ago

Contributor: @daledah owed $250 via Upwork Contributor+: @abdulrahuman5196 due $250 via NewDot

@daledah , can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~021841911357137490129

Not a consistently reproducible bug,

@abdulrahuman5196 , can you expand on this? We have an option to test monthly for edgecase bugs, which we might want to do here.

abdulrahuman5196 commented 1 month ago

can you expand on this? We have an option to test monthly for edgecase bugs, which we might want to do here.

@mallenexpensify This would be fine. I think the test case would already be in regression test. Anyways added the test steps if required.

1) Go to Profile > Timezone 2) Switch to manual timezone 3) Select another timezone 4) Switch back to automatic timezone 5) Verify that: The timezone is updated, no error appears

JmillsExpensify commented 1 month ago

$250 approved for @abdulrahuman5196

daledah commented 1 month ago

@daledah , can you please accept the job and reply here once you have?

@mallenexpensify I accepted, thanks 🙏

puneetlath commented 1 month ago

Thanks for stepping in here @mallenexpensify. I've gone ahead and paid @daledah so we're good to go here. Thanks everyone!

Expensify / App