[Hold for payment][$500] Chat - Action menu for MP4 files has "Copy to clipboard" and "Edit comment" functions

[kbecciv]

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: v1.4.35-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:
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:

Pre-requisite: user must be logged in.

1. Go to any chat.
2. Send a MP4 file.
3. Right click on it for the action menu to appear

Expected Result:

"Copy to clipboard" and "Edit comment" should not be options for MP4 files.

Actual Result:

"Copy to clipboard" and "Edit comment" are functions available for MP4 files. When edited, the file is no longer available.

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 evidences>

Recording.5972.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bd7a8a1a0e4bc08b
• Upwork Job ID: 1753109564515966976
• Last Price Increase: 2024-02-01
• Automatic offers:
• DylanDylann | Reviewer | 0
• Tony-MK | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[kbecciv]

We think that this bug might be related to #vip-vsb
CC @quinthar

[ikevin127]

Proposal

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

"Copy to clipboard" and "Edit comment" are functions should not be available for MP4 files.
When edited, the file is no longer available.

What is the root cause of that problem?

The issue here is that for the context menu actions we don't check the attachment type so we can show / hide these actions depending on the attachment type.

Within ContextMenuActions, for every action we have a shouldShow prop where this check is currently missing for the mentioned file types.

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

We should add logic similar to isVideo from FileUtils to check for the attachment types within ContextMenuActions and display the actions accordingly.

For this we should agree on an array of attachment file types and apply a ReportUtils lib where we're checking within reportAction.attachmentInfo.type against the array and if the file type exists we don't show the action.
We will apply this within ContextMenuActions for both the edit and copy to clipboard actions.

Videos

MacOS: Chrome / Safari

Before	After
Screen.Recording.2024-02-01.at.19.58.06.mov	Screen.Recording.2024-02-01.at.20.00.00.mov

[Krishna2323]

Proposal

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

Chat - Action menu for MP4 files has "Copy to clipboard" and "Edit comment" functions.

What is the root cause of that problem?

In isReportActionAttachment we don't check for attachmentInfo existence before returning a value for isReportActionAttachment and in edit action we don't use isReportActionAttachment check.

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

We should also check for attachmentInfo property in reportAction before returning a value for isReportActionAttachment.

    if (reportAction && 'attachmentInfo' in reportAction) {
        return true;
    }

In isReportActionAttachment util function.

App/src/libs/ReportActionsUtils.ts

Lines 667 to 679 in ff23077

	function isReportActionAttachment(reportAction: OnyxEntry<ReportAction>): boolean {
	const message = reportAction?.message?.[0];
	if (reportAction && 'isAttachment' in reportAction) {
	return reportAction.isAttachment ?? false;
	}
	if (message) {
	return isReportMessageAttachment(message);
	}
	return false;
	}

This property always exists on attachments.

Use isReportActionAttachment for editAction also instead of replacing isReportMessageAttachment with isReportActionAttachment in canEditReportAction.

Or add isReportActionAttachment check in canEditReportAction

Result

[mkhutornyi]

I think this should be fixed in backend.
Backend is sending file name in text property for some reason. It should be [Attachment]

[Krishna2323]

@mkhutornyi, yeah its a backend issue, the isAttachment property is also returned as false from the backend.

[Tony-MK]

Proposal

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

Chat - Action menu for MP4 files has "Copy to clipboard" and "Edit comment" functions

What is the root cause of that problem?

The root cause of the problem is that we use isReportMessageAttachment in canEditReportAction and the first condition seen below will fail because text is the video's filename.

App/src/libs/isReportMessageAttachment.ts

Lines 19 to 20 in afa88d4

	const regex = new RegExp(` ${CONST.ATTACHMENT_SOURCE_ATTRIBUTE}="(.*)"`, 'i');
	return text === CONST.ATTACHMENT_MESSAGE_TEXT && (!!html.match(regex) || html === CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML);

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

1. Switch isReportMessageAttachment with isReportActionAttachment in the canEditReportAction function.

2. Check for reportAction.attachmentInfo in the isReportActionAttachment because reportAction.isAttachment is false.

if (reportAction && ('isAttachment' in reportAction || 'attachmentInfo' in reportAction)) {
    return  !!(reportAction?.attachmentInfo ?? reportAction.isAttachment);
}

What alternative solutions did you explore? (Optional)

We could ignore step 1 and only execute step 2 in the isReportMessageAttachment function.

[DylanDylann]

I will review it tomorrow

[melvin-bot]

@kadiealexander, @DylanDylann Whoops! This issue is 2 days overdue. Let's get this updated quick!

[DylanDylann]

Waiting for more proposal

[Krishna2323]

Unhidden my initial proposal if this needs to be fixed on frontend.
@DylanDylann, tagging you for a review.

