MM-15439 Restores 1.0 compatibility for webhook filtering #72

levb · 2019-05-23T17:34:28Z

By default, only the events supported by 1.0 are shown (eventIssueCreated, eventIssueReopened, eventIssueResolved, eventUnresolvedIssueDeleted)
Extra parameters can be added to the webhook URL to post more event types to Mattermost, see https://github.com/mattermost/mattermost-plugin-jira/pull/72/files#diff-afb678e7f5aecce88e775e4aa8c70174R56

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

…into lev-MM-15439-webhooks

jasonblais · 2019-05-23T17:49:35Z

By default, only the events supported by 1.0 are shown (eventIssueCreated, eventIssueReopened, eventIssueResolved, eventUnresolvedIssueDeleted)

Does this mean the following events are supported, as listed in ticket

Issue created
Issue updated - issue assignee updated; issue transitioned (e.g. Reopened, In Progress, Resolved, Submitted)
Issue deleted
Comment created
Comment updated
Comment deleted

levb · 2019-05-23T17:45:20Z

server/webhook.go

 		parsed.style = mdRootStyle
 		headline = fmt.Sprintf("created %v", issue)
 		parsed.details = parsed.mdIssueCreatedDetails()
 		parsed.text = parsed.mdIssueDescription()
 	case "jira:issue_deleted":
+		parsed.events = parsed.events | eventIssueDeleted


See the 1.1.1 implementation

mattermost-plugin-jira/server/webhook.go

Lines 65 to 75 in d885e8a

case "jira:issue_updated":

isResolutionChange := false

for _, change := range w.ChangeLog.Items {

if change.Field == "resolution" {

isResolutionChange = (change.FromString == "") != (change.ToString == "")

break

}

}

if !isResolutionChange {

return nil, nil

}

levb · 2019-05-23T17:49:15Z

server/webhook.go

+	eventMax = iota
+)
+
+const maskLegacy = eventIssueCreated |


see the 1.1.1 implementation

mattermost-plugin-jira/server/webhook.go

Lines 63 to 81 in d885e8a

switch w.WebhookEvent {

case "jira:issue_created":

case "jira:issue_updated":

isResolutionChange := false

for _, change := range w.ChangeLog.Items {

if change.Field == "resolution" {

isResolutionChange = (change.FromString == "") != (change.ToString == "")

break

}

}

if !isResolutionChange {

return nil, nil

}

case "jira:issue_deleted":

if w.Issue.Fields.Resolution != nil {

return nil, nil

}

default:

return nil, nil

and

mattermost-plugin-jira/server/webhook.go

Lines 146 to 153 in d885e8a

if change.Field == "resolution" {

if change.ToString == "" && change.FromString != "" {

verb = "reopened"

} else if change.ToString != "" && change.FromString == "" {

verb = "resolved"

}

break

}

server/webhook.go

server/utils.go

server/webhook.go

jasonblais

From reading the diff and our conversation, changes look correct. Will rely on QA to do final verification post-merge

levb · 2019-05-24T14:21:17Z

@crspeller @cpoile this is ready re-review

crspeller · 2019-05-24T14:54:44Z

server/utils.go

 }

