#4755 Combining consecutive user join/leave system messages to single message and few other changes. (#5945)

fix 7 and 8 remove @ at "{username} joined the channel" refactor and update test
2017-07-31 23:47:21 +05:30
--- a/app/channel.go
+++ b/app/channel.go
@@ -897,19 +897,75 @@ func JoinChannel(channel *model.Channel, userId string) *model.AppError {
 	return nil
 }

-func postJoinChannelMessage(user *model.User, channel *model.Channel) *model.AppError {
+func createUserActivitySystemMessage(channel *model.Channel, user *model.User, otherUsername, messageType, message string) *model.AppError {
+	props := model.StringInterface{"username": user.Username}
+	if messageType == model.POST_ADD_TO_CHANNEL {
+		props["addedUsername"] = otherUsername
+	}
+
+	if messageType == model.POST_REMOVE_FROM_CHANNEL {
+		props["removedUsername"] = user.Username
+		delete(props, "username")
+	}
+
 	post := &model.Post{
 		ChannelId: channel.Id,
-		Message:   fmt.Sprintf(utils.T("api.channel.join_channel.post_and_forget"), user.Username),
-		Type:      model.POST_JOIN_CHANNEL,
+		Message:   message,
+		Type:      messageType,
 		UserId:    user.Id,
-		Props: model.StringInterface{
-			"username": user.Username,
-		},
+		Props:     props,
 	}

 	if _, err := CreatePost(post, channel.TeamId, false); err != nil {
-		return model.NewLocAppError("postJoinChannelMessage", "api.channel.post_user_add_remove_message_and_forget.error", nil, err.Error())
+		return model.NewLocAppError("createUserActivitySystemMessage", "api.channel.create_user_activity_post.error", nil, err.Error())
+	}
+
+	return nil
+}
+
+func updateUserActivitySystemMessage(post *model.Post, username, otherUsername, messageType, messageToAdd string) *model.AppError {
+	post.Message += " " + messageToAdd
+	props := model.StringMap{"type": messageType, "username": username}
+	if messageType == model.POST_ADD_TO_CHANNEL {
+		props["addedUsername"] = otherUsername
+	}
+
+	if messageType == model.POST_REMOVE_FROM_CHANNEL {
+		props["removedUsername"] = username
+		delete(props, "username")
+	}
+
+	if val, ok := post.Props["messages"]; ok {
+		post.Props["messages"] = append(val.([]interface{}), props)
+	} else {
+		oldProps := make(model.StringInterface)
+		for key, value := range post.Props {
+			oldProps[key] = value
+		}
+		oldProps["type"] = post.Type
+		post.Props["messages"] = []interface{}{oldProps, props}
+	}
+
+	if _, err := UpdatePost(post, false); err != nil {
+		return model.NewLocAppError("updateUserActivitySystemMessage", "api.channel.update_user_activity_post.error", nil, err.Error())
+	}
+
+	return nil
+}
+
+func postJoinChannelMessage(user *model.User, channel *model.Channel) *model.AppError {
+	message := fmt.Sprintf(utils.T("api.channel.join_channel.post_and_forget"), user.Username)
+
+	if post, err := GetLastPostForChannel(channel.Id); err != nil {
+		return err
+	} else if post.IsUserActivitySystemMessage() {
+		if err := updateUserActivitySystemMessage(post, user.Username, "", model.POST_JOIN_CHANNEL, message); err != nil {
+			return err
+		}
+	} else {
+		if err := createUserActivitySystemMessage(channel, user, "", model.POST_JOIN_CHANNEL, message); err != nil {
+			return err
+		}
 	}

 	return nil
@@ -954,55 +1010,64 @@ func LeaveChannel(channelId string, userId string) *model.AppError {
 }

 func postLeaveChannelMessage(user *model.User, channel *model.Channel) *model.AppError {
-	post := &model.Post{
-		ChannelId: channel.Id,
-		Message:   fmt.Sprintf(utils.T("api.channel.leave.left"), user.Username),
-		Type:      model.POST_LEAVE_CHANNEL,
-		UserId:    user.Id,
-		Props: model.StringInterface{
-			"username": user.Username,
-		},
-	}
+	message := fmt.Sprintf(utils.T("api.channel.leave.left"), user.Username)

-	if _, err := CreatePost(post, channel.TeamId, false); err != nil {
-		return model.NewLocAppError("postLeaveChannelMessage", "api.channel.post_user_add_remove_message_and_forget.error", nil, err.Error())
+	if post, err := GetLastPostForChannel(channel.Id); err != nil {
+		return err
+	} else if post.IsUserActivitySystemMessage() {
+		if err := updateUserActivitySystemMessage(post, user.Username, "", model.POST_LEAVE_CHANNEL, message); err != nil {
+			return err
+		}
+	} else {
+		if err := createUserActivitySystemMessage(channel, user, "", model.POST_LEAVE_CHANNEL, message); err != nil {
+			return err
+		}
 	}

 	return nil
 }

 func PostAddToChannelMessage(user *model.User, addedUser *model.User, channel *model.Channel) *model.AppError {
-	post := &model.Post{
-		ChannelId: channel.Id,
-		Message:   fmt.Sprintf(utils.T("api.channel.add_member.added"), addedUser.Username, user.Username),
-		Type:      model.POST_ADD_TO_CHANNEL,
-		UserId:    user.Id,
-		Props: model.StringInterface{
-			"username":      user.Username,
-			"addedUsername": addedUser.Username,
-		},
-	}
+	message := fmt.Sprintf(utils.T("api.channel.add_member.added"), user.Username, addedUser.Username)

-	if _, err := CreatePost(post, channel.TeamId, false); err != nil {
-		return model.NewLocAppError("postAddToChannelMessage", "api.channel.post_user_add_remove_message_and_forget.error", nil, err.Error())
+	if post, err := GetLastPostForChannel(channel.Id); err != nil {
+		return err
+	} else if post.IsUserActivitySystemMessage() {
+		if err := updateUserActivitySystemMessage(post, user.Username, addedUser.Username, model.POST_ADD_TO_CHANNEL, message); err != nil {
+			return err
+		}
+
+		SendNotificationsForSystemMessageAddRemove(user, addedUser, channel, post)
+	} else {
+		if err := createUserActivitySystemMessage(channel, user, addedUser.Username, model.POST_ADD_TO_CHANNEL, message); err != nil {
+			return err
+		}
 	}

 	return nil
 }

 func PostRemoveFromChannelMessage(removerUserId string, removedUser *model.User, channel *model.Channel) *model.AppError {
-	post := &model.Post{
-		ChannelId: channel.Id,
-		Message:   fmt.Sprintf(utils.T("api.channel.remove_member.removed"), removedUser.Username),
-		Type:      model.POST_REMOVE_FROM_CHANNEL,
-		UserId:    removerUserId,
-		Props: model.StringInterface{
-			"removedUsername": removedUser.Username,
-		},
+	message := fmt.Sprintf(utils.T("api.channel.remove_member.removed"), removedUser.Username)
+
+	removerUser, err := GetUser(removerUserId)
+	if err != nil {
+		fmt.Printf("err: %+v\n", err)
+		return err
 	}

-	if _, err := CreatePost(post, channel.TeamId, false); err != nil {
-		return model.NewLocAppError("postRemoveFromChannelMessage", "api.channel.post_user_add_remove_message_and_forget.error", nil, err.Error())
+	if post, err := GetLastPostForChannel(channel.Id); err != nil {
+		return err
+	} else if post.IsUserActivitySystemMessage() {
+		if err := updateUserActivitySystemMessage(post, removedUser.Username, removerUser.Username, model.POST_REMOVE_FROM_CHANNEL, message); err != nil {
+			return err
+		}
+
+		SendNotificationsForSystemMessageAddRemove(removerUser, removedUser, channel, post)
+	} else {
+		if err := createUserActivitySystemMessage(channel, removedUser, removerUser.Username, model.POST_ADD_TO_CHANNEL, message); err != nil {
+			return err
+		}
 	}

 	return nil
@@ -1046,20 +1111,28 @@ func RemoveUserFromChannel(userIdToRemove string, removerUserId string, channel
 		return err
 	}

-	var user *model.User
-	if user, err = GetUser(userIdToRemove); err != nil {
+	var userToRemove *model.User
+	if userToRemove, err = GetUser(userIdToRemove); err != nil {
 		return err
 	}

 	if userIdToRemove == removerUserId {
-		postLeaveChannelMessage(user, channel)
+		postLeaveChannelMessage(userToRemove, channel)
 	} else {
-		go PostRemoveFromChannelMessage(removerUserId, user, channel)
+		go PostRemoveFromChannelMessage(removerUserId, userToRemove, channel)
 	}

 	return nil
 }

+func GetLastPostForChannel(channelId string) (*model.Post, *model.AppError) {
+	result := <-Srv.Store.Post().GetLastPostForChannel(channelId)
+	if result.Err != nil {
+		return nil, result.Err
+	}
+	return result.Data.(*model.Post), nil
+}
+
 func GetNumberOfChannelsOnTeam(teamId string) (int, *model.AppError) {
 	// Get total number of channels on current team
 	if result := <-Srv.Store.Channel().GetTeamChannels(teamId); result.Err != nil {
--- a/app/channel_test.go
+++ b/app/channel_test.go
@@ -1,6 +1,11 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See License.txt for license information.
+
 package app

 import (
+	"fmt"
+	"reflect"
 	"testing"

 	"github.com/mattermost/platform/model"
@@ -64,3 +69,193 @@ func TestPermanentDeleteChannel(t *testing.T) {
 		t.Error("outgoing webhook wasn't deleted")
 	}
 }
+
+func PostUserActivitySystemMessage(t *testing.T, testFunc func(user *model.User, channel *model.Channel) *model.AppError, postType string, messageKey string) {
+	th := Setup().InitBasic()
+
+	// Test when last post was a leave/join post
+	user := &model.User{Id: model.NewId(), Email: "test@example.com", Username: "test"}
+
+	post := &model.Post{
+		ChannelId: th.BasicChannel.Id, UserId: user.Id, Type: postType, Message: "message",
+		Props: model.StringInterface{"username": user.Username},
+	}
+	post = (<-Srv.Store.Post().Save(post)).Data.(*model.Post)
+
+	testFunc(user, th.BasicChannel)
+	post = (<-Srv.Store.Post().GetSingle(post.Id)).Data.(*model.Post)
+	if post.Message != "message "+fmt.Sprintf(utils.T(messageKey), user.Username) {
+		t.Fatal("Leave/join a channel message wasn't appended to last leave/join post")
+	}
+
+	if !reflect.DeepEqual(
+		post.Props["messages"].([]interface{}),
+		[]interface{}{
+			map[string]interface{}{"type": postType, "username": user.Username},
+			map[string]interface{}{"type": postType, "username": user.Username},
+		},
+	) {
+		t.Fatal("Invalid leave/join a channel props")
+	}
+
+	// Test when last post was not a leave/join post
+	post.Id = ""
+	post.Message = "message1"
+	post.Type = model.POST_DEFAULT
+	post.Props = nil
+	post = (<-Srv.Store.Post().Save(post)).Data.(*model.Post)
+
+	testFunc(user, th.BasicChannel)
+	post = (<-Srv.Store.Post().GetSingle(post.Id)).Data.(*model.Post)
+	if post.Message == "message1 "+fmt.Sprintf(utils.T(messageKey), user.Username) {
+		t.Fatal("Leave/join a channel message was appended to last non leave/join post")
+	}
+
+	if _, ok := post.Props["messages"]; ok {
+		t.Fatal("Invalid leave/join a channel props. \"message\" shouldn't be present")
+	}
+}
+
+func TestPostJoinChannelMessage(t *testing.T) {
+	PostUserActivitySystemMessage(t, postJoinChannelMessage, model.POST_JOIN_CHANNEL, "api.channel.join_channel.post_and_forget")
+}
+
+func TestPostLeaveChannelMessage(t *testing.T) {
+	PostUserActivitySystemMessage(t, postLeaveChannelMessage, model.POST_LEAVE_CHANNEL, "api.channel.leave.left")
+}
+
+func TestPostAddToChannelMessage(t *testing.T) {
+	th := Setup().InitBasic()
+
+	// Test when last post was a user activity system message post
+	user := &model.User{Id: model.NewId(), Email: "test@example.com", Username: "test"}
+	addedUser := &model.User{Id: model.NewId(), Email: "test1@example.com", Username: "test1"}
+
+	post := &model.Post{
+		ChannelId: th.BasicChannel.Id, UserId: user.Id, Type: model.POST_ADD_TO_CHANNEL, Message: "message",
+		Props: model.StringInterface{"username": user.Username, "addedUsername": addedUser.Username},
+	}
+	post = (<-Srv.Store.Post().Save(post)).Data.(*model.Post)
+
+	PostAddToChannelMessage(user, addedUser, th.BasicChannel)
+	post = (<-Srv.Store.Post().GetSingle(post.Id)).Data.(*model.Post)
+	if post.Message != "message "+fmt.Sprintf(utils.T("api.channel.add_member.added"), user.Username, addedUser.Username) {
+		t.Fatal("Add message wasn't appended to last user activity system message post")
+	}
+
+	if !reflect.DeepEqual(
+		post.Props["messages"].([]interface{}),
+		[]interface{}{
+			map[string]interface{}{"type": model.POST_ADD_TO_CHANNEL, "username": user.Username, "addedUsername": addedUser.Username},
+			map[string]interface{}{"type": model.POST_ADD_TO_CHANNEL, "username": user.Username, "addedUsername": addedUser.Username},
+		},
+	) {
+		t.Fatal("Invalid added to channel props")
+	}
+
+	// Test when last post was not a user activity system message post
+	post.Id = ""
+	post.Message = "message1"
+	post.Type = model.POST_DEFAULT
+	post.Props = nil
+	post = (<-Srv.Store.Post().Save(post)).Data.(*model.Post)
+
+	PostAddToChannelMessage(user, addedUser, th.BasicChannel)
+	post = (<-Srv.Store.Post().GetSingle(post.Id)).Data.(*model.Post)
+	if post.Message == "message1\n"+fmt.Sprintf(utils.T("api.channel.add_member.added"), addedUser.Username, user.Username) {
+		t.Fatal("Added to channel message was appended to last non user activity system message post")
+	}
+
+	if _, ok := post.Props["messages"]; ok {
+		t.Fatal("Invalid added to channel props. \"message\" shouldn't be present")
+	}
+}
+
+func TestPostRemoveFromChannelMessage(t *testing.T) {
+	th := Setup().InitBasic()
+	user := th.BasicUser
+
+	// // Test when last post was a user activity system message post
+	removedUser := &model.User{Id: model.NewId(), Email: "test1@example.com", Username: "test1"}
+
+	post := &model.Post{
+		ChannelId: th.BasicChannel.Id, UserId: user.Id, Type: model.POST_REMOVE_FROM_CHANNEL, Message: "message",
+		Props: model.StringInterface{"removedUsername": removedUser.Username},
+	}
+	post = (<-Srv.Store.Post().Save(post)).Data.(*model.Post)
+
+	PostRemoveFromChannelMessage(user.Id, removedUser, th.BasicChannel)
+	post = (<-Srv.Store.Post().GetSingle(post.Id)).Data.(*model.Post)
+	if post.Message != "message "+fmt.Sprintf(utils.T("api.channel.remove_member.removed"), removedUser.Username) {
+		t.Fatal("Post removed from a channel message wasn't appended to last user activity system message post")
+	}
+
+	if !reflect.DeepEqual(
+		post.Props["messages"].([]interface{}),
+		[]interface{}{
+			map[string]interface{}{"type": model.POST_REMOVE_FROM_CHANNEL, "removedUsername": removedUser.Username},
+			map[string]interface{}{"type": model.POST_REMOVE_FROM_CHANNEL, "removedUsername": removedUser.Username},
+		},
+	) {
+		t.Fatal("Invalid removed from a channel props")
+	}
+
+	// Test when last post was not a user activity system message post
+	post.Id = ""
+	post.Message = "message1"
+	post.Type = model.POST_DEFAULT
+	post.Props = nil
+	post = (<-Srv.Store.Post().Save(post)).Data.(*model.Post)
+
+	PostRemoveFromChannelMessage(user.Id, removedUser, th.BasicChannel)
+	post = (<-Srv.Store.Post().GetSingle(post.Id)).Data.(*model.Post)
+	if post.Message == "message1 "+fmt.Sprintf(utils.T("api.channel.remove_member.removed"), removedUser.Username) {
+		t.Fatal("Post removed from a channel message was appended to last non user activity system message post")
+	}
+
+	if _, ok := post.Props["messages"]; ok {
+		t.Fatal("Invalid removed from a channel props. \"message\" shouldn't be present")
+	}
+}
+
+func TestGetLastPostForChannel(t *testing.T) {
+	th := Setup().InitBasic()
+	user := th.BasicUser
+
+	post1 := &model.Post{
+		ChannelId: th.BasicChannel.Id,
+		UserId:    user.Id,
+		Message:   "message",
+	}
+	post1 = (<-Srv.Store.Post().Save(post1)).Data.(*model.Post)
+
+	rpost1, err := GetLastPostForChannel(th.BasicChannel.Id)
+	if err != nil {
+		t.Fatal("Last post should have been returned")
+	}
+
+	if post1.Message != rpost1.Message {
+		t.Fatal("Should match post message")
+	}
+
+	post2 := &model.Post{
+		ChannelId: th.BasicChannel.Id,
+		UserId:    user.Id,
+		Message:   "message",
+	}
+	post2 = (<-Srv.Store.Post().Save(post2)).Data.(*model.Post)
+
+	rpost2, err := GetLastPostForChannel(th.BasicChannel.Id)
+	if err != nil {
+		t.Fatal("Last post should have been returned")
+	}
+
+	if post2.Message != rpost2.Message {
+		t.Fatal("Should match post message")
+	}
+
+	_, err = GetLastPostForChannel(model.NewId())
+	if err != nil {
+		t.Fatal("Should not return err")
+	}
+}
--- a/app/notification.go
+++ b/app/notification.go
@@ -306,6 +306,91 @@ func SendNotifications(post *model.Post, team *model.Team, channel *model.Channe
 	return mentionedUsersList, nil
 }

+func SendNotificationsForSystemMessageAddRemove(user *model.User, otherUser *model.User, channel *model.Channel, post *model.Post) *model.AppError {
+	otherUserChannelMember, _ := GetChannelMember(channel.Id, otherUser.Id)
+	team, err := GetTeam(channel.TeamId)
+	if err != nil {
+		return err
+	}
+
+	err = (<-Srv.Store.Channel().IncrementMentionCount(channel.Id, otherUser.Id)).Err
+	if err != nil {
+		return err
+	}
+
+	if utils.Cfg.EmailSettings.SendEmailNotifications {
+		userAllowsEmails := otherUser.NotifyProps[model.EMAIL_NOTIFY_PROP] != "false"
+		if otherUserChannelMember != nil {
+			channelEmail, ok := otherUserChannelMember.NotifyProps[model.EMAIL_NOTIFY_PROP]
+			if ok && channelEmail != model.CHANNEL_NOTIFY_DEFAULT {
+				userAllowsEmails = channelEmail != "false"
+			}
+		}
+
+		var status *model.Status
+		var err *model.AppError
+		if status, err = GetStatus(otherUser.Id); err != nil {
+			status = &model.Status{
+				UserId:         otherUser.Id,
+				Status:         model.STATUS_OFFLINE,
+				Manual:         false,
+				LastActivityAt: 0,
+				ActiveChannel:  "",
+			}
+		}
+
+		senderName := utils.T("system.message.name")
+		if userAllowsEmails && status.Status != model.STATUS_ONLINE && otherUser.DeleteAt == 0 {
+			if err = sendNotificationEmail(post, otherUser, channel, team, senderName, user); err != nil {
+				return err
+			}
+		}
+	}
+
+	if otherUserChannelMember != nil {
+		sendPushNotifications := false
+		if *utils.Cfg.EmailSettings.SendPushNotifications {
+			pushServer := *utils.Cfg.EmailSettings.PushNotificationServer
+			if pushServer == model.MHPNS && (!utils.IsLicensed || !*utils.License.Features.MHPNS) {
+				l4g.Warn(utils.T("api.post.send_notifications_and_forget.push_notification.mhpnsWarn"))
+				sendPushNotifications = false
+			} else {
+				sendPushNotifications = true
+			}
+		}
+
+		channelName := channel.DisplayName
+		senderUsername := user.Username
+
+		if sendPushNotifications {
+			var status *model.Status
+			var err *model.AppError
+			if status, err = GetStatus(otherUser.Id); err != nil {
+				status = &model.Status{UserId: otherUser.Id, Status: model.STATUS_OFFLINE, Manual: false, LastActivityAt: 0, ActiveChannel: ""}
+			}
+
+			if ShouldSendPushNotification(otherUser, otherUserChannelMember.NotifyProps, true, status, post) {
+				if err = sendPushNotification(post, otherUser, channel, senderUsername, channelName, true); err != nil {
+					return err
+				}
+			}
+		}
+
+		message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_POSTED, "", post.ChannelId, "", nil)
+		message.Add("post", post.ToJson())
+		message.Add("channel_type", channel.Type)
+		message.Add("channel_display_name", channelName)
+		message.Add("channel_name", channel.Name)
+		message.Add("sender_name", senderUsername)
+		message.Add("team_id", channel.TeamId)
+		message.Add("mentions", otherUser.Username)
+
+		Publish(message)
+	}
+
+	return nil
+}
+
 func sendNotificationEmail(post *model.Post, user *model.User, channel *model.Channel, team *model.Team, senderName string, sender *model.User) *model.AppError {
 	if channel.IsGroupOrDirect() {
 		if result := <-Srv.Store.Team().GetTeamsByUserId(user.Id); result.Err != nil {
--- a/app/post.go
+++ b/app/post.go
@@ -250,8 +250,8 @@ func UpdatePost(post *model.Post, safeUpdate bool) (*model.Post, *model.AppError
 			return nil, err
 		}

-		if oldPost.IsSystemMessage() {
-			err := model.NewAppError("UpdatePost", "api.post.update_post.system_message.app_error", nil, "id="+post.Id, http.StatusBadRequest)
+		if oldPost.IsSystemMessage() && !oldPost.IsUserActivitySystemMessage() {
+			err := model.NewAppError("updatePost", "api.post.update_post.system_message.app_error", nil, "id="+post.Id, http.StatusBadRequest)
 			return nil, err
 		}