[HOLD for payment 2023-12-20] [$500] Split - User can select or unselect members on comfirmation page when splitting bill from FAB

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.11-2
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. Launch New Expensify app
2. Click FAB > Request money
3. Enter amount > Next
4. Click Split next to a few users that have prior chat (important)
5. Click Next

Expected Result:

User will not be given the choice to select and unselect bill split participants on the confirmation page when it is created from FAB (PROD behavior)

Actual Result:

User is able select and unselect bill split participants on the confirmation page when it is created from FAB

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Recording.696.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0157740199e6470999
• Upwork Job ID: 1734327112370180096
• Last Price Increase: 2023-12-11
• Automatic offers:
• c3024 | Reviewer | 28040213
• shubham1206agra | Contributor | 28040214

[github-actions]

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

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

[thienlnam]

@lanitochka17 I'm not quite following what the issue is from watching the video - I don't see any participants being edited from the confirmation page

[thienlnam]

Ah tested it locally and I see the issue now

[lanitochka17]

@thienlnam TestRail https://expensify.testrail.io/index.php?/tests/view/4057459

[thienlnam]

Looks like it likely was caused by #28618

[thienlnam]

We ideally don't want to revert this huge PR - if we can have it fixed externally that would be a much better outcome

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0157740199e6470999

[melvin-bot]

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

[abzokhattab]

Proposal

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

User can modify participants list when splitting bill using FAB

What is the root cause of that problem?

The following condition in the confirmation page decides whether the user is allowed to modify the participant's selections or not :

App/src/pages/iou/request/step/IOURequestStepConfirmation.js

Line 329 in 968f39f

canModifyParticipants={!_.isEmpty(report.reportID)}

the reportID is fetched from the params.reportID and here the new link that the user is routed to when clicking on FAB -> Request Money:

https://dev.new.expensify.com:8082/create/request/start/1/1437418385097478/manual

so in the new implementation of the money request, the url will always contain the report ID:

however here is the old one: https://new.expensify.com/request/new/manual

so in the old implementation the param.reportID will be null in this case and thus the user wont be able to modify the list

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

we can use the isFromGlobalCreate prop of the transcation which is introduced in this PR: https://github.com/Expensify/App/pull/32878/files

canModifyParticipants={!transaction.isFromGlobalCreate} 

[luacmartins]

We're adding isFromGlobalCreate param here, which I think can be used here too

[c3024]

@abzokhattab could you update your proposal with isFromGlobalCreate?

[shubham1206agra]

Proposal

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

Split - User can select or unselect members on comfirmation page when splitting bill from FAB

What is the root cause of that problem?

In IOURequestStepConfirmation

App/src/pages/iou/request/step/IOURequestStepConfirmation.js

Line 329 in 968f39f

canModifyParticipants={!_.isEmpty(report.reportID)}

This condition is being used to allow modifying the participant's list, but this condition will always be evaluated to true now as all requests have report ID due to recent refactoring.

But in the old implementation, the param.reportID was null in the given case and due to this, the user wasn't able to modify the list.

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

Use isFromGlobalCreate which is being created in #32878, and modify the condition to

canModifyParticipants={!transaction.isFromGlobalCreate} 

[abzokhattab]

@abzokhattab could you update your proposal with isFromGlobalCreate?

updated .. here is the poc :

pr can be raised in the next half an hour

Screen.Recording.2023-12-12.at.4.04.12.AM.mov

[c3024]

This issue is also pending. These being deploy blockers I think @shubham1206agra can raise PR quickly for the present issue.

Proposal by @shubham1206agra looks good to me.

🎀 👀 🎀 C+ Reviewed

[melvin-bot]

Current assignee @thienlnam is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[shubham1206agra]

@thienlnam Can you do the assignment here?

[melvin-bot]

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

Offer link
Upwork job

[melvin-bot]

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

[shubham1206agra]

@c3024 PR is ready

[Julesssss]

I'm going to Cp this now

[puneetlath]

Confirmed this is fixed.

[Julesssss]

Thank you!

[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.4.11-25 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix request flow #32918

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

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:

• @shubham1206agra requires payment automatic offer (Contributor)
• @c3024 requires payment automatic offer (Reviewer)

[c3024]

• [@c3024] The PR that introduced the bug has been identified. Link to the PR: Refactor the money request creation flow #28618
• [@c3024] 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: https://github.com/Expensify/App/pull/28618/files#r1433945152
• [@c3024] 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: N/A. This is part of a large refactor and this is a specific case of money request from FAB and a discussion for this is not useful.
• [@c3024] Determine if we should create a regression test for this bug: This was identified during testing in staging so not required.
• [@c3024] 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. N/A

[melvin-bot]

@thienlnam, @shubham1206agra, @c3024 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@thienlnam, @shubham1206agra, @c3024 Eep! 4 days overdue now. Issues have feelings too...

[slafortune]

Self-assigned to payout C and C+ - Done!