Skip to content

Commit

Permalink
PLT-4535/PLT-4503 Fix inactive users in searches and add option funct…
Browse files Browse the repository at this point in the history 
…ionality to DB user search (#4413)

* Add options to user database search

* Fix inactive users showing up incorrectly in some user searches

* Read JSON for searchUsers API into anonymous struct

* Move anonymous struct to be a normal struct in model directory and upadte client to use it

* Added clarification comment about slightly odd query condition in search
  • Loading branch information
@jwilander
jwilander committed Nov 2, 2016
1 parent b45cc44 commit 137ade2
Show file tree
Hide file tree
Showing 14 changed files with 279 additions and 83 deletions.
37 changes: 20 additions & 17 deletions api/user.go
View file
Expand Up @@ -2592,33 +2592,36 @@ func sanitizeProfile(c *Context, user *model.User) *model.User {
}

func searchUsers(c *Context, w http.ResponseWriter, r *http.Request) {
props := model.MapFromJson(r.Body)
props := model.UserSearchFromJson(r.Body)
if props == nil {
c.SetInvalidParam("searchUsers", "")
return
}

term := props["term"]
if len(term) == 0 {
if len(props.Term) == 0 {
c.SetInvalidParam("searchUsers", "term")
return
}

teamId := props["team_id"]
inChannelId := props["in_channel"]
notInChannelId := props["not_in_channel"]

if inChannelId != "" && !HasPermissionToChannelContext(c, inChannelId, model.PERMISSION_READ_CHANNEL) {
if props.InChannelId != "" && !HasPermissionToChannelContext(c, props.InChannelId, model.PERMISSION_READ_CHANNEL) {
return
}

if notInChannelId != "" && !HasPermissionToChannelContext(c, notInChannelId, model.PERMISSION_READ_CHANNEL) {
if props.NotInChannelId != "" && !HasPermissionToChannelContext(c, props.NotInChannelId, model.PERMISSION_READ_CHANNEL) {
return
}

searchOptions := map[string]bool{}
searchOptions[store.USER_SEARCH_OPTION_USERNAME_ONLY] = true
searchOptions[store.USER_SEARCH_OPTION_ALLOW_INACTIVE] = props.AllowInactive

var uchan store.StoreChannel
if inChannelId != "" {
uchan = Srv.Store.User().SearchInChannel(inChannelId, term, store.USER_SEARCH_TYPE_USERNAME)
} else if notInChannelId != "" {
uchan = Srv.Store.User().SearchNotInChannel(teamId, notInChannelId, term, store.USER_SEARCH_TYPE_USERNAME)
if props.InChannelId != "" {
uchan = Srv.Store.User().SearchInChannel(props.InChannelId, props.Term, searchOptions)
} else if props.NotInChannelId != "" {
uchan = Srv.Store.User().SearchNotInChannel(props.TeamId, props.NotInChannelId, props.Term, searchOptions)
} else {
uchan = Srv.Store.User().Search(teamId, term, store.USER_SEARCH_TYPE_USERNAME)
uchan = Srv.Store.User().Search(props.TeamId, props.Term, searchOptions)
}

if result := <-uchan; result.Err != nil {
Expand Down Expand Up @@ -2674,8 +2677,8 @@ func autocompleteUsersInChannel(c *Context, w http.ResponseWriter, r *http.Reque
return
}

uchan := Srv.Store.User().SearchInChannel(channelId, term, store.USER_SEARCH_TYPE_ALL)
nuchan := Srv.Store.User().SearchNotInChannel(teamId, channelId, term, store.USER_SEARCH_TYPE_ALL)
uchan := Srv.Store.User().SearchInChannel(channelId, term, map[string]bool{})
nuchan := Srv.Store.User().SearchNotInChannel(teamId, channelId, term, map[string]bool{})

autocomplete := &model.UserAutocompleteInChannel{}

Expand Down Expand Up @@ -2720,7 +2723,7 @@ func autocompleteUsersInTeam(c *Context, w http.ResponseWriter, r *http.Request)
}
}

uchan := Srv.Store.User().Search(teamId, term, store.USER_SEARCH_TYPE_ALL)
uchan := Srv.Store.User().Search(teamId, term, map[string]bool{})

autocomplete := &model.UserAutocompleteInTeam{}

Expand Down
58 changes: 48 additions & 10 deletions api/user_test.go
View file
Expand Up @@ -2034,10 +2034,14 @@ func TestGetProfilesNotInChannel(t *testing.T) {
}

func TestSearchUsers(t *testing.T) {
th := Setup().InitBasic()
th := Setup().InitBasic().InitSystemAdmin()
Client := th.BasicClient

if result, err := Client.SearchUsers(th.BasicUser.Username, "", map[string]string{}); err != nil {
inactiveUser := th.CreateUser(Client)
LinkUserToTeam(inactiveUser, th.BasicTeam)
th.SystemAdminClient.Must(th.SystemAdminClient.UpdateActive(inactiveUser.Id, false))

if result, err := Client.SearchUsers(model.UserSearch{Term: th.BasicUser.Username}); err != nil {
t.Fatal(err)
} else {
users := result.Data.([]*model.User)
Expand All @@ -2054,7 +2058,41 @@ func TestSearchUsers(t *testing.T) {
}
}

if result, err := Client.SearchUsers(th.BasicUser.Username, "", map[string]string{"in_channel": th.BasicChannel.Id}); err != nil {
if result, err := Client.SearchUsers(model.UserSearch{Term: inactiveUser.Username, TeamId: th.BasicTeam.Id}); err != nil {
t.Fatal(err)
} else {
users := result.Data.([]*model.User)

found := false
for _, user := range users {
if user.Id == inactiveUser.Id {
found = true
}
}

if found {
t.Fatal("should not have found inactive user")
}
}

if result, err := Client.SearchUsers(model.UserSearch{Term: inactiveUser.Username, TeamId: th.BasicTeam.Id, AllowInactive: true}); err != nil {
t.Fatal(err)
} else {
users := result.Data.([]*model.User)

found := false
for _, user := range users {
if user.Id == inactiveUser.Id {
found = true
}
}

if !found {
t.Fatal("should have found inactive user")
}
}

if result, err := Client.SearchUsers(model.UserSearch{Term: th.BasicUser.Username, InChannelId: th.BasicChannel.Id}); err != nil {
t.Fatal(err)
} else {
users := result.Data.([]*model.User)
Expand All @@ -2075,7 +2113,7 @@ func TestSearchUsers(t *testing.T) {
}
}

if result, err := Client.SearchUsers(th.BasicUser2.Username, "", map[string]string{"not_in_channel": th.BasicChannel.Id}); err != nil {
if result, err := Client.SearchUsers(model.UserSearch{Term: th.BasicUser2.Username, NotInChannelId: th.BasicChannel.Id}); err != nil {
t.Fatal(err)
} else {
users := result.Data.([]*model.User)
Expand All @@ -2102,7 +2140,7 @@ func TestSearchUsers(t *testing.T) {
}
}

if result, err := Client.SearchUsers(th.BasicUser2.Username, th.BasicTeam.Id, map[string]string{"not_in_channel": th.BasicChannel.Id}); err != nil {
if result, err := Client.SearchUsers(model.UserSearch{Term: th.BasicUser2.Username, TeamId: th.BasicTeam.Id, NotInChannelId: th.BasicChannel.Id}); err != nil {
t.Fatal(err)
} else {
users := result.Data.([]*model.User)
Expand All @@ -2129,7 +2167,7 @@ func TestSearchUsers(t *testing.T) {
}
}

if result, err := Client.SearchUsers(th.BasicUser.Username, "junk", map[string]string{"not_in_channel": th.BasicChannel.Id}); err != nil {
if result, err := Client.SearchUsers(model.UserSearch{Term: th.BasicUser.Username, TeamId: "junk", NotInChannelId: th.BasicChannel.Id}); err != nil {
t.Fatal(err)
} else {
users := result.Data.([]*model.User)
Expand All @@ -2141,7 +2179,7 @@ func TestSearchUsers(t *testing.T) {

th.LoginBasic2()

if result, err := Client.SearchUsers(th.BasicUser.Username, "", map[string]string{}); err != nil {
if result, err := Client.SearchUsers(model.UserSearch{Term: th.BasicUser.Username}); err != nil {
t.Fatal(err)
} else {
users := result.Data.([]*model.User)
Expand All @@ -2158,15 +2196,15 @@ func TestSearchUsers(t *testing.T) {
}
}

if _, err := Client.SearchUsers("", "", map[string]string{}); err == nil {
if _, err := Client.SearchUsers(model.UserSearch{}); err == nil {
t.Fatal("should have errored - blank term")
}

if _, err := Client.SearchUsers(th.BasicUser.Username, "", map[string]string{"in_channel": th.BasicChannel.Id}); err == nil {
if _, err := Client.SearchUsers(model.UserSearch{Term: th.BasicUser.Username, InChannelId: th.BasicChannel.Id}); err == nil {
t.Fatal("should not have access")
}

if _, err := Client.SearchUsers(th.BasicUser.Username, "", map[string]string{"not_in_channel": th.BasicChannel.Id}); err == nil {
if _, err := Client.SearchUsers(model.UserSearch{Term: th.BasicUser.Username, NotInChannelId: th.BasicChannel.Id}); err == nil {
t.Fatal("should not have access")
}
}
Expand Down
6 changes: 2 additions & 4 deletions model/client.go
View file
Expand Up @@ -572,10 +572,8 @@ func (c *Client) GetProfilesByIds(userIds []string) (*Result, *AppError) {

// SearchUsers returns a list of users that have a username matching or similar to the search term. Must
// be authenticated.
func (c *Client) SearchUsers(term string, teamId string, options map[string]string) (*Result, *AppError) {
options["term"] = term
options["team_id"] = teamId
if r, err := c.DoApiPost("/users/search", MapToJson(options)); err != nil {
func (c *Client) SearchUsers(params UserSearch) (*Result, *AppError) {
if r, err := c.DoApiPost("/users/search", params.ToJson()); err != nil {
return nil, err
} else {
defer closeBody(r)
Expand Down
39 changes: 39 additions & 0 deletions model/user_search.go
View file
@@ -0,0 +1,39 @@
// Copyright (c) 2016 Mattermost, Inc. All Rights Reserved.
// See License.txt for license information.

package model

import (
"encoding/json"
"io"
)

type UserSearch struct {
Term string `json:"term"`
TeamId string `json:"team_id"`
InChannelId string `json:"in_channel_id"`
NotInChannelId string `json:"not_in_channel_id"`
AllowInactive bool `json:"allow_inactive"`
}

// ToJson convert a User to a json string
func (u *UserSearch) ToJson() string {
b, err := json.Marshal(u)
if err != nil {
return ""
} else {
return string(b)
}
}

// UserSearchFromJson will decode the input and return a User
func UserSearchFromJson(data io.Reader) *UserSearch {
decoder := json.NewDecoder(data)
var us UserSearch
err := decoder.Decode(&us)
if err == nil {
return &us
} else {
return nil
}
}
19 changes: 19 additions & 0 deletions model/user_search_test.go
View file
@@ -0,0 +1,19 @@
// Copyright (c) 2016 Mattermost, Inc. All Rights Reserved.
// See License.txt for license information.

package model

import (
"strings"
"testing"
)

func TestUserSearchJson(t *testing.T) {
userSearch := UserSearch{Term: NewId(), TeamId: NewId()}
json := userSearch.ToJson()
ruserSearch := UserSearchFromJson(strings.NewReader(json))

if userSearch.Term != ruserSearch.Term {
t.Fatal("Terms do not match")
}
}

0 comments on commit 137ade2

Please sign in to comment.