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

[HOLD for payment 2024-11-21] [$250] mWweb - Chat - From search page, tapping on image doesn't show image #51161

Closed
1 of 8 tasks
IuliiaHerets opened this issue Oct 21, 2024 · 25 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Oct 21, 2024

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


Version Number: V9. 0.51-1
Reproducible in staging?: Y
Reproducible in production?: Y
Issue reported by: Applause Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Tap on a chat
  3. Pate the text in compose box - test demo imagetest
  4. Send the message
  5. Navigate to LHN
  6. Tap bottom search icon
  7. From the dropdown, navigate to the chat
  8. Tap on the image

Expected Result:

From search page, tapping on image must show image.

Actual Result:

From search page, tapping on image doesn't show image.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6640158_1729418863865.Screenrecorder-2024-10-20-15-28-41-958_compress_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021848845902924647302
  • Upwork Job ID: 1848845902924647302
  • Last Price Increase: 2024-10-29
  • Automatic offers:
    • huult | Contributor | 104681846
Issue OwnerCurrent Issue Owner: @stephanieelliott / @stephanieelliott
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 21, 2024
Copy link

melvin-bot bot commented Oct 21, 2024

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

@IuliiaHerets
Copy link
Author

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

@mkzie2
Copy link
Contributor

mkzie2 commented Oct 21, 2024

Proposal

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

From search page, tapping on image doesn't show image.

What is the root cause of that problem?

We don't pass reportID to attachmentContextValue

const attachmentContextValue = {type: CONST.ATTACHMENT_TYPE.SEARCH};

Then we navigate to the attachment page with reportID as -1. As the result, the image doesn't show

