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

[$500] Scan - Workspace admin is unable to replace receipt for expense created by employee #29829

Closed
6 tasks done
lanitochka17 opened this issue Oct 17, 2023 · 39 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Oct 17, 2023

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: 1.3.85-2
Reproducible in staging?: Yes
Reproducible in production?: Yes
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. Go to staging.new.expensify.com
  2. [Employee] Create a Scan request in workspace chat
  3. [Admin] Go to expense request details page created by the employee
  4. [Admin] Edit all the fields of the Scan request including the receipt
    Note that admin can edit all the fields except receipt

Expected Result:

The admin can edit all the fields in the workspace expense created by employee

Actual Result:

The admin can edit all the fields in the workspace expense created by employee, except receipt. Replacing the receipt reverts it to the original receipt uploaded by the employee

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

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Bug6240990_1697572063677.20231018_003035.mp4
MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015ffebdb6212649cd
  • Upwork Job ID: 1714387319090634752
  • Last Price Increase: 2023-10-31
  • Automatic offers:
    • abdulrahuman5196 | Reviewer | 27560318
    • DylanDylann | Contributor | 27560319
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 17, 2023
@melvin-bot melvin-bot bot changed the title Scan - Workspace admin is unable to replace receipt for expense created by employee [$500] Scan - Workspace admin is unable to replace receipt for expense created by employee Oct 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

Job added to Upwork: https://www.upwork.com/jobs/~015ffebdb6212649cd

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

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

@graylewis
Copy link
Contributor

graylewis commented Oct 17, 2023

This seems like a backend issue. The frontend allows you to try to replace it but the backend returns the following response:

{
    "jsonCode": 401,
    "message": "401 Unauthorized",
    "onyxData": [],
    "requestID": "817b98b0ec05b99a-AMS"
}

@AmjedNazzal
Copy link
Contributor

I agree with @graylewis, the user is exposed to edit reciept page which calls IOU.replaceReceipt only if it's either the request owner or an admin so it shouldn't return Unauthorized, perhaps when the backend side was done it was expected that some extra parameter would be provided or it was not considered that it could be an admin replacing the receipt.

@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2023

Triggered auto assignment to @pecanoro (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@bfitzexpensify
Copy link
Contributor

@pecanoro can you help confirm that this is a back-end issue?

@pecanoro
Copy link
Contributor

Let me check in Slack if admins should be able to replace the receipts.

@pecanoro
Copy link
Contributor

We decided that it makes no sense to let the admin replace the receipt so we need to block that in the front end!

@AmjedNazzal
Copy link
Contributor

@pecanoro Then we should probably remove the option all together for anyone except the account that uploaded the receipt right?

@pecanoro
Copy link
Contributor

Yes, that sounds good!

@AmjedNazzal
Copy link
Contributor

@bfitzexpensify Should a feauture request be created or how should this be done?

@pecanoro
Copy link
Contributor

This is still a bug since there is a reason why the back-end does not allow it and it is because the front-end shouldn't either.

@graylewis
Copy link
Contributor

graylewis commented Oct 19, 2023

In that case:
@pecanoro @AmjedNazzal @bfitzexpensify

Proposal

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

The frontend displays an option to replace the receipt for an expense created by an employee when it shouldn't

What is the root cause of that problem?

The canEdit variable which determines whether the frontend should display the option to edit a receipt request is true whenever the user is an admin

const canEdit = !isSettled && !isDeleted && (isAdmin || isRequestor);

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

Since admins should only be allowed to edit a receipt request when they created it, we can simply remove isAdmin from that line of code and canEdit will be calculated appropriately.

const canEdit = !isSettled && !isDeleted && isRequestor;

What alternative solutions did you explore? (Optional)

N/A

@melvin-bot melvin-bot bot added the Overdue label Oct 23, 2023
@abdulrahuman5196
Copy link
Contributor

Will be reviewing today

@melvin-bot melvin-bot bot removed the Overdue label Oct 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 24, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Oct 25, 2023
@pecanoro
Copy link
Contributor

@abdulrahuman5196 Friendly bump!

@melvin-bot
Copy link

melvin-bot bot commented Oct 30, 2023

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

@abdulrahuman5196
Copy link
Contributor

abdulrahuman5196 commented Oct 30, 2023

@graylewis For me, the receipt is only seen by me. Not seen by the admin. So admin is unable to go to receipt screen itself.

Screenshot 2023-10-30 at 8 36 19 PM

Could you kindly check if the same is happening for you? Or anyone else possible to check the same? I think something in backend changed.

@pecanoro
Copy link
Contributor

@abdulrahuman5196 Is still scanning? I think it might change when it is not scanning anymore. I will check later as well.

@abdulrahuman5196
Copy link
Contributor

abdulrahuman5196 commented Oct 30, 2023

Nope. Even after hours not changing. Tried manually updating in which case, scanning progress was removed but still the receipt was only visible by the creator.
I think it could be a new bug

@melvin-bot
Copy link

melvin-bot bot commented Oct 31, 2023

@pecanoro @bfitzexpensify @abdulrahuman5196 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!

Copy link

melvin-bot bot commented Oct 31, 2023

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

@pecanoro
Copy link
Contributor

pecanoro commented Nov 1, 2023

@abdulrahuman5196 Hmm if it says only visible to you it means it's stuck scanning or something. If you sign out, still same problem?

Maybe try this fake receipt:
0_-_SOudr4bEhBADo5

@abdulrahuman5196
Copy link
Contributor

Let me check again

@DylanDylann
Copy link
Contributor

Proposal

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

  • App allows the user to replace the receipt but it is not allowed from BE side.

What is the root cause of that problem?

  • Currently, we use canEdit variable to check whether we display the "Replace receipt" image or not:
    const canEdit = !isSettled && !isDeleted && (isAdmin || isRequestor);
  • When user is admin but not requestor, it will display the Replace and Delete receipt buttons but these actions are not allowed from BE side.

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

  • Generally, we need to prevent users from going to the flow "Replace" or "Delete receipt" by:

1. Firstly, hide Replace and Delete receipt if the user is not the requestor.

  • Do it by updating canEditFieldOfMoneyRequest that is used in the EditRequestPage to check if we can some field or not:

    App/src/libs/ReportUtils.js

    Lines 1660 to 1678 in 925ff67

    function canEditFieldOfMoneyRequest(reportAction, reportID, fieldToEdit) {
    // A list of fields that cannot be edited by anyone, once a money request has been settled
    const nonEditableFieldsWhenSettled = [
    CONST.EDIT_REQUEST_FIELD.AMOUNT,
    CONST.EDIT_REQUEST_FIELD.CURRENCY,
    CONST.EDIT_REQUEST_FIELD.DATE,
    CONST.EDIT_REQUEST_FIELD.RECEIPT,
    CONST.EDIT_REQUEST_FIELD.DISTANCE,
    ];
    // Checks if this user has permissions to edit this money request
    if (!canEditMoneyRequest(reportAction)) {
    return false; // User doesn't have permission to edit
    }
    // Checks if the report is settled
    // Checks if the provided property is a restricted one
    return !isSettled(reportID) || !nonEditableFieldsWhenSettled.includes(fieldToEdit);
    }

    to:
function canEditFieldOfMoneyRequest(reportAction, reportID, fieldToEdit) {
    ...
+    const nonEditableFieldsWhenIsAdmin = [CONST.EDIT_REQUEST_FIELD.RECEIPT];
+    const isRequestor = currentUserAccountID === reportAction.actorAccountID;
+    if (isAdmin && !isRequestor && nonEditableFieldsWhenIsAdmin.includes(fieldToEdit)) {
+       return false;
    }
    ....
const canEdit = ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, parentReport.reportID, CONST.EDIT_REQUEST_FIELD.RECEIPT)

2. Secondly, we need to handle the case that opening app by deeplink. When user is admin, try accessing the route like /r/reportID/edit/receipt should dismiss the edit modal.

  • This will be handled if the 1.Firstly is implemented because we have already the logic:
    useEffect(() => {
    // Do not dismiss the modal, when a current user can edit this property of the money request.
    if (ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, parentReport.reportID, fieldToEdit)) {
    return;
    }
    // Dismiss the modal when a current user cannot edit a money request.
    Navigation.isNavigationReady().then(() => {
    Navigation.dismissModal();
    });
    }, [parentReportAction, parentReport.reportID, fieldToEdit]);

