Review fixes of #872

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,
-		RolesForDMNotification: map[string]*bool{
-			subCommandAssignee: &trueValue,
-			subCommandMention:  &trueValue,
-			subCommandReporter: &trueValue,
-			subCommandWatching: &trueValue,
+		Notifications: true,
+		RolesForDMNotification: map[string]bool{
+			subCommandAssignee: true,
+			subCommandMention:  true,
+			subCommandReporter: true,
+			subCommandWatching: true,
 		},
 	}
 

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,243 @@ 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
+	}{
+		"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-AccountID",
+				},
+			},
+			wh: webhook{
+				JiraWebhook: &JiraWebhook{
+					Issue: jira.Issue{
+						Fields: &jira.IssueFields{
+							Creator: &jira.User{},
+						},
+					},
+				},
+			},
+			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)
+			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
+	}{
+		"Success": {
+			instanceID: testInstance1.InstanceID,
+			reporter:   &jira.User{},
+		},
+		"Unable to load instance": {
+			instanceID: "test-instanceID",
+			reporter:   &jira.User{},
+		},
+		"Reporter is nil": {
+			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,
+			)
+			if tt.reporter == nil || tt.instanceID == "test-instanceID" {
+				assert.Equal(t, len(wh.notifications), totalNotifications)
+			} else {
+				assert.Equal(t, len(wh.notifications), 1+totalNotifications)
+			}
+		})
+	}
+}
+
+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)
+	jiraAccountID := "test-jiraAccountID"
+	jiraUsername := "test-jiraUsername"
+	p.instanceStore = p.getMockInstanceStoreKV(1)
+	p.userStore = getMockUserStoreKV()
+
+	tests := map[string]struct {
+		wh          *webhook
+		instanceID  types.ID
+		connection  *Connection
+		expectedErr error
+	}{
+		"Success": {
+			wh:         &webhook{},
+			instanceID: testInstance1.InstanceID,
+			connection: &Connection{
+				User: jira.User{AccountID: "test-AccountID"},
+				Settings: &ConnectionSettings{
+					Notifications: true,
+					RolesForDMNotification: (map[string]bool{
+						subCommandAssignee: true,
+						subCommandMention:  true,
+						subCommandReporter: true,
+						subCommandWatching: true,
+					}),
+				},
+			},
+			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, jiraAccountID, 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/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 {