thumbnailImageComponent
) : (
<ShowContextMenuContext.Consumer>
{({anchor, report, reportNameValuePairs, action, checkIfContextMenuActive, isDisabled}) => (
<AttachmentContext.Consumer>
{({reportID, accountID, type}) => (
<PressableWithoutFocus
style={[styles.noOutline]}
onPress={() => {
if (!source || !type) {
return;
}
const route = ROUTES.ATTACHMENTS?.getRoute(reportID ?? '-1', type, source, accountID, isAttachmentOrReceipt);

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

Pass the reportID to attachmentContextValue

const attachmentContextValue = {type: CONST.ATTACHMENT_TYPE.SEARCH, reportID: item?.reportID ?? '-1'};

const attachmentContextValue = {type: CONST.ATTACHMENT_TYPE.SEARCH};

What alternative solutions did you explore? (Optional)

@stephanieelliott stephanieelliott added the External Added to denote the issue can be worked on by a contributor label Oct 22, 2024
Copy link

melvin-bot bot commented Oct 22, 2024

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

@melvin-bot melvin-bot bot changed the title mWweb - Chat - From search page, tapping on image doesn't show image [$250] mWweb - Chat - From search page, tapping on image doesn't show image Oct 22, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 22, 2024
Copy link

melvin-bot bot commented Oct 22, 2024

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

@huult
Copy link
Contributor

huult commented Oct 23, 2024

Edited by proposal-police: This proposal was edited at 2024-10-23 17:19:12 UTC.

Proposal

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

From search page, tapping on image doesn't show image

What is the root cause of that problem?

This is a case of HTML rendering. The root cause of this issue is that the image does not have an extension in its source URL, and no file name is provided. As a result, the isFileImage condition check fails, causing the image not to display and the default attachment image to be shown instead.

We open this image from Search, and in this case, we use AttachmentView to render it. However, the condition check isFileImage failed for this attachment because:

1 The check for whether this source is an image file failed because it doesn't have an extension like PNG or JPG, so isSourceImage returned false. This is the source image for this ticket: https://camo.githubusercontent.com/4848d0f965f332077b77a1a0488c3e66b4769032104f4de6890bae218b4add8d/68747470733a2f2f70696373756d2e70686f746f732f69642f313036372f3230302f333030

  1. The condition check isFileNameImage is also false because this image doesn't have a fileName, as we did not send it when opening the AttachmentModal

the condition check at if (isFileImage) { fails, resulting in the default attachment being loaded, as described in this issue.

const isSourceImage = typeof source === 'number' || (typeof source === 'string' && Str.isImage(source));
const isFileNameImage = file?.name && Str.isImage(file.name);
const isFileImage = isSourceImage || isFileNameImage;
if (isFileImage) {

The reason we don't have a file name in this case is that we did not send the file name to the AttachmentModal when we opened the image.

const route = ROUTES.ATTACHMENTS?.getRoute(reportID ?? '-1', type, source, accountID, isAttachmentOrReceipt);

We don't have the originalFileName here:

We can load this image from the Home page because when opened from there, it uses AttachmentCarousel, which contains the extractAttachments function to retrieve file information, allowing it to load successfully.

const fileInfo = FileUtils.splitExtensionFromFileName(fileName);
if (!fileInfo.fileExtension) {
fileName = `${fileInfo.fileName || 'image'}.jpg`;
}

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

For those cases where the attachment doesn't have a source extension, we must provide a fileName for the attachment

To resolve this issue, AttachmentView needs to have a fileName, so we must send the fileName to this component. In this case, we will handle something like this:

  1. We get file name from htmlAttribs.src and send to AttachmentModal
//.src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.tsx#L60
+   let fileName = htmlAttribs[CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE] || FileUtils.getFileName(`${isAttachmentOrReceipt ? attachmentSourceAttribute : htmlAttribs.src}`);
+    const fileInfo = FileUtils.splitExtensionFromFileName(fileName);
+    if (!fileInfo.fileExtension) {
+        fileName = `${fileInfo.fileName || 'image'}.jpg`;
+    }
//.src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.tsx#L101
-       const route = ROUTES.ATTACHMENTS?.getRoute(reportID ?? '-1', type, source, accountID, isAttachmentOrReceipt);
+       const route = ROUTES.ATTACHMENTS?.getRoute(reportID ?? '-1', type, source, accountID, isAttachmentOrReceipt, fileName);
  1. Update routes to recive new param
//.src/ROUTES.ts#L312
-       getRoute: (reportID: string, type: ValueOf<typeof CONST.ATTACHMENT_TYPE>, url: string, accountID?: number, isAuthTokenRequired?: boolean) => {
+       getRoute: (reportID: string, type: ValueOf<typeof CONST.ATTACHMENT_TYPE>, url: string, accountID?: number, isAuthTokenRequired?: boolean, fileName?: string) => {
        ...
-        return `attachment?source=${encodeURIComponent(url)}&type=${type}${reportParam}${accountParam}${authTokenParam}` as const;
+        const fileNameParam = fileName ? `&fileName=${fileName}` : '';
+        return `attachment?source=${encodeURIComponent(url)}&type=${type}${reportParam}${accountParam}${authTokenParam}${fileNameParam}` as const;
}       ,
  1. Set fileName for AttachmentModal
//.src/pages/home/report/ReportAttachments.tsx#L37
+       const fileName = route.params.fileName;
        <AttachmentModal
            ...
+            originalFileName={fileName ?? ''}
        />

Here is test branch

Screen.Recording.2024-10-24.at.00.17.14.mov

@sobitneupane
Copy link
Contributor

Thanks for the proposal @mkzie2

Could you please add more depth to your RCA? It seems like you're mainly validating your solution in the RCA section.

If we pass reportID in attachmentContextValue, we will end up calling AttachmentCarousel instead of AttachmentView from AttachmentModal, which will display all attachments in the report not just the one clicked. I doubt that’s what we want. @shubham1206agra, it seems like you implemented this feature—was it a intentional decision not to pass reportID?

const attachmentContextValue = {type: CONST.ATTACHMENT_TYPE.SEARCH};

@huult
Copy link
Contributor

huult commented Oct 26, 2024

@sobitneupane , They can't update the reportId in attachmentContextValue because it calls AttachmentCarousel instead of AttachmentView. However, when opening an attachment in Search, only one attachment is opened, so we use AttachmentView in the current flow. In this ticket, the image is rendered as HTML and opened from Search, so we should use AttachmentView to open it, similar to other cases (e.g., Expensify sources or HTML images with file names). The issue occurs because the file name is missing and the source URL doesn't have an extension like PNG or JPG..., preventing it from opening. Therefore, we simply add a file name to the AttachmentModal so the image can open in AttachmentView for cases where the source URL doesn't have an extension. as proposed above.

@huult
Copy link
Contributor

huult commented Oct 26, 2024

Proposal Updated

  • Add more description to the described RCA and solution.

@sobitneupane can you review my proposal?
The short RCA: The condition check isFileName returned false in AttachmentView, so the default image is displayed. The reason it returned false is that this source name doesn’t have an extension (link below), and we did not send a file name for AttachmentModal.

https://camo.githubusercontent.com/4848d0f965f332077b77a1a0488c3e66b4769032104f4de6890bae218b4add8d/68747470733a2f2f70696373756d2e70686f746f732f69642f313036372f3230302f333030

Copy link

melvin-bot bot commented Oct 28, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Oct 28, 2024
Copy link

melvin-bot bot commented Oct 29, 2024

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

@stephanieelliott
Copy link
Contributor

Hey @sobitneupane any thoughts considering the context provided here?

Copy link

melvin-bot bot commented Oct 30, 2024

@sobitneupane, @stephanieelliott Eep! 4 days overdue now. Issues have feelings too...

@sobitneupane
Copy link
Contributor

Thanks for the proposal @huult

Proposal from @huult looks good to me.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Oct 31, 2024

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

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

melvin-bot bot commented Oct 31, 2024

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

@huult
Copy link
Contributor

huult commented Nov 1, 2024

Thank you all. I will create the PR within 24 hours

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Nov 1, 2024
@stephanieelliott
Copy link
Contributor

Hey @sobitneupane the PR is waiting for your review, can you take a look?

@stephanieelliott
Copy link
Contributor

PR has been merged to main

@garrettmknight
Copy link
Contributor

@puneetlath / @sobitneupane looks like your fix might be causing a crash here: #52475

@garrettmknight garrettmknight removed the Reviewing Has a PR in review label Nov 13, 2024
@melvin-bot melvin-bot bot added the Overdue label Nov 13, 2024
@garrettmknight garrettmknight added Daily KSv2 and removed Weekly KSv2 labels Nov 13, 2024
@melvin-bot melvin-bot bot removed the Overdue label Nov 13, 2024
@huult
Copy link
Contributor

huult commented Nov 13, 2024

@garrettmknight I think this bug is not related to this ticket because it occurs due to inputting a link with long text in the composer. I tried to reproduce it, and it's working well.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Nov 14, 2024
@melvin-bot melvin-bot bot changed the title [$250] mWweb - Chat - From search page, tapping on image doesn't show image [HOLD for payment 2024-11-21] [$250] mWweb - Chat - From search page, tapping on image doesn't show image Nov 14, 2024
Copy link

melvin-bot bot commented Nov 14, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.61-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-11-21. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Nov 14, 2024

@sobitneupane @puneetlath / @stephanieelliott @sobitneupane The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Nov 20, 2024
@huult
Copy link
Contributor

huult commented Nov 22, 2024

Hi @puneetlath @stephanieelliott , Sorry, I still have not received the payment. Can you help me check? Maybe we have an issue with Upwork?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Nov 22, 2024
@stephanieelliott
Copy link
Contributor

Summarizing payment on this issue:

  • Contributor: @huult $250 via Upwork - PAID!
  • Contributor+: @sobitneupane $250 via ND - please request!

Upwork job is here: https://www.upwork.com/jobs/~021848845902924647302

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Development

No branches or pull requests

7 participants