[$250] Chat - No download button for attachment sent with comment

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

2.99k stars 2.5k forks source link

[$250] Chat - No download button for attachment sent with comment #41563

Closed izarutskaya closed 1 week ago

izarutskaya commented 2 weeks 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: v1.4.70-1 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/4536722 Email or phone of affected tester (no customers): applausetester+vd_web050224@applause.expensifail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Pre-requisite: the user must be logged in.

Go to any chat.
Enter any text on the compose box but do not send the message yet.
Tap on "+" and send an attachment, like an image.
Wait for the attachment to upload.
Verify the comment and the attachment have been sent.
Tap and hold on the attachment.
Verify the is no download button.

Expected Result:

Download button should be displayed.

Actual Result:

There is no download button.

Workaround:

Unknown

Platforms:

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

[ ] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/43e36a3d-9854-44d9-a9d6-d417a1867e08

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~010803b7248a9b3406
Upwork Job ID: 1786385833222553600
Last Price Increase: 2024-05-03

Issue Owner

Current Issue Owner: @sobitneupane

melvin-bot[bot] commented 2 weeks ago

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

izarutskaya commented 2 weeks ago

We think this issue might be related to the #vip-vsb.

ShridharGoel commented 2 weeks ago

Proposal

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

No download button for attachment sent with comment

What is the root cause of that problem?

Text + attachment is not considered as attachment due to the condition in isReportActionAttachment.

https://github.com/Expensify/App/blob/0f39e4370ccd84ccf7e1647b42cb505a69a2e9a3/src/libs/ReportActionsUtils.ts#L849

Also, we are checking messageHtml to not be exactly same as CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML which needs to be updated for text + attachment.

https://github.com/Expensify/App/blob/0f39e4370ccd84ccf7e1647b42cb505a69a2e9a3/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L463

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

Change the condition in isReportActionAttachment to below:

return (reportAction?.isAttachment || !!reportAction?.attachmentInfo) ?? false;

In the shouldShow condition for download in ContextMenuActions, change it to below:

isAttachment && !reportAction?.isOptimisticAction && !!reportAction?.reportActionID && !ReportActionsUtils.isMessageDeleted(reportAction) && !isOffline

