Skip to content

[GH-773]: Fixed Issue Jira's autolink should support issue links that contain a comment link in the URL#871

Closed
Kshitij-Katiyar wants to merge 12 commits intomattermost:masterfrom
Brightscout:mm-773
Closed

[GH-773]: Fixed Issue Jira's autolink should support issue links that contain a comment link in the URL#871
Kshitij-Katiyar wants to merge 12 commits intomattermost:masterfrom
Brightscout:mm-773

Conversation

@Kshitij-Katiyar
Copy link
Copy Markdown
Contributor

@Kshitij-Katiyar Kshitij-Katiyar commented Sep 21, 2022

Summary

  • Currently the autolink config used by this plugin converts issue links to shorthand links, but it does not work when the originally posted URL has a comment link at the end of the posted URL. Fixed this issue updated the pattern for focusedCommentId as well.
  • This PR contains all the changes from Pr #839. So this PR alone is enough to fix this issue so we can close Pr #839 afterward.

Issue

#2)

* [MI-2119]: Jira's autolink should issue links that contain a comment link in the URL

* [MI-2119]: Review fixes
1. Chnaged the name of few variables

* [MI-2119]:Review fixes
1. Changed the name of a variable
@mattermod
Copy link
Copy Markdown
Contributor

Hello @Kshitij-Katiyar,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@javaguirre javaguirre self-requested a review September 29, 2022 08:07
Copy link
Copy Markdown
Contributor

@javaguirre javaguirre left a comment

Choose a reason for hiding this comment

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

Let me know when you can review this!

* [MI-2119]: Jira's autolink should issue links that contain a comment link in the URL

* [MI-2119]: Review fixes
1. Chnaged the name of few variables

* [MI-2119]:Review fixes
1. Changed the name of a variable

* [MI-2199]: Review fixes done
1. Added constants

* [MI-2199]:Review fixes
1. Added constants properly

* [MI-2119]: Review fixes done
1. Changed the name of the variable

* [MI-2119]: Review fixes done
1.Improved code quality

* [MI-2119]:Review Fixes
1. Changed the name of variable
@Kshitij-Katiyar Kshitij-Katiyar requested review from javaguirre and removed request for mickmister September 30, 2022 10:07
Copy link
Copy Markdown
Contributor

@javaguirre javaguirre 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 very much for the effort!

@javaguirre
Copy link
Copy Markdown
Contributor

Could you post a video on how to use this new change? Something like this so it's easier to QA.

#863 (comment)

Thank you!

@Nityanand13
Copy link
Copy Markdown
Contributor

Nityanand13 commented Oct 5, 2022

Could you post a video on how to use this new change? Something like this so it's easier to QA.

#863 (comment)

Thank you!

Vedio link for this PR:
https://drive.google.com/file/d/1ucoAzdEsrFd5L5Pw5W5XQPyw-6BVdCY_/view?usp=sharing

@javaguirre
Copy link
Copy Markdown
Contributor

Can't see the video, could you please check if you can upload a compatible format with Drive? Thank you @Nityanand13 !

@Nityanand13
Copy link
Copy Markdown
Contributor

Nityanand13 commented Oct 6, 2022

Can't see the video, could you please check if you can upload a compatible format with Drive? Thank you @Nityanand13 !

Vedio link for this PR:
Fixed_Issue_Jira_autolink_should_support_issue_links_that_contain_a_comment_link_in_the_URL.webm

I think now it should be visible

@javaguirre javaguirre added the 3: QA Review Requires review by a QA tester label Oct 11, 2022
@mickmister
Copy link
Copy Markdown
Contributor

@Kshitij-Katiyar I'm thinking we should have tests in the autolink repo to test these regex changes, like mattermost-community/mattermost-plugin-autolink#189. Please let me know your thoughts on this 👍

@Nityanand13
Copy link
Copy Markdown
Contributor

Nityanand13 commented Oct 18, 2022

@Kshitij-Katiyar I'm thinking we should have tests in the autolink repo to test these regex changes, like mattermost/mattermost-plugin-autolink#189. Please let me know your thoughts on this +1

@mickmister Test cases for these regex changes have already been written and it is even merged in the master as well. You can look at here: #L121. And there is no any changes in regex after that

@mickmister
Copy link
Copy Markdown
Contributor

Thanks @Nityanand13! Do you know the behavior of backwards compatibility for this feature? If I have existing autolinks present from previous versions of the Jira plugin, what is the result when I upgrade the plugin? Do I have any duplicate autolink entries?

@mattermod
Copy link
Copy Markdown
Contributor

This PR 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!

@Nityanand13
Copy link
Copy Markdown
Contributor

Thanks @Nityanand13! Do you know the behavior of backwards compatibility for this feature? If I have existing autolinks present from previous versions of the Jira plugin, what is the result when I upgrade the plugin? Do I have any duplicate autolink entries?

If I have an existing autolink present from the previous versions of the Jira plugin and when I upgrade the plugin with this feature then it is not working. To see this feature, we should remove the mattermost-autolink field from config.json of the mattermost server and restart the server after upgrading the plugin with the new autolink regex.

@hanzei hanzei requested a review from mickmister November 29, 2022 14:11
@hanzei hanzei added 2: Dev Review Requires review by a core committer and removed Lifecycle/1:stale labels Nov 29, 2022
@mickmister
Copy link
Copy Markdown
Contributor

If I have an existing autolink present from the previous versions of the Jira plugin and when I upgrade the plugin with this feature then it is not working. To see this feature, we should remove the mattermost-autolink field from config.json of the mattermost server and restart the server after upgrading the plugin with the new autolink regex.

@Nityanand13 The feature needs to be backwards compatible if possible. If there are breaking changes, we need to be clear with release notes about what the admin needs to do manually to have the plugin operating as intended. We'll also need to release the plugin as a new major version 4.0.0.

@mickmister
Copy link
Copy Markdown
Contributor

@Nityanand13 What is the behavior if the admin does not remove the entries from config.json?

@Nityanand13
Copy link
Copy Markdown
Contributor

Nityanand13 commented Dec 2, 2022

@Nityanand13 What is the behavior if the admin does not remove the entries from config.json?

@mickmister In the mattermost-autolink field inside config.json, we have the links field which is the array of objects that contains the information about the link that is supported like the template, pattern, scope, etc. When I upgrade the plugin with this feature then the information about the new types of links gets appended at the last of this array. And now if publish a comment link in the mattermost, then its pattern gets matched with the pattern of the issue link present earlier in the array while the pattern of the comment link is at the last of the array.
And when we remove the mattermost-autolink field from config.json and restart the server then that array gets modified such that the information about the comment link comes first.

Kshitij-Katiyar and others added 5 commits April 7, 2023 16:41
Copy link
Copy Markdown
Member

@wiggin77 wiggin77 left a comment

Choose a reason for hiding this comment

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

Let me know when this is ready - I'm removing from my queue for now.

@Kshitij-Katiyar
Copy link
Copy Markdown
Contributor Author

@avas27JTG @wiggin77 We have fixed this PR. Please review this

@Kshitij-Katiyar Kshitij-Katiyar added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester and removed Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) Lifecycle/frozen Contributor labels May 21, 2025
@AayushChaudhary0001
Copy link
Copy Markdown

While testing this PR it seems that the code changes are not working fine , let me know if I am missing something

@Kshitij-Katiyar
Copy link
Copy Markdown
Contributor Author

#1207 is a recreation of this PR with all the changes hence closing this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Jira's autolink should support issue links that contain a comment link in the URL