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

[$250] [HOLD for payment 2024-05-06] Group split scan - Error appears for Merchant when clicking Split expense for the second time #40749

Closed
6 tasks done
kavimuru opened this issue Apr 23, 2024 · 30 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Apr 23, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.64-0
Reproducible in staging?: y
Reproducible in production?: n
If this was caught during regression testing, add the test name, ID and link from TestRail: n/a
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to FAB > Split expense > Scan.
  3. Create split scan expense with a few users.
  4. In group chat, click on the split preview.
  5. Click Split expense.
  6. Click Split expense again.

Expected Result:

There will be no error under Merchant field because Merchant is not a requirement for non-workspace expense.

Actual Result:

Error shows up under Merchant field when clicking Split expense for the second time.

Workaround:

unknown

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

Bug6458020_1713821712347.20240423_053140.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013158f9aa7567f244
  • Upwork Job ID: 1790113061570207744
  • Last Price Increase: 2024-05-13
Issue OwnerCurrent Issue Owner: @
@kavimuru kavimuru added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 23, 2024
Copy link

melvin-bot bot commented Apr 23, 2024

Triggered auto assignment to @robertjchen (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link

melvin-bot bot commented Apr 23, 2024

Triggered auto assignment to @johncschuster (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.

Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@kavimuru
Copy link
Author

@johncschuster FYI 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.

@kavimuru
Copy link
Author

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

@allgandalf
Copy link
Contributor

Proposal

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

Error appears for merchant even when it is not required for non workspace expenses

What is the root cause of that problem?

This is a regression from #38543, Here we have added a new condition to display merchant error:

if ((isMerchantRequired && isMerchantEmpty) || (shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction ?? null))) {
setMerchantError(true);
return;

The second part of this condition would return true when we the merchant is empty and shouldDisplayFieldError would be set to true the first time we click on submit button because the amount field is empty.

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

We need to add a extra && check of isMerchantRequired, so that we only set the error to true if the merchant is required:

diff --git a/src/components/MoneyRequestConfirmationList.tsx b/src/components/MoneyRequestConfirmationList.tsx
index 99901dd261..87d605a7a5 100755
@@ -577,7 +577,7 @@ function MoneyRequestConfirmationList({
             if (selectedParticipants.length === 0) {
                 return;
             }
-            if ((isMerchantRequired && isMerchantEmpty) || (shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction ?? null))) {
+            if ((isMerchantRequired && isMerchantEmpty) || (isMerchantRequired && shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction ?? null))) {
                 setMerchantError(true);
                 return;
             }

What alternative solutions did you explore? (Optional)

@saifelance
Copy link

@GandalfGwaihir
We should modify the condition to ensure that the error is set to true only if the merchant is required. So, an additional && check for isMerchantRequired needs to be added. This adjustment ensures that the error is set only when the merchant is both required and empty:

`diff --git a/src/components/MoneyRequestConfirmationList.tsx b/src/components/MoneyRequestConfirmationList.tsx
index 99901dd261..87d605a7a5 100755
@@ -577,7 +577,7 @@ function MoneyRequestConfirmationList({
if (selectedParticipants.length === 0) {
return;
}

  •        if ((isMerchantRequired && isMerchantEmpty) || (shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction ?? null))) {
    
  •        if ((isMerchantRequired && isMerchantEmpty) || (isMerchantRequired && shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction ?? null))) {
               setMerchantError(true);
               return;
           }
    

`

@mountiny mountiny added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Apr 23, 2024
@mountiny
Copy link
Contributor

This is minor UX issue, demoting @koko57 seems like its coming from your PR

@koko57
Copy link
Contributor

koko57 commented Apr 23, 2024

@mountiny As the follow-up PR wasn't merged yet I can fix it there

@koko57
Copy link
Contributor

koko57 commented Apr 24, 2024

@mountiny PR opened: #40868

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 Weekly KSv2 labels Apr 24, 2024
@mountiny
Copy link
Contributor

mountiny commented May 9, 2024

LEts just go through upwork until ND is ready for you as it would create a backlog

@johncschuster johncschuster added the External Added to denote the issue can be worked on by a contributor label May 13, 2024
Copy link

melvin-bot bot commented May 13, 2024

Job added to Upwork: https://www.upwork.com/jobs/~013158f9aa7567f244

@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-05-06] Group split scan - Error appears for Merchant when clicking Split expense for the second time [$250] [HOLD for payment 2024-05-06] Group split scan - Error appears for Merchant when clicking Split expense for the second time May 13, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 13, 2024
Copy link

melvin-bot bot commented May 13, 2024

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

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels May 13, 2024
@johncschuster johncschuster removed Help Wanted Apply this label when an issue is open to proposals by contributors Daily KSv2 labels May 13, 2024
@johncschuster
Copy link
Contributor

Applying the External label to create the job. @Ollyws, Upwork isn't returning you when I search for you because... Upwork 🙄 . Can you apply to the job and ping me? I'll get payment issued ASAP!

@melvin-bot melvin-bot bot removed the Overdue label May 13, 2024
@johncschuster
Copy link
Contributor

johncschuster commented May 14, 2024

@Ollyws Aha! I found you. Your offer is here. Can you accept that?

Edit: This offer has been withdrawn so you can request via ND.

@melvin-bot melvin-bot bot added the Monthly KSv2 label May 14, 2024
@johncschuster
Copy link
Contributor

@Ollyws, I see you've just been added to the list of contributors who can accept payment via NewDot. I'll go ahead and post a payment summary instead so you can be paid via that method.

@johncschuster
Copy link
Contributor

Payment Summary

Contributor+: @Ollyws - $250 via Manual Request

@johncschuster
Copy link
Contributor

@robertjchen can you complete the BZ checklist above?

@Ollyws
Copy link
Contributor

Ollyws commented May 17, 2024

Requested in ND, thanks!

@JmillsExpensify
Copy link

$250 approved for @Ollyws

@robertjchen
Copy link
Contributor

Working through checklist

@robertjchen robertjchen added Weekly KSv2 and removed Monthly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production labels May 30, 2024
@melvin-bot melvin-bot bot added the Overdue label Jun 7, 2024
@johncschuster
Copy link
Contributor

Bumping this for Melvin. How's the checklist going, Robert?

@melvin-bot melvin-bot bot removed the Overdue label Jun 7, 2024
@robertjchen
Copy link
Contributor

@johncschuster Made my assessment, and left the last item for you 👍

@johncschuster
Copy link
Contributor

Thanks @robertjchen! Looks like we can close this up!

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. Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants