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] Expense - Shouldn't show "delete receipt" option for eReceipts #46289

Closed
6 tasks done
lanitochka17 opened this issue Jul 26, 2024 · 29 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Jul 26, 2024

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?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6553075_1721940175005.20240726_043744.mp4

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
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 26, 2024
Copy link

melvin-bot bot commented Jul 26, 2024

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
Copy link
Author

@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 melvin-bot bot added the Overdue label Jul 29, 2024
Copy link

melvin-bot bot commented Jul 29, 2024

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

@puneetlath
Copy link
Contributor

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 melvin-bot bot removed the Overdue label Jul 29, 2024
@puneetlath puneetlath changed the title Expense - Receipt remains the same after deleting or adding receipt for card transaction Expense - Shouldn't show "delete receipt" option for eReceipts Jul 29, 2024
@puneetlath puneetlath added the External Added to denote the issue can be worked on by a contributor label Jul 29, 2024
@melvin-bot melvin-bot bot changed the title Expense - Shouldn't show "delete receipt" option for eReceipts [$250] Expense - Shouldn't show "delete receipt" option for eReceipts Jul 29, 2024
Copy link

melvin-bot bot commented Jul 29, 2024

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

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

melvin-bot bot commented Jul 29, 2024

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

@gijoe0295
Copy link
Contributor

gijoe0295 commented Jul 29, 2024

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 #46373.

@gijoe0295
Copy link
Contributor

Proposal Updated

  • Add code changes

@daledah
Copy link
Contributor

daledah commented Jul 31, 2024

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:

App/src/libs/ReportUtils.ts

Lines 2762 to 2765 in fb83ab1

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

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;
    }

@melvin-bot melvin-bot bot added the Overdue label Jul 31, 2024
@alitoshmatov
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Aug 1, 2024
@puneetlath
Copy link
Contributor

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.

Copy link

melvin-bot bot commented Aug 5, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

Copy link

melvin-bot bot commented Aug 6, 2024

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

@puneetlath
Copy link
Contributor

@alitoshmatov how can I help you move this forward?

@alitoshmatov
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Aug 7, 2024
@alitoshmatov
Copy link
Contributor

alitoshmatov commented Aug 7, 2024

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 🎀 👀 🎀

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 7, 2024
@puneetlath
Copy link
Contributor

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

Copy link

melvin-bot bot commented Aug 7, 2024

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

Copy link

melvin-bot bot commented Aug 7, 2024

📣 @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 📖

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 8, 2024
@daledah
Copy link
Contributor

daledah commented Aug 8, 2024

@alitoshmatov This PR is ready for review.

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Sep 2, 2024
Copy link

melvin-bot bot commented Sep 2, 2024

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!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Sep 2, 2024
@alitoshmatov
Copy link
Contributor

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

cc: @puneetlath

@puneetlath
Copy link
Contributor

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

@alitoshmatov
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

daledah commented Sep 11, 2024

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

@puneetlath
Copy link
Contributor

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

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

@daledah
Copy link
Contributor

daledah commented Sep 11, 2024

@puneetlath I accepted thx

@puneetlath
Copy link
Contributor

All paid. Thanks everyone!

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. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

5 participants