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.34k stars 2.77k forks source link

[$250] Dupe detection - Merchant field shows (none) instead of blank when (none) is selected #46998

Open IuliiaHerets opened 1 month ago

IuliiaHerets 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: v9.0.17-1 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to DM.
  3. Submit an expense with no merchant and description.
  4. Submit another expense with the same amount, and with merchant and description.
  5. Go to the transaction thread of the second expense (Step 4).
  6. Click Review duplicates.
  7. Click Keep this one (any).
  8. During the review, select (none) for Merchant and None for Description.
  9. Proceed to confirmation page.

Expected Result:

Merchant field should be empty since none is selected.

Actual Result:

Merchant field shows (none) when (none) is selected, while Description field is empty when None is selected.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/7814aaab-d69c-4d19-905d-b2c005355917

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0194455fbb3684c1f1
  • Upwork Job ID: 1823630303854706776
  • Last Price Increase: 2024-08-21
  • Automatic offers:
    • Krishna2323 | Contributor | 103626733
Issue OwnerCurrent Issue Owner: @eVoloshchak
melvin-bot[bot] commented 1 month ago

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

IuliiaHerets commented 1 month ago

We think that this bug might be related to #wave-collect - Release 1

IuliiaHerets commented 1 month ago

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

Krishna2323 commented 1 month ago

Edited by proposal-police: This proposal was edited at 2023-10-11T18:10:00Z.

Proposal

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

Dupe detection - Merchant field shows (none) instead of blank when (none) is selected

What is the root cause of that problem?

In ReviewMerchant we are not checking if the merchant is merchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT to consider it as empty. https://github.com/Expensify/App/blob/6a6c242e79bb930b58146cb3e31f74d4e1fa0840/src/pages/TransactionDuplicate/ReviewMerchant.tsx#L25-L26

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

Update the condition to:

    const options = useMemo(
        () =>
            compareResult.change.merchant?.map((merchant) =>
                !merchant || merchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT
                    ? {text: translate('violations.none'), value: ''}
                    : {
                          text: merchant,
                          value: merchant,
                      },
            ),
        [compareResult.change.merchant, translate],
    );

What alternative solutions did you explore? (Optional)

Update the title prop in MoneyRequestView. We can create a variable for the updatedTransaction?.modifiedMerchant, just like we have merchantTitle. https://github.com/Expensify/App/blob/6a6c242e79bb930b58146cb3e31f74d4e1fa0840/src/components/ReportActionItem/MoneyRequestView.tsx#L541 To:

title={(updatedTransaction?.modifiedMerchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT ? '' : updatedTransaction?.modifiedMerchant) ?? merchantTitle}

This is the same way we show empty merchant for a transaction when merchant is (none) so I think this is the more correct way fix this issue. https://github.com/Expensify/App/blob/6a6c242e79bb930b58146cb3e31f74d4e1fa0840/src/components/ReportActionItem/MoneyRequestView.tsx#L240

dylanexpensify commented 1 month ago

I believe we need to have something in for merchant, otherwise an error is thrown, so this isn't a bug

Krishna2323 commented 1 month ago

Merchant field shows (none) when (none) is selected, while Description field is empty when None is selected.

@dylanexpensify, merchant is only required in workspace chats, direct request to any user can be made without merchant so this is a valid bug. In the video below you can see that the merchant field is empty after creating the request, '(none)' is not shown.

@parasharrajat, I believe you have worked on this feature, can you please confirm this? πŸ™πŸ»

https://github.com/user-attachments/assets/d7dd90f8-79b9-48e8-825f-ee465ac9aece

dylanexpensify commented 1 month ago

Ah, good catch @Krishna2323

parasharrajat commented 1 month ago

Looks like a valid bug to me as well.

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~0194455fbb3684c1f1

dylanexpensify commented 1 month ago

Forgot to hit external.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

@eVoloshchak, @dylanexpensify Eep! 4 days overdue now. Issues have feelings too...

Krishna2323 commented 1 month ago

@eVoloshchak, friendly bump for review.

melvin-bot[bot] commented 1 month ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

eVoloshchak commented 1 month ago

@Krishna2323's proposal LGTM, both solutions are equivalent, I think we should proceed with the main one (update condition in ReviewMerchant.tsx)

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

@Beamanator @eVoloshchak @dylanexpensify 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!

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 πŸ“–

Krishna2323 commented 3 weeks ago

@eVoloshchak PR ready for review ^

Krishna2323 commented 3 weeks ago

@eVoloshchak, friendly bump for the review :)

dylanexpensify commented 1 week ago

pending deploy and regression

melvin-bot[bot] commented 4 days ago

This issue has not been updated in over 15 days. @Beamanator, @eVoloshchak, @dylanexpensify, @Krishna2323 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!