[HOLD for payment 2023-04-14] [$1000] Selected timezone option is not scrolled initially for the timezones list #16905

mountiny · 2023-04-04T11:25:29Z

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:

Break down in numbered steps

Goto profile page
Select Timezone

Expected Result:

Describe what you think should've happened

The selected values should be scrolled to and visible on the list.

Actual Result:

Describe what actually happened

Observe the selected value is not scrolled and visible directly on screen on opening the list

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

User has to scroll to it

Platforms:

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

Version Number: Staging(v1.2.90-4)
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
Expensify/Expensify Issue URL:
Issue reported by: @Pujan92
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1679926798768949

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~015b1ab2dc02fa1f2c
Upwork Job ID: 1643213233754124288
Last Price Increase: 2023-04-04

MelvinBot · 2023-04-04T11:25:34Z

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

MelvinBot · 2023-04-04T11:25:38Z

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

MelvinBot · 2023-04-04T11:25:41Z

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

MelvinBot · 2023-04-04T11:25:45Z

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

MelvinBot · 2023-04-04T11:25:54Z

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

MelvinBot · 2023-04-04T11:25:58Z

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

Pujan92 · 2023-04-04T11:28:20Z

Proposal

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

Selected timezone option isn't get scrolled initially

What is the root cause of that problem?

We are not setting the initiallyFocusedOptionKey correctly which is causing this issue.

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

App/src/pages/settings/Profile/TimezoneSelectPage.js

Line 142 in 10d3d52

initiallyFocusedOptionKey={this.currentSelectedTimezone}

this.currentSelectedTimezone should be replaced with this.timezone.selected
App/src/pages/settings/Profile/TimezoneSelectPage.js

Lines 74 to 76 in 10d3d52

getKey(text) {

return `${text}-${(new Date()).getTime()}`;

}

We can get rid of the function getKey in TimezoneSelectPage and pass the text for keyForList like the way it was earlier.

What alternative solutions did you explore? (Optional)

I am not sure why this(getKey) is required but if it remains then we need to consider text here instead of keyForList

App/src/components/OptionsSelector/BaseOptionsSelector.js

Line 159 in 10d3d52

    
           const indexOfInitiallyFocusedOption = _.findIndex(allOptions, option => option.keyForList === this.props.initiallyFocusedOptionKey);

Screen.Recording.2023-03-24.at.8.13.09.PM.mov

mountiny · 2023-04-04T11:43:14Z

@sobitneupane Let me know what you think of the proposal

alexxxwork · 2023-04-04T11:48:43Z

Proposal

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

Current timezone option isn't selected initially

What is the root cause of that problem?

We have a mssing variable here

App/src/pages/settings/Profile/TimezoneSelectPage.js

Line 142 in 10d3d52

initiallyFocusedOptionKey={this.currentSelectedTimezone}

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

we should replace it with this.state.timezoneOptions.filter(tz => tz.text === this.timezone.selected)[0]?.keyForList

situchan · 2023-04-04T12:09:27Z

Proposal

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

Current timezone user selected is not initially visible when enter Timezone page

What is the root cause of that problem?

this.currentSelectedTimezone was old value before #15176 and now deprecated.
The PR author forgot to replace this value with the new one they introduced.

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