-func getUserURL(issue *JIRAWebhookIssue, user *jira.User) string {
+func getUserURL(user *jira.User) string {
+	// TODO is this right?


TODO? What's this about?

This function is not used in 2.0, and will likely be deprecated with @cpoile's implementation of /jira settings notify.

I can cut a separate PR and simply remove all "notify" functionality from v2.0 since it's not used. This function will disappear then.

crspeller · 2019-05-24T15:03:54Z

server/webhook.go

@@ -192,43 +228,52 @@ func parse(in io.Reader, linkf func(w *JIRAWebhook) string) (*parsedJIRAWebhook,
 	issue := parsed.mdIssueType() + " " + linkf(parsed.JIRAWebhook)
 	switch parsed.WebhookEvent {
 	case "jira:issue_created":
+		parsed.event(eventCreated)


I like the encapsulation, maybe consider completely encapsulating the functionality of the bitset? Or to save time use a library like: https://github.com/willf/bitset

The other thing is there is a mix of concerns in this function. We are creating the bitset but also deriving a headline and other stuff. Maybe split this into multiple functions? Like createEventsBitset and createHeadline, etc.

I'll do a separate PR for 2.1 for this. I am inclined not to bring in an extra dependency for what I consider temporary feature flags. If exposing the bit-logic is not sufficiently intention-revealing, I'd rather just use a go map, it's not a material performance difference in the context.

cpoile

Nothing really to add, looks good to me.

jasonblais · 2019-05-24T16:15:36Z

@levb Please create tickets for any follow-up PRs for 2.0/2.1 if applicable, e.g. #72 (comment) and #72 (comment)

* MM-15439 Restores 1.0 compatibility for webhook filtering (#72) * wip * MM-15439: Restored webhook compatibility with 1.0 * Fixed the tests * Fixed eventIssueDeleted->eventUresolvedIssueDeleted * Added support for eventIssueAssigned to URL params * PR feedback * PR feedback: adjusted default event mask * PR feedback from #72, minor * Style: minor simplification, maskAll * Refactored for testability - Refactored webhook code, added tests - Added separate interfaces (`UserStore`, `InstanceStore`, etc.) to KV methods - Added store (kv) interface pointers to `type Plugin` - Added POC mocks for some test stores (as needed) * Added .code-workspace to .gitignore * PR feedback: KV->Store, misc * PR feedback: removed inner function * PR feedback: clarify recursive URL-unescaping

* 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Lev Brouk <[email protected]> Co-authored-by: Michael Kochell <[email protected]>

* [MI-3483] Added issue status field in subscription modal (#71) * [MI-3483] Added the status field in subscription modal * [MI-3483] Code refactoring * [MI-3483] Fix testcases * [MI-3483] Added error handling and code refactoring * [MI-3483] Review fixes * [MI-3483] Review fixes * [MI-3483] Review fixes * [MI-3597] Moved the issue status field into filters section of modal (#72) * [MI-3597] Moved the issue status field into filters section of modal * [MI-3597] FIxed a minor issue * [MI-3597] Review fix * [MI-3628] Updated the code to return the status details in the meta data API (#74) * [MI-3628] Updated the code to return the status details in the meta data API instead of making a separate API call. * [MI-3628] Review fixes: Small code optimization * [MI-3690] Review fixes - Added testcases for status field. - Fixed issue: create meta info API not working for Jira server instance * [MM-4] Review fixes: Fixed some variable names * [MM-4] Updated the test data files and test cases * [MM-71] Fix issue: status filter not working with issue created event

Lev Brouk added 4 commits May 15, 2019 13:40

wip

c9a42bb

MM-15439: Restored webhook compatibility with 1.0

854f470

Merge branch 'jira2' of github.com:mattermost/mattermost-plugin-jira …

1fa87e7

…into lev-MM-15439-webhooks

Fixed the tests

d9e0eac

levb added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer labels May 23, 2019

levb requested review from cpoile, crspeller and jasonblais May 23, 2019 17:34

Fixed eventIssueDeleted->eventUresolvedIssueDeleted

787e76b

levb commented May 23, 2019

View reviewed changes

Added support for eventIssueAssigned to URL params

7f7b730

jasonblais reviewed May 23, 2019

View reviewed changes

server/webhook.go Show resolved Hide resolved

crspeller requested changes May 23, 2019

View reviewed changes

server/utils.go Outdated Show resolved Hide resolved

server/webhook.go Show resolved Hide resolved

server/webhook.go Outdated Show resolved Hide resolved

server/webhook.go Show resolved Hide resolved

crspeller added this to the v2.0 milestone May 23, 2019

PR feedback

0f5621d

levb requested review from crspeller and jasonblais May 23, 2019 20:39

PR feedback: adjusted default event mask

c3975c9

jasonblais approved these changes May 24, 2019

View reviewed changes

jasonblais removed the 1: PM Review Requires review by a product manager label May 24, 2019

crspeller requested changes May 24, 2019

View reviewed changes

levb requested a review from crspeller May 24, 2019 15:51

crspeller approved these changes May 24, 2019

View reviewed changes

cpoile approved these changes May 24, 2019

View reviewed changes

cpoile merged commit ee38bcb into mattermost:jira2 May 24, 2019

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

levb mentioned this pull request May 27, 2019

MM-15439 refactor and merge webhooks #72 from jira2 #82

Merged

levb deleted the lev-MM-15439-webhooks branch May 27, 2019 22:52

levb added a commit that referenced this pull request Aug 10, 2020

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

71d648c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-15439 Restores 1.0 compatibility for webhook filtering #72

MM-15439 Restores 1.0 compatibility for webhook filtering #72

levb commented May 23, 2019 •

edited

Loading

jasonblais commented May 23, 2019

levb May 23, 2019 •

edited

Loading

levb May 23, 2019 •

edited

Loading

jasonblais left a comment

levb commented May 24, 2019

crspeller May 24, 2019

levb May 24, 2019

crspeller May 24, 2019

levb May 24, 2019

crspeller May 24, 2019

cpoile left a comment

jasonblais commented May 24, 2019

	case "jira:issue_updated":
	isResolutionChange := false
	for _, change := range w.ChangeLog.Items {
	if change.Field == "resolution" {
	isResolutionChange = (change.FromString == "") != (change.ToString == "")
	break
	}
	}
	if !isResolutionChange {
	return nil, nil
	}

	switch w.WebhookEvent {
	case "jira:issue_created":
	case "jira:issue_updated":
	isResolutionChange := false
	for _, change := range w.ChangeLog.Items {
	if change.Field == "resolution" {
	isResolutionChange = (change.FromString == "") != (change.ToString == "")
	break
	}
	}
	if !isResolutionChange {
	return nil, nil
	}
	case "jira:issue_deleted":
	if w.Issue.Fields.Resolution != nil {
	return nil, nil
	}
	default:
	return nil, nil

	if change.Field == "resolution" {
	if change.ToString == "" && change.FromString != "" {
	verb = "reopened"
	} else if change.ToString != "" && change.FromString == "" {
	verb = "resolved"
	}
	break
	}

MM-15439 Restores 1.0 compatibility for webhook filtering #72

MM-15439 Restores 1.0 compatibility for webhook filtering #72

Conversation

levb commented May 23, 2019 • edited Loading

jasonblais commented May 23, 2019

levb May 23, 2019 • edited Loading

Choose a reason for hiding this comment

levb May 23, 2019 • edited Loading

Choose a reason for hiding this comment

jasonblais left a comment

Choose a reason for hiding this comment

levb commented May 24, 2019

crspeller May 24, 2019

Choose a reason for hiding this comment

levb May 24, 2019

Choose a reason for hiding this comment

crspeller May 24, 2019

Choose a reason for hiding this comment

levb May 24, 2019

Choose a reason for hiding this comment

crspeller May 24, 2019

Choose a reason for hiding this comment

cpoile left a comment

Choose a reason for hiding this comment

jasonblais commented May 24, 2019

levb commented May 23, 2019 •

edited

Loading

levb May 23, 2019 •

edited

Loading

levb May 23, 2019 •

edited

Loading