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

[PAID] [$500] Expense - "Enter a merchant" is still shown when the expense is paid #34415

Closed
3 of 6 tasks
kavimuru opened this issue Jan 12, 2024 · 24 comments
Closed
3 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Jan 12, 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: v1.4.24-4
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

  1. [Member] Go to workspace chat and create a scan request.
  2. [Member] Manually enter the amount of the scanning request, while leaving Merchant empty.
  3. [Admin] Pay the request.
  4. [Admin] Navigate to request details view.

EXPECTED RESULT

The Merchant row will not show "Enter a merchant" since admin can no longer edit the paid expense.

ACTUAL RESULT

The Merchant row shows "Enter a merchant" with a red dot when there is no way to edit merchant for a paid expense

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

Bug6339900_1705050248772.bandicam_2024-01-12_02-47-53-164.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018efb2cb983d9645c
  • Upwork Job ID: 1749775068612321280
  • Last Price Increase: 2024-01-23
  • Automatic offers:
    • c3024 | Reviewer | 28128948
    • DylanDylann | Contributor | 28128950
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 12, 2024
Copy link

melvin-bot bot commented Jan 12, 2024

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

@DylanDylann
Copy link
Contributor

DylanDylann commented Jan 12, 2024

Proposal

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

The Merchant row shows "Enter a merchant" with a red dot when there is no way to edit merchant for a paid expense

What is the root cause of that problem?

error={hasErrors && isEmptyMerchant ? translate('common.error.enterMerchant') : ''}

In here we don't check if the request money is paid or not

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

We shouldn't show error if the request is paid or cancelled

error={hasErrors && isEmptyMerchant ? translate('common.error.enterMerchant') : ''}

error={!isSettled && !isCancelled && hasErrors && isEmptyMerchant ? translate('common.error.enterMerchant') : ''}

We also update the same thing with brickRoadIndicator

brickRoadIndicator={hasViolations('merchant') || (hasErrors && isEmptyMerchant) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}

brickRoadIndicator={hasViolations('merchant') || (!isSettled && !isCancelled && hasErrors && isEmptyMerchant) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}

Or (depend on the expected)

brickRoadIndicator={!isSettled && !isCancelled && (hasViolations('merchant') || (hasErrors && isEmptyMerchant)) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}

What alternative solutions did you explore? (Optional)

NA

@strepanier03
Copy link
Contributor

Hmmm, gonna think about this one and likely raise it, then circle back Tuesday next week when I'm back in office.

@melvin-bot melvin-bot bot added the Overdue label Jan 15, 2024
Copy link

melvin-bot bot commented Jan 16, 2024

@strepanier03 Whoops! This issue is 2 days overdue. Let's get this updated quick!

@strepanier03
Copy link
Contributor

Update tomorrow.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 17, 2024
@strepanier03
Copy link
Contributor

All right, repro'd and raised in Slack here.

image

@melvin-bot melvin-bot bot removed the Overdue label Jan 22, 2024
@strepanier03 strepanier03 changed the title Expense - "Enter a merchant" is still shown when the expense is paid [Hot Picks] Expense - "Enter a merchant" is still shown when the expense is paid Jan 22, 2024
@strepanier03
Copy link
Contributor

All right, tied to Wave 5 and discussion in Slack produced good outcome.

tl;dr: Rather than editing the message, we shouldn't be allowing an expense to be paid without a merchant.

@dylanexpensify dylanexpensify added Hot Pick Ready for an engineer to pick up and run with External Added to denote the issue can be worked on by a contributor labels Jan 23, 2024
@melvin-bot melvin-bot bot changed the title [Hot Picks] Expense - "Enter a merchant" is still shown when the expense is paid [$500] [Hot Picks] Expense - "Enter a merchant" is still shown when the expense is paid Jan 23, 2024
Copy link

melvin-bot bot commented Jan 23, 2024

Job added to Upwork: https://www.upwork.com/jobs/~018efb2cb983d9645c

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

melvin-bot bot commented Jan 23, 2024

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

@flodnv flodnv changed the title [$500] [Hot Picks] Expense - "Enter a merchant" is still shown when the expense is paid [$500] Expense - "Enter a merchant" is still shown when the expense is paid Jan 23, 2024
@Pujan92
Copy link
Contributor

Pujan92 commented Jan 23, 2024

Proposal

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

Enter merchant violation displayed even the expense is paid

What is the root cause of that problem?

We don't have any logic to show/hide violation when the expense is paid or cancelled.

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

We can ignore showing any violation when the expense gets paid or cancelled(if required) here.
canUseViolations && !isSettled && !isCancelled

const hasViolations = useCallback((field) => canUseViolations && getViolationsForField(field).length > 0, [canUseViolations, getViolationsForField]);

Also we need to consider the same condition before showing any violation message.

@stitesExpensify stitesExpensify self-assigned this Jan 24, 2024
@stitesExpensify
Copy link
Contributor

Hey @c3024! I know you just got assigned as c+ yesterday but it looks like there are a couple of proposals if you wouldn't mind checking them out!

@c3024
Copy link
Contributor

c3024 commented Jan 24, 2024

I'll check the proposals and update in a few hours.

@c3024
Copy link
Contributor

c3024 commented Jan 25, 2024

@DylanDylann 's proposal here looks good to me. brickRoadIndicator and error fields conditions should include the settled condition and show nothing accordingly.

🎀 👀 🎀 C+ Reviewed

Copy link

melvin-bot bot commented Jan 29, 2024

📣 @c3024 🎉 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 Jan 29, 2024

📣 @DylanDylann 🎉 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 removed the Overdue label Jan 29, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jan 30, 2024
@DylanDylann
Copy link
Contributor

@c3024 The PR is ready for review #35360

@dylanexpensify dylanexpensify moved this from HOT PICKS to Release 3: Migration for All in [#whatsnext] Wave 05 - Deprecate Free Jan 31, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Feb 7, 2024
@melvin-bot melvin-bot bot changed the title [$500] Expense - "Enter a merchant" is still shown when the expense is paid [HOLD for payment 2024-02-14] [$500] Expense - "Enter a merchant" is still shown when the expense is paid Feb 7, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Feb 7, 2024
Copy link

melvin-bot bot commented Feb 7, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Feb 7, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.37-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-02-14. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Feb 7, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@c3024 / @DylanDylann] The PR that introduced the bug has been identified. Link to the PR:
  • [@c3024 / @DylanDylann] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@c3024 / @DylanDylann] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@c3024 / @DylanDylann] Determine if we should create a regression test for this bug.
  • [@c3024 / @DylanDylann] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@strepanier03] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@strepanier03
Copy link
Contributor

Looks like we're all set for tomorrow. @c3024, feel free to complete the checklist above and I'll do the reg test.

@c3024
Copy link
Contributor

c3024 commented Feb 14, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@c3024] The PR that introduced the bug has been identified. Link to the PR: Display Smartscan errors #26155
  • [@c3024] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: 03f6abe#r138623871
  • [@c3024] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: This is a specific case of settled request missed. A discussion was not started because it does not appear that this could have been identified earlier.
  • [@c3024] Determine if we should create a regression test for this bug. Yes
  • [@c3024] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

  1. Login with admin of a workspace on a device and member of the workspace on another device.
  2. [Member] Go to workspace chat and create a scan request.
  3. [Member] Manually enter the amount of the scanning request, while leaving Merchant empty.
  4. [Admin] Pay the request.
  5. [Admin] Navigate to the money request details view.
  6. Verify that there is no error message nor RBR in merchant field.

👍 or 👎

@strepanier03
Copy link
Contributor

Reg test done and contracts paid and closed. Thanks again @c3024 and @DylanDylann for helping get this one over the finish line!

@strepanier03 strepanier03 changed the title [HOLD for payment 2024-02-14] [$500] Expense - "Enter a merchant" is still shown when the expense is paid [PAID] [$500] Expense - "Enter a merchant" is still shown when the expense is paid Feb 14, 2024
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 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Development

No branches or pull requests

8 participants