XYZ-87: GlobalRelay DM Exports do not Include Channel Name (#8275)

* Adding ChannelMemberHistory table records when DM channels are created. This ensures that both participants in a DM are shown in compliance export, even if only one of them typed anything

* Direct/Group Message channels now get pretty display names for the purpose of compliance exports

* Fixed string formatting in t.Fatal calls

* Changed uses of ChannelMemberHistory over to ChannelMemberHistoryResult in tests. This should have been done as a part of the work in XYZ-110, but seems to have been missed in this branch for some reason

app/channel.go

@@ -225,6 +225,14 @@ func (a *App) createDirectChannel(userId string, otherUserId string) (*model.Cha
 		}
 	} else {
 		channel := result.Data.(*model.Channel)
+
+		if result := <-a.Srv.Store.ChannelMemberHistory().LogJoinEvent(userId, channel.Id, model.GetMillis()); result.Err != nil {
+			l4g.Warn("Failed to update ChannelMemberHistory table %v", result.Err)
+		}
+		if result := <-a.Srv.Store.ChannelMemberHistory().LogJoinEvent(otherUserId, channel.Id, model.GetMillis()); result.Err != nil {
+			l4g.Warn("Failed to update ChannelMemberHistory table %v", result.Err)
+		}
+
 		return channel, nil
 	}
 }
@@ -1426,7 +1434,16 @@ func (a *App) GetDirectChannel(userId1, userId2 string) (*model.Channel, *model.
 		}
 		a.InvalidateCacheForUser(userId1)
 		a.InvalidateCacheForUser(userId2)
-		return result.Data.(*model.Channel), nil
+
+		channel := result.Data.(*model.Channel)
+		if result := <-a.Srv.Store.ChannelMemberHistory().LogJoinEvent(userId1, channel.Id, model.GetMillis()); result.Err != nil {
+			l4g.Warn("Failed to update ChannelMemberHistory table %v", result.Err)
+		}
+		if result := <-a.Srv.Store.ChannelMemberHistory().LogJoinEvent(userId2, channel.Id, model.GetMillis()); result.Err != nil {
+			l4g.Warn("Failed to update ChannelMemberHistory table %v", result.Err)
+		}
+
+		return channel, nil
 	} else if result.Err != nil {
 		return nil, model.NewAppError("GetOrCreateDMChannel", "web.incoming_webhook.channel.app_error", nil, "err="+result.Err.Message, result.Err.StatusCode)
 	}

app/channel_test.go

@@ -110,7 +110,7 @@ func TestMoveChannel(t *testing.T) {
 	}
 }
 
-func TestJoinDefaultChannelsTownSquare(t *testing.T) {
+func TestJoinDefaultChannelsCreatesChannelMemberHistoryRecordTownSquare(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
 
@@ -136,7 +136,7 @@ func TestJoinDefaultChannelsTownSquare(t *testing.T) {
 	assert.True(t, found)
 }
 
-func TestJoinDefaultChannelsOffTopic(t *testing.T) {
+func TestJoinDefaultChannelsCreatesChannelMemberHistoryRecordOffTopic(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
 
@@ -162,7 +162,7 @@ func TestJoinDefaultChannelsOffTopic(t *testing.T) {
 	assert.True(t, found)
 }
 
-func TestCreateChannelPublic(t *testing.T) {
+func TestCreateChannelPublicCreatesChannelMemberHistoryRecord(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
 
@@ -176,7 +176,7 @@ func TestCreateChannelPublic(t *testing.T) {
 	assert.Equal(t, publicChannel.Id, histories[0].ChannelId)
 }
 
-func TestCreateChannelPrivate(t *testing.T) {
+func TestCreateChannelPrivateCreatesChannelMemberHistoryRecord(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
 
@@ -205,7 +205,7 @@ func TestUpdateChannelPrivacy(t *testing.T) {
 	}
 }
 
-func TestCreateGroupChannel(t *testing.T) {
+func TestCreateGroupChannelCreatesChannelMemberHistoryRecord(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
 
@@ -233,7 +233,62 @@ func TestCreateGroupChannel(t *testing.T) {
 	}
 }
 
-func TestAddUserToChannel(t *testing.T) {
+func TestCreateDirectChannelCreatesChannelMemberHistoryRecord(t *testing.T) {
+	th := Setup().InitBasic()
+	defer th.TearDown()
+
+	user1 := th.CreateUser()
+	user2 := th.CreateUser()
+
+	if channel, err := th.App.CreateDirectChannel(user1.Id, user2.Id); err != nil {
+		t.Fatal("Failed to create direct channel. Error: " + err.Message)
+	} else {
+		// there should be a ChannelMemberHistory record for both users
+		histories := store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, channel.Id)).([]*model.ChannelMemberHistoryResult)
+		assert.Len(t, histories, 2)
+
+		historyId0 := histories[0].UserId
+		historyId1 := histories[1].UserId
+		switch historyId0 {
+		case user1.Id:
+			assert.Equal(t, user2.Id, historyId1)
+		case user2.Id:
+			assert.Equal(t, user1.Id, historyId1)
+		default:
+			t.Fatal("Unexpected user id " + historyId0 + " in ChannelMemberHistory table")
+		}
+	}
+}
+
+func TestGetDirectChannelCreatesChannelMemberHistoryRecord(t *testing.T) {
+	th := Setup().InitBasic()
+	defer th.TearDown()
+
+	user1 := th.CreateUser()
+	user2 := th.CreateUser()
+
+	// this function call implicitly creates a direct channel between the two users if one doesn't already exist
+	if channel, err := th.App.GetDirectChannel(user1.Id, user2.Id); err != nil {
+		t.Fatal("Failed to create direct channel. Error: " + err.Message)
+	} else {
+		// there should be a ChannelMemberHistory record for both users
+		histories := store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, channel.Id)).([]*model.ChannelMemberHistoryResult)
+		assert.Len(t, histories, 2)
+
+		historyId0 := histories[0].UserId
+		historyId1 := histories[1].UserId
+		switch historyId0 {
+		case user1.Id:
+			assert.Equal(t, user2.Id, historyId1)
+		case user2.Id:
+			assert.Equal(t, user1.Id, historyId1)
+		default:
+			t.Fatal("Unexpected user id " + historyId0 + " in ChannelMemberHistory table")
+		}
+	}
+}
+
+func TestAddUserToChannelCreatesChannelMemberHistoryRecord(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
 
@@ -263,7 +318,7 @@ func TestAddUserToChannel(t *testing.T) {
 	assert.Equal(t, groupUserIds, channelMemberHistoryUserIds)
 }
 
-func TestRemoveUserFromChannel(t *testing.T) {
+func TestRemoveUserFromChannelUpdatesChannelMemberHistoryRecord(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
 

store/sqlstore/compliance_store.go

@@ -223,7 +223,11 @@ func (s SqlComplianceStore) MessageExport(after int64, limit int) store.StoreCha
 				Posts.Type AS PostType,
 				Posts.FileIds AS PostFileIds,
 				Channels.Id AS ChannelId,
-				Channels.DisplayName AS ChannelDisplayName,
+				CASE 
+					WHEN Channels.Type = 'D' THEN 'Direct Message'
+					WHEN Channels.Type = 'G' THEN 'Group Message'
+					ELSE Channels.DisplayName
+				END AS ChannelDisplayName,
 				Users.Id AS UserId,
 				Users.Email AS UserEmail,
 				Users.Username

store/storetest/compliance_store.go

@@ -16,7 +16,10 @@ func TestComplianceStore(t *testing.T, ss store.Store) {
 	t.Run("", func(t *testing.T) { testComplianceStore(t, ss) })
 	t.Run("ComplianceExport", func(t *testing.T) { testComplianceExport(t, ss) })
 	t.Run("ComplianceExportDirectMessages", func(t *testing.T) { testComplianceExportDirectMessages(t, ss) })
-	t.Run("MessageExport", func(t *testing.T) { testComplianceMessageExport(t, ss) })
+	t.Run("MessageExportPublicChannel", func(t *testing.T) { testMessageExportPublicChannel(t, ss) })
+	t.Run("MessageExportPrivateChannel", func(t *testing.T) { testMessageExportPrivateChannel(t, ss) })
+	t.Run("MessageExportDirectMessageChannel", func(t *testing.T) { testMessageExportDirectMessageChannel(t, ss) })
+	t.Run("MessageExportGroupMessageChannel", func(t *testing.T) { testMessageExportGroupMessageChannel(t, ss) })
 }
 
 func testComplianceStore(t *testing.T, ss store.Store) {
@@ -319,7 +322,7 @@ func testComplianceExportDirectMessages(t *testing.T, ss store.Store) {
 	}
 }
 
-func testComplianceMessageExport(t *testing.T, ss store.Store) {
+func testMessageExportPublicChannel(t *testing.T, ss store.Store) {
 	// get the starting number of message export entries
 	startTime := model.GetMillis()
 	var numMessageExports = 0
@@ -360,15 +363,14 @@ func testComplianceMessageExport(t *testing.T, ss store.Store) {
 		UserId: user2.Id,
 	}, -1))
 
-	// need a public channel as well as a DM channel between the two users
+	// need a public channel
 	channel := &model.Channel{
 		TeamId:      team.Id,
 		Name:        model.NewId(),
-		DisplayName: "Channel2",
+		DisplayName: "Public Channel",
 		Type:        model.CHANNEL_OPEN,
 	}
 	channel = store.Must(ss.Channel().Save(channel, -1)).(*model.Channel)
-	directMessageChannel := store.Must(ss.Channel().CreateDirectChannel(user1.Id, user2.Id)).(*model.Channel)
 
 	// user1 posts twice in the public channel
 	post1 := &model.Post{
@@ -387,22 +389,13 @@ func testComplianceMessageExport(t *testing.T, ss store.Store) {
 	}
 	post2 = store.Must(ss.Post().Save(post2)).(*model.Post)
 
-	// user1 also sends a DM to user2
-	post3 := &model.Post{
-		ChannelId: directMessageChannel.Id,
-		UserId:    user1.Id,
-		CreateAt:  startTime + 20,
-		Message:   "zz" + model.NewId() + "c",
-	}
-	post3 = store.Must(ss.Post().Save(post3)).(*model.Post)
-
-	// fetch the message exports for all three posts that user1 sent
+	// fetch the message exports for both posts that user1 sent
 	messageExportMap := map[string]model.MessageExport{}
 	if r1 := <-ss.Compliance().MessageExport(startTime-10, 10); r1.Err != nil {
 		t.Fatal(r1.Err)
 	} else {
 		messages := r1.Data.([]*model.MessageExport)
-		assert.Equal(t, numMessageExports+3, len(messages))
+		assert.Equal(t, numMessageExports+2, len(messages))
 
 		for _, v := range messages {
 			messageExportMap[*v.PostId] = *v
@@ -428,13 +421,276 @@ func testComplianceMessageExport(t *testing.T, ss store.Store) {
 	assert.Equal(t, user1.Id, *messageExportMap[post2.Id].UserId)
 	assert.Equal(t, user1.Email, *messageExportMap[post2.Id].UserEmail)
 	assert.Equal(t, user1.Username, *messageExportMap[post2.Id].Username)
-
-	// post3 is a DM between user1 and user2
-	assert.Equal(t, post3.Id, *messageExportMap[post3.Id].PostId)
-	assert.Equal(t, post3.CreateAt, *messageExportMap[post3.Id].PostCreateAt)
-	assert.Equal(t, post3.Message, *messageExportMap[post3.Id].PostMessage)
-	assert.Equal(t, directMessageChannel.Id, *messageExportMap[post3.Id].ChannelId)
-	assert.Equal(t, user1.Id, *messageExportMap[post3.Id].UserId)
-	assert.Equal(t, user1.Email, *messageExportMap[post3.Id].UserEmail)
-	assert.Equal(t, user1.Username, *messageExportMap[post3.Id].Username)
+}
+
+func testMessageExportPrivateChannel(t *testing.T, ss store.Store) {
+	// get the starting number of message export entries
+	startTime := model.GetMillis()
+	var numMessageExports = 0
+	if r1 := <-ss.Compliance().MessageExport(startTime-10, 10); r1.Err != nil {
+		t.Fatal(r1.Err)
+	} else {
+		messages := r1.Data.([]*model.MessageExport)
+		numMessageExports = len(messages)
+	}
+
+	// need a team
+	team := &model.Team{
+		DisplayName: "DisplayName",
+		Name:        "zz" + model.NewId() + "b",
+		Email:       model.NewId() + "@nowhere.com",
+		Type:        model.TEAM_OPEN,
+	}
+	team = store.Must(ss.Team().Save(team)).(*model.Team)
+
+	// and two users that are a part of that team
+	user1 := &model.User{
+		Email:    model.NewId(),
+		Username: model.NewId(),
+	}
+	user1 = store.Must(ss.User().Save(user1)).(*model.User)
+	store.Must(ss.Team().SaveMember(&model.TeamMember{
+		TeamId: team.Id,
+		UserId: user1.Id,
+	}, -1))
+
+	user2 := &model.User{
+		Email:    model.NewId(),
+		Username: model.NewId(),
+	}
+	user2 = store.Must(ss.User().Save(user2)).(*model.User)
+	store.Must(ss.Team().SaveMember(&model.TeamMember{
+		TeamId: team.Id,
+		UserId: user2.Id,
+	}, -1))
+
+	// need a private channel
+	channel := &model.Channel{
+		TeamId:      team.Id,
+		Name:        model.NewId(),
+		DisplayName: "Private Channel",
+		Type:        model.CHANNEL_PRIVATE,
+	}
+	channel = store.Must(ss.Channel().Save(channel, -1)).(*model.Channel)
+
+	// user1 posts twice in the private channel
+	post1 := &model.Post{
+		ChannelId: channel.Id,
+		UserId:    user1.Id,
+		CreateAt:  startTime,
+		Message:   "zz" + model.NewId() + "a",
+	}
+	post1 = store.Must(ss.Post().Save(post1)).(*model.Post)
+
+	post2 := &model.Post{
+		ChannelId: channel.Id,
+		UserId:    user1.Id,
+		CreateAt:  startTime + 10,
+		Message:   "zz" + model.NewId() + "b",
+	}
+	post2 = store.Must(ss.Post().Save(post2)).(*model.Post)
+
+	// fetch the message exports for both posts that user1 sent
+	messageExportMap := map[string]model.MessageExport{}
+	if r1 := <-ss.Compliance().MessageExport(startTime-10, 10); r1.Err != nil {
+		t.Fatal(r1.Err)
+	} else {
+		messages := r1.Data.([]*model.MessageExport)
+		assert.Equal(t, numMessageExports+2, len(messages))
+
+		for _, v := range messages {
+			messageExportMap[*v.PostId] = *v
+		}
+	}
+
+	// post1 was made by user1 in channel1 and team1
+	assert.Equal(t, post1.Id, *messageExportMap[post1.Id].PostId)
+	assert.Equal(t, post1.CreateAt, *messageExportMap[post1.Id].PostCreateAt)
+	assert.Equal(t, post1.Message, *messageExportMap[post1.Id].PostMessage)
+	assert.Equal(t, channel.Id, *messageExportMap[post1.Id].ChannelId)
+	assert.Equal(t, channel.DisplayName, *messageExportMap[post1.Id].ChannelDisplayName)
+	assert.Equal(t, user1.Id, *messageExportMap[post1.Id].UserId)
+	assert.Equal(t, user1.Email, *messageExportMap[post1.Id].UserEmail)
+	assert.Equal(t, user1.Username, *messageExportMap[post1.Id].Username)
+
+	// post2 was made by user1 in channel1 and team1
+	assert.Equal(t, post2.Id, *messageExportMap[post2.Id].PostId)
+	assert.Equal(t, post2.CreateAt, *messageExportMap[post2.Id].PostCreateAt)
+	assert.Equal(t, post2.Message, *messageExportMap[post2.Id].PostMessage)
+	assert.Equal(t, channel.Id, *messageExportMap[post2.Id].ChannelId)
+	assert.Equal(t, channel.DisplayName, *messageExportMap[post2.Id].ChannelDisplayName)
+	assert.Equal(t, user1.Id, *messageExportMap[post2.Id].UserId)
+	assert.Equal(t, user1.Email, *messageExportMap[post2.Id].UserEmail)
+	assert.Equal(t, user1.Username, *messageExportMap[post2.Id].Username)
+}
+
+func testMessageExportDirectMessageChannel(t *testing.T, ss store.Store) {
+	// get the starting number of message export entries
+	startTime := model.GetMillis()
+	var numMessageExports = 0
+	if r1 := <-ss.Compliance().MessageExport(startTime-10, 10); r1.Err != nil {
+		t.Fatal(r1.Err)
+	} else {
+		messages := r1.Data.([]*model.MessageExport)
+		numMessageExports = len(messages)
+	}
+
+	// need a team
+	team := &model.Team{
+		DisplayName: "DisplayName",
+		Name:        "zz" + model.NewId() + "b",
+		Email:       model.NewId() + "@nowhere.com",
+		Type:        model.TEAM_OPEN,
+	}
+	team = store.Must(ss.Team().Save(team)).(*model.Team)
+
+	// and two users that are a part of that team
+	user1 := &model.User{
+		Email:    model.NewId(),
+		Username: model.NewId(),
+	}
+	user1 = store.Must(ss.User().Save(user1)).(*model.User)
+	store.Must(ss.Team().SaveMember(&model.TeamMember{
+		TeamId: team.Id,
+		UserId: user1.Id,
+	}, -1))
+
+	user2 := &model.User{
+		Email:    model.NewId(),
+		Username: model.NewId(),
+	}
+	user2 = store.Must(ss.User().Save(user2)).(*model.User)
+	store.Must(ss.Team().SaveMember(&model.TeamMember{
+		TeamId: team.Id,
+		UserId: user2.Id,
+	}, -1))
+
+	// as well as a DM channel between those users
+	directMessageChannel := store.Must(ss.Channel().CreateDirectChannel(user1.Id, user2.Id)).(*model.Channel)
+
+	// user1 also sends a DM to user2
+	post := &model.Post{
+		ChannelId: directMessageChannel.Id,
+		UserId:    user1.Id,
+		CreateAt:  startTime + 20,
+		Message:   "zz" + model.NewId() + "c",
+	}
+	post = store.Must(ss.Post().Save(post)).(*model.Post)
+
+	// fetch the message export for the post that user1 sent
+	messageExportMap := map[string]model.MessageExport{}
+	if r1 := <-ss.Compliance().MessageExport(startTime-10, 10); r1.Err != nil {
+		t.Fatal(r1.Err)
+	} else {
+		messages := r1.Data.([]*model.MessageExport)
+		assert.Equal(t, numMessageExports+1, len(messages))
+
+		for _, v := range messages {
+			messageExportMap[*v.PostId] = *v
+		}
+	}
+
+	// post is a DM between user1 and user2
+	// there is no channel display name for direct messages, so we sub in the string "Direct Message" instead
+	assert.Equal(t, post.Id, *messageExportMap[post.Id].PostId)
+	assert.Equal(t, post.CreateAt, *messageExportMap[post.Id].PostCreateAt)
+	assert.Equal(t, post.Message, *messageExportMap[post.Id].PostMessage)
+	assert.Equal(t, directMessageChannel.Id, *messageExportMap[post.Id].ChannelId)
+	assert.Equal(t, "Direct Message", *messageExportMap[post.Id].ChannelDisplayName)
+	assert.Equal(t, user1.Id, *messageExportMap[post.Id].UserId)
+	assert.Equal(t, user1.Email, *messageExportMap[post.Id].UserEmail)
+	assert.Equal(t, user1.Username, *messageExportMap[post.Id].Username)
+}
+
+func testMessageExportGroupMessageChannel(t *testing.T, ss store.Store) {
+	// get the starting number of message export entries
+	startTime := model.GetMillis()
+	var numMessageExports = 0
+	if r1 := <-ss.Compliance().MessageExport(startTime-10, 10); r1.Err != nil {
+		t.Fatal(r1.Err)
+	} else {
+		messages := r1.Data.([]*model.MessageExport)
+		numMessageExports = len(messages)
+	}
+
+	// need a team
+	team := &model.Team{
+		DisplayName: "DisplayName",
+		Name:        "zz" + model.NewId() + "b",
+		Email:       model.NewId() + "@nowhere.com",
+		Type:        model.TEAM_OPEN,
+	}
+	team = store.Must(ss.Team().Save(team)).(*model.Team)
+
+	// and three users that are a part of that team
+	user1 := &model.User{
+		Email:    model.NewId(),
+		Username: model.NewId(),
+	}
+	user1 = store.Must(ss.User().Save(user1)).(*model.User)
+	store.Must(ss.Team().SaveMember(&model.TeamMember{
+		TeamId: team.Id,
+		UserId: user1.Id,
+	}, -1))
+
+	user2 := &model.User{
+		Email:    model.NewId(),
+		Username: model.NewId(),
+	}
+	user2 = store.Must(ss.User().Save(user2)).(*model.User)
+	store.Must(ss.Team().SaveMember(&model.TeamMember{
+		TeamId: team.Id,
+		UserId: user2.Id,
+	}, -1))
+
+	user3 := &model.User{
+		Email:    model.NewId(),
+		Username: model.NewId(),
+	}
+	user3 = store.Must(ss.User().Save(user3)).(*model.User)
+	store.Must(ss.Team().SaveMember(&model.TeamMember{
+		TeamId: team.Id,
+		UserId: user3.Id,
+	}, -1))
+
+	// can't create a group channel directly, because importing app creates an import cycle, so we have to fake it
+	groupMessageChannel := &model.Channel{
+		TeamId: team.Id,
+		Name:   model.NewId(),
+		Type:   model.CHANNEL_GROUP,
+	}
+	groupMessageChannel = store.Must(ss.Channel().Save(groupMessageChannel, -1)).(*model.Channel)
+
+	// user1 posts in the GM
+	post := &model.Post{
+		ChannelId: groupMessageChannel.Id,
+		UserId:    user1.Id,
+		CreateAt:  startTime + 20,
+		Message:   "zz" + model.NewId() + "c",
+	}
+	post = store.Must(ss.Post().Save(post)).(*model.Post)
+
+	// fetch the message export for the post that user1 sent
+	messageExportMap := map[string]model.MessageExport{}
+	if r1 := <-ss.Compliance().MessageExport(startTime-10, 10); r1.Err != nil {
+		t.Fatal(r1.Err)
+	} else {
+		messages := r1.Data.([]*model.MessageExport)
+		assert.Equal(t, numMessageExports+1, len(messages))
+
+		for _, v := range messages {
+			messageExportMap[*v.PostId] = *v
+		}
+	}
+
+	// post is a DM between user1 and user2
+	// there is no channel display name for direct messages, so we sub in the string "Direct Message" instead
+	assert.Equal(t, post.Id, *messageExportMap[post.Id].PostId)
+	assert.Equal(t, post.CreateAt, *messageExportMap[post.Id].PostCreateAt)
+	assert.Equal(t, post.Message, *messageExportMap[post.Id].PostMessage)
+	assert.Equal(t, groupMessageChannel.Id, *messageExportMap[post.Id].ChannelId)
+	assert.Equal(t, "Group Message", *messageExportMap[post.Id].ChannelDisplayName)
+	assert.Equal(t, user1.Id, *messageExportMap[post.Id].UserId)
+	assert.Equal(t, user1.Email, *messageExportMap[post.Id].UserEmail)
+	assert.Equal(t, user1.Username, *messageExportMap[post.Id].Username)
 }