[Tony-MK]

Hi @DylanDylann, I updated my proposal.
Could you review it?
Thanks

[DylanDylann]

Thanks for everyone. @Krishna2323's proposal doesn't fix this issue completely (miss "Edit comment" option). @Tony-MK's proposal looks good to me

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[Krishna2323]

@DylanDylann, I don't think we should modify isReportMessageAttachment with isReportActionAttachment in the canEditReportAction function, because canEditReportAction is also used in ReportScreen and isReportMessageAttachment is different from isReportActionAttachment.

Also I think my proposal was the first to identify the correct root cause and provide a solution, though I accidentally deleted the point for adding that check for edit case also but I think it's very minor thing to select a proposal, that would have been checked during the PR as it has been also included in the expected outcome. Thanks

[Krishna2323]

In isReportActionAttachment we don't check for attachmentInfo existence before returning a value for isReportActionAttachment and in edit action we don't use isReportActionAttachment check.

hey @DylanDylann, I deleted the edit action point from my solution by mistake but I think I still have it in the root cause section😅. So I think my proposal is still the first with the correct solution.

[DylanDylann]

because canEditReportAction is also used in ReportScreen and isReportMessageAttachment is different from isReportActionAttachment.

I don't see any problem with modifying isReportMessageAttachment with isReportActionAttachment in the canEditReportAction function

[Krishna2323]

I do think it may cause regression, but even if it do not, I think switching isReportMessageAttachment with isReportActionAttachment in the canEditReportAction function is same as adding isReportActionAttachment check in edit action.

Thanks for everyone. @Krishna2323's #35578 (comment) doesn't fix this issue completely (miss "Edit comment" option).

Also you missed the "Edit comment" option point in my root cause.

and in edit action we don't use isReportActionAttachment check.

Can you pls re-evaluate proposals 🙏🏻

cc: @srikarparsi

[melvin-bot]

@Tony-MK, @srikarparsi, @kadiealexander, @DylanDylann Whoops! This issue is 2 days overdue. Let's get this updated quick!

[kadiealexander]

Not overdue, being worked on in the PR.

[DylanDylann]

@Tony-MK any update on the PR

[melvin-bot]

@Tony-MK @srikarparsi @kadiealexander @DylanDylann 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!

[Tony-MK]

Sorry for the delay, @DylanDylann.

I am having issues testing android - mWeb.

However, I should be done by tomorrow.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[mkhutornyi]

@Tony-MK can you please confirm your PR will fix #36828 as well?

[techievivek]

@srikarparsi It seems like the backend is returning the original filename instead of the [Attachment] text. Were you able to look into it? There is also a comment from @mkhutornyi regarding the same: #35578 (comment).

CC @francoisl since you were involved with this project. 🙇

Also, it would be great to have this fixed as quickly as possible @DylanDylann @Tony-MK, since we had another issue created today that was marked as deploy blocker #36828.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[DylanDylann]

@Tony-MK Please help to speed up our PR, because our PR will fix other deploy blocker issues

[Tony-MK]

@DylanDylann Sorry for the delay. Let's finish the PR.

[francoisl]

It seems like the backend is returning the original filename instead of the [Attachment] text. Were you able to look into it? There is also a comment from

@techievivek I'm working on this already, we also need to make the backend return [Attachment] for the lastMessageText value when uploading a video.

[techievivek]

Ok great, thanks for confirming @francoisl 🙇

[DylanDylann]

@kadiealexander The PR was deployed to production 2 weeks ago. Please help to process the next step

[kadiealexander]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@DylanDylann] The PR that introduced the bug has been identified. Link to the PR: NA
• [@DylanDylann] 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: NA
• [@DylanDylann] 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: NA
• [@DylanDylann] Determine if we should create a regression test for this bug. Yes
• [@DylanDylann] 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.
• [@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/379066

[kadiealexander]

Payouts due:

• Contributor: $500 @Tony-MK
• Contributor+: $500 @DylanDylann

Upwork job is here.

[DylanDylann]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@DylanDylann] The PR that introduced the bug has been identified. Link to the PR: NA
[@DylanDylann] 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: NA
[@DylanDylann] 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: NA
[@DylanDylann] Determine if we should create a regression test for this bug. Yes
[@DylanDylann] 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.

Regression Test Proposal

1. Go to a chat that has a video that was not uploaded by the device.
2. Open the video's context menu by holding down or left-click.
3. Verify that "Download" is a menu option.
4. Verify "Copy to clipboard" and "Edit comment" are not menu options.
5. Upload a video.
6. Open the video's context menu by holding down or left-click.
7. Verify that "Download" is a menu option.
8. Verify "Copy to clipboard" and "Edit comment" are not menu options.

Do we agree 👍 or 👎