MM-61731 - enhance failed sent system bot message (#29253)

* MM-61731-enhance-failed-sent-system-bot-message * adjust test and adjust i18n messages * set back correct value * improve naming and get channels logic * hide channel name if the channel is private --------- Co-authored-by: Mattermost Build <build@mattermost.com>
2024-11-14 16:56:40 +01:00
--- a/server/channels/app/scheduled_post_job.go
+++ b/server/channels/app/scheduled_post_job.go
@@ -6,6 +6,7 @@ package app
 import (
 	"fmt"
 	"net/http"
+	"strings"
 	"time"

 	"github.com/mattermost/mattermost/server/v8/platform/services/telemetry"
@@ -418,13 +419,60 @@ func (a *App) notifyUser(rctx request.CTX, userId string, userFailedMessages []*
 	}

 	T := i18n.GetUserTranslations(user.Locale)
-	messageContent := T("app.scheduled_post.failed_messages", map[string]interface{}{
-		"Count": len(userFailedMessages),
+
+	type channelErrorKey struct {
+		ChannelId string
+		ErrorCode string
+	}
+
+	channelErrorCounts := make(map[channelErrorKey]int)
+	channelIdsSet := make(map[string]struct{})
+	for _, msg := range userFailedMessages {
+		key := channelErrorKey{ChannelId: msg.ChannelId, ErrorCode: msg.ErrorCode}
+		channelErrorCounts[key]++
+		channelIdsSet[msg.ChannelId] = struct{}{}
+	}
+
+	channelNames := make(map[string]string)
+	for channelId := range channelIdsSet {
+		ch, err := a.GetChannel(rctx, channelId)
+		if err != nil {
+			rctx.Logger().Error("Failed to get channel", mlog.String("channel_id", channelId), mlog.Err(err))
+			channelNames[channelId] = T("app.scheduled_post.unknown_channel")
+			continue
+		}
+		if ch.Type != model.ChannelTypePrivate {
+			channelNames[channelId] = ch.DisplayName
+		} else {
+			channelNames[channelId] = T("app.scheduled_post.private_channel")
+		}
+	}
+
+	var messageBuilder strings.Builder
+
+	totalFailedMessages := len(userFailedMessages)
+	messageHeader := T("app.scheduled_post.failed_messages", map[string]interface{}{
+		"Count": totalFailedMessages,
 	})
+	messageBuilder.WriteString(messageHeader)
+	messageBuilder.WriteString("\n")
+
+	for key, count := range channelErrorCounts {
+		channelName := channelNames[key.ChannelId]
+		errorReason := getErrorReason(T, key.ErrorCode)
+
+		detailedMessage := T("app.scheduled_post.failed_message_detail", map[string]interface{}{
+			"Count":       count,
+			"ChannelName": channelName,
+			"ErrorReason": errorReason,
+		})
+		messageBuilder.WriteString(detailedMessage)
+		messageBuilder.WriteString("\n")
+	}

 	post := &model.Post{
 		ChannelId: channel.Id,
-		Message:   messageContent,
+		Message:   messageBuilder.String(),
 		Type:      model.PostTypeDefault,
 		UserId:    systemBot.UserId,
 	}
@@ -433,3 +481,32 @@ func (a *App) notifyUser(rctx request.CTX, userId string, userFailedMessages []*
 		rctx.Logger().Error("Failed to post notification about failed scheduled messages", mlog.Err(err))
 	}
 }
+
+func getErrorReason(T i18n.TranslateFunc, errorCode string) string {
+	var reason string
+	switch errorCode {
+	case "unknown":
+		reason = T("app.scheduled_post.error_reason.unknown")
+	case "channel_archived":
+		reason = T("app.scheduled_post.error_reason.channel_archived")
+	case "channel_not_found":
+		reason = T("app.scheduled_post.error_reason.channel_not_found")
+	case "user_missing":
+		reason = T("app.scheduled_post.error_reason.user_missing")
+	case "user_deleted":
+		reason = T("app.scheduled_post.error_reason.user_deleted")
+	case "no_channel_permission":
+		reason = T("app.scheduled_post.error_reason.no_channel_permission")
+	case "no_channel_member":
+		reason = T("app.scheduled_post.error_reason.no_channel_member")
+	case "thread_deleted":
+		reason = T("app.scheduled_post.error_reason.thread_deleted")
+	case "unable_to_send":
+		reason = T("app.scheduled_post.error_reason.unable_to_send")
+	case "invalid_post":
+		reason = T("app.scheduled_post.error_reason.invalid_post")
+	default:
+		reason = errorCode
+	}
+	return reason
+}
--- a/server/channels/app/scheduled_post_job_test.go
+++ b/server/channels/app/scheduled_post_job_test.go
@@ -4,6 +4,7 @@
 package app

 import (
+	"strings"
 	"testing"
 	"time"

@@ -281,6 +282,9 @@ func TestHandleFailedScheduledPosts(t *testing.T) {
 		user1 := th.BasicUser
 		user2 := th.BasicUser2

+		channel1 := th.BasicChannel
+		channel2 := th.CreateChannel(th.Context, th.BasicTeam)
+
 		// Create failed scheduled posts: 1 for user1 and 2 for user2
 		failedScheduledPosts := []*model.ScheduledPost{
 			{
@@ -288,7 +292,7 @@ func TestHandleFailedScheduledPosts(t *testing.T) {
 				Draft: model.Draft{
 					CreateAt:  model.GetMillis(),
 					UserId:    user1.Id,
-					ChannelId: th.BasicChannel.Id,
+					ChannelId: channel1.Id,
 					Message:   "Failed scheduled post for user 1",
 				},
 				ErrorCode: model.ScheduledPostErrorUnknownError,
@@ -298,7 +302,7 @@ func TestHandleFailedScheduledPosts(t *testing.T) {
 				Draft: model.Draft{
 					CreateAt:  model.GetMillis(),
 					UserId:    user2.Id,
-					ChannelId: th.BasicChannel.Id,
+					ChannelId: channel1.Id,
 					Message:   "Failed scheduled post 1 for user 2",
 				},
 				ErrorCode: model.ScheduledPostErrorCodeNoChannelPermission,
@@ -308,7 +312,7 @@ func TestHandleFailedScheduledPosts(t *testing.T) {
 				Draft: model.Draft{
 					CreateAt:  model.GetMillis(),
 					UserId:    user2.Id,
-					ChannelId: th.BasicChannel.Id,
+					ChannelId: channel2.Id,
 					Message:   "Failed scheduled post 2 for user 2",
 				},
 				ErrorCode: model.ScheduledPostErrorNoChannelMember,
@@ -342,22 +346,22 @@ func TestHandleFailedScheduledPosts(t *testing.T) {
 				if received.GetBroadcast().UserId == user2.Id {
 					assert.Equal(t, model.WebsocketScheduledPostUpdated, received.EventType())
 				}
-			case <-time.After(1 * time.Second):
+			case <-time.After(3 * time.Second):
 				t.Errorf("Timeout while waiting for a WebSocket event for scheduled post %d", i+1)
 			}
 		}

 		// Helper function to check notifications for a specific user
-		checkUserNotification := func(user *model.User, expectedCount int) {
-			// Wait time for notifications to be sent (adding 2 secs because it is run in a separate rountine)
-			var timeout = 2 * time.Second
+		checkUserNotification := func(user *model.User) {
+			// Wait time for notifications to be sent (adding 5 secs because it is run in a separate goroutine)
+			var timeout = 5 * time.Second
 			begin := time.Now()
 			channel, appErr := th.App.GetOrCreateDirectChannel(rctx, user.Id, systemBot.UserId)
 			assert.True(t, appErr == nil)

 			var posts *model.PostList
 			// wait for the notification to be sent into the channel.
-			// idea is to get the channel and try to find posts, if not, wait 100ms and try again until timout or there is posts lengh
+			// idea is to get the channel and try to find posts, if not, wait 100ms and try again until timeout or there is posts length
 			for {
 				if time.Since(begin) > timeout {
 					break
@@ -371,24 +375,85 @@ func TestHandleFailedScheduledPosts(t *testing.T) {
 			}
 			assert.NotEmpty(t, posts.Posts, "Expected notification for user %s to have been sent", user.Id)

-			// Validate the actual content of the notification posted (to include count verification)
-			T := i18n.GetUserTranslations(user.Locale)
-			messageContent := T("app.scheduled_post.failed_messages", map[string]interface{}{
-				"Count": expectedCount,
-			})
+			// Collect failed messages for users
+			var userFailedMessages []*model.ScheduledPost
+			for _, sp := range failedScheduledPosts {
+				if sp.UserId == user.Id {
+					userFailedMessages = append(userFailedMessages, sp)
+				}
+			}

+			T := i18n.GetUserTranslations(user.Locale)
+
+			// Aggregate failed messages by channel and error code
+			type channelErrorKey struct {
+				ChannelId string
+				ErrorCode string
+			}
+
+			channelErrorCounts := make(map[channelErrorKey]int)
+			channelIdsSet := make(map[string]struct{})
+			for _, msg := range userFailedMessages {
+				key := channelErrorKey{ChannelId: msg.ChannelId, ErrorCode: msg.ErrorCode}
+				channelErrorCounts[key]++
+				channelIdsSet[msg.ChannelId] = struct{}{}
+			}
+
+			// Get the channel names
+			channelNames := make(map[string]string)
+			for channelId := range channelIdsSet {
+				ch, err := th.App.GetChannel(rctx, channelId)
+				assert.Nil(t, err)
+				channelNames[channelId] = ch.DisplayName
+			}
+
+			// Helper function to get error reason
+			getErrorReason := func(T i18n.TranslateFunc, errorCode string) string {
+				key := "app.scheduled_post.error_reason." + errorCode
+				reason := T(key)
+				if reason == key {
+					return errorCode
+				}
+				return reason
+			}
+
+			// Build the expected message content
+			var messageBuilder strings.Builder
+
+			messageHeader := T("app.scheduled_post.failed_messages", map[string]interface{}{
+				"Count": len(userFailedMessages),
+			})
+			messageBuilder.WriteString(messageHeader)
+			messageBuilder.WriteString("\n")
+
+			for key, count := range channelErrorCounts {
+				channelName := channelNames[key.ChannelId]
+				errorReason := getErrorReason(T, key.ErrorCode)
+
+				detailMessage := T("app.scheduled_post.failed_message_detail", map[string]interface{}{
+					"Count":       count,
+					"ChannelName": channelName,
+					"ErrorReason": errorReason,
+				})
+				messageBuilder.WriteString(detailMessage)
+				messageBuilder.WriteString("\n")
+			}
+
+			expectedMessageContent := messageBuilder.String()
+
+			// Validate the actual content of the notification posted
 			found := false
 			for _, post := range posts.Posts {
-				if post.UserId == systemBot.UserId && post.Message == messageContent {
+				if post.UserId == systemBot.UserId && post.Message == expectedMessageContent {
 					found = true
 					break
 				}
 			}
-			assert.True(t, found, "Notification post not found for user %s with expected count %d", user.Id, expectedCount)
+			assert.True(t, found, "\nNotification post not found for user %s with expected message. \n Expected: %s \n", user.Id, expectedMessageContent)
 		}

 		// Check notifications sent for failed messages for both users
-		checkUserNotification(user1, 1)
-		checkUserNotification(user2, 2)
+		checkUserNotification(user1)
+		checkUserNotification(user2)
 	})
 }
--- a/server/i18n/en.json
+++ b/server/i18n/en.json
@@ -6546,6 +6546,53 @@
    "id": "app.save_scheduled_post.save.app_error",
    "translation": "Error occurred saving the scheduled post."
  },
+  {
+    "id": "app.scheduled_post.error_reason.channel_archived",
+    "translation": "Channel is archived"
+  },
+  {
+    "id": "app.scheduled_post.error_reason.channel_not_found",
+    "translation": "Channel not found"
+  },
+  {
+    "id": "app.scheduled_post.error_reason.invalid_post",
+    "translation": "Invalid post content"
+  },
+  {
+    "id": "app.scheduled_post.error_reason.no_channel_member",
+    "translation": "Not a member of the channel"
+  },
+  {
+    "id": "app.scheduled_post.error_reason.no_channel_permission",
+    "translation": "No permission to post in channel"
+  },
+  {
+    "id": "app.scheduled_post.error_reason.thread_deleted",
+    "translation": "Thread has been deleted"
+  },
+  {
+    "id": "app.scheduled_post.error_reason.unable_to_send",
+    "translation": "Unable to send the message"
+  },
+  {
+    "id": "app.scheduled_post.error_reason.unknown",
+    "translation": "Unknown Error"
+  },
+  {
+    "id": "app.scheduled_post.error_reason.user_deleted",
+    "translation": "User account is deleted"
+  },
+  {
+    "id": "app.scheduled_post.error_reason.user_missing",
+    "translation": "User does not exist"
+  },
+  {
+    "id": "app.scheduled_post.failed_message_detail",
+    "translation": {
+      "one": "- {{.Count}} in channel {{.ChannelName}}. Reason: {{.ErrorReason}}",
+      "other": "- {{.Count}} in channel {{.ChannelName}}. Reason: {{.ErrorReason}}"
+    }
+  },
  {
    "id": "app.scheduled_post.failed_messages",
    "translation": {
@@ -6557,6 +6604,14 @@
    "id": "app.scheduled_post.permanent_delete_by_user.app_error",
    "translation": "Unable to delete scheduled posts for user."
  },
+  {
+    "id": "app.scheduled_post.private_channel",
+    "translation": "Private channel"
+  },
+  {
+    "id": "app.scheduled_post.unknown_channel",
+    "translation": "Unknown Channel"
+  },
  {
    "id": "app.scheme.delete.app_error",
    "translation": "Unable to delete this scheme."