Review fixes of mattermost#872 (#7)

* [MI-2088]:Add Support for DM subscriptions for watchers and reporters * [MI-2088]: Review Fixes 1. Improved the name of certain variable 2. Improved spacing * [MI-2088]:Revieww fixes 1.Changes the names of few variables 2.Improved code complexity * [MI-2088]: Review fixes done 1.Changed the name of few variables * [MI-2088]:fixed review fixes * [MI-2088]:fixed review fixes * [MI-2088]:fixed review fixes * [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 * [MI-2088]: Review fixes done 1. Changed the name of various variables 2. Used constants everywhere * [MI-2088]: Review fixes done 1. Added Test cases 2. Used booleans instead of pointer in RolesForDMNotification * [MI-2088]: Review fixes done 1. Modified test cases * [MI-2088]: Review fixes done 1. Self reviewed * [MI-2088]: Review fixes done 1. Changed the formatting 2. Changed the name of few variables 3. Improved code quality * [MI-2088]: Fixes CI errors * [MI-2088]: Review fixes done 1. Improved code quality * [MI-2088]: Review fixes done 1. Improved code quality * [MI-2088]: Review fixes done 1. Improved code quality Co-authored-by: Kshitij Katiyar <kshitij.katiyar@brightscout.com>
Brightscout · Oct 13, 2022 · 9a77507 · 9a77507
1 parent 2ef95e2
commit 9a77507
Show file tree

Hide file tree

Showing 12 changed files with 335 additions and 138 deletions.
diff --git 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,
-		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,
 		},
 	}
 
@@ -264,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
@@ -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.go b/server/issue.go
@@ -1133,13 +1133,38 @@ 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 +1194,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
 		}