From 49e0473753c2e4e2e02e30c17a7793657b71363f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Thu, 27 Sep 2018 16:15:41 +0200 Subject: MM-11567: Autocomplete search in: for DMs and GMs (#9430) * MM-11567: Autocomplete search in: for DMs and GMs * Adding unit tests * Allowing to search Direct Messages in the autocompletion * Fix it in TE --- api4/channel_test.go | 137 +++++++++++++++++++++++++-- app/post.go | 54 ++++++++++- store/sqlstore/channel_store.go | 71 +++++++++++--- store/sqlstore/channel_store_experimental.go | 19 ++-- store/storetest/channel_store.go | 33 ++++++- 5 files changed, 280 insertions(+), 34 deletions(-) diff --git a/api4/channel_test.go b/api4/channel_test.go index 2aec90aea..918b37c0f 100644 --- a/api4/channel_test.go +++ b/api4/channel_test.go @@ -2109,9 +2109,11 @@ func TestAutocompleteChannels(t *testing.T) { []string{"town"}, }, } { - if channels, resp := th.Client.AutocompleteChannelsForTeam(tc.teamId, tc.fragment); resp.Error != nil { - t.Fatal("Test case " + tc.description + " failed. Err: " + resp.Error.Error()) - } else { + t.Run(tc.description, func(t *testing.T) { + channels, resp := th.Client.AutocompleteChannelsForTeam(tc.teamId, tc.fragment) + if resp.Error != nil { + t.Fatal("Err: " + resp.Error.Error()) + } for _, expectedInclude := range tc.expectedIncludes { found := false for _, channel := range *channels { @@ -2121,17 +2123,140 @@ func TestAutocompleteChannels(t *testing.T) { } } if !found { - t.Fatal("Test case " + tc.description + " failed. Expected but didn't find channel: " + expectedInclude) + t.Fatal("Expected but didn't find channel: " + expectedInclude) } } for _, expectedExclude := range tc.expectedExcludes { for _, channel := range *channels { if channel.Name == expectedExclude { - t.Fatal("Test case " + tc.description + " failed. Found channel we didn't want: " + expectedExclude) + t.Fatal("Found channel we didn't want: " + expectedExclude) } } } - } + }) + } +} + +func TestAutocompleteChannelsForSearch(t *testing.T) { + th := Setup().InitBasic().InitSystemAdmin() + defer th.TearDown() + + th.LoginSystemAdminWithClient(th.SystemAdminClient) + th.LoginBasicWithClient(th.Client) + + u1 := th.CreateUserWithClient(th.SystemAdminClient) + u2 := th.CreateUserWithClient(th.SystemAdminClient) + u3 := th.CreateUserWithClient(th.SystemAdminClient) + u4 := th.CreateUserWithClient(th.SystemAdminClient) + + // A private channel to make sure private channels are not used + utils.DisableDebugLogForTest() + ptown, _ := th.SystemAdminClient.CreateChannel(&model.Channel{ + DisplayName: "Town", + Name: "town", + Type: model.CHANNEL_PRIVATE, + TeamId: th.BasicTeam.Id, + }) + defer func() { + th.Client.DeleteChannel(ptown.Id) + }() + mypriv, _ := th.Client.CreateChannel(&model.Channel{ + DisplayName: "My private town", + Name: "townpriv", + Type: model.CHANNEL_PRIVATE, + TeamId: th.BasicTeam.Id, + }) + defer func() { + th.Client.DeleteChannel(mypriv.Id) + }() + utils.EnableDebugLogForTest() + + dc1, resp := th.Client.CreateDirectChannel(th.BasicUser.Id, u1.Id) + CheckNoError(t, resp) + defer func() { + th.Client.DeleteChannel(dc1.Id) + }() + + dc2, resp := th.SystemAdminClient.CreateDirectChannel(u2.Id, u3.Id) + CheckNoError(t, resp) + defer func() { + th.SystemAdminClient.DeleteChannel(dc2.Id) + }() + + gc1, resp := th.Client.CreateGroupChannel([]string{th.BasicUser.Id, u2.Id, u3.Id}) + CheckNoError(t, resp) + defer func() { + th.Client.DeleteChannel(gc1.Id) + }() + + gc2, resp := th.SystemAdminClient.CreateGroupChannel([]string{u2.Id, u3.Id, u4.Id}) + CheckNoError(t, resp) + defer func() { + th.SystemAdminClient.DeleteChannel(gc2.Id) + }() + + for _, tc := range []struct { + description string + teamId string + fragment string + expectedIncludes []string + expectedExcludes []string + }{ + { + "Basic town-square", + th.BasicTeam.Id, + "town", + []string{"town-square", "townpriv"}, + []string{"off-topic", "town"}, + }, + { + "Basic off-topic", + th.BasicTeam.Id, + "off-to", + []string{"off-topic"}, + []string{"town-square", "town", "townpriv"}, + }, + { + "Basic town square and off topic", + th.BasicTeam.Id, + "to", + []string{"off-topic", "town-square", "townpriv"}, + []string{"town"}, + }, + { + "Direct and group messages", + th.BasicTeam.Id, + "fakeuser", + []string{dc1.Name, gc1.Name}, + []string{dc2.Name, gc2.Name}, + }, + } { + t.Run(tc.description, func(t *testing.T) { + channels, resp := th.Client.AutocompleteChannelsForTeamForSearch(tc.teamId, tc.fragment) + if resp.Error != nil { + t.Fatal("Err: " + resp.Error.Error()) + } + for _, expectedInclude := range tc.expectedIncludes { + found := false + for _, channel := range *channels { + if channel.Name == expectedInclude { + found = true + break + } + } + if !found { + t.Fatal("Expected but didn't find channel: " + expectedInclude + " Channels: " + fmt.Sprintf("%v", channels)) + } + } + + for _, expectedExclude := range tc.expectedExcludes { + for _, channel := range *channels { + if channel.Name == expectedExclude { + t.Fatal("Found channel we didn't want: " + expectedExclude) + } + } + } + }) } } diff --git a/app/post.go b/app/post.go index fc941a68f..da6f7ac2b 100644 --- a/app/post.go +++ b/app/post.go @@ -616,6 +616,43 @@ func (a *App) DeletePostFiles(post *model.Post) { } } +func (a *App) parseAndFetchChannelIdByNameFromInFilter(channelName, userId, teamId string, includeDeleted bool) (*model.Channel, error) { + if strings.HasPrefix(channelName, "@") && strings.Contains(channelName, ",") { + var userIds []string + users, err := a.GetUsersByUsernames(strings.Split(channelName[1:], ","), false) + if err != nil { + return nil, err + } + for _, user := range users { + userIds = append(userIds, user.Id) + } + + channel, err := a.GetGroupChannel(userIds) + if err != nil { + return nil, err + } + return channel, nil + } + + if strings.HasPrefix(channelName, "@") && !strings.Contains(channelName, ",") { + user, err := a.GetUserByUsername(channelName[1:]) + if err != nil { + return nil, err + } + channel, err := a.GetDirectChannel(userId, user.Id) + if err != nil { + return nil, err + } + return channel, nil + } + + channel, err := a.GetChannelByName(channelName, teamId, includeDeleted) + if err != nil { + return nil, err + } + return channel, nil +} + func (a *App) SearchPostsInTeam(terms string, userId string, teamId string, isOrSearch bool, includeDeletedChannels bool, timeZoneOffset int, page, perPage int) (*model.PostSearchResults, *model.AppError) { paramsList := model.ParseSearchParams(terms, timeZoneOffset) includeDeleted := includeDeletedChannels && *a.Config().TeamSettings.ExperimentalViewArchivedChannels @@ -630,11 +667,12 @@ func (a *App) SearchPostsInTeam(terms string, userId string, teamId string, isOr if params.Terms != "*" { // Convert channel names to channel IDs for idx, channelName := range params.InChannels { - if channel, err := a.GetChannelByName(channelName, teamId, includeDeleted); err != nil { + channel, err := a.parseAndFetchChannelIdByNameFromInFilter(channelName, userId, teamId, includeDeletedChannels) + if err != nil { mlog.Error(fmt.Sprint(err)) - } else { - params.InChannels[idx] = channel.Id + continue } + params.InChannels[idx] = channel.Id } // Convert usernames to user IDs @@ -695,6 +733,16 @@ func (a *App) SearchPostsInTeam(terms string, userId string, teamId string, isOr params.OrTerms = isOrSearch // don't allow users to search for everything if params.Terms != "*" { + for idx, channelName := range params.InChannels { + if strings.HasPrefix(channelName, "@") { + channel, err := a.parseAndFetchChannelIdByNameFromInFilter(channelName, userId, teamId, includeDeletedChannels) + if err != nil { + mlog.Error(fmt.Sprint(err)) + continue + } + params.InChannels[idx] = channel.Name + } + } channels = append(channels, a.Srv.Store.Post().Search(teamId, userId, params)) } } diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index b4d3bd8fa..7a9e4e687 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -1691,14 +1691,14 @@ func (s SqlChannelStore) AutocompleteInTeam(teamId string, term string, includeD var channels model.ChannelList - if likeClause, likeTerm := s.buildLIKEClause(term); likeClause == "" { + if likeClause, likeTerm := s.buildLIKEClause(term, "Name, DisplayName, Purpose"); likeClause == "" { if _, err := s.GetReplica().Select(&channels, fmt.Sprintf(queryFormat, ""), map[string]interface{}{"TeamId": teamId}); err != nil { result.Err = model.NewAppError("SqlChannelStore.AutocompleteInTeam", "store.sql_channel.search.app_error", nil, "term="+term+", "+", "+err.Error(), http.StatusInternalServerError) } } else { // Using a UNION results in index_merge and fulltext queries and is much faster than the ref // query you would get using an OR of the LIKE and full-text clauses. - fulltextClause, fulltextTerm := s.buildFulltextClause(term) + fulltextClause, fulltextTerm := s.buildFulltextClause(term, "Name, DisplayName, Purpose") likeQuery := fmt.Sprintf(queryFormat, "AND "+likeClause) fulltextQuery := fmt.Sprintf(queryFormat, "AND "+fulltextClause) query := fmt.Sprintf("(%v) UNION (%v) LIMIT 50", likeQuery, fulltextQuery) @@ -1730,7 +1730,7 @@ func (s SqlChannelStore) AutocompleteInTeamForSearch(teamId string, userId strin JOIN ChannelMembers AS CM ON CM.ChannelId = C.Id WHERE - C.TeamId = :TeamId + (C.TeamId = :TeamId OR (C.TeamId = '' AND C.Type = 'G')) AND CM.UserId = :UserId ` + deleteFilter + ` %v @@ -1738,14 +1738,14 @@ func (s SqlChannelStore) AutocompleteInTeamForSearch(teamId string, userId strin var channels model.ChannelList - if likeClause, likeTerm := s.buildLIKEClause(term); likeClause == "" { + if likeClause, likeTerm := s.buildLIKEClause(term, "Name, DisplayName, Purpose"); likeClause == "" { if _, err := s.GetReplica().Select(&channels, fmt.Sprintf(queryFormat, ""), map[string]interface{}{"TeamId": teamId, "UserId": userId}); err != nil { result.Err = model.NewAppError("SqlChannelStore.AutocompleteInTeamForSearch", "store.sql_channel.search.app_error", nil, "term="+term+", "+", "+err.Error(), http.StatusInternalServerError) } } else { // Using a UNION results in index_merge and fulltext queries and is much faster than the ref // query you would get using an OR of the LIKE and full-text clauses. - fulltextClause, fulltextTerm := s.buildFulltextClause(term) + fulltextClause, fulltextTerm := s.buildFulltextClause(term, "Name, DisplayName, Purpose") likeQuery := fmt.Sprintf(queryFormat, "AND "+likeClause) fulltextQuery := fmt.Sprintf(queryFormat, "AND "+fulltextClause) query := fmt.Sprintf("(%v) UNION (%v) LIMIT 50", likeQuery, fulltextQuery) @@ -1755,6 +1755,14 @@ func (s SqlChannelStore) AutocompleteInTeamForSearch(teamId string, userId strin } } + directChannels, err := s.autocompleteInTeamForSearchDirectMessages(userId, term) + if err != nil { + result.Err = err + return + } + + channels = append(channels, directChannels...) + sort.Slice(channels, func(a, b int) bool { return strings.ToLower(channels[a].DisplayName) < strings.ToLower(channels[b].DisplayName) }) @@ -1762,6 +1770,48 @@ func (s SqlChannelStore) AutocompleteInTeamForSearch(teamId string, userId strin }) } +func (s SqlChannelStore) autocompleteInTeamForSearchDirectMessages(userId string, term string) ([]*model.Channel, *model.AppError) { + queryFormat := ` + SELECT + C.*, + OtherUsers.Username as DisplayName + FROM + Channels AS C + JOIN + ChannelMembers AS CM ON CM.ChannelId = C.Id + INNER JOIN ( + SELECT + ICM.ChannelId AS ChannelId, IU.Username AS Username + FROM + Users as IU + JOIN + ChannelMembers AS ICM ON ICM.UserId = IU.Id + WHERE + IU.Id != :UserId + %v + ) AS OtherUsers ON OtherUsers.ChannelId = C.Id + WHERE + C.Type = 'D' + AND CM.UserId = :UserId + LIMIT 50` + + var channels model.ChannelList + + if likeClause, likeTerm := s.buildLIKEClause(term, "IU.Username, IU.Nickname"); likeClause == "" { + if _, err := s.GetReplica().Select(&channels, fmt.Sprintf(queryFormat, ""), map[string]interface{}{"UserId": userId}); err != nil { + return nil, model.NewAppError("SqlChannelStore.AutocompleteInTeamForSearch", "store.sql_channel.search.app_error", nil, "term="+term+", "+", "+err.Error(), http.StatusInternalServerError) + } + } else { + query := fmt.Sprintf(queryFormat, "AND "+likeClause) + + if _, err := s.GetReplica().Select(&channels, query, map[string]interface{}{"UserId": userId, "LikeTerm": likeTerm}); err != nil { + return nil, model.NewAppError("SqlChannelStore.AutocompleteInTeamForSearch", "store.sql_channel.search.app_error", nil, "term="+term+", "+", "+err.Error(), http.StatusInternalServerError) + } + } + + return channels, nil +} + func (s SqlChannelStore) SearchInTeam(teamId string, term string, includeDeleted bool) store.StoreChannel { return store.Do(func(result *store.StoreResult) { deleteFilter := "AND DeleteAt = 0" @@ -1814,9 +1864,8 @@ func (s SqlChannelStore) SearchMore(userId string, teamId string, term string) s }) } -func (s SqlChannelStore) buildLIKEClause(term string) (likeClause, likeTerm string) { +func (s SqlChannelStore) buildLIKEClause(term string, searchColumns string) (likeClause, likeTerm string) { likeTerm = term - searchColumns := "Name, DisplayName, Purpose" // These chars must be removed from the like query. for _, c := range ignoreLikeSearchChar { @@ -1847,12 +1896,10 @@ func (s SqlChannelStore) buildLIKEClause(term string) (likeClause, likeTerm stri return } -func (s SqlChannelStore) buildFulltextClause(term string) (fulltextClause, fulltextTerm string) { +func (s SqlChannelStore) buildFulltextClause(term string, searchColumns string) (fulltextClause, fulltextTerm string) { // Copy the terms as we will need to prepare them differently for each search type. fulltextTerm = term - searchColumns := "Name, DisplayName, Purpose" - // These chars must be treated as spaces in the fulltext query. for _, c := range spaceFulltextSearchChar { fulltextTerm = strings.Replace(fulltextTerm, c, " ", -1) @@ -1891,13 +1938,13 @@ func (s SqlChannelStore) buildFulltextClause(term string) (fulltextClause, fullt func (s SqlChannelStore) performSearch(searchQuery string, term string, parameters map[string]interface{}) store.StoreResult { result := store.StoreResult{} - likeClause, likeTerm := s.buildLIKEClause(term) + likeClause, likeTerm := s.buildLIKEClause(term, "Name, DisplayName, Purpose") if likeTerm == "" { // If the likeTerm is empty after preparing, then don't bother searching. searchQuery = strings.Replace(searchQuery, "SEARCH_CLAUSE", "", 1) } else { parameters["LikeTerm"] = likeTerm - fulltextClause, fulltextTerm := s.buildFulltextClause(term) + fulltextClause, fulltextTerm := s.buildFulltextClause(term, "Name, DisplayName, Purpose") parameters["FulltextTerm"] = fulltextTerm searchQuery = strings.Replace(searchQuery, "SEARCH_CLAUSE", "AND ("+likeClause+" OR "+fulltextClause+")", 1) } diff --git a/store/sqlstore/channel_store_experimental.go b/store/sqlstore/channel_store_experimental.go index 7cf142aba..164622654 100644 --- a/store/sqlstore/channel_store_experimental.go +++ b/store/sqlstore/channel_store_experimental.go @@ -555,14 +555,14 @@ func (s SqlChannelStoreExperimental) AutocompleteInTeam(teamId string, term stri var channels model.ChannelList - if likeClause, likeTerm := s.buildLIKEClause(term); likeClause == "" { + if likeClause, likeTerm := s.buildLIKEClause(term, "c.Name, c.DisplayName, c.Purpose"); likeClause == "" { if _, err := s.GetReplica().Select(&channels, fmt.Sprintf(queryFormat, ""), map[string]interface{}{"TeamId": teamId}); err != nil { result.Err = model.NewAppError("SqlChannelStore.AutocompleteInTeam", "store.sql_channel.search.app_error", nil, "term="+term+", "+", "+err.Error(), http.StatusInternalServerError) } } else { // Using a UNION results in index_merge and fulltext queries and is much faster than the ref // query you would get using an OR of the LIKE and full-text clauses. - fulltextClause, fulltextTerm := s.buildFulltextClause(term) + fulltextClause, fulltextTerm := s.buildFulltextClause(term, "c.Name, c.DisplayName, c.Purpose") likeQuery := fmt.Sprintf(queryFormat, "AND "+likeClause) fulltextQuery := fmt.Sprintf(queryFormat, "AND "+fulltextClause) query := fmt.Sprintf("(%v) UNION (%v) LIMIT 50", likeQuery, fulltextQuery) @@ -647,13 +647,12 @@ func (s SqlChannelStoreExperimental) SearchMore(userId string, teamId string, te }) } -func (s SqlChannelStoreExperimental) buildLIKEClause(term string) (likeClause, likeTerm string) { +func (s SqlChannelStoreExperimental) buildLIKEClause(term string, searchColumns string) (likeClause, likeTerm string) { if !s.IsExperimentalPublicChannelsMaterializationEnabled() { - return s.SqlChannelStore.buildLIKEClause(term) + return s.SqlChannelStore.buildLIKEClause(term, searchColumns) } likeTerm = term - searchColumns := "c.Name, c.DisplayName, c.Purpose" // These chars must be removed from the like query. for _, c := range ignoreLikeSearchChar { @@ -684,16 +683,14 @@ func (s SqlChannelStoreExperimental) buildLIKEClause(term string) (likeClause, l return } -func (s SqlChannelStoreExperimental) buildFulltextClause(term string) (fulltextClause, fulltextTerm string) { +func (s SqlChannelStoreExperimental) buildFulltextClause(term string, searchColumns string) (fulltextClause, fulltextTerm string) { if !s.IsExperimentalPublicChannelsMaterializationEnabled() { - return s.SqlChannelStore.buildFulltextClause(term) + return s.SqlChannelStore.buildFulltextClause(term, searchColumns) } // Copy the terms as we will need to prepare them differently for each search type. fulltextTerm = term - searchColumns := "c.Name, c.DisplayName, c.Purpose" - // These chars must be treated as spaces in the fulltext query. for _, c := range spaceFulltextSearchChar { fulltextTerm = strings.Replace(fulltextTerm, c, " ", -1) @@ -736,13 +733,13 @@ func (s SqlChannelStoreExperimental) performSearch(searchQuery string, term stri result := store.StoreResult{} - likeClause, likeTerm := s.buildLIKEClause(term) + likeClause, likeTerm := s.buildLIKEClause(term, "c.Name, c.DisplayName, c.Purpose") if likeTerm == "" { // If the likeTerm is empty after preparing, then don't bother searching. searchQuery = strings.Replace(searchQuery, "SEARCH_CLAUSE", "", 1) } else { parameters["LikeTerm"] = likeTerm - fulltextClause, fulltextTerm := s.buildFulltextClause(term) + fulltextClause, fulltextTerm := s.buildFulltextClause(term, "c.Name, c.DisplayName, c.Purpose") parameters["FulltextTerm"] = fulltextTerm searchQuery = strings.Replace(searchQuery, "SEARCH_CLAUSE", "AND ("+likeClause+" OR "+fulltextClause+")", 1) } diff --git a/store/storetest/channel_store.go b/store/storetest/channel_store.go index 636d96649..7cb86242f 100644 --- a/store/storetest/channel_store.go +++ b/store/storetest/channel_store.go @@ -2023,6 +2023,30 @@ func testChannelStoreSearchInTeam(t *testing.T, ss store.Store) { } func testChannelStoreAutocompleteInTeamForSearch(t *testing.T, ss store.Store) { + u1 := &model.User{} + u1.Email = MakeEmail() + u1.Username = "user1" + model.NewId() + u1.Nickname = model.NewId() + store.Must(ss.User().Save(u1)) + + u2 := &model.User{} + u2.Email = MakeEmail() + u2.Username = "user2" + model.NewId() + u2.Nickname = model.NewId() + store.Must(ss.User().Save(u2)) + + u3 := &model.User{} + u3.Email = MakeEmail() + u3.Username = "user3" + model.NewId() + u3.Nickname = model.NewId() + store.Must(ss.User().Save(u3)) + + u4 := &model.User{} + u4.Email = MakeEmail() + u4.Username = "user4" + model.NewId() + u4.Nickname = model.NewId() + store.Must(ss.User().Save(u4)) + o1 := model.Channel{} o1.TeamId = model.NewId() o1.DisplayName = "ChannelA" @@ -2032,7 +2056,7 @@ func testChannelStoreAutocompleteInTeamForSearch(t *testing.T, ss store.Store) { m1 := model.ChannelMember{} m1.ChannelId = o1.Id - m1.UserId = model.NewId() + m1.UserId = u1.Id m1.NotifyProps = model.GetDefaultChannelNotifyProps() store.Must(ss.Channel().SaveMember(&m1)) @@ -2084,17 +2108,22 @@ func testChannelStoreAutocompleteInTeamForSearch(t *testing.T, ss store.Store) { o5.Type = model.CHANNEL_PRIVATE store.Must(ss.Channel().Save(&o5, -1)) + store.Must(ss.Channel().CreateDirectChannel(u1.Id, u2.Id)) + store.Must(ss.Channel().CreateDirectChannel(u2.Id, u3.Id)) + tt := []struct { name string term string includeDeleted bool expectedMatches int }{ - {"Empty search (list all)", "", false, 3}, + {"Empty search (list all)", "", false, 4}, {"Narrow search", "ChannelA", false, 2}, {"Wide search", "Cha", false, 3}, + {"Direct messages", "user", false, 1}, {"Wide search with archived channels", "Cha", true, 4}, {"Narrow with archived channels", "ChannelA", true, 3}, + {"Direct messages with archived channels", "user", true, 1}, {"Search without results", "blarg", true, 0}, } -- cgit v1.2.3-1-g7c22