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.79k forks source link

[$250] Expense - Shouldn't show "delete receipt" option for eReceipts #46289

Closed lanitochka17 closed 2 weeks ago

lanitochka17 commented 2 months 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.13-0 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/4768840 Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Log in to applausetester+pendingreceipt@applause.expensifail.com
  3. Open a card transaction (the one with colorful receipt) - https://staging.new.expensify.com/r/7013138223320456
  4. Click on the receipt
  5. Click 3-dot menu
  6. Click Delete receipt
  7. Click Add receipt and upload a receipt

Expected Result:

The receipt will change after deleting or adding a receipt

Actual Result:

The receipt remains the same after deleting or adding a receipt for card transaction

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/a5d57352-ab83-4580-8018-335d7e49e582

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0105eee26e9f25403e
  • Upwork Job ID: 1818000401430758165
  • Last Price Increase: 2024-08-05
  • Automatic offers:
    • alitoshmatov | Reviewer | 103437449
Issue OwnerCurrent Issue Owner: @alitoshmatov
melvin-bot[bot] commented 2 months ago

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

lanitochka17 commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

puneetlath commented 2 months ago

So it seems to me like a bug that you even see the "delete receipt" option for an eReceipt. I think we should fix that here. And we can fix the issue of the receipt not being replaced here: https://expensify.slack.com/archives/C049HHMV9SM/p1722272821021819

melvin-bot[bot] commented 2 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0105eee26e9f25403e

melvin-bot[bot] commented 2 months ago

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

gijoe0295 commented 2 months ago

Proposal

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

Shouldn't show "delete receipt" option for eReceipts

What is the root cause of that problem?

This is a feature request

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

To do that, we need to implement a mechanism to pass TransactionUtils.hasEReceipt() from MoneyRequestView to TransactionReceipt page. Since they are not within a component tree, we can leverage the route params.

  1. Introduce a new prop readonly to ReportActionItemImage and pass it from MoneyRequestView here: readonly={TransactionUtils.hasEReceipt(transaction)}
  2. Introduce a new param readonly to TransactionReceipt screen here and its route here
  3. When we navigate to TransactionReceipt screen from ReportActionItemImage here, remember to pass readonly to getRoute
  4. In TransactionReceipt, retrieve the readonly param from route and append to the canEditReceipt condition here to canEditReceipt={canEditReceipt && !readonly}

This can also resolve https://github.com/Expensify/App/issues/46373.

gijoe0295 commented 2 months ago

Proposal Updated

daledah commented 2 months ago

Proposal

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

you even see the "delete receipt" option for an eReceipt

What is the root cause of that problem?

We don't have logic to prevent user from editing "receipt" in function canEditFieldOfMoneyRequest: https://github.com/Expensify/App/blob/fb83ab10801e12ef20a7568389850f6744f72cc9/src/libs/ReportUtils.ts#L2762-L2765

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

Should use:

    if (fieldToEdit === CONST.EDIT_REQUEST_FIELD.RECEIPT) {
        const isRequestor = currentUserAccountID === reportAction?.actorAccountID;
        return !TransactionUtils.hasEReceipt(transaction) && !TransactionUtils.hasEReceipt(transaction) &&  !isInvoiceReport(moneyRequestReport) && !TransactionUtils.isReceiptBeingScanned(transaction) && !TransactionUtils.isDistanceRequest(transaction) && isRequestor;
    }
alitoshmatov commented 2 months ago

Is there a way for me to test this eReceipts on my account?

puneetlath commented 1 month ago

Hmm, I think you'd need to use mock data. As the only way to get one for real is to connect a credit card to your account to import transactions.

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? ๐Ÿ’ธ

melvin-bot[bot] commented 1 month ago

@puneetlath, @alitoshmatov Eep! 4 days overdue now. Issues have feelings too...

puneetlath commented 1 month ago

@alitoshmatov how can I help you move this forward?

alitoshmatov commented 1 month ago

@gijoe0295 Thank you for your proposal, I don't think you are in a right direction with your solution we have direct access to isEReceipt variable that context.

alitoshmatov commented 1 month ago

I am a little bit confused about expected result, the title says don't show delete receipt, the expected result says

The receipt will change after deleting or adding a receipt

If we don't want any action on the receipt we can go with @daledah 's proposal which correctly removes all editing actions from receipt

C+ reviewed ๐ŸŽ€ ๐Ÿ‘€ ๐ŸŽ€

puneetlath commented 1 month ago

Correct, we shouldn't show the option to delete an eReceipt. Assigning @daledah

melvin-bot[bot] commented 1 month ago

๐Ÿ“ฃ @alitoshmatov ๐ŸŽ‰ 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

melvin-bot[bot] commented 1 month ago

๐Ÿ“ฃ @daledah You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review ๐Ÿง‘โ€๐Ÿ’ป Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing ๐Ÿ“–

daledah commented 1 month ago

@alitoshmatov This PR is ready for review.

melvin-bot[bot] commented 4 weeks ago

This issue has not been updated in over 15 days. @puneetlath, @alitoshmatov, @daledah 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!

alitoshmatov commented 3 weeks ago

The PR was deployed to production 2 weeks ago, waiting for payment

cc: @puneetlath

puneetlath commented 3 weeks ago

Ah got it. Can you complete the checklist @alitoshmatov?

alitoshmatov commented 2 weeks ago

Regression Test Proposal

  1. Open a card transaction with eReceipt
  2. Click on the receipt
  3. Make sure there is no three dot, which means we can not perform any action on this receipt
puneetlath commented 2 weeks ago

Regression test issue: https://github.com/Expensify/Expensify/issues/427573

I paid @alitoshmatov. @daledah looks like you weren't auto-hired for some reason. Can you link me your upwork profile so I can pay you?

daledah commented 2 weeks ago

@puneetlath Here's my Upwork profile https://www.upwork.com/freelancers/~0138d999529f34d33f, please help send the offer. TIA

puneetlath commented 2 weeks ago

Offer here: https://www.upwork.com/nx/wm/offer/103916509

Please ping me on this issue when you've accepted @daledah

daledah commented 2 weeks ago

@puneetlath I accepted thx

puneetlath commented 2 weeks ago

All paid. Thanks everyone!