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

[Hold for #27392] [$500] mWeb-Profile-Asia/Calcutta is displayed as Asia/Kolkata & selected timezone not shown with tick mark #27927

Closed
2 of 6 tasks
izarutskaya opened this issue Sep 21, 2023 · 33 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Not a priority Reviewing Has a PR in review

Comments

@izarutskaya
Copy link

izarutskaya commented Sep 21, 2023

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


Action Performed:

Pre-condition: change device time zone to India

  1. Go to https://staging.new.expensify.com/

  2. Tap profile icon

  3. Tap profile

  4. Tap timezone

  5. Toggle on "automatically determine your location" and note Asia/Calcutta is displayed

  6. Toggle off "automatically determine your location"

  7. Tap Asia/Calcutta to note selected option not shown with tick mark on top

  8. Enter Asia

  9. Scroll down to find Asia/Calcutta, then Asia/Kolkata

Expected Result:

When user taps Asia/Calcutta, the option Asia/Calcutta with tick mark should be displayed inside on top. Both automatically determine location and time zone options must show same spelling and not Calcutta/Kolkata and tapping on it, inside the same timezone option must be displayed with tick mark on top.

Actual Result:

When user taps Asia/Calcutta, the option Asia/Calcutta with tick mark is not found on top. When user enters Asia in search and scroll down, the option Asia/Calcutta with tick mark not found, but shown Asia/Kolkata , and that also without tick mark.

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.3.72-6

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

Notes/Photos/Videos: Any additional supporting documentation

Bug6208333_timw.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/~01e2fd5178e62c26f5
  • Upwork Job ID: 1704963311276171264
  • Last Price Increase: 2023-09-28
  • Automatic offers:
    • ntdiary | Reviewer | 27049168
    • tienifr | Contributor | 27049171
@izarutskaya izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 21, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 21, 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

@NicMendonca NicMendonca added the External Added to denote the issue can be worked on by a contributor label Sep 21, 2023
@melvin-bot melvin-bot bot changed the title mWeb-Profile-Asia/Calcutta is displayed as Asia/Kolkata & selected timezone not shown with tick mark [$500] mWeb-Profile-Asia/Calcutta is displayed as Asia/Kolkata & selected timezone not shown with tick mark Sep 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 21, 2023

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

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

melvin-bot bot commented Sep 21, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 21, 2023

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

@pranabs1ngh
Copy link

pranabs1ngh commented Sep 21, 2023

Proposal

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

When user taps Asia/Calcutta, the option Asia/Calcutta with tick mark is not found on top. When user enters Asia in search and scroll down, the option Asia/Calcutta with tick mark not found, but shown Asia/Kolkata , and that also without tick mark

What is the root cause of that problem?

Asia/Calcutta option is missing from timezones list in TIMEZONES.js file.

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

Simply adding this option into the list fixes the issue. When timezone option is selected, after adding this item in the list, it comes preselected with a tickmark which is the expected result of this issue.

@tienifr
Copy link
Contributor

tienifr commented Sep 22, 2023

Proposal

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

Some timezone names have different variations.

What is the root cause of that problem?

This is due to our migration from momentjs to date-fns library recently. The supported timezone list in momentjs is different from our own timezones list. Specifically in this case, it's Asia/Calcuta and Asia/Kolkata. That's Calcuta does not appear in the list and while Kolkata is a different variation, it won't show the selected mark.

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

  1. Add support for all IANA timezones using formatjs (reference). This will be done in [HOLD for payment 2024-01-18] [$250] Dev: IOS - App crashes when opening a transaction report #27392.
  2. Once step 1 is done, Add all IANA timezones to our TIMEZONES list. If we didn't add polyfills for these timezones, after selecting the app might crash/would not recognize the selected timezone.

This is what we have done before removing momentjs, we used moment.tz.names() (i.e., the attached list above) instead of our own list.

What alternative solutions did you explore? (Optional)

Keep the timezone list minimal as currently but allow searching/displaying by alias just like we did with searching for emojis.

@melvin-bot melvin-bot bot added the Overdue label Sep 25, 2023
@ntdiary
Copy link
Contributor

ntdiary commented Sep 25, 2023

@tienifr, I feel like you make a good point, let's wait and see how to standardize the timezone data to thoroughly fix this issue. : )

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Sep 25, 2023
@NicMendonca
Copy link
Contributor

let's wait and see how to standardize the timezone data

@ntdiary is this being worked on elsewhere?

@melvin-bot melvin-bot bot removed the Overdue label Sep 27, 2023
@ntdiary
Copy link
Contributor

ntdiary commented Sep 27, 2023

@ntdiary is this being worked on elsewhere?

  1. Add support for all IANA timezones using formatjs (reference). This will be done in [HOLD for payment 2024-01-18] [$250] Dev: IOS - App crashes when opening a transaction report #27392.

Strictly speaking, not yet. Step 1 in the proposal could potentially be implemented in issue #27392. I personally lean towards this approach, but still need to look further into the timezone data size.

@melvin-bot
Copy link

melvin-bot bot commented Sep 28, 2023

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

@suneox
Copy link
Contributor

suneox commented Sep 29, 2023

The same root cause with this issue so I would like to bring out a proposal for this issue

Proposal

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

mWeb-Profile-Asia/Calcutta is displayed as Asia/Kolkata & selected timezone not shown with tick mark

What is the root cause of that problem?

When a user turns on Automatically determine your location will update a timezone via API UpdateAutomaticTimezone, then turn off and navigate to TimezoneSelectPage the timezoneOptions for selection list parse from import TIMEZONES.js but the list of timezones is missed from

// All timezones were taken from: https://raw.githubusercontent.com/leon-do/Timezones/main/timezone.json

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

We should get data from a timezone database and I have parse List of tz database time zones upload here we can replace for TIMEZONES.js

What alternative solutions did you explore? (Optional)

We will create timezoneBackwardMap from tz_database

Screenshot 2023-09-28 at 01 04 48
const timezoneBackwardMap = {
    'Africa/Accra': 'Africa/Abidjan',
    'Africa/Addis_Ababa': 'Africa/Nairobi',
    .....
};

At function getUserTimezone if timezone not exits on TIMEZONES.ts we will get backward link from timezoneBackwardMap

const getUserTimezone = (currentUserPersonalDetails) => {
    const timezone = lodashGet(currentUserPersonalDetails, 'timezone', CONST.DEFAULT_TIME_ZONE);
    if(TIMEZONES.includes(timezone)) {
        return timezone;
    }
    return timezoneBackwardMap[timezone]
};

Then At function updateAutomaticTimezone also check if timezone not exits on TIMEZONES.ts we will get backward link from timezoneBackwardMap

    const parameters: UpdateAutomaticTimezoneParams = {
        timezone: TIMEZONES.includes(timezone.selected) ? JSON.stringify(timezone) : JSON.stringify({ selected: timezoneBackwardMap[timezone.selected], automatic: timezone.automatic }),
    };

@suneox
Copy link
Contributor

suneox commented Sep 29, 2023

Hi @ntdiary From my proposal we don't need to depend on blockers because the issue #27392 handle for formatjs library, in this issue the list of timezone get direct from TIMEZONES.js. The List of tz database also update to latest you can try the list I have parse at this link or using my alternative solution only save the primary timezone do not save a backward timezone

@tienifr
Copy link
Contributor

tienifr commented Sep 29, 2023

@suneox We're using date-fns library which is a wrapper for Intl and would not support that full list across platforms/devices without the polyfills mentioned in #27392. If we selected one of those timezones, are you sure that date-fns would work, or the app would crash?

@suneox
Copy link
Contributor

suneox commented Sep 29, 2023

@tienifr At TimezoneSelectPage the list of timezone imports direct from TIMEZONES.js and the timezoneOptions for selection list only parse from imported timezones without date-fns, and the function getUserTimezone also only get from API. So this issue can be resolved without waiting for #27392 because the expectation for this issue is "user can see timezone and selection timezone show tick mark"

If we selected one of those timezones, are you sure that date-fns would work, or the app would crash?

My alternative proposal may be able to resolve your issue
If we do not fix this issue at this time issue #27392 still happen because the current TIMEZONES.js also includes a backward timezone link

@melvin-bot melvin-bot bot added the Overdue label Sep 30, 2023
@NicMendonca
Copy link
Contributor

@ntdiary proposals for you here!

@melvin-bot melvin-bot bot removed the Overdue label Oct 2, 2023
@ntdiary
Copy link
Contributor

ntdiary commented Oct 3, 2023

Reviewing now. : )

