From af819fa02070e2691be47d0a7d7377c4333d01c2 Mon Sep 17 00:00:00 2001 From: Nityanand Rai Date: Thu, 1 Sep 2022 12:47:21 +0530 Subject: [PATCH 01/17] [MI-2088]:Add Support for DM subscriptions for watchers and reporters --- go.mod | 2 +- go.sum | 21 ++-- server/client.go | 34 ++++++- server/client_cloud.go | 50 +--------- server/client_server.go | 15 --- server/command.go | 36 ++++++- server/command_test.go | 79 +++++++++++---- server/http.go | 4 - server/issue.go | 207 ++++++++++++++++++++++++++++++++++++--- server/kv_mock_test.go | 8 +- server/plugin.go | 5 +- server/settings.go | 54 ++++++++-- server/subscribe_test.go | 1 - server/user.go | 31 +++++- server/user_cloud.go | 7 +- server/user_server.go | 10 +- server/user_test.go | 22 ++++- server/webhook.go | 41 ++++++-- server/webhook_parser.go | 25 ++--- server/webhook_worker.go | 10 +- 20 files changed, 493 insertions(+), 169 deletions(-) diff --git a/go.mod b/go.mod index d66e09214..7a606d509 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/mattermost/mattermost-plugin-jira go 1.13 require ( - github.com/andygrunwald/go-jira v1.10.0 + github.com/andygrunwald/go-jira v1.15.1 github.com/dghubble/oauth1 v0.5.0 github.com/dgrijalva/jwt-go v3.2.0+incompatible github.com/gorilla/mux v1.8.0 diff --git a/go.sum b/go.sum index 64fa7ea74..335fd263f 100644 --- a/go.sum +++ b/go.sum @@ -168,8 +168,8 @@ github.com/andybalholm/cascadia v1.0.0/go.mod h1:GsXiBklL0woXo1j/WYWtSYYC4ouU9Pq github.com/andybalholm/cascadia v1.1.0/go.mod h1:GsXiBklL0woXo1j/WYWtSYYC4ouU9PqHO0sqidkEA4Y= github.com/andybalholm/cascadia v1.2.0/go.mod h1:YCyR8vOZT9aZ1CHEd8ap0gMVm2aFgxBp0T0eFw1RUQY= github.com/andybalholm/cascadia v1.3.1/go.mod h1:R4bJ1UQfqADjvDa4P6HZHLh/3OxWWEqc0Sk8XGwHqvA= -github.com/andygrunwald/go-jira v1.10.0 h1:+HPPK7++6/hW8ygtr2Yc0wd+Qu139NrWiTD/r1cYxO0= -github.com/andygrunwald/go-jira v1.10.0/go.mod h1:KEsrADP1cEXRxVWTaDtpLyyZN1LM9p6Jn8W5+sDzxhc= +github.com/andygrunwald/go-jira v1.15.1 h1:6J9aYKb9sW8bxv3pBLYBrs0wdsFrmGI5IeTgWSKWKc8= +github.com/andygrunwald/go-jira v1.15.1/go.mod h1:GIYN1sHOIsENWUZ7B4pDeT/nxEtrZpE8l0987O67ZR8= github.com/anmitsu/go-shlex v0.0.0-20161002113705-648efa622239/go.mod h1:2FmKhYUyUczH0OGQWaF5ceTx0UBShxjsH6f8oGKYe2c= github.com/antihax/optional v1.0.0/go.mod h1:uupD/76wgC+ih3iEmQUL+0Ugr19nfwCT1kdvxnR2qWY= github.com/apache/arrow/go/arrow v0.0.0-20200601151325-b2287a20f230/go.mod h1:QNYViu/X0HXDHw7m3KXzWSVXIbfUvJqBFe6Gj8/pYA0= @@ -524,7 +524,6 @@ github.com/fatih/color v1.12.0/go.mod h1:ELkj/draVOlAH/xkhN6mQ50Qd0MPOk5AAr3maGE github.com/fatih/color v1.13.0 h1:8LOYc1KYPPmyKMuN8QV2DNRWNbLo6LZ0iLs8+mlH53w= github.com/fatih/color v1.13.0/go.mod h1:kLAiJbzzSOZDVNGyDpeOxJ47H46qBXwg5ILebYFFOfk= github.com/fatih/set v0.2.1/go.mod h1:+RKtMCH+favT2+3YecHGxcc0b4KyVWA1QWWJUs4E0CI= -github.com/fatih/structs v1.0.0/go.mod h1:9NiDSp5zOcgEDl+j00MP/WkGVPOlPRLejGD8Ga6PJ7M= github.com/fatih/structs v1.1.0 h1:Q7juDM0QtcnhCpeyLGQKyg4TOIghuNXrkL32pHAUMxo= github.com/fatih/structs v1.1.0/go.mod h1:9NiDSp5zOcgEDl+j00MP/WkGVPOlPRLejGD8Ga6PJ7M= github.com/fcjr/aia-transport-go v1.2.2/go.mod h1:onSqSq3tGkM14WusDx7q9FTheS9R1KBtD+QBWI6zG/w= @@ -653,7 +652,10 @@ github.com/gogo/protobuf v1.2.2-0.20190723190241-65acae22fc9d/go.mod h1:SlYgWuQ5 github.com/gogo/protobuf v1.3.0/go.mod h1:SlYgWuQ5SjCEi6WLHjHCa1yvBfUnHcTbrrZtXPKa29o= github.com/gogo/protobuf v1.3.1/go.mod h1:SlYgWuQ5SjCEi6WLHjHCa1yvBfUnHcTbrrZtXPKa29o= github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q= +github.com/golang-jwt/jwt v3.2.2+incompatible h1:IfV12K8xAKAnZqdXVzCZ+TOjboZ2keLg81eXfW3O+oY= github.com/golang-jwt/jwt v3.2.2+incompatible/go.mod h1:8pz2t5EyA70fFQQSrl6XZXzqecmYZeUEB8OUGHkxJ+I= +github.com/golang-jwt/jwt/v4 v4.3.0 h1:kHL1vqdqWNfATmA0FNMdmZNMyZI1U6O31X4rlIPoBog= +github.com/golang-jwt/jwt/v4 v4.3.0/go.mod h1:/xlHOz8bRuivTWchD4jCa+NbatV+wEUSzwAxVc6locg= github.com/golang-migrate/migrate/v4 v4.14.1/go.mod h1:l7Ks0Au6fYHuUIxUhQ0rcVX1uLlJg54C/VvW7tvxSz0= github.com/golang-migrate/migrate/v4 v4.15.1/go.mod h1:/CrBenUbcDqsW29jGTR/XFqCfVi/Y6mHXlooCcSOJMQ= github.com/golang-sql/civil v0.0.0-20190719163853-cb61b32ac6fe/go.mod h1:8vg3r2VgvsThLBIFL93Qb5yWzgyZWhEmBwUJWevAkK0= @@ -719,13 +721,14 @@ github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.3/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.4/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= -github.com/google/go-cmp v0.5.6 h1:BKbKCqvP6I+rmFHt06ZmyQtvB8xAkWdhFyr0ZUNZcxQ= github.com/google/go-cmp v0.5.6/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= +github.com/google/go-cmp v0.5.7 h1:81/ik6ipDQS2aGcBfIN5dHDB36BwrStyeAQquSYCV4o= +github.com/google/go-cmp v0.5.7/go.mod h1:n+brtR0CgQNWTVd5ZUFpTBC8YFBDLK/h/bpaJ8/DtOE= github.com/google/go-github v17.0.0+incompatible/go.mod h1:zLgOLi98H3fifZn+44m+umXrS52loVEgC2AApnigrVQ= github.com/google/go-github/v35 v35.2.0/go.mod h1:s0515YVTI+IMrDoy9Y4pHt9ShGpzHvHO8rZ7L7acgvs= -github.com/google/go-querystring v0.0.0-20170111101155-53e6ce116135/go.mod h1:odCYkC5MyYFN7vkCjXpyrEuKhc/BUO6wN/zVPAxq5ck= -github.com/google/go-querystring v1.0.0 h1:Xkwi/a1rcvNg1PPYe5vI8GbeBY/jrVuDX5ASuANWTrk= github.com/google/go-querystring v1.0.0/go.mod h1:odCYkC5MyYFN7vkCjXpyrEuKhc/BUO6wN/zVPAxq5ck= +github.com/google/go-querystring v1.1.0 h1:AnCroh3fv4ZBgVIf1Iwtovgjaw/GiKJo8M8yD/fhyJ8= +github.com/google/go-querystring v1.1.0/go.mod h1:Kcdr2DB4koayq7X8pmAG4sNG59So17icRSOU623lUBU= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/gofuzz v1.1.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/martian v2.1.0+incompatible/go.mod h1:9I4somxYTbIHy5NJKHRl3wXiIaQGbYVAs8BPL6v8lEs= @@ -1569,8 +1572,8 @@ github.com/tinylib/msgp v1.1.6 h1:i+SbKraHhnrf9M5MYmvQhFnbLhAXSDWF8WWsuyRdocw= github.com/tinylib/msgp v1.1.6/go.mod h1:75BAfg2hauQhs3qedfdDZmWAPcFMAvJE5b9rGOMufyw= github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U= github.com/tmc/grpc-websocket-proxy v0.0.0-20190109142713-0ad062ec5ee5/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U= -github.com/trivago/tgo v1.0.1 h1:bxatjJIXNIpV18bucU4Uk/LaoxvxuOlp/oowRHyncLQ= -github.com/trivago/tgo v1.0.1/go.mod h1:w4dpD+3tzNIIiIfkWWa85w5/B77tlvdZckQ+6PkFnhc= +github.com/trivago/tgo v1.0.7 h1:uaWH/XIy9aWYWpjm2CU3RpcqZXmX2ysQ9/Go+d9gyrM= +github.com/trivago/tgo v1.0.7/go.mod h1:w4dpD+3tzNIIiIfkWWa85w5/B77tlvdZckQ+6PkFnhc= github.com/ttacon/chalk v0.0.0-20160626202418-22c06c80ed31/go.mod h1:onvgF043R+lC5RZ8IT9rBXDaEDnpnw/Cl+HFiw+v/7Q= github.com/tv42/httpunix v0.0.0-20150427012821-b75d8614f926/go.mod h1:9ESjWnEqriFuLhtthL60Sar/7RFoluCcXsuvEwTV5KM= github.com/tylerb/graceful v1.2.15/go.mod h1:LPYTbOYmUTdabwRt0TGhLllQ0MUNbs0Y5q1WXJOI9II= @@ -1721,7 +1724,6 @@ golang.org/x/crypto v0.0.0-20190313024323-a1f597ede03a/go.mod h1:djNgcEr1/C05ACk golang.org/x/crypto v0.0.0-20190325154230-a5d413f7728c/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20190411191339-88737f569e3a/go.mod h1:WFFai1msRO1wXaEeE5yQxYXgSfI8pQAWXbQop6sCtWE= golang.org/x/crypto v0.0.0-20190422162423-af44ce270edf/go.mod h1:WFFai1msRO1wXaEeE5yQxYXgSfI8pQAWXbQop6sCtWE= -golang.org/x/crypto v0.0.0-20190426145343-a29dc8fdc734/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20190506204251-e1dfcc566284/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20190510104115-cbcb75029529/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20190605123033-f99c8df09eb5/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= @@ -2057,6 +2059,7 @@ golang.org/x/sys v0.0.0-20220207234003-57398862261d h1:Bm7BNOQt2Qv7ZqysjeLjgCBan golang.org/x/sys v0.0.0-20220207234003-57398862261d/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/term v0.0.0-20201117132131-f5c789dd3221/go.mod h1:Nr5EML6q2oocZ2LXRh80K7BxOlk5/8JxuGnuhpl+muw= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= +golang.org/x/term v0.0.0-20210220032956-6a3ed077a48d/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= diff --git a/server/client.go b/server/client.go index 7efe93bdf..5b99869a9 100644 --- a/server/client.go +++ b/server/client.go @@ -12,7 +12,6 @@ import ( "mime/multipart" "net/http" "net/url" - "path" "regexp" "strconv" "strings" @@ -55,7 +54,7 @@ type UserService interface { // ProjectService is the interface for project-related APIs. type ProjectService interface { GetProject(key string) (*jira.Project, error) - ListProjects(query string, limit int) (jira.ProjectList, error) + GetAllProjectKeys() ([]string, error) } // SearchService is the interface for search-related APIs. @@ -78,6 +77,7 @@ type IssueService interface { GetTransitions(issueKey string) ([]jira.Transition, error) UpdateAssignee(issueKey string, user *jira.User) error UpdateComment(issueKey string, comment *jira.Comment) (*jira.Comment, error) + GetWatchers(instanceID, issueKey string, connection *Connection) (*jira.Watches, error) } // JiraClient is the common implementation of most Jira APIs, except those that are @@ -168,6 +168,20 @@ func (client JiraClient) RESTPostAttachment(issueID string, data []byte, name st return attachments[0], nil } +func (client JiraClient) GetAllProjectKeys() ([]string, error) { + projectlist, resp, err := client.Jira.Project.GetList() + if err != nil { + return nil, userFriendlyJiraError(resp, err) + } + + keys := make([]string, 0, len(*projectlist)) + for _, project := range *projectlist { + keys = append(keys, project.Key) + } + + return keys, nil +} + // GetProject returns a Project by key. func (client JiraClient) GetProject(key string) (*jira.Project, error) { project, resp, err := client.Jira.Project.Get(key) @@ -186,6 +200,20 @@ func (client JiraClient) GetIssue(key string, options *jira.GetQueryOptions) (*j return issue, nil } +// GetWatchers returns an array of Jira users watching for a given issue. +func (client JiraClient) GetWatchers(instanceID, issueKey string, connection *Connection) (*jira.Watches, error) { + var watchers jira.Watches + params := map[string]string{ + "accountId": connection.AccountID, + } + + err := client.RESTGet(instanceID+"/rest/api/2/issue/"+issueKey+"/watchers", params, &watchers) + if err != nil { + return nil, err + } + return &watchers, nil +} + // GetTransitions returns transitions for an issue with issueKey. func (client JiraClient) GetTransitions(issueKey string) ([]jira.Transition, error) { transitions, resp, err := client.Jira.Issue.GetTransitions(issueKey) @@ -392,7 +420,7 @@ func endpointURL(endpoint string) (string, error) { } if parsedURL.Scheme == "" { // relative path - endpoint = path.Join("/rest/api", endpoint) + endpoint = fmt.Sprintf("/rest/api/%s", endpoint) } return endpoint, nil } diff --git a/server/client_cloud.go b/server/client_cloud.go index 44a3cea3f..1e8b011df 100644 --- a/server/client_cloud.go +++ b/server/client_cloud.go @@ -4,8 +4,7 @@ package main import ( - "encoding/json" - "strconv" + "encoding/json" jira "github.com/andygrunwald/go-jira" "github.com/pkg/errors" @@ -67,50 +66,3 @@ func (client jiraCloudClient) GetUserGroups(connection *Connection) ([]*jira.Use } return groups, nil } - -func (client jiraCloudClient) ListProjects(query string, limit int) (jira.ProjectList, error) { - type searchResult struct { - Values jira.ProjectList `json:"values"` - StartAt int `json:"startAt"` - MaxResults int `json:"maxResults"` - Total int `json:"total"` - IsLast bool `json:"isLast"` - } - - remaining := 50 - fetchAll := false - if limit > 0 { - remaining = limit - } - if limit < 0 { - fetchAll = true - } - - var out jira.ProjectList - for { - opts := map[string]string{ - "startAt": strconv.Itoa(len(out)), - "maxResults": strconv.Itoa(remaining), - "expand": "issueTypes", - } - var result searchResult - err := client.RESTGet("/3/project/search", opts, &result) - if err != nil { - return nil, err - } - if len(result.Values) > remaining { - result.Values = result.Values[:remaining] - } - out = append(out, result.Values...) - remaining -= len(result.Values) - - if !fetchAll && remaining == 0 { - // Got enough. - return out, nil - } - if len(result.Values) == 0 || result.IsLast { - // Ran out of results. - return out, nil - } - } -} diff --git a/server/client_server.go b/server/client_server.go index 8b19bf3f4..5350c8126 100644 --- a/server/client_server.go +++ b/server/client_server.go @@ -62,18 +62,3 @@ func (client jiraServerClient) GetUserGroups(connection *Connection) ([]*jira.Us } return result.Groups.Items, nil } - -func (client jiraServerClient) ListProjects(query string, limit int) (jira.ProjectList, error) { - plist, resp, err := client.Jira.Project.GetList() - if err != nil { - return nil, userFriendlyJiraError(resp, err) - } - if plist == nil { - return jira.ProjectList{}, nil - } - result := *plist - if limit > 0 && len(result) > limit { - result = result[:limit] - } - return result, nil -} diff --git a/server/command.go b/server/command.go index 0b5d8f94c..d79fef897 100644 --- a/server/command.go +++ b/server/command.go @@ -251,12 +251,33 @@ func createSettingsCommand(optInstance bool) *model.AutocompleteData { "list", "", "View your current settings") settings.AddCommand(list) - notifications := model.NewAutocompleteData( - "notifications", "[on|off]", "Update your user notifications settings") - notifications.AddStaticListArgument("value", true, []model.AutocompleteListItem{ + setting := []model.AutocompleteListItem{ {HelpText: "Turn notifications on", Item: "on"}, {HelpText: "Turn notifications off", Item: "off"}, - }) + } + + notifications := model.NewAutocompleteData( + "notifications", "[assinee|mention|reporter]", "manage notifications") + + assigneeNotifications := model.NewAutocompleteData( + subCommandAssignee, "", "manage assignee notifications") + assigneeNotifications.AddStaticListArgument("value", true, setting) + + mentionNotifications := model.NewAutocompleteData( + subCommandMention, "", "manage mention notifications") + mentionNotifications.AddStaticListArgument("value", true, setting) + + reporterNotifications := model.NewAutocompleteData( + subCommandReporter, "", "manage reporter notifications") + reporterNotifications.AddStaticListArgument("value", true, setting) + + watchingNotifications := model.NewAutocompleteData( + subCommandWatching, "", "manage watching notifications") + reporterNotifications.AddStaticListArgument("value", true, setting) + notifications.AddCommand(assigneeNotifications) + notifications.AddCommand(mentionNotifications) + notifications.AddCommand(reporterNotifications) + notifications.AddCommand(watchingNotifications) withFlagInstance(notifications, optInstance, routeAutocompleteInstalledInstanceWithAlias) settings.AddCommand(notifications) @@ -367,10 +388,12 @@ func (p *Plugin) ExecuteCommand(c *plugin.Context, commandArgs *model.CommandArg if err != nil { return p.responsef(commandArgs, err.Error()), nil } + args := strings.Fields(commandArgs.Command) if len(args) == 0 || args[0] != "/jira" { return p.help(commandArgs), nil } + return jiraCommandHandler.Handle(p, c, commandArgs, args[1:]...), nil } @@ -583,7 +606,10 @@ func executeSettings(p *Plugin, c *plugin.Context, header *model.CommandArgs, ar switch args[0] { case "list": - return p.responsef(header, "Current settings:\n%s", conn.Settings.String()) + if conn.Settings != nil { + return p.responsef(header, "Current settings:\n%s", conn.Settings.String()) + } + return p.responsef(header, "Please connect to jira account `/jira connect`") case "notifications": return p.settingsNotifications(header, instance.GetID(), user.MattermostUserID, conn, args) default: diff --git a/server/command_test.go b/server/command_test.go index 01a1c307d..00fd51a13 100644 --- a/server/command_test.go +++ b/server/command_test.go @@ -24,12 +24,13 @@ import ( ) const ( - mockUserIDWithNotifications = "1" - mockUserIDWithoutNotifications = "2" - mockUserIDUnknown = "3" - mockUserIDSysAdmin = "4" - mockUserIDNonSysAdmin = "5" - mattermostSiteURL = "https://somelink.com" + mockUserIDWithNotifications = "1" + mockUserIDWithoutNotifications = "2" + mockUserIDUnknown = "3" + mockUserIDSysAdmin = "4" + mockUserIDNonSysAdmin = "5" + mattermostSiteURL = "https://somelink.com" + mockMattermostIDWithNotifications = "testMattermostUserId012345" ) type mockUserStoreKV struct { @@ -69,8 +70,15 @@ func getMockUserStoreKV() mockUserStoreKV { }, } + trueValue := true withNotifications := connection // copy - withNotifications.Settings = &ConnectionSettings{Notifications: true} + withNotifications.Settings = &ConnectionSettings{ + Notifications: trueValue, + SendNotificationsForMention: &trueValue, + SendNotificationsForAssignee: &trueValue, + SendNotificationsForReporter: &trueValue, + SendNotificationsForWatching: &trueValue, + } return mockUserStoreKV{ users: map[types.ID]*User{ @@ -79,9 +87,10 @@ func getMockUserStoreKV() mockUserStoreKV { mockUserIDWithoutNotifications: newuser(mockUserIDWithoutNotifications), }, connections: map[types.ID]*Connection{ - mockUserIDWithNotifications: &withNotifications, - mockUserIDWithoutNotifications: &connection, - "connected_user": &connection, + mockUserIDWithNotifications: &withNotifications, + mockUserIDWithoutNotifications: &connection, + "connected_user": &connection, + mockMattermostIDWithNotifications: &withNotifications, }, } } @@ -165,12 +174,12 @@ func TestPlugin_ExecuteCommand_Settings(t *testing.T) { "no params, with notifications": { commandArgs: &model.CommandArgs{Command: "/jira settings", UserId: mockUserIDWithNotifications}, numInstances: 1, - expectedMsg: "Current settings:\n\tNotifications: on", + expectedMsg: "Current settings:\n\t- Notifications for assignee: on \n\t- Notifications for mention: on \n\t- Notifications for reporter: on \n\t- Notifications for watching: on", }, "no params, without notifications": { commandArgs: &model.CommandArgs{Command: "/jira settings", UserId: mockUserIDWithoutNotifications}, numInstances: 1, - expectedMsg: "Current settings:\n\tNotifications: off", + expectedMsg: "Current settings:\n\t- Notifications for assignee: off \n\t- Notifications for mention: off \n\t- Notifications for reporter: off \n\t- Notifications for watching: off", }, "unknown setting": { commandArgs: &model.CommandArgs{Command: "/jira settings" + " test", UserId: mockUserIDWithoutNotifications}, @@ -180,22 +189,52 @@ func TestPlugin_ExecuteCommand_Settings(t *testing.T) { "set notifications without value": { commandArgs: &model.CommandArgs{Command: "/jira settings" + " notifications", UserId: mockUserIDWithoutNotifications}, numInstances: 1, - expectedMsg: "`/jira settings notifications [value]`\n* Invalid value. Accepted values are: `on` or `off`.", + expectedMsg: "`/jira settings notifications [assignee|mention|reporter|watching] [value]`\n* Invalid value. Accepted values are: `on` or `off`.", }, "set notification with unknown value": { commandArgs: &model.CommandArgs{Command: "/jira settings notifications test", UserId: mockUserIDWithoutNotifications}, numInstances: 1, - expectedMsg: "`/jira settings notifications [value]`\n* Invalid value. Accepted values are: `on` or `off`.", + expectedMsg: "`/jira settings notifications [assignee|mention|reporter|watching] [value]`\n* Invalid value. Accepted values are: `on` or `off`.", + }, + "enable assignee notifications": { + commandArgs: &model.CommandArgs{Command: "/jira settings notifications assignee on", UserId: mockUserIDWithoutNotifications}, + numInstances: 1, + expectedMsg: "Settings updated.\n\tAssignee notifications on.", + }, + "disable assignee notifications": { + commandArgs: &model.CommandArgs{Command: "/jira settings notifications assignee off", UserId: mockUserIDWithNotifications}, + numInstances: 1, + expectedMsg: "Settings updated.\n\tAssignee notifications off.", + }, + "enable reporter notifications": { + commandArgs: &model.CommandArgs{Command: "/jira settings notifications reporter on", UserId: mockUserIDWithoutNotifications}, + numInstances: 1, + expectedMsg: "Settings updated.\n\tReporter notifications on.", + }, + "disable reporter notifications": { + commandArgs: &model.CommandArgs{Command: "/jira settings notifications reporter off", UserId: mockUserIDWithNotifications}, + numInstances: 1, + expectedMsg: "Settings updated.\n\tReporter notifications off.", + }, + "enable mention notifications": { + commandArgs: &model.CommandArgs{Command: "/jira settings notifications mention on", UserId: mockUserIDWithoutNotifications}, + numInstances: 1, + expectedMsg: "Settings updated.\n\tMention notifications on.", + }, + "disable mention notifications": { + commandArgs: &model.CommandArgs{Command: "/jira settings notifications mention off", UserId: mockUserIDWithNotifications}, + numInstances: 1, + expectedMsg: "Settings updated.\n\tMention notifications off.", }, - "enable notifications": { - commandArgs: &model.CommandArgs{Command: "/jira settings notifications on", UserId: mockUserIDWithoutNotifications}, + "enable watching notifications": { + commandArgs: &model.CommandArgs{Command: "/jira settings notifications watching on", UserId: mockUserIDWithoutNotifications}, numInstances: 1, - expectedMsg: "Settings updated. Notifications on.", + expectedMsg: "Settings updated.\n\tWatching notifications on.", }, - "disable notifications": { - commandArgs: &model.CommandArgs{Command: "/jira settings notifications off", UserId: mockUserIDWithNotifications}, + "disable watching notifications": { + commandArgs: &model.CommandArgs{Command: "/jira settings notifications watching off", UserId: mockUserIDWithNotifications}, numInstances: 1, - expectedMsg: "Settings updated. Notifications off.", + expectedMsg: "Settings updated.\n\tWatching notifications off.", }, } for name, tt := range tests { diff --git a/server/http.go b/server/http.go index a0189ebda..c59ace5d5 100644 --- a/server/http.go +++ b/server/http.go @@ -104,10 +104,6 @@ func (p *Plugin) serveHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Req routeUserStart, routeAPISubscribeWebhook: - if path == routeIncomingWebhook && instanceURL == "" { - break - } - callbackInstanceID, err = p.ResolveWebhookInstanceURL(instanceURL) if err != nil { return respondErr(w, http.StatusInternalServerError, err) diff --git a/server/issue.go b/server/issue.go index 448b1819d..70f0999a0 100644 --- a/server/issue.go +++ b/server/issue.go @@ -502,13 +502,13 @@ func (p *Plugin) httpGetJiraProjectMetadata(w http.ResponseWriter, r *http.Reque instanceID := r.FormValue("instance_id") - plist, connection, err := p.ListJiraProjects(types.ID(instanceID), types.ID(mattermostUserID)) + cimd, connection, err := p.GetJiraProjectMetadata(types.ID(instanceID), types.ID(mattermostUserID)) if err != nil { return respondErr(w, http.StatusInternalServerError, errors.WithMessage(err, "failed to GetProjectMetadata")) } - if len(plist) == 0 { + if len(cimd.Projects) == 0 { _, err = respondJSON(w, map[string]interface{}{ "error": "You do not have permission to create issues in any projects. Please contact your Jira admin.", }) @@ -518,20 +518,21 @@ func (p *Plugin) httpGetJiraProjectMetadata(w http.ResponseWriter, r *http.Reque } } - projects := []utils.ReactSelectOption{} - issues := map[string][]utils.ReactSelectOption{} - for _, prj := range plist { - projects = append(projects, utils.ReactSelectOption{ + type option = utils.ReactSelectOption + projects := make([]option, 0, len(cimd.Projects)) + issues := make(map[string][]option, len(cimd.Projects)) + for _, prj := range cimd.Projects { + projects = append(projects, option{ Value: prj.Key, Label: prj.Name, }) - issueTypes := []utils.ReactSelectOption{} + issueTypes := make([]option, 0, len(prj.IssueTypes)) for _, issue := range prj.IssueTypes { - if issue.Subtask { + if issue.Subtasks { continue } - issueTypes = append(issueTypes, utils.ReactSelectOption{ - Value: issue.ID, + issueTypes = append(issueTypes, option{ + Value: issue.Id, Label: issue.Name, }) } @@ -545,16 +546,16 @@ func (p *Plugin) httpGetJiraProjectMetadata(w http.ResponseWriter, r *http.Reque }) } -func (p *Plugin) ListJiraProjects(instanceID, mattermostUserID types.ID) (jira.ProjectList, *Connection, error) { +func (p *Plugin) GetJiraProjectMetadata(instanceID, mattermostUserID types.ID) (*jira.CreateMetaInfo, *Connection, error) { client, _, connection, err := p.getClient(instanceID, mattermostUserID) if err != nil { return nil, nil, err } - plist, err := client.ListProjects("", -1) + metainfo, err := client.GetCreateMeta(nil) if err != nil { return nil, nil, err } - return plist, connection, nil + return metainfo, connection, nil } var reJiraIssueKey = regexp.MustCompile(`^([[:alnum:]]+)-([[:digit:]]+)$`) @@ -1039,3 +1040,183 @@ func (p *Plugin) getClient(instanceID, mattermostUserID types.ID) (Client, Insta } return client, instance, connection, nil } + +func (p *Plugin) checkIssueWatchers(wh *webhook, instanceID types.ID) { + if !wh.eventTypes.ContainsAny("event_created_comment") { + return + } + + jwhook := wh.JiraWebhook + commentAuthor := mdUser(&jwhook.Comment.UpdateAuthor) + commentMessage := fmt.Sprintf("%s **commented** on %s:\n> %s", commentAuthor, jwhook.mdKeySummaryLink(), jwhook.Comment.Body) + client, connection, err := wh.fetchConnectedUser(p, instanceID) + if err != nil || client == nil { + return + } + + watcherUsers, err := client.GetWatchers(instanceID.String(), wh.Issue.ID, connection) + if err != nil { + p.errorf("error while getting watchers for issue , err : %v", err) + return + } + + for _, watcherUser := range watcherUsers.Watchers { + + whUserNotification := webhookUserNotification{ + jiraUsername: watcherUser.Name, + jiraAccountID: watcherUser.AccountID, + message: commentMessage, + postType: PostTypeComment, + commentSelf: wh.JiraWebhook.Comment.Self, + notificationType: "watching", + } + + wh.notifications = append(wh.notifications, whUserNotification) + } +} + +func (p *Plugin) applyReporterNotification(wh *webhook, instanceID types.ID, reporter *jira.User) { + if !wh.eventTypes.ContainsAny("event_created_comment") { + return + } + + jwhook := wh.JiraWebhook + + if reporter == nil || + (reporter.Name != "" && reporter.Name == jwhook.User.Name) || + (reporter.AccountID != "" && reporter.AccountID == jwhook.Comment.UpdateAuthor.AccountID) { + return + } + + commentAuthor := mdUser(&jwhook.Comment.UpdateAuthor) + + commentMessage := fmt.Sprintf("%s **commented** on %s:\n> %s", commentAuthor, jwhook.mdKeySummaryLink(), jwhook.Comment.Body) + + c, err := p.GetUserSetting(wh, instanceID, reporter.Name, reporter.AccountID) + if err != nil || c.Settings == nil || !c.Settings.ShouldReceiveNotificationsForReporter() { + return + } + + wh.notifications = append(wh.notifications, webhookUserNotification{ + jiraUsername: reporter.Name, + jiraAccountID: reporter.AccountID, + message: commentMessage, + postType: PostTypeComment, + commentSelf: jwhook.Comment.Self, + notificationType: "reporter", + }) +} + +func (p *Plugin) GetUserSetting(wh *webhook, instanceID types.ID, jiraAccountID, jiraUsername string) (*Connection, error) { + var err error + instance, err := p.instanceStore.LoadInstance(instanceID) + if err != nil { + return nil, err + } + var mattermostUserID types.ID + if jiraAccountID != "" { + mattermostUserID, err = p.userStore.LoadMattermostUserID(instance.GetID(), jiraAccountID) + } else { + mattermostUserID, err = p.userStore.LoadMattermostUserID(instance.GetID(), jiraUsername) + } + + if err != nil { + return nil, err + } + + c, err := p.userStore.LoadConnection(instanceID, mattermostUserID) + if err != nil { + return nil, err + } + + return c, nil +} + +func (s *ConnectionSettings) ShouldReceiveNotificationsForAssignee() bool { + if s.SendNotificationsForAssignee != nil { + return *s.SendNotificationsForAssignee + } + + // Check old setting for backwards compatibility + return s.Notifications +} + +func (s *ConnectionSettings) ShouldReceiveNotificationsForReporter() bool { + if s.SendNotificationsForReporter != nil { + return *s.SendNotificationsForReporter + } + + return s.Notifications +} + +func (s *ConnectionSettings) ShouldReceiveNotificationsForMention() bool { + if s.SendNotificationsForMention != nil { + return *s.SendNotificationsForMention + } + + return s.Notifications +} + +func (s *ConnectionSettings) ShouldReceiveNotificationsForWatching() bool { + if s.SendNotificationsForWatching != nil { + return *s.SendNotificationsForWatching + } + + return s.Notifications +} + +func (wh *webhook) fetchConnectedUser(p *Plugin, instanceID types.ID) (Client, *Connection, error) { + var accountInformation []map[string]string + + if wh.Issue.Fields != nil && wh.Issue.Fields.Creator != nil { + accountInformation = append(accountInformation, map[string]string{ + "AccountID": wh.Issue.Fields.Creator.AccountID, + "Name": wh.Issue.Fields.Creator.Name, + }) + } + + if wh.Issue.Fields != nil && wh.Issue.Fields.Assignee != nil { + accountInformation = append(accountInformation, map[string]string{ + "AccountID": wh.Issue.Fields.Assignee.AccountID, + "Name": wh.Issue.Fields.Assignee.Name, + }) + } + + if wh.Issue.Fields != nil && wh.Issue.Fields.Reporter != nil { + accountInformation = append(accountInformation, map[string]string{ + "AccountID": wh.Issue.Fields.Reporter.AccountID, + "Name": wh.Issue.Fields.Reporter.Name, + }) + } + + instance, err := p.instanceStore.LoadInstance(instanceID) + if err != nil { + return nil, nil, err + } + for _, account := range accountInformation { + var mattermostUserID types.ID + if account["AccountID"] != "" { + mattermostUserID, err = p.userStore.LoadMattermostUserID(instance.GetID(), account["AccountID"]) + } else { + mattermostUserID, err = p.userStore.LoadMattermostUserID(instance.GetID(), account["Name"]) + } + + if err != nil { + continue + } + + c, err2 := p.userStore.LoadConnection(instance.GetID(), mattermostUserID) + if err2 != nil { + continue + } + + client, err2 := instance.GetClient(c) + if err2 != nil { + continue + } + + return client, c, nil + } + + return nil, nil, nil +} diff --git a/server/kv_mock_test.go b/server/kv_mock_test.go index 51b142f94..4be78e2df 100644 --- a/server/kv_mock_test.go +++ b/server/kv_mock_test.go @@ -79,10 +79,14 @@ func (store mockUserStore) StoreConnection(types.ID, types.ID, *Connection) erro return nil } func (store mockUserStore) LoadConnection(types.ID, types.ID) (*Connection, error) { - return &Connection{}, nil + return &Connection{ + Settings: &ConnectionSettings{ + Notifications: true, + }, + }, nil } func (store mockUserStore) LoadMattermostUserID(instanceID types.ID, jiraUserName string) (types.ID, error) { - return "testMattermostUserId012345", nil + return mockMattermostIDWithNotifications, nil } func (store mockUserStore) DeleteConnection(instanceID, mattermostUserID types.ID) error { return nil diff --git a/server/plugin.go b/server/plugin.go index 160658bd6..571525556 100644 --- a/server/plugin.go +++ b/server/plugin.go @@ -351,13 +351,12 @@ func (p *Plugin) AddAutolinksForCloudInstance(ci *cloudInstance) error { return fmt.Errorf("unable to get jira client for server: %w", err) } - plist, err := jiraCloudClient{JiraClient{Jira: client}}.ListProjects("", -1) + keys, err := JiraClient{Jira: client}.GetAllProjectKeys() if err != nil { return fmt.Errorf("unable to get project keys: %w", err) } - for _, proj := range plist { - key := proj.Key + for _, key := range keys { err = p.AddAutolinks(key, ci.BaseURL) } if err != nil { diff --git a/server/settings.go b/server/settings.go index a3fa3dfe2..43620fdc1 100644 --- a/server/settings.go +++ b/server/settings.go @@ -1,6 +1,8 @@ package main import ( + "strings" + "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-plugin-jira/server/utils/types" @@ -9,17 +11,24 @@ import ( const ( settingOn = "on" settingOff = "off" + + errStoreNewSettings = "Could not store new settings. Please contact your system administrator. error: %v" + errConnectToJira = "Your username is not connected to Jira. Please type `/jira connect`. %v" + subCommandAssignee = "assignee" + subCommandMention = "mention" + subCommandReporter = "reporter" + subCommandWatching = "watching" ) func (p *Plugin) settingsNotifications(header *model.CommandArgs, instanceID, mattermostUserID types.ID, connection *Connection, args []string) *model.CommandResponse { - const helpText = "`/jira settings notifications [value]`\n* Invalid value. Accepted values are: `on` or `off`." + const helpText = "`/jira settings notifications [assignee|mention|reporter|watching] [value]`\n* Invalid value. Accepted values are: `on` or `off`." - if len(args) != 2 { + if len(args) != 3 { return p.responsef(header, helpText) } var value bool - switch args[1] { + switch args[2] { case settingOn: value = true case settingOff: @@ -31,21 +40,48 @@ func (p *Plugin) settingsNotifications(header *model.CommandArgs, instanceID, ma if connection.Settings == nil { connection.Settings = &ConnectionSettings{} } - connection.Settings.Notifications = value + switch args[1] { + case subCommandAssignee: + connection.Settings.SendNotificationsForAssignee = &value + case subCommandMention: + connection.Settings.SendNotificationsForMention = &value + case subCommandReporter: + connection.Settings.SendNotificationsForReporter = &value + case subCommandWatching: + connection.Settings.SendNotificationsForWatching = &value + default: + return p.responsef(header, helpText) + } + if err := p.userStore.StoreConnection(instanceID, mattermostUserID, connection); err != nil { p.errorf("settingsNotifications, err: %v", err) - p.responsef(header, "Could not store new settings. Please contact your system administrator. error: %v", err) + p.responsef(header, errStoreNewSettings, err) } // send back the actual value updatedConnection, err := p.userStore.LoadConnection(instanceID, mattermostUserID) if err != nil { - return p.responsef(header, "Your username is not connected to Jira. Please type `jira connect`. %v", err) + return p.responsef(header, errConnectToJira, err) } notifications := settingOff - if updatedConnection.Settings.Notifications { - notifications = settingOn + switch args[1] { + case subCommandAssignee: + if *updatedConnection.Settings.SendNotificationsForAssignee { + notifications = settingOn + } + case subCommandMention: + if *updatedConnection.Settings.SendNotificationsForMention { + notifications = settingOn + } + case subCommandReporter: + if *updatedConnection.Settings.SendNotificationsForReporter { + notifications = settingOn + } + case subCommandWatching: + if *updatedConnection.Settings.SendNotificationsForWatching { + notifications = settingOn + } } - return p.responsef(header, "Settings updated. Notifications %s.", notifications) + return p.responsef(header, "Settings updated.\n\t%s notifications %s.", strings.Title(args[1]), notifications) } diff --git a/server/subscribe_test.go b/server/subscribe_test.go index c9ce769a4..2cad226ba 100644 --- a/server/subscribe_test.go +++ b/server/subscribe_test.go @@ -1381,7 +1381,6 @@ func TestGetChannelsSubscribed(t *testing.T) { r := bytes.NewReader(data) bb, err := ioutil.ReadAll(r) require.Nil(t, err) - wh, err := ParseWebhook(bb) assert.Nil(t, err) diff --git a/server/user.go b/server/user.go index e25d5b1c5..82411d1b4 100644 --- a/server/user.go +++ b/server/user.go @@ -43,15 +43,36 @@ func (c *Connection) JiraAccountID() types.ID { } type ConnectionSettings struct { - Notifications bool `json:"notifications"` + Notifications bool `json:"notifications"` + SendNotificationsForMention *bool `json:"send_notifications_for_mention"` + SendNotificationsForAssignee *bool `json:"send_notifications_for_assignee"` + SendNotificationsForReporter *bool `json:"send_notifications_for_reporter"` + SendNotificationsForWatching *bool `json:"send_notifications_for_watching"` } func (s *ConnectionSettings) String() string { - notifications := "off" - if s != nil && s.Notifications { - notifications = "on" + assigneeNotifications := "Notifications for assignee: off" + mentionNotifications := "Notifications for mention: off" + reporterNotifications := "Notifications for reporter: off" + watchingNotifications := "Notifications for watching: off" + + if s != nil && s.ShouldReceiveNotificationsForAssignee() { + assigneeNotifications = "Notifications for assignee: on" + } + + if s != nil && s.ShouldReceiveNotificationsForMention() { + mentionNotifications = "Notifications for mention: on" + } + + if s != nil && s.ShouldReceiveNotificationsForReporter() { + reporterNotifications = "Notifications for reporter: on" + } + + if s != nil && s.ShouldReceiveNotificationsForWatching() { + watchingNotifications = "Notifications for watching: on" } - return fmt.Sprintf("\tNotifications: %s", notifications) + + return fmt.Sprintf("\t- %s \n\t- %s \n\t- %s \n\t- %s", assigneeNotifications, mentionNotifications, reporterNotifications, watchingNotifications) } func NewUser(mattermostUserID types.ID) *User { diff --git a/server/user_cloud.go b/server/user_cloud.go index 4fdd53832..67e1e25df 100644 --- a/server/user_cloud.go +++ b/server/user_cloud.go @@ -92,6 +92,7 @@ func (p *Plugin) httpACUserInteractive(w http.ResponseWriter, r *http.Request, i } mmToken := r.FormValue(argMMToken) + trueValue := true connection := &Connection{ PluginVersion: manifest.Version, User: jira.User{ @@ -102,7 +103,11 @@ func (p *Plugin) httpACUserInteractive(w http.ResponseWriter, r *http.Request, i }, // Set default settings the first time a user connects Settings: &ConnectionSettings{ - Notifications: true, + Notifications: trueValue, + SendNotificationsForWatching: &trueValue, + SendNotificationsForMention: &trueValue, + SendNotificationsForAssignee: &trueValue, + SendNotificationsForReporter: &trueValue, }, } diff --git a/server/user_server.go b/server/user_server.go index f5fa2b9fb..253209942 100644 --- a/server/user_server.go +++ b/server/user_server.go @@ -102,9 +102,15 @@ func (p *Plugin) httpOAuth1aComplete(w http.ResponseWriter, r *http.Request, ins return http.StatusInternalServerError, err } connection.User = *juser - + trueValue := true // Set default settings the first time a user connects - connection.Settings = &ConnectionSettings{Notifications: true} + connection.Settings = &ConnectionSettings{ + Notifications: trueValue, + SendNotificationsForWatching: &trueValue, + SendNotificationsForMention: &trueValue, + SendNotificationsForAssignee: &trueValue, + SendNotificationsForReporter: &trueValue, + } err = p.connectUser(instance, types.ID(mattermostUserID), connection) if err != nil { diff --git a/server/user_test.go b/server/user_test.go index b26761de6..0ea7cca5d 100644 --- a/server/user_test.go +++ b/server/user_test.go @@ -11,17 +11,31 @@ import ( ) func TestUserSettings_String(t *testing.T) { + valueTrue := true + valueFalse := false tests := map[string]struct { settings ConnectionSettings expectedOutput string }{ "notifications on": { - settings: ConnectionSettings{Notifications: false}, - expectedOutput: "\tNotifications: off", + settings: ConnectionSettings{ + Notifications: valueTrue, + SendNotificationsForMention: nil, + SendNotificationsForAssignee: nil, + SendNotificationsForReporter: nil, + SendNotificationsForWatching: nil, + }, + expectedOutput: "\t- Notifications for assignee: on \n\t- Notifications for mention: on \n\t- Notifications for reporter: on \n\t- Notifications for watching: on", }, "notifications off": { - settings: ConnectionSettings{Notifications: true}, - expectedOutput: "\tNotifications: on", + settings: ConnectionSettings{ + Notifications: valueFalse, + SendNotificationsForMention: nil, + SendNotificationsForAssignee: nil, + SendNotificationsForReporter: nil, + SendNotificationsForWatching: nil, + }, + expectedOutput: "\t- Notifications for assignee: off \n\t- Notifications for mention: off \n\t- Notifications for reporter: off \n\t- Notifications for watching: off", }, } for name, tt := range tests { diff --git a/server/webhook.go b/server/webhook.go index f684dea0f..f78bd43e7 100644 --- a/server/webhook.go +++ b/server/webhook.go @@ -46,11 +46,12 @@ type webhook struct { } type webhookUserNotification struct { - jiraUsername string - jiraAccountID string - message string - postType string - commentSelf string + jiraUsername string + jiraAccountID string + message string + postType string + commentSelf string + notificationType string } func (wh *webhook) Events() StringSet { @@ -73,7 +74,7 @@ func (wh webhook) PostToChannel(p *Plugin, instanceID types.ID, channelID, fromU if wh.text != "" && !p.getConfig().HideDecriptionComment { text = p.replaceJiraAccountIds(instanceID, wh.text) } - + if text != "" || len(wh.fields) != 0 { model.ParseSlackAttachment(post, []*model.SlackAttachment{ { @@ -101,7 +102,6 @@ func (wh *webhook) PostNotifications(p *Plugin, instanceID types.ID) ([]*model.P if len(wh.notifications) == 0 { return nil, http.StatusOK, nil } - // We will only send webhook events if we have a connected instance. instance, err := p.instanceStore.LoadInstance(instanceID) if err != nil { @@ -110,6 +110,9 @@ func (wh *webhook) PostNotifications(p *Plugin, instanceID types.ID) ([]*model.P } posts := []*model.Post{} + var mapForNotification = make(map[types.ID]int) + + for _, notification := range wh.notifications { var mattermostUserID types.ID var err error @@ -137,7 +140,29 @@ func (wh *webhook) PostNotifications(p *Plugin, instanceID types.ID) ([]*model.P } // If this is a comment-related webhook, we need to check if they have permissions to read that. // Otherwise, check if they can view the issue. - + switch notification.notificationType { + case subCommandAssignee: + if !c.Settings.ShouldReceiveNotificationsForAssignee() { + continue + } + case subCommandMention: + if !c.Settings.ShouldReceiveNotificationsForMention() { + continue + } + case subCommandReporter: + if !c.Settings.ShouldReceiveNotificationsForReporter() { + continue + } + case subCommandWatching: + if !c.Settings.ShouldReceiveNotificationsForWatching() { + continue + } + } + if _, ok := mapForNotification[mattermostUserID]; ok { + continue + } else { + mapForNotification[mattermostUserID] = 1 + } isCommentEvent := wh.Events().Intersection(commentEvents).Len() > 0 if isCommentEvent { err = client.RESTGet(notification.commentSelf, nil, &struct{}{}) diff --git a/server/webhook_parser.go b/server/webhook_parser.go index ad1dfc71a..20e6e100a 100644 --- a/server/webhook_parser.go +++ b/server/webhook_parser.go @@ -275,9 +275,10 @@ func appendCommentNotifications(wh *webhook, verb string) { } notification := webhookUserNotification{ - message: message, - postType: PostTypeMention, - commentSelf: jwh.Comment.Self, + message: message, + postType: PostTypeMention, + commentSelf: jwh.Comment.Self, + notificationType: "mention", } if isAccountID { @@ -299,11 +300,12 @@ func appendCommentNotifications(wh *webhook, verb string) { } wh.notifications = append(wh.notifications, webhookUserNotification{ - jiraUsername: jwh.Issue.Fields.Assignee.Name, - jiraAccountID: jwh.Issue.Fields.Assignee.AccountID, - message: fmt.Sprintf("%s **commented** on %s:\n>%s", commentAuthor, jwh.mdKeySummaryLink(), jwh.Comment.Body), - postType: PostTypeComment, - commentSelf: jwh.Comment.Self, + jiraUsername: jwh.Issue.Fields.Assignee.Name, + jiraAccountID: jwh.Issue.Fields.Assignee.AccountID, + message: fmt.Sprintf("%s **commented** on %s:\n>%s", commentAuthor, jwh.mdKeySummaryLink(), jwh.Comment.Body), + postType: PostTypeComment, + commentSelf: jwh.Comment.Self, + notificationType: "assignee", }) } @@ -380,9 +382,10 @@ func appendNotificationForAssignee(wh *webhook) { } wh.notifications = append(wh.notifications, webhookUserNotification{ - jiraUsername: jwh.Issue.Fields.Assignee.Name, - jiraAccountID: jwh.Issue.Fields.Assignee.AccountID, - message: fmt.Sprintf("%s **assigned** you to %s", jwh.mdUser(), jwh.mdKeySummaryLink()), + jiraUsername: jwh.Issue.Fields.Assignee.Name, + jiraAccountID: jwh.Issue.Fields.Assignee.AccountID, + message: fmt.Sprintf("%s **assigned** you to %s", jwh.mdUser(), jwh.mdKeySummaryLink()), + notificationType: "assignee", }) } diff --git a/server/webhook_worker.go b/server/webhook_worker.go index a008e665f..f3cafd522 100644 --- a/server/webhook_worker.go +++ b/server/webhook_worker.go @@ -39,15 +39,17 @@ func (ww webhookWorker) process(msg *webhookMessage) (err error) { if err != nil { return err } + v := wh.(*webhook) + if err = v.JiraWebhook.expandIssue(ww.p, msg.InstanceID); err != nil { + return err + } + ww.p.checkIssueWatchers(v, msg.InstanceID) + ww.p.applyReporterNotification(v, msg.InstanceID, v.Issue.Fields.Reporter) if _, _, err = wh.PostNotifications(ww.p, msg.InstanceID); err != nil { ww.p.errorf("WebhookWorker id: %d, error posting notifications, err: %v", ww.id, err) } - v := wh.(*webhook) - if err = v.JiraWebhook.expandIssue(ww.p, msg.InstanceID); err != nil { - return err - } channelsSubscribed, err := ww.p.getChannelsSubscribed(v, msg.InstanceID) if err != nil { From 9f2c461f000139aa9155ef70b8545341c7cf5a48 Mon Sep 17 00:00:00 2001 From: Nityanand Rai Date: Fri, 2 Sep 2022 13:49:03 +0530 Subject: [PATCH 02/17] [MI-2088]: Review Fixes 1. Improved the name of certain variable 2. Improved spacing --- server/client.go | 5 ++--- server/client_cloud.go | 2 +- server/command.go | 2 +- server/issue.go | 41 +++++++++++++++++++--------------------- server/webhook.go | 4 +--- server/webhook_worker.go | 1 - 6 files changed, 24 insertions(+), 31 deletions(-) diff --git a/server/client.go b/server/client.go index 5b99869a9..b9f520cca 100644 --- a/server/client.go +++ b/server/client.go @@ -200,15 +200,14 @@ func (client JiraClient) GetIssue(key string, options *jira.GetQueryOptions) (*j return issue, nil } -// GetWatchers returns an array of Jira users watching for a given issue. +// GetWatchers returns an array of Jira users watching a given issue. func (client JiraClient) GetWatchers(instanceID, issueKey string, connection *Connection) (*jira.Watches, error) { var watchers jira.Watches params := map[string]string{ "accountId": connection.AccountID, } - err := client.RESTGet(instanceID+"/rest/api/2/issue/"+issueKey+"/watchers", params, &watchers) - if err != nil { + if err := client.RESTGet(instanceID+"/rest/api/2/issue/"+issueKey+"/watchers", params, &watchers); err != nil { return nil, err } return &watchers, nil diff --git a/server/client_cloud.go b/server/client_cloud.go index 1e8b011df..6ea8c4c61 100644 --- a/server/client_cloud.go +++ b/server/client_cloud.go @@ -4,7 +4,7 @@ package main import ( - "encoding/json" + "encoding/json" jira "github.com/andygrunwald/go-jira" "github.com/pkg/errors" diff --git a/server/command.go b/server/command.go index d79fef897..17e085ac9 100644 --- a/server/command.go +++ b/server/command.go @@ -257,7 +257,7 @@ func createSettingsCommand(optInstance bool) *model.AutocompleteData { } notifications := model.NewAutocompleteData( - "notifications", "[assinee|mention|reporter]", "manage notifications") + "notifications", "[assinee|mention|reporter|watching]", "manage notifications") assigneeNotifications := model.NewAutocompleteData( subCommandAssignee, "", "manage assignee notifications") diff --git a/server/issue.go b/server/issue.go index 70f0999a0..2f74d7797 100644 --- a/server/issue.go +++ b/server/issue.go @@ -502,13 +502,13 @@ func (p *Plugin) httpGetJiraProjectMetadata(w http.ResponseWriter, r *http.Reque instanceID := r.FormValue("instance_id") - cimd, connection, err := p.GetJiraProjectMetadata(types.ID(instanceID), types.ID(mattermostUserID)) + metainfo, connection, err := p.GetJiraProjectMetadata(types.ID(instanceID), types.ID(mattermostUserID)) if err != nil { return respondErr(w, http.StatusInternalServerError, errors.WithMessage(err, "failed to GetProjectMetadata")) } - if len(cimd.Projects) == 0 { + if len(metainfo.Projects) == 0 { _, err = respondJSON(w, map[string]interface{}{ "error": "You do not have permission to create issues in any projects. Please contact your Jira admin.", }) @@ -519,9 +519,9 @@ func (p *Plugin) httpGetJiraProjectMetadata(w http.ResponseWriter, r *http.Reque } type option = utils.ReactSelectOption - projects := make([]option, 0, len(cimd.Projects)) - issues := make(map[string][]option, len(cimd.Projects)) - for _, prj := range cimd.Projects { + projects := make([]option, 0, len(metainfo.Projects)) + issues := make(map[string][]option, len(metainfo.Projects)) + for _, prj := range metainfo.Projects { projects = append(projects, option{ Value: prj.Key, Label: prj.Name, @@ -1054,13 +1054,13 @@ func (p *Plugin) checkIssueWatchers(wh *webhook, instanceID types.ID) { return } - watcherUsers, err := client.GetWatchers(instanceID.String(), wh.Issue.ID, connection) + watchers, err := client.GetWatchers(instanceID.String(), wh.Issue.ID, connection) if err != nil { - p.errorf("error while getting watchers for issue , err : %v", err) + p.errorf("error while getting watchers for issue , err : %v , issue id : %v", err, wh.Issue.ID) return } - for _, watcherUser := range watcherUsers.Watchers { + for _, watcherUser := range watchers.Watchers { whUserNotification := webhookUserNotification{ jiraUsername: watcherUser.Name, @@ -1081,7 +1081,7 @@ func (p *Plugin) applyReporterNotification(wh *webhook, instanceID types.ID, rep } jwhook := wh.JiraWebhook - + if reporter == nil || (reporter.Name != "" && reporter.Name == jwhook.User.Name) || (reporter.AccountID != "" && reporter.AccountID == jwhook.Comment.UpdateAuthor.AccountID) { @@ -1092,11 +1092,11 @@ func (p *Plugin) applyReporterNotification(wh *webhook, instanceID types.ID, rep commentMessage := fmt.Sprintf("%s **commented** on %s:\n> %s", commentAuthor, jwhook.mdKeySummaryLink(), jwhook.Comment.Body) - c, err := p.GetUserSetting(wh, instanceID, reporter.Name, reporter.AccountID) - if err != nil || c.Settings == nil || !c.Settings.ShouldReceiveNotificationsForReporter() { + connection, err := p.GetUserSetting(wh, instanceID, reporter.Name, reporter.AccountID) + if err != nil || connection.Settings == nil || !connection.Settings.ShouldReceiveNotificationsForReporter() { return } - + wh.notifications = append(wh.notifications, webhookUserNotification{ jiraUsername: reporter.Name, jiraAccountID: reporter.AccountID, @@ -1108,7 +1108,6 @@ func (p *Plugin) applyReporterNotification(wh *webhook, instanceID types.ID, rep } func (p *Plugin) GetUserSetting(wh *webhook, instanceID types.ID, jiraAccountID, jiraUsername string) (*Connection, error) { - var err error instance, err := p.instanceStore.LoadInstance(instanceID) if err != nil { return nil, err @@ -1119,17 +1118,16 @@ func (p *Plugin) GetUserSetting(wh *webhook, instanceID types.ID, jiraAccountID, } else { mattermostUserID, err = p.userStore.LoadMattermostUserID(instance.GetID(), jiraUsername) } - if err != nil { return nil, err } - c, err := p.userStore.LoadConnection(instanceID, mattermostUserID) + connection, err := p.userStore.LoadConnection(instanceID, mattermostUserID) if err != nil { return nil, err } - return c, nil + return connection, nil } func (s *ConnectionSettings) ShouldReceiveNotificationsForAssignee() bool { @@ -1200,22 +1198,21 @@ func (wh *webhook) fetchConnectedUser(p *Plugin, instanceID types.ID) (Client, * } else { mattermostUserID, err = p.userStore.LoadMattermostUserID(instance.GetID(), account["Name"]) } - if err != nil { continue } - c, err2 := p.userStore.LoadConnection(instance.GetID(), mattermostUserID) - if err2 != nil { + connection, err := p.userStore.LoadConnection(instance.GetID(), mattermostUserID) + if err != nil { continue } - client, err2 := instance.GetClient(c) - if err2 != nil { + client, err := instance.GetClient(connection) + if err != nil { continue } - return client, c, nil + return client, connection, nil } return nil, nil, nil diff --git a/server/webhook.go b/server/webhook.go index f78bd43e7..43a7c553f 100644 --- a/server/webhook.go +++ b/server/webhook.go @@ -74,7 +74,7 @@ func (wh webhook) PostToChannel(p *Plugin, instanceID types.ID, channelID, fromU if wh.text != "" && !p.getConfig().HideDecriptionComment { text = p.replaceJiraAccountIds(instanceID, wh.text) } - + if text != "" || len(wh.fields) != 0 { model.ParseSlackAttachment(post, []*model.SlackAttachment{ { @@ -111,8 +111,6 @@ func (wh *webhook) PostNotifications(p *Plugin, instanceID types.ID) ([]*model.P posts := []*model.Post{} var mapForNotification = make(map[types.ID]int) - - for _, notification := range wh.notifications { var mattermostUserID types.ID var err error diff --git a/server/webhook_worker.go b/server/webhook_worker.go index f3cafd522..518dd1868 100644 --- a/server/webhook_worker.go +++ b/server/webhook_worker.go @@ -50,7 +50,6 @@ func (ww webhookWorker) process(msg *webhookMessage) (err error) { ww.p.errorf("WebhookWorker id: %d, error posting notifications, err: %v", ww.id, err) } - channelsSubscribed, err := ww.p.getChannelsSubscribed(v, msg.InstanceID) if err != nil { return err From 0e1219ed58636ceed33a62cca7c6038cdd0f7084 Mon Sep 17 00:00:00 2001 From: Nityanand Rai Date: Mon, 5 Sep 2022 13:00:54 +0530 Subject: [PATCH 03/17] [MI-2088]:Revieww fixes 1.Changes the names of few variables 2.Improved code complexity --- server/client.go | 11 ++++++----- server/command.go | 2 +- server/issue.go | 14 +++++++------- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/server/client.go b/server/client.go index b9f520cca..6425806b4 100644 --- a/server/client.go +++ b/server/client.go @@ -169,13 +169,13 @@ func (client JiraClient) RESTPostAttachment(issueID string, data []byte, name st } func (client JiraClient) GetAllProjectKeys() ([]string, error) { - projectlist, resp, err := client.Jira.Project.GetList() + projectList, resp, err := client.Jira.Project.GetList() if err != nil { return nil, userFriendlyJiraError(resp, err) } - keys := make([]string, 0, len(*projectlist)) - for _, project := range *projectlist { + keys := make([]string, 0, len(*projectList)) + for _, project := range *projectList { keys = append(keys, project.Key) } @@ -206,8 +206,9 @@ func (client JiraClient) GetWatchers(instanceID, issueKey string, connection *Co params := map[string]string{ "accountId": connection.AccountID, } + endpoint := fmt.Sprintf("%s%s%s%s", instanceID, "/rest/api/2/issue/",issueKey,"/watchers") - if err := client.RESTGet(instanceID+"/rest/api/2/issue/"+issueKey+"/watchers", params, &watchers); err != nil { + if err := client.RESTGet(endpoint, params, &watchers); err != nil { return nil, err } return &watchers, nil @@ -418,7 +419,7 @@ func endpointURL(endpoint string) (string, error) { return "", err } if parsedURL.Scheme == "" { - // relative path + // Relative path endpoint = fmt.Sprintf("/rest/api/%s", endpoint) } return endpoint, nil diff --git a/server/command.go b/server/command.go index 17e085ac9..1d49527f9 100644 --- a/server/command.go +++ b/server/command.go @@ -609,7 +609,7 @@ func executeSettings(p *Plugin, c *plugin.Context, header *model.CommandArgs, ar if conn.Settings != nil { return p.responsef(header, "Current settings:\n%s", conn.Settings.String()) } - return p.responsef(header, "Please connect to jira account `/jira connect`") + return p.responsef(header, "Please connect to jira account using command `/jira connect`") case "notifications": return p.settingsNotifications(header, instance.GetID(), user.MattermostUserID, conn, args) default: diff --git a/server/issue.go b/server/issue.go index 2f74d7797..7f0dee676 100644 --- a/server/issue.go +++ b/server/issue.go @@ -521,13 +521,13 @@ func (p *Plugin) httpGetJiraProjectMetadata(w http.ResponseWriter, r *http.Reque type option = utils.ReactSelectOption projects := make([]option, 0, len(metainfo.Projects)) issues := make(map[string][]option, len(metainfo.Projects)) - for _, prj := range metainfo.Projects { + for _, project := range metainfo.Projects { projects = append(projects, option{ - Value: prj.Key, - Label: prj.Name, + Value: project.Key, + Label: project.Name, }) - issueTypes := make([]option, 0, len(prj.IssueTypes)) - for _, issue := range prj.IssueTypes { + issueTypes := make([]option, 0, len(project.IssueTypes)) + for _, issue := range project.IssueTypes { if issue.Subtasks { continue } @@ -536,7 +536,7 @@ func (p *Plugin) httpGetJiraProjectMetadata(w http.ResponseWriter, r *http.Reque Label: issue.Name, }) } - issues[prj.Key] = issueTypes + issues[project.Key] = issueTypes } return respondJSON(w, OutProjectMetadata{ @@ -1056,7 +1056,7 @@ func (p *Plugin) checkIssueWatchers(wh *webhook, instanceID types.ID) { watchers, err := client.GetWatchers(instanceID.String(), wh.Issue.ID, connection) if err != nil { - p.errorf("error while getting watchers for issue , err : %v , issue id : %v", err, wh.Issue.ID) + p.errorf("error while getting watchers for the issue id %v , err : %v", wh.Issue.ID, err) return } From 4a9c195ebe36a797098f349796291e4c8d30f374 Mon Sep 17 00:00:00 2001 From: Nityanand Rai Date: Wed, 7 Sep 2022 16:51:12 +0530 Subject: [PATCH 04/17] [MI-2088]: Review fixes done 1.Changed the name of few variables --- server/client.go | 2 +- server/issue.go | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/server/client.go b/server/client.go index 6425806b4..b0f9b2e38 100644 --- a/server/client.go +++ b/server/client.go @@ -206,7 +206,7 @@ func (client JiraClient) GetWatchers(instanceID, issueKey string, connection *Co params := map[string]string{ "accountId": connection.AccountID, } - endpoint := fmt.Sprintf("%s%s%s%s", instanceID, "/rest/api/2/issue/",issueKey,"/watchers") + endpoint := fmt.Sprintf("%s%s%s%s", instanceID, "/rest/api/2/issue/", issueKey, "/watchers") if err := client.RESTGet(endpoint, params, &watchers); err != nil { return nil, err diff --git a/server/issue.go b/server/issue.go index 7f0dee676..185bd92bf 100644 --- a/server/issue.go +++ b/server/issue.go @@ -502,13 +502,13 @@ func (p *Plugin) httpGetJiraProjectMetadata(w http.ResponseWriter, r *http.Reque instanceID := r.FormValue("instance_id") - metainfo, connection, err := p.GetJiraProjectMetadata(types.ID(instanceID), types.ID(mattermostUserID)) + metaInfo, connection, err := p.GetJiraProjectMetadata(types.ID(instanceID), types.ID(mattermostUserID)) if err != nil { return respondErr(w, http.StatusInternalServerError, errors.WithMessage(err, "failed to GetProjectMetadata")) } - if len(metainfo.Projects) == 0 { + if len(metaInfo.Projects) == 0 { _, err = respondJSON(w, map[string]interface{}{ "error": "You do not have permission to create issues in any projects. Please contact your Jira admin.", }) @@ -519,9 +519,9 @@ func (p *Plugin) httpGetJiraProjectMetadata(w http.ResponseWriter, r *http.Reque } type option = utils.ReactSelectOption - projects := make([]option, 0, len(metainfo.Projects)) - issues := make(map[string][]option, len(metainfo.Projects)) - for _, project := range metainfo.Projects { + projects := make([]option, 0, len(metaInfo.Projects)) + issues := make(map[string][]option, len(metaInfo.Projects)) + for _, project := range metaInfo.Projects { projects = append(projects, option{ Value: project.Key, Label: project.Name, @@ -1061,7 +1061,6 @@ func (p *Plugin) checkIssueWatchers(wh *webhook, instanceID types.ID) { } for _, watcherUser := range watchers.Watchers { - whUserNotification := webhookUserNotification{ jiraUsername: watcherUser.Name, jiraAccountID: watcherUser.AccountID, From cecaaaa53f51308e26402812251ad53c0da6fc25 Mon Sep 17 00:00:00 2001 From: Kshitij Katiyar Date: Wed, 21 Sep 2022 16:05:07 +0530 Subject: [PATCH 05/17] [MI-2088]:fixed review fixes --- server/client.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/server/client.go b/server/client.go index b0f9b2e38..33a82b298 100644 --- a/server/client.go +++ b/server/client.go @@ -12,6 +12,7 @@ import ( "mime/multipart" "net/http" "net/url" + "path" "regexp" "strconv" "strings" @@ -174,9 +175,9 @@ func (client JiraClient) GetAllProjectKeys() ([]string, error) { return nil, userFriendlyJiraError(resp, err) } - keys := make([]string, 0, len(*projectList)) - for _, project := range *projectList { - keys = append(keys, project.Key) + keys := make([]string, len(*projectList)) + for index, project := range *projectList { + keys[index] = project.Key } return keys, nil @@ -206,7 +207,7 @@ func (client JiraClient) GetWatchers(instanceID, issueKey string, connection *Co params := map[string]string{ "accountId": connection.AccountID, } - endpoint := fmt.Sprintf("%s%s%s%s", instanceID, "/rest/api/2/issue/", issueKey, "/watchers") + endpoint := fmt.Sprintf("%s/rest/api/2/issue/%s/watchers", instanceID, issueKey) if err := client.RESTGet(endpoint, params, &watchers); err != nil { return nil, err @@ -420,7 +421,7 @@ func endpointURL(endpoint string) (string, error) { } if parsedURL.Scheme == "" { // Relative path - endpoint = fmt.Sprintf("/rest/api/%s", endpoint) + endpoint = path.Join("/rest/api", endpoint) } return endpoint, nil } From 47d419601531e050d92cbeb52016099dfdcfa5e4 Mon Sep 17 00:00:00 2001 From: Kshitij Katiyar Date: Thu, 22 Sep 2022 15:42:21 +0530 Subject: [PATCH 06/17] [MI-2088]:fixed review fixes --- server/client.go | 1 + server/command.go | 16 ++++++-------- server/command_test.go | 20 ++++++++---------- server/issue.go | 48 ++++++++++++++++++++++-------------------- server/kv_mock_test.go | 2 +- server/settings.go | 4 ++-- 6 files changed, 44 insertions(+), 47 deletions(-) diff --git a/server/client.go b/server/client.go index 33a82b298..d0b9dedad 100644 --- a/server/client.go +++ b/server/client.go @@ -207,6 +207,7 @@ func (client JiraClient) GetWatchers(instanceID, issueKey string, connection *Co params := map[string]string{ "accountId": connection.AccountID, } + endpoint := fmt.Sprintf("%s/rest/api/2/issue/%s/watchers", instanceID, issueKey) if err := client.RESTGet(endpoint, params, &watchers); err != nil { diff --git a/server/command.go b/server/command.go index 1d49527f9..1b7ff1036 100644 --- a/server/command.go +++ b/server/command.go @@ -257,22 +257,18 @@ func createSettingsCommand(optInstance bool) *model.AutocompleteData { } notifications := model.NewAutocompleteData( - "notifications", "[assinee|mention|reporter|watching]", "manage notifications") + "notifications", "[assignee|mention|reporter|watching]", "manage notifications") - assigneeNotifications := model.NewAutocompleteData( - subCommandAssignee, "", "manage assignee notifications") + assigneeNotifications := model.NewAutocompleteData(subCommandAssignee, "", "manage assignee notifications") assigneeNotifications.AddStaticListArgument("value", true, setting) - mentionNotifications := model.NewAutocompleteData( - subCommandMention, "", "manage mention notifications") + mentionNotifications := model.NewAutocompleteData(subCommandMention, "", "manage mention notifications") mentionNotifications.AddStaticListArgument("value", true, setting) - reporterNotifications := model.NewAutocompleteData( - subCommandReporter, "", "manage reporter notifications") + reporterNotifications := model.NewAutocompleteData(subCommandReporter, "", "manage reporter notifications") reporterNotifications.AddStaticListArgument("value", true, setting) - watchingNotifications := model.NewAutocompleteData( - subCommandWatching, "", "manage watching notifications") + watchingNotifications := model.NewAutocompleteData(subCommandWatching, "", "manage watching notifications") reporterNotifications.AddStaticListArgument("value", true, setting) notifications.AddCommand(assigneeNotifications) notifications.AddCommand(mentionNotifications) @@ -609,7 +605,7 @@ func executeSettings(p *Plugin, c *plugin.Context, header *model.CommandArgs, ar if conn.Settings != nil { return p.responsef(header, "Current settings:\n%s", conn.Settings.String()) } - return p.responsef(header, "Please connect to jira account using command `/jira connect`") + return p.responsef(header, "Please connect to Jira account using the command `/jira connect`") case "notifications": return p.settingsNotifications(header, instance.GetID(), user.MattermostUserID, conn, args) default: diff --git a/server/command_test.go b/server/command_test.go index 00fd51a13..5ca08cf0d 100644 --- a/server/command_test.go +++ b/server/command_test.go @@ -24,13 +24,12 @@ import ( ) const ( - mockUserIDWithNotifications = "1" - mockUserIDWithoutNotifications = "2" - mockUserIDUnknown = "3" - mockUserIDSysAdmin = "4" - mockUserIDNonSysAdmin = "5" - mattermostSiteURL = "https://somelink.com" - mockMattermostIDWithNotifications = "testMattermostUserId012345" + mockUserIDWithNotifications = "1" + mockUserIDWithoutNotifications = "2" + mockUserIDUnknown = "3" + mockUserIDSysAdmin = "4" + mockUserIDNonSysAdmin = "5" + mattermostSiteURL = "https://somelink.com" ) type mockUserStoreKV struct { @@ -87,10 +86,9 @@ func getMockUserStoreKV() mockUserStoreKV { mockUserIDWithoutNotifications: newuser(mockUserIDWithoutNotifications), }, connections: map[types.ID]*Connection{ - mockUserIDWithNotifications: &withNotifications, - mockUserIDWithoutNotifications: &connection, - "connected_user": &connection, - mockMattermostIDWithNotifications: &withNotifications, + mockUserIDWithNotifications: &withNotifications, + mockUserIDWithoutNotifications: &connection, + "connected_user": &connection, }, } } diff --git a/server/issue.go b/server/issue.go index 185bd92bf..8791f0dd8 100644 --- a/server/issue.go +++ b/server/issue.go @@ -22,12 +22,15 @@ import ( ) const ( - labelsField = "labels" - statusField = "status" - reporterField = "reporter" - priorityField = "priority" - descriptionField = "description" - resolutionField = "resolution" + labelsField = "labels" + statusField = "status" + reporterField = "reporter" + priorityField = "priority" + descriptionField = "description" + resolutionField = "resolution" + createdCommentEvent = "event_created_comment" + notificationTypeReporter = "reporter" + notificationTypeWatching = "watching" ) func makePost(userID, channelID, message string) *model.Post { @@ -519,22 +522,22 @@ func (p *Plugin) httpGetJiraProjectMetadata(w http.ResponseWriter, r *http.Reque } type option = utils.ReactSelectOption - projects := make([]option, 0, len(metaInfo.Projects)) + projects := make([]option, len(metaInfo.Projects)) issues := make(map[string][]option, len(metaInfo.Projects)) - for _, project := range metaInfo.Projects { - projects = append(projects, option{ + for index, project := range metaInfo.Projects { + projects[index] = option{ Value: project.Key, Label: project.Name, - }) - issueTypes := make([]option, 0, len(project.IssueTypes)) - for _, issue := range project.IssueTypes { - if issue.Subtasks { + } + issueTypes := make([]option, len(project.IssueTypes)) + for index, issueType := range project.IssueTypes { + if issueType.Subtasks { continue } - issueTypes = append(issueTypes, option{ - Value: issue.Id, - Label: issue.Name, - }) + issueTypes[index] = option{ + Value: issueType.Id, + Label: issueType.Name, + } } issues[project.Key] = issueTypes } @@ -1042,7 +1045,7 @@ func (p *Plugin) getClient(instanceID, mattermostUserID types.ID) (Client, Insta } func (p *Plugin) checkIssueWatchers(wh *webhook, instanceID types.ID) { - if !wh.eventTypes.ContainsAny("event_created_comment") { + if !wh.eventTypes.ContainsAny(createdCommentEvent) { return } @@ -1051,6 +1054,7 @@ func (p *Plugin) checkIssueWatchers(wh *webhook, instanceID types.ID) { commentMessage := fmt.Sprintf("%s **commented** on %s:\n> %s", commentAuthor, jwhook.mdKeySummaryLink(), jwhook.Comment.Body) client, connection, err := wh.fetchConnectedUser(p, instanceID) if err != nil || client == nil { + p.errorf("error while fetching connected users for the instance id %v , err : %v", instanceID, err) return } @@ -1067,7 +1071,7 @@ func (p *Plugin) checkIssueWatchers(wh *webhook, instanceID types.ID) { message: commentMessage, postType: PostTypeComment, commentSelf: wh.JiraWebhook.Comment.Self, - notificationType: "watching", + notificationType: notificationTypeWatching, } wh.notifications = append(wh.notifications, whUserNotification) @@ -1075,12 +1079,11 @@ func (p *Plugin) checkIssueWatchers(wh *webhook, instanceID types.ID) { } func (p *Plugin) applyReporterNotification(wh *webhook, instanceID types.ID, reporter *jira.User) { - if !wh.eventTypes.ContainsAny("event_created_comment") { + if !wh.eventTypes.ContainsAny(createdCommentEvent) { return } jwhook := wh.JiraWebhook - if reporter == nil || (reporter.Name != "" && reporter.Name == jwhook.User.Name) || (reporter.AccountID != "" && reporter.AccountID == jwhook.Comment.UpdateAuthor.AccountID) { @@ -1088,7 +1091,6 @@ func (p *Plugin) applyReporterNotification(wh *webhook, instanceID types.ID, rep } commentAuthor := mdUser(&jwhook.Comment.UpdateAuthor) - commentMessage := fmt.Sprintf("%s **commented** on %s:\n> %s", commentAuthor, jwhook.mdKeySummaryLink(), jwhook.Comment.Body) connection, err := p.GetUserSetting(wh, instanceID, reporter.Name, reporter.AccountID) @@ -1102,7 +1104,7 @@ func (p *Plugin) applyReporterNotification(wh *webhook, instanceID types.ID, rep message: commentMessage, postType: PostTypeComment, commentSelf: jwhook.Comment.Self, - notificationType: "reporter", + notificationType: notificationTypeReporter, }) } diff --git a/server/kv_mock_test.go b/server/kv_mock_test.go index 4be78e2df..a57d7a2f7 100644 --- a/server/kv_mock_test.go +++ b/server/kv_mock_test.go @@ -86,7 +86,7 @@ func (store mockUserStore) LoadConnection(types.ID, types.ID) (*Connection, erro }, nil } func (store mockUserStore) LoadMattermostUserID(instanceID types.ID, jiraUserName string) (types.ID, error) { - return mockMattermostIDWithNotifications, nil + return mockUserIDWithNotifications, nil } func (store mockUserStore) DeleteConnection(instanceID, mattermostUserID types.ID) error { return nil diff --git a/server/settings.go b/server/settings.go index 43620fdc1..b57b84f42 100644 --- a/server/settings.go +++ b/server/settings.go @@ -12,8 +12,8 @@ const ( settingOn = "on" settingOff = "off" - errStoreNewSettings = "Could not store new settings. Please contact your system administrator. error: %v" - errConnectToJira = "Your username is not connected to Jira. Please type `/jira connect`. %v" + errStoreNewSettings = "Could not store new settings. Please contact your system administrator. Error: %v" + errConnectToJira = "Your account is not connected to Jira. Please type `/jira connect`. %v" subCommandAssignee = "assignee" subCommandMention = "mention" subCommandReporter = "reporter" From 7c5d1a9f117592c253e4a8149848befc3cd00ef7 Mon Sep 17 00:00:00 2001 From: Kshitij Katiyar Date: Thu, 22 Sep 2022 16:07:09 +0530 Subject: [PATCH 07/17] [MI-2088]:fixed review fixes --- server/issue.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/issue.go b/server/issue.go index 8791f0dd8..1ead6fe6c 100644 --- a/server/issue.go +++ b/server/issue.go @@ -1054,7 +1054,7 @@ func (p *Plugin) checkIssueWatchers(wh *webhook, instanceID types.ID) { commentMessage := fmt.Sprintf("%s **commented** on %s:\n> %s", commentAuthor, jwhook.mdKeySummaryLink(), jwhook.Comment.Body) client, connection, err := wh.fetchConnectedUser(p, instanceID) if err != nil || client == nil { - p.errorf("error while fetching connected users for the instance id %v , err : %v", instanceID, err) + p.errorf("error while fetching connected users for the instanceID %v , Error : %v", instanceID, err) return } From b7523238a1ace1e7a893db09770865ef86d54550 Mon Sep 17 00:00:00 2001 From: Nityanand Rai Date: Wed, 28 Sep 2022 15:49:47 +0530 Subject: [PATCH 08/17] [MI-2088]:Review fixes done 1.Changed the way to store assignee, mention, reporter and watching in struct so that the code can be easily tested --- server/command_test.go | 12 +++++++----- server/issue.go | 32 ++++---------------------------- server/settings.go | 37 ++++++++++--------------------------- server/user.go | 15 ++++++--------- server/user_cloud.go | 12 +++++++----- server/user_server.go | 12 +++++++----- server/user_test.go | 24 ++++++++++++++---------- server/webhook.go | 19 ++----------------- 8 files changed, 57 insertions(+), 106 deletions(-) diff --git a/server/command_test.go b/server/command_test.go index 5ca08cf0d..49f27811a 100644 --- a/server/command_test.go +++ b/server/command_test.go @@ -72,11 +72,13 @@ func getMockUserStoreKV() mockUserStoreKV { trueValue := true withNotifications := connection // copy withNotifications.Settings = &ConnectionSettings{ - Notifications: trueValue, - SendNotificationsForMention: &trueValue, - SendNotificationsForAssignee: &trueValue, - SendNotificationsForReporter: &trueValue, - SendNotificationsForWatching: &trueValue, + Notifications: trueValue, + RoleNotification: map[string]*bool{ + "assignee": &trueValue, + "mention": &trueValue, + "reporter": &trueValue, + "watching": &trueValue, + }, } return mockUserStoreKV{ diff --git a/server/issue.go b/server/issue.go index 1ead6fe6c..fb6aaad48 100644 --- a/server/issue.go +++ b/server/issue.go @@ -1094,7 +1094,7 @@ func (p *Plugin) applyReporterNotification(wh *webhook, instanceID types.ID, rep commentMessage := fmt.Sprintf("%s **commented** on %s:\n> %s", commentAuthor, jwhook.mdKeySummaryLink(), jwhook.Comment.Body) connection, err := p.GetUserSetting(wh, instanceID, reporter.Name, reporter.AccountID) - if err != nil || connection.Settings == nil || !connection.Settings.ShouldReceiveNotificationsForReporter() { + if err != nil || connection.Settings == nil || !connection.Settings.ShouldReceiveNotificationsFor("reporter") { return } @@ -1131,39 +1131,15 @@ func (p *Plugin) GetUserSetting(wh *webhook, instanceID types.ID, jiraAccountID, return connection, nil } -func (s *ConnectionSettings) ShouldReceiveNotificationsForAssignee() bool { - if s.SendNotificationsForAssignee != nil { - return *s.SendNotificationsForAssignee +func (s *ConnectionSettings) ShouldReceiveNotificationsFor(role string) bool { + if val, ok := s.RoleNotification[role]; ok { + return *val } // Check old setting for backwards compatibility return s.Notifications } -func (s *ConnectionSettings) ShouldReceiveNotificationsForReporter() bool { - if s.SendNotificationsForReporter != nil { - return *s.SendNotificationsForReporter - } - - return s.Notifications -} - -func (s *ConnectionSettings) ShouldReceiveNotificationsForMention() bool { - if s.SendNotificationsForMention != nil { - return *s.SendNotificationsForMention - } - - return s.Notifications -} - -func (s *ConnectionSettings) ShouldReceiveNotificationsForWatching() bool { - if s.SendNotificationsForWatching != nil { - return *s.SendNotificationsForWatching - } - - return s.Notifications -} - func (wh *webhook) fetchConnectedUser(p *Plugin, instanceID types.ID) (Client, *Connection, error) { var accountInformation []map[string]string diff --git a/server/settings.go b/server/settings.go index b57b84f42..f7e12e20a 100644 --- a/server/settings.go +++ b/server/settings.go @@ -20,6 +20,13 @@ const ( subCommandWatching = "watching" ) +func (connection *Connection) sendNotificationsFor(role string, hasNotification bool) bool { + if role != "assignee" && role != "mention" && role != "reporter" && role != "watching" { + return false + } + connection.Settings.RoleNotification[role] = &hasNotification + return true +} func (p *Plugin) settingsNotifications(header *model.CommandArgs, instanceID, mattermostUserID types.ID, connection *Connection, args []string) *model.CommandResponse { const helpText = "`/jira settings notifications [assignee|mention|reporter|watching] [value]`\n* Invalid value. Accepted values are: `on` or `off`." @@ -40,16 +47,7 @@ func (p *Plugin) settingsNotifications(header *model.CommandArgs, instanceID, ma if connection.Settings == nil { connection.Settings = &ConnectionSettings{} } - switch args[1] { - case subCommandAssignee: - connection.Settings.SendNotificationsForAssignee = &value - case subCommandMention: - connection.Settings.SendNotificationsForMention = &value - case subCommandReporter: - connection.Settings.SendNotificationsForReporter = &value - case subCommandWatching: - connection.Settings.SendNotificationsForWatching = &value - default: + if !connection.sendNotificationsFor(args[1], value) { return p.responsef(header, helpText) } @@ -64,23 +62,8 @@ func (p *Plugin) settingsNotifications(header *model.CommandArgs, instanceID, ma return p.responsef(header, errConnectToJira, err) } notifications := settingOff - switch args[1] { - case subCommandAssignee: - if *updatedConnection.Settings.SendNotificationsForAssignee { - notifications = settingOn - } - case subCommandMention: - if *updatedConnection.Settings.SendNotificationsForMention { - notifications = settingOn - } - case subCommandReporter: - if *updatedConnection.Settings.SendNotificationsForReporter { - notifications = settingOn - } - case subCommandWatching: - if *updatedConnection.Settings.SendNotificationsForWatching { - notifications = settingOn - } + if *updatedConnection.Settings.RoleNotification[args[1]] { + notifications = settingOn } return p.responsef(header, "Settings updated.\n\t%s notifications %s.", strings.Title(args[1]), notifications) diff --git a/server/user.go b/server/user.go index 82411d1b4..2ef7a106b 100644 --- a/server/user.go +++ b/server/user.go @@ -43,11 +43,8 @@ func (c *Connection) JiraAccountID() types.ID { } type ConnectionSettings struct { - Notifications bool `json:"notifications"` - SendNotificationsForMention *bool `json:"send_notifications_for_mention"` - SendNotificationsForAssignee *bool `json:"send_notifications_for_assignee"` - SendNotificationsForReporter *bool `json:"send_notifications_for_reporter"` - SendNotificationsForWatching *bool `json:"send_notifications_for_watching"` + Notifications bool `json:"notifications"` + RoleNotification map[string]*bool } func (s *ConnectionSettings) String() string { @@ -56,19 +53,19 @@ func (s *ConnectionSettings) String() string { reporterNotifications := "Notifications for reporter: off" watchingNotifications := "Notifications for watching: off" - if s != nil && s.ShouldReceiveNotificationsForAssignee() { + if s != nil && s.ShouldReceiveNotificationsFor("assignee") { assigneeNotifications = "Notifications for assignee: on" } - if s != nil && s.ShouldReceiveNotificationsForMention() { + if s != nil && s.ShouldReceiveNotificationsFor("mention") { mentionNotifications = "Notifications for mention: on" } - if s != nil && s.ShouldReceiveNotificationsForReporter() { + if s != nil && s.ShouldReceiveNotificationsFor("reporter") { reporterNotifications = "Notifications for reporter: on" } - if s != nil && s.ShouldReceiveNotificationsForWatching() { + if s != nil && s.ShouldReceiveNotificationsFor("watching") { watchingNotifications = "Notifications for watching: on" } diff --git a/server/user_cloud.go b/server/user_cloud.go index 67e1e25df..00dcbcdba 100644 --- a/server/user_cloud.go +++ b/server/user_cloud.go @@ -103,11 +103,13 @@ func (p *Plugin) httpACUserInteractive(w http.ResponseWriter, r *http.Request, i }, // Set default settings the first time a user connects Settings: &ConnectionSettings{ - Notifications: trueValue, - SendNotificationsForWatching: &trueValue, - SendNotificationsForMention: &trueValue, - SendNotificationsForAssignee: &trueValue, - SendNotificationsForReporter: &trueValue, + Notifications: trueValue, + RoleNotification: map[string]*bool{ + "mention": &trueValue, + "assignee": &trueValue, + "reporter": &trueValue, + "watching": &trueValue, + }, }, } diff --git a/server/user_server.go b/server/user_server.go index 253209942..7955720b2 100644 --- a/server/user_server.go +++ b/server/user_server.go @@ -105,11 +105,13 @@ func (p *Plugin) httpOAuth1aComplete(w http.ResponseWriter, r *http.Request, ins trueValue := true // Set default settings the first time a user connects connection.Settings = &ConnectionSettings{ - Notifications: trueValue, - SendNotificationsForWatching: &trueValue, - SendNotificationsForMention: &trueValue, - SendNotificationsForAssignee: &trueValue, - SendNotificationsForReporter: &trueValue, + Notifications: trueValue, + RoleNotification: map[string]*bool{ + "mention": &trueValue, + "assignee": &trueValue, + "reporter": &trueValue, + "watching": &trueValue, + }, } err = p.connectUser(instance, types.ID(mattermostUserID), connection) diff --git a/server/user_test.go b/server/user_test.go index 0ea7cca5d..6c251664c 100644 --- a/server/user_test.go +++ b/server/user_test.go @@ -19,21 +19,25 @@ func TestUserSettings_String(t *testing.T) { }{ "notifications on": { settings: ConnectionSettings{ - Notifications: valueTrue, - SendNotificationsForMention: nil, - SendNotificationsForAssignee: nil, - SendNotificationsForReporter: nil, - SendNotificationsForWatching: nil, + Notifications: valueTrue, + RoleNotification: map[string]*bool{ + "assignee": nil, + "mention": nil, + "reporter": nil, + "watching": nil, + }, }, expectedOutput: "\t- Notifications for assignee: on \n\t- Notifications for mention: on \n\t- Notifications for reporter: on \n\t- Notifications for watching: on", }, "notifications off": { settings: ConnectionSettings{ - Notifications: valueFalse, - SendNotificationsForMention: nil, - SendNotificationsForAssignee: nil, - SendNotificationsForReporter: nil, - SendNotificationsForWatching: nil, + Notifications: valueFalse, + RoleNotification: map[string]*bool{ + "assignee": nil, + "mention": nil, + "reporter": nil, + "watching": nil, + }, }, expectedOutput: "\t- Notifications for assignee: off \n\t- Notifications for mention: off \n\t- Notifications for reporter: off \n\t- Notifications for watching: off", }, diff --git a/server/webhook.go b/server/webhook.go index 43a7c553f..b4e6b7e1c 100644 --- a/server/webhook.go +++ b/server/webhook.go @@ -138,23 +138,8 @@ func (wh *webhook) PostNotifications(p *Plugin, instanceID types.ID) ([]*model.P } // If this is a comment-related webhook, we need to check if they have permissions to read that. // Otherwise, check if they can view the issue. - switch notification.notificationType { - case subCommandAssignee: - if !c.Settings.ShouldReceiveNotificationsForAssignee() { - continue - } - case subCommandMention: - if !c.Settings.ShouldReceiveNotificationsForMention() { - continue - } - case subCommandReporter: - if !c.Settings.ShouldReceiveNotificationsForReporter() { - continue - } - case subCommandWatching: - if !c.Settings.ShouldReceiveNotificationsForWatching() { - continue - } + if !c.Settings.ShouldReceiveNotificationsFor(notification.notificationType) { + continue } if _, ok := mapForNotification[mattermostUserID]; ok { continue From f6f0a2c372f8030d180e0b899450285be2bafab8 Mon Sep 17 00:00:00 2001 From: Nityanand Rai Date: Thu, 29 Sep 2022 12:02:57 +0530 Subject: [PATCH 09/17] [MI-2088]: Review fixes done 1. Changed the name of various variables 2. Used constants everywhere --- server/command_test.go | 10 +++++----- server/issue.go | 6 +++--- server/settings.go | 10 +++++----- server/user.go | 12 ++++++------ server/user_cloud.go | 10 +++++----- server/user_server.go | 10 +++++----- server/user_test.go | 20 ++++++++++---------- server/webhook.go | 2 +- 8 files changed, 40 insertions(+), 40 deletions(-) diff --git a/server/command_test.go b/server/command_test.go index 49f27811a..4f9f6c7f2 100644 --- a/server/command_test.go +++ b/server/command_test.go @@ -73,11 +73,11 @@ func getMockUserStoreKV() mockUserStoreKV { withNotifications := connection // copy withNotifications.Settings = &ConnectionSettings{ Notifications: trueValue, - RoleNotification: map[string]*bool{ - "assignee": &trueValue, - "mention": &trueValue, - "reporter": &trueValue, - "watching": &trueValue, + RolesForDMNotification: map[string]*bool{ + subCommandAssignee: &trueValue, + subCommandMention: &trueValue, + subCommandReporter: &trueValue, + subCommandWatching: &trueValue, }, } diff --git a/server/issue.go b/server/issue.go index fb6aaad48..306bfe86f 100644 --- a/server/issue.go +++ b/server/issue.go @@ -1094,7 +1094,7 @@ func (p *Plugin) applyReporterNotification(wh *webhook, instanceID types.ID, rep commentMessage := fmt.Sprintf("%s **commented** on %s:\n> %s", commentAuthor, jwhook.mdKeySummaryLink(), jwhook.Comment.Body) connection, err := p.GetUserSetting(wh, instanceID, reporter.Name, reporter.AccountID) - if err != nil || connection.Settings == nil || !connection.Settings.ShouldReceiveNotificationsFor("reporter") { + if err != nil || connection.Settings == nil || !connection.Settings.ShouldReceiveNotification(notificationTypeReporter) { return } @@ -1131,8 +1131,8 @@ func (p *Plugin) GetUserSetting(wh *webhook, instanceID types.ID, jiraAccountID, return connection, nil } -func (s *ConnectionSettings) ShouldReceiveNotificationsFor(role string) bool { - if val, ok := s.RoleNotification[role]; ok { +func (s *ConnectionSettings) ShouldReceiveNotification(role string) bool { + if val, ok := s.RolesForDMNotification[role]; ok { return *val } diff --git a/server/settings.go b/server/settings.go index f7e12e20a..62a3d870a 100644 --- a/server/settings.go +++ b/server/settings.go @@ -20,11 +20,11 @@ const ( subCommandWatching = "watching" ) -func (connection *Connection) sendNotificationsFor(role string, hasNotification bool) bool { - if role != "assignee" && role != "mention" && role != "reporter" && role != "watching" { +func (connection *Connection) sendNotification(role string, hasNotification bool) bool { + if role != subCommandAssignee && role != subCommandMention && role != subCommandReporter && role != subCommandWatching { return false } - connection.Settings.RoleNotification[role] = &hasNotification + connection.Settings.RolesForDMNotification[role] = &hasNotification return true } func (p *Plugin) settingsNotifications(header *model.CommandArgs, instanceID, mattermostUserID types.ID, connection *Connection, args []string) *model.CommandResponse { @@ -47,7 +47,7 @@ func (p *Plugin) settingsNotifications(header *model.CommandArgs, instanceID, ma if connection.Settings == nil { connection.Settings = &ConnectionSettings{} } - if !connection.sendNotificationsFor(args[1], value) { + if !connection.sendNotification(args[1], value) { return p.responsef(header, helpText) } @@ -62,7 +62,7 @@ func (p *Plugin) settingsNotifications(header *model.CommandArgs, instanceID, ma return p.responsef(header, errConnectToJira, err) } notifications := settingOff - if *updatedConnection.Settings.RoleNotification[args[1]] { + if *updatedConnection.Settings.RolesForDMNotification[args[1]] { notifications = settingOn } diff --git a/server/user.go b/server/user.go index 2ef7a106b..2bd4a89ae 100644 --- a/server/user.go +++ b/server/user.go @@ -43,8 +43,8 @@ func (c *Connection) JiraAccountID() types.ID { } type ConnectionSettings struct { - Notifications bool `json:"notifications"` - RoleNotification map[string]*bool + Notifications bool `json:"notifications"` + RolesForDMNotification map[string]*bool } func (s *ConnectionSettings) String() string { @@ -53,19 +53,19 @@ func (s *ConnectionSettings) String() string { reporterNotifications := "Notifications for reporter: off" watchingNotifications := "Notifications for watching: off" - if s != nil && s.ShouldReceiveNotificationsFor("assignee") { + if s != nil && s.ShouldReceiveNotification(subCommandAssignee) { assigneeNotifications = "Notifications for assignee: on" } - if s != nil && s.ShouldReceiveNotificationsFor("mention") { + if s != nil && s.ShouldReceiveNotification(subCommandMention) { mentionNotifications = "Notifications for mention: on" } - if s != nil && s.ShouldReceiveNotificationsFor("reporter") { + if s != nil && s.ShouldReceiveNotification(subCommandReporter) { reporterNotifications = "Notifications for reporter: on" } - if s != nil && s.ShouldReceiveNotificationsFor("watching") { + if s != nil && s.ShouldReceiveNotification(subCommandWatching) { watchingNotifications = "Notifications for watching: on" } diff --git a/server/user_cloud.go b/server/user_cloud.go index 00dcbcdba..686d4b9fb 100644 --- a/server/user_cloud.go +++ b/server/user_cloud.go @@ -104,11 +104,11 @@ func (p *Plugin) httpACUserInteractive(w http.ResponseWriter, r *http.Request, i // Set default settings the first time a user connects Settings: &ConnectionSettings{ Notifications: trueValue, - RoleNotification: map[string]*bool{ - "mention": &trueValue, - "assignee": &trueValue, - "reporter": &trueValue, - "watching": &trueValue, + RolesForDMNotification: map[string]*bool{ + subCommandMention: &trueValue, + subCommandAssignee: &trueValue, + subCommandReporter: &trueValue, + subCommandWatching: &trueValue, }, }, } diff --git a/server/user_server.go b/server/user_server.go index 7955720b2..046ecc33b 100644 --- a/server/user_server.go +++ b/server/user_server.go @@ -106,11 +106,11 @@ func (p *Plugin) httpOAuth1aComplete(w http.ResponseWriter, r *http.Request, ins // Set default settings the first time a user connects connection.Settings = &ConnectionSettings{ Notifications: trueValue, - RoleNotification: map[string]*bool{ - "mention": &trueValue, - "assignee": &trueValue, - "reporter": &trueValue, - "watching": &trueValue, + RolesForDMNotification: map[string]*bool{ + subCommandMention: &trueValue, + subCommandAssignee: &trueValue, + subCommandReporter: &trueValue, + subCommandWatching: &trueValue, }, } diff --git a/server/user_test.go b/server/user_test.go index 6c251664c..6bc48c83e 100644 --- a/server/user_test.go +++ b/server/user_test.go @@ -20,11 +20,11 @@ func TestUserSettings_String(t *testing.T) { "notifications on": { settings: ConnectionSettings{ Notifications: valueTrue, - RoleNotification: map[string]*bool{ - "assignee": nil, - "mention": nil, - "reporter": nil, - "watching": nil, + RolesForDMNotification: map[string]*bool{ + subCommandAssignee: nil, + subCommandMention: nil, + subCommandReporter: nil, + subCommandWatching: nil, }, }, expectedOutput: "\t- Notifications for assignee: on \n\t- Notifications for mention: on \n\t- Notifications for reporter: on \n\t- Notifications for watching: on", @@ -32,11 +32,11 @@ func TestUserSettings_String(t *testing.T) { "notifications off": { settings: ConnectionSettings{ Notifications: valueFalse, - RoleNotification: map[string]*bool{ - "assignee": nil, - "mention": nil, - "reporter": nil, - "watching": nil, + RolesForDMNotification: map[string]*bool{ + subCommandAssignee: nil, + subCommandMention: nil, + subCommandReporter: nil, + subCommandWatching: nil, }, }, expectedOutput: "\t- Notifications for assignee: off \n\t- Notifications for mention: off \n\t- Notifications for reporter: off \n\t- Notifications for watching: off", diff --git a/server/webhook.go b/server/webhook.go index b4e6b7e1c..a2c606c20 100644 --- a/server/webhook.go +++ b/server/webhook.go @@ -138,7 +138,7 @@ func (wh *webhook) PostNotifications(p *Plugin, instanceID types.ID) ([]*model.P } // If this is a comment-related webhook, we need to check if they have permissions to read that. // Otherwise, check if they can view the issue. - if !c.Settings.ShouldReceiveNotificationsFor(notification.notificationType) { + if !c.Settings.ShouldReceiveNotification(notification.notificationType) { continue } if _, ok := mapForNotification[mattermostUserID]; ok { From 5e3ff1d03633c26df28bdcbc942fc3c3a0be93bb Mon Sep 17 00:00:00 2001 From: Nityanand Rai Date: Thu, 6 Oct 2022 19:54:04 +0530 Subject: [PATCH 10/17] [MI-2088]: Review fixes done 1. Added Test cases 2. Used booleans instead of pointer in RolesForDMNotification --- server/command_test.go | 10 +- server/issue.go | 43 ++++--- server/issue_test.go | 250 +++++++++++++++++++++++++++++++++++++++++ server/settings.go | 4 +- server/user.go | 2 +- server/user_cloud.go | 10 +- server/user_server.go | 10 +- server/user_test.go | 20 ++-- 8 files changed, 304 insertions(+), 45 deletions(-) diff --git a/server/command_test.go b/server/command_test.go index 4f9f6c7f2..912881d69 100644 --- a/server/command_test.go +++ b/server/command_test.go @@ -73,11 +73,11 @@ func getMockUserStoreKV() mockUserStoreKV { withNotifications := connection // copy withNotifications.Settings = &ConnectionSettings{ Notifications: trueValue, - RolesForDMNotification: map[string]*bool{ - subCommandAssignee: &trueValue, - subCommandMention: &trueValue, - subCommandReporter: &trueValue, - subCommandWatching: &trueValue, + RolesForDMNotification: map[string]bool{ + subCommandAssignee: trueValue, + subCommandMention: trueValue, + subCommandReporter: trueValue, + subCommandWatching: trueValue, }, } diff --git a/server/issue.go b/server/issue.go index 306bfe86f..9f2764d0c 100644 --- a/server/issue.go +++ b/server/issue.go @@ -1133,13 +1133,37 @@ func (p *Plugin) GetUserSetting(wh *webhook, instanceID types.ID, jiraAccountID, func (s *ConnectionSettings) ShouldReceiveNotification(role string) bool { if val, ok := s.RolesForDMNotification[role]; ok { - return *val + return val } // Check old setting for backwards compatibility return s.Notifications } +func (p *Plugin) fetchConnectedUserFromAccount(account map[string]string, instance Instance) (Client, *Connection, error) { + var mattermostUserID types.ID + var err error + if account["AccountID"] != "" { + mattermostUserID, err = p.userStore.LoadMattermostUserID(instance.GetID(), account["AccountID"]) + } else { + mattermostUserID, err = p.userStore.LoadMattermostUserID(instance.GetID(), account["Name"]) + } + if err != nil { + return nil, nil, err + } + + connection, err := p.userStore.LoadConnection(instance.GetID(), mattermostUserID) + if err != nil { + return nil, nil, err + } + + client, err := instance.GetClient(connection) + if err != nil { + return nil, connection, err + } + return client, connection, nil +} + func (wh *webhook) fetchConnectedUser(p *Plugin, instanceID types.ID) (Client, *Connection, error) { var accountInformation []map[string]string @@ -1169,22 +1193,7 @@ func (wh *webhook) fetchConnectedUser(p *Plugin, instanceID types.ID) (Client, * return nil, nil, err } for _, account := range accountInformation { - var mattermostUserID types.ID - if account["AccountID"] != "" { - mattermostUserID, err = p.userStore.LoadMattermostUserID(instance.GetID(), account["AccountID"]) - } else { - mattermostUserID, err = p.userStore.LoadMattermostUserID(instance.GetID(), account["Name"]) - } - if err != nil { - continue - } - - connection, err := p.userStore.LoadConnection(instance.GetID(), mattermostUserID) - if err != nil { - continue - } - - client, err := instance.GetClient(connection) + client, connection, err := p.fetchConnectedUserFromAccount(account, instance) if err != nil { continue } diff --git a/server/issue_test.go b/server/issue_test.go index 0920912cd..0f7c1d694 100644 --- a/server/issue_test.go +++ b/server/issue_test.go @@ -17,6 +17,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/mattermost/mattermost-plugin-jira/server/utils/kvstore" + "github.com/mattermost/mattermost-plugin-jira/server/utils/types" ) const ( @@ -279,6 +280,255 @@ func TestRouteShareIssuePublicly(t *testing.T) { } } +func TestShouldReceiveNotification(t *testing.T) { + s := ConnectionSettings{} + falseValue := false + tests := map[string]struct { + role string + notification bool + }{ + subCommandAssignee: { + role: subCommandAssignee, + notification: falseValue, + }, + subCommandMention: { + role: subCommandMention, + notification: falseValue, + }, + subCommandReporter: { + role: subCommandReporter, + notification: falseValue, + }, + subCommandWatching: { + role: subCommandWatching, + notification: falseValue, + }, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + val := s.ShouldReceiveNotification( + tt.role, + ) + assert.Equal(t, tt.notification, val) + }) + } + +} + +func TestFetchConnectedUser(t *testing.T) { + tests := map[string]struct { + p *Plugin + instanceID types.ID + client Client + connection *Connection + wh webhook + expectedErr error + }{ + "No Field": { + p: &Plugin{ + instanceStore: mockInstanceStore{}, + userStore: mockUserStore{}, + }, + instanceID: testInstance1.InstanceID, + client: nil, + connection: nil, + wh: webhook{ + JiraWebhook: &JiraWebhook{ + Issue: jira.Issue{}, + }, + }, + expectedErr: nil, + }, + "Success": { + p: &Plugin{ + instanceStore: mockInstanceStore{}, + userStore: mockUserStore{}, + }, + instanceID: testInstance1.InstanceID, + client: testClient{}, + connection: &Connection{ + Settings: &ConnectionSettings{ + Notifications: true, + RolesForDMNotification: nil, + }, + }, + wh: webhook{ + JiraWebhook: &JiraWebhook{ + Issue: jira.Issue{ + Fields: &jira.IssueFields{ + Creator: &jira.User{}, + }, + }, + }, + }, + expectedErr: nil, + }, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + client, connection, error := tt.wh.fetchConnectedUser( + tt.p, + tt.instanceID, + ) + assert.Equal(t, tt.connection, connection) + assert.Equal(t, tt.client, client) + if tt.expectedErr != nil { + assert.Error(t, tt.expectedErr, error) + } + }) + } +} + +func TestApplyReporterNotification(t *testing.T) { + api := &plugintest.API{} + + api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + + api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) + p := Plugin{} + p.SetAPI(api) + p.instanceStore = p.getMockInstanceStoreKV(1) + p.userStore = getMockUserStoreKV() + + tests := map[string]struct { + wh *webhook + instanceID types.ID + reporter *jira.User + }{ + "No reporter": { + wh: &webhook{ + eventTypes: map[string]bool{ + createdCommentEvent: true, + }, + JiraWebhook: &JiraWebhook{ + Comment: jira.Comment{ + UpdateAuthor: jira.User{}, + }, + Issue: jira.Issue{ + Key: "TEST-10", + Fields: &jira.IssueFields{ + Type: jira.IssueType{ + Name: "Story", + }, + Summary: "", + }, + Self: "Abc", + }, + }, + notifications: []webhookUserNotification{}, + }, + instanceID: testInstance1.InstanceID, + reporter: nil, + }, + "Success": { + wh: &webhook{ + eventTypes: map[string]bool{ + createdCommentEvent: true, + }, + JiraWebhook: &JiraWebhook{ + Comment: jira.Comment{ + UpdateAuthor: jira.User{}, + }, + Issue: jira.Issue{ + Key: "TEST-10", + Fields: &jira.IssueFields{ + Type: jira.IssueType{ + Name: "Story", + }, + Summary: "", + }, + Self: "Abc", + }, + }, + notifications: []webhookUserNotification{}, + }, + instanceID: testInstance1.InstanceID, + reporter: &jira.User{}, + }, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + l1 := len(tt.wh.notifications) + p.applyReporterNotification( + tt.wh, + tt.instanceID, + tt.reporter, + ) + if tt.reporter == nil { + assert.Equal(t, len(tt.wh.notifications)-l1, 0) + } else { + assert.Equal(t, len(tt.wh.notifications)-l1, 1) + } + }) + } +} + +func TestGetUserSetting(t *testing.T) { + api := &plugintest.API{} + + api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + + api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) + p := Plugin{} + p.SetAPI(api) + p.instanceStore = p.getMockInstanceStoreKV(1) + p.userStore = getMockUserStoreKV() + trueValue := true + + tests := map[string]struct { + wh *webhook + instanceID types.ID + jiraAccountID string + jiraUsername string + connection *Connection + expectedErr error + }{ + "No instanceID": { + wh: &webhook{}, + instanceID: "", + jiraAccountID: "", + jiraUsername: "", + connection: nil, + expectedErr: errors.New("NO InstanceID was found"), + }, + "Success": { + wh: &webhook{}, + instanceID: testInstance1.InstanceID, + jiraAccountID: "", + jiraUsername: "", + connection: &Connection{ + User: jira.User{ + AccountID: "test", + }, + Settings: &ConnectionSettings{ + Notifications: trueValue, + RolesForDMNotification: (map[string]bool{ + subCommandAssignee: trueValue, + subCommandMention: trueValue, + subCommandReporter: trueValue, + subCommandWatching: trueValue, + }), + }, + }, + expectedErr: nil, + }, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + connection, error := p.GetUserSetting( + tt.wh, + tt.instanceID, + tt.jiraAccountID, + tt.jiraUsername, + ) + assert.Equal(t, tt.connection, connection) + if tt.expectedErr != nil { + assert.Error(t, tt.expectedErr, error) + } + }) + } +} + func TestRouteAttachCommentToIssue(t *testing.T) { api := &plugintest.API{} diff --git a/server/settings.go b/server/settings.go index 62a3d870a..01a39a2ff 100644 --- a/server/settings.go +++ b/server/settings.go @@ -24,7 +24,7 @@ func (connection *Connection) sendNotification(role string, hasNotification bool if role != subCommandAssignee && role != subCommandMention && role != subCommandReporter && role != subCommandWatching { return false } - connection.Settings.RolesForDMNotification[role] = &hasNotification + connection.Settings.RolesForDMNotification[role] = hasNotification return true } func (p *Plugin) settingsNotifications(header *model.CommandArgs, instanceID, mattermostUserID types.ID, connection *Connection, args []string) *model.CommandResponse { @@ -62,7 +62,7 @@ func (p *Plugin) settingsNotifications(header *model.CommandArgs, instanceID, ma return p.responsef(header, errConnectToJira, err) } notifications := settingOff - if *updatedConnection.Settings.RolesForDMNotification[args[1]] { + if updatedConnection.Settings.RolesForDMNotification[args[1]] { notifications = settingOn } diff --git a/server/user.go b/server/user.go index 2bd4a89ae..0d8b22671 100644 --- a/server/user.go +++ b/server/user.go @@ -44,7 +44,7 @@ func (c *Connection) JiraAccountID() types.ID { type ConnectionSettings struct { Notifications bool `json:"notifications"` - RolesForDMNotification map[string]*bool + RolesForDMNotification map[string]bool } func (s *ConnectionSettings) String() string { diff --git a/server/user_cloud.go b/server/user_cloud.go index 686d4b9fb..b6d211a0d 100644 --- a/server/user_cloud.go +++ b/server/user_cloud.go @@ -104,11 +104,11 @@ func (p *Plugin) httpACUserInteractive(w http.ResponseWriter, r *http.Request, i // Set default settings the first time a user connects Settings: &ConnectionSettings{ Notifications: trueValue, - RolesForDMNotification: map[string]*bool{ - subCommandMention: &trueValue, - subCommandAssignee: &trueValue, - subCommandReporter: &trueValue, - subCommandWatching: &trueValue, + RolesForDMNotification: map[string]bool{ + subCommandMention: trueValue, + subCommandAssignee: trueValue, + subCommandReporter: trueValue, + subCommandWatching: trueValue, }, }, } diff --git a/server/user_server.go b/server/user_server.go index 046ecc33b..cc4331c73 100644 --- a/server/user_server.go +++ b/server/user_server.go @@ -106,11 +106,11 @@ func (p *Plugin) httpOAuth1aComplete(w http.ResponseWriter, r *http.Request, ins // Set default settings the first time a user connects connection.Settings = &ConnectionSettings{ Notifications: trueValue, - RolesForDMNotification: map[string]*bool{ - subCommandMention: &trueValue, - subCommandAssignee: &trueValue, - subCommandReporter: &trueValue, - subCommandWatching: &trueValue, + RolesForDMNotification: map[string]bool{ + subCommandMention: trueValue, + subCommandAssignee: trueValue, + subCommandReporter: trueValue, + subCommandWatching: trueValue, }, } diff --git a/server/user_test.go b/server/user_test.go index 6bc48c83e..bff5c987d 100644 --- a/server/user_test.go +++ b/server/user_test.go @@ -20,11 +20,11 @@ func TestUserSettings_String(t *testing.T) { "notifications on": { settings: ConnectionSettings{ Notifications: valueTrue, - RolesForDMNotification: map[string]*bool{ - subCommandAssignee: nil, - subCommandMention: nil, - subCommandReporter: nil, - subCommandWatching: nil, + RolesForDMNotification: map[string]bool{ + subCommandAssignee: valueTrue, + subCommandMention: valueTrue, + subCommandReporter: valueTrue, + subCommandWatching: valueTrue, }, }, expectedOutput: "\t- Notifications for assignee: on \n\t- Notifications for mention: on \n\t- Notifications for reporter: on \n\t- Notifications for watching: on", @@ -32,11 +32,11 @@ func TestUserSettings_String(t *testing.T) { "notifications off": { settings: ConnectionSettings{ Notifications: valueFalse, - RolesForDMNotification: map[string]*bool{ - subCommandAssignee: nil, - subCommandMention: nil, - subCommandReporter: nil, - subCommandWatching: nil, + RolesForDMNotification: map[string]bool{ + subCommandAssignee: valueFalse, + subCommandMention: valueFalse, + subCommandReporter: valueFalse, + subCommandWatching: valueFalse, }, }, expectedOutput: "\t- Notifications for assignee: off \n\t- Notifications for mention: off \n\t- Notifications for reporter: off \n\t- Notifications for watching: off", From d93fa60d719a147e35dd7d977f03ca594e991edf Mon Sep 17 00:00:00 2001 From: Nityanand Rai Date: Mon, 10 Oct 2022 13:01:00 +0530 Subject: [PATCH 11/17] [MI-2088]: Review fixes done 1. Modified test cases --- server/issue_test.go | 152 +++++++++++++++++++++++-------------------- 1 file changed, 81 insertions(+), 71 deletions(-) diff --git a/server/issue_test.go b/server/issue_test.go index 0f7c1d694..506778b82 100644 --- a/server/issue_test.go +++ b/server/issue_test.go @@ -281,19 +281,24 @@ func TestRouteShareIssuePublicly(t *testing.T) { } func TestShouldReceiveNotification(t *testing.T) { - s := ConnectionSettings{} + cs := ConnectionSettings{} + cs.RolesForDMNotification = make(map[string]bool) + cs.RolesForDMNotification[subCommandAssignee] = true + cs.RolesForDMNotification[subCommandMention] = true falseValue := false + trueValue := true tests := map[string]struct { - role string - notification bool + role string + notification bool + RolesForDMNotification map[string]bool }{ subCommandAssignee: { role: subCommandAssignee, - notification: falseValue, + notification: trueValue, }, subCommandMention: { role: subCommandMention, - notification: falseValue, + notification: trueValue, }, subCommandReporter: { role: subCommandReporter, @@ -306,7 +311,7 @@ func TestShouldReceiveNotification(t *testing.T) { } for name, tt := range tests { t.Run(name, func(t *testing.T) { - val := s.ShouldReceiveNotification( + val := cs.ShouldReceiveNotification( tt.role, ) assert.Equal(t, tt.notification, val) @@ -316,19 +321,24 @@ func TestShouldReceiveNotification(t *testing.T) { } func TestFetchConnectedUser(t *testing.T) { + api := &plugintest.API{} + + api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + + api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) + p := &Plugin{} + p.SetAPI(api) + p.instanceStore = p.getMockInstanceStoreKV(1) + p.userStore = getMockUserStoreKV() + trueValue := true tests := map[string]struct { - p *Plugin instanceID types.ID client Client connection *Connection wh webhook expectedErr error }{ - "No Field": { - p: &Plugin{ - instanceStore: mockInstanceStore{}, - userStore: mockUserStore{}, - }, + "Issue Feild not found": { instanceID: testInstance1.InstanceID, client: nil, connection: nil, @@ -339,17 +349,36 @@ func TestFetchConnectedUser(t *testing.T) { }, expectedErr: nil, }, - "Success": { - p: &Plugin{ - instanceStore: mockInstanceStore{}, - userStore: mockUserStore{}, + "Unable to load instance": { + instanceID: "instanceID", + client: nil, + connection: nil, + wh: webhook{ + JiraWebhook: &JiraWebhook{ + Issue: jira.Issue{ + Fields: &jira.IssueFields{ + Creator: &jira.User{}, + }, + }, + }, }, + expectedErr: errors.New("instance " + fmt.Sprintf("\"%s\"", "instanceID") + " not found"), + }, + "Success": { instanceID: testInstance1.InstanceID, client: testClient{}, connection: &Connection{ Settings: &ConnectionSettings{ - Notifications: true, - RolesForDMNotification: nil, + Notifications: trueValue, + RolesForDMNotification: map[string]bool{ + subCommandAssignee: trueValue, + subCommandMention: trueValue, + subCommandReporter: trueValue, + subCommandWatching: trueValue, + }, + }, + User: jira.User{ + AccountID: "test", }, }, wh: webhook{ @@ -367,7 +396,7 @@ func TestFetchConnectedUser(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { client, connection, error := tt.wh.fetchConnectedUser( - tt.p, + p, tt.instanceID, ) assert.Equal(t, tt.connection, connection) @@ -389,75 +418,56 @@ func TestApplyReporterNotification(t *testing.T) { p.SetAPI(api) p.instanceStore = p.getMockInstanceStoreKV(1) p.userStore = getMockUserStoreKV() - + wh := &webhook{ + eventTypes: map[string]bool{ + createdCommentEvent: true, + }, + JiraWebhook: &JiraWebhook{ + Comment: jira.Comment{ + UpdateAuthor: jira.User{}, + }, + Issue: jira.Issue{ + Key: "TEST-10", + Fields: &jira.IssueFields{ + Type: jira.IssueType{ + Name: "Story", + }, + Summary: "", + }, + Self: "Abc", + }, + }, + notifications: []webhookUserNotification{}, + } tests := map[string]struct { - wh *webhook instanceID types.ID reporter *jira.User }{ + "Unable to load instance": { + instanceID: "instanceID", + reporter: &jira.User{}, + }, "No reporter": { - wh: &webhook{ - eventTypes: map[string]bool{ - createdCommentEvent: true, - }, - JiraWebhook: &JiraWebhook{ - Comment: jira.Comment{ - UpdateAuthor: jira.User{}, - }, - Issue: jira.Issue{ - Key: "TEST-10", - Fields: &jira.IssueFields{ - Type: jira.IssueType{ - Name: "Story", - }, - Summary: "", - }, - Self: "Abc", - }, - }, - notifications: []webhookUserNotification{}, - }, instanceID: testInstance1.InstanceID, reporter: nil, }, "Success": { - wh: &webhook{ - eventTypes: map[string]bool{ - createdCommentEvent: true, - }, - JiraWebhook: &JiraWebhook{ - Comment: jira.Comment{ - UpdateAuthor: jira.User{}, - }, - Issue: jira.Issue{ - Key: "TEST-10", - Fields: &jira.IssueFields{ - Type: jira.IssueType{ - Name: "Story", - }, - Summary: "", - }, - Self: "Abc", - }, - }, - notifications: []webhookUserNotification{}, - }, instanceID: testInstance1.InstanceID, reporter: &jira.User{}, }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { - l1 := len(tt.wh.notifications) + totalNotifications := len(wh.notifications) p.applyReporterNotification( - tt.wh, + wh, tt.instanceID, tt.reporter, ) - if tt.reporter == nil { - assert.Equal(t, len(tt.wh.notifications)-l1, 0) + if tt.reporter == nil || tt.instanceID == "instanceID" { + assert.Equal(t, len(wh.notifications)-totalNotifications, 0) } else { - assert.Equal(t, len(tt.wh.notifications)-l1, 1) + assert.Equal(t, len(wh.notifications)-totalNotifications, 1) } }) } @@ -483,13 +493,13 @@ func TestGetUserSetting(t *testing.T) { connection *Connection expectedErr error }{ - "No instanceID": { + "Unable to load instance": { wh: &webhook{}, - instanceID: "", + instanceID: "instanceID", jiraAccountID: "", jiraUsername: "", connection: nil, - expectedErr: errors.New("NO InstanceID was found"), + expectedErr: errors.New("instance " + fmt.Sprintf("\"%s\"", "instanceID") + " not found"), }, "Success": { wh: &webhook{}, From 9cd26d504526150a67b7afc0c3bb53f193406474 Mon Sep 17 00:00:00 2001 From: Nityanand Rai Date: Mon, 10 Oct 2022 16:19:24 +0530 Subject: [PATCH 12/17] [MI-2088]: Review fixes done 1. Self reviewed --- server/issue_test.go | 44 +++++++++++++++++--------------------------- 1 file changed, 17 insertions(+), 27 deletions(-) diff --git a/server/issue_test.go b/server/issue_test.go index 506778b82..2957b88b0 100644 --- a/server/issue_test.go +++ b/server/issue_test.go @@ -285,8 +285,6 @@ func TestShouldReceiveNotification(t *testing.T) { cs.RolesForDMNotification = make(map[string]bool) cs.RolesForDMNotification[subCommandAssignee] = true cs.RolesForDMNotification[subCommandMention] = true - falseValue := false - trueValue := true tests := map[string]struct { role string notification bool @@ -294,26 +292,24 @@ func TestShouldReceiveNotification(t *testing.T) { }{ subCommandAssignee: { role: subCommandAssignee, - notification: trueValue, + notification: true, }, subCommandMention: { role: subCommandMention, - notification: trueValue, + notification: true, }, subCommandReporter: { role: subCommandReporter, - notification: falseValue, + notification: false, }, subCommandWatching: { role: subCommandWatching, - notification: falseValue, + notification: false, }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { - val := cs.ShouldReceiveNotification( - tt.role, - ) + val := cs.ShouldReceiveNotification(tt.role) assert.Equal(t, tt.notification, val) }) } @@ -330,7 +326,6 @@ func TestFetchConnectedUser(t *testing.T) { p.SetAPI(api) p.instanceStore = p.getMockInstanceStoreKV(1) p.userStore = getMockUserStoreKV() - trueValue := true tests := map[string]struct { instanceID types.ID client Client @@ -369,12 +364,12 @@ func TestFetchConnectedUser(t *testing.T) { client: testClient{}, connection: &Connection{ Settings: &ConnectionSettings{ - Notifications: trueValue, + Notifications: true, RolesForDMNotification: map[string]bool{ - subCommandAssignee: trueValue, - subCommandMention: trueValue, - subCommandReporter: trueValue, - subCommandWatching: trueValue, + subCommandAssignee: true, + subCommandMention: true, + subCommandReporter: true, + subCommandWatching: true, }, }, User: jira.User{ @@ -419,9 +414,7 @@ func TestApplyReporterNotification(t *testing.T) { p.instanceStore = p.getMockInstanceStoreKV(1) p.userStore = getMockUserStoreKV() wh := &webhook{ - eventTypes: map[string]bool{ - createdCommentEvent: true, - }, + eventTypes: map[string]bool{createdCommentEvent: true}, JiraWebhook: &JiraWebhook{ Comment: jira.Comment{ UpdateAuthor: jira.User{}, @@ -483,7 +476,6 @@ func TestGetUserSetting(t *testing.T) { p.SetAPI(api) p.instanceStore = p.getMockInstanceStoreKV(1) p.userStore = getMockUserStoreKV() - trueValue := true tests := map[string]struct { wh *webhook @@ -507,16 +499,14 @@ func TestGetUserSetting(t *testing.T) { jiraAccountID: "", jiraUsername: "", connection: &Connection{ - User: jira.User{ - AccountID: "test", - }, + User: jira.User{AccountID: "test"}, Settings: &ConnectionSettings{ - Notifications: trueValue, + Notifications: true, RolesForDMNotification: (map[string]bool{ - subCommandAssignee: trueValue, - subCommandMention: trueValue, - subCommandReporter: trueValue, - subCommandWatching: trueValue, + subCommandAssignee: true, + subCommandMention: true, + subCommandReporter: true, + subCommandWatching: true, }), }, }, From 2acc22c322f577d30c668320cf9f07a60caf8204 Mon Sep 17 00:00:00 2001 From: Nityanand Rai Date: Tue, 11 Oct 2022 13:32:31 +0530 Subject: [PATCH 13/17] [MI-2088]: Review fixes done 1. Changed the formatting 2. Changed the name of few variables 3. Improved code quality --- server/command_test.go | 13 +++-- server/issue_test.go | 118 ++++++++++++++++++----------------------- server/user_cloud.go | 11 ++-- server/user_server.go | 11 ++-- server/user_test.go | 22 ++++---- 5 files changed, 79 insertions(+), 96 deletions(-) diff --git a/server/command_test.go b/server/command_test.go index 912881d69..7a495b3c5 100644 --- a/server/command_test.go +++ b/server/command_test.go @@ -65,19 +65,18 @@ func getMockUserStoreKV() mockUserStoreKV { connection := Connection{ User: jira.User{ - AccountID: "test", + AccountID: "test-AccountID", }, } - trueValue := true withNotifications := connection // copy withNotifications.Settings = &ConnectionSettings{ - Notifications: trueValue, + Notifications: true, RolesForDMNotification: map[string]bool{ - subCommandAssignee: trueValue, - subCommandMention: trueValue, - subCommandReporter: trueValue, - subCommandWatching: trueValue, + subCommandAssignee: true, + subCommandMention: true, + subCommandReporter: true, + subCommandWatching: true, }, } diff --git a/server/issue_test.go b/server/issue_test.go index 2957b88b0..86e86d185 100644 --- a/server/issue_test.go +++ b/server/issue_test.go @@ -333,32 +333,6 @@ func TestFetchConnectedUser(t *testing.T) { wh webhook expectedErr error }{ - "Issue Feild not found": { - instanceID: testInstance1.InstanceID, - client: nil, - connection: nil, - wh: webhook{ - JiraWebhook: &JiraWebhook{ - Issue: jira.Issue{}, - }, - }, - expectedErr: nil, - }, - "Unable to load instance": { - instanceID: "instanceID", - client: nil, - connection: nil, - wh: webhook{ - JiraWebhook: &JiraWebhook{ - Issue: jira.Issue{ - Fields: &jira.IssueFields{ - Creator: &jira.User{}, - }, - }, - }, - }, - expectedErr: errors.New("instance " + fmt.Sprintf("\"%s\"", "instanceID") + " not found"), - }, "Success": { instanceID: testInstance1.InstanceID, client: testClient{}, @@ -373,7 +347,7 @@ func TestFetchConnectedUser(t *testing.T) { }, }, User: jira.User{ - AccountID: "test", + AccountID: "test-AccountID", }, }, wh: webhook{ @@ -387,13 +361,36 @@ func TestFetchConnectedUser(t *testing.T) { }, expectedErr: nil, }, + "Issue Field not found": { + instanceID: testInstance1.InstanceID, + client: nil, + connection: nil, + wh: webhook{ + JiraWebhook: &JiraWebhook{ + Issue: jira.Issue{}, + }, + }, + expectedErr: nil, + }, + "Unable to load instance": { + instanceID: "test-instanceID", + client: nil, + connection: nil, + wh: webhook{ + JiraWebhook: &JiraWebhook{ + Issue: jira.Issue{ + Fields: &jira.IssueFields{ + Creator: &jira.User{}, + }, + }, + }, + }, + expectedErr: errors.New("instance " + fmt.Sprintf("\"%s\"", "instanceID") + " not found"), + }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { - client, connection, error := tt.wh.fetchConnectedUser( - p, - tt.instanceID, - ) + client, connection, error := tt.wh.fetchConnectedUser(p, tt.instanceID) assert.Equal(t, tt.connection, connection) assert.Equal(t, tt.client, client) if tt.expectedErr != nil { @@ -436,18 +433,18 @@ func TestApplyReporterNotification(t *testing.T) { instanceID types.ID reporter *jira.User }{ + "Success": { + instanceID: testInstance1.InstanceID, + reporter: &jira.User{}, + }, "Unable to load instance": { - instanceID: "instanceID", + instanceID: "test-instanceID", reporter: &jira.User{}, }, - "No reporter": { + "Reporter is nil": { instanceID: testInstance1.InstanceID, reporter: nil, }, - "Success": { - instanceID: testInstance1.InstanceID, - reporter: &jira.User{}, - }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { @@ -457,10 +454,10 @@ func TestApplyReporterNotification(t *testing.T) { tt.instanceID, tt.reporter, ) - if tt.reporter == nil || tt.instanceID == "instanceID" { - assert.Equal(t, len(wh.notifications)-totalNotifications, 0) + if tt.reporter == nil || tt.instanceID == "test-instanceID" { + assert.Equal(t, len(wh.notifications), totalNotifications) } else { - assert.Equal(t, len(wh.notifications)-totalNotifications, 1) + assert.Equal(t, len(wh.notifications), 1+totalNotifications) } }) } @@ -474,32 +471,22 @@ func TestGetUserSetting(t *testing.T) { api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) p := Plugin{} p.SetAPI(api) + jiraAccountID := "test-jiraAccountID" + jiraUsername := "test-jiraUsername" p.instanceStore = p.getMockInstanceStoreKV(1) p.userStore = getMockUserStoreKV() tests := map[string]struct { - wh *webhook - instanceID types.ID - jiraAccountID string - jiraUsername string - connection *Connection - expectedErr error + wh *webhook + instanceID types.ID + connection *Connection + expectedErr error }{ - "Unable to load instance": { - wh: &webhook{}, - instanceID: "instanceID", - jiraAccountID: "", - jiraUsername: "", - connection: nil, - expectedErr: errors.New("instance " + fmt.Sprintf("\"%s\"", "instanceID") + " not found"), - }, "Success": { - wh: &webhook{}, - instanceID: testInstance1.InstanceID, - jiraAccountID: "", - jiraUsername: "", + wh: &webhook{}, + instanceID: testInstance1.InstanceID, connection: &Connection{ - User: jira.User{AccountID: "test"}, + User: jira.User{AccountID: "test-AccountID"}, Settings: &ConnectionSettings{ Notifications: true, RolesForDMNotification: (map[string]bool{ @@ -512,15 +499,16 @@ func TestGetUserSetting(t *testing.T) { }, expectedErr: nil, }, + "Unable to load instance": { + wh: &webhook{}, + instanceID: "instanceID", + connection: nil, + expectedErr: errors.New("instance " + fmt.Sprintf("\"%s\"", "instanceID") + " not found"), + }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { - connection, error := p.GetUserSetting( - tt.wh, - tt.instanceID, - tt.jiraAccountID, - tt.jiraUsername, - ) + connection, error := p.GetUserSetting(tt.wh, tt.instanceID, jiraAccountID, jiraUsername) assert.Equal(t, tt.connection, connection) if tt.expectedErr != nil { assert.Error(t, tt.expectedErr, error) diff --git a/server/user_cloud.go b/server/user_cloud.go index b6d211a0d..a923c6e5f 100644 --- a/server/user_cloud.go +++ b/server/user_cloud.go @@ -92,7 +92,6 @@ func (p *Plugin) httpACUserInteractive(w http.ResponseWriter, r *http.Request, i } mmToken := r.FormValue(argMMToken) - trueValue := true connection := &Connection{ PluginVersion: manifest.Version, User: jira.User{ @@ -103,12 +102,12 @@ func (p *Plugin) httpACUserInteractive(w http.ResponseWriter, r *http.Request, i }, // Set default settings the first time a user connects Settings: &ConnectionSettings{ - Notifications: trueValue, + Notifications: true, RolesForDMNotification: map[string]bool{ - subCommandMention: trueValue, - subCommandAssignee: trueValue, - subCommandReporter: trueValue, - subCommandWatching: trueValue, + subCommandMention: true, + subCommandAssignee: true, + subCommandReporter: true, + subCommandWatching: true, }, }, } diff --git a/server/user_server.go b/server/user_server.go index cc4331c73..24c71914c 100644 --- a/server/user_server.go +++ b/server/user_server.go @@ -102,15 +102,14 @@ func (p *Plugin) httpOAuth1aComplete(w http.ResponseWriter, r *http.Request, ins return http.StatusInternalServerError, err } connection.User = *juser - trueValue := true // Set default settings the first time a user connects connection.Settings = &ConnectionSettings{ - Notifications: trueValue, + Notifications: true, RolesForDMNotification: map[string]bool{ - subCommandMention: trueValue, - subCommandAssignee: trueValue, - subCommandReporter: trueValue, - subCommandWatching: trueValue, + subCommandMention: true, + subCommandAssignee: true, + subCommandReporter: true, + subCommandWatching: true, }, } diff --git a/server/user_test.go b/server/user_test.go index bff5c987d..bd3f58993 100644 --- a/server/user_test.go +++ b/server/user_test.go @@ -11,32 +11,30 @@ import ( ) func TestUserSettings_String(t *testing.T) { - valueTrue := true - valueFalse := false tests := map[string]struct { settings ConnectionSettings expectedOutput string }{ "notifications on": { settings: ConnectionSettings{ - Notifications: valueTrue, + Notifications: true, RolesForDMNotification: map[string]bool{ - subCommandAssignee: valueTrue, - subCommandMention: valueTrue, - subCommandReporter: valueTrue, - subCommandWatching: valueTrue, + subCommandAssignee: true, + subCommandMention: true, + subCommandReporter: true, + subCommandWatching: true, }, }, expectedOutput: "\t- Notifications for assignee: on \n\t- Notifications for mention: on \n\t- Notifications for reporter: on \n\t- Notifications for watching: on", }, "notifications off": { settings: ConnectionSettings{ - Notifications: valueFalse, + Notifications: false, RolesForDMNotification: map[string]bool{ - subCommandAssignee: valueFalse, - subCommandMention: valueFalse, - subCommandReporter: valueFalse, - subCommandWatching: valueFalse, + subCommandAssignee: false, + subCommandMention: false, + subCommandReporter: false, + subCommandWatching: false, }, }, expectedOutput: "\t- Notifications for assignee: off \n\t- Notifications for mention: off \n\t- Notifications for reporter: off \n\t- Notifications for watching: off", From 2df6112432b22911e1457bb214611c3fcce84254 Mon Sep 17 00:00:00 2001 From: Nityanand Rai Date: Wed, 12 Oct 2022 11:22:48 +0530 Subject: [PATCH 14/17] [MI-2088]: Fixes CI errors --- server/kv.go | 24 ++++++++++++------------ server/settings.go | 3 +++ server/user.go | 8 ++++---- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/server/kv.go b/server/kv.go index b26f9d3a8..70aa77e61 100644 --- a/server/kv.go +++ b/server/kv.go @@ -568,13 +568,13 @@ func UpdateInstances(store InstanceStore, updatef func(instances *Instances) err // MigrateV2Instances migrates instance record(s) from the V2 data model. // -// - v2keyKnownJiraInstances ("known_jira_instances") was stored as a -// map[string]string (InstanceID->Type), needs to be stored as Instances. -// https://github.com/mattermost/mattermost-plugin-jira/blob/885efe8eb70c92bcea64d1ced6e67710eda77b6e/server/kv.go#L375 -// - v2keyCurrentJIRAInstance ("current_jira_instance") stored an Instance; will -// be used to set the default instance. -// - The instances themselves should be forward-compatible, including -// CurrentInstance. +// - v2keyKnownJiraInstances ("known_jira_instances") was stored as a +// map[string]string (InstanceID->Type), needs to be stored as Instances. +// https://github.com/mattermost/mattermost-plugin-jira/blob/885efe8eb70c92bcea64d1ced6e67710eda77b6e/server/kv.go#L375 +// - v2keyCurrentJIRAInstance ("current_jira_instance") stored an Instance; will +// be used to set the default instance. +// - The instances themselves should be forward-compatible, including +// CurrentInstance. func MigrateV2Instances(p *Plugin) (*Instances, error) { // Check if V3 instances exist and return them if found instances, err := p.instanceStore.LoadInstances() @@ -646,7 +646,7 @@ func MigrateV2Instances(p *Plugin) (*Instances, error) { return instances, nil } -// MigrateV3ToV2 performs necessary migrations when reverting from V3 to V2 +// MigrateV3ToV2 performs necessary migrations when reverting from V3 to V2 func MigrateV3ToV2(p *Plugin) string { // migrate V3 instances to v2 v2Instances, msg := MigrateV3InstancesToV2(p) @@ -675,10 +675,10 @@ func MigrateV3ToV2(p *Plugin) string { // MigrateV3InstancesToV2 migrates instance record(s) from the V3 data model. // -// - v3 instances need to be stored as v2keyKnownJiraInstances -// (known_jira_instances) map[string]string (InstanceID->Type), -// - v2keyCurrentJIRAInstance ("current_jira_instance") stored an Instance; will -// be used to set the default instance. +// - v3 instances need to be stored as v2keyKnownJiraInstances +// (known_jira_instances) map[string]string (InstanceID->Type), +// - v2keyCurrentJIRAInstance ("current_jira_instance") stored an Instance; will +// be used to set the default instance. func MigrateV3InstancesToV2(p *Plugin) (JiraV2Instances, string) { v3instances, err := p.instanceStore.LoadInstances() if err != nil { diff --git a/server/settings.go b/server/settings.go index 01a39a2ff..7b3a54349 100644 --- a/server/settings.go +++ b/server/settings.go @@ -24,6 +24,9 @@ func (connection *Connection) sendNotification(role string, hasNotification bool if role != subCommandAssignee && role != subCommandMention && role != subCommandReporter && role != subCommandWatching { return false } + if connection.Settings.RolesForDMNotification == nil { + connection.Settings.RolesForDMNotification = make(map[string]bool) + } connection.Settings.RolesForDMNotification[role] = hasNotification return true } diff --git a/server/user.go b/server/user.go index 0d8b22671..67d5456b1 100644 --- a/server/user.go +++ b/server/user.go @@ -34,12 +34,12 @@ type Connection struct { DefaultProjectKey string `json:"default_project_key,omitempty"` } -func (c *Connection) JiraAccountID() types.ID { - if c.AccountID != "" { - return types.ID(c.AccountID) +func (connection *Connection) JiraAccountID() types.ID { + if connection.AccountID != "" { + return types.ID(connection.AccountID) } - return types.ID(c.Name) + return types.ID(connection.Name) } type ConnectionSettings struct { From 03e76ba91366003f26617aa69d98fce7f9cce20a Mon Sep 17 00:00:00 2001 From: Nityanand Rai Date: Wed, 12 Oct 2022 13:08:38 +0530 Subject: [PATCH 15/17] [MI-2088]: Review fixes done 1. Improved code quality --- server/issue_test.go | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/server/issue_test.go b/server/issue_test.go index 86e86d185..c6c2504b6 100644 --- a/server/issue_test.go +++ b/server/issue_test.go @@ -286,9 +286,8 @@ func TestShouldReceiveNotification(t *testing.T) { cs.RolesForDMNotification[subCommandAssignee] = true cs.RolesForDMNotification[subCommandMention] = true tests := map[string]struct { - role string - notification bool - RolesForDMNotification map[string]bool + role string + notification bool }{ subCommandAssignee: { role: subCommandAssignee, @@ -417,14 +416,14 @@ func TestApplyReporterNotification(t *testing.T) { UpdateAuthor: jira.User{}, }, Issue: jira.Issue{ - Key: "TEST-10", + Key: "test-key", Fields: &jira.IssueFields{ Type: jira.IssueType{ Name: "Story", }, Summary: "", }, - Self: "Abc", + Self: "test-self", }, }, notifications: []webhookUserNotification{}, @@ -449,11 +448,7 @@ func TestApplyReporterNotification(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { totalNotifications := len(wh.notifications) - p.applyReporterNotification( - wh, - tt.instanceID, - tt.reporter, - ) + p.applyReporterNotification(wh, tt.instanceID, tt.reporter) if tt.reporter == nil || tt.instanceID == "test-instanceID" { assert.Equal(t, len(wh.notifications), totalNotifications) } else { From 50637bd234d15ab2df6a4eea7493f1f4225017d4 Mon Sep 17 00:00:00 2001 From: Nityanand Rai Date: Thu, 13 Oct 2022 12:18:45 +0530 Subject: [PATCH 16/17] [MI-2088]: Review fixes done 1. Improved code quality --- server/issue.go | 1 + server/issue_test.go | 89 +++++++++++++++++--------------------------- 2 files changed, 36 insertions(+), 54 deletions(-) diff --git a/server/issue.go b/server/issue.go index 9f2764d0c..df518b402 100644 --- a/server/issue.go +++ b/server/issue.go @@ -1161,6 +1161,7 @@ func (p *Plugin) fetchConnectedUserFromAccount(account map[string]string, instan if err != nil { return nil, connection, err } + return client, connection, nil } diff --git a/server/issue_test.go b/server/issue_test.go index c6c2504b6..7749a6a24 100644 --- a/server/issue_test.go +++ b/server/issue_test.go @@ -84,6 +84,16 @@ func (client testClient) AddComment(issueKey string, comment *jira.Comment) (*ji return nil, nil } +func setupTestPlugin(api *plugintest.API) *Plugin { + api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() + + api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) + p := &Plugin{} + p.SetAPI(api) + p.instanceStore = p.getMockInstanceStoreKV(1) + p.userStore = getMockUserStoreKV() + return p +} func TestTransitionJiraIssue(t *testing.T) { api := &plugintest.API{} @@ -93,7 +103,7 @@ func TestTransitionJiraIssue(t *testing.T) { p.userStore = getMockUserStoreKV() p.instanceStore = p.getMockInstanceStoreKV(1) - tests := map[string]struct { + for name, tt := range map[string]struct { issueKey string toState string expectedMsg string @@ -129,9 +139,7 @@ func TestTransitionJiraIssue(t *testing.T) { expectedMsg: fmt.Sprintf("[%s](%s/browse/%s) transitioned to `In Progress`", existingIssueKey, mockInstance1URL, existingIssueKey), expectedErr: nil, }, - } - - for name, tt := range tests { + } { t.Run(name, func(t *testing.T) { actual, err := p.TransitionIssue(&InTransitionIssue{ InstanceID: testInstance1.InstanceID, @@ -150,7 +158,7 @@ func TestTransitionJiraIssue(t *testing.T) { func TestRouteIssueTransition(t *testing.T) { api := &plugintest.API{} - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) @@ -161,7 +169,7 @@ func TestRouteIssueTransition(t *testing.T) { p.userStore = getMockUserStoreKV() - tests := map[string]struct { + for name, tt := range map[string]struct { bb []byte request *model.PostActionIntegrationRequest expectedCode int @@ -189,8 +197,7 @@ func TestRouteIssueTransition(t *testing.T) { }, expectedCode: http.StatusInternalServerError, }, - } - for name, tt := range tests { + } { t.Run(name, func(t *testing.T) { bb, err := json.Marshal(tt.request) assert.Nil(t, err) @@ -208,7 +215,7 @@ func TestRouteShareIssuePublicly(t *testing.T) { api := &plugintest.API{} p := Plugin{} api.On("SendEphemeralPost", mock.Anything, mock.Anything).Return(nil) - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) api.On("CreatePost", mock.AnythingOfType("*model.Post")).Return(&model.Post{}, nil) api.On("DeleteEphemeralPost", validUserID, "").Return() @@ -216,7 +223,7 @@ func TestRouteShareIssuePublicly(t *testing.T) { p.instanceStore = p.getMockInstanceStoreKV(1) p.userStore = getMockUserStoreKV() - tests := map[string]struct { + for name, tt := range map[string]struct { bb []byte request *model.PostActionIntegrationRequest expectedCode int @@ -266,8 +273,7 @@ func TestRouteShareIssuePublicly(t *testing.T) { }, expectedCode: http.StatusOK, }, - } - for name, tt := range tests { + } { t.Run(name, func(t *testing.T) { bb, err := json.Marshal(tt.request) assert.Nil(t, err) @@ -285,7 +291,7 @@ func TestShouldReceiveNotification(t *testing.T) { cs.RolesForDMNotification = make(map[string]bool) cs.RolesForDMNotification[subCommandAssignee] = true cs.RolesForDMNotification[subCommandMention] = true - tests := map[string]struct { + for name, tt := range map[string]struct { role string notification bool }{ @@ -305,27 +311,18 @@ func TestShouldReceiveNotification(t *testing.T) { role: subCommandWatching, notification: false, }, - } - for name, tt := range tests { + } { t.Run(name, func(t *testing.T) { val := cs.ShouldReceiveNotification(tt.role) assert.Equal(t, tt.notification, val) }) } - } func TestFetchConnectedUser(t *testing.T) { - api := &plugintest.API{} + p := setupTestPlugin(&plugintest.API{}) - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) - - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) - p := &Plugin{} - p.SetAPI(api) - p.instanceStore = p.getMockInstanceStoreKV(1) - p.userStore = getMockUserStoreKV() - tests := map[string]struct { + for name, tt := range map[string]struct { instanceID types.ID client Client connection *Connection @@ -384,10 +381,9 @@ func TestFetchConnectedUser(t *testing.T) { }, }, }, - expectedErr: errors.New("instance " + fmt.Sprintf("\"%s\"", "instanceID") + " not found"), + expectedErr: errors.New(fmt.Sprintf("instance %s not found", fmt.Sprintf("%q", "test-instanceID"))), }, - } - for name, tt := range tests { + } { t.Run(name, func(t *testing.T) { client, connection, error := tt.wh.fetchConnectedUser(p, tt.instanceID) assert.Equal(t, tt.connection, connection) @@ -400,15 +396,8 @@ func TestFetchConnectedUser(t *testing.T) { } func TestApplyReporterNotification(t *testing.T) { - api := &plugintest.API{} - - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + p := setupTestPlugin(&plugintest.API{}) - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) - p := Plugin{} - p.SetAPI(api) - p.instanceStore = p.getMockInstanceStoreKV(1) - p.userStore = getMockUserStoreKV() wh := &webhook{ eventTypes: map[string]bool{createdCommentEvent: true}, JiraWebhook: &JiraWebhook{ @@ -428,7 +417,7 @@ func TestApplyReporterNotification(t *testing.T) { }, notifications: []webhookUserNotification{}, } - tests := map[string]struct { + for name, tt := range map[string]struct { instanceID types.ID reporter *jira.User }{ @@ -444,8 +433,7 @@ func TestApplyReporterNotification(t *testing.T) { instanceID: testInstance1.InstanceID, reporter: nil, }, - } - for name, tt := range tests { + } { t.Run(name, func(t *testing.T) { totalNotifications := len(wh.notifications) p.applyReporterNotification(wh, tt.instanceID, tt.reporter) @@ -456,22 +444,16 @@ func TestApplyReporterNotification(t *testing.T) { } }) } + } func TestGetUserSetting(t *testing.T) { - api := &plugintest.API{} - - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + p := setupTestPlugin(&plugintest.API{}) - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) - p := Plugin{} - p.SetAPI(api) jiraAccountID := "test-jiraAccountID" jiraUsername := "test-jiraUsername" - p.instanceStore = p.getMockInstanceStoreKV(1) - p.userStore = getMockUserStoreKV() - tests := map[string]struct { + for name, tt := range map[string]struct { wh *webhook instanceID types.ID connection *Connection @@ -500,8 +482,7 @@ func TestGetUserSetting(t *testing.T) { connection: nil, expectedErr: errors.New("instance " + fmt.Sprintf("\"%s\"", "instanceID") + " not found"), }, - } - for name, tt := range tests { + } { t.Run(name, func(t *testing.T) { connection, error := p.GetUserSetting(tt.wh, tt.instanceID, jiraAccountID, jiraUsername) assert.Equal(t, tt.connection, connection) @@ -515,7 +496,7 @@ func TestGetUserSetting(t *testing.T) { func TestRouteAttachCommentToIssue(t *testing.T) { api := &plugintest.API{} - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) @@ -539,7 +520,7 @@ func TestRouteAttachCommentToIssue(t *testing.T) { IssueKey string `json:"issueKey"` } - tests := map[string]struct { + for name, tt := range map[string]struct { method string header string request *requestStruct @@ -614,8 +595,7 @@ func TestRouteAttachCommentToIssue(t *testing.T) { }, expectedCode: http.StatusOK, }, - } - for name, tt := range tests { + } { t.Run(name, func(t *testing.T) { p := Plugin{} p.SetAPI(api) @@ -636,4 +616,5 @@ func TestRouteAttachCommentToIssue(t *testing.T) { assert.Equal(t, tt.expectedCode, w.Result().StatusCode, "no request data") }) } + } From 842c34e6f919fe6eecd10d7be4fdb0da573d022c Mon Sep 17 00:00:00 2001 From: Nityanand Rai Date: Thu, 13 Oct 2022 15:02:51 +0530 Subject: [PATCH 17/17] [MI-2088]: Review fixes done 1. Improved code quality --- server/command_test.go | 2 +- server/http_test.go | 24 ++++++++++---------- server/issue_test.go | 45 +++++++++---------------------------- server/plugin_test.go | 6 ++--- server/user_test.go | 4 ++-- server/webhook_http_test.go | 6 ++--- 6 files changed, 31 insertions(+), 56 deletions(-) diff --git a/server/command_test.go b/server/command_test.go index 7a495b3c5..fced9f4cd 100644 --- a/server/command_test.go +++ b/server/command_test.go @@ -263,7 +263,7 @@ func TestPlugin_ExecuteCommand_Settings(t *testing.T) { func TestPlugin_ExecuteCommand_Installation(t *testing.T) { api := &plugintest.API{} api.On("LogError", mock.AnythingOfTypeArgument("string")).Return(nil) - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) + api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return() api.On("KVSet", mock.AnythingOfType("string"), mock.Anything, mock.Anything).Return(nil) api.On("KVSetWithExpiry", mock.AnythingOfType("string"), mock.Anything, mock.Anything).Return(nil) api.On("KVGet", keyInstances).Return(nil, nil) diff --git a/server/http_test.go b/server/http_test.go index 3a391f629..2a79d849a 100644 --- a/server/http_test.go +++ b/server/http_test.go @@ -316,9 +316,9 @@ func TestSubscribe(t *testing.T) { api := &plugintest.API{} p := Plugin{} - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) - api.On("LogError", mockAnythingOfTypeBatch("string", 10)...).Return(nil) - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return() + api.On("LogError", mockAnythingOfTypeBatch("string", 10)...).Return() + api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() api.On("GetChannelMember", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return(&model.ChannelMember{}, (*model.AppError)(nil)) api.On("CreatePost", mock.AnythingOfType("*model.Post")).Return(&model.Post{}, nil) @@ -427,9 +427,9 @@ func TestDeleteSubscription(t *testing.T) { api := &plugintest.API{} p := Plugin{} - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) - api.On("LogError", mockAnythingOfTypeBatch("string", 10)...).Return(nil) - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return() + api.On("LogError", mockAnythingOfTypeBatch("string", 10)...).Return() + api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() api.On("GetChannelMember", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return(&model.ChannelMember{}, (*model.AppError)(nil)) api.On("CreatePost", mock.AnythingOfType("*model.Post")).Return(&model.Post{}, nil) @@ -646,9 +646,9 @@ func TestEditSubscription(t *testing.T) { api := &plugintest.API{} p := Plugin{} - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) - api.On("LogError", mockAnythingOfTypeBatch("string", 10)...).Return(nil) - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return() + api.On("LogError", mockAnythingOfTypeBatch("string", 10)...).Return() + api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() api.On("GetChannelMember", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return(&model.ChannelMember{}, (*model.AppError)(nil)) api.On("CreatePost", mock.AnythingOfType("*model.Post")).Return(&model.Post{}, nil) @@ -807,9 +807,9 @@ func TestGetSubscriptionsForChannel(t *testing.T) { api := &plugintest.API{} p := Plugin{} - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) - api.On("LogError", mockAnythingOfTypeBatch("string", 10)...).Return(nil) - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return() + api.On("LogError", mockAnythingOfTypeBatch("string", 10)...).Return() + api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() api.On("GetChannelMember", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return(&model.ChannelMember{}, (*model.AppError)(nil)) diff --git a/server/issue_test.go b/server/issue_test.go index 7749a6a24..d50a3647a 100644 --- a/server/issue_test.go +++ b/server/issue_test.go @@ -87,7 +87,7 @@ func (client testClient) AddComment(issueKey string, comment *jira.Comment) (*ji func setupTestPlugin(api *plugintest.API) *Plugin { api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) + api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return() p := &Plugin{} p.SetAPI(api) p.instanceStore = p.getMockInstanceStoreKV(1) @@ -98,10 +98,7 @@ func setupTestPlugin(api *plugintest.API) *Plugin { func TestTransitionJiraIssue(t *testing.T) { api := &plugintest.API{} api.On("SendEphemeralPost", mock.Anything, mock.Anything).Return(nil) - p := Plugin{} - p.SetAPI(api) - p.userStore = getMockUserStoreKV() - p.instanceStore = p.getMockInstanceStoreKV(1) + p := setupTestPlugin(api) for name, tt := range map[string]struct { issueKey string @@ -157,17 +154,8 @@ func TestTransitionJiraIssue(t *testing.T) { func TestRouteIssueTransition(t *testing.T) { api := &plugintest.API{} - - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() - - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) - api.On("SendEphemeralPost", mock.Anything, mock.Anything).Return(nil) - - p := Plugin{} - p.SetAPI(api) - - p.userStore = getMockUserStoreKV() + p := setupTestPlugin(api) for name, tt := range map[string]struct { bb []byte @@ -213,15 +201,10 @@ func TestRouteIssueTransition(t *testing.T) { func TestRouteShareIssuePublicly(t *testing.T) { validUserID := "1" api := &plugintest.API{} - p := Plugin{} api.On("SendEphemeralPost", mock.Anything, mock.Anything).Return(nil) - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) api.On("CreatePost", mock.AnythingOfType("*model.Post")).Return(&model.Post{}, nil) api.On("DeleteEphemeralPost", validUserID, "").Return() - p.SetAPI(api) - p.instanceStore = p.getMockInstanceStoreKV(1) - p.userStore = getMockUserStoreKV() + p := setupTestPlugin(api) for name, tt := range map[string]struct { bb []byte @@ -381,7 +364,7 @@ func TestFetchConnectedUser(t *testing.T) { }, }, }, - expectedErr: errors.New(fmt.Sprintf("instance %s not found", fmt.Sprintf("%q", "test-instanceID"))), + expectedErr: errors.New(fmt.Sprintf("instance %q not found", "test-instanceID")), }, } { t.Run(name, func(t *testing.T) { @@ -496,10 +479,6 @@ func TestGetUserSetting(t *testing.T) { func TestRouteAttachCommentToIssue(t *testing.T) { api := &plugintest.API{} - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() - - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) - api.On("GetPost", "error_post").Return(nil, &model.AppError{Id: "1"}) api.On("GetPost", "post_not_found").Return(nil, (*model.AppError)(nil)) @@ -513,6 +492,11 @@ func TestRouteAttachCommentToIssue(t *testing.T) { api.On("PublishWebSocketEvent", "update_defaults", mock.AnythingOfType("map[string]interface {}"), mock.AnythingOfType("*model.WebsocketBroadcast")) + p := setupTestPlugin(api) + p.updateConfig(func(conf *config) { + conf.mattermostSiteURL = "https://somelink.com" + }) + type requestStruct struct { PostID string `json:"post_id"` InstanceID string `json:"instance_id"` @@ -597,14 +581,6 @@ func TestRouteAttachCommentToIssue(t *testing.T) { }, } { t.Run(name, func(t *testing.T) { - p := Plugin{} - p.SetAPI(api) - p.updateConfig(func(conf *config) { - conf.mattermostSiteURL = "https://somelink.com" - }) - p.userStore = getMockUserStoreKV() - p.instanceStore = p.getMockInstanceStoreKV(1) - tt.request.InstanceID = testInstance1.InstanceID.String() bb, err := json.Marshal(tt.request) assert.Nil(t, err) @@ -616,5 +592,4 @@ func TestRouteAttachCommentToIssue(t *testing.T) { assert.Equal(t, tt.expectedCode, w.Result().StatusCode, "no request data") }) } - } diff --git a/server/plugin_test.go b/server/plugin_test.go index 80f4975c3..a59ce89c4 100644 --- a/server/plugin_test.go +++ b/server/plugin_test.go @@ -109,9 +109,9 @@ func TestPlugin(t *testing.T) { t.Run(name, func(t *testing.T) { api := &plugintest.API{} - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) - api.On("LogError", mockAnythingOfTypeBatch("string", 10)...).Return(nil) - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return() + api.On("LogError", mockAnythingOfTypeBatch("string", 10)...).Return() + api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() api.On("KVGet", mock.AnythingOfTypeArgument("string")).Return(make([]byte, 0), (*model.AppError)(nil)) api.On("GetDirectChannel", mockAnythingOfTypeBatch("string", 2)...).Return( diff --git a/server/user_test.go b/server/user_test.go index bd3f58993..5f654cad5 100644 --- a/server/user_test.go +++ b/server/user_test.go @@ -57,9 +57,9 @@ func TestRouteUserStart(t *testing.T) { } api := &plugintest.API{} - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) + api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return() p := Plugin{} p.SetAPI(api) diff --git a/server/webhook_http_test.go b/server/webhook_http_test.go index 133f2a034..077ebbc73 100644 --- a/server/webhook_http_test.go +++ b/server/webhook_http_test.go @@ -603,9 +603,9 @@ func TestWebhookHTTP(t *testing.T) { t.Run(name, func(t *testing.T) { api := &plugintest.API{} - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) - api.On("LogError", mockAnythingOfTypeBatch("string", 10)...).Return(nil) - api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) + api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return() + api.On("LogError", mockAnythingOfTypeBatch("string", 10)...).Return() + api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return() api.On("GetUserByUsername", "theuser").Return(&model.User{ Id: "theuserid",