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

[Kshitij-Katiyar]

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

• Fixes https://github.com/mattermost/mattermost-plugin-jira/issues/773
• Pr #839

[mattermod]

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]

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

https://github.com/mattermost/mattermost-plugin-jira/pull/863#issuecomment-1200475963

Thank you!

[Nityanand13]

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]

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

[Nityanand13]

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

[mickmister]

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

[Nityanand13]

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

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]

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]

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.

[mickmister]

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]

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

[Nityanand13]

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

[mickmister]

@Nityanand13 So in the case of not removing the entries in config.json (we should assume that the admin is not going to do remove them), do the autolinks function properly?

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.

As long as the link operations are done correctly in both cases, it's fine to have both in the config. Other than general housekeeping, is there a reason why the admin would want to remove the existing autolinks?

Also mentioned this here https://github.com/mattermost/mattermost-plugin-autolink/pull/189#issuecomment-1278189485

[mattermod]

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]

@Nityanand13 So in the case of not removing the entries in config.json (we should assume that the admin is not going to do remove them), do the autolinks function properly?

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.

As long as the link operations are done correctly in both cases, it's fine to have both in the config. Other than general housekeeping, is there a reason why the admin would want to remove the existing autolinks?

Also mentioned this here mattermost/mattermost-plugin-autolink#189 (comment)

@mickmister In case we are not removing the mattermost-autolink field from config.json and restarting the server, then autolink functions properly for issue links but not for issue links containing a comment link in the URL. To make sure that autolink functions properly for the issue links containing a comment link as well, we should remove the mattermost-autolink field from config.json and restart the server.

[mickmister]

To make sure that autolink functions properly for the issue links containing a comment link as well, we should remove the mattermost-autolink field from config.json and restart the server.

@Nityanand13 This is not an option for Mattermost Cloud instances. We'll need to automate this somehow. Maybe the Autolink plugin needs to support an endpoint to delete them or something. There's also complications with HA and multiple instances of the plugin attempting these operations simultaneously. @hanzei @levb What do you think about this issue?

[hanzei]

If the autolink plugin would support a propper CRUD methods in its client, the JIRA plugin could fix the link issues programmatically. Maybe it's worth updating the autolink plugin, prepackaging the new version and then updating the links programmatically via JIRA.

[hanzei]

Opened https://github.com/mattermost/mattermost-plugin-autolink/issues/207 for Brightscout

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 31.22%. Comparing base (f986c15) to head (df69291). Report is 98 commits behind head on master.

:exclamation: Current head df69291 differs from pull request most recent head c0eb515. Consider uploading reports for the commit c0eb515 to get more accurate results

Files	Patch %	Lines
server/plugin.go	0.00%	21 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #871      +/-   ##
==========================================
- Coverage   31.46%   31.22%   -0.24%     
==========================================
  Files          49       49              
  Lines        5982     6027      +45     
==========================================
  Hits         1882     1882              
- Misses       3911     3956      +45     
  Partials      189      189              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.