App: [HOLD for payment 2023-06-08] [$1000] Italic [Attachment] message is considered as attachment and app crashes on click of download for that message

These are all paid. Closing.

@rushatgabhane I think we can fix that. And I think the “Uploading attachment…” for download buttons is something that is happening on main already. We can fix that as well. I mentioned both concerns (edit & download) here.

I was thinking something like:

isReportMessageAttachment

    return text === CONST.ATTACHMENT_MESSAGE_TEXT && (!!html.match(regex) || html === 'Uploading attachment...');

ContextMenuActions shouldShow download

    return isAttachment && message.html !== 'Uploading attachment...' && reportAction.reportActionID;

Okay yeah I think that’s fair, let’s proceed for now with @eh2077’s PR and full payment, and we can figure out the rest later. Will assign you now!

Backend fix still being reviewed (we had to do a reviewer swap coz of some dev environment issues). Should be soon! I’ll actually put this on HOLD until that’s up, just to make things clear

Cool! @Prince-Mendiratta can you apply here? https://www.upwork.com/jobs/~018d53d5272968557e

Oh right - @lschurr we were going to pay @Prince-Mendiratta a bit of a bonus for helping out, I think $250 should be okay if that’s alright with you?

@lschurr no we don’t need a regression test.

P.S. applied to the job using dad’s account - “Satish Gabhane”

Hi @lschurr, I already have a offer for the job on upwork.

merged!

What we could do instead is - don’t show any edit message option for the Uploading attachment... message. (I don’t think that’s possible tho)

@dangrous @s77rt I don’t think we can fix the “regression”. If we fix it, then we’ll cause a similar bug - “Download button shown on Uploading attachment... message”

Oh that’s a weird regression. I mean weird that we coded it that way in the first place. Tricksy.

Given that we’re within the regression period we should figure out that first and then we can get you sorted, @Prince-Mendiratta

I commented in the linked issue https://github.com/Expensify/App/issues/19379#issuecomment-1562067941

@lschurr Applied. Thanks!

Hi @lschurr, thanks I have just applied.

Removing you @joekaufmanexpensify since I’m back from OOO.

I’m heading OOO from May 17-23rd. I’ve re-added the Bug label to re-assign the issue and kept myself as assigned as well. If this is still open on May 24th, I’ll take it back from you @joekaufmanexpensify

Latest update: PR just merged, waiting for auto-messages for BZ checklist/payment.

Wow it happened again 😅 @eh2077

Cool! That’s in review, with the update to the regex since that does seem to catch more instances. Not sure it’s foolproof but we’ll take what we can get. At this point someone would really have to be trying to get their app to break.

Also, I realized that using the string in CONST.ATTACHMENT_SOURCE_ATTRIBUTE isn’t perfect, as you can still fool it by writing [[Attachment]](https://www.google.com/data-expensify-source) or something else that manually includes that phrase in the html, but I think we’re much closer and you’re getting into really weird edge cases at that point.

That’s true and I also pointed out this in my proposal https://github.com/Expensify/App/issues/17289#issuecomment-1505972593 and suggested to use a regex to check if there’s the special property inside an anchor tag.

Okay backend fix is in process! We’ll definitely need some front end fixes still for at least that ContextMenuActions stuff (when I was testing, I no longer saw the download icon for _[Attachment]_ but I ALSO didn’t see the edit message) and maybe the other places too.

Also, I realized that using the string in CONST.ATTACHMENT_SOURCE_ATTRIBUTE isn’t perfect, as you can still fool it by writing [[Attachment]](https://www.google.com/data-expensify-source) or something else that manually includes that phrase in the html, but I think we’re much closer and you’re getting into really weird edge cases at that point.

@Prince-Mendiratta yes!

I think External should be fine!