[MM 15099] direct message notifications on off #62

cpoile · 2019-05-17T19:47:21Z

Summary

Add "/jira settings notifications [on/off]" command, and re-enable personal notifications

turned DMs back on
a notifications setting which is persisted in the KV store
a new UserSettings struct to keep this (and future) settings. Could have added it to the JIRAUser struct, but wanted to keep Jira-specific settings separate from Mattermost-specific settings.
chose to put the notification check in the createBotDMPost function instead of earlier -- this way the code is one place vs. many, at the cost of a couple extra calls before the check.
apologies for the multiple commits -- I couldn't rebase without having conflicts -- I think its because I merged jira2 yesterday.
ignore the testdata file changes -- that's just formatting

Todo

when we merge MM-15462 Basic subscribe to jira events. #61 I just need to add a few lines to make this work with v2 webhooks (which I'll do before we merge this)

Needs advice:

I am loading the JiraInstance (https://github.com/mattermost/mattermost-plugin-jira/pull/62/files#diff-afb678e7f5aecce88e775e4aa8c70174R317) in order to find the mattermostId we need to DM. I couldn't think of a clean way to mock LoadCurrentJiraInstance, so I'm not throwing an error when we can't find one. I'm open to any thoughts on how to mock it (or any other solution -- maybe changing LoadCurrentJiraInstance?). I kept the alternative (which I'll remove or include before merging, depending on what we decide is the best way to go).

Links

https://mattermost.atlassian.net/browse/MM-15099

@hanzei

* 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 * Update README.md * Update README.md

* Proposed text updates for /jira slash commands Feedback welcome * Address review feedback on command.go

* 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

* 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

* WIP: Doc updates for roadmap and Jira Server install docs * Update README.md * Update README.md * Update based on feedback

* Adding /jira transition * Moving transitionJiraIssue to a better home.

* Refactor of create-issue modal. * Names feeback.

…ost#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

* UI fixes for template and menu item * Updating flexparent * Updating templates

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

* 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

* [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

# Conflicts: # server/command.go

levb

Mostly LGTM.

I do not believe one needs to do r.Body.Close() in the server, it may even be hurtful for connection management. I looked it up in the source a while ago to be sure, but please double-check.

server/atlassian_connect.go

server/command.go

server/issue.go

…tions-on-off # Conflicts: # README.md # server/command.go # server/http.go # server/issue.go # server/templates/ac/user_confirm.html # server/templates/ac/user_connected.html # server/templates/oauth1/complete.html # server/user.go # server/user_cloud.go # server/user_server.go # server/utils.go # server/webhook.go # webapp/src/action_types/index.js # webapp/src/actions/index.js # webapp/src/components/jira_field.jsx # webapp/src/components/jira_fields.jsx # webapp/src/components/modals/create_issue/create_issue.jsx # webapp/src/components/post_menu_actions/create_issue/create_issue.jsx # webapp/src/components/react_select_setting.jsx # webapp/src/jira_issue_metadata.jsx # webapp/src/plugin.jsx # webapp/src/reducers/index.js # webapp/src/selectors/index.js

…tions-on-off # Conflicts: # server/command.go

crspeller

Looking good! Just a few comments.

server/command.go

server/webhook_slack_attachment.go

crspeller · 2019-05-23T19:51:27Z

server/webhook.go

+	// This bothers me, to do this for every webhook event...
+	ji, err := p.LoadCurrentJIRAInstance()
+	if err != nil {
+		// It won't break anything if we can't find the Jira Instance here -- we just can't notify anyone.


Probably returning an error here is the right thing to do. Not finding the jira instance should be an edge case.

I think that if finding the jiraInstance was crucial to the settings command, or some other action by the user, returning an error would definitely be the right thing to do. But here it's in response to a webhook, and the chance of us not finding a current instance would be pretty low. If we didn't find one, it would mean a webhook was left connected and the instance uninstalled -- do we want a million errors being triggered? Probably an error would have thrown or the process stopped way earlier than here...

And the other reason I'm kind of against throwing an error is that I don't know how to mock p.LoadCurrentJIRAInstance in the tests. :) If we want to throw an error, could you show me how?

Chatted offline. Going to leave as is for now with a separate ticket to investigate improving the testing situation here.

server/webhook.go

…h2 and Added PKCE code for OAuth2 (#953) * Migrate Docs from GitBook (#948) * Migrate Docs from GitBook * Update readme.md * Update readme.md Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> * Update readme.md Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> * Update readme.md Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> --------- Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> * [MI-3153] Fixed issue: comment webhook notification not working * [MI-3153] Added PKCE along with authorization code for getting the access token * [MI-3153] Review fixes * [MI-3159] Review fixes of comments given by Kshitij * [MI-3159] Review fixes given by Ayush * [MI-3159] Removed the code which was forcing the template window to close after user is connected * [MI-3153] Review fixes * [MI-3159] Review fixes * [MI-3159] Review fix * [MI-3282] Added logic to expand issue and issue with DM notification for comment webhook notification (#58) * [MI-3282] Added logic to expand issue in comment webhook notification * [MI-3282] Fix lint errors * [MI-3315] Fixed issue: comment DM notification not working due to invalid API call * [MI-3282] Review fixes * [MI-3282] Review fix * [MI-3331] Review fixes on PR #953 (Comment notification issue) (#60) * [MI-3331] Review fixes on PR #953 (Comment notification issue) * [MI-3331] Added separate condition to check if the instance is a cloud instance or not * [MI-3335] Review fixes on Jira PR #953(Comment notification issue) (#62) 1. Added conditional for separation out the logic for jira cloud and server --------- Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com>

* Initial commit to support OAuth2 authentication * Implemented modal to configure OAuth2 credentials * Made the field names consistent * Removed install_cloud_oauth.md template * Added telemetry tracking for oauth2 setup flow * Fixed typo * Implemented few review comments * Reverted webapp changes * Reverted package-lock.json file * Fixed depreciation * Using refresh token to get another access token * Implemented review comments & QA findings * Implemented few additional review comments * fixed enterprise check while updating instances * Renamed function name for easy understanding * Fixed failing instances test * fix merge issue * fix merge issue 2 * check for cloud oauth instance for comment webhooks * Fixed issue: Comment notification not working after implementing OAuth2 and Added PKCE code for OAuth2 (#953) * Migrate Docs from GitBook (#948) * Migrate Docs from GitBook * Update readme.md * Update readme.md Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> * Update readme.md Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> * Update readme.md Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> --------- Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> * [MI-3153] Fixed issue: comment webhook notification not working * [MI-3153] Added PKCE along with authorization code for getting the access token * [MI-3153] Review fixes * [MI-3159] Review fixes of comments given by Kshitij * [MI-3159] Review fixes given by Ayush * [MI-3159] Removed the code which was forcing the template window to close after user is connected * [MI-3153] Review fixes * [MI-3159] Review fixes * [MI-3159] Review fix * [MI-3282] Added logic to expand issue and issue with DM notification for comment webhook notification (#58) * [MI-3282] Added logic to expand issue in comment webhook notification * [MI-3282] Fix lint errors * [MI-3315] Fixed issue: comment DM notification not working due to invalid API call * [MI-3282] Review fixes * [MI-3282] Review fix * [MI-3331] Review fixes on PR #953 (Comment notification issue) (#60) * [MI-3331] Review fixes on PR #953 (Comment notification issue) * [MI-3331] Added separate condition to check if the instance is a cloud instance or not * [MI-3335] Review fixes on Jira PR #953(Comment notification issue) (#62) 1. Added conditional for separation out the logic for jira cloud and server --------- Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> --------- Co-authored-by: Rohitesh Gupta <1429138+srkgupta@users.noreply.github.com> Co-authored-by: Mattermost Build <build@mattermost.com> Co-authored-by: Raghav Aggarwal <raghav.aggarwal@brightscout.com> Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com>

levb and others added 30 commits April 25, 2019 13:03

Update README.md

a48270e

Revert accidental commit

918faf6

Update README.md (mattermost#38)

d6d1389

* Update README.md * Update README.md * Update README.md

Update README.md

0c381e4

quick fix of url

7b82ff1

correct the menu item used in Jira

2e85229

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

f737bf5

* Proposed text updates for /jira slash commands Feedback welcome * Address review feedback on command.go

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

7991579

* 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 (mattermost#46)

03acbae

* Adding /jira transition * Moving transitionJiraIssue to a better home.

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

33b0a3a

* Refactor of create-issue modal. * Names feeback.

Fixed /jira subcommand parsing consistency (mattermost#45)

96bee47

Fixed creating server/dist/templates/templates on a 2nd make (matterm…

8ba8802

…ost#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 (mattermost#52)

43eb30e

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

6612955

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

815ad08

* UI fixes for template and menu item * Updating flexparent * Updating templates

Update Jira application link for Cloud (mattermost#48)

897cc00

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

10af812

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

Update README.md

b17831b

Update README.md

4d8f5a7

Add 'subject to change' for timeline in README

f6dbb7f

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

b8fee88

* notifications working, added issue_created

e92470e

wip

924779f

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

92b691d

* notification setting persisted, command line updated

70cfc22

cpoile requested review from levb and crspeller May 17, 2019 19:50

cpoile added the 2: Dev Review Requires review by a core committer label May 17, 2019

crspeller and others added 6 commits May 17, 2019 17:28

Fix govet

acd6315

Adding new CI

3502b9f

Updating Makefile and moving to go modules

05b9957

Temporarily use jira2 as CI branch.

6e202d9

fixing tests

a7e8054

Merge branch 'jira2' into MM-15099-notifications-on-off

c6c16d5

# Conflicts: # server/command.go

levb reviewed May 20, 2019

View reviewed changes

server/atlassian_connect.go Outdated Show resolved Hide resolved

server/command.go Outdated Show resolved Hide resolved

server/issue.go Outdated Show resolved Hide resolved

PR comments

d7c87c3

cpoile requested a review from levb May 21, 2019 14:04

levb approved these changes May 21, 2019

View reviewed changes

jasonblais added this to the v2.1 milestone May 21, 2019

crspeller changed the base branch from jira2 to master May 22, 2019 17:42

cpoile added 4 commits May 23, 2019 13:12

fixes needed after The Great Merge

77a60df

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

35b6550

Merge remote-tracking branch 'upstream/master' into MM-15099-notifica…

b176375

…tions-on-off # Conflicts: # server/command.go

crspeller requested changes May 23, 2019

View reviewed changes

cpoile requested a review from crspeller May 23, 2019 21:19

PR comments

85a4ca2

crspeller approved these changes May 24, 2019

View reviewed changes

crspeller merged commit 1b8644d into mattermost:master May 24, 2019

cpoile deleted the MM-15099-notifications-on-off branch May 24, 2019 15:37

cpoile added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels May 24, 2019

jasonblais mentioned this pull request May 27, 2019

Add Support Additional Events #6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM 15099] direct message notifications on off #62

[MM 15099] direct message notifications on off #62

cpoile commented May 17, 2019 •

edited

Loading

levb left a comment

crspeller left a comment

crspeller May 23, 2019

cpoile May 23, 2019

crspeller May 24, 2019

[MM 15099] direct message notifications on off #62

[MM 15099] direct message notifications on off #62

Conversation

cpoile commented May 17, 2019 • edited Loading

Summary

Todo

Needs advice:

Links

levb left a comment

Choose a reason for hiding this comment

crspeller left a comment

Choose a reason for hiding this comment

crspeller May 23, 2019

Choose a reason for hiding this comment

cpoile May 23, 2019

Choose a reason for hiding this comment

crspeller May 24, 2019

Choose a reason for hiding this comment

cpoile commented May 17, 2019 •

edited

Loading