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

[$1000] Web - Country search input is cleared/reset when updating language on another device #23851

Closed
1 of 6 tasks
kbecciv opened this issue Jul 29, 2023 · 48 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff

Comments

@kbecciv
Copy link

kbecciv commented Jul 29, 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:

  1. Login to any account
  2. Go to Settings -> Profile -> Persona details -> Home address -> Country
  3. Type any text to search a country
  4. Go to another device, login same account
  5. Change to another language on this device

Expected Result:

Country search input should be remain current search text when updating language on another device

Actual Result:

Country search input is cleared/reset when updating language on another device

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.47-2
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

Screen.Recording.2023-07-28.at.21.48.37.mov
Recording.3990.mp4

Expensify/Expensify Issue URL:
Issue reported by: @hoangzinh
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690556043839129

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016ec8ed64c16cfa11
  • Upwork Job ID: 1694407216392552448
  • Last Price Increase: 2023-08-23
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 29, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 29, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 29, 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

@kbecciv
Copy link
Author

kbecciv commented Jul 29, 2023

Proposal by: @hoangzinh
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690556043839129

Proposal

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

Country search input is cleared/reset when updating language on another device

What is the root cause of that problem?

In the CountryPicker, we have a useEffect that reset the search text value when allCountries is changed. allCountries is changed when preferred language changed => when preferred language changed it reset the search text value, in this case it will reset to an empty string.

useEffect(() => {
setSearchValue(lodashGet(allCountries, value, ''));
}, [value, allCountries]);

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

I guess the useEffect above is used to set search text value when props.value is changed, then it will be used in the CountrySelectorModal
So I think we can remove the useEffect above and call set search text value with props.value in the show picker modal callback here

const showPickerModal = () => {
setIsPickerVisible(true);
};

@melvin-bot melvin-bot bot added the Overdue label Jul 31, 2023
@alexpensify
Copy link
Contributor

Not overdue - I was assigned over the weekend and will test soon.

