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

thienlnam commented 1 year ago

As part of https://github.com/Expensify/App/pull/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'

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~012cece4afbcaaddc6
Upwork Job ID: 1702626130119409664
Last Price Increase: 2023-09-15

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

📣 @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 commented 1 year ago

Assigning situ to this since he helped review the original PR

hungvu193 commented 1 year ago

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?

In our MoneyRequestParticipantsSelector remove props shouldShowConfirmButton. This will make our footer always shows when an option is selected.

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

            }

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

https://github.com/Expensify/App/assets/16502320/3112f831-2437-41d5-933f-876b11983a21

situchan commented 1 year ago

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

hungvu193 commented 1 year ago

Something like this

https://github.com/Expensify/App/assets/16502320/055da8ec-1bfc-4287-9c4c-089d754fb35a

situchan commented 1 year ago

This is form error example, isn't it?

samh-nl commented 1 year ago

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?

Add a check here:

https://github.com/Expensify/App/blob/d9f35a3b12a0404c4f7db2a458f9c4a6f222b356/src/components/MoneyRequestConfirmationList.js#L359

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(...);
    }
}

Add prop errorText to the ButtonWithDropdownMenu component
Pass the error here:

https://github.com/Expensify/App/blob/d9f35a3b12a0404c4f7db2a458f9c4a6f222b356/src/components/MoneyRequestConfirmationList.js#L408-L412

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

What alternative solutions did you explore? (Optional)

hungvu193 commented 1 year ago

This is form error example, isn't it?

similar to that, I'm using FormHelpMessage

samh-nl commented 1 year ago

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:

situchan commented 1 year ago

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 commented 1 year ago

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

https://github.com/Expensify/App/blob/76c1559fb3fbee1d07c8f210b522b63edd03920e/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js#L247

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:

We need to pass a custom footerContent prop into OptionsSelector component here.
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
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:

https://github.com/Expensify/App/assets/9639873/a7ef377f-6f62-4188-8347-d0dc3d276c36

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[bot] commented 1 year ago

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

situchan commented 1 year ago

reviewing today

situchan commented 1 year ago

@thienlnam can we re-clarify the scope to fix here?

thienlnam commented 1 year ago

We've had to CP various issues (including hiding the button when there is a workspace selected with additional participants) - which was mostly a stopgap to prevent the app from crashing when a user tried to do it

This issue should properly handle this error with the form errors. So it would

Remove the logic for hiding the button (so the button is always visible)
Update it so a form error is shown when you try to select a workspace along with additional participants

situchan commented 1 year ago

ok, and what about selecting workspace with other participants? (i.e. 1 workspace + 1 participant, 2 workspaces) Should we enable them or show error for now?

thienlnam commented 1 year ago

Show an error for now - those flows aren't supported. You can only select a single workspace right now

NicMendonca commented 1 year ago

@situchan do we have everything we need from @thienlnam to move forward with a proposal? Or still waiting?

melvin-bot[bot] commented 1 year ago

@NicMendonca @thienlnam @situchan this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

NicMendonca commented 1 year ago

@situchan is reviewing this today - https://expensify.slack.com/archives/C02NK2DQWUX/p1696258170330759?thread_ts=1696258116.084179&cid=C02NK2DQWUX

NicMendonca commented 1 year ago

@situchan bumped you here: https://expensify.slack.com/archives/C02NK2DQWUX/p1696542533862449

melvin-bot[bot] commented 1 year ago

@NicMendonca @thienlnam @situchan this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

hoangzinh commented 1 year ago

Hey team, It seems it's fixed somewhere, at the moment we can't split a bill with either multiple workspaces or a workspace and additional participants.

https://github.com/Expensify/App/assets/9639873/6ed7a2de-1c34-4c78-a993-6ac6db70f71e

melvin-bot[bot] commented 1 year ago

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

thienlnam commented 1 year ago

It's fixed, but we need to update it so that it works within our form errors

https://github.com/Expensify/App/issues/27508#issuecomment-1732772409

hungvu193 commented 1 year ago

Updated my proposal to meet the requirement https://github.com/Expensify/App/issues/27508#issuecomment-1732772409

situchan commented 1 year ago

One more thing to confirm: should we also prevent hiding button when nothing is selected and instead show error like "Please select participants" on button click? cc: @thienlnam

