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.

Sign up for GitHub

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

Jump to bottom

Slash-commands now look for exact match #142

Closed
wants to merge 1 commit into from
Closed

Slash-commands now look for exact match #142

wants to merge 1 commit into from

Conversation

jrepe
Copy link

@jrepe jrepe commented Oct 24, 2020

Summary

The pull request fixes the issue where /autolink [command] doesn't look for exact match when manipulating links.

However, the current implementation might be too naive, because it'll affect all commands and not just those that manipulate links, so I'd appreciate feedback if that is desirable or not, otherwise I'll need to give more thought to how to fix this only for specific commands.

Ticket Link

Fixes #138

@jrepe jrepe requested a review from levb as a code owner October 24, 2020 19:50
@mattermod
Copy link
Contributor

Hello @jrepe,

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.

Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project.

Please help complete the Mattermost contribution license agreement?
Once you have signed the CLA, please comment with /check-cla and confirm that the CLA check is green.

This is a standard procedure for many open source projects.

Please let us know if you have any questions.

We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team.

@jrepe
Copy link
Author

jrepe commented Oct 24, 2020

/check-cla

@hanzei hanzei requested a review from jfrerich October 26, 2020 20:44
@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Oct 26, 2020
jfrerich
jfrerich approved these changes Oct 27, 2020
View reviewed changes
Copy link
Contributor

@jfrerich jfrerich left a comment

Choose a reason for hiding this comment

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

Nicely done :) Would you mind adding a test for this particular code change?

@jrepe
Copy link
Author

jrepe commented Oct 28, 2020

Of course, will do 👍

levb
levb approved these changes Oct 28, 2020
View reviewed changes
Copy link
Contributor

@levb levb left a comment

Choose a reason for hiding this comment

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

I will kinda miss the regex there, but I agree that it was probably more confusing in partial match situations.

LGTM

@levb levb removed the 2: Dev Review Requires review by a core committer label Oct 28, 2020
@levb levb requested review from aaronrothschild and DHaussermann and removed request for aaronrothschild October 28, 2020 13:10
@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!

/cc @jasonblais @jfrerich @emilyacook

@jasonblais
Copy link
Contributor

@DHaussermann kind reminder to help with reviews, thank you!

@DHaussermann
Copy link

Hey @jrepe thank you so much for fixing this issue! This bug was especially annoying.

I've done some testing and the issue is fixed but, I did find a small issue introduced by this solution. In cases where you're using list to do a search, the fact that it works as a begins with is actually useful. For example if I have Jira-ABC. Jira-123 and Jira-EXT I can type /autolink list Jira to show all 3 records

With this change, it will not pull them back as list must now be an exact match. I have tested this with all of the commands and it solves the issue for manipulating the data. But, would it be possible to make an exception for list to preserve the previous behavior?

@jrepe
Copy link
Author

jrepe commented Nov 9, 2020

Hey, thanks for the comment, I'll look into it. I imagined that this might have some consequences somewhere else that would be undesirable. Sorry for the delay though, I've had little time lately, but will get on this as soon as I can.

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

/cc @jasonblais @jfrerich @emilyacook

@jfrerich
Copy link
Contributor

How's it going, @jrepe? Sounds like you have been busy, but I just wanted to check in to see if you have any further questions or need any assistance?

@hanzei hanzei added the Awaiting Submitter Action Blocked on the author label Dec 2, 2020
@hanzei hanzei removed the request for review from DHaussermann December 2, 2020 10:38
@lindy65
Copy link

lindy65 commented May 26, 2021

Hi @jrepe - I'm helping review stale PRs and, as we haven't heard from you in 6 months, I'll close this PR for now. Please feel free to re-open it at any time if you're able to prioritize it.

I've re-added the "Up for Grabs" label to #138

//cc @jfrerich @levb

@lindy65 lindy65 closed this May 26, 2021
@mattermod mattermod removed the Awaiting Submitter Action Blocked on the author label May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfrerich jfrerich jfrerich approved these changes

@levb levb levb approved these changes

Assignees
No one assigned
Labels
3: QA Review Requires review by a QA tester Hacktoberfest hacktoberfest-accepted Lifecycle/1:stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Slash-commands not looking for exact match on link name
8 participants
@jrepe @mattermod @jasonblais @DHaussermann @jfrerich @lindy65 @levb @hanzei