@melvin-bot melvin-bot bot removed the Overdue label Jul 31, 2023
@alexpensify alexpensify changed the title Web - Country search input is cleared/reset when updating language on another device [HOLD #17548] Web - Country search input is cleared/reset when updating language on another device Aug 2, 2023
@alexpensify alexpensify added Weekly KSv2 and removed Daily KSv2 labels Aug 2, 2023
@alexpensify
Copy link
Contributor

There are some upcoming changes to the Country selector. I'm going to put it on hold to confirm if this behavior still happens when the update goes to production.

@hoangzinh
Copy link
Contributor

@alexpensify could you point to the PR or GH Issue about those changes? Thanks

@alexpensify
Copy link
Contributor

I believe the options will become dropdown following this update: #22018

@hoangzinh
Copy link
Contributor

@alexpensify I just reviewed the PR above, but I think it's only fix the initial value of country. Btw, the PR you mentioned above is deployed to staging.

@alexpensify
Copy link
Contributor

Thanks for flagging, I'll test again to confirm if we get the same experience now. @hoangzinh please test too and share if you get the same result as before.

@hoangzinh
Copy link
Contributor

@alexpensify I still reproduce the bug in latest staging

Screen.Recording.2023-08-17.at.23.49.05.mp4

@alexpensify alexpensify added External Added to denote the issue can be worked on by a contributor Daily KSv2 and removed Weekly KSv2 labels Aug 23, 2023
@melvin-bot melvin-bot bot changed the title [HOLD #17548] Web - Country search input is cleared/reset when updating language on another device [$1000] [HOLD #17548] Web - Country search input is cleared/reset when updating language on another device Aug 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2023

Job added to Upwork: https://www.upwork.com/jobs/~016ec8ed64c16cfa11

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

melvin-bot bot commented Aug 23, 2023

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

@alexpensify alexpensify changed the title [$1000] [HOLD #17548] Web - Country search input is cleared/reset when updating language on another device [$1000] Web - Country search input is cleared/reset when updating language on another device Aug 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2023

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

@tienifr
Copy link
Contributor

tienifr commented Aug 28, 2023

@allroundexperts yes, it causes the behavior inconsistency between the CountryPicker and StatePicker

Currently it's consistent, after the PR it's not consistent any more 😄

I agree let's minimize discussion here and wait for the final call from @puneetlath!

@tienifr
Copy link
Contributor

tienifr commented Aug 28, 2023

And I think StatePicker is quite beyond of scope of this issue, but thanks @tienifr for pointing out.

I think we have too much of these discussions, since both add value here, if @hoangzinh agrees we can split bounty and you can go ahead with the PR (and have the speed bonus if any) 🤝. We should foster more collaboration and try to add as much value as possible to Expensify!

@melvin-bot melvin-bot bot added the Overdue label Aug 30, 2023
@alexpensify
Copy link
Contributor

@puneetlath - we need some feedback here, thanks!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Aug 30, 2023
@puneetlath
Copy link
Contributor

Hi, sorry, I know I'm coming into this super late, but I'm not sure I agree that this is a bug. Is this a real scenario, where a user would change their language preference on one device while actively using the product in another?

And if they did, I don't think the user would be surprised to see the input cleared in that scenario. That behavior feels fine and normal to me.

I know a lot of work has gone into this already, so my apologies, but my vote would be to close this issue.

@melvin-bot melvin-bot bot removed the Overdue label Sep 4, 2023
@tienifr
Copy link
Contributor

tienifr commented Sep 6, 2023

And if they did, I don't think the user would be surprised to see the input cleared in that scenario. That behavior feels fine and normal to me.

@puneetlath Other inputs (like the Pronouns selection) behave differently in that scenario (it won't be cleared), so would that be a valid inconsistency?

@alexpensify
Copy link
Contributor

@puneetlath can we get your feedback again or should we move this to Slack for a bigger discussion?

@puneetlath
Copy link
Contributor

Does this have to be handled on an input-by-input basis? Or in other words, do the other inputs do what the proposal suggests?

I would be down for a global solution. But if we have to handle each input individually and then remember to do it for every one going forward, I don't think this is worth our time just for the two tabs scenario.

@tienifr
Copy link
Contributor

tienifr commented Sep 9, 2023

Does this have to be handled on an input-by-input basis?

@puneetlath No, this just happens for 2 inputs due to redundant code that can be removed (more details here).

Or in other words, do the other inputs do what the proposal suggests?

Yes, other inputs already work fine.

But if we have to handle each input individually and then remember to do it for every one going forward, I don't think this is worth our time just for the two tabs scenario.

We don't have to add anything to future inputs going forward, only the 2 inputs above need to be fixed.

@puneetlath
Copy link
Contributor

Ok got it, thanks for the clarifications. I'm going to go ahead and assign @hoangzinh as @allroundexperts suggested.

@tienifr
Copy link
Contributor

tienifr commented Sep 11, 2023

@puneetlath FYI @hoangzinh's proposal only fixes 1 instance of the issue and my proposal fixes the full scope. There was a bounty split discussed and @hoangzinh agrees with it here with the thumbs up. Do you want to assign both or just handle the split later during payment?

@puneetlath
Copy link
Contributor

Ah cool, that sounds good and works for me.

@hoangzinh
Copy link
Contributor

Oh it seems Melvin bot is not triggered the auto hire flow. Btw I gonna create the PR first.

@hoangzinh
Copy link
Contributor

hoangzinh commented Sep 11, 2023

I found that the bug we would like to fix here that has been fixed in this PR already #26066. I couldn't reproduce the bug in the latest main branch

@allroundexperts
Copy link
Contributor

Wow. I guess we can close this then!

@puneetlath
Copy link
Contributor

Ah wow ok. Well thanks for your effort! Onto the next one.

@hoangzinh
Copy link
Contributor

@puneetlath Can I get reporting bug bonus? Because I reported before the bug that I mentioned here #23851 (comment)

@puneetlath
Copy link
Contributor

Hmm, yes I suppose your bug was reported first. Can you go ahead and apply to the job?

@puneetlath puneetlath reopened this Sep 12, 2023
@alexpensify
Copy link
Contributor

I'm OOO until Thursday but will work on the required process when I return. The key is applying to the job. Thanks!

@hoangzinh
Copy link
Contributor

Thanks @puneetlath . I applied the job link here #23851 (comment)

@puneetlath
Copy link
Contributor

All handled. Thanks y'all for all the effort.

@alexpensify
Copy link
Contributor

Thanks @puneetlath -- no action on my end. The payment was sent via Upwork.

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 Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests

7 participants