Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[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
1 of 6 tasks
kbecciv opened this issue Apr 2, 2024 · 19 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Apr 2, 2024

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?

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6435487_1712058302059.35253.mp4

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
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 2, 2024
Copy link

melvin-bot bot commented Apr 2, 2024

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

@kbecciv kbecciv changed the title Split bill - Split bill shortcut directs to 404 page after splitting a bill with one account Web - Split bill - Split bill shortcut directs to 404 page after splitting a bill with one account Apr 2, 2024
@kbecciv
Copy link
Author

kbecciv commented Apr 2, 2024

@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
Copy link
Author

kbecciv commented Apr 2, 2024

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

@bernhardoj
Copy link
Contributor

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.

const isAllowedToCreateRequest = _.isEmpty(report.reportID) || ReportUtils.canCreateRequest(report, policy, iouType);

And that's because getMoneyRequestOptions doesn't include the split bill option for 1:1 DM.

App/src/libs/ReportUtils.ts

Lines 4989 to 4995 in d7161ae

function canCreateRequest(report: OnyxEntry<Report>, policy: OnyxEntry<Policy>, iouType: (typeof CONST.IOU.TYPE)[keyof typeof CONST.IOU.TYPE]): boolean {
const participantAccountIDs = report?.participantAccountIDs ?? [];
if (!canUserPerformWriteAction(report)) {
return false;
}
return getMoneyRequestOptions(report, policy, participantAccountIDs).includes(iouType);
}

App/src/libs/ReportUtils.ts

Lines 4802 to 4809 in d7161ae

if (
(isChatRoom(report) && otherParticipants.length > 0) ||
(isDM(report) && hasMultipleOtherParticipants) ||
(isGroupChat(report) && otherParticipants.length > 0) ||
(isPolicyExpenseChat(report) && report?.isOwnPolicyExpenseChat)
) {
options = [CONST.IOU.TYPE.SPLIT];
}

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.

@lschurr lschurr added the External Added to denote the issue can be worked on by a contributor label Apr 2, 2024
@melvin-bot melvin-bot bot changed the title Web - Split bill - Split bill shortcut directs to 404 page after splitting a bill with one account [$500] Web - Split bill - Split bill shortcut directs to 404 page after splitting a bill with one account Apr 2, 2024
Copy link

melvin-bot bot commented Apr 2, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 2, 2024
Copy link

melvin-bot bot commented Apr 2, 2024

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

@dragnoir
Copy link
Contributor

dragnoir commented Apr 2, 2024

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

<FullPageNotFoundView shouldShow={!IOUUtils.isValidMoneyRequestType(iouType) || !isAllowedToCreateRequest}>

The issue here is within isAllowedToCreateRequest

const isAllowedToCreateRequest = _.isEmpty(report.reportID) || ReportUtils.canCreateRequest(report, policy, iouType);

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:

20240402_213028.mp4

@lschurr
Copy link
Contributor

lschurr commented Apr 2, 2024

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

@arielgreen arielgreen changed the title [$500] Web - Split bill - Split bill shortcut directs to 404 page after splitting a bill with one account [LOW] [Splits] [$500] Web - Split bill - Split bill shortcut directs to 404 page after splitting a bill with one account Apr 2, 2024
@jjcoffee
Copy link
Contributor

jjcoffee commented Apr 4, 2024

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!

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Apr 4, 2024

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

@jjcoffee
Copy link
Contributor

jjcoffee commented Apr 4, 2024

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

@NikkiWines
Copy link
Contributor

Thanks for double checking @jjcoffee. Agreed that @bernhardoj's proposal looks good 👍

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 4, 2024
Copy link

melvin-bot bot commented Apr 4, 2024

📣 @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

Copy link

melvin-bot bot commented Apr 4, 2024

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

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Apr 5, 2024
@bernhardoj
Copy link
Contributor

PR is ready

cc: @jjcoffee

@jjcoffee
Copy link
Contributor

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

@lschurr
Copy link
Contributor

lschurr commented Apr 16, 2024

Thanks for the bump @jjcoffee!

@lschurr lschurr changed the title [LOW] [Splits] [$500] Web - Split bill - Split bill shortcut directs to 404 page after splitting a bill with one account [Hold for payment 04-17-2024][$500] Web - Split bill - Split bill shortcut directs to 404 page after splitting a bill with one account Apr 16, 2024
@lschurr
Copy link
Contributor

lschurr commented Apr 16, 2024

Payment summary:

@lschurr
Copy link
Contributor

lschurr commented Apr 17, 2024

Paid!

@lschurr lschurr closed this as completed Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

6 participants