[$500] IOU - The last member is focused when I remove members

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!

Version Number: 1.3.95.0 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 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

Navigate to https://staging.new.expensify.com/r/8468866637847294
Log in
Click on the "2FAB" button
Click on the "Start chat" button
Add 4 different members to the group
Click on the "Create chat" button
Click on the "+" button in the Composer
Click on the "Split bill" button
Type any amount
Click on the "Next" button
De-select members

Expected Result:

The first member should be focused.

Actual Result:

The last member is focused when I remove members.

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/93399543/6dea1e19-748f-4bca-ba24-c66fee444b23

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01d3bc058e2dff750b
Upwork Job ID: 1720450358180958208
Last Price Increase: 2023-12-01

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01d3bc058e2dff750b

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @miljakljajic (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)

[ ] 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[bot] commented 1 year ago

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

kbecciv commented 1 year ago

The issue was found during the execution of TR on step 10 https://expensify.testrail.io/index.php?/tests/view/3936772&group_by=cases:section_id&group_id=229063&group_order=asc

DylanDylann commented 1 year ago

Proposal

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

IOU - The last member is focused when I remove members

What is the root cause of that problem?

We always set newFocusedIndex = props.selectedOptions.length https://github.com/Expensify/App/blob/bc9cb8f8ca149c1f90dd09de9f19ae97bcaf07f0/src/components/OptionsSelector/BaseOptionsSelector.js#L116

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

We need to introduce new props called shouldFocusFirstSelectableMemberWhenUnselecting (default false)

In here https://github.com/Expensify/App/blob/bc9cb8f8ca149c1f90dd09de9f19ae97bcaf07f0/src/components/OptionsSelector/BaseOptionsSelector.js#L116 if shouldFocusFirstSelectableMemberWhenUnselecting === true we will use new logic here

const disableSection = _.filter(this.props.sections, (section => section.isDisabled))
        const disableLength = _.reduce(disableSection, 
            (accumulator, currentValue) => accumulator + currentValue.data.length,
            0,
          );
        let newFocusedIndex = prevProps.selectedOptions.length < this.props.selectedOptions.length ?  this.props.selectedOptions.length : disableLength;
        if (prevProps.selectedOptions.length === this.props.selectedOptions.length) {
            newFocusedIndex = this.state.focusedIndex
        }

if shouldFocusFirstSelectableMemberWhenUnselecting === false we use old logic to ensure that this change won't break other flow

And then in MoneyRequestParticipantsSelector we will pass shouldFocusFirstSelectableMemberWhenUnselecting= true to OptionSelector Component

What alternative solutions did you explore? (Optional)

When user unselect member we should update focusIndex to the last selected member

abzokhattab commented 1 year ago

Proposal

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

The first unselected member is focused when removing members in multiselect

What is the root cause of that problem?

we set the newFocusedIndex to the last selected item: https://github.com/Expensify/App/blob/bc9cb8f8ca149c1f90dd09de9f19ae97bcaf07f0/src/components/OptionsSelector/BaseOptionsSelector.js#L116

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

in case of unselect we need to check if the new sections list lenght is smaller than the pre selection,

if this is the case then we can use the initialFocusedIndex prop as the list in the splitWith is a special case where the first selected item is at index 1 not 0

