[Ready for Payment][$250] Android - Chat - On sending attachment+text, on edit url is not shown as hyperlink

[lanitochka17]

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: 9.0.14
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/4787789
Email or phone of affected tester (no customers): N/A
Issue reported by: Applause - Internal Team

Action Performed:

1. Launch app
2. Tap on a report
3. Enter a url + upload a image and send the message
4. Long press and edit the message sent
5. Enter a url and email id along with previous url entered and save the message

Expected Result:

On sending attachment+text, on edit url must be shown as hyperlink

Actual Result:

On sending attachment+text, on edit url is not shown as hyperlink

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 evidence

Bug6556677_1722285950066.Screenrecorder-2024-07-30-02-04-50-96_compress_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a793178e3d46661b
• Upwork Job ID: 1821194033275560441
• Last Price Increase: 2024-08-07

Issue Owner

Current Issue Owner: @JmillsExpensify

[melvin-bot]

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

[lanitochka17]

We think that this bug might be related to #vip-vsp

[lanitochka17]

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

[bernhardoj]

Proposal

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

Adding a link before the image markdown doesn't convert the link, example:

google.com
![alt](source.com)

What is the root cause of that problem?

This issue comes from the ExpensiMark autolink markdown regex. The regex doesn't allow a self-closing tag after the link. The image markdown is converted to <img ... /> which is a self-closing tag, so the link is never matched.

It was updated to solve this issue.

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

The issue that the self-closing tag regex is trying to solve looks like doesn't even happen anymore, so we can remove the self-closing tag regex .+\\/> from the autolink regex.

[melvin-bot]

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

[melvin-bot]

@JmillsExpensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[JmillsExpensify]

I'll open up this issue for proposal review since we have one already. If it's an easy fix we can address it. Otherwise, I think we should close the issue.

[melvin-bot]

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

[melvin-bot]

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

[getusha]

@bernhardoj I don't see any issues after applying the change too.
But i noticed it's not adding the text before the link as alt attribute of the image

[getusha]

Same issue on staging, do you think it's a bug and simple enough to handle it here?

[bernhardoj]

I can repro that and the img tag itself has the alt attribute,

but our ImageRenderer never uses that (not sure why). We can fix it by passing the alt as accessibilityLabel to the image.

[getusha]

@bernhardoj the solution is working fine, but it's not highlighting the link in the composer if it's placed before the image.

[bernhardoj]

@getusha we would need to update the live markdown to use the new update from expensify-common. You can test it locally by updating node_modules/@expensify/react-native-live-markdown/lib/parser/react-native-live-markdown-parser.js and search for the "autolink" rule.

[melvin-bot]

@JmillsExpensify @getusha 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!

[getusha]

@bernhardoj's proposal looks good to me.
🎀 👀 🎀 C+ Reviewed!

[melvin-bot]

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

[dangrous]

ah okay cool - if we don't have that bug anymore I'm fine with this fix. To double check, though, we should include testing steps for that issue as well as the issue raised here. Assigning!

[bernhardoj]

expensify-common PR is ready

cc: @getusha

[melvin-bot]

@JmillsExpensify, @dangrous, @bernhardoj, @getusha Eep! 4 days overdue now. Issues have feelings too...

[dangrous]

Not overdue, Expensify/expensify-common#782 was just merged. I think we need an additional app PR to bump the version to 2.0.76 right?

[bernhardoj]

@dangrous We need to bump the version in live markdown first. I have opened the live markdown PR here

cc: @getusha

[dangrous]

ah great, I'll take a look at that shortly and merge

[bernhardoj]

The App PR is ready

cc: @getusha

[dangrous]

This looks like it was affected by the missing automation on prod deploy. I think we're in the 7 day waiting period @JmillsExpensify? But not sure when it started.

[dangrous]

I'm pretty sure this one has completed the 7-day regression period, but not 100%. @JmillsExpensify do you know if there's an easy way to confirm? There was discussion about this somewhere...

[dangrous]

Now this is definitely ready to go.

[bernhardoj]

Requested in ND.

[garrettmknight]

Payment Summary:

• Contributor: @bernhardoj $250
• Reviewer: @getusha $250

[JmillsExpensify]

$250 approved for @bernhardoj

[JmillsExpensify]

@getusha do you mind filling out the BZ checklist?

[getusha]

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:

[@] The PR that introduced the bug has been identified. Link to the PR: Expensify/expensify-common#661
[@] 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: N/a
[@] 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: N/a
[@] Determine if we should create a regression test for this bug. No, i don't think this issue is impactful. it's more like an edge case.
[@] 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. N/a

[garrettmknight]

$250 approved for @getusha