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

[$250] AU-With a participant sets zero amount creating split expense displays error #46802

Closed izarutskaya closed 2 weeks ago

izarutskaya commented 1 month 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: 9.0.16 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4815729 Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Open a group chat with few members
  3. Tap plus icon -- split expense
  4. Enter an amount and tap next
  5. Replace the amount to zero for first participant
  6. Tap split expense

Expected Result:

After entering zero as amount for first participant and tapping split must show error. If not, error must not be shown after creating split expense also.

Actual Result:

After entering zero as amount for first participant and tapping split no error is displayed. But unexpected error is shown after creating split expense also.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/4b68e895-ff3c-47fe-8478-132842df284c

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01329020483b531ccb
  • Upwork Job ID: 1823118569609242552
  • Last Price Increase: 2024-08-12
  • Automatic offers:
    • Krishna2323 | Contributor | 103591335
Issue OwnerCurrent Issue Owner: @jayeshmangwani
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

izarutskaya commented 1 month ago

We think this issue might be related to the #vip-split

Krishna2323 commented 1 month ago

Proposal

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

AU-With a participant sets zero amount creating split expense displays error

What is the root cause of that problem?

We are using selectedParticipants instead of splitParticipants. https://github.com/Expensify/App/blob/40a6c763fb4cefb9a15716aaeb009e83e27b8c9a/src/pages/iou/request/step/IOURequestStepConfirmation.tsx#L364

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

Use splitParticipants instead of selectedParticipants.

What alternative solutions did you explore? (Optional)

Krishna2323 commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-08-05 10:56:05 UTC.

Proposal

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

AU-With a participant sets zero amount creating split expense displays error

What is the root cause of that problem?

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

When validating in MoneyRequestConfirmationList, we can check if the current user has a invalid amount and if true we will throw an error, we can create a new error message for that.

https://github.com/Expensify/App/blob/e3a9aa8263338722944c9f6aa259a7160ac373a4/src/components/MoneyRequestConfirmationList.tsx#L409-L433

        if (transaction?.splitShares[currentUserPersonalDetails.accountID] && transaction?.splitShares[currentUserPersonalDetails.accountID]?.amount === 0) {
            setFormError('iou.error.invalidAmountCurrentUser');
            return;
        }

        // OR

        if (transaction?.splitShares[currentUserPersonalDetails.accountID] && !transaction?.splitShares[currentUserPersonalDetails.accountID]?.amount) {
            setFormError('iou.error.invalidAmountCurrentUser');
            return;
        }        

What alternative solutions did you explore? (Optional)

Result

https://github.com/user-attachments/assets/d444a6e6-a10f-4d45-a593-d751a25505a4

Krishna2323 commented 1 month ago

Proposal Updated

daledah commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-08-05 10:55:58 UTC.

Proposal

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

After entering zero as amount for first participant and tapping split no error is displayed. But unexpected error is shown after creating split expense also.

What is the root cause of that problem?

When there's an error, we don't have logic to prevent creating the split bill and show the error message

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

In MoneyRequestConfirmationList add new invalidInputs state to store the index of invalid inputs

when users split the bill within at least one amount is 0, we don't allow users to split the bill and

  1. set the formError and invalidInputs state
  2. When users change the amount, we should update invalidInputs to remove the current input. If invalidInputs is empty, we should reset formError -> the formError message will be dismissed
  3. We don't have any logic to set error for MoneyRequestAmountInput

-> we can add the new isError prop (getting from invalidInputs) to MoneyRequestAmountInput, so when there's an error, the input will show the red bottom border

What alternative solutions did you explore? (Optional)

Krishna2323 commented 1 month ago

Proposal Updated

alexpensify commented 1 month ago

It's still on my testing list; I'll get to it soon.

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~01329020483b531ccb

melvin-bot[bot] commented 1 month ago

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

alexpensify commented 1 month ago

@jayeshmangwani - can you please review the proposals and identify if one will fix this issue? Thanks!

jayeshmangwani commented 1 month ago

Thanks for the proposal, If we decide to show an error message when a logged-in user sets their own split amount to 0, we can proceed with @Krishna2323 's Proposal.

Sidenote: We'll need the error message copies to display that the split amount cannot be set to 0 for oneself.

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 month ago

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

daledah commented 1 month ago

@jayeshmangwani Can you pls confirm the expectation here?

  1. The error message above Split expense button should be shown only when users press on it
  1. Replace the amount to zero for first participant
  2. Tap split expense
  1. we should show the error indicator below the invalid input, so users know where they do incorrectly.

cc @srikarparsi

jayeshmangwani commented 1 month ago