additionally we need to adjust the default value of the initialFocusedIndex to 0 so that it is fixed with all other components that use optionsSelector

        const newFocusedIndex = prevProps.selectedOptions.length < this.props.selectedOptions.length ? this.props.selectedOptions.length : this.props.initialFocusedIndex;
        const isNewFocusedIndex = newFocusedIndex !== this.state.focusedIndex;

        // eslint-disable-next-line react/no-did-update-set-state
        this.setState(
            {
                allOptions: newOptions,
                focusedIndex: newFocusedIndex,

adjust this default value to 0: https://github.com/Expensify/App/blob/ebfc96a76f3d6e45c2c1ab954300547cb59f3715/src/components/OptionsSelector/optionsSelectorPropTypes.js#L177

and finally in the splitwith selector add

            initialFocusedIndex={1}

this way in the case of unselect it will focus always on the initially focus index and in the case of select it will focus on the first unselected item.

POC:

Uploading Screen Recording 2023-11-03 at 5.33.55 PM.mov…

bernhardoj commented 1 year ago

Not a bug, we always highlight the first unselected option.

DylanDylann commented 1 year ago

@bernhardoj Could you help to link the discussion about it?

s77rt commented 1 year ago

@DylanDylann Thanks for the proposal. Your RCA makes sense however the solution would break other cases where it's intended to focus the first unselected option e.g. when creating a new group from FAB.

https://github.com/Expensify/App/assets/16493223/93705f8b-10b5-41af-8f17-cd4e492c1ed9

s77rt commented 1 year ago

@abzokhattab Thanks for the proposal but it's seems to be a dupe.

melvin-bot[bot] commented 1 year ago

📣 @anzokhattab! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

abzokhattab commented 1 year ago

@s77rt not duplicate … i have used the initialFocusedIndex to set the focus .. where in the first proposal, the index 1 is always focused … there is a problem with this approach, it only works with the “split with” … it doesn’t work with search/add to group selection, therefore this is not a fully working solution.

my proposal works with other cases, please double check

Thanks

https://github.com/Expensify/App/assets/59809993/bb4c7697-533d-4822-af1e-6c539a0670bb

DylanDylann commented 1 year ago

@s77rt Updated proposal

s77rt commented 1 year ago

@abzokhattab The behavior on the group chat creation should not change. i.e. the focus should be on the first unselected option (and not the first option).

s77rt commented 1 year ago

@DylanDylann Same not as above ^

DylanDylann commented 1 year ago

@s77rt Oh got it. Updated proposal

s77rt commented 1 year ago

@DylanDylann Thanks for the update. Introducing a new prop would probably open many ways to fix this bug but the fix would be done per instance basis. Can't we instead detect the change in the component itself e.g. use the number of sections. Or maybe have the focus always set to the first option of the second section.

DylanDylann commented 1 year ago

@s77rt Could you help to confirm the expected first?

In the split bill participant (using OptionSelector Component)
- YES or NO : Unselect the member will move focus to the first selected option (except disable option)
- YES or NO : Select the member will move focus to the first unselected option
In the Other Page where using OptionSelector Component (Like creating group page,...)
- YES or NO : Unselect the member will move focus to the first unselected option
- YES or NO : Select the member will move focus to the first unselected option

s77rt commented 1 year ago

@DylanDylann I'm not totally sure here. I think we should first recheck whether this is a bug

s77rt commented 1 year ago

Slack discussion: https://expensify.slack.com/archives/C01GTK53T8Q/p1699183749228599

s77rt commented 1 year ago

Based on the slack discussion, this is the expected behavior. Let's just update the TR:

Verify that when deselecting a member the focus is moved to the first unselected option in the list

cc @miljakljajic

s77rt commented 1 year ago

Not overdue. @miljakljajic ^

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? 💸

s77rt commented 1 year ago

Not overdue. @miljakljajic https://github.com/Expensify/App/issues/30851#issuecomment-1793791157

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? 💸

s77rt commented 1 year ago

Not overdue. @miljakljajic https://github.com/Expensify/App/issues/30851#issuecomment-1793791157

s77rt commented 12 months ago

@miljakljajic ^

melvin-bot[bot] commented 11 months ago

@s77rt @miljakljajic 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!

melvin-bot[bot] commented 11 months ago

@s77rt, @miljakljajic Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 11 months ago

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

s77rt commented 11 months ago

Not looking for proposals. Just need to update TR. @miljakljajic https://github.com/Expensify/App/issues/30851#issuecomment-1793791157

s77rt commented 11 months ago

@miljakljajic ^

melvin-bot[bot] commented 11 months ago

@s77rt, @miljakljajic Whoops! This issue is 2 days overdue. Let's get this updated quick!

s77rt commented 11 months ago

@miljakljajic https://github.com/Expensify/App/issues/30851#issuecomment-1793791157

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

@s77rt @miljakljajic this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Decide whether any proposals currently meet our guidelines and can be approved as-is today
If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 11 months ago

Current assignee @s77rt is eligible for the Internal assigner, not assigning anyone new.

miljakljajic commented 11 months ago

I'll update TestRail now! Thank you @s77rt !

Expensify / App