diff --git a/src/components/views/rooms/MemberList.js b/src/components/views/rooms/MemberList.js index 92c486825c..0924a5ec38 100644 --- a/src/components/views/rooms/MemberList.js +++ b/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 : "") + ", " + (u ? u.getLastActiveTs() : "") + ", " + (u ? u.currentlyActive : "") + ", " + (u ? u.presence : "") + ")"; } }, @@ -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 - // ...and then alphabetically. - // We could tiebreak instead by "last recently spoken in this room" if we wanted to. + // 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. - const userA = memberA.user; - const userB = memberB.user; + // console.log(`Comparing userA=${this.memberString(memberA)} userB=${this.memberString(memberB)}`); - // if (!userA || !userB) { - // console.log("comparing " + memberA.name + " user=" + memberA.user + " with " + memberB.name + " user=" + memberB.user); - // } + const userA = memberA.user; + const userB = memberB.user; - if (!userA && !userB) return 0; - if (userA && !userB) return -1; - if (!userA && userB) return 1; + // if (!userA) console.log("!! MISSING USER FOR A-SIDE: " + memberA.name + " !!"); + // if (!userB) console.log("!! MISSING USER FOR B-SIDE: " + memberB.name + " !!"); - // console.log("comparing " + this.memberString(memberA) + " and " + this.memberString(memberB)); + if (!userA && !userB) return 0; + if (userA && !userB) return -1; + if (!userA && userB) return 1; - 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; - } - } else { - // console.log("comparing power: " + memberA.powerLevel + " and " + memberB.powerLevel); - return memberB.powerLevel - memberA.powerLevel; - } + // 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 + }; + + 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; } + } - if (userA.currentlyActive && !userB.currentlyActive) return -1; - if (!userA.currentlyActive && userB.currentlyActive) return 1; + // Second by power level + if (memberA.powerLevel !== memberB.powerLevel) { + // console.log("Comparing on power level - returning"); + return memberB.powerLevel - memberA.powerLevel; + } - // 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 + // Third by last active + if (this._showPresence && userA.getLastActiveTs() !== userB.getLastActiveTs()) { + // console.log("Comparing on last active timestamp - returning"); 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) { diff --git a/test/components/views/rooms/MemberList-test.js b/test/components/views/rooms/MemberList-test.js new file mode 100644 index 0000000000..b9d96635a2 --- /dev/null +++ b/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(, 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); + }); +}); + +