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

Fixed /jira subcommand parsing consistency #45

Merged
merged 2 commits into from
May 8, 2019
Merged

Fixed /jira subcommand parsing consistency #45

merged 2 commits into from
May 8, 2019

Conversation

levb
Copy link
Contributor

@levb levb commented May 6, 2019

Cleaned up /jira sub-command parsing

Ticket: https://mattermost.atlassian.net/browse/MM-15335

@levb levb added the 2: Dev Review Requires review by a core committer label May 6, 2019
@levb levb requested review from cpoile and jfrerich May 6, 2019 17:31
@crspeller
Copy link
Member

@levb Considered using https://github.com/spf13/cobra ? That's what MM server uses.

@levb
Copy link
Contributor Author

levb commented May 6, 2019

@crspeller sure thing. IMO we should consider extracting some of this functionality into p.Helpers, and can then choose whatever implementation we want, right? Separate ticket?

@crspeller
Copy link
Member

@levb Certainly a separate ticket for moving stuff out to p.Helpers. But you might consider doing the cobra migration for this ticket. Your call.

@cpoile
Copy link
Member

cpoile commented May 6, 2019

@crspeller isn't Cobra for CLI, and this is for slash command parsing?

@crspeller
Copy link
Member

@cpoile Aren't slash commands like a CLI?

It have used cobra for this before. An example is matterbuild here: https://github.com/mattermost/matterbuild/blob/master/server/server.go

@cpoile
Copy link
Member

cpoile commented May 6, 2019

@crspeller that's awesome, didn't realize.. :)

Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Looks great to me, one minor non-blocking nit-pick.

