[HOLD for payment 2023-08-01] [$1000] Request Money - User gets highlighted in send/request money on changing of language during the process

kbecciv commented 1 year ago

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:

From device 1, login with user A
From device 2, login with user A
From device 1, open any report, click on plus, click on send/request money, enter any amount and click continue
From device 2, change the language from English to Spanish or vis a versa
Observe in device 1 that user gets highlighted after language change (by default user is not highlighted)

Expected Result:

App should not highlight user in send/request money on language change

Actual Result:

App highlights user in send/request money while changing language during the process

Workaround:

Unknown

Platforms:

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

[ ] Android / native
[x] Android / Chrome
[ ] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

Version Number: 1.3.29-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

https://github.com/Expensify/App/assets/93399543/c0e4ee98-fe71-45f4-a6d1-7859987300d2

https://github.com/Expensify/App/assets/93399543/427846e1-96a8-4add-b463-65338b625553

Expensify/Expensify Issue URL: Issue reported by:@dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1687200047072349

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~013b389dab3622e04c
Upwork Job ID: 1673230286457331712
Last Price Increase: 2023-07-17

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] 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
[x] 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.
[x] 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.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~013b389dab3622e04c

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

bernhardoj commented 1 year ago

Proposal

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

Changing the app language will highlight the first item in the money request confirmation list.

What is the root cause of that problem?

The money request confirmation list uses OptionsSelector to show the list. The first time we open it, it will get the initial focus index. In a money request confirmation list case, the initially focused index is 1. https://github.com/Expensify/App/blob/64b720c5b74c3001bf3f9eceac1f54511611c6b7/src/components/OptionsSelector/BaseOptionsSelector.js#L55-L60 https://github.com/Expensify/App/blob/64b720c5b74c3001bf3f9eceac1f54511611c6b7/src/components/OptionsSelector/BaseOptionsSelector.js#L174-L190

It has some conditions to decide what is the correct index.

When the app language is changed, the list text (header) is updated to the selected language. This will trigger the update to OptionsSelector. https://github.com/Expensify/App/blob/64b720c5b74c3001bf3f9eceac1f54511611c6b7/src/components/OptionsSelector/BaseOptionsSelector.js#L127-L139

In componentDidUpdate, we immediately set the new index to be this.props.selectedOptions.length, which is 0. The money request confirmation list is not selectable. That's why the first item becomes highlighted.

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

We should update the new focused index with the value from this.getInitiallyFocusedIndex too. const newFocusedIndex = this.getInitiallyFocusedIndex(newOptions);

rojiphil commented 1 year ago

Proposal

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

User gets highlighted in the Money Request Confirmation Page when the language gets changed.

What is the root cause of that problem?

The root cause of the problem is that the index of the focused row is getting updated even when the state value of focusedIndex has not changed.

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

To solve the problem, we need to prevent updates to focusedIndex if it has not changed. 1) Prevent updates to focusedIndex when it has not changed.

-      componentDidUpdate(prevProps) {
+     componentDidUpdate(prevProps, prevStates) {
        ...
-        const newFocusedIndex = this.props.selectedOptions.length;
+        const newFocusedIndex = prevState.focusedIndex !== this.state.focusedIndex ? this.props.selectedOptions.length : this.state.focusedIndex;

Test Video

https://github.com/Expensify/App/assets/3004659/b5cda8d6-8a7e-48d4-81d2-273e1d30f60e

What alternative solutions did you explore? (Optional)

bfitzexpensify commented 1 year ago

@aimane-chnaif couple of proposals ready for review when you get a chance!

bfitzexpensify commented 1 year ago

Bump to review proposals @aimane-chnaif

melvin-bot[bot] commented 1 year ago

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

aimane-chnaif commented 1 year ago

@bernhardoj your solution easily causes regression:

https://github.com/Expensify/App/assets/96077027/c7fb371e-391b-4b73-a58a-b147e14bc149

aimane-chnaif commented 1 year ago

Hmm, actually not a regression since it already happens in production. But I still think this case should be fixed here. That being said, no satisfactory proposals yet.

rojiphil commented 1 year ago

@aimane-chnaif Were you able to review my proposal ? I think this should solve the problem. Please let me know your feedback.

aimane-chnaif commented 1 year ago

yes, reviewed already

rojiphil commented 1 year ago

@aimane-chnaif Two observations: a) I think, this specific issue is referring to the confirmation page reached via the report screen. My proposal focuses on this issue.
b) Also, I have a feeling that you came to the participants' page via the Sidebar screen. But, I do agree that the identified issue on the participants' page is also a valid one and needs to be addressed. I have made a slight modification to the logic to include this change too in my proposal. I think, this solves the problem. Will also share a test video in some time.

rojiphil commented 1 year ago

@aimane-chnaif I have also attached the test video in the proposal itself. Kindly let me know what you think.

bernhardoj commented 1 year ago

@aimane-chnaif I think that's okay. When we are searching, the index is always reset to selectedOptions.length. I don't think we want to disable that behavior. I think checking the length doesn't help because the length could be the same but the list is different. Last, I think we can hold this one for the list refactor here https://github.com/Expensify/App/issues/11795.

bfitzexpensify commented 1 year ago

@aimane-chnaif couple of comments to address - thank you!

ShogunFire commented 1 year ago

Proposal

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

When we change the language, the selected option changes

What is the root cause of that problem?

In BaseOptionSelector, in ComponentDidUpdate here: https://github.com/Expensify/App/blob/3475812665c9e57b2be2ea6feb18e82562340fce/src/components/OptionsSelector/BaseOptionsSelector.js#L135-L139

