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

[Issue Payment] [$500] Update error flow for prevent splitting bill with workspace and additional participants #27508

Closed
thienlnam opened this issue Sep 15, 2023 · 67 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@thienlnam
Copy link
Contributor

thienlnam commented Sep 15, 2023

As part of #25564, we merged together split bill and request money. However, now you have the ability to split a bill with a workspace as a participant along with other members. This is not supported, and needs to be cleaned up.

We talked about it here: https://expensify.slack.com/archives/C01GTK53T8Q/p1694770696392489?thread_ts=1694746421.663479&cid=C01GTK53T8Q

Let's add a form error that prevents you from having multiple participants that include a workspace when you try to hit 'Split Bill'
Screenshot 2023-09-15 at 6 10 45 PM

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012cece4afbcaaddc6
  • Upwork Job ID: 1702626130119409664
  • Last Price Increase: 2023-09-15
@thienlnam thienlnam added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 15, 2023
@thienlnam thienlnam self-assigned this Sep 15, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 2023

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

@thienlnam thienlnam added the External Added to denote the issue can be worked on by a contributor label Sep 15, 2023
@melvin-bot melvin-bot bot changed the title Prevent splitting bill with workspace and additional participants [$500] Prevent splitting bill with workspace and additional participants Sep 15, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 2023

Job added to Upwork: https://www.upwork.com/jobs/~012cece4afbcaaddc6

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 15, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 2023

Current assignee @NicMendonca is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 2023

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

@thienlnam thienlnam assigned situchan and unassigned fedirjh Sep 15, 2023
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 15, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 2023

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

@thienlnam
Copy link
Contributor Author

Assigning situ to this since he helped review the original PR

@hungvu193
Copy link
Contributor

hungvu193 commented Sep 15, 2023

Proposal

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

Prevent splitting bill with workspace and additional participants

What is the root cause of that problem?

We're only hiding the confirm button if our we're splitting with workspace, we need to update this logic.

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

  1. In our MoneyRequestParticipantsSelector remove props shouldShowConfirmButton. This will make our footer always shows when an option is selected.
  2. Add a footer content with confirm button and FormHelperMessage that helps displaying the error message if needed.
                footerContent={
                    isAllowedToSplit ? <View>
                    <Button
                        success
                        style={[styles.w100]}
                        text={translate('common.confirm')}
                        onPress={navigateToSplit}
                        pressOnEnter
                        enterKeyEventListenerPriority={1}
                    />
                    {!shouldShowConfirmButton && <FormHelpMessage message="iou.error.invalidSplitWS" />}
                </View> : null
                   
                }
  1. Create a wrapper function for our navigateToSplit, if there's error (isAllowedToSplit or shouldShowConfirmButton are false), we should early return instead of calling navigateToSplit.

What alternative solutions did you explore? (Optional)

Similar above but instead of having constant message, we should introduce 2 new state:

const [hasError, setHasError] = useState(false);
const [errorMessage, setErrorMessage] = useState('');

Add an validation on user selecting the option and set the error based on that.

Result
Screen.Recording.2023-10-10.at.13.46.37.mp4

@situchan
Copy link
Contributor

Please share demo video with form error displayed. Message can be dummy for now. (We need to get copy)

@hungvu193
Copy link
Contributor

hungvu193 commented Sep 15, 2023

Something like this

Screen.Recording.2023-09-15.at.18.01.51.mov

@situchan
Copy link
Contributor

This is form error example, isn't it?
Screenshot 2023-09-15 at 5 03 26 PM

@samh-nl
Copy link
Contributor

samh-nl commented Sep 15, 2023

Proposal

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

Prevent splitting bill with workspace and additional participants

What is the root cause of that problem?

No check is performed to exclude the possibility of a workspace being involved when there are multiple participants.

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

  1. Add a check here:

