[Hold for Payment 08-30-23][$1000] Web - There is a delete option on a reply inside IOU report screen on the receiver's side

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Send a money request from account A to account B
2. Open the IOU report by clicking on the IOU preview on both account A and account B
3. Send a reply from account B
4. Hover over the sent reply from account A

Expected Result:

There should be no delete icon on account A

Actual Result:

Account A can see and click the delete button on the reply sent from account B. And if user A deletes the message Auth error shows up

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.47-2
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
Notes/Photos/Videos: Any additional supporting documentation

2023-07-28.13.22.27.mp4

Recording.3975.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Nathan-Mulugeta
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690539901030749

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b7d728393a99ac94
• Upwork Job ID: 1686409939086225408
• Last Price Increase: 2023-08-01
• Automatic offers:
• huzaifa-99 | Contributor | 26056682
• Nathan-Mulugeta | Reporter | 26056686

[melvin-bot]

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

[melvin-bot]

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

[huzaifa-99]

Proposal

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

We want to hide the delete icon in the mini context menu for user A if the message was sent from user B.

What is the root cause of that problem?

In #18735, we added the feature for policy admins to delete any message in workspace rooms, which displayed the delete icon to workspace admins.

Now when we request money from a user (in 1-1 or group chat), the requestor (User A) becomes the policy admin (policy.role === admin) for the money request. When the requestee (User B) replies to the money request action inside a thread, this condition becomes true for User A and shows the delete icon for User A

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

We can add a condition here to check if the report belongs to a workspace or is a 1-1/group report by checking the report.chatType which is '' for dm's (threads like this)

return policy.role === CONST.POLICY.ROLE.ADMIN && report.chatType !== ''

this would hide the delete icon. We can also utils like isWorkspaceThread()/isDm().

What alternative solutions did you explore? (Optional)

We could remove the policy.role === admin and set it to something like __FAKE__ for the money request action/report, but i think its simpler to check for chatType instead

[melvin-bot]

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

[sakluger]

Thanks for the report, the video makes this issue really clear 👍

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01b7d728393a99ac94

[melvin-bot]

Current assignee @sakluger is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[nickshuttle]

Proposal

Please re-state the problem that we are trying to solve in this issue.
We want to hide the delete icon in the mini context menu for user A if the message was sent from user B.

What is the root cause of that problem?

(Taken from above to avoid confusion)
In #18735, we added the feature for policy admins to delete any message in workspace rooms, which displayed the delete icon to workspace admins.

Now when we request money from a user (in 1-1 or group chat), the requestor (User A) becomes the policy admin (policy.role === admin) for the money request. When the requestee (User B) replies to the money request action inside a thread, this condition becomes true for User A and shows the delete icon for User A

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

Plan of action:

• Break up canDeleteReportAction into two clear usage utils. One for Delete as action owner and one for Delete with admin rights.
• For action owner no changes needed.
• For Admin we can block list certain report types from the check, example: Utilize isMoneyRequestReport to ensure admin access is ignored if the report is a money request type.

You can either do the block list as part of the admin until or pair it with another util. Example: (canDeleteReportActionWithAdmin && !isMoneyRequestReport)

What alternative solutions did you explore? (Optional)
Alternatively, you could look above in this thread for other options but I think those are blocking access on too wide of a scale. It may/may not be the case that we want to block admin deletes on ALL dms but it is the case we want to block it on IOU types.

Also - Regardless of solution breaking that canDeleteReportAction is vital to avoid running into this issue with another type down the line. It's just not clear to both have admin and owner rights checked in the same function like this.

[melvin-bot]

📣 @nickshuttle! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[nickshuttle]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~01860966a7fc3f0688

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[mananjadhav]

Thanks for the proposals folks. Both the proposals have the correct RCA, I don't like the idea of splitting canDeleteReportAction, as if we have more cases in the future, we wouldn't want to create than many utils.

Hence, @huzaifa-99's proposal here looks good to me. @huzaifa-99 I think it makes sense to use existing utils if we have any. But please note, I'll check with the BZ team member which all accesses should block the Delete action, and we should cover all of them in the current scope.

@sakluger @MonilBhavsar Can you confirm the types of chat/report, etc. where we want to hide the Delete option? I can see this bug report too.

🎀 👀 🎀 C+ reviewed.

[melvin-bot]

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

[huzaifa-99]

Thanks for the review @mananjadhav

@huzaifa-99 I think it makes sense to use existing utils if we have any. But please note, I'll check with the BZ team member which all accesses should block the Delete action, and we should cover all of them in the current scope.
@sakluger @MonilBhavsar Can you confirm the types of chat/report, etc. where we want to hide the Delete option? I can see this bug report too.

sure thing. In that case, I think its better to wait for the team's confirmation (i.e where we want to show the delete option) and then start the pr

[nickshuttle]

Thanks for the proposals folks. Both the proposals have the correct RCA, I don't like the idea of splitting canDeleteReportAction, as if we have more cases in the future, we wouldn't want to create than many utils.

Hence, @huzaifa-99's proposal here looks good to me. @huzaifa-99 I think it makes sense to use existing utils if we have any. But please note, I'll check with the BZ team member which all accesses should block the Delete action, and we should cover all of them in the current scope.