We change the options and the focused index even if the options have not changed.

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

If the update is due to a language change we should only update the options but not the focused Index, we can do this:

const newOptions = this.flattenSections();

//If only the preferredLocale has changed we don't update the focused index
if(prevProps.preferredLocale != this.props.preferredLocale){
    this.setState({
        allOptions: newOptions,
    });
    return;
}

const newFocusedIndex = this.props.selectedOptions.length;

Result:

https://github.com/Expensify/App/assets/19537677/b3cd4aa5-ab46-4ba8-afc7-9e466da079f3

What alternative solutions did you explore? (Optional)

rojiphil commented 1 year ago

@aimane-chnaif I think we need to update the focusedIndex only when the state of focusedIndex has changed within componentDidUpdate. I think this will solve the problem. The last time I updated the proposal with my comment here, I had already integrated the logic. I have refactored the code and have updated the proposal accordingly for your consideration.

melvin-bot[bot] commented 1 year ago

@bfitzexpensify @aimane-chnaif 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!

aimane-chnaif commented 1 year ago

@bfitzexpensify do you think this behavior is fine? I don't see any reason why it should reset selection when change language.

bfitzexpensify commented 1 year ago

I agree that behaviour isn't ideal. I think we want to make sure the selection remains the same when changing the language.

melvin-bot[bot] commented 1 year ago

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

ShogunFire commented 1 year ago

@aimane-chnaif My Proposal keeps the selection unchanged for those particular case, I have added an alternative solution if we want to make it work even when the text of the options changes during translation, I can make it more detailed if necessary, what do you think ?

aimane-chnaif commented 1 year ago

@ShogunFire sure, can you please provide more details about alternative solution?

ShogunFire commented 1 year ago

@aimane-chnaif Actually I just found out that we don't really need my alternative solutions for now because everytime we use OptionsSelector, the options are emails, phone numbers or currencies which don't change when we change language.

If we still want to do it, just to confirm,if the order of the options change should we keep focusing the same translated option or should we keep focusing the same index as before ?

Example In english, let's say the option2 is selected: {option1, 'Airport'} {option2, 'Car'} {option3, 'Train'}

We change the language to french (didn't find a good example in spanish :P): {option1, 'Avion'} {option3, 'Train'} {option2, 'Voiture'}

The option2 is now at the index3, should I focus the index 3 or should I keep focusing the index 2 ?

ShogunFire commented 1 year ago

Also if options have changed but the focused option is still here, should we keep focusing this option ?

aimane-chnaif commented 1 year ago

if options have changed

@ShogunFire can you please elaborate more?

ShogunFire commented 1 year ago

Let's say instead of {option1, 'Airport'} {option2, 'Car'} {option3, 'Train'}

We now have {option1, 'Airport'} {option2, 'Car'} {option4, 'Jet'} {option3, 'Train'} {option5, 'Yatch'}

Should we keep focusing on option2 'Car' if that was the case before ?

ShogunFire commented 1 year ago

Or let's say Train was focused before so that the index change

ShogunFire commented 1 year ago

If the focused index doesn't change even when the order changes we can just check if the update is due to preferredLocale, but if we want to keep focusing the same option I am thinking of using keyForList to get the same option in the new list

aimane-chnaif commented 1 year ago

We can just handle locale change here. If options list changes, it's fine to reset selection - this seems like super edge case, more like dynamically updating issues which are recently closed.

ShogunFire commented 1 year ago

Proposal

Updated

rojiphil commented 1 year ago

@aimane-chnaif You have not commented on my updated proposal yet. My latest comment is here. The proposal you are currently considering is already addressed in my proposal. How is this proposal better than mine?

aimane-chnaif commented 1 year ago

@rojiphil you should have commented like this for updated proposal to be reviewed.

rojiphil commented 1 year ago

@rojiphil you should have commented like this for updated proposal to be reviewed.

Oh! Didn't realize this aspect as I am also relatively new. Thanks for pointing this out.

rojiphil commented 1 year ago

Proposal

Updated

ShogunFire commented 1 year ago

@rojiphil If the options change for example when we filter the participants, your proposal will keep focusing the same index, if that index still exist, otherwise it will focus nothing. Also your proposal can make the screen scrolls even when the focused index hasn't changed

https://github.com/Expensify/App/assets/19537677/53dd0369-2dd7-4590-b170-fcdaddcf5cdc

rojiphil commented 1 year ago

@ShogunFire Interesting observations. Appreciate your comments. Thanks.

melvin-bot[bot] commented 1 year ago

@bfitzexpensify @aimane-chnaif 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!

bfitzexpensify commented 1 year ago

@aimane-chnaif please let us know if one of the proposals we have is sufficient - thanks!

melvin-bot[bot] commented 1 year ago

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

aimane-chnaif commented 1 year ago

@ShogunFire do we even need this change?

    this.setState({
        allOptions: newOptions,
    });

aimane-chnaif commented 1 year ago

I am thinking of case where options list requires dynamic locale change. Maybe I missed but might exist.

ShogunFire commented 1 year ago

I am thinking of case where options list requires dynamic locale change. Maybe I missed but might exist.

I didn't find either but I was thinking it is more secure to change the list anyway. If you think it's ok to just return we can remove this line.

aimane-chnaif commented 1 year ago

As it's no harm, let's add that for now to be safe. @ShogunFire's proposal looks good to me. 🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

Upwork job

melvin-bot[bot] commented 1 year ago

📣 @ShogunFire You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

Expensify / App