const confirm = useCallback(

To ensure that if there is more than 1 participant, a workspace cannot be included.

if (props.iouType === CONST.IOU.MONEY_REQUEST_TYPE.SPLIT) {
    if (selectedParticipants.length > 1 && _.some(selectedParticipants, ReportUtils.isPolicyExpenseChat)) {
        setError(...);
    }
}
  1. Add prop errorText to the ButtonWithDropdownMenu component
  2. Pass the error here:

<ButtonWithDropdownMenu
isDisabled={shouldDisableButton}
onPress={(_event, value) => confirm(value)}
options={splitOrRequestOptions}
buttonSize={CONST.DROPDOWN_BUTTON_SIZE.LARGE}

  1. In ButtonWithDropdownMenu, render <FormHelpMessage message={props.errorText} /> if the error is set.

What alternative solutions did you explore? (Optional)

@hungvu193
Copy link
Contributor

This is form error example, isn't it? Screenshot 2023-09-15 at 5 03 26 PM

similar to that, I'm using FormHelpMessage

@samh-nl
Copy link
Contributor

samh-nl commented Sep 15, 2023

To ensure I'm reading the spec correctly, splitting with a workspace is possible as long as there are no other participants, right?

Also, selecting a workspace leads to an empty row (but looking at the OP printscreen this is a known issue) and all workspaces get automatically selected:

image

@situchan
Copy link
Contributor

To ensure I'm reading the spec correctly, splitting with a workspace is possible as long as there are no other participants, right?

right

@hoangzinh
Copy link
Contributor

hoangzinh commented Sep 16, 2023

Proposal

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

Prevent splitting bill with workspace and additional participants

Actually, I'm not really like the idea of show form error if we're splitting bill with workspace and additional participants. I like the idea of @thienlnam about hide the "Split" button for WS here https://expensify.slack.com/archives/C01GTK53T8Q/p1694769296727379?thread_ts=1694746421.663479&cid=C01GTK53T8Q
But if it's a short term solution, then I will try to give proposal for it

What is the root cause of that problem?

Currently, in MoneyRequestParticipantsSelector component, we haven't check if we're splitting bill with workspace and additional participants yet. We're using default behavior of OptionsSelector, that means it will navigate to Split page if user click on the "Add to split" button

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

All previous proposals would like to show error in the Split bill page, but I think we should stop user go to next step in the selecting participants page. In order to do that:

  1. We need to pass a custom footerContent prop into OptionsSelector component here.
  2. In footerContent custom prop, we can build with a FormHelpMessage + Button component, the onPress of Button need to check whether participants greater than 1 and it has any WS chat, then it will show the FormHelpMessage with the proper error message text
  3. When we add a new participant here, we need to pass isPolicyExpenseChat: option.isPolicyExpenseChat, reportID: option.reportID as well, same as we did here, so we can use those new data to check in the 2nd point above.

Demo:

Screen.Recording.2023-09-16.at.17.49.27.mp4

What alternative solutions did you explore? (Optional)

Hide the split button on the right of all WS chat as @thienlnam suggested here https://expensify.slack.com/archives/C01GTK53T8Q/p1694769296727379?thread_ts=1694746421.663479&cid=C01GTK53T8Q, from begining, not just after user selects a participant

@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 2023

@NicMendonca, @thienlnam, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@hoangzinh
Copy link
Contributor

Split bill between a workspace and additional users is not allowed. Please select a single workspace or individual users.

@LLPeckham does it include the case of split bill with more than 1 workspace? (I.e user adds workspace A and workspace B to list split bill participants)

@LLPeckham
Copy link
Contributor

@hoangzinh - I must be misunderstanding what the error is here as I'm not up to speed on all the comments in this GH. Are we basically saying someone is not allowed to split bill between a workspace and select additional users, but they can select two workspaces at the same time?

@hoangzinh
Copy link
Contributor

but they can select two workspaces at the same time?

Ah, they can't in this case.

@LLPeckham
Copy link
Contributor

Ah ok, can you please let me know what the error is that we're writing the copy for? I just want to understand what the user would be trying to do that they can't do so that I can propose the right copy.

So it sounds like when they split a bill they can only:

  • Select one workspace at a time
    OR
  • Select individual users only

They cannot select more than one workspace, and they cannot select a workspace and additional individual users?

If that is correct, then my copy suggestion above should make sense, right?

Split bill between a workspace and additional users is not allowed. Please select a single workspace or individual users.

@situchan
Copy link
Contributor

  • single workspace ✅
  • multiple workspaces 🚫
  • single user ✅
  • multiple users ✅
  • mix of workspace + user 🚫

@LLPeckham
Copy link
Contributor

LLPeckham commented Oct 26, 2023

Ok, I think I see what you were saying in your comment now thanks for bearing with me! -- to account for if someone chose either two workspaces, or a workspace and additional users, what about something like:

Split bill is only allowed between a single Workspace or individual users. Please update your selection.

Also - if it's possible to tailor the error message then this would be better:

  • If someone selects a Workspace and additional users: Split bill is only allowed between a single Workspace or individual users. Please update your selection.
  • If someone selects two or more Workspaces: Split bill is only allowed between a single Workspace. Please remove additional Workspaces.

@LLPeckham
Copy link
Contributor

@thienlnam - LMK if you think this reads ok and we can get the Spanish translation as well.

Also just checking with the rest of the marketing team if we want to capitalize "Workspaces" or not - so will come back once I get a consensus there.

@thienlnam
Copy link
Contributor Author

That looks great!

@LLPeckham
Copy link
Contributor

Alright, @thienlnam @hoangzinh - I went to get the Spanish translation and @iwiznia had some good thoughts here as this error message and the experience is still pretty confusing. So rather than actually writing an error message, Ioni suggested that we do two things to get rid of the need to even show an error:

  • Scenario 1: If someone selects split bill with a workspace then we don't allow them to select anything else (aka individuals and other workspaces are greyed out/not shown) and just move on to the next screen.
  • Scenario 2: If someone selected to split a bill with individuals then we don't show them workspaces to split with, and they can select individual participants and move on to the next screen.

This way, there is no need for an error at all and is less confusing for the user.

What do you think about this change rather than showing that error message?

@melvin-bot melvin-bot bot added the Overdue label Oct 30, 2023
@LLPeckham
Copy link
Contributor

Bumping @thienlnam @hoangzinh

@melvin-bot melvin-bot bot removed the Overdue label Oct 30, 2023
@hoangzinh
Copy link
Contributor

@LLPeckham it looks like the same thought as @thienlnam raised here https://expensify.slack.com/archives/C01GTK53T8Q/p1694770873084719?thread_ts=1694746421.663479&cid=C01GTK53T8Q. Do you mind to repost your comment above in that Slack thread and we continue to discuss on it?

@thienlnam
Copy link
Contributor Author

thienlnam commented Oct 30, 2023

I'm going to comment in that thread (internal) - we decided not to do that because it's not consistent with our existing error flows

@LLPeckham
Copy link
Contributor

English: Split bill is only allowed between a single workspace or individual users. Please update your selection.
Spanish: Solo puedes dividir una cuenta entre un único espacio de trabajo o con usuarios individuales. Por favor actualiza tu selección.

@LLPeckham LLPeckham removed the Waiting for copy User facing verbiage needs polishing label Oct 31, 2023
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Oct 31, 2023
@hoangzinh
Copy link
Contributor

It looks like Melvin didn't process the next step for this issue. The PR #30302 has been deployed to PROD 10/11/2023

Screenshot 2023-11-19 at 08 00 43

cc @NicMendonca

@NicMendonca NicMendonca added Daily KSv2 and removed Weekly KSv2 labels Dec 4, 2023
@NicMendonca NicMendonca changed the title [$500] Update error flow for prevent splitting bill with workspace and additional participants [Issue Payment] [$500] Update error flow for prevent splitting bill with workspace and additional participants Dec 4, 2023
@NicMendonca NicMendonca added the Awaiting Payment Auto-added when associated PR is deployed to production label Dec 4, 2023
@NicMendonca
Copy link
Contributor

@situchan just re-sent you the offer

@situchan
Copy link
Contributor

situchan commented Dec 4, 2023

@NicMendonca accepted thanks

This issue meets bonus condition:

  • PR created before announcement
  • PR merged in 3 business days (Oct 31-Nov 2) considering Waiting for copy period

@NicMendonca
Copy link
Contributor

@hoangzinh can you please apply here? https://www.upwork.com/jobs/~01637d93b3c12105d2

@hoangzinh
Copy link
Contributor

Applied. Thanks @NicMendonca

@NicMendonca
Copy link
Contributor

all set here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

9 participants