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

MM-17818 Adding API for adding/replacing links. #85

Merged
merged 7 commits into from
Feb 3, 2020
Merged

MM-17818 Adding API for adding/replacing links. #85

merged 7 commits into from
Feb 3, 2020

Conversation

crspeller
Copy link
Contributor

@crspeller crspeller commented Jan 23, 2020 •
edited

Summary

Adds an API route for replacing links. Some refactoring into packages to facilitate testing and easy imports for clients.

Example of use in JIRA PR: mattermost/mattermost-plugin-jira#453

@crspeller crspeller added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jan 23, 2020
@crspeller crspeller mentioned this pull request Jan 23, 2020
Merged
levb
levb reviewed Jan 23, 2020
View reviewed changes
go.mod Show resolved Hide resolved
levb
levb suggested changes Jan 23, 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.

2 big requests:

  1. reconsider subpackage naming
  2. support batch updates of links, maybe later add List/Delete

@crspeller
Copy link
Contributor Author

@levb

  1. Going to rename link to autolink. Others I think are fine.
  2. Not sure batch update is the right complexity tradeoff. The many config changes works for now. Probably rather do the move to storing in the KV store rather than the config?

@levb
Copy link
Contributor

levb commented Jan 23, 2020

I'm generally ok with the ^^, but I am concerned about what happens on community when you update the config? (all plugins on all servers are notified, each time). Agree that KV is the best answer.

@crspeller
Copy link
Contributor Author

Going to do that as a follow up ticket: https://mattermost.atlassian.net/browse/MM-22001

mickmister
mickmister reviewed Jan 24, 2020
View reviewed changes
server/api/api.go
for i := range links {
if links[i].Name == newLink.Name || links[i].Pattern == newLink.Pattern {
links[i] = newLink
found = true
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Was wondering how/where the uniqueness is checked

Copy link
Contributor

Choose a reason for hiding this comment

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

On that note, @crspeller we should not be saving what hasn't changed. I think it's just too wasteful, TBH. Add (content) de=duplication, maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

I added #88

@levb
Copy link
Contributor

levb commented Jan 25, 2020

so, is this where we left it?

mv link autolink
mv autolinkplugin plugin

cpoile
cpoile requested changes Jan 27, 2020
View reviewed changes
server/api/api.go Show resolved Hide resolved
server/api/api.go Outdated Show resolved Hide resolved
server/api/api.go Outdated Show resolved Hide resolved
@crspeller crspeller requested review from cpoile and levb January 27, 2020 17:22
levb
levb suggested changes Jan 30, 2020
View reviewed changes
server/api/api.go Outdated Show resolved Hide resolved
server/api/api.go Show resolved Hide resolved
server/api/api.go Show resolved Hide resolved
server/api/api.go Show resolved Hide resolved
server/api/api.go Show resolved Hide resolved
server/api/api.go Outdated Show resolved Hide resolved
server/api/api.go Show resolved Hide resolved
server/autolinkplugin/plugin.go Show resolved Hide resolved
@crspeller crspeller requested a review from levb January 30, 2020 22:48
levb
levb suggested changes Jan 30, 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.

Just one last request to consider - avoid saving if nothing changed?

server/api/api.go Outdated Show resolved Hide resolved
server/api/api.go Show resolved Hide resolved
server/api/api.go
for i := range links {
if links[i].Name == newLink.Name || links[i].Pattern == newLink.Pattern {
links[i] = newLink
found = true
Copy link
Contributor

Choose a reason for hiding this comment

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

On that note, @crspeller we should not be saving what hasn't changed. I think it's just too wasteful, TBH. Add (content) de=duplication, maybe?

@crspeller
Copy link
Contributor Author

@levb Fixed the issue where duplicates where being saved anyway. Also you convinced me that the header way is better.

@crspeller crspeller requested a review from levb January 31, 2020 01:34
levb
levb approved these changes Feb 2, 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.

LGTM, with the caveat of #88

server/api/api.go Outdated
ifPluginId := r.Context().Value(PluginIDContextValue)
pluginId, ok := ifPluginId.(string)
if ok && pluginId != "" {
pluginId := r.Header.Get("Mattermost-Plugin-ID")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

server/api/api.go
for i := range links {
if links[i].Name == newLink.Name || links[i].Pattern == newLink.Pattern {
links[i] = newLink
found = true
Copy link
Contributor

Choose a reason for hiding this comment

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

I added #88

@levb
check for "no change" (#88)
18825c3 
* check for "no change"

* fixed an error treating empty UserID

* minor
@crspeller crspeller removed the 2: Dev Review Requires review by a core committer label Feb 3, 2020
DHaussermann
DHaussermann approved these changes Feb 3, 2020
View reviewed changes
Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

I am having issues building this locally even after getting the branch for the corresponding functionality in the other repo.

Please merge and I will test from master and create follow-up issues as needed.

@DHaussermann DHaussermann removed the 3: QA Review Requires review by a QA tester label Feb 3, 2020
@crspeller crspeller merged commit 8c82b7d into master Feb 3, 2020
@crspeller crspeller deleted the mm-17818 branch February 3, 2020 18:30
@mickmister mickmister mentioned this pull request Feb 12, 2020
Closed
@jfrerich jfrerich mentioned this pull request Mar 19, 2020
Merged
3 tasks
@hanzei hanzei added the 4: Reviews Complete All reviewers have approved the pull request label Apr 30, 2020
@hanzei hanzei mentioned this pull request May 13, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mickmister mickmister mickmister left review comments

@levb levb levb approved these changes

@cpoile cpoile cpoile approved these changes

@DHaussermann DHaussermann DHaussermann approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@crspeller @levb @cpoile @mickmister @DHaussermann @hanzei