What alternative solutions did you explore? (Optional)

  • NA

@melvin-bot melvin-bot bot added the Overdue label Nov 6, 2023
@abdulrahuman5196
Copy link
Contributor

Checking now with the fake bill

@melvin-bot melvin-bot bot removed the Overdue label Nov 6, 2023
@abdulrahuman5196
Copy link
Contributor

Now I am able use the fake bill. REviewing the proposals

@abdulrahuman5196
Copy link
Contributor

@DylanDylann 's proposal here #29829 (comment) looks good and works well.

🎀 👀 🎀
C+ Reviewed

cc: @pecanoro

Copy link

melvin-bot bot commented Nov 7, 2023

@pecanoro @bfitzexpensify @abdulrahuman5196 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

@pecanoro
Copy link
Contributor

pecanoro commented Nov 7, 2023

@abdulrahuman5196 Thank you for double-checking! Assigning @DylanDylann to the issue!

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

melvin-bot bot commented Nov 7, 2023

📣 @abdulrahuman5196 🎉 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 Nov 7, 2023

📣 @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 added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Nov 8, 2023
@DylanDylann
Copy link
Contributor

@abdulrahuman5196 PR #31024 is ready for review

@abdulrahuman5196
Copy link
Contributor

The PR that introduced the bug has been identified. Link to the PR:
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:
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:

Not a regression. New expectation was set with the PR.

Determine if we should create a regression test for this bug.

Yes.

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.

  1. [Workspace Member account]Create a Scan request in workspace chat
  2. [Workspace Admin] Go to expense request details page created by the member account
  3. [Workspace Admin] Edit all the fields of the Scan request including the receipt
  4. Observe that admin can edit all the fields except the receipt

@bfitzexpensify Seems melvin didn't auto update this issue. The payment date is today - #31024 (comment)

@bfitzexpensify bfitzexpensify added Daily KSv2 and removed Weekly KSv2 labels Dec 3, 2023
@bfitzexpensify
Copy link
Contributor

Sorry for the delay, only seeing that comment now, looks like melvin did miss this one. Payments complete, and regression steps proposed in https://github.com/Expensify/Expensify/issues/342297. Closing this one out.

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

No branches or pull requests

7 participants