@melvin-bot melvin-bot bot added the Overdue label Oct 5, 2023
@NicMendonca
Copy link
Contributor

@hayata-suenaga this is waiting on your review: #27927 (comment)

@melvin-bot melvin-bot bot removed the Overdue label Oct 5, 2023
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 6, 2023

📣 @ntdiary 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

@melvin-bot
Copy link

melvin-bot bot commented Oct 6, 2023

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@hayata-suenaga
Copy link
Contributor

sorry for the delay in assignment

@melvin-bot melvin-bot bot added the Overdue label Oct 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

@ntdiary, @NicMendonca, @hayata-suenaga, @tienifr Whoops! This issue is 2 days overdue. Let's get this updated quick!

@hayata-suenaga
Copy link
Contributor

the draft PR is up ⬆️

@tienifr
Copy link
Contributor

tienifr commented Oct 12, 2023

The proposal for this issue has two parts: the first part is to use polyfill to prevent crash which would be fixed via #27392. The mentioned PR above is to fix it. Also, as suggested by C+, let's HOLD this to see if the polyfill approach actually works.

@hayata-suenaga hayata-suenaga changed the title [$500] mWeb-Profile-Asia/Calcutta is displayed as Asia/Kolkata & selected timezone not shown with tick mark [App Issue #27392] [$500] mWeb-Profile-Asia/Calcutta is displayed as Asia/Kolkata & selected timezone not shown with tick mark Oct 15, 2023
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Nov 6, 2023
Copy link

melvin-bot bot commented Nov 6, 2023

This issue has not been updated in over 15 days. @ntdiary, @NicMendonca, @hayata-suenaga, @tienifr 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!

@hayata-suenaga
Copy link
Contributor

@tienifr could you check if the issue was fixed by the other PR?

@situchan
Copy link
Contributor

situchan commented Nov 6, 2023

That PR was reverted due to crash on macOS Sonoma

@hayata-suenaga
Copy link
Contributor

Then should we proceed with the proposed solution?

@situchan
Copy link
Contributor

situchan commented Nov 7, 2023

Actually, I think we can close as #27392 should cover this issue as well.
Otherwise, still hold.

@NicMendonca NicMendonca changed the title [App Issue #27392] [$500] mWeb-Profile-Asia/Calcutta is displayed as Asia/Kolkata & selected timezone not shown with tick mark [Hold for #27392] [$500] mWeb-Profile-Asia/Calcutta is displayed as Asia/Kolkata & selected timezone not shown with tick mark Nov 13, 2023
@melvin-bot melvin-bot bot closed this as completed Jan 3, 2024
Copy link

melvin-bot bot commented Jan 3, 2024

@ntdiary, @NicMendonca, @hayata-suenaga, @tienifr, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

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. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Not a priority Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

8 participants