Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

Fix incorrect link handling of inline MD images #4543

Merged
merged 3 commits into from
Jan 6, 2020

Conversation

abdusabri
Copy link
Contributor

@abdusabri abdusabri commented Dec 18, 2019

Summary

As of MM-12898, inline markdown images open the image preview modal, except if the markdown image itself is a link. e.g. [![Build Status](https://travis-ci.org/mattermost/platform.svg?branch=master)](https://travis-ci.org/mattermost/platform).

This PR fixes the following bug:

For a case like ![image](https://www.mattermost.org/wp-content/uploads/2016/03/logoHorizontal.png) an image plus some text that has [a link](https://somelink)

Observed behavior: The image doesn't have a pointer cursor, and doesn't open the preview modal

Expected behavior: The image should have a pointer cursor and open the preview modal

Reason of wrong behavior: just because there is a link ([a link](https://somelink)) in the markdown, it falsely assumed that the image itself is actually a link - which is not the case in the example given above

The bug is not a regression, it is a missed case since the implementation of MM-12898, but was only discovered after MM-15232, MM-18158. The issue can be reproduced on community V5.18 and latest master. The bug is not limited to channel header descriptions or its associated system messages, and can be reproduced in regular chat messages as well.

Ticket Link

https://mattermost.atlassian.net/browse/MM-21519

Screenshots

Before the fix
image

After the fix
image

@hanzei hanzei added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Dec 18, 2019
@jgilliam17 jgilliam17 added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Dec 26, 2019
Copy link
Contributor

@jgilliam17 jgilliam17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @abdusabri
Tested, looks good to merge.
Markdown has a pointer and opens in the preview as expected.

@jgilliam17 jgilliam17 added QA Review Done and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Dec 26, 2019
@mattermod
Copy link
Contributor

Test server destroyed

@mattermod
Copy link
Contributor

This issue has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @hanzei

Copy link
Contributor

@sudheerDev sudheerDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abdusabri 👍 Thanks for the PR.

@sudheerDev sudheerDev added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter Lifecycle/1:stale labels Jan 6, 2020
@sudheerDev
Copy link
Contributor

/update-branch

@jasonblais jasonblais added the AutoMerge used by Mattermod to merge PR automatically label Jan 6, 2020
@mattermod
Copy link
Contributor

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

@sudheerDev sudheerDev removed the AutoMerge used by Mattermod to merge PR automatically label Jan 6, 2020
@sudheerDev sudheerDev merged commit 63b56b5 into mattermost:master Jan 6, 2020
@sudheerDev sudheerDev added the CherryPick/Candidate A candidate for a quality or patch release, but not yet approved label Jan 6, 2020
@amyblais amyblais added CherryPick/Approved Meant for the quality or patch release tracked in the milestone and removed CherryPick/Candidate A candidate for a quality or patch release, but not yet approved labels Jan 6, 2020
@amyblais amyblais added this to the v5.19.0 milestone Jan 6, 2020
@amyblais
Copy link
Member

amyblais commented Jan 6, 2020

This can be cherry-picked to v5.19 as long as it's not risky. @sudheerDev

@sudheerDev
Copy link
Contributor

/cherry-pick release-5.19

@mattermod
Copy link
Contributor

Error trying doing the automated Cherry picking. Please do this manually

+++ Updating remotes...
Fetching upstream
Fetching origin
+++ Updating remotes done...
+++ Creating local branch automated-cherry-pick-of-#4543-upstream-release-5.19-1578335234
Branch 'automated-cherry-pick-of-#4543-upstream-release-5.19-1578335234' set up to track remote branch 'release-5.19' from 'upstream'.
+++ Downloading patch to /tmp/4543.patch (in case you need to do this again)

+++ About to attempt cherry pick of PR. To reattempt:
  $ git am -3 /tmp/4543.patch

Applying: Fix incorrect link handling of inline MD images
Using index info to reconstruct a base tree...
M	utils/__snapshots__/message_html_to_component.test.jsx.snap
M	utils/message_html_to_component.jsx
M	utils/message_html_to_component.test.jsx
Falling back to patching base and 3-way merge...
Auto-merging utils/message_html_to_component.test.jsx
CONFLICT (content): Merge conflict in utils/message_html_to_component.test.jsx
Auto-merging utils/message_html_to_component.jsx
CONFLICT (content): Merge conflict in utils/message_html_to_component.jsx
Auto-merging utils/__snapshots__/message_html_to_component.test.jsx.snap
Patch failed at 0001 Fix incorrect link handling of inline MD images
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

+++ Conflicts detected:

UU utils/message_html_to_component.jsx
UU utils/message_html_to_component.test.jsx

+++ Aborting in-progress git am.

+++ Returning you to the master branch and cleaning up.

@sudheerDev
Copy link
Contributor

@amyblais will manually cherrypick

sudheerDev pushed a commit that referenced this pull request Jan 6, 2020
@sudheerDev sudheerDev added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Jan 6, 2020
@amyblais amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Jan 9, 2020
@ogi-m ogi-m added the Tests/Done Release tests have been written label Jan 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Not Needed Does not require documentation QA Review Done Tests/Done Release tests have been written
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants