[Wrap up] [$1000] Default highlight of next possible selection feature breaks in split bill when we press on continue and comeback to user selection page

[kavimuru]

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. Open the app
2. Click on plus and click on split bill
3. Add any amount and continue
4. Try to select and unselect few users and observe that app by default highlights the next possible selection (Same logic throughout the app i.e. on invite members, new group etc)
5. Select few users and click on continue
6. Click on back and observe that rather than highlighting next possible selection, app highlights very first selected user
7. Select or unselect any user and observe that app rectifies the issue and again highlights next possible selection

Expected Result:

App should highlight next possible selection when we revisit the user selection page from further steps

Actual Result:

App highlights first selected value on revisit of user selection page breaking the default behavior maintained throughout the app on forms like invite member, new group etc.

Workaround:

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

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.3
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

Recording.303.mp4

default.feature.of.next.possible.selection.breaks.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0182c61bb6abf912f3
• Upwork Job ID: 1649395809845350400
• Last Price Increase: 2023-04-21

[MelvinBot]

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

[MelvinBot]

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

[conorpendergrast]

It sounds like the bug report here is:

1. If you are choosing who to split a bill with, then the highlighted user is the next unselected user
2. If you then return to the same screen, to choose who to split a bill with, then the highlighted user is the next selected user

Example of 1	Example of 2

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0182c61bb6abf912f3

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[allroundexperts]

Proposal

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

Default highlighted option does not persist when you come back to the page

What is the root cause of that problem?

The root cause of this issue is that whenever the component mounts, it is calculating the getInitiallyFocusedIndex without taking into account if there are already selected options. This can be seen here.

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

The getInitiallyFocusedIndex function should be modified such that it returns correct index if there are already selected options. The following condition needs to be added here.

        if (this.props.selectedOptions.length > 0) {
            return this.props.selectedOptions.length;
        }

We'll also need to update the scroll position when the component mounts depending on the number of selected options. There are two places where we need to do this. First, we need to change this line to:

if (this.props.selectedOptions.length === 0) {
    this.scrollToIndex(this.state.focusedIndex, false);
}

Secondly, we also need to update this line to:

this.scrollToIndex(this.props.selectedOptions.length ? 0 : this.state.focusedIndex, false);

What alternative solutions did you explore? (Optional)

None

[dukenv0307]

Proposal

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

Default highlighted option is wrong when you come back to the page

What is the root cause of that problem?

Default focus index is 0 if not shouldTextInputAppearBelowOptions and initiallyFocusedOptionKey.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Line 156 in a33b719

const defaultIndex = this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0;

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

Change the default focus index to this.props.selectedOptions.length If not shouldTextInputAppearBelowOptions and initiallyFocusedOptionKey: const defaultIndex = this.props.shouldTextInputAppearBelowOptions ? allOptions.length : this.props.selectedOptions.length;
And then we also need to change

App/src/components/OptionsSelector/BaseOptionsSelector.js

Line 308 in a33b719

this.scrollToIndex(this.state.focusedIndex, false);

to this.scrollToIndex(0, false); to scroll to first option when go back.

What alternative solutions did you explore? (Optional)

NA

Result

Screencast.2023-04-21.20.30.34.mp4

[fedirjh]

@allroundexperts's proposal looks good to me.

cc @yuwenmemon

🎀👀🎀 C+ reviewed

[fedirjh]

@dukenv0307 Thanks for the proposal. However it's a duplicate of @allroundexperts's proposal.

[MelvinBot]

📣 @allroundexperts You have been assigned to this job by @yuwenmemon!
Please apply to this job in Upwork 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 📖

[allroundexperts]

PR created #17950

[MelvinBot]

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

[MelvinBot]

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

• Fix default highlighted option in base option selector #17950

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

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: @dhanashree-sawant - $250
• Contributor that fixed the issue: @allroundexperts - $1000 base + 50% bonus = $1500
• Contributor+ that helped on the issue and/or PR: @fedirjh - $1000 base + 50% bonus = $1500

Assigned: April 24th 5:35pm
Merged: April 26th 6:57pm

• 50% bonus

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]

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:

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

[conorpendergrast]

Offers sent via Upwork, paying tomorrow (2023-05-05) unless there's a regression

[conorpendergrast]

All three paid. Upwork is being Upwork, so I'll have to come back to this page to end one of the contracts later.

[Edit: contract ended finally]

@fedirjh Over to you for the steps here please

[melvin-bot]

@yuwenmemon @conorpendergrast @allroundexperts @fedirjh 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!

[conorpendergrast]

Ah now Melvin

[fedirjh]

BugZero Checklist:

1. Link to the PR: I couldn't find any PR that introduced this bug. It seems that this behavior is new and was not implemented before. However, the new behavior could have been implemented in PR fix/14314: Timezone changes to some 1st option in the list while pressing the enter key #14938
2. Link to comment: N/A
3. Link to discussion: This was not a bug introduced by a PR. We just made an improvement to make the behavior consistent. We're not sure if there was anything we could have done to catch it before. @conorpendergrast, please let me know if we should start a discussion about this.

---

Regression Test Proposal:

1. Open the app.
2. Click on the + sign and select Split Bill.
3. Enter an amount and click Continue.
4. Select a few users.
5. Verify that the app highlights the next possible selection.
6. Click Continue to reach the final step.
7. Click Back to return to the user selection page.
8. Verify that the app highlights the next possible selection.

Do we agree 👍 or 👎

[conorpendergrast]

Sounds good to me. @yuwenmemon any disagreement? If there isn't, I'll create the regression test issue, and will close this out

[conorpendergrast]

I created the issue for the regression test here: https://github.com/Expensify/Expensify/issues/282499

Closing this all out now, thanks everyone