@sakluger @MonilBhavsar Can you confirm the types of chat/report, etc. where we want to hide the Delete option? I can see this bug report too.

🎀 👀 🎀 C+ reviewed.

I understand to some degree. However, the place this is being called already calls many utils. You know the code base far better than me but it seems like having utils with understandable outcomes is going to serve the future devs much better than a single method with many rules inside it that are murky.

shouldShow: (type, reportAction, isArchivedRoom, betas, menuTarget, isChronosReport, reportID) => // Until deleting parent threads is supported in FE, we will prevent the user from deleting a thread parent type === CONTEXT_MENU_TYPES.REPORT_ACTION && ReportUtils.canDeleteReportAction(reportAction, reportID) && !isArchivedRoom && !isChronosReport && !ReportActionUtils.isMessageDeleted(reportAction),

These overloaded && checks like this scare me. I really would figure out how to refactor this into something not so fragile to where the menu is being loaded up.

Anyway - Thank you for looking everything over.

[mananjadhav]

I agree with your points @nickshuttle. It works both ways. One having these large overloaded conditions can be daunting and definitely affect the readability, but having the same utils created based on different thread types would add to the code smells. Better way would be to split these conditions into context based flags, which then make the util more readable.

[nickshuttle]

@mananjadhav - Yeah, I understand. It's one of those things where you are stuck between things. The way it is already has code smell (Conditional Complexity) but not done correctly refactoring could put a new code smell into place.

The approach right now is going to build on top of that complexity with no clear call out for the next developer. If you don't break it, I really would suggest finding a way to make it super clear to the next developer that there are XYZ conditions being checked. Once you add policy admin === true && (thisChat === true || thatChat === true) it's gonna get weird the next time some new message type needs to make this choice.

Really, if you were up to bigger changes there could be a better approach where you could created some sort of rules engine instead of this direct checks on different states. That would allow for finer grained control in a much more testable/maintainable way.

[huzaifa-99]

Really, if you were up to bigger changes there could be a better approach where you could created some sort of rules engine instead of this direct checks on different states. That would allow for finer grained control in a much more testable/maintainable way.

Not sure, but I think we already have some of that planned in the typescript migration.

[sakluger]

@sakluger @MonilBhavsar Can you confirm the types of chat/report, etc. where we want to hide the Delete option? I can see this bug report too.

In both this case and the one you linked, a user is able to delete someone else's message/request. In general, a user should never be able to delete someone else's message or request. You should only be able to delete something that you created yourself.

There may be other places where the delete button isn't appropriate, but I can't think of them at the moment.

[melvin-bot]

📣 @mananjadhav Please request via NewDot manual requests for the Reviewer role ($1000)

[melvin-bot]

📣 @huzaifa-99 🎉 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]

📣 @Nathan-Mulugeta 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[huzaifa-99]

Thanks for the review and assignment, pr will be ready shortly

[huzaifa-99]

PR is ready for review #24396 @mananjadhav @neil-marcellini

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @huzaifa-99 got assigned: 2023-08-10 19:33:27 Z
• when the PR got merged: 2023-08-17 19:35:32 UTC
• days elapsed: 5

On to the next one 🚀

[mananjadhav]

This was deployed on production 2 days ago but the title wasn't updated.

[mananjadhav]

@sakluger This was deployed to production for 7 days but the title wasn't updated. Can you please post the payout summary?

@neil-marcellini I think this is eligible for the timeline bonus, as C+ review was done within the 3 days.

[huzaifa-99]

@neil-marcellini I think this is eligible for the timeline bonus, as C+ review was done within the 3 days.

I have the same request @neil-marcellini.

[mananjadhav]

@neil-marcellini @sakluger quick bump on the previous comments.

[mananjadhav]

@neil-marcellini @sakluger another follow up.

[sakluger]

Sorry for the delay! Yes, I reviewed the PR and this one qualifies for the bonus because it was C+ approved well within the allowed time.

Summarizing payouts for this issue:

Reporter: @Nathan-Mulugeta - $250 (paid on Upwork)
Contributor: @huzaifa-99 $1500 (hired on Upwork)
Contributor+: @mananjadhav $1500 (payable via Manual Request)

Above payments include efficiency bonus 🎉
Upwork job: https://www.upwork.com/jobs/~01b7d728393a99ac94

@huzaifa-99 could yuu please accept the offer in Upwork so that we can complete the payment? Thanks!

[mananjadhav]

Thanks @sakluger. Appreciate your help here.

[huzaifa-99]

Thank you @sakluger. I have accepted the offer.

[Nathan-Mulugeta]

Hey @sakluger I didn't receive any payments yet. You might have paid wrong person?

[Nathan-Mulugeta]

Just received the payment thanks.

[sakluger]

Phew, glad you go the payment @Nathan-Mulugeta!

@mananjadhav did you send the payment request in NewDot? Would you like me to leave the issue open until you've done that, or will you track and handle outside the issue?

[mananjadhav]

Yes @sakluger. I've raised the payout request on NewDot.

I'll track this outside the issue. cc - @JmillsExpensify

[JmillsExpensify]

$1,500 approved for payment via NewDot based on BZ summary.

[sakluger]

Thanks everyone!