https://github.com/Expensify/App/assets/108292595/5eb4285d-c4da-4882-9529-afc0221ad746

To be consistent with distance request in https://github.com/Expensify/App/pull/29125:

thienlnam commented 1 year ago

One more thing to confirm: should we also prevent hiding button when nothing is selected and instead show error like "Please select participants" on button click?

This is an interesting one, I think the current behavior is fine since you don't have to split with anyone and can just select a name.

melvin-bot[bot] commented 1 year ago

@NicMendonca @thienlnam @situchan this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Decide whether any proposals currently meet our guidelines and can be approved as-is today
If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 1 year ago

Current assignee @situchan is eligible for the Internal assigner, not assigning anyone new.

situchan commented 1 year ago

@hungvu193 from your video, I still see that form error position is misaligned. It should be above button similar to distance page I shared in https://github.com/Expensify/App/issues/27508#issuecomment-1754568833

hungvu193 commented 1 year ago

@hungvu193 from your video, I still see that form error position is misaligned. It should be above button similar to distance page I shared in https://github.com/Expensify/App/issues/27508#issuecomment-1754568833

I see. Will update it shortly

BhuvaneshPatil commented 1 year ago

Proposal

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

Update error flow for preventing splitting bill with workspace and additional participants are selected

What is the root cause of that problem?

In current implementation we are hiding the confirm button when there are participants other than workspace https://github.com/Expensify/App/blob/f75b1ef29cc588f516ff42856353f021df5401c0/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js#L242

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

To show the confirm button, we can add button and error message to footerContent prop of OptionsSelector

footerContent={
                isAllowedToSplit ? (
                    <View>
                        {hasError ? (
                            <DotIndicatorMessage
                                style={[styles.mh4, styles.mv3]}
                                messages={getError()}
                                type="error"
                            />
                        ) : null}
                        <Button
                            success
                            style={[styles.w100]}
                            text={translate('common.confirm')}
                            onPress={navigateNext}
                            pressOnEnter
                            enterKeyEventListenerPriority={1}
                        />
                    </View>
                ) : null
            }

We will create a new state, hasError, that will be set to true once user tries to submit/go next. This ensures the flow where we display the error when user clicks the next button (similar to distance request)

We create the getError method, that will return the error

const getError = () => {
    if (_.keys(participants).length > 1 && hasPolicyExpenseChatParticipant) {
        return {0: "Can't split with workspace when other participants are selected"};
    }
};

Message can be changed.

Then we create a function that will executed when we press the next button, it will check for error and set the state hasError to true if there is error and return, else it will call navigateToSplit()
We also add new useEffect, that will be called when we change the participants and set the hasError to false.

https://github.com/Expensify/App/assets/27822551/5a1eb11a-f9fa-41c1-a97c-ae962f2741ca

What alternative solutions did you explore? (Optional)

NicMendonca commented 1 year ago

@situchan proposals here! ^

melvin-bot[bot] commented 1 year ago

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

situchan commented 1 year ago

I will approve on Monday.

situchan commented 1 year ago

I think we should go with @hoangzinh's proposal as they first proposed the right solution. 🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 year ago

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

thienlnam commented 1 year ago

Great, thanks!

hoangzinh commented 1 year ago

@thienlnam could you add Waiting for copy label and help to confirm the translation? Thanks

English	Spanish
Split bill between either workspaces or a workspace and individual users are not allowed	No se permite dividir la factura entre espacios de trabajo o entre un espacio de trabajo y usuarios individuales.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @LLPeckham (Waiting for copy), see https://stackoverflow.com/c/expensify/questions/7025/ for more details.

thienlnam commented 1 year ago

Hey @LLPeckham 👋 We just need some help confirming the copy in this message https://github.com/Expensify/App/issues/27508#issuecomment-1779629757

You can only split a bill with a workspace, OR split a bill with participants but you cannot split a bill with a workspace and then a bunch of participants

LLPeckham commented 1 year ago

Heya! Ok just want to confirm - is the error message that's being proposed going to show up here:

And then, also confirming that someone can split bill with a workspace but will get that error if they try to add additional attendees?

In terms of error copy, what about something like:

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

Might be too long, so just LMK and I can try to re-work.

hoangzinh commented 1 year ago

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 commented 1 year ago

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

Expensify / App