Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[HOLD for payment 2023-12-20] [$500] Split - Participants under “Split with” list disappear after unselecting them #32808

Closed kbecciv closed 10 months ago

kbecciv commented 11 months 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: v1.4.11-1 Reproducible in staging?: y Reproducible in production?: n 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:

  1. Go to staging.new.expensify.com.
  2. Go to group chat > + > Split bill.
  3. Enter amount > Next.
  4. Unselect the participants.

Expected Result:

Group participants will not disappear from the “Split with” list after unselecting them.

Actual Result:

Group participants disappear from the “Split with” list after unselecting them.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/9ae38753-b36c-4e25-acf0-f55f7f1936a7

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01103b76f36fd3edb8
  • Upwork Job ID: 1734224694853259264
  • Last Price Increase: 2023-12-11
  • Automatic offers:
    • s-alves10 | Contributor | 28035106
github-actions[bot] commented 11 months ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
melvin-bot[bot] commented 11 months ago

Triggered auto assignment to @nkuoch (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

tgolen commented 11 months ago

I'll look into it this morning.

s-alves10 commented 11 months ago

Proposal

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

Participants under "Split with" list disappear when unselecting them

What is the root cause of that problem?

This is a regression of the PR https://github.com/Expensify/App/pull/28618

The root cause is in the following code segment https://github.com/Expensify/App/blob/a16df0d7b095b98ed3b23df4add3508cbd8a69a2/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js#L371

In the above code, selectedParticipantsFiltered is a filtered participants list(selected = true) as you can see here https://github.com/Expensify/App/blob/a16df0d7b095b98ed3b23df4add3508cbd8a69a2/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js#L361

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

Update the above line to

        const unselectedParticipants = _.filter(selectedParticipants, (participant) => !participant.selected);

This works as expected

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 11 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01103b76f36fd3edb8

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

📣 @s-alves10 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer 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 📖

tgolen commented 11 months ago

The above proposal from @s-alves10 is correct. It was a fix that I missed adding to the refactored component.

s-alves10 commented 11 months ago

@allroundexperts

PR is ready for review: https://github.com/Expensify/App/pull/32823

cubuspl42 commented 11 months ago

I'll handle the PR review if that's all right. @allroundexperts was unassigned from the issue as @tgolen took the proposal selection on him, and I was pullerbear'ed into the PR.

allroundexperts commented 11 months ago

All yours @cubuspl42!

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

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

If no regressions arise, payment will be issued on 2023-12-20. :confetti_ball:

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.

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

melvin-bot[bot] commented 11 months ago

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:

tgolen commented 11 months ago

I don't think we need a regression test. This was caught during regression testing.

s-alves10 commented 11 months ago
  • [x] [@s-alves10] Determine if we should create a regression test for this bug.
  • [x] [@s-alves10] 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.

This is a regression of refactoring PR https://github.com/Expensify/App/pull/28618. I don't think we need regression test here

Do we agree 👍 or 👎

melvin-bot[bot] commented 10 months ago

@tgolen, @s-alves10 Eep! 4 days overdue now. Issues have feelings too...

tgolen commented 10 months ago

It looks like this can be closed out and we agree that we don't need a regression test.

tgolen commented 10 months ago

Oops, it looks like this hasn't been paid out. Reopening and assigning someone to help with the payout.

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

Bug0 Triage Checklist (Main S/O)

tgolen commented 10 months ago

@bfitzexpensify Could you please help pay this issue according to https://github.com/Expensify/App/issues/32808#issuecomment-1853706054?

bfitzexpensify commented 10 months ago

Payment complete, closing this out.