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

[GH-57] add autocomplete #62

Merged
merged 7 commits into from
Aug 3, 2020
Merged

[GH-57] add autocomplete #62

merged 7 commits into from
Aug 3, 2020

Conversation

jfrerich
Copy link
Contributor

Summary

This PR adds autocomplete to the confluence plugin.

A new endpoint is added to handle the dynamic retrieving of subscription aliases for the edit and unsubscribe commands.

Ticket Link

Fixes #57

jfrerich added 2 commits July 13, 2020 17:08
@jfrerich
Add slash commands
1868f66 
Does not include implemented http endpoint for subscription name autocomplete
@jfrerich
Add autocomplete endpoint for subscription names
dc5be5f
@jfrerich jfrerich added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jul 14, 2020
@jfrerich jfrerich requested review from iomodo and mickmister July 14, 2020 22:06
@jfrerich jfrerich changed the title Gh 57 autocomplete [GH-57] add autocomplete Jul 14, 2020
@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2020 •
edited
Loading

Codecov Report

Merging #62 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #62   +/-   ##
=======================================
  Coverage   29.48%   29.48%           
=======================================
  Files           8        8           
  Lines         234      234           
=======================================
  Hits           69       69           
  Misses        153      153           
  Partials       12       12

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 05cc4cf...d2f7405. Read the comment docs.

jfrerich
jfrerich commented Jul 14, 2020
View reviewed changes
server/controller/get_subscriptions.go
}

func handleGetChannelSubscriptions(w http.ResponseWriter, r *http.Request) {
channelID := r.FormValue("channel_id")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iomodo, I'm not sure of the exact commit needed from master, but I believe this is the functionality that is not currently in a released version?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm out of the loop here. Can you please explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a change to autocomplete that now passes the channel_id to the URL as a query param. This was only recently introduced. Without this new feature, you have no way of detecting the current channel for which you are typing an autocomplete command.

Copy link

Choose a reason for hiding this comment

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

True, changes are for 5.26 so you can grab the lates master for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jfrerich @iomodo Do we need to update the minimum server version because of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we needed to update the min server version. I committed the min_server_version to 5.26.0.

What is the correct action with respect to merging? I added the Do Not Merge/Awaiting PR label. We can merge post 5.26.0 release. How does that sound and is that necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iomodo, @mickmister

  • updated min_server_version to v5.26.0 in plugin.json
  • updated go.mod with latest server version commit via go get github.com/mattermost/mattermost-server/v5@93a537a636299e1e049505bdaf072f8d3ae974a4

mickmister
mickmister approved these changes Jul 15, 2020
View reviewed changes
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.

LGTM, just a few non-blocking nits

server/controller/get_subscriptions.go Outdated Show resolved Hide resolved
server/controller/get_subscriptions.go
}

func handleGetChannelSubscriptions(w http.ResponseWriter, r *http.Request) {
channelID := r.FormValue("channel_id")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm out of the loop here. Can you please explain?

server/controller/get_subscriptions.go Outdated Show resolved Hide resolved
server/command/command.go Outdated Show resolved Hide resolved
@jfrerich jfrerich requested a review from mickmister July 16, 2020 02:15
iomodo
iomodo approved these changes Jul 16, 2020
View reviewed changes
Copy link

@iomodo iomodo left a comment

Choose a reason for hiding this comment

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

LGTM!

@jfrerich
update min server version. This is needed for the new autocomplete
8fc3c01 
feature which allows getting the current channel_id
@jfrerich jfrerich added the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Jul 16, 2020
@jfrerich jfrerich removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Jul 17, 2020
hanzei
hanzei reviewed Jul 22, 2020
View reviewed changes
go.mod
@@ -5,7 +5,7 @@ go 1.13
require (
bou.ke/monkey v1.0.2
github.com/gorilla/mux v1.7.4
github.com/mattermost/mattermost-server/v5 v5.24.1
github.com/mattermost/mattermost-server/v5 v5.3.2-0.20200717105608-93a537a63629
Copy link
Collaborator

Choose a reason for hiding this comment

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

What changes in mattermost-server that are only in master are needed for this PR?

Copy link

Choose a reason for hiding this comment

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

getting the channel_id in the dynamic argument endpoint would be the one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it 👍

@hanzei hanzei removed the 2: Dev Review Requires review by a core committer label Jul 25, 2020
@hanzei hanzei added this to the v1.3.0 milestone Jul 25, 2020
@jfrerich jfrerich requested a review from DHaussermann July 28, 2020 18:01
DHaussermann
DHaussermann approved these changes Jul 30, 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.

Tested and passed

  • Slash command auto complete works as expected
  • Tested that all commands have been included
  • Briefly smoke tested the functionality
    no issues found
    LGTM!

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Jul 30, 2020
@jfrerich jfrerich merged commit 014c742 into master Aug 3, 2020
@jfrerich jfrerich deleted the GH-57-autocomplete branch August 3, 2020 14:17
@jfrerich jfrerich mentioned this pull request Aug 5, 2020
Open
This was referenced Nov 29, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanzei hanzei hanzei left review comments

@iomodo iomodo iomodo approved these changes

@DHaussermann DHaussermann DHaussermann approved these changes

@mickmister mickmister mickmister approved these changes

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

Successfully merging this pull request may close these issues.

Add slash command autocomplete functionality
6 participants
@jfrerich @codecov-commenter @iomodo @mickmister @hanzei @DHaussermann