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.36k stars 2.78k forks source link

[Hold for payment 04-17-2024][$500] Web - Split bill - Split bill shortcut directs to 404 page after splitting a bill with one account #39406

Closed kbecciv closed 5 months ago

kbecciv commented 6 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.59-0 Reproducible in staging?: y Reproducible in production?: y Issue reported by: Applause - Internal Team

Action Performed:

  1. Click on FAB and select Request money
  2. Select manual > enter any valid amount and continue
  3. Click on Split on any one participant and click on Add to split
  4. Click on Split {currency} {amount}
  5. Click on FAB and click on the split bill shortcut

Expected Result:

Split bill shortcut should open

Actual Result:

Hmm... it's not here page opens

Workaround:

n/a

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/461d3b0a-aa08-4c3c-bdc1-c2649c866602

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01356e4f802ce02d01
  • Upwork Job ID: 1775206920005824512
  • Last Price Increase: 2024-04-02
  • Automatic offers:
    • jjcoffee | Reviewer | 0
    • bernhardoj | Contributor | 0
melvin-bot[bot] commented 6 months ago

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

kbecciv commented 6 months ago

@lschurr I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

kbecciv commented 6 months ago

We think that this bug might be related to #vip-split

bernhardoj commented 6 months ago

Proposal

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

Pressing the split bill shortcut after splitting bill with one user leads to not found page.

What is the root cause of that problem?

When we open the split bill from the shortcut, the canCreateRequest condition will be false. https://github.com/Expensify/App/blob/d7161ae40ea3c09a041fca7c680bdfd8dea0d4fe/src/pages/iou/request/IOURequestStartPage.js#L116

And that's because getMoneyRequestOptions doesn't include the split bill option for 1:1 DM. https://github.com/Expensify/App/blob/d7161ae40ea3c09a041fca7c680bdfd8dea0d4fe/src/libs/ReportUtils.ts#L4989-L4995 https://github.com/Expensify/App/blob/d7161ae40ea3c09a041fca7c680bdfd8dea0d4fe/src/libs/ReportUtils.ts#L4802-L4809

That's why you can't see the split bill option from composer + menu, but you can still split with one user.

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

Split bill with one user is totally valid, so I think we can start showing the split bill option for 1:1 DM too by removing the hasMultipleOtherParticipants conditions (or replacing it with otherParticipants.length > 0).

What alternative solutions did you explore? (Optional)

If we don't want to show the split bill option in the composer + menu but still allow it from the shortcut, then we can:

  1. Add a new param to canCreateRequest whether to include a split bill for 1:1 DM or not and pass true for the money request page. OR
  2. Add a new param to the money request page indicating the user is coming from the shortcut and always allow the user to create a request.
melvin-bot[bot] commented 6 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01356e4f802ce02d01

melvin-bot[bot] commented 6 months ago

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

dragnoir commented 5 months ago

Proposal

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

Split bill shortcut directs to 404 page after splitting a bill with one account

What is the root cause of that problem?

The IOURequestStartPage checks isAllowedToCreateRequest is true to display the FullPageNotFoundView https://github.com/Expensify/App/blob/ee5ab95f2c893738fb48756a9002e9d9dadd9a31/src/pages/iou/request/IOURequestStartPage.js#L151

The issue here is within isAllowedToCreateRequest https://github.com/Expensify/App/blob/ee5ab95f2c893738fb48756a9002e9d9dadd9a31/src/pages/iou/request/IOURequestStartPage.js#L116 as mentioned on the comment:

// Allow the user to create the request if we are creating the request in global menu or the report can create the request

So isAllowedToCreateRequest uses ReportUtils.canCreateRequest to check if we are creating the request in global menu. But we are not checking if we are creatring the request from the shortcut.

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

We need to check if we are creatring the request from the shortcut.

Inside IOURequestStartPage we need to subscribe to Onyx quickAction

export  default  withOnyx({
// ...
+ quickAction: {
+   key: ONYXKEYS.NVP_QUICK_ACTION_GLOBAL_CREATE,
+ },
})(IOURequestStartPage);

Add quickAction?.action === "splitManual" to isAllowedToCreateRequest

const  isAllowedToCreateRequest  =  _.isEmpty(report.reportID) ||  ReportUtils.canCreateRequest(report, policy, iouType) ||  quickAction?.action  ===  "splitManual";

POC video:

https://github.com/Expensify/App/assets/12425932/e64eda1f-667a-4e4d-88bf-2c6d1be165bf

lschurr commented 5 months ago

Hi @jjcoffee - Could you take a look at the proposals on this one?

jjcoffee commented 5 months ago

Looks like we have an inconsistency here as @bernhardoj's proposal points out. We can create a split with a single user from the FAB, but it doesn't show as an option in a 1:1 chat (from the composer + menu). I would say it makes sense to fix up that consistency, so I'd go with the main solution.

I'm not 100% whether there might be some reason for keeping the Split Bill option out of 1:1 DMs, so asking on Slack just in case!

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 5 months ago

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

jjcoffee commented 5 months ago

Confirmed on Slack that Split bill should be present on 1:1 DMs.

NikkiWines commented 5 months ago

Thanks for double checking @jjcoffee. Agreed that @bernhardoj's proposal looks good πŸ‘

melvin-bot[bot] commented 5 months ago

πŸ“£ @jjcoffee πŸŽ‰ 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[bot] commented 5 months ago

πŸ“£ @bernhardoj πŸŽ‰ 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 πŸ“–

bernhardoj commented 5 months ago

PR is ready

cc: @jjcoffee

jjcoffee commented 5 months ago

PR hit production last week (2024-04-10), so payment should be due tomorrow!

lschurr commented 5 months ago

Thanks for the bump @jjcoffee!

lschurr commented 5 months ago

Payment summary:

lschurr commented 5 months ago

Paid!