The error message above Split expense button should be shown only when users press on it

IMO, the error should appear above the form button, not when the user presses it. Instead, the error should trigger when the user changes the amount to 0. Also, @srikarparsi what's your take on the behavior when the split amount is set to 0 for oneself?

Screenshot 2024-08-14 at 14 41 50
jayeshmangwani commented 1 month ago

@srikarparsi Whenever you can, please check these comments https://github.com/Expensify/App/issues/46802#issuecomment-2288262240 and https://github.com/Expensify/App/issues/46802#issuecomment-2285950860

Krishna2323 commented 1 month ago

@srikarparsi, friendly bump ^

melvin-bot[bot] commented 1 month ago

@alexpensify @jayeshmangwani @srikarparsi 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!

srikarparsi commented 1 month ago

Yes, I agree with what you've said @jayeshmangwani, sorry for the delay. In the future we will want to allow for setting your own split to 0 but that is de-prioritized so this frontend fix will be good in the meanwhile to avoid this bug. Internal discussion here

The error should appear above the Split Expense button as it does for other errors. The error should appear immediately after the user enters their own split amount as 0 (should not wait for the user to press the split expense button).

@Krishna2323's proposal looks good to me. @alexpensify, do you think you could help with getting the english and spanish copies for the error text? Something on the lines of "Split amount for yourself must be greater than 0"

melvin-bot[bot] commented 1 month ago

πŸ“£ @Krishna2323 πŸŽ‰ 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[bot] commented 1 month ago

@alexpensify, @jayeshmangwani, @srikarparsi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Krishna2323 commented 1 month ago

PR will be up today.

Krishna2323 commented 1 month ago

@jayeshmangwani, PR ready for review ^

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @VictoriaExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

alexpensify commented 1 month ago

Next Steps

This PR is going through the review process. @VictoriaExpensify I reassigned the Bug label because @Krishna2323 needs help getting the translations (https://github.com/Expensify/App/pull/47886#issuecomment-2307448608) for this update. Can you please help move this GH along while I'm OOO? Thanks!

Heads up, I will be offline until Tuesday, September 3, 2024, and will not actively watch over this GitHub during that period.

If anything urgent is needed here, please ask for help in the #expensify-open-source Slack Room-- thanks!

jayeshmangwani commented 1 month ago

needs help getting the translations (https://github.com/Expensify/App/pull/47886#issuecomment-2307448608) for this update

@VictoriaExpensify Can you please help getting the translations ?

Krishna2323 commented 1 month ago

@jayeshmangwani, could you please start a thread in slack? I can't do that as I'm not in the channel.

jayeshmangwani commented 1 month ago

@Krishna2323 I've opened the thread here

jayeshmangwani commented 3 weeks ago

@Krishna2323 Since you don't have access to Slack, here's an update: We have the English copy, Please enter a non-zero amount for your split., Srikar is working on getting us the Spanish copy.

Krishna2323 commented 3 weeks ago

@jayeshmangwani, any updates on the Spanish copy?

jayeshmangwani commented 3 weeks ago

@Krishna2323 There's no update on the slack thread.

jayeshmangwani commented 3 weeks ago

@srikarparsi should I use this GPT to get the Spanish copy and then post it on Slack for confirmation from someone on the team?

srikarparsi commented 3 weeks ago

Hey! Sorry for the delay, I asked internally in Slack and just waiting on a response. I'll update once I hear back.

srikarparsi commented 3 weeks ago

Hey, here's the spanish translation:

Por favor, introduce una cantidad diferente de cero para tu parte.

alexpensify commented 3 weeks ago

Thanks, @VictoriaExpensify, for your help! I'm back online and taking this GH back.

jayeshmangwani commented 3 weeks ago

Hey, here's the spanish translation:

Thanks for the translations!

@Krishna2323 We now have the Spanish version as well. Please update it.

jayeshmangwani commented 3 weeks ago

@alexpensify The PR was merged yesterday. I think we should remove the daily tag and replace it with a weekly tag.

alexpensify commented 3 weeks ago

Thanks for the update-- I've manually updated this GH since automation didn't kick in.

alexpensify commented 2 weeks ago

Payouts due: 2024-09-11

Upwork job is here.

alexpensify commented 2 weeks ago

Closing - I completed the payment process in Upwork for @Krishna2323. @jayeshmangwani please submit a request in Chat. Thanks!

jayeshmangwani commented 2 weeks ago

Requested on ND as per https://github.com/Expensify/App/issues/46802#issuecomment-2344815319

garrettmknight commented 2 weeks ago

$250 approved for @jayeshmangwani