Note: We can instead use !(messageHtml.includes(CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML))` check also but isOptimisticAction would be better.

abzokhattab commented 2 weeks ago

Proposal

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

Download button is not shown in the context menu for attachment sent with comment

What is the root cause of that problem?

the isReportActionAttachment is used here to decide whether the download button should be shown in the menu context or not
the isReportActionAttachment function uses another function isReportMessageAttachment which checks for whether the message is equal to [Attachment] or not. it doesn't consider the case where there is a another comment beside the attachment

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

two solution:

either we can change the message.text === CONST.ATTACHMENT_MESSAGE_TEXT check in this line to message.text.includes(CONST.ATTACHMENT_MESSAGE_TEXT). but this might introduce regressions in other places because the isReportActionAttachment and the isReportMessageAttachment functions are used in multiple locations.
or a better solution would be to create a new function isReportMessageContainsAttachment inside the ReportActionsUtils file that would check for wether the messages contains the attachment pattern or not:

function isReportMessageContainsAttachment(reportAction: OnyxEntry<ReportAction>): boolean {
    const messageText = reportAction?.message?.[0]?.text ?? '';
    return isReportActionAttachment(reportAction) || messageText.includes(CONST.ATTACHMENT_MESSAGE_TEXT); 
    // or to be more precise we can use `messageText.endsWith` instead of `messageText.includes` since the [Attachment] word is always placed at the end of the message.

}

then change this to:

            const isAttachment = ReportActionsUtils.isReportMessageContainsAttachment(reportAction);

bernhardoj commented 2 weeks ago

Proposal

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

The download button isn't available for an attachment + text action.

What is the root cause of that problem?

One of the conditions for the download button to show is that the isAttachment condition is true. However, ReportActionsUtils.isReportActionAttachment checks whether the action contains ONLY attachment or not.

https://github.com/Expensify/App/blob/0f39e4370ccd84ccf7e1647b42cb505a69a2e9a3/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L459-L465

Because in our case it's an attachment + text, the download option won't show.

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

I would keep ReportActionsUtils.isReportActionAttachment behavior the same, that is to check whether the action contains only attachment or not, otherwise it could break some parts of the app function.

So, I suggest creating a new function called ReportActionsUtils.isReportActionAttachmentAndText to check if an action is an attachment + text, based on isReportActionAttachment, but simpler.

function isReportActionAttachmentAndText(reportAction: OnyxEntry<ReportAction>) {
    const message = reportAction?.message?.[0];
    return isReportMessageAttachmentAndText(message);
}

I don't include the check for reportAction.isAttachment and reportAction.attachmentInfo because both data are only available on the client, so if you re-login, you won't see it anymore. isReportMessageAttachmentAndText will look like this:

export default function isReportMessageAttachmentAndText(message: Message | undefined): boolean {
    if (!message?.text || !message.html) {
        return false;
    }

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

It will return true if the message text last word is [Attachment] (or a video) and the message HTML match the attachment source regex.

Then, use the new function in the context menu should show logic here https://github.com/Expensify/App/blob/0f39e4370ccd84ccf7e1647b42cb505a69a2e9a3/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L459-L465 (isAttachment || isAttachmentAndText)

dragnoir commented 2 weeks ago

Proposal

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

No download button for attachment sent with comment

What is the root cause of that problem?

The context menu item doesn't display because the isAttachment turns false here

https://github.com/Expensify/App/blob/0f39e4370ccd84ccf7e1647b42cb505a69a2e9a3/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L459-L460

this is because the isReportMessageAttachment function turns.

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

1- First we need to solve isReportActionAttachment

For this issue, the condition fail here: https://github.com/Expensify/App/blob/c026588f0bb6890389d3b7c3966e63d819d0dfca/src/libs/ReportActionsUtils.ts#L848-L850

We need to solve this because the values reportAction?.isAttachment and !!reportAction?.attachmentInfo does not exist and only available on local Onyx, not from another device or a new logged account.

We can make this as a second condition and move up the if (message) one.

function  isReportActionAttachment(reportAction:  OnyxEntry<ReportAction>):  boolean {
  const  message  =  reportAction?.message?.[0];

 if (message) {
    return  isReportMessageAttachment(message);
  }

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

  return  false;
}

2- solve isReportMessageAttachment

when we fix isReportActionAttachment, the code will check here https://github.com/Expensify/App/blob/c026588f0bb6890389d3b7c3966e63d819d0dfca/src/libs/ReportActionsUtils.ts#L852-L854

Even we have the message, the isReportMessageAttachment will turn false.

There, here

https://github.com/Expensify/App/blob/0f39e4370ccd84ccf7e1647b42cb505a69a2e9a3/src/libs/isReportMessageAttachment.ts#L20C83-L21C5

We need to turn true if message.html.match(regex) is true. This mean the message html contain the data-expensify-source attribute.

one solution can be by replacing on the condition above the && by ||

return  message.text  ===  CONST.ATTACHMENT_MESSAGE_TEXT  ||  !!Str.isVideo(message.text) ||  !!message.html.match(regex) ||  message.html  ===  CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML;

we can also create a sperate condition for this case where:

message.html.match(regex)
the word "[Attachment]" is inside the message.text? As an example a message with text "ss" and attachment will have inside message.text ==> "ss [Attachment]"

POC:

https://github.com/Expensify/App/assets/12425932/c4f3b95d-a8ac-459c-a190-c78ef81d1b2b

melvin-bot[bot] commented 2 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~010803b7248a9b3406

melvin-bot[bot] commented 2 weeks ago

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

tsa321 commented 2 weeks ago

@sobitneupane Actually we can send multiple images in one report action. If we send a text with attachment then edit then copy paste the attachment part mutipples times, then we get multiples images. Example of the message :

Many images ![czNmcy1wcml2YXRlL3Jhd3BpeGVsX2ltYWdlcy93ZWJzaXRlX2NvbnRlbnQvcHUyMzMxNjM2LWltYWdlLTAxLXJtNTAzXzMtbDBqOXFrNnEucG5n.png](https://images.rawpixel.com/image_png_800/czNmcy1wcml2YXRlL3Jhd3BpeGVsX2ltYWdlcy93ZWJzaXRlX2NvbnRlbnQvcHUyMzMxNjM2LWltYWdlLTAxLXJtNTAzXzMtbDBqOXFrNnEucG5n.png)
![czNmcy1wcml2YXRlL3Jhd3BpeGVsX2ltYWdlcy93ZWJzaXRlX2NvbnRlbnQvcHUyMzMxNjM2LWltYWdlLTAxLXJtNTAzXzMtbDBqOXFrNnEucG5n.png](https://images.pexels.com/photos/2071873/pexels-photo-2071873.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=1)
![czNmcy1wcml2YXRlL3Jhd3BpeGVsX2ltYWdlcy93ZWJzaXRlX2NvbnRlbnQvcHUyMzMxNjM2LWltYWdlLTAxLXJtNTAzXzMtbDBqOXFrNnEucG5n.png](https://images.rawpixel.com/image_png_800/czNmcy1wcml2YXRlL3Jhd3BpeGVsX2ltYWdlcy93ZWJzaXRlX2NvbnRlbnQvcHUyMzMxNjM2LWltYWdlLTAxLXJtNTAzXzMtbDBqOXFrNnEucG5n.png)
![czNmcy1wcml2YXRlL3Jhd3BpeGVsX2ltYWdlcy93ZWJzaXRlX2NvbnRlbnQvcHUyMzMxNjM2LWltYWdlLTAxLXJtNTAzXzMtbDBqOXFrNnEucG5n.png](https://images.pexels.com/photos/45201/kitty-cat-kitten-pet-45201.jpeg)
![czNmcy1wcml2YXRlL3Jhd3BpeGVsX2ltYWdlcy93ZWJzaXRlX2NvbnRlbnQvcHUyMzMxNjM2LWltYWdlLTAxLXJtNTAzXzMtbDBqOXFrNnEucG5n.png](https://images.pexels.com/photos/1472999/pexels-photo-1472999.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=1)

copy paste that text and sent the text, you will get many images. Should we show the download button for this multiple images too? What is the expected result for this case?

sobitneupane commented 1 week ago

Thanks for bringing this up, @tsa321

@slafortune What would be the expected behavior if user uploads multiple images in a single action as suggested by @tsa321 here? My inclination is to display the Download option only if the message is a single attachment. If the message includes both text and an attachment, or if there are multiple attachments, it might be best not to show the Download option at all, which is the current state.

melvin-bot[bot] commented 1 week ago

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

slafortune commented 1 week ago

Thanks for bringing this up, @sobitneupane - we are going to do nothing for now. Here’s an issue for the next iteration of showing the in-lime image in the live preview - https://github.com/Expensify/App/issues/40181#top