It seems not safe to remove date suffix in key (reason: #15176 (comment))
We can simply define and reuse this.currentSelectedTimezone when get all timezones list similar to what I did in #16872.

App/src/pages/settings/Profile/TimezoneSelectPage.js

Lines 83 to 87 in df10eeb

    
           getTimezoneOption(text) { 
        
               return { 
        
                   text, 
        
                   keyForList: this.getKey(text),

Implementation detail:

define keyForList as this.getKey(text) at the beginning of getTimezoneOption(text) function
if text is this.timezone.selected, then define this.currentSelectedTimezone variable and set keyForList
when return json data, use keyForList already defined instead of this.getKey(text) which is redundant

That's all! And no need to change this line:

App/src/pages/settings/Profile/TimezoneSelectPage.js

Line 142 in 10d3d52

initiallyFocusedOptionKey={this.currentSelectedTimezone}

sobitneupane · 2023-04-05T06:17:04Z

Thanks for the proposal @Pujan92.

We can get rid of the function getKey in TimezoneSelectPage and pass the text for keyForList like the way it was earlier.

Please do include the reference when you are talking about the earlier version.

Looks like we might require getKey(#15176 (comment))

I am not sure why this(getKey) is required but if it remains then we need to consider text here instead of keyForList

I am not sure I understand what you mean to say by above statement.

sobitneupane · 2023-04-05T06:20:46Z

Thanks for the proposal @alexxxwork.

Please try to be more descriptive in your proposal. Proposal from @alexxxwork looks good to me.

🎀👀🎀 C+ reviewed

cc:@mountiny

situchan · 2023-04-05T06:30:03Z

Adding my cents here:
@alexxxwork's solution also works but filter operation is triggered every time re-render, i.e. when search value changes.
As we need it only first time render, my solution is more performant.

alexxxwork · 2023-04-05T06:35:46Z

@situchan I think there's no too much difference on this data volumes and we could just use more clear approach.

sobitneupane · 2023-04-05T06:49:33Z

but filter operation is triggered every time re-render

@situchan Yes, I chose the proposal over yours because of the same reason. With your proposal if I select a timezone and scroll and go back. It won't scroll me back to the selected timezone.

situchan · 2023-04-05T06:55:45Z

With your proposal if I select a timezone and scroll and go back. It won't scroll be back to the selected timezone.

@sobitneupane Sorry, can you please share video what that means? Just asking because if any issue with that solution, going to fix in #16872

sobitneupane · 2023-04-05T06:58:34Z

Screen.Recording.2023-04-05.at.12.41.43.mov

situchan · 2023-04-05T07:11:18Z

ok so that's the case when navigate to Timezone page directly and navigation is reversed.
This also happens with this solution 👇

we should replace it with this.state.timezoneOptions.filter(tz => tz.text === this.timezone.selected)[0]?.keyForList

This still doesn't fix your video issue because component is not re-mounted after navigating back.
Not sure if this is worth fixing.

MelvinBot · 2023-04-05T07:11:21Z

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

Pujan92 · 2023-04-05T12:43:33Z

Looks like we might require getKey(#15176 (comment))

@sobitneupane I mean I tried without the getKey part and was not able to reproduce the issue #15176 (comment) which mentioned by @mollfpr. @priyeshshah11 can you plz confirm whether getKey seems required or not?
My thought in the proposal is to straight forward pass the selected value with this.timezone.selected instead of traversing the array to find the selected keyForList.

Screencast without using getKey

Simulator.Screen.Recording.-.iPhone.14.-.2023-04-05.at.18.18.29.mp4

Pujan92 · 2023-04-06T05:41:13Z

Thanks @priyeshshah11 for the response, @mollfpr could you plz help by trying this proposal where I am suggesting to remove getKey function which earlier was added to solve your mentioned issue. To add, I am not reproducing this issue without getKey and I believe that issue might not be tied with the list keys.

Simulator.Screen.Recording.-.iPhone.14.-.2023-04-06.at.11.03.37.mp4

MelvinBot · 2023-04-07T21:52:47Z

Reviewing label has been removed, please complete the "BugZero Checklist".

MelvinBot · 2023-04-07T21:52:49Z

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.96-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

Fixes initially selected key for timezones list #16972

If no regressions arise, payment will be issued on 2023-04-14. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

MelvinBot · 2023-04-07T21:52:53Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@sobitneupane / @mountiny] The PR that introduced the bug has been identified. Link to the PR: Disables timezone options if it is set to automatic #15176
[@sobitneupane / @mountiny] 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: Disables timezone options if it is set to automatic #15176 (comment)
[@sobitneupane / @mountiny] 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: https://expensify.slack.com/archives/C049HHMV9SM/p1681279519887149
[@lschurr] Determine if we should create a regression test for this bug.
[@sobitneupane] 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.
[@lschurr] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/275503

Pujan92 · 2023-04-10T06:05:39Z

@sobitneupane @mountiny I thought we were waiting for the confirmation of getKey function removal from @mollfpr.
@mollfpr let us know in case you have time and if you can help over here. :)

mountiny · 2023-04-11T17:57:39Z

For now, we just went ahead with that solution

Pujan92 · 2023-04-11T18:31:34Z

For now, we just went ahead with that solution

Ok, Thanks!

mollfpr · 2023-04-11T18:33:36Z

I'm sorry for not getting back to you sooner. @Pujan92 the issue I mentioned in the PR is starting from the select timezone page and then returning to the initial timezone page. It's working fine if you start the app from the initial timezone page.

Here's the latest commit from the #15176 PR without getKey. It's even worst than the issue I mentioned.

Simulator.Screen.Recording.-.iPhone.14.-.2023-04-12.at.01.29.19.mp4

And here's if you start from the initial timezone page.

Simulator.Screen.Recording.-.iPhone.14.-.2023-04-12.at.01.29.50.mp4

I hope this will clear up your objection!

Pujan92 · 2023-04-11T19:23:19Z

@mollfpr Thanks for the response, I don't know but I am not able to reproduce the one in which multiple items have been selected.
But for the issue case where you mentioned that the newly selected timezone is not reflected for a long when we start the app direct on select timezone page is bcoz the api openApp takes time to complete when we directly open the select timezone page, so when it completes it has the initial selected value which has been set in middle of the transition. Post that we won't see this issue. Here is the video, I hope you are referring to this issue only.

Screen.Recording.2023-04-12.at.12.46.07.AM.mp4

Based on the above observation getKey isn't make much sense to me, so I still doubt that after adding getKey aren't you facing this bcoz it should occur and occurring for me irrespective of the usage getKey.

lschurr · 2023-04-11T21:26:38Z

@mountiny @sobitneupane - Are we still good to continue working through the BZ checklist on this one? Do we need a new regression test?

mountiny · 2023-04-11T21:31:27Z

Is there any regression test for this? cc @sobitneupane

sobitneupane · 2023-04-12T06:05:36Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@sobitneupane / @mountiny] The PR that introduced the bug has been identified. Link to the PR:

#15176

[@sobitneupane / @mountiny] 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:

#15176 (comment)

[@sobitneupane / @mountiny] 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:

https://expensify.slack.com/archives/C049HHMV9SM/p1681279519887149

sobitneupane · 2023-04-12T06:10:57Z

Regression Test Proposal

Go to Settings -> Profile -> Timezone
Make sure "Automatically determine your location" toggle is turned off and Tap on timezone
Verify that currently selected timezone is marked and visible without scrolling

Do we agree 👍 or 👎

mountiny · 2023-04-12T12:14:30Z

Thank you Sobit 🙇

@lschurr this should be ready

lschurr · 2023-04-12T15:37:45Z

Added regression test request: https://github.com/Expensify/Expensify/issues/275503

lschurr · 2023-04-13T18:44:29Z

@alexxxwork and @sobitneupane - Could you apply for the job: https://www.upwork.com/jobs/~015b1ab2dc02fa1f2c

sobitneupane · 2023-04-13T19:02:38Z

Thanks @lschurr. Applied

Pujan92 · 2023-04-13T19:03:54Z

@alexxxwork and @sobitneupane - Could you apply for the job: https://www.upwork.com/jobs/~015b1ab2dc02fa1f2c

Shall I apply for reporting the issue?

alexxxwork · 2023-04-13T19:22:56Z

@lschurr I've applied, thank you.

lschurr · 2023-04-13T22:06:02Z

Shall I apply for reporting the issue?

Yes @Pujan92!

lschurr · 2023-04-14T14:58:08Z

All paid. Closing!

mountiny added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 4, 2023

mountiny self-assigned this Apr 4, 2023

melvin-bot bot assigned lschurr Apr 4, 2023

mountiny added the External Added to denote the issue can be worked on by a contributor label Apr 4, 2023

melvin-bot bot changed the title ~~Selected timezone option is not scrolled initially for the timezones list~~ [$1000] Selected timezone option is not scrolled initially for the timezones list Apr 4, 2023

melvin-bot bot assigned sobitneupane Apr 4, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 4, 2023

mountiny assigned alexxxwork Apr 5, 2023

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 5, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Apr 7, 2023

melvin-bot bot changed the title ~~[$1000] Selected timezone option is not scrolled initially for the timezones list~~ [HOLD for payment 2023-04-14] [$1000] Selected timezone option is not scrolled initially for the timezones list Apr 7, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Apr 7, 2023

sobitneupane mentioned this issue Apr 12, 2023

Disables timezone options if it is set to automatic #15176

Merged

55 tasks

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 14, 2023

lschurr closed this as completed Apr 14, 2023

[HOLD for payment 2023-04-14] [$1000] Selected timezone option is not scrolled initially for the timezones list #16905

[HOLD for payment 2023-04-14] [$1000] Selected timezone option is not scrolled initially for the timezones list #16905

Comments

mountiny commented Apr 4, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

MelvinBot commented Apr 4, 2023

MelvinBot commented Apr 4, 2023

Bug0 Triage Checklist (Main S/O)

MelvinBot commented Apr 4, 2023

MelvinBot commented Apr 4, 2023

MelvinBot commented Apr 4, 2023

MelvinBot commented Apr 4, 2023

Pujan92 commented Apr 4, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

mountiny commented Apr 4, 2023

alexxxwork commented Apr 4, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

situchan commented Apr 4, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

sobitneupane commented Apr 5, 2023

sobitneupane commented Apr 5, 2023

situchan commented Apr 5, 2023

alexxxwork commented Apr 5, 2023

sobitneupane commented Apr 5, 2023 • edited Loading

situchan commented Apr 5, 2023

sobitneupane commented Apr 5, 2023

situchan commented Apr 5, 2023

MelvinBot commented Apr 5, 2023

Pujan92 commented Apr 5, 2023 • edited Loading

Pujan92 commented Apr 6, 2023

MelvinBot commented Apr 7, 2023

MelvinBot commented Apr 7, 2023

MelvinBot commented Apr 7, 2023 • edited by lschurr Loading

Pujan92 commented Apr 10, 2023 • edited Loading

mountiny commented Apr 11, 2023

Pujan92 commented Apr 11, 2023

mollfpr commented Apr 11, 2023

Pujan92 commented Apr 11, 2023 • edited Loading

lschurr commented Apr 11, 2023

mountiny commented Apr 11, 2023

sobitneupane commented Apr 12, 2023

sobitneupane commented Apr 12, 2023

mountiny commented Apr 12, 2023

lschurr commented Apr 12, 2023

lschurr commented Apr 13, 2023

sobitneupane commented Apr 13, 2023

Pujan92 commented Apr 13, 2023

alexxxwork commented Apr 13, 2023

lschurr commented Apr 13, 2023

lschurr commented Apr 14, 2023

mountiny commented Apr 4, 2023 •

edited by melvin-bot bot

Loading

Pujan92 commented Apr 4, 2023 •

edited

Loading

alexxxwork commented Apr 4, 2023 •

edited

Loading

situchan commented Apr 4, 2023 •

edited

Loading

sobitneupane commented Apr 5, 2023 •

edited

Loading

Pujan92 commented Apr 5, 2023 •

edited

Loading

MelvinBot commented Apr 7, 2023 •

edited by lschurr

Loading

Pujan92 commented Apr 10, 2023 •

edited

Loading

Pujan92 commented Apr 11, 2023 •

edited

Loading