[$250] Bug: some URL only comments are not editable reported by @kidroca

[kavimuru]

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. write a comment with the following content: https://www.expensify.com/chat-attachments
2. write other link only comments like https://www.google.com/

Expected Result:

Edit option appears for all the links

Actual Result:

Edit option does not appear for links with some content (https://www.expensify.com/chat-attachments)

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.2.19-1 Reproducible in staging?: y Reproducible in production?: y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/198048401-d48d92df-d91d-4c56-88d4-32351de8eb0a.mp4

https://user-images.githubusercontent.com/43996225/198048429-9f42c833-d4a3-4f4d-97f8-00871816e3a0.mp4

Expensify/Expensify Issue URL: Issue reported by: @kidroca Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1666720513413669

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @greg-schroeder (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[greg-schroeder]

It appears to be the hyphen - that is throwing things off - I tried many other URL variations and for whatever reason the hyphen is what's blocking editing

[melvin-bot[bot]]

Triggered auto assignment to @tgolen (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[melvin-bot[bot]]

Triggered auto assignment to @stephanieelliott (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[kidroca]

It appears to be the hyphen - that is throwing things off - I tried many other URL variations and for whatever reason the hyphen is what's blocking editing

Nope:

Similar links with hyphen that allow editing

Looks like it's specifically due to the text "chat-attachment" or "chat-attachments"

• I was able to edit the link until I made it end with /chat-attachments

[greg-schroeder]

Oh huh, I guess I was wrong ¯_(ツ)_/¯ ... I should have kept trying 😅

Hid my comment as off topic

[aimane-chnaif]

This is api issue. All urls that contain chat-attachments are considered as attachment, not link. And this bad data comes from pusher event after AddComment request

Here's log:

Also, this causes another bug: (shows [Attachment] as last message in LHN)

[stephanieelliott]

Oh interesting. What do you think @tgolen, if this theory is correct and it is in fact an API issue, can this still be fixed externally? Or do we need to change this to Internal?

[tgolen]

Yeah, It looks like it is probably the backend not doing a very thorough job of detecting attachment links. The code is here: https://github.com/Expensify/Web-Expensify/blob/1fbdfc6b3fa70e59e502f24edb1e3af21cdfe4e9/lib/ReportUtils.php#L1716 (private repo), but it looks like this:

    /**
     * Replaces all attachment tags with the text [Attachment]
     */
    public static function replaceAttachmentTagsWithText(?string $html = ''): string
    {
        $regexes = [
            '/<img[^>]* src=\"[^\"]*\"[^>]*>/', // <img> tags with a src attribute
            '/<a[^>]* href=\"[^>]*chat-attachments[^>]*\">[^<]+<\/a>/', // <a> tags with a href attribute that includes "chat-attachments"
        ];
        $replaceText = self::LAST_MESSAGE_TEXT_ATTACHMENT;

        foreach ($regexes as $regex) {
            $html = preg_replace($regex, $replaceText, $html ?? '');
        }

        return $html;
    }

We probably want that regex to also be testing that the host is an expensify.com host? Looks like this should be an internal issue.

[melvin-bot[bot]]

A Contributor Manager will be assigned to issue payment via Upwork if we deploy an associated Pull Request to production. Per Contributing.md.

[melvin-bot[bot]]

Current assignee @tgolen is eligible for the Demolition assigner, not assigning anyone new.

[stephanieelliott]

Cool, thanks Tim! Switched up the labels here.

[tgolen]

I can work on it this week, but I'm curious about what we think the regex should be changed to. cc @AndrewGable @iwiznia @flodnv @cead22. This would be my suggestion:

<a[^>]* href=\"[^>].*(?:expensify\.com\/chat-attachments){1}.*[^>]*\">[^<]+<\/a>

Which essentially requires "expensify.com/chat-attachments" to exist once in the URL for it to match the regex (also works fine for staging.expensify.com).

[iwiznia]

Sorry, I don't know what replaceAttachmentTagsWithText does / it's used for or how does that make the UI not show the edit icon.

[iwiznia]

Ah, sorry, I see it above...

Some things I don't get:

• href=\"[^>].* I think . should not be there, we want any char that's not >, not any char
• attachments){1} what's the {1} doing here?
• .*[^>]* I think the .* here is wrong and we should do: [^>]* for the same reason as the 1st one

[tgolen]

Honestly, there is a lot in this regex that I don't understand, so I'm very open to any improvements.

href=\"[^>].* I think . should not be there, we want any char that's not >, not any char

Do you know how to do that? I'm not sure how to accomplish that...

attachments){1} what's the {1} doing here?

My understanding is that it's requiring expensify\.com\/chat-attachments to exist exactly once

[kidroca]

Can't we just skip running this logic for comments and only run it for attachments ?

I see we use the AddComment command when I send an url like .../chat-attachments But when I actually make an attachment we use the AddAttachment command

If we only run the replaceAttachmentTagsWithText logic for AddAttachment commands, it'll never replace a comment message by mistake

---

I found the bug by copying image url, from one chat to another

https://user-images.githubusercontent.com/12156624/199194614-73f65cc3-d1ce-4856-9cc8-775f9b795e1e.mp4

If the copied expensify.com/chat-attachments url should be considered an attachment, shouldn't it appear as an attachment instead of a link? (E.g. I tried to share an attachment to another chat, instead of searching and picking the same file again)

[tgolen]

I guess that would be another solution to it as well. I see right now we are replacing that text on the string $sanitizedComment.' '.$attachmentHTML;. If we only did the replacement on the $attachmentHTML string, then it would essentially do what you are suggesting.

[cead22]

href=\"[^>].* I think . should not be there, we want any char that's not >, not any char

Do you know how to do that? I'm not sure how to accomplish that...

I think Ionatan's suggestion here was to simply remove the period, ie , href=\"[^>]*, so after href=\" it accepts "any character that isn't >, zero or more times"

[iwiznia]

I think Ionatan's suggestion here was to simply remove the period, ie , href="[^>]*, so after href=" it accepts "any character that isn't >, zero or more times"

Yep, exactly, I was already suggesting the change.

My understanding is that it's requiring expensify.com/chat-attachments to exist exactly once

I am confused because unless you enclose it in parenthesis and add +, * or a range, default is to match it exactly once.

[tgolen]

OK, cool. Thanks! So, this is my updated regex:

<a[^>]* href=\"[^>]*expensify.com\/chat-attachments[^>]*\">[^<]+<\/a>

and I'll only do the replacement on the $attachmentHTML and not the $sanitizedComment variable.

[michaelhaxhiu]

This seems to be moving forward!

@stephanieelliott just dropping a note as a reminder to keep the pressure on to find a contributor and get this one closed out. 👍

[iwiznia]

OK, cool. Thanks! So, this is my updated regex:

Almost! Small tweak:

<a[^>]* href=\"[^>]*expensify\.com\/chat-attachments[^>]*\">[^<]+<\/a>

[tgolen]

@michaelhaxhiu no worries on this one. It's going to be an internal fix.

@iwiznia Ah yes, thank you! Missing that one period to escape

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@tgolen, @stephanieelliott, @thesahindia Still overdue 6 days?! Let's take care of this!

[stephanieelliott]

I saw a few closed PR so went ahead and tested on staging, just noting that this is still occurring. So this in is still a WIP!

[tgolen]

@stephanieelliott thanks for testing that. I did some fixes, so I am not sure what exactly is still occurring. Can you add some more details about what exactly you tested and found wasn't working?