func (ch CommandHandler) Handle(p *Plugin, c *plugin.Context, args ...string) *model.CommandResponse {
for n := len(args); n > 0; n-- {
h := ch.handlers[strings.Join(args[:n], "/")]
if h != nil {
Copy link
Member

Choose a reason for hiding this comment

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

nit: more idiomatic way:

if h, ok := ch.handlers[strings.Join(args[:n], "/")]; ok {
   return h(p, c, args[n:]...)
}

But 0/5 personally (because there's no chance you'll have a nil value instead of a missing entry).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree. In this case there is no semantic difference between "no entry" and a nil entry, so checking the value is the right way.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

@hanzei
Copy link
Collaborator

hanzei commented May 7, 2019

I didn't know a SetArgs method exists in Cobra. I was looking for a smarter way to parse slash commands for a long time. Locking forward to seeing this as a helper.

@levb levb merged commit 96bee47 into mattermost:jira2 May 8, 2019
@levb levb added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels May 8, 2019
@levb
Copy link
Contributor Author

levb commented May 8, 2019

Will consider COBRA later, as a separate PR

crspeller pushed a commit that referenced this pull request May 24, 2019
* JIRAv2 initial PR (#36)

* Refactored the JIRA plugin to support more events

* Removed Enabled config setting

* PR feedback

* PR feedback from @hanzei

* Added back webhook tests, updated for MD

* coverage

* Added back formatting as Slack attachments

* Add atlassian connect functionality

* Updated plugin.json

* merged the minimal webapp from hackathon

* Updated .gitignore

* wip Merged plugin.go from hackathon

* wip oauth failures

* wip oauth2 with a static clientid seems to work

* wip /jira connect seems to work

* wip trying JWT webhook setup

* wiop

* wip

* wip connect auth and the beginning of issue

* Added webapp from hackathon

* Create JIRA Issue is showing up

* wip create issue with semi-fakeuser mapping

* JWT verification in user-config page

* moved JWT verification into auth.go

* WIP WIP WIP in the middle of something

* wip flow works, need to add encryption

* User mapping works

* added /jira-disconnect

* Fix websocket event handling for connect and add disconnect

* WIP fixed websocket initialization

* WIP - TODO encrypt Atl account ID in config

* Secured the auth flow, passing mm_token and jwt to the final endpoint

* Restored legacy webhook support

* Process mentions in Webhooks, rearrange files

* Point Gopkg.toml at mattermost-server/master for now

* Adding comments for posts mentioning JIRA issues

* Some error logging

* Cleanup

* 5.8 compatibility: replace GetBundlePath with old-style config hacking

* go test cleanup

* Removed unused files

* Fixed MM-15004, no lnger requires admin to connect user

* wip prefixing and instances - appears to work with connect

* MM-15003 Prefix KV keys with the JIRA instance ID

- Reworked the kv store functions to use the JIRA BaseURL as a part of the
key.
- Other cleanup

* Added JIRA Server and OAuth1

- Added back OAuth1 functionality
- Added `type JIRAInstance` and moved a lot of instance-specific data
  there
- Added "cloud" and "server" instance types, for now with switches
- Added `/jira add server url` command to add JIRA Server instances

* wip style/naming

* JIRA Server auth appears to work

* Cosmetic PR feedback, fixed atlassian-connect.json

* Fixed a crash if no current JIRAInsttance exists

* GetJIRAClient refactor

* GetJIRACLient appears to work

* CreateIssue works in server and cloud

* Added (encoded) URL to Atlassian-connect.json `key` value (e.g. `"mattermost-https-e1ba36fb-ngrok-io"`), making it unique per Mattermost instance. Now can add multiple Mattermost instances to the same JIRA Cloud instance
* Removed caching of the project keys in anticipation of the new cache
* Webapp CreateIssue: use f.schema.system rather than f.key to identify field keys, this works with JIRA Server and Cloud versions.
* Webapp: CreateIssue renderFields: initialize `description` to an empty value to avoid JS errors

* PR feedback: ephf -> responsef

* Cleanup, /jira command improvements

- added `/jira help`
- added instance numbers to `/jira instance list`, ability to do
 `/jira instance select {number}`
- style: refactored command.go
- style: http naming, and using constants for all routes
- style: updated webapp to match
- style: eliminated unused oauth2.code

* Fixed 2 typos in 1 URL

* WIP prep to having a revoke button on the confirm page

- fixed a typo in `/jira instance listt` output
- removed unused routeOAuth1Connect and related code
- renamed StoreOAuth1RequestToken to StoreOneTimeSecret and such

* PR feedback (style), expire OTS

* PR feedback: Do not change original posts

- Do not change the original posts when creating issues
- require MM server 5.6 to build
- Style, error handling

* Fixed "Connect to JIRA" URL in post menu item

* PR feedback: added explicit Gopkg.toml deps

* PR feedback: style and error handling

* PR feedback: more style

* Fix tests.

* Fix linting.

* Removed notify() from webhooks for now, to pass the tests

* Uncommented the tests @cristopher took out

* Uncommented the test I missed

* Minor fixes

* Update README.md

* Revert accidental commit

* Update README.md (#38)

* Update README.md

* Update README.md

* Update README.md

* Update README.md

* quick fix of url

* correct the menu item used in Jira

* Proposed text updates for /jira slash commands (#41)

* Proposed text updates for /jira slash commands

Feedback welcome

* Address review feedback on command.go

* Some cleanup for Jira plugin (#42)

* Update instance_server.go

* JIRA -> Jira in error text

* JIRA > Jira, text update for Jira Server URL help text

* Update kv.go

* Update message_posted.go

* JIRA > Jira

* Update user_oauth1.go

* Update utils.go

* JIRA > Jira

* JIRA > Jira

* Update create_issue.jsx

* Update http.go

* JIRA > Jira

* MM-15244 Jira Cloud user connect security (#39)

* Adjusted the cloud auth flow as per spec

- changed cloud user mapping flow:
  1. serve a page that retrieves mm_token and auto-resubmits
     (`/ac/user_redirect.html`)
  2. (skipped/not implemented) a confirm page that would displat both
     usernames, and have a submit button
  3. connect the accounts, and serve a summary page with a "Disconnect"
     button (`/ac/user_connected.html`)
  4. disconnect the accounts if the above button is clicked,
     (`/ac/user_disconnected.html`)
- refactored http to use withXxxInstance
- refactored use of templates

* Added OTS to Jira Cloud user connect flow

- Added `Secret` field to `type AuthToken`
- Populate `Secret` with a random md5(256bytes) secret
- Verify and remove OTS
- Added a missing ./server/templates/ac/user_connected.html

* Added a confirm page in tthe flow

* Cleanup of Jira server connect flow

* PR feedback: style

* Careful with that lock, Eugene

* Style, govet, added a template I missed

* CAREFUL with that lock, Eugene

* resolved merge conflicts

* PR feedback: clarified respondWithTemplate

* PR feedback: md5->sha256

* PR feedback: separated Store[Current]JIRAInstance functions

* PR feedback: Like _really_ careful with that lock, Eugene

* Doc updates for roadmap and Jira Server install docs (#40)

* WIP: Doc updates for roadmap and Jira Server install docs

* Update README.md

* Update README.md

* Update based on feedback

* MM-15101 Adding /jira transition (#46)

* Adding /jira transition

* Moving transitionJiraIssue to a better home.

* MM-15253 Refactor of create-issue modal. (#44)

* Refactor of create-issue modal.

* Names feeback.

* Fixed /jira subcommand parsing consistency (#45)

* Fixed creating server/dist/templates/templates on a 2nd make (#50)

* Fixed creating server/dist/templates/templates on 2nd make

* make the cp switch portable

ref: https://unix.stackexchange.com/questions/18712/difference-between-cp-r-and-cp-r-copy-command

* small fixes to /jira instance delete (#52)

* Some improvments to cloud connect flow. (#55)

* MM-15096 UI fixes for template pages and menu item (#54)

* UI fixes for template and menu item

* Updating flexparent

* Updating templates

* Update Jira application link for Cloud (#48)

* (no ticket?) Fixed a bug in unescaping webhook secret (#53)

Also added `/jira webhook` command to see a URL custom-fit to the
current channel

* MM-15400 Added some protections to /installed link (#47)

* Added some protections to /installed link

* Forgot to reset the timeout back to 15min

* PR feedback: Installed flag

* Fixed s in URLs

* PR feedback: use  more consistently

* PR feedback: return a 403, not a 401 if already installed

* Update README.md

* Update README.md

* Add 'subject to change' for timeline in README

* Added response logging for Jira API errors (#59)

* * notifications working, added issue_created

* wip

* * new make lifecycle commands: debug, webapp-debug, reset, stop

* [MM-14773] [MM-15440] - simplify settings and setup (#57)

* [MM-14773] [MM-15440] - simplify settings and setup

* * restrict `/jira webhook` to system admins; some cleanup

* * remove public webhook instructions, remove generate webhook secret

* fixups

* Update server/command.go

Co-Authored-By: Jason Blais <13119842+jasonblais@users.noreply.github.com>

* Update server/command.go

Co-Authored-By: Jason Blais <13119842+jasonblais@users.noreply.github.com>

* PR comments

* * notification setting persisted, command line updated

* * disable notifications working

* Revert "* new make lifecycle commands: debug, webapp-debug, reset, stop"

This reverts commit 92b691d

* Fix govet

* Adding new CI

* Updating Makefile and moving to go modules

* Temporarily use jira2 as CI branch.

* fixing tests

* PR comments

* fixes needed after The Great Merge

* updated for firehose webhook (#61) now on master

* PR comments
@levb levb deleted the lev-MM-15335-command-syntax branch May 27, 2019 22:52
levb pushed a commit that referenced this pull request Jul 8, 2019
* JIRAv2 initial PR (#36)

* Refactored the JIRA plugin to support more events

* Removed Enabled config setting

* PR feedback

* PR feedback from @hanzei

* Added back webhook tests, updated for MD

* coverage

* Added back formatting as Slack attachments

* Add atlassian connect functionality

* Updated plugin.json

* merged the minimal webapp from hackathon

* Updated .gitignore

* wip Merged plugin.go from hackathon

* wip oauth failures

* wip oauth2 with a static clientid seems to work

* wip /jira connect seems to work

* wip trying JWT webhook setup

* wiop

* wip

* wip connect auth and the beginning of issue

* Added webapp from hackathon

* Create JIRA Issue is showing up

* wip create issue with semi-fakeuser mapping

* JWT verification in user-config page

* moved JWT verification into auth.go

* WIP WIP WIP in the middle of something

* wip flow works, need to add encryption

* User mapping works

* added /jira-disconnect

* Fix websocket event handling for connect and add disconnect

* WIP fixed websocket initialization

* WIP - TODO encrypt Atl account ID in config

* Secured the auth flow, passing mm_token and jwt to the final endpoint

* Restored legacy webhook support

* Process mentions in Webhooks, rearrange files

* Point Gopkg.toml at mattermost-server/master for now

* Adding comments for posts mentioning JIRA issues

* Some error logging

* Cleanup

* 5.8 compatibility: replace GetBundlePath with old-style config hacking

* go test cleanup

* Removed unused files

* Fixed MM-15004, no lnger requires admin to connect user

* wip prefixing and instances - appears to work with connect

* MM-15003 Prefix KV keys with the JIRA instance ID

- Reworked the kv store functions to use the JIRA BaseURL as a part of the
key.
- Other cleanup

* Added JIRA Server and OAuth1

- Added back OAuth1 functionality
- Added `type JIRAInstance` and moved a lot of instance-specific data
  there
- Added "cloud" and "server" instance types, for now with switches
- Added `/jira add server url` command to add JIRA Server instances

* wip style/naming

* JIRA Server auth appears to work

* Cosmetic PR feedback, fixed atlassian-connect.json

* Fixed a crash if no current JIRAInsttance exists

* GetJIRAClient refactor

* GetJIRACLient appears to work

* CreateIssue works in server and cloud

* Added (encoded) URL to Atlassian-connect.json `key` value (e.g. `"mattermost-https-e1ba36fb-ngrok-io"`), making it unique per Mattermost instance. Now can add multiple Mattermost instances to the same JIRA Cloud instance
* Removed caching of the project keys in anticipation of the new cache
* Webapp CreateIssue: use f.schema.system rather than f.key to identify field keys, this works with JIRA Server and Cloud versions.
* Webapp: CreateIssue renderFields: initialize `description` to an empty value to avoid JS errors

* PR feedback: ephf -> responsef

* Cleanup, /jira command improvements

- added `/jira help`
- added instance numbers to `/jira instance list`, ability to do
 `/jira instance select {number}`
- style: refactored command.go
- style: http naming, and using constants for all routes
- style: updated webapp to match
- style: eliminated unused oauth2.code

* Fixed 2 typos in 1 URL

* WIP prep to having a revoke button on the confirm page

- fixed a typo in `/jira instance listt` output
- removed unused routeOAuth1Connect and related code
- renamed StoreOAuth1RequestToken to StoreOneTimeSecret and such

* PR feedback (style), expire OTS

* PR feedback: Do not change original posts

- Do not change the original posts when creating issues
- require MM server 5.6 to build
- Style, error handling

* Fixed "Connect to JIRA" URL in post menu item

* PR feedback: added explicit Gopkg.toml deps

* PR feedback: style and error handling

* PR feedback: more style

* Fix tests.

* Fix linting.

* Removed notify() from webhooks for now, to pass the tests

* Uncommented the tests @cristopher took out

* Uncommented the test I missed

* Minor fixes

* Update README.md

* Revert accidental commit

* Update README.md (#38)

* Update README.md

* Update README.md

* Update README.md

* Update README.md

* quick fix of url

* correct the menu item used in Jira

* Proposed text updates for /jira slash commands (#41)

* Proposed text updates for /jira slash commands

Feedback welcome

* Address review feedback on command.go

* Some cleanup for Jira plugin (#42)

* Update instance_server.go

* JIRA -> Jira in error text

* JIRA > Jira, text update for Jira Server URL help text

* Update kv.go

* Update message_posted.go

* JIRA > Jira

* Update user_oauth1.go

* Update utils.go

* JIRA > Jira

* JIRA > Jira

* Update create_issue.jsx

* Update http.go

* JIRA > Jira

* MM-15244 Jira Cloud user connect security (#39)

* Adjusted the cloud auth flow as per spec

- changed cloud user mapping flow:
  1. serve a page that retrieves mm_token and auto-resubmits
     (`/ac/user_redirect.html`)
  2. (skipped/not implemented) a confirm page that would displat both
     usernames, and have a submit button
  3. connect the accounts, and serve a summary page with a "Disconnect"
     button (`/ac/user_connected.html`)
  4. disconnect the accounts if the above button is clicked,
     (`/ac/user_disconnected.html`)
- refactored http to use withXxxInstance
- refactored use of templates

* Added OTS to Jira Cloud user connect flow

- Added `Secret` field to `type AuthToken`
- Populate `Secret` with a random md5(256bytes) secret
- Verify and remove OTS
- Added a missing ./server/templates/ac/user_connected.html

* Added a confirm page in tthe flow

* Cleanup of Jira server connect flow

* PR feedback: style

* Careful with that lock, Eugene

* Style, govet, added a template I missed

* CAREFUL with that lock, Eugene

* resolved merge conflicts

* PR feedback: clarified respondWithTemplate

* PR feedback: md5->sha256

* PR feedback: separated Store[Current]JIRAInstance functions

* PR feedback: Like _really_ careful with that lock, Eugene

* Doc updates for roadmap and Jira Server install docs (#40)

* WIP: Doc updates for roadmap and Jira Server install docs

* Update README.md

* Update README.md

* Update based on feedback

* MM-15101 Adding /jira transition (#46)

* Adding /jira transition

* Moving transitionJiraIssue to a better home.

* MM-15253 Refactor of create-issue modal. (#44)

* Refactor of create-issue modal.

* Names feeback.

* Fixed /jira subcommand parsing consistency (#45)

* Fixed creating server/dist/templates/templates on a 2nd make (#50)

* Fixed creating server/dist/templates/templates on 2nd make

* make the cp switch portable

ref: https://unix.stackexchange.com/questions/18712/difference-between-cp-r-and-cp-r-copy-command

* small fixes to /jira instance delete (#52)

* Some improvments to cloud connect flow. (#55)

* MM-15096 UI fixes for template pages and menu item (#54)

* UI fixes for template and menu item

* Updating flexparent

* Updating templates

* Update Jira application link for Cloud (#48)

* (no ticket?) Fixed a bug in unescaping webhook secret (#53)

Also added `/jira webhook` command to see a URL custom-fit to the
current channel

* MM-15400 Added some protections to /installed link (#47)

* Added some protections to /installed link

* Forgot to reset the timeout back to 15min

* PR feedback: Installed flag

* Fixed s in URLs

* PR feedback: use  more consistently

* PR feedback: return a 403, not a 401 if already installed

* Update README.md

* Update README.md

* Add 'subject to change' for timeline in README

* Added response logging for Jira API errors (#59)

* [MM-14773] [MM-15440] - simplify settings and setup (#57)

* [MM-14773] [MM-15440] - simplify settings and setup

* * restrict `/jira webhook` to system admins; some cleanup

* * remove public webhook instructions, remove generate webhook secret

* fixups

* Update server/command.go

Co-Authored-By: Jason Blais <13119842+jasonblais@users.noreply.github.com>

* Update server/command.go

Co-Authored-By: Jason Blais <13119842+jasonblais@users.noreply.github.com>

* PR comments

* Added /jira assign <issue-key> <assignee> command
Upgrade go-jira version

* Use NewDecoder in place of unmarshaling
move /jira assign help to non sys admin section
use /api/3/ in place of /api/2/
return string from assignJiraIssue and have caller wrap in responsef()

* Move defer to before NewDecoder function

* Clean up nits.
Pass jiraUsers pointer to Do function and have that function handle
decode and defer of resp.Body.Close

* Added /jira assign <issue-key> <assignee> command
Upgrade go-jira version

* Use NewDecoder in place of unmarshaling
move /jira assign help to non sys admin section
use /api/3/ in place of /api/2/
return string from assignJiraIssue and have caller wrap in responsef()

* Move defer to before NewDecoder function

* Clean up nits.
Pass jiraUsers pointer to Do function and have that function handle
decode and defer of resp.Body.Close

* Delete file.  It is not in master

* Add support for assigning issues to jira users
  - if if multiple returned users, print the display names as bullet list
  - limit number of users shown to 10

* gofmt fix

* Reword response to user
levb pushed a commit that referenced this pull request Aug 10, 2020
* fetch instances on modal open

* Fix errors regarding redux update and missing channelId prop

* Get instances when subscribe modal opens

* fix tests

* Handle case where default instance value in frontend is stale (uninstalled)

* Make if statement more safe

* Make if statement more safe

* fix test

* lint
levb added a commit that referenced this pull request Aug 10, 2020
* iterate through instances when printing out the channel subscriptions
update tests for new printing output format

don't require --intance for subscribelist

* Update tests

* shorten field name

* several PR feedback fixes

* remove unused variable
rename map variable

* Disconnect users on instance uninstall (#41)

* [GH-4] add EE license check (#22)

* restrict plugin activation based on license

* run go mod tidy -v

* wip

* add enterprise package

* rename license naming to enterprise
if user does not have enterprise license, check number of instances
  installed before allowed to install another cloud or server instance

* remove function

* fix review feedback

* Add license checking tests

* wip

* wip by Lev

* wip

* wip

* remove comments

* remove function to return pointer to true bool value

* getMockInstanceStoreKV(0) should return initialized empty store
was returning instance store with one instance
correct all occurences call of getMockInstanceStoreKV(0) to call with 1
instanance. This fixes many failing tests
Add tests for InstallInstance function with extensive license checking

* fix linting

Co-authored-by: Lev Brouk <lev@mattermost.com>

* Fixed #31, #33 (#54)

- Use plugin URL as the "home" for app links
- Added extra logging for suspicious callbacks
- Use templates for `/jira instance install` output

* Fix test (#64)

* GH-21 Fixed websocket update on user disconnect, instance uninstall (#61)

* Fixed websocket update on user disconnect

* Revert "Fixed websocket update on user disconnect"

This reverts commit 84ca4b27ca2a3ed55408bf359786b3162dc1b29a.

* Fixed empty set refresh, defaulting in commands

* fixed instance status update

* GH-49: Fixed webhook, transition commands (#67)

* Use connectInstances array for userConnected redux selector (#71)

* Use connectInstances array for userConnected redux selector

* add null check

* [GH-45] Make sure frontend has up-to-date default instance data (#65)

* fetch instances on modal open

* Fix errors regarding redux update and missing channelId prop

* Get instances when subscribe modal opens

* fix tests

* Handle case where default instance value in frontend is stale (uninstalled)

* Make if statement more safe

* Make if statement more safe

* fix test

* lint

* GH-60: Fixed multi-work command alias parsing in webapp (#72)

* Fixed --instance

Co-authored-by: Lev <1187448+levb@users.noreply.github.com>
Co-authored-by: Lev Brouk <lev@mattermost.com>
Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com>
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.

5 participants