Merge pull request #2381 from matrix-org/travis/fix-memberlist-order

Flatten and simplify the memberlist sorting algorithm

src/components/views/rooms/MemberList.js

@@ -68,7 +68,9 @@ module.exports = React.createClass({
         // We listen for changes to the lastPresenceTs which is essentially
         // listening for all presence events (we display most of not all of
         // the information contained in presence events).
-        cli.on("User.lastPresenceTs", this.onUserLastPresenceTs);
+        cli.on("User.lastPresenceTs", this.onUserPresenceChange);
+        cli.on("User.presence", this.onUserPresenceChange);
+        cli.on("User.currentlyActive", this.onUserPresenceChange);
         // cli.on("Room.timeline", this.onRoomTimeline);
     },
 
@@ -81,7 +83,9 @@ module.exports = React.createClass({
             cli.removeListener("Room.myMembership", this.onMyMembership);
             cli.removeListener("RoomState.events", this.onRoomStateEvent);
             cli.removeListener("Room", this.onRoom);
-            cli.removeListener("User.lastPresenceTs", this.onUserLastPresenceTs);
+            cli.removeListener("User.lastPresenceTs", this.onUserPresenceChange);
+            cli.removeListener("User.presence", this.onUserPresenceChange);
+            cli.removeListener("User.currentlyActive", this.onUserPresenceChange);
         }
 
         // cancel any pending calls to the rate_limited_funcs
@@ -132,12 +136,12 @@ module.exports = React.createClass({
         };
     },
 
-    onUserLastPresenceTs(event, user) {
+    onUserPresenceChange(event, user) {
         // Attach a SINGLE listener for global presence changes then locate the
         // member tile and re-render it. This is more efficient than every tile
-        // evar attaching their own listener.
-        // console.log("explicit presence from " + user.userId);
+        // ever attaching their own listener.
         const tile = this.refs[user.userId];
+        // console.log(`Got presence update for ${user.userId}. hasTile=${!!tile}`);
         if (tile) {
             this._updateList(); // reorder the membership list
         }
@@ -181,6 +185,10 @@ module.exports = React.createClass({
     },
 
     _updateList: new rate_limited_func(function() {
+        this._updateListNow();
+    }, 500),
+
+    _updateListNow: function() {
         // console.log("Updating memberlist");
         const newState = {
             loading: false,
@@ -189,7 +197,7 @@ module.exports = React.createClass({
         newState.filteredJoinedMembers = this._filterMembers(newState.members, 'join', this.state.searchQuery);
         newState.filteredInvitedMembers = this._filterMembers(newState.members, 'invite', this.state.searchQuery);
         this.setState(newState);
-    }, 500),
+    },
 
     getMembersWithUser: function() {
         if (!this.props.roomId) return [];
@@ -267,7 +275,8 @@ module.exports = React.createClass({
         if (!member) {
             return "(null)";
         } else {
-            return "(" + member.name + ", " + member.powerLevel + ", " + member.user.lastActiveAgo + ", " + member.user.currentlyActive + ")";
+            const u = member.user;
+            return "(" + member.name + ", " + member.powerLevel + ", " + (u ? u.lastActiveAgo : "<null>") + ", " + (u ? u.getLastActiveTs() : "<null>") + ", " + (u ? u.currentlyActive : "<null>") + ", " + (u ? u.presence : "<null>") + ")";
         }
     },
 
@@ -275,48 +284,59 @@ module.exports = React.createClass({
     // returns 0 if a and b are equivalent in ordering
     // returns positive if a comes after b.
     memberSort: function(memberA, memberB) {
-            // order by last active, with "active now" first.
-            // ...and then by power
+        // order by presence, with "active now" first.
+        // ...and then by power level
+        // ...and then by last active
         // ...and then alphabetically.
         // We could tiebreak instead by "last recently spoken in this room" if we wanted to.
 
+        // console.log(`Comparing userA=${this.memberString(memberA)} userB=${this.memberString(memberB)}`);
+
         const userA = memberA.user;
         const userB = memberB.user;
 
-            // if (!userA || !userB) {
-            //     console.log("comparing " + memberA.name + " user=" + memberA.user + " with " + memberB.name + " user=" + memberB.user);
-            // }
+        // if (!userA) console.log("!! MISSING USER FOR A-SIDE: " + memberA.name + " !!");
+        // if (!userB) console.log("!! MISSING USER FOR B-SIDE: " + memberB.name + " !!");
 
         if (!userA && !userB) return 0;
         if (userA && !userB) return -1;
         if (!userA && userB) return 1;
 
-            // console.log("comparing " + this.memberString(memberA) + " and " + this.memberString(memberB));
+        // First by presence
+        if (this._showPresence) {
+            const convertPresence = (p) => p === 'unavailable' ? 'online' : p;
+            const presenceIndex = p => {
+                const order = ['active', 'online', 'offline'];
+                const idx = order.indexOf(convertPresence(p));
+                return idx === -1 ? order.length : idx; // unknown states at the end
+            };
 
-            if ((userA.currentlyActive && userB.currentlyActive) || !this._showPresence) {
-                // console.log(memberA.name + " and " + memberB.name + " are both active");
-                if (memberA.powerLevel === memberB.powerLevel) {
-                    // console.log(memberA + " and " + memberB + " have same power level");
-                    if (memberA.name && memberB.name) {
-                        // console.log("comparing names: " + memberA.name + " and " + memberB.name);
-                        const nameA = memberA.name[0] === '@' ? memberA.name.substr(1) : memberA.name;
-                        const nameB = memberB.name[0] === '@' ? memberB.name.substr(1) : memberB.name;
-                        return nameA.localeCompare(nameB);
-                    } else {
-                        return 0;
+            const idxA = presenceIndex(userA.currentlyActive ? 'active' : userA.presence);
+            const idxB = presenceIndex(userB.currentlyActive ? 'active' : userB.presence);
+            // console.log(`userA_presenceGroup=${idxA} userB_presenceGroup=${idxB}`);
+            if (idxA !== idxB) {
+                // console.log("Comparing on presence group - returning");
+                return idxA - idxB;
             }
-                } else {
-                    // console.log("comparing power: " + memberA.powerLevel + " and " + memberB.powerLevel);
+        }
+
+        // Second by power level
+        if (memberA.powerLevel !== memberB.powerLevel) {
+            // console.log("Comparing on power level - returning");
             return memberB.powerLevel - memberA.powerLevel;
         }
+
+        // Third by last active
+        if (this._showPresence && userA.getLastActiveTs() !== userB.getLastActiveTs()) {
+            // console.log("Comparing on last active timestamp - returning");
+            return userB.getLastActiveTs() - userA.getLastActiveTs();
         }
 
-            if (userA.currentlyActive && !userB.currentlyActive) return -1;
-            if (!userA.currentlyActive && userB.currentlyActive) return 1;
-
-            // For now, let's just order things by timestamp. It's really annoying
-            // that a user disappears from sight just because they temporarily go offline
-            return userB.getLastActiveTs() - userA.getLastActiveTs();
+        // Fourth by name (alphabetical)
+        const nameA = memberA.name[0] === '@' ? memberA.name.substr(1) : memberA.name;
+        const nameB = memberB.name[0] === '@' ? memberB.name.substr(1) : memberB.name;
+        // console.log(`Comparing userA_name=${nameA} against userB_name=${nameB} - returning`);
+        return nameA.localeCompare(nameB);
     },
 
     onSearchQueryChanged: function(ev) {

test/components/views/rooms/MemberList-test.js

@@ -0,0 +1,301 @@
+import React from 'react';
+import ReactTestUtils from 'react-addons-test-utils';
+import ReactDOM from 'react-dom';
+import expect from 'expect';
+import lolex from 'lolex';
+
+import * as TestUtils from 'test-utils';
+
+import sdk from '../../../../src/index';
+import MatrixClientPeg from '../../../../src/MatrixClientPeg';
+
+import {Room, RoomMember, User} from 'matrix-js-sdk';
+
+function generateRoomId() {
+    return '!' + Math.random().toString().slice(2, 10) + ':domain';
+}
+
+
+describe('MemberList', () => {
+    function createRoom(opts) {
+        const room = new Room(generateRoomId(), null, client.getUserId());
+        if (opts) {
+            Object.assign(room, opts);
+        }
+        return room;
+    }
+
+    let parentDiv = null;
+    let sandbox = null;
+    let client = null;
+    let root = null;
+    let clock = null;
+    let memberListRoom;
+    let memberList = null;
+
+    let adminUsers = [];
+    let moderatorUsers = [];
+    let defaultUsers = [];
+
+    beforeEach(function() {
+        TestUtils.beforeEach(this);
+        sandbox = TestUtils.stubClient(sandbox);
+        client = MatrixClientPeg.get();
+        client.hasLazyLoadMembersEnabled = () => false;
+
+        clock = lolex.install();
+
+        parentDiv = document.createElement('div');
+        document.body.appendChild(parentDiv);
+
+        // Make room
+        memberListRoom = createRoom();
+        expect(memberListRoom.roomId).toBeTruthy();
+
+        // Make users
+        adminUsers = [];
+        moderatorUsers = [];
+        defaultUsers = [];
+        const usersPerLevel = 2;
+        for (let i = 0; i < usersPerLevel; i++) {
+            const adminUser = new RoomMember(memberListRoom.roomId, `@admin${i}:localhost`);
+            adminUser.membership = "join";
+            adminUser.powerLevel = 100;
+            adminUser.user = new User(adminUser.userId);
+            adminUser.user.currentlyActive = true;
+            adminUser.user.presence = 'online';
+            adminUser.user.lastPresenceTs = 1000;
+            adminUser.user.lastActiveAgo = 10;
+            adminUsers.push(adminUser);
+
+            const moderatorUser = new RoomMember(memberListRoom.roomId, `@moderator${i}:localhost`);
+            moderatorUser.membership = "join";
+            moderatorUser.powerLevel = 50;
+            moderatorUser.user = new User(moderatorUser.userId);
+            moderatorUser.user.currentlyActive = true;
+            moderatorUser.user.presence = 'online';
+            moderatorUser.user.lastPresenceTs = 1000;
+            moderatorUser.user.lastActiveAgo = 10;
+            moderatorUsers.push(moderatorUser);
+
+            const defaultUser = new RoomMember(memberListRoom.roomId, `@default${i}:localhost`);
+            defaultUser.membership = "join";
+            defaultUser.powerLevel = 0;
+            defaultUser.user = new User(defaultUser.userId);
+            defaultUser.user.currentlyActive = true;
+            defaultUser.user.presence = 'online';
+            defaultUser.user.lastPresenceTs = 1000;
+            defaultUser.user.lastActiveAgo = 10;
+            defaultUsers.push(defaultUser);
+        }
+
+        client.getRoom = (roomId) => {
+            if (roomId === memberListRoom.roomId) return memberListRoom;
+            else return null;
+        };
+        memberListRoom.currentState = {
+            members: {},
+            getStateEvents: () => [], // ignore 3pid invites
+        };
+        for (const member of [...adminUsers, ...moderatorUsers, ...defaultUsers]) {
+            memberListRoom.currentState.members[member.userId] = member;
+        }
+
+        const MemberList = sdk.getComponent('views.rooms.MemberList');
+        const WrappedMemberList = TestUtils.wrapInMatrixClientContext(MemberList);
+        const gatherWrappedRef = (r) => {
+            memberList = r;
+        };
+        root = ReactDOM.render(<WrappedMemberList roomId={memberListRoom.roomId}
+                                                  wrappedRef={gatherWrappedRef} />, parentDiv);
+    });
+
+    afterEach((done) => {
+        if (parentDiv) {
+            ReactDOM.unmountComponentAtNode(parentDiv);
+            parentDiv.remove();
+            parentDiv = null;
+        }
+        sandbox.restore();
+
+        clock.uninstall();
+
+        done();
+    });
+
+    function expectOrderedByPresenceAndPowerLevel(memberTiles, isPresenceEnabled) {
+        let prevMember = null;
+        for (const tile of memberTiles) {
+            const memberA = prevMember;
+            const memberB = tile.props.member;
+            prevMember = memberB; // just in case an expect fails, set this early
+            if (!memberA) {
+                continue;
+            }
+
+            console.log("COMPARING A VS B:");
+            console.log(memberList.memberString(memberA));
+            console.log(memberList.memberString(memberB));
+
+            const userA = memberA.user;
+            const userB = memberB.user;
+
+            let groupChange = false;
+
+            if (isPresenceEnabled) {
+                const convertPresence = (p) => p === 'unavailable' ? 'online' : p;
+                const presenceIndex = p => {
+                    const order = ['active', 'online', 'offline'];
+                    const idx = order.indexOf(convertPresence(p));
+                    return idx === -1 ? order.length : idx; // unknown states at the end
+                };
+
+                const idxA = presenceIndex(userA.currentlyActive ? 'active' : userA.presence);
+                const idxB = presenceIndex(userB.currentlyActive ? 'active' : userB.presence);
+                console.log("Comparing presence groups...");
+                expect(idxB).toBeGreaterThanOrEqual(idxA);
+                groupChange = idxA !== idxB;
+            } else {
+                console.log("Skipped presence groups");
+            }
+
+            if (!groupChange) {
+                console.log("Comparing power levels...");
+                expect(memberA.powerLevel).toBeGreaterThanOrEqual(memberB.powerLevel);
+                groupChange = memberA.powerLevel !== memberB.powerLevel;
+            } else {
+                console.log("Skipping power level check due to group change");
+            }
+
+            if (!groupChange) {
+                if (isPresenceEnabled) {
+                    console.log("Comparing last active timestamp...");
+                    expect(userB.getLastActiveTs()).toBeLessThanOrEqual(userA.getLastActiveTs());
+                    groupChange = userA.getLastActiveTs() !== userB.getLastActiveTs();
+                } else {
+                    console.log("Skipping last active timestamp");
+                }
+            } else {
+                console.log("Skipping last active timestamp check due to group change");
+            }
+
+            if (!groupChange) {
+                const nameA = memberA.name[0] === '@' ? memberA.name.substr(1) : memberA.name;
+                const nameB = memberB.name[0] === '@' ? memberB.name.substr(1) : memberB.name;
+                const nameCompare = nameB.localeCompare(nameA);
+                console.log("Comparing name");
+                expect(nameCompare).toBeGreaterThanOrEqual(0);
+            } else {
+                console.log("Skipping name check due to group change");
+            }
+        }
+    }
+
+    function itDoesOrderMembersCorrectly(enablePresence) {
+        const MemberTile = sdk.getComponent("rooms.MemberTile");
+        describe('does order members correctly', () => {
+            // Note: even if presence is disabled, we still expect that the presence
+            // tests will pass. All expectOrderedByPresenceAndPowerLevel does is ensure
+            // the order is perceived correctly, regardless of what we did to the members.
+
+            // Each of the 4 tests here is done to prove that the member list can meet
+            // all 4 criteria independently. Together, they should work.
+
+            it('by presence state', () => {
+                // Intentionally pick users that will confuse the power level sorting
+                const activeUsers = [defaultUsers[0]];
+                const onlineUsers = [adminUsers[0]];
+                const offlineUsers = [...moderatorUsers, ...adminUsers.slice(1), ...defaultUsers.slice(1)];
+                activeUsers.forEach((u) => {
+                    u.user.currentlyActive = true;
+                    u.user.presence = 'online';
+                });
+                onlineUsers.forEach((u) => {
+                    u.user.currentlyActive = false;
+                    u.user.presence = 'online';
+                });
+                offlineUsers.forEach((u) => {
+                    u.user.currentlyActive = false;
+                    u.user.presence = 'offline';
+                });
+
+                // Bypass all the event listeners and skip to the good part
+                memberList._showPresence = enablePresence;
+                memberList._updateListNow();
+
+                const tiles = ReactTestUtils.scryRenderedComponentsWithType(root, MemberTile);
+                expectOrderedByPresenceAndPowerLevel(tiles, enablePresence);
+            });
+
+            it('by power level', () => {
+                // We already have admin, moderator, and default users so leave them alone
+
+                // Bypass all the event listeners and skip to the good part
+                memberList._showPresence = enablePresence;
+                memberList._updateListNow();
+
+                const tiles = ReactTestUtils.scryRenderedComponentsWithType(root, MemberTile);
+                expectOrderedByPresenceAndPowerLevel(tiles, enablePresence);
+            });
+
+            it('by last active timestamp', () => {
+                // Intentionally pick users that will confuse the power level sorting
+                // lastActiveAgoTs == lastPresenceTs - lastActiveAgo
+                const activeUsers = [defaultUsers[0]];
+                const semiActiveUsers = [adminUsers[0]];
+                const inactiveUsers = [...moderatorUsers, ...adminUsers.slice(1), ...defaultUsers.slice(1)];
+                activeUsers.forEach((u) => {
+                    u.powerLevel = 100; // set everyone to the same PL to avoid running that check
+                    u.user.lastPresenceTs = 1000;
+                    u.user.lastActiveAgo = 0;
+                });
+                semiActiveUsers.forEach((u) => {
+                    u.powerLevel = 100;
+                    u.user.lastPresenceTs = 1000;
+                    u.user.lastActiveAgo = 50;
+                });
+                inactiveUsers.forEach((u) => {
+                    u.powerLevel = 100;
+                    u.user.lastPresenceTs = 1000;
+                    u.user.lastActiveAgo = 100;
+                });
+
+                // Bypass all the event listeners and skip to the good part
+                memberList._showPresence = enablePresence;
+                memberList._updateListNow();
+
+                const tiles = ReactTestUtils.scryRenderedComponentsWithType(root, MemberTile);
+                expectOrderedByPresenceAndPowerLevel(tiles, enablePresence);
+            });
+
+            it('by name', () => {
+                // Intentionally put everyone on the same level to force a name comparison
+                const allUsers = [...adminUsers, ...moderatorUsers, ...defaultUsers];
+                allUsers.forEach((u) => {
+                    u.user.currentlyActive = true;
+                    u.user.presence = "online";
+                    u.user.lastPresenceTs = 1000;
+                    u.user.lastActiveAgo = 0;
+                    u.powerLevel = 100;
+                });
+
+                // Bypass all the event listeners and skip to the good part
+                memberList._showPresence = enablePresence;
+                memberList._updateListNow();
+
+                const tiles = ReactTestUtils.scryRenderedComponentsWithType(root, MemberTile);
+                expectOrderedByPresenceAndPowerLevel(tiles, enablePresence);
+            });
+        });
+    }
+
+    describe('when presence is enabled', () => {
+        itDoesOrderMembersCorrectly(true);
+    });
+
+    describe('when presence is not enabled', () => {
+        itDoesOrderMembersCorrectly(false);
+    });
+});
+
+