From 10a627fac1938d3d91d510082e750b47ccb3b016 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Wed, 19 Oct 2022 20:51:00 +0530 Subject: [PATCH] MM-45723: Include DM/GM channels for user indexing (#21391) There were 2 separate issues here: 1. We were excluding DM/GM channels for user indexing. Not sure why. This prevented users to be shown as channel members in autocomplete results in both Bleve and Elasticsearch. 2. We were incorrectly deciding to search across the team based on if the current channel is in the list of allowed channels or not. But we were incorrectly wiping the list of allowed channels even if a channelid was passed. This makes it work same as a DB search. One catch is that for the correct results to appear, a re-indexing is required. ```release-note Autocompletion results using Elasticsearch or Bleve will correctly show a user as a channel member in DM/GM channels. To force this change to appear, a re-indexing will be necessary. ``` --- store/searchlayer/user_layer.go | 23 +++++++++++++++++++---- store/sqlstore/user_store.go | 11 ++++++++++- store/storetest/user_store.go | 25 +++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 5 deletions(-) diff --git a/store/searchlayer/user_layer.go b/store/searchlayer/user_layer.go index 2c63dabd3a..268b18cacc 100644 --- a/store/searchlayer/user_layer.go +++ b/store/searchlayer/user_layer.go @@ -37,7 +37,7 @@ func (s *SearchUserStore) deleteUserIndex(user *model.User) { func (s *SearchUserStore) Search(teamId, term string, options *model.UserSearchOptions) ([]*model.User, error) { for _, engine := range s.rootStore.searchEngine.GetActiveEngines() { if engine.IsSearchEnabled() { - listOfAllowedChannels, nErr := s.getListOfAllowedChannelsForTeam(teamId, options.ViewRestrictions) + listOfAllowedChannels, nErr := s.getListOfAllowedChannels(teamId, "", options.ViewRestrictions) if nErr != nil { mlog.Warn("Encountered error on Search.", mlog.String("search_engine", engine.GetName()), mlog.Err(nErr)) continue @@ -148,7 +148,7 @@ func (s *SearchUserStore) autocompleteUsersInChannelByEngine(engine searchengine return autocomplete, nil } -// getListOfAllowedChannelsForTeam return the list of allowed channels to search user based on the +// getListOfAllowedChannels return the list of allowed channels to search user based on the // next scenarios: // - If there isn't view restrictions (team or channel) and no team id to filter them, then all // channels are allowed (nil return) @@ -159,7 +159,7 @@ func (s *SearchUserStore) autocompleteUsersInChannelByEngine(engine searchengine // - If we receive channels restrictions we get: // - If we don't have team id, we get those restricted channels (guest accounts and quick search) // - If we have a team id then we only return those restricted channels that belongs to that team -func (s *SearchUserStore) getListOfAllowedChannelsForTeam(teamId string, viewRestrictions *model.ViewUsersRestrictions) ([]string, error) { +func (s *SearchUserStore) getListOfAllowedChannels(teamId, channelId string, viewRestrictions *model.ViewUsersRestrictions) ([]string, error) { var listOfAllowedChannels []string if viewRestrictions == nil && teamId == "" { // nil return without error means all channels are allowed @@ -174,6 +174,20 @@ func (s *SearchUserStore) getListOfAllowedChannelsForTeam(teamId string, viewRes for _, channel := range channels { listOfAllowedChannels = append(listOfAllowedChannels, channel.Id) } + + if channelId != "" { + ch, err := s.rootStore.Channel().Get(channelId, true) + if err != nil { + return nil, errors.Wrapf(err, "failed to get channel with id: %s", channelId) + } + // Check if DM/GM channel, and add to the list. + // This is because GetTeamChannels does not return DM/GM channels. + // And since the channelId is passed from the API layer, it is already + // auth checked to confirm that the user has permission. + if ch.IsGroupOrDirect() { + listOfAllowedChannels = append(listOfAllowedChannels, channelId) + } + } return listOfAllowedChannels, nil } @@ -196,7 +210,7 @@ func (s *SearchUserStore) getListOfAllowedChannelsForTeam(teamId string, viewRes func (s *SearchUserStore) AutocompleteUsersInChannel(teamId, channelId, term string, options *model.UserSearchOptions) (*model.UserAutocompleteInChannel, error) { for _, engine := range s.rootStore.searchEngine.GetActiveEngines() { if engine.IsAutocompletionEnabled() { - listOfAllowedChannels, nErr := s.getListOfAllowedChannelsForTeam(teamId, options.ViewRestrictions) + listOfAllowedChannels, nErr := s.getListOfAllowedChannels(teamId, channelId, options.ViewRestrictions) if nErr != nil { mlog.Warn("Encountered error on AutocompleteUsersInChannel.", mlog.String("search_engine", engine.GetName()), mlog.Err(nErr)) continue @@ -205,6 +219,7 @@ func (s *SearchUserStore) AutocompleteUsersInChannel(teamId, channelId, term str return &model.UserAutocompleteInChannel{}, nil } options.ListOfAllowedChannels = listOfAllowedChannels + autocomplete, nErr := s.autocompleteUsersInChannelByEngine(engine, teamId, channelId, term, options) if nErr != nil { mlog.Warn("Encountered error on AutocompleteUsersInChannel.", mlog.String("search_engine", engine.GetName()), mlog.Err(nErr)) diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index e41d047157..9eb8535d90 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -1805,7 +1805,16 @@ func (us SqlUserStore) GetUsersBatchForIndexing(startTime int64, startFileID str `). From("ChannelMembers cm"). Join("Channels c ON cm.ChannelId = c.Id"). - Where(sq.Eq{"c.Type": model.ChannelTypeOpen, "cm.UserId": userIds}). + Where(sq.And{ + sq.Eq{ + "cm.UserId": userIds, + }, + sq.Or{ + sq.Eq{"c.Type": model.ChannelTypeOpen}, + sq.Eq{"c.Type": model.ChannelTypeDirect}, + sq.Eq{"c.Type": model.ChannelTypeGroup}, + }, + }). ToSql() if err != nil { return nil, errors.Wrap(err, "GetUsersBatchForIndexing_ToSql2") diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 3dd02d495a..c938056249 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -4912,10 +4912,35 @@ func testUserStoreGetUsersBatchForIndexing(t *testing.T, ss store.Store) { }) require.NoError(t, err) + cDM := &model.Channel{ + Name: model.NewId() + "__" + model.NewId(), + Type: model.ChannelTypeDirect, + } + cm1 := &model.ChannelMember{ + UserId: u3.Id, + ChannelId: cDM.Id, + NotifyProps: model.GetDefaultChannelNotifyProps(), + } + cm2 := &model.ChannelMember{ + UserId: u2.Id, + ChannelId: cDM.Id, + NotifyProps: model.GetDefaultChannelNotifyProps(), + } + cDM, nErr = ss.Channel().SaveDirectChannel(cDM, cm1, cm2) + require.NoError(t, nErr) + // Getting all users res1List, err := ss.User().GetUsersBatchForIndexing(u1.CreateAt-1, "", 100) require.NoError(t, err) assert.Len(t, res1List, 3) + for _, user := range res1List { + switch user.Id { + case u2.Id: + assert.ElementsMatch(t, user.ChannelsIds, []string{cPub1.Id, cPub2.Id, cDM.Id}) + case u3.Id: + assert.ElementsMatch(t, user.ChannelsIds, []string{cPub2.Id, cDM.Id}) + } + } // Testing pagination res2List, err := ss.User().GetUsersBatchForIndexing(u1.CreateAt-1, "", 1)