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

[kbecciv]

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. From device 1, login with user A
2. From device 2, login with user A
3. From device 1, open any report, click on plus, click on send/request money, enter any amount and click continue
4. From device 2, change the language from English to Spanish or vis a versa
5. 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
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• 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

user.highlight.send.or.request.money.language.change.1.mp4

Recording.5134.mp4

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]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[bernhardoj]

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.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 55 to 60 in 64b720c

	const allOptions = this.flattenSections();
	const focusedIndex = this.getInitiallyFocusedIndex(allOptions);
	this.state = {
	allOptions,
	focusedIndex,

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 174 to 190 in 64b720c

	getInitiallyFocusedIndex(allOptions) {
	if (this.props.selectedOptions.length > 0) {
	return this.props.selectedOptions.length;
	}
	const defaultIndex = this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0;
	if (_.isUndefined(this.props.initiallyFocusedOptionKey)) {
	return defaultIndex;
	}
	const indexOfInitiallyFocusedOption = _.findIndex(allOptions, (option) => option.keyForList === this.props.initiallyFocusedOptionKey);
	if (indexOfInitiallyFocusedOption >= 0) {
	return indexOfInitiallyFocusedOption;
	}
	return defaultIndex;
	}

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.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 127 to 139 in 64b720c

	componentDidUpdate(prevProps) {
	if (_.isEqual(this.props.sections, prevProps.sections)) {
	return;
	}
	const newOptions = this.flattenSections();
	const newFocusedIndex = this.props.selectedOptions.length;
	// eslint-disable-next-line react/no-did-update-set-state
	this.setState(
	{
	allOptions: newOptions,
	focusedIndex: newFocusedIndex,
	},

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]

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

21538.mp4

What alternative solutions did you explore? (Optional)

[bfitzexpensify]

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

[bfitzexpensify]

Bump to review proposals @aimane-chnaif

[melvin-bot]

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

[aimane-chnaif]

@bernhardoj your solution easily causes regression:

Screen.Recording.2023-07-03.at.5.13.33.PM.mov

[aimane-chnaif]

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]

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

[aimane-chnaif]

yes, reviewed already

[rojiphil]

@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]

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

[bernhardoj]

@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 #11795.

[bfitzexpensify]

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

[melvin-bot]

📣 @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]

📣 @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 📖

[melvin-bot]

🎯 ⚡️ Woah @aimane-chnaif / @ShogunFire, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @ShogunFire got assigned: 2023-07-18 17:49:50 Z
• when the PR got merged: 2023-07-21 16:00:58 UTC

On to the next one 🚀

[melvin-bot]

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

[melvin-bot]

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

• Fix: When the language changes, we should not change the focused index in a list #23117

If no regressions arise, payment will be issued on 2023-08-01. 🎊

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

For reference, here are some details about the assignees on this issue:

• @ShogunFire requires payment
• @aimane-chnaif requires payment

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

[melvin-bot]

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:

• [@aimane-chnaif] The PR that introduced the bug has been identified. Link to the PR:
• [@aimane-chnaif] 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:
• [@aimane-chnaif] 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:
• [@aimane-chnaif] Determine if we should create a regression test for this bug.
• [@aimane-chnaif] 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.
• [@bfitzexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[bfitzexpensify]

Offers sent - @aimane-chnaif, please complete the BZ checklist when you get a chance, thank you!

[bfitzexpensify]

Bump on the BZ checklist, thanks @aimane-chnaif

[bfitzexpensify]

Another friendly bump @aimane-chnaif

[bfitzexpensify]

Another bump @aimane-chnaif - please complete this when you get a chance and I'll finalise payment, thank you!

[aimane-chnaif]

Sorry for late.

No PRs caused regression. As this is edge case (should use another device to change language), minor visual issue and doesn't affect user, I don't think regression test step is needed here.

[bfitzexpensify]

Thanks @aimane-chnaif, I agree. OK, last payment finalised, closing this out