Merge pull request #1824 from hmhealey/plt1525

PLT-1525 Fixed ChannelInviteModal displaying some users that are already in the channel

api/channel.go

@@ -9,9 +9,14 @@ import (
 	"github.com/gorilla/mux"
 	"github.com/mattermost/platform/model"
 	"net/http"
+	"strconv"
 	"strings"
 )
 
+const (
+	defaultExtraMemberLimit = 100
+)
+
 func InitChannel(r *mux.Router) {
 	l4g.Debug("Initializing channel api routes")
 
@@ -27,6 +32,7 @@ func InitChannel(r *mux.Router) {
 	sr.Handle("/update_notify_props", ApiUserRequired(updateNotifyProps)).Methods("POST")
 	sr.Handle("/{id:[A-Za-z0-9]+}/", ApiUserRequiredActivity(getChannel, false)).Methods("GET")
 	sr.Handle("/{id:[A-Za-z0-9]+}/extra_info", ApiUserRequired(getChannelExtraInfo)).Methods("GET")
+	sr.Handle("/{id:[A-Za-z0-9]+}/extra_info/{member_limit:-?[0-9]+}", ApiUserRequired(getChannelExtraInfo)).Methods("GET")
 	sr.Handle("/{id:[A-Za-z0-9]+}/join", ApiUserRequired(join)).Methods("POST")
 	sr.Handle("/{id:[A-Za-z0-9]+}/leave", ApiUserRequired(leave)).Methods("POST")
 	sr.Handle("/{id:[A-Za-z0-9]+}/delete", ApiUserRequired(deleteChannel)).Methods("POST")
@@ -730,10 +736,19 @@ func getChannel(c *Context, w http.ResponseWriter, r *http.Request) {
 }
 
 func getChannelExtraInfo(c *Context, w http.ResponseWriter, r *http.Request) {
-
 	params := mux.Vars(r)
 	id := params["id"]
 
+	var memberLimit int
+	if memberLimitString, ok := params["member_limit"]; !ok {
+		memberLimit = defaultExtraMemberLimit
+	} else if memberLimitInt64, err := strconv.ParseInt(memberLimitString, 10, 0); err != nil {
+		c.Err = model.NewAppError("getChannelExtraInfo", "Failed to parse member limit", err.Error())
+		return
+	} else {
+		memberLimit = int(memberLimitInt64)
+	}
+
 	sc := Srv.Store.Channel().Get(id)
 	var channel *model.Channel
 	if cresult := <-sc; cresult.Err != nil {
@@ -743,13 +758,13 @@ func getChannelExtraInfo(c *Context, w http.ResponseWriter, r *http.Request) {
 		channel = cresult.Data.(*model.Channel)
 	}
 
-	extraEtag := channel.ExtraEtag()
+	extraEtag := channel.ExtraEtag(memberLimit)
 	if HandleEtag(extraEtag, w, r) {
 		return
 	}
 
 	scm := Srv.Store.Channel().GetMember(id, c.Session.UserId)
-	ecm := Srv.Store.Channel().GetExtraMembers(id, 100)
+	ecm := Srv.Store.Channel().GetExtraMembers(id, memberLimit)
 	ccm := Srv.Store.Channel().GetMemberCount(id)
 
 	if cmresult := <-scm; cmresult.Err != nil {

api/channel_benchmark_test.go

@@ -189,7 +189,7 @@ func BenchmarkGetChannelExtraInfo(b *testing.B) {
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
 		for j := range channels {
-			Client.Must(Client.GetChannelExtraInfo(channels[j].Id, ""))
+			Client.Must(Client.GetChannelExtraInfo(channels[j].Id, -1, ""))
 		}
 	}
 }

api/channel_test.go

@@ -674,7 +674,7 @@ func TestGetChannelExtraInfo(t *testing.T) {
 	channel1 := &model.Channel{DisplayName: "A Test API Name", Name: "a" + model.NewId() + "a", Type: model.CHANNEL_OPEN, TeamId: team.Id}
 	channel1 = Client.Must(Client.CreateChannel(channel1)).Data.(*model.Channel)
 
-	rget := Client.Must(Client.GetChannelExtraInfo(channel1.Id, ""))
+	rget := Client.Must(Client.GetChannelExtraInfo(channel1.Id, -1, ""))
 	data := rget.Data.(*model.ChannelExtra)
 	if data.Id != channel1.Id {
 		t.Fatal("couldnt't get extra info")
@@ -690,7 +690,7 @@ func TestGetChannelExtraInfo(t *testing.T) {
 
 	currentEtag := rget.Etag
 
-	if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, currentEtag); err != nil {
+	if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, -1, currentEtag); err != nil {
 		t.Fatal(err)
 	} else if cache_result.Data.(*model.ChannelExtra) != nil {
 		t.Log(cache_result.Data)
@@ -708,7 +708,7 @@ func TestGetChannelExtraInfo(t *testing.T) {
 	Client2.LoginByEmail(team.Name, user2.Email, "pwd")
 	Client2.Must(Client2.JoinChannel(channel1.Id))
 
-	if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, currentEtag); err != nil {
+	if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, -1, currentEtag); err != nil {
 		t.Fatal(err)
 	} else if cache_result.Data.(*model.ChannelExtra) == nil {
 		t.Log(cache_result.Data)
@@ -717,7 +717,7 @@ func TestGetChannelExtraInfo(t *testing.T) {
 		currentEtag = cache_result.Etag
 	}
 
-	if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, currentEtag); err != nil {
+	if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, -1, currentEtag); err != nil {
 		t.Fatal(err)
 	} else if cache_result.Data.(*model.ChannelExtra) != nil {
 		t.Log(cache_result.Data)
@@ -728,7 +728,7 @@ func TestGetChannelExtraInfo(t *testing.T) {
 
 	Client2.Must(Client2.LeaveChannel(channel1.Id))
 
-	if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, currentEtag); err != nil {
+	if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, -1, currentEtag); err != nil {
 		t.Fatal(err)
 	} else if cache_result.Data.(*model.ChannelExtra) == nil {
 		t.Log(cache_result.Data)
@@ -737,7 +737,7 @@ func TestGetChannelExtraInfo(t *testing.T) {
 		currentEtag = cache_result.Etag
 	}
 
-	if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, currentEtag); err != nil {
+	if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, -1, currentEtag); err != nil {
 		t.Fatal(err)
 	} else if cache_result.Data.(*model.ChannelExtra) != nil {
 		t.Log(cache_result.Data)
@@ -745,6 +745,42 @@ func TestGetChannelExtraInfo(t *testing.T) {
 	} else {
 		currentEtag = cache_result.Etag
 	}
+
+	Client2.Must(Client2.JoinChannel(channel1.Id))
+
+	if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, 2, currentEtag); err != nil {
+		t.Fatal(err)
+	} else if extra := cache_result.Data.(*model.ChannelExtra); extra == nil {
+		t.Fatal("response should not be empty")
+	} else if len(extra.Members) != 2 {
+		t.Fatal("should've returned 2 members")
+	} else if extra.MemberCount != 2 {
+		t.Fatal("should've returned member count of 2")
+	} else {
+		currentEtag = cache_result.Etag
+	}
+
+	if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, 1, currentEtag); err != nil {
+		t.Fatal(err)
+	} else if extra := cache_result.Data.(*model.ChannelExtra); extra == nil {
+		t.Fatal("response should not be empty")
+	} else if len(extra.Members) != 1 {
+		t.Fatal("should've returned only 1 member")
+	} else if extra.MemberCount != 2 {
+		t.Fatal("should've returned member count of 2")
+	} else {
+		currentEtag = cache_result.Etag
+	}
+
+	if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, 1, currentEtag); err != nil {
+		t.Fatal(err)
+	} else if cache_result.Data.(*model.ChannelExtra) != nil {
+		t.Log(cache_result.Data)
+		t.Fatal("response should be empty")
+	} else {
+		currentEtag = cache_result.Etag
+	}
+
 }
 
 func TestAddChannelMember(t *testing.T) {

model/channel.go

@@ -57,8 +57,8 @@ func (o *Channel) Etag() string {
 	return Etag(o.Id, o.UpdateAt)
 }
 
-func (o *Channel) ExtraEtag() string {
+func (o *Channel) ExtraEtag(memberLimit int) string {
-	return Etag(o.Id, o.ExtraUpdateAt)
+	return Etag(o.Id, o.ExtraUpdateAt, memberLimit)
 }
 
 func (o *Channel) IsValid() *AppError {

model/client.go

@@ -591,8 +591,8 @@ func (c *Client) UpdateLastViewedAt(channelId string) (*Result, *AppError) {
 	}
 }
 
-func (c *Client) GetChannelExtraInfo(id string, etag string) (*Result, *AppError) {
+func (c *Client) GetChannelExtraInfo(id string, memberLimit int, etag string) (*Result, *AppError) {
-	if r, err := c.DoApiGet("/channels/"+id+"/extra_info", "", etag); err != nil {
+	if r, err := c.DoApiGet("/channels/"+id+"/extra_info/"+strconv.FormatInt(int64(memberLimit), 10), "", etag); err != nil {
 		return nil, err
 	} else {
 		return &Result{r.Header.Get(HEADER_REQUEST_ID),

store/sql_channel_store.go

@@ -603,7 +603,14 @@ func (s SqlChannelStore) GetExtraMembers(channelId string, limit int) StoreChann
 		result := StoreResult{}
 
 		var members []model.ExtraMember
-		_, err := s.GetReplica().Select(&members, "SELECT Id, Nickname, Email, ChannelMembers.Roles, Username FROM ChannelMembers, Users WHERE ChannelMembers.UserId = Users.Id AND ChannelId = :ChannelId LIMIT :Limit", map[string]interface{}{"ChannelId": channelId, "Limit": limit})
+		var err error
+
+		if limit != -1 {
+			_, err = s.GetReplica().Select(&members, "SELECT Id, Nickname, Email, ChannelMembers.Roles, Username FROM ChannelMembers, Users WHERE ChannelMembers.UserId = Users.Id AND ChannelId = :ChannelId LIMIT :Limit", map[string]interface{}{"ChannelId": channelId, "Limit": limit})
+		} else {
+			_, err = s.GetReplica().Select(&members, "SELECT Id, Nickname, Email, ChannelMembers.Roles, Username FROM ChannelMembers, Users WHERE ChannelMembers.UserId = Users.Id AND ChannelId = :ChannelId", map[string]interface{}{"ChannelId": channelId})
+		}
+
 		if err != nil {
 			result.Err = model.NewAppError("SqlChannelStore.GetExtraMembers", "We couldn't get the extra info for channel members", "channel_id="+channelId+", "+err.Error())
 		} else {

web/react/components/channel_invite_modal.jsx

@@ -20,9 +20,14 @@ export default class ChannelInviteModal extends React.Component {
         this.onListenerChange = this.onListenerChange.bind(this);
         this.handleInvite = this.handleInvite.bind(this);
 
-        this.state = this.getStateFromStores();
+        // the state gets populated when the modal is shown
+        this.state = {};
     }
     shouldComponentUpdate(nextProps, nextState) {
+        if (!this.props.show && !nextProps.show) {
+            return false;
+        }
+
         if (!Utils.areObjectsEqual(this.props, nextProps)) {
             return true;
         }
@@ -34,13 +39,25 @@ export default class ChannelInviteModal extends React.Component {
         return false;
     }
     getStateFromStores() {
-        function getId(user) {
+        const users = UserStore.getActiveOnlyProfiles();
-            return user.id;
-        }
-        var users = UserStore.getActiveOnlyProfiles();
-        var memberIds = ChannelStore.getCurrentExtraInfo().members.map(getId);
 
-        var loading = $.isEmptyObject(users);
+        if ($.isEmptyObject(users)) {
+            return {
+                loading: true
+            };
+        }
+
+        // make sure we have all members of this channel before rendering
+        const extraInfo = ChannelStore.getCurrentExtraInfo();
+        if (extraInfo.member_count !== extraInfo.members.length) {
+            AsyncClient.getChannelExtraInfo(this.props.channel.id, -1);
+
+            return {
+                loading: true
+            };
+        }
+
+        const memberIds = extraInfo.members.map((user) => user.id);
 
         var nonmembers = [];
         for (var id in users) {
@@ -55,7 +72,7 @@ export default class ChannelInviteModal extends React.Component {
 
         return {
             nonmembers,
-            loading
+            loading: false
         };
     }
     onShow() {

web/react/components/channel_members_modal.jsx

@@ -1,6 +1,7 @@
 // Copyright (c) 2015 Mattermost, Inc. All Rights Reserved.
 // See License.txt for license information.
 
+import LoadingScreen from './loading_screen.jsx';
 import MemberList from './member_list.jsx';
 import ChannelInviteModal from './channel_invite_modal.jsx';
 
@@ -21,9 +22,10 @@ export default class ChannelMembersModal extends React.Component {
         this.onChange = this.onChange.bind(this);
         this.handleRemove = this.handleRemove.bind(this);
 
-        const state = this.getStateFromStores();
+        // the rest of the state gets populated when the modal is shown
-        state.showInviteModal = false;
+        this.state = {
-        this.state = state;
+            showInviteModal: false
+        };
     }
     shouldComponentUpdate(nextProps, nextState) {
         if (!Utils.areObjectsEqual(this.props, nextProps)) {
@@ -37,8 +39,18 @@ export default class ChannelMembersModal extends React.Component {
         return false;
     }
     getStateFromStores() {
+        const extraInfo = ChannelStore.getCurrentExtraInfo();
+
+        if (extraInfo.member_count !== extraInfo.members.length) {
+            AsyncClient.getChannelExtraInfo(this.props.channel.id, -1);
+
+            return {
+                loading: true
+            };
+        }
+
         const users = UserStore.getActiveOnlyProfiles();
-        const memberList = ChannelStore.getCurrentExtraInfo().members;
+        const memberList = extraInfo.members;
 
         const nonmemberList = [];
         for (const id in users) {
@@ -71,14 +83,14 @@ export default class ChannelMembersModal extends React.Component {
 
         return {
             nonmemberList,
-            memberList
+            memberList,
+            loading: false
         };
     }
     onShow() {
         if ($(window).width() > 768) {
             $(ReactDOM.findDOMNode(this.refs.modalBody)).perfectScrollbar();
         }
-        this.onChange();
     }
     componentDidUpdate(prevProps) {
         if (this.props.show && !prevProps.show) {
@@ -89,6 +101,8 @@ export default class ChannelMembersModal extends React.Component {
         if (!this.props.show && nextProps.show) {
             ChannelStore.addExtraInfoChangeListener(this.onChange);
             ChannelStore.addChangeListener(this.onChange);
+
+            this.onChange();
         } else if (this.props.show && !nextProps.show) {
             ChannelStore.removeExtraInfoChangeListener(this.onChange);
             ChannelStore.removeChangeListener(this.onChange);
@@ -154,6 +168,21 @@ export default class ChannelMembersModal extends React.Component {
             isAdmin = Utils.isAdmin(currentMember.roles) || Utils.isAdmin(UserStore.getCurrentUser().roles);
         }
 
+        let content;
+        if (this.state.loading) {
+            content = (<LoadingScreen />);
+        } else {
+            content = (
+                <div className='team-member-list'>
+                    <MemberList
+                        memberList={this.state.memberList}
+                        isAdmin={isAdmin}
+                        handleRemove={this.handleRemove}
+                    />
+                </div>
+            );
+        }
+
         return (
             <div>
                 <Modal
@@ -178,13 +207,7 @@ export default class ChannelMembersModal extends React.Component {
                         ref='modalBody'
                         style={{maxHeight}}
                     >
-                        <div className='team-member-list'>
+                        {content}
-                            <MemberList
-                                memberList={this.state.memberList}
-                                isAdmin={isAdmin}
-                                handleRemove={this.handleRemove}
-                            />
-                        </div>
                     </Modal.Body>
                     <Modal.Footer>
                         <button

web/react/utils/async_client.jsx

@@ -168,7 +168,7 @@ export function getMoreChannels(force) {
     }
 }
 
-export function getChannelExtraInfo(id) {
+export function getChannelExtraInfo(id, memberLimit) {
     let channelId;
     if (id) {
         channelId = id;
@@ -185,6 +185,7 @@ export function getChannelExtraInfo(id) {
 
         client.getChannelExtraInfo(
             channelId,
+            memberLimit,
             (data, textStatus, xhr) => {
                 callTracker['getChannelExtraInfo_' + channelId] = 0;
 

web/react/utils/client.jsx

@@ -824,10 +824,17 @@ export function getChannelCounts(success, error) {
     });
 }
 
-export function getChannelExtraInfo(id, success, error) {
+export function getChannelExtraInfo(id, memberLimit, success, error) {
+    let url = '/api/v1/channels/' + id + '/extra_info';
+
+    if (memberLimit) {
+        url += '/' + memberLimit;
+    }
+
     $.ajax({
-        url: '/api/v1/channels/' + id + '/extra_info',
+        url,
         dataType: 'json',
+        contentType: 'application/json',
         type: 'GET',
         success,
         error: function onError(xhr, status, err) {