Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[issue-174] Add Jira Autolink Issue With Comment Test Case #185

Merged
merged 4 commits into from
Mar 29, 2022

Conversation

sibasankarnayak
Copy link
Contributor

added jira test for url with comment and made it project independent

ticket here
Fixes #174

@sibasankarnayak sibasankarnayak requested a review from levb as a code owner March 3, 2022 14:46
@mattermod
Copy link
Contributor

Hello @sibasankarnayak,

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.

@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2022

Codecov Report

Merging #185 (08b75cf) into master (affe9ee) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #185   +/-   ##
=======================================
  Coverage   41.49%   41.49%           
=======================================
  Files           6        6           
  Lines         670      670           
=======================================
  Hits          278      278           
  Misses        375      375           
  Partials       17       17           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update affe9ee...08b75cf. Read the comment docs.

@sibasankarnayak
Copy link
Contributor Author

sibasankarnayak commented Mar 10, 2022

@mickmister added the test for regex independent of project.

@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Mar 11, 2022
@mattermod
Copy link
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!

@catalintomai catalintomai added this to the v1.3.0 milestone Mar 23, 2022
@hanzei hanzei requested a review from catalintomai March 28, 2022 16:38
@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer Lifecycle/1:stale labels Mar 29, 2022
@hanzei hanzei merged commit e73e585 into mattermost-community:master Mar 29, 2022
Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

I have a few questions on this. Since this PR is a proxy for how we will implement this in the Jira plugin, I'd like to resolve concerns here first, even though this is already merged.

server/autolink/lib_jira_test.go Show resolved Hide resolved
"Comment url replacement",
autolink.Autolink{
Pattern: "(https://mattermost.atlassian.net/browse/)(?P<project_id>\\w+)(-)(?P<jira_id>\\d+)[?](focusedCommentId)(=)(?P<comment_id>\\d+)",
Template: "[${project_id}-${jira_id} With Comment #${comment_id}](https://mattermost.atlassian.net/browse/${project_id}-${jira_id}?focusedCommentId=${comment_id})",
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if having the comment id in the link text is that helpful. The capitalization of "With Comment" seems weird too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mickmister can you suggest a message for this , planning to raise a PR to fix your comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

[${project_id}-${jira_id} (comment)](https://mattermost.atlassian.net/browse/${project_id}-${jira_id}?focusedCommentId=${comment_id})

which would render as

MM-1337 (comment)

This is how GitHub does their comment links, such as #185 (comment)

server/autolink/lib_jira_test.go Show resolved Hide resolved
@hanzei
Copy link
Contributor

hanzei commented Apr 1, 2022

Sorry @mickmister , I should have waited for your review.

@sibasankarnayak
Copy link
Contributor Author

I have a few questions on this. Since this PR is a proxy for how we will implement this in the Jira plugin, I'd like to resolve concerns here first, even though this is already merged.

@mickmister sure i raised this PR with your suggested changes , wanted suggestion for message on comment like how should it look like.

This is the PR raised against your suggestion #189

@mickmister
Copy link
Contributor

Sorry @mickmister, I should have waited for your review.

No worries @hanzei. It took me quite a while to get to this 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Jira Autolink Issue With Comment Test Case
7 participants