Review fixes of #872

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,
 		},
 	}
 

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
 		}

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) {
+	cs := ConnectionSettings{}
+	cs.RolesForDMNotification = make(map[string]bool)
+	cs.RolesForDMNotification[subCommandAssignee] = true
+	cs.RolesForDMNotification[subCommandMention] = true
+	tests := map[string]struct {
+		role                   string
+		notification           bool
+		RolesForDMNotification map[string]bool
+	}{
+		subCommandAssignee: {
+			role:         subCommandAssignee,
+			notification: true,
+		},
+		subCommandMention: {
+			role:         subCommandMention,
+			notification: true,
+		},
+		subCommandReporter: {
+			role:         subCommandReporter,
+			notification: false,
+		},
+		subCommandWatching: {
+			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{}
+
+	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 {
+		instanceID  types.ID
+		client      Client
+		connection  *Connection
+		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{},
+			connection: &Connection{
+				Settings: &ConnectionSettings{
+					Notifications: true,
+					RolesForDMNotification: map[string]bool{
+						subCommandAssignee: true,
+						subCommandMention:  true,
+						subCommandReporter: true,
+						subCommandWatching: true,
+					},
+				},
+				User: jira.User{
+					AccountID: "test",
+				},
+			},
+			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(
+				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()
+	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 {
+		instanceID types.ID
+		reporter   *jira.User
+	}{
+		"Unable to load instance": {
+			instanceID: "instanceID",
+			reporter:   &jira.User{},
+		},
+		"No reporter": {
+			instanceID: testInstance1.InstanceID,
+			reporter:   nil,
+		},
+		"Success": {
+			instanceID: testInstance1.InstanceID,
+			reporter:   &jira.User{},
+		},
+	}
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			totalNotifications := len(wh.notifications)
+			p.applyReporterNotification(
+				wh,
+				tt.instanceID,
+				tt.reporter,
+			)
+			if tt.reporter == nil || tt.instanceID == "instanceID" {
+				assert.Equal(t, len(wh.notifications)-totalNotifications, 0)
+			} else {
+				assert.Equal(t, len(wh.notifications)-totalNotifications, 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()
+
+	tests := map[string]struct {
+		wh            *webhook
+		instanceID    types.ID
+		jiraAccountID string
+		jiraUsername  string
+		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:  "",
+			connection: &Connection{
+				User: jira.User{AccountID: "test"},
+				Settings: &ConnectionSettings{
+					Notifications: true,
+					RolesForDMNotification: (map[string]bool{
+						subCommandAssignee: true,
+						subCommandMention:  true,
+						subCommandReporter: true,
+						subCommandWatching: true,
+					}),
+				},
+			},
+			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{}
 

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
 	}
 

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 {

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,
 			},
 		},
 	}