From 14aba9bccbaa431de11630d4f1c13ab465b8122f Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Wed, 22 Jul 2020 09:04:40 -0400 Subject: [PATCH] MM-26410/MM-26825 Improve syncing between favorites category and preferences (#15048) * MM-26410 Allow moving channels into Favorites when they're favorited in prefs * MM-26410 Fix management of Favorites category when updating preferences * MM-26410 Add management of Favorites category when deleting preferences * Address feedback 1 * Remove WHERE (1=1) from query * Remove unnecessary sq.Expr * Rewrite query to use left join * Remove redundant where statement and add some more tests * Fix linting issues * Rename addChannelToFavoritesCategory to addChannelToFavoritesCategory --- api4/preference_test.go | 444 ++++++++++++++++++++++++++ app/preference.go | 16 +- i18n/en.json | 8 + store/opentracing_layer.go | 20 +- store/sqlstore/channel_store.go | 205 +++++++++--- store/store.go | 3 +- store/storetest/channel_store.go | 99 ++++++ store/storetest/mocks/ChannelStore.go | 24 +- store/timer_layer.go | 18 +- 9 files changed, 788 insertions(+), 49 deletions(-) diff --git a/api4/preference_test.go b/api4/preference_test.go index 8c98f0f890..62e2e12744 100644 --- a/api4/preference_test.go +++ b/api4/preference_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/mattermost/mattermost-server/v5/model" @@ -295,6 +296,233 @@ func TestUpdatePreferencesWebsocket(t *testing.T) { } } +func TestUpdateSidebarPreferences(t *testing.T) { + t.Run("when favoriting a channel, should add it to the Favorites sidebar category", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + user := th.BasicUser + + team1 := th.CreateTeam() + th.LinkUserToTeam(user, team1) + + _, resp := th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + + channel := th.CreateChannelWithClientAndTeam(th.Client, model.CHANNEL_OPEN, team1.Id) + th.AddUserToChannel(user, channel) + + // Confirm that the sidebar is populated correctly to begin with + categories, resp := th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + require.NotContains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + require.Contains(t, categories.Categories[1].Channels, channel.Id) + + // Favorite the channel + _, resp = th.Client.UpdatePreferences(user.Id, &model.Preferences{ + { + UserId: user.Id, + Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, + Name: channel.Id, + Value: "true", + }, + }) + require.Nil(t, resp.Error) + + // Confirm that the channel was added to the Favorites + categories, resp = th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + assert.Contains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + assert.NotContains(t, categories.Categories[1].Channels, channel.Id) + + // And unfavorite the channel + _, resp = th.Client.UpdatePreferences(user.Id, &model.Preferences{ + { + UserId: user.Id, + Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, + Name: channel.Id, + Value: "false", + }, + }) + require.Nil(t, resp.Error) + + // The channel should've been removed from the Favorites + categories, resp = th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + require.NotContains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + assert.Contains(t, categories.Categories[1].Channels, channel.Id) + }) + + t.Run("when favoriting a DM channel, should add it to the Favorites sidebar category for all teams", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + user := th.BasicUser + user2 := th.BasicUser2 + + team1 := th.CreateTeam() + th.LinkUserToTeam(user, team1) + team2 := th.CreateTeam() + th.LinkUserToTeam(user, team2) + + dmChannel := th.CreateDmChannel(user2) + + // Favorite the channel + _, resp := th.Client.UpdatePreferences(user.Id, &model.Preferences{ + { + UserId: user.Id, + Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, + Name: dmChannel.Id, + Value: "true", + }, + }) + require.Nil(t, resp.Error) + + // Confirm that the channel was added to the Favorites on all teams + categories, resp := th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + assert.Contains(t, categories.Categories[0].Channels, dmChannel.Id) + require.Equal(t, model.SidebarCategoryDirectMessages, categories.Categories[2].Type) + assert.NotContains(t, categories.Categories[2].Channels, dmChannel.Id) + + categories, resp = th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team2.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + assert.Contains(t, categories.Categories[0].Channels, dmChannel.Id) + require.Equal(t, model.SidebarCategoryDirectMessages, categories.Categories[2].Type) + assert.NotContains(t, categories.Categories[2].Channels, dmChannel.Id) + + // And unfavorite the channel + _, resp = th.Client.UpdatePreferences(user.Id, &model.Preferences{ + { + UserId: user.Id, + Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, + Name: dmChannel.Id, + Value: "false", + }, + }) + require.Nil(t, resp.Error) + + // The channel should've been removed from the Favorites on all teams + categories, resp = th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + require.NotContains(t, categories.Categories[0].Channels, dmChannel.Id) + require.Equal(t, model.SidebarCategoryDirectMessages, categories.Categories[2].Type) + assert.Contains(t, categories.Categories[2].Channels, dmChannel.Id) + + categories, resp = th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team2.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + require.NotContains(t, categories.Categories[0].Channels, dmChannel.Id) + require.Equal(t, model.SidebarCategoryDirectMessages, categories.Categories[2].Type) + assert.Contains(t, categories.Categories[2].Channels, dmChannel.Id) + }) + + t.Run("when favoriting a channel, should not affect other users' favorites categories", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + user := th.BasicUser + user2 := th.BasicUser2 + + client2 := th.CreateClient() + th.LoginBasic2WithClient(client2) + + team1 := th.CreateTeam() + th.LinkUserToTeam(user, team1) + th.LinkUserToTeam(user2, team1) + + _, resp := th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + _, resp = client2.GetSidebarCategoriesForTeamForUser(user2.Id, team1.Id, "") + require.Nil(t, resp.Error) + + channel := th.CreateChannelWithClientAndTeam(th.Client, model.CHANNEL_OPEN, team1.Id) + th.AddUserToChannel(user, channel) + th.AddUserToChannel(user2, channel) + + // Confirm that the sidebar is populated correctly to begin with + categories, resp := th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + require.NotContains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + require.Contains(t, categories.Categories[1].Channels, channel.Id) + + categories, resp = client2.GetSidebarCategoriesForTeamForUser(user2.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + require.NotContains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + require.Contains(t, categories.Categories[1].Channels, channel.Id) + + // Favorite the channel + _, resp = th.Client.UpdatePreferences(user.Id, &model.Preferences{ + { + UserId: user.Id, + Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, + Name: channel.Id, + Value: "true", + }, + }) + require.Nil(t, resp.Error) + + // Confirm that the channel was not added to Favorites for the second user + categories, resp = client2.GetSidebarCategoriesForTeamForUser(user2.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + assert.NotContains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + assert.Contains(t, categories.Categories[1].Channels, channel.Id) + + // Favorite the channel for the second user + _, resp = client2.UpdatePreferences(user2.Id, &model.Preferences{ + { + UserId: user2.Id, + Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, + Name: channel.Id, + Value: "true", + }, + }) + require.Nil(t, resp.Error) + + // Confirm that the channel is now in the Favorites for the second user + categories, resp = client2.GetSidebarCategoriesForTeamForUser(user2.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + assert.Contains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + assert.NotContains(t, categories.Categories[1].Channels, channel.Id) + + // And unfavorite the channel + _, resp = th.Client.UpdatePreferences(user.Id, &model.Preferences{ + { + UserId: user.Id, + Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, + Name: channel.Id, + Value: "false", + }, + }) + require.Nil(t, resp.Error) + + // The channel should still be in the second user's favorites + categories, resp = client2.GetSidebarCategoriesForTeamForUser(user2.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + assert.Contains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + assert.NotContains(t, categories.Categories[1].Channels, channel.Id) + }) +} + func TestDeletePreferences(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() @@ -396,3 +624,219 @@ func TestDeletePreferencesWebsocket(t *testing.T) { } } } + +func TestDeleteSidebarPreferences(t *testing.T) { + t.Run("when removing a favorited channel preference, should remove it from the Favorites sidebar category", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + user := th.BasicUser + + team1 := th.CreateTeam() + th.LinkUserToTeam(user, team1) + + _, resp := th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + + channel := th.CreateChannelWithClientAndTeam(th.Client, model.CHANNEL_OPEN, team1.Id) + th.AddUserToChannel(user, channel) + + // Confirm that the sidebar is populated correctly to begin with + categories, resp := th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + require.NotContains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + require.Contains(t, categories.Categories[1].Channels, channel.Id) + + // Favorite the channel + _, resp = th.Client.UpdatePreferences(user.Id, &model.Preferences{ + { + UserId: user.Id, + Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, + Name: channel.Id, + Value: "true", + }, + }) + require.Nil(t, resp.Error) + + // Confirm that the channel was added to the Favorites + categories, resp = th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + assert.Contains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + assert.NotContains(t, categories.Categories[1].Channels, channel.Id) + + // And unfavorite the channel by deleting the preference + _, resp = th.Client.DeletePreferences(user.Id, &model.Preferences{ + { + UserId: user.Id, + Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, + Name: channel.Id, + }, + }) + require.Nil(t, resp.Error) + + // The channel should've been removed from the Favorites + categories, resp = th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + require.NotContains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + assert.Contains(t, categories.Categories[1].Channels, channel.Id) + }) + + t.Run("when removing a favorited DM preference, should remove it from the Favorites sidebar category", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + user := th.BasicUser + user2 := th.BasicUser2 + + team1 := th.CreateTeam() + th.LinkUserToTeam(user, team1) + team2 := th.CreateTeam() + th.LinkUserToTeam(user, team2) + + dmChannel := th.CreateDmChannel(user2) + + // Favorite the channel + _, resp := th.Client.UpdatePreferences(user.Id, &model.Preferences{ + { + UserId: user.Id, + Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, + Name: dmChannel.Id, + Value: "true", + }, + }) + require.Nil(t, resp.Error) + + // Confirm that the channel was added to the Favorites on all teams + categories, resp := th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + assert.Contains(t, categories.Categories[0].Channels, dmChannel.Id) + require.Equal(t, model.SidebarCategoryDirectMessages, categories.Categories[2].Type) + assert.NotContains(t, categories.Categories[2].Channels, dmChannel.Id) + + categories, resp = th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team2.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + assert.Contains(t, categories.Categories[0].Channels, dmChannel.Id) + require.Equal(t, model.SidebarCategoryDirectMessages, categories.Categories[2].Type) + assert.NotContains(t, categories.Categories[2].Channels, dmChannel.Id) + + // And unfavorite the channel by deleting the preference + _, resp = th.Client.DeletePreferences(user.Id, &model.Preferences{ + { + UserId: user.Id, + Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, + Name: dmChannel.Id, + }, + }) + require.Nil(t, resp.Error) + + // The channel should've been removed from the Favorites on all teams + categories, resp = th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + require.NotContains(t, categories.Categories[0].Channels, dmChannel.Id) + require.Equal(t, model.SidebarCategoryDirectMessages, categories.Categories[2].Type) + assert.Contains(t, categories.Categories[2].Channels, dmChannel.Id) + + categories, resp = th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team2.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + require.NotContains(t, categories.Categories[0].Channels, dmChannel.Id) + require.Equal(t, model.SidebarCategoryDirectMessages, categories.Categories[2].Type) + assert.Contains(t, categories.Categories[2].Channels, dmChannel.Id) + }) + + t.Run("when removing a favorited channel preference, should not affect other users' favorites categories", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + user := th.BasicUser + user2 := th.BasicUser2 + + client2 := th.CreateClient() + th.LoginBasic2WithClient(client2) + + team1 := th.CreateTeam() + th.LinkUserToTeam(user, team1) + th.LinkUserToTeam(user2, team1) + + _, resp := th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + _, resp = client2.GetSidebarCategoriesForTeamForUser(user2.Id, team1.Id, "") + require.Nil(t, resp.Error) + + channel := th.CreateChannelWithClientAndTeam(th.Client, model.CHANNEL_OPEN, team1.Id) + th.AddUserToChannel(user, channel) + th.AddUserToChannel(user2, channel) + + // Confirm that the sidebar is populated correctly to begin with + categories, resp := th.Client.GetSidebarCategoriesForTeamForUser(user.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + require.NotContains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + require.Contains(t, categories.Categories[1].Channels, channel.Id) + + categories, resp = client2.GetSidebarCategoriesForTeamForUser(user2.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + require.NotContains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + require.Contains(t, categories.Categories[1].Channels, channel.Id) + + // Favorite the channel for both users + _, resp = th.Client.UpdatePreferences(user.Id, &model.Preferences{ + { + UserId: user.Id, + Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, + Name: channel.Id, + Value: "true", + }, + }) + require.Nil(t, resp.Error) + + _, resp = client2.UpdatePreferences(user2.Id, &model.Preferences{ + { + UserId: user2.Id, + Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, + Name: channel.Id, + Value: "true", + }, + }) + require.Nil(t, resp.Error) + + // Confirm that the channel is in the Favorites for the second user + categories, resp = client2.GetSidebarCategoriesForTeamForUser(user2.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + assert.Contains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + assert.NotContains(t, categories.Categories[1].Channels, channel.Id) + + // And unfavorite the channel for the first user by deleting the preference + _, resp = th.Client.UpdatePreferences(user.Id, &model.Preferences{ + { + UserId: user.Id, + Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, + Name: channel.Id, + Value: "false", + }, + }) + require.Nil(t, resp.Error) + + // The channel should still be in the second user's favorites + categories, resp = client2.GetSidebarCategoriesForTeamForUser(user2.Id, team1.Id, "") + require.Nil(t, resp.Error) + require.Equal(t, model.SidebarCategoryFavorites, categories.Categories[0].Type) + assert.Contains(t, categories.Categories[0].Channels, channel.Id) + require.Equal(t, model.SidebarCategoryChannels, categories.Categories[1].Type) + assert.NotContains(t, categories.Categories[1].Channels, channel.Id) + }) +} diff --git a/app/preference.go b/app/preference.go index 64ecfc0b6e..7cc048870b 100644 --- a/app/preference.go +++ b/app/preference.go @@ -54,9 +54,11 @@ func (a *App) UpdatePreferences(userId string, preferences model.Preferences) *m } if err := a.Srv().Store.Channel().UpdateSidebarChannelsByPreferences(&preferences); err != nil { - return err + return model.NewAppError("UpdatePreferences", "api.preference.update_preferences.update_sidebar.app_error", nil, err.Error(), http.StatusInternalServerError) } + message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_SIDEBAR_CATEGORY_UPDATED, "", "", userId, nil) + // TODO this needs to be updated to include information on which categories changed a.Publish(message) message = model.NewWebSocketEvent(model.WEBSOCKET_EVENT_PREFERENCES_CHANGED, "", "", userId, nil) @@ -69,7 +71,7 @@ func (a *App) UpdatePreferences(userId string, preferences model.Preferences) *m func (a *App) DeletePreferences(userId string, preferences model.Preferences) *model.AppError { for _, preference := range preferences { if userId != preference.UserId { - err := model.NewAppError("deletePreferences", "api.preference.delete_preferences.delete.app_error", nil, + err := model.NewAppError("DeletePreferences", "api.preference.delete_preferences.delete.app_error", nil, "userId="+userId+", preference.UserId="+preference.UserId, http.StatusForbidden) return err } @@ -82,7 +84,15 @@ func (a *App) DeletePreferences(userId string, preferences model.Preferences) *m } } - message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_PREFERENCES_DELETED, "", "", userId, nil) + if err := a.Srv().Store.Channel().DeleteSidebarChannelsByPreferences(&preferences); err != nil { + return model.NewAppError("DeletePreferences", "api.preference.delete_preferences.update_sidebar.app_error", nil, err.Error(), http.StatusInternalServerError) + } + + message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_SIDEBAR_CATEGORY_UPDATED, "", "", userId, nil) + // TODO this needs to be updated to include information on which categories changed + a.Publish(message) + + message = model.NewWebSocketEvent(model.WEBSOCKET_EVENT_PREFERENCES_DELETED, "", "", userId, nil) message.Add("preferences", preferences.ToJson()) a.Publish(message) diff --git a/i18n/en.json b/i18n/en.json index f31029db83..f334940ec1 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1814,6 +1814,10 @@ "id": "api.preference.delete_preferences.delete.app_error", "translation": "Unable to delete user preferences." }, + { + "id": "api.preference.delete_preferences.update_sidebar.app_error", + "translation": "Unable to update sidebar to match deleted preferences" + }, { "id": "api.preference.preferences_category.get.app_error", "translation": "Unable to get user preferences." @@ -1822,6 +1826,10 @@ "id": "api.preference.update_preferences.set.app_error", "translation": "Unable to set user preferences." }, + { + "id": "api.preference.update_preferences.update_sidebar.app_error", + "translation": "Unable to update sidebar to match updated preferences" + }, { "id": "api.push_notification.disabled.app_error", "translation": "Push Notifications are disabled on this server." diff --git a/store/opentracing_layer.go b/store/opentracing_layer.go index b2e38c27ba..3fb170e10f 100644 --- a/store/opentracing_layer.go +++ b/store/opentracing_layer.go @@ -702,6 +702,24 @@ func (s *OpenTracingLayerChannelStore) DeleteSidebarCategory(categoryId string) return resultVar0 } +func (s *OpenTracingLayerChannelStore) DeleteSidebarChannelsByPreferences(preferences *model.Preferences) error { + origCtx := s.Root.Store.Context() + span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ChannelStore.DeleteSidebarChannelsByPreferences") + s.Root.Store.SetContext(newCtx) + defer func() { + s.Root.Store.SetContext(origCtx) + }() + + defer span.Finish() + resultVar0 := s.ChannelStore.DeleteSidebarChannelsByPreferences(preferences) + if resultVar0 != nil { + span.LogFields(spanlog.Error(resultVar0)) + ext.Error.Set(span, true) + } + + return resultVar0 +} + func (s *OpenTracingLayerChannelStore) Get(id string, allowFromCache bool) (*model.Channel, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ChannelStore.Get") @@ -2205,7 +2223,7 @@ func (s *OpenTracingLayerChannelStore) UpdateSidebarChannelCategoryOnMove(channe return resultVar0 } -func (s *OpenTracingLayerChannelStore) UpdateSidebarChannelsByPreferences(preferences *model.Preferences) *model.AppError { +func (s *OpenTracingLayerChannelStore) UpdateSidebarChannelsByPreferences(preferences *model.Preferences) error { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ChannelStore.UpdateSidebarChannelsByPreferences") s.Root.Store.SetContext(newCtx) diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index c546f0f5dd..5987579f90 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -479,16 +479,16 @@ func (s SqlChannelStore) MigrateSidebarCategories(fromTeamId, fromUserId string) func (s SqlChannelStore) CreateInitialSidebarCategories(userId, teamId string) error { transaction, err := s.GetMaster().Begin() if err != nil { - return err + return errors.Wrap(err, "CreateInitialSidebarCategories: begin_transaction") } defer finalizeTransaction(transaction) if err := s.createInitialSidebarCategoriesT(transaction, userId, teamId); err != nil { - return err + return errors.Wrap(err, "CreateInitialSidebarCategories: createInitialSidebarCategoriesT") } if err := transaction.Commit(); err != nil { - return err + return errors.Wrap(err, "CreateInitialSidebarCategories: commit_transaction") } return nil @@ -507,7 +507,7 @@ func (s SqlChannelStore) createInitialSidebarCategoriesT(transaction *gorp.Trans var existingTypes []model.SidebarCategoryType _, err := transaction.Select(&existingTypes, selectQuery, selectParams...) if err != nil { - return err + return errors.Wrap(err, "createInitialSidebarCategoriesT: failed to select existing categories") } hasCategoryOfType := func(categoryType model.SidebarCategoryType) bool { @@ -530,7 +530,7 @@ func (s SqlChannelStore) createInitialSidebarCategoriesT(transaction *gorp.Trans SortOrder: model.DefaultSidebarSortOrderFavorites, Type: model.SidebarCategoryFavorites, }); err != nil { - return err + return errors.Wrap(err, "createInitialSidebarCategoriesT: failed to insert favorites category") } } @@ -544,7 +544,7 @@ func (s SqlChannelStore) createInitialSidebarCategoriesT(transaction *gorp.Trans SortOrder: model.DefaultSidebarSortOrderChannels, Type: model.SidebarCategoryChannels, }); err != nil { - return err + return errors.Wrap(err, "createInitialSidebarCategoriesT: failed to insert channels category") } } @@ -558,7 +558,7 @@ func (s SqlChannelStore) createInitialSidebarCategoriesT(transaction *gorp.Trans SortOrder: model.DefaultSidebarSortOrderDMs, Type: model.SidebarCategoryDirectMessages, }); err != nil { - return err + return errors.Wrap(err, "createInitialSidebarCategoriesT: failed to insert direct messages category") } } @@ -3901,19 +3901,17 @@ func (s SqlChannelStore) UpdateSidebarCategories(userId, teamId string, categori } // And then add the new ones - var preferences []interface{} - for _, channelID := range category.Channels { - preferences = append(preferences, &model.Preference{ + // This breaks the PreferenceStore abstraction, but it should be safe to assume that everything is a SQL + // store in this package. + if err := s.Preference().(*SqlPreferenceStore).save(transaction, &model.Preference{ Name: channelID, UserId: userId, Category: model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, Value: "true", - }) - } - - if err = transaction.Insert(preferences...); err != nil { - return nil, model.NewAppError("SqlPostStore.UpdateSidebarCategory", "store.sql_channel.sidebar_categories.app_error", nil, err.Error(), http.StatusInternalServerError) + }); err != nil { + return nil, model.NewAppError("SqlPostStore.UpdateSidebarCategory", "store.sql_channel.sidebar_categories.app_error", nil, err.Error(), http.StatusInternalServerError) + } } } else { // Remove any old favorites that might have been in this category @@ -3950,47 +3948,180 @@ func (s SqlChannelStore) UpdateSidebarCategories(userId, teamId string, categori return updatedCategories, nil } -// UpdateSidebarChannelByPreference is called when the Preference table is being updated to keep SidebarCategories in sync +// UpdateSidebarChannelsByPreferences is called when the Preference table is being updated to keep SidebarCategories in sync // At the moment, it's only handling Favorites and NOT DMs/GMs (those will be handled client side) -func (s SqlChannelStore) UpdateSidebarChannelsByPreferences(preferences *model.Preferences) *model.AppError { +func (s SqlChannelStore) UpdateSidebarChannelsByPreferences(preferences *model.Preferences) error { transaction, err := s.GetMaster().Begin() if err != nil { - return model.NewAppError("SqlChannelStore.UpdateSidebarChannelsByPreferences", "store.sql_channel.sidebar_categories.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError) + return errors.Wrap(err, "UpdateSidebarChannelsByPreferences: begin_transaction") } - defer finalizeTransaction(transaction) + for _, preference := range *preferences { + preference := preference + if preference.Category != model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL { continue } - params := map[string]interface{}{ - "UserId": preference.UserId, - "ChannelId": preference.Name, - "CategoryType": model.SidebarCategoryFavorites, - } + // if new preference is false - remove the channel from the appropriate sidebar category if preference.Value == "false" { - var deleteQuery string - if s.DriverName() == model.DATABASE_DRIVER_MYSQL { - deleteQuery = "DELETE SidebarChannels FROM SidebarChannels LEFT JOIN SidebarCategories ON SidebarCategories.Id = SidebarChannels.CategoryId WHERE SidebarCategories.Type=:CategoryType AND SidebarCategories.UserId=:UserId AND SidebarChannels.UserId=:UserId AND ChannelId=:ChannelId" - } else { - deleteQuery = "DELETE FROM SidebarChannels USING SidebarChannels AS chan LEFT OUTER JOIN SidebarCategories AS cat ON cat.Id = chan.CategoryId WHERE cat.Type=:CategoryType AND cat.UserId = :UserId AND chan.UserId = :UserId AND cat.TeamId = :TeamId AND chan.ChannelId=:ChannelId" - } - - if _, err := transaction.Exec(deleteQuery, params); err != nil { - return model.NewAppError("SqlChannelStore.UpdateSidebarChannelByPreference", "store.sql_channel.sidebar_categories.app_error", nil, err.Error(), http.StatusInternalServerError) + if err := s.removeSidebarEntriesForPreferenceT(transaction, &preference); err != nil { + return errors.Wrap(err, "UpdateSidebarChannelsByPreferences: removeSidebarEntriesForPreferenceT") } } else { - // otherwise - insert new channel into the apropriate category. ignore duplicate error - if _, err := transaction.Exec("INSERT INTO SidebarChannels (ChannelId, UserId, CategoryId, SortOrder) SELECT Id AS CategoryId, :UserId AS UserId, :ChannelId AS ChannelId, MAX(SidebarChannels.SortOrder)+10 FROM SidebarCategories INNER JOIN SidebarChannels ON SidebarChannels.CategoryId = SidebarCategories.Id WHERE SidebarCategories.Type=:CategoryType AND SidebarCategories.UserId=:UserId GROUP BY SidebarChannels.CategoryId, SidebarCategories.Id", params); err != nil && !IsUniqueConstraintError(err, []string{"UserId"}) { - return model.NewAppError("SqlChannelStore.UpdateSidebarChannelByPreference", "store.sql_channel.sidebar_categories.app_error", nil, err.Error(), http.StatusInternalServerError) + if err := s.addChannelToFavoritesCategoryT(transaction, &preference); err != nil { + return errors.Wrap(err, "UpdateSidebarChannelsByPreferences: addChannelToFavoritesCategoryT") } } } if err := transaction.Commit(); err != nil { - return model.NewAppError("SqlChannelStore.UpdateSidebarChannelByPreference", "store.sql_channel.sidebar_categories.commit_transaction.app_error", nil, err.Error(), http.StatusInternalServerError) + return errors.Wrap(err, "UpdateSidebarChannelsByPreferences: commit_transaction") } + + return nil +} + +func (s SqlChannelStore) removeSidebarEntriesForPreferenceT(transaction *gorp.Transaction, preference *model.Preference) error { + if preference.Category != model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL { + return nil + } + + // Delete any corresponding SidebarChannels entries in a Favorites category corresponding to this preference. This + // can't use the query builder because it uses DB-specific syntax + params := map[string]interface{}{ + "UserId": preference.UserId, + "ChannelId": preference.Name, + "CategoryType": model.SidebarCategoryFavorites, + } + var query string + if s.DriverName() == model.DATABASE_DRIVER_MYSQL { + query = ` + DELETE + SidebarChannels + FROM + SidebarChannels + JOIN + SidebarCategories ON SidebarChannels.CategoryId = SidebarCategories.Id + WHERE + SidebarChannels.UserId = :UserId + AND SidebarChannels.ChannelId = :ChannelId + AND SidebarCategories.Type = :CategoryType` + } else { + query = ` + DELETE FROM + SidebarChannels + USING + SidebarCategories + WHERE + SidebarChannels.CategoryId = SidebarCategories.Id + AND SidebarChannels.UserId = :UserId + AND SidebarChannels.ChannelId = :ChannelId + AND SidebarCategories.Type = :CategoryType` + } + + if _, err := transaction.Exec(query, params); err != nil { + return errors.Wrap(err, "Failed to remove sidebar entries for preference") + } + + return nil +} + +func (s SqlChannelStore) addChannelToFavoritesCategoryT(transaction *gorp.Transaction, preference *model.Preference) error { + if preference.Category != model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL { + return nil + } + + var channel *model.Channel + if obj, err := transaction.Get(&model.Channel{}, preference.Name); err != nil { + return errors.Wrapf(err, "Failed to get favorited channel with id=%s", preference.Name) + } else { + channel = obj.(*model.Channel) + } + + // Get the IDs of the Favorites category/categories that the channel needs to be added to + builder := s.getQueryBuilder(). + Select("SidebarCategories.Id"). + From("SidebarCategories"). + LeftJoin("SidebarChannels on SidebarCategories.Id = SidebarChannels.CategoryId and SidebarChannels.ChannelId = ?", preference.Name). + Where(sq.Eq{ + "SidebarCategories.UserId": preference.UserId, + "Type": model.SidebarCategoryFavorites, + }). + Where("SidebarChannels.ChannelId is null") + + if channel.TeamId != "" { + builder = builder.Where(sq.Eq{"TeamId": channel.TeamId}) + } + + idsQuery, idsParams, _ := builder.ToSql() + + var categoryIds []string + if _, err := transaction.Select(&categoryIds, idsQuery, idsParams...); err != nil { + return errors.Wrap(err, "Failed to get Favorites sidebar categories") + } + + if len(categoryIds) == 0 { + // The channel is already in the Favorites category/categories + return nil + } + + // For each category ID, insert a row into SidebarChannels with the given channel ID and a SortOrder that's less than + // all existing SortOrders in the category so that the newly favorited channel comes first + insertQuery, insertParams, _ := s.getQueryBuilder(). + Insert("SidebarChannels"). + Columns( + "ChannelId", + "CategoryId", + "UserId", + "SortOrder", + ). + Select( + sq.Select(). + Column("? as ChannelId", preference.Name). + Column("SidebarCategories.Id as CategoryId"). + Column("? as UserId", preference.UserId). + Column("COALESCE(MIN(SidebarChannels.SortOrder) - 10, 0) as SortOrder"). + From("SidebarCategories"). + LeftJoin("SidebarChannels on SidebarCategories.Id = SidebarChannels.CategoryId"). + Where(sq.Eq{ + "SidebarCategories.Id": categoryIds, + }). + GroupBy("SidebarCategories.Id")).ToSql() + + if _, err := transaction.Exec(insertQuery, insertParams...); err != nil { + return errors.Wrap(err, "Failed to add sidebar entries for favorited channel") + } + + return nil +} + +// DeleteSidebarChannelsByPreferences is called when the Preference table is being updated to keep SidebarCategories in sync +// At the moment, it's only handling Favorites and NOT DMs/GMs (those will be handled client side) +func (s SqlChannelStore) DeleteSidebarChannelsByPreferences(preferences *model.Preferences) error { + transaction, err := s.GetMaster().Begin() + if err != nil { + return errors.Wrap(err, "DeleteSidebarChannelsByPreferences: begin_transaction") + } + defer finalizeTransaction(transaction) + + for _, preference := range *preferences { + preference := preference + + if preference.Category != model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL { + continue + } + + if err := s.removeSidebarEntriesForPreferenceT(transaction, &preference); err != nil { + return errors.Wrap(err, "DeleteSidebarChannelsByPreferences: removeSidebarEntriesForPreferenceT") + } + } + + if err := transaction.Commit(); err != nil { + return errors.Wrap(err, "DeleteSidebarChannelsByPreferences: commit_transaction") + } + return nil } diff --git a/store/store.go b/store/store.go index b829a63661..63ea52d67e 100644 --- a/store/store.go +++ b/store/store.go @@ -225,7 +225,8 @@ type ChannelStore interface { CreateSidebarCategory(userId, teamId string, newCategory *model.SidebarCategoryWithChannels) (*model.SidebarCategoryWithChannels, *model.AppError) UpdateSidebarCategoryOrder(userId, teamId string, categoryOrder []string) *model.AppError UpdateSidebarCategories(userId, teamId string, categories []*model.SidebarCategoryWithChannels) ([]*model.SidebarCategoryWithChannels, *model.AppError) - UpdateSidebarChannelsByPreferences(preferences *model.Preferences) *model.AppError + UpdateSidebarChannelsByPreferences(preferences *model.Preferences) error + DeleteSidebarChannelsByPreferences(preferences *model.Preferences) error DeleteSidebarCategory(categoryId string) *model.AppError GetAllChannelsForExportAfter(limit int, afterId string) ([]*model.ChannelForExport, *model.AppError) GetAllDirectChannelsForExportAfter(limit int, afterId string) ([]*model.DirectChannelForExport, *model.AppError) diff --git a/store/storetest/channel_store.go b/store/storetest/channel_store.go index a3b06a6eb5..a8084467cd 100644 --- a/store/storetest/channel_store.go +++ b/store/storetest/channel_store.go @@ -7436,6 +7436,105 @@ func testUpdateSidebarCategories(t *testing.T, ss store.Store, s SqlSupplier) { assert.Nil(t, res) }) + t.Run("should add and remove favorites preferences, even if the channel is already favorited in preferences", func(t *testing.T) { + userId := model.NewId() + teamId := model.NewId() + teamId2 := model.NewId() + + // Create the initial categories and find the favorites categories in each team + nErr := ss.Channel().CreateInitialSidebarCategories(userId, teamId) + require.Nil(t, nErr) + + categories, err := ss.Channel().GetSidebarCategories(userId, teamId) + require.Nil(t, err) + + favoritesCategory := categories.Categories[0] + require.Equal(t, model.SidebarCategoryFavorites, favoritesCategory.Type) + + nErr = ss.Channel().CreateInitialSidebarCategories(userId, teamId2) + require.Nil(t, nErr) + + categories2, err := ss.Channel().GetSidebarCategories(userId, teamId2) + require.Nil(t, err) + + favoritesCategory2 := categories2.Categories[0] + require.Equal(t, model.SidebarCategoryFavorites, favoritesCategory2.Type) + + // Create a direct channel + otherUserId := model.NewId() + + dmChannel, nErr := ss.Channel().SaveDirectChannel( + &model.Channel{ + Name: model.GetDMNameFromIds(userId, otherUserId), + Type: model.CHANNEL_DIRECT, + }, + &model.ChannelMember{ + UserId: userId, + NotifyProps: model.GetDefaultChannelNotifyProps(), + }, + &model.ChannelMember{ + UserId: otherUserId, + NotifyProps: model.GetDefaultChannelNotifyProps(), + }, + ) + assert.Nil(t, nErr) + + // Assign it to favorites on the first team. The favorites preference gets set for all teams. + _, err = ss.Channel().UpdateSidebarCategories(userId, teamId, []*model.SidebarCategoryWithChannels{ + { + SidebarCategory: favoritesCategory.SidebarCategory, + Channels: []string{dmChannel.Id}, + }, + }) + assert.Nil(t, err) + + res, err := ss.Preference().Get(userId, model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, dmChannel.Id) + assert.Nil(t, err) + assert.NotNil(t, res) + assert.Equal(t, "true", res.Value) + + // Assign it to favorites on the second team. The favorites preference is already set. + updated, err := ss.Channel().UpdateSidebarCategories(userId, teamId, []*model.SidebarCategoryWithChannels{ + { + SidebarCategory: favoritesCategory2.SidebarCategory, + Channels: []string{dmChannel.Id}, + }, + }) + assert.Nil(t, err) + assert.Equal(t, []string{dmChannel.Id}, updated[0].Channels) + + res, err = ss.Preference().Get(userId, model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, dmChannel.Id) + assert.Nil(t, err) + assert.NotNil(t, res) + assert.Equal(t, "true", res.Value) + + // Remove it from favorites on the first team. This clears the favorites preference for all teams. + _, err = ss.Channel().UpdateSidebarCategories(userId, teamId, []*model.SidebarCategoryWithChannels{ + { + SidebarCategory: favoritesCategory.SidebarCategory, + Channels: []string{}, + }, + }) + assert.Nil(t, err) + + res, err = ss.Preference().Get(userId, model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, dmChannel.Id) + assert.Equal(t, sql.ErrNoRows.Error(), err.DetailedError) + assert.Nil(t, res) + + // Remove it from favorites on the second team. The favorites preference was already deleted. + _, err = ss.Channel().UpdateSidebarCategories(userId, teamId2, []*model.SidebarCategoryWithChannels{ + { + SidebarCategory: favoritesCategory2.SidebarCategory, + Channels: []string{}, + }, + }) + assert.Nil(t, err) + + res, err = ss.Preference().Get(userId, model.PREFERENCE_CATEGORY_FAVORITE_CHANNEL, dmChannel.Id) + assert.Equal(t, sql.ErrNoRows.Error(), err.DetailedError) + assert.Nil(t, res) + }) + t.Run("should not affect other users' favorites preferences", func(t *testing.T) { userId := model.NewId() teamId := model.NewId() diff --git a/store/storetest/mocks/ChannelStore.go b/store/storetest/mocks/ChannelStore.go index a75789b077..b6c5d7a3d1 100644 --- a/store/storetest/mocks/ChannelStore.go +++ b/store/storetest/mocks/ChannelStore.go @@ -263,6 +263,20 @@ func (_m *ChannelStore) DeleteSidebarCategory(categoryId string) *model.AppError return r0 } +// DeleteSidebarChannelsByPreferences provides a mock function with given fields: preferences +func (_m *ChannelStore) DeleteSidebarChannelsByPreferences(preferences *model.Preferences) error { + ret := _m.Called(preferences) + + var r0 error + if rf, ok := ret.Get(0).(func(*model.Preferences) error); ok { + r0 = rf(preferences) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // Get provides a mock function with given fields: id, allowFromCache func (_m *ChannelStore) Get(id string, allowFromCache bool) (*model.Channel, error) { ret := _m.Called(id, allowFromCache) @@ -2072,16 +2086,14 @@ func (_m *ChannelStore) UpdateSidebarChannelCategoryOnMove(channel *model.Channe } // UpdateSidebarChannelsByPreferences provides a mock function with given fields: preferences -func (_m *ChannelStore) UpdateSidebarChannelsByPreferences(preferences *model.Preferences) *model.AppError { +func (_m *ChannelStore) UpdateSidebarChannelsByPreferences(preferences *model.Preferences) error { ret := _m.Called(preferences) - var r0 *model.AppError - if rf, ok := ret.Get(0).(func(*model.Preferences) *model.AppError); ok { + var r0 error + if rf, ok := ret.Get(0).(func(*model.Preferences) error); ok { r0 = rf(preferences) } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*model.AppError) - } + r0 = ret.Error(0) } return r0 diff --git a/store/timer_layer.go b/store/timer_layer.go index 6ea4af21e3..5aca686f77 100644 --- a/store/timer_layer.go +++ b/store/timer_layer.go @@ -664,6 +664,22 @@ func (s *TimerLayerChannelStore) DeleteSidebarCategory(categoryId string) *model return resultVar0 } +func (s *TimerLayerChannelStore) DeleteSidebarChannelsByPreferences(preferences *model.Preferences) error { + start := timemodule.Now() + + resultVar0 := s.ChannelStore.DeleteSidebarChannelsByPreferences(preferences) + + elapsed := float64(timemodule.Since(start)) / float64(timemodule.Second) + if s.Root.Metrics != nil { + success := "false" + if resultVar0 == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("ChannelStore.DeleteSidebarChannelsByPreferences", success, elapsed) + } + return resultVar0 +} + func (s *TimerLayerChannelStore) Get(id string, allowFromCache bool) (*model.Channel, error) { start := timemodule.Now() @@ -2033,7 +2049,7 @@ func (s *TimerLayerChannelStore) UpdateSidebarChannelCategoryOnMove(channel *mod return resultVar0 } -func (s *TimerLayerChannelStore) UpdateSidebarChannelsByPreferences(preferences *model.Preferences) *model.AppError { +func (s *TimerLayerChannelStore) UpdateSidebarChannelsByPreferences(preferences *model.Preferences) error { start := timemodule.Now() resultVar0 := s.ChannelStore.UpdateSidebarChannelsByPreferences(preferences)