From 866ed68615fadb80315e89813a235997ea6c96ff Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 24 Feb 2020 19:43:11 -0700 Subject: [PATCH] Ensure DMs tagged outside of account data work in the invite dialog Fixes https://github.com/vector-im/riot-web/issues/12418 Includes a refactor so we don't need to litter the code with the same magic string for DM tags. --- src/actions/RoomListActions.js | 14 +++++++------- src/components/views/dialogs/InviteDialog.js | 19 ++++++++++++++++++- src/components/views/rooms/RoomList.js | 8 ++++---- src/stores/RoomListStore.js | 13 ++++++++----- test/components/views/rooms/RoomList-test.js | 15 ++++++++------- 5 files changed, 45 insertions(+), 24 deletions(-) diff --git a/src/actions/RoomListActions.js b/src/actions/RoomListActions.js index d534fe5d1d..10a3848dda 100644 --- a/src/actions/RoomListActions.js +++ b/src/actions/RoomListActions.js @@ -15,7 +15,7 @@ limitations under the License. */ import { asyncAction } from './actionCreators'; -import RoomListStore from '../stores/RoomListStore'; +import RoomListStore, {TAG_DM} from '../stores/RoomListStore'; import Modal from '../Modal'; import * as Rooms from '../Rooms'; import { _t } from '../languageHandler'; @@ -73,11 +73,11 @@ RoomListActions.tagRoom = function(matrixClient, room, oldTag, newTag, oldIndex, const roomId = room.roomId; // Evil hack to get DMs behaving - if ((oldTag === undefined && newTag === 'im.vector.fake.direct') || - (oldTag === 'im.vector.fake.direct' && newTag === undefined) + if ((oldTag === undefined && newTag === TAG_DM) || + (oldTag === TAG_DM && newTag === undefined) ) { return Rooms.guessAndSetDMRoom( - room, newTag === 'im.vector.fake.direct', + room, newTag === TAG_DM, ).catch((err) => { const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog"); console.error("Failed to set direct chat tag " + err); @@ -91,10 +91,10 @@ RoomListActions.tagRoom = function(matrixClient, room, oldTag, newTag, oldIndex, const hasChangedSubLists = oldTag !== newTag; // More evilness: We will still be dealing with moving to favourites/low prio, - // but we avoid ever doing a request with 'im.vector.fake.direct`. + // but we avoid ever doing a request with TAG_DM. // // if we moved lists, remove the old tag - if (oldTag && oldTag !== 'im.vector.fake.direct' && + if (oldTag && oldTag !== TAG_DM && hasChangedSubLists ) { const promiseToDelete = matrixClient.deleteRoomTag( @@ -112,7 +112,7 @@ RoomListActions.tagRoom = function(matrixClient, room, oldTag, newTag, oldIndex, } // if we moved lists or the ordering changed, add the new tag - if (newTag && newTag !== 'im.vector.fake.direct' && + if (newTag && newTag !== TAG_DM && (hasChangedSubLists || metaData) ) { // metaData is the body of the PUT to set the tag, so it must diff --git a/src/components/views/dialogs/InviteDialog.js b/src/components/views/dialogs/InviteDialog.js index 15e307fcd0..d229a4eedd 100644 --- a/src/components/views/dialogs/InviteDialog.js +++ b/src/components/views/dialogs/InviteDialog.js @@ -34,6 +34,7 @@ import {humanizeTime} from "../../../utils/humanize"; import createRoom, {canEncryptToAllUsers} from "../../../createRoom"; import {inviteMultipleToRoom} from "../../../RoomInvite"; import SettingsStore from '../../../settings/SettingsStore'; +import RoomListStore, {TAG_DM} from "../../../stores/RoomListStore"; export const KIND_DM = "dm"; export const KIND_INVITE = "invite"; @@ -332,7 +333,23 @@ export default class InviteDialog extends React.PureComponent { } _buildRecents(excludedTargetIds: Set): {userId: string, user: RoomMember, lastActive: number} { - const rooms = DMRoomMap.shared().getUniqueRoomsWithIndividuals(); + const rooms = DMRoomMap.shared().getUniqueRoomsWithIndividuals(); // map of userId => js-sdk Room + + // Also pull in all the rooms tagged as m.direct so we don't miss anything. Sometimes the + // room list doesn't tag the room for the DMRoomMap, but does for the room list. + const taggedRooms = RoomListStore.getRoomLists(); + const dmTaggedRooms = taggedRooms[TAG_DM]; + const myUserId = MatrixClientPeg.get().getUserId(); + for (const dmRoom of dmTaggedRooms) { + const otherMembers = dmRoom.getJoinedMembers().filter(u => u.userId !== myUserId); + for (const member of otherMembers) { + if (rooms[member.userId]) continue; // already have a room + + console.warn(`Adding DM room for ${member.userId} as ${dmRoom.roomId} from tag, not DM map`); + rooms[member.userId] = dmRoom; + } + } + const recents = []; for (const userId in rooms) { // Filter out user IDs that are already in the room / should be excluded diff --git a/src/components/views/rooms/RoomList.js b/src/components/views/rooms/RoomList.js index f41400ecfc..99f30d9ba1 100644 --- a/src/components/views/rooms/RoomList.js +++ b/src/components/views/rooms/RoomList.js @@ -28,7 +28,7 @@ import rate_limited_func from "../../../ratelimitedfunc"; import * as Rooms from '../../../Rooms'; import DMRoomMap from '../../../utils/DMRoomMap'; import TagOrderStore from '../../../stores/TagOrderStore'; -import RoomListStore from '../../../stores/RoomListStore'; +import RoomListStore, {TAG_DM} from '../../../stores/RoomListStore'; import CustomRoomTagStore from '../../../stores/CustomRoomTagStore'; import GroupStore from '../../../stores/GroupStore'; import RoomSubList from '../../structures/RoomSubList'; @@ -718,11 +718,11 @@ export default createReactClass({ incomingCall: incomingCallIfTaggedAs('m.favourite'), }, { - list: this.state.lists['im.vector.fake.direct'], + list: this.state.lists[TAG_DM], label: _t('Direct Messages'), - tagName: "im.vector.fake.direct", + tagName: TAG_DM, order: "recent", - incomingCall: incomingCallIfTaggedAs('im.vector.fake.direct'), + incomingCall: incomingCallIfTaggedAs(TAG_DM), onAddRoom: () => {dis.dispatch({action: 'view_create_chat'});}, addRoomLabel: _t("Start chat"), }, diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index a0785cf10e..0a38ac6591 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -37,11 +37,14 @@ const CATEGORY_BOLD = "bold"; // Unread messages (not notified, 'Mentions Only const CATEGORY_IDLE = "idle"; // Nothing of interest const CATEGORY_ORDER = [CATEGORY_RED, CATEGORY_GREY, CATEGORY_BOLD, CATEGORY_IDLE]; + +export const TAG_DM = "im.vector.fake.direct"; + const LIST_ORDERS = { "m.favourite": "manual", "im.vector.fake.invite": "recent", "im.vector.fake.recent": "recent", - "im.vector.fake.direct": "recent", + [TAG_DM]: "recent", "m.lowpriority": "recent", "im.vector.fake.archived": "recent", }; @@ -95,7 +98,7 @@ class RoomListStore extends Store { "im.vector.fake.invite": [], "m.favourite": [], "im.vector.fake.recent": [], - "im.vector.fake.direct": [], + [TAG_DM]: [], "m.lowpriority": [], "im.vector.fake.archived": [], }; @@ -323,7 +326,7 @@ class RoomListStore extends Store { } else if (dmRoomMap.getUserIdForRoomId(room.roomId) && tags.length === 0) { // We intentionally don't duplicate rooms in other tags into the people list // as a feature. - tags.push("im.vector.fake.direct"); + tags.push(TAG_DM); } else if (tags.length === 0) { tags.push("im.vector.fake.recent"); } @@ -553,7 +556,7 @@ class RoomListStore extends Store { "im.vector.fake.invite": [], "m.favourite": [], "im.vector.fake.recent": [], - "im.vector.fake.direct": [], + [TAG_DM]: [], "m.lowpriority": [], "im.vector.fake.archived": [], }; @@ -590,7 +593,7 @@ class RoomListStore extends Store { } } else if (dmRoomMap.getUserIdForRoomId(room.roomId)) { // "Direct Message" rooms (that we're still in and that aren't otherwise tagged) - lists["im.vector.fake.direct"].push({room, category: this._calculateCategory(room)}); + lists[TAG_DM].push({room, category: this._calculateCategory(room)}); } else { lists["im.vector.fake.recent"].push({room, category: this._calculateCategory(room)}); } diff --git a/test/components/views/rooms/RoomList-test.js b/test/components/views/rooms/RoomList-test.js index dc92221c21..8dc4647920 100644 --- a/test/components/views/rooms/RoomList-test.js +++ b/test/components/views/rooms/RoomList-test.js @@ -14,6 +14,7 @@ import DMRoomMap from '../../../../src/utils/DMRoomMap.js'; import GroupStore from '../../../../src/stores/GroupStore.js'; import { MatrixClient, Room, RoomMember } from 'matrix-js-sdk'; +import {TAG_DM} from "../../../../src/stores/RoomListStore"; function generateRoomId() { return '!' + Math.random().toString().slice(2, 10) + ':domain'; @@ -152,7 +153,7 @@ describe('RoomList', () => { // Set up the room that will be moved such that it has the correct state for a room in // the section for oldTag if (['m.favourite', 'm.lowpriority'].includes(oldTag)) movingRoom.tags = {[oldTag]: {}}; - if (oldTag === 'im.vector.fake.direct') { + if (oldTag === TAG_DM) { // Mock inverse m.direct DMRoomMap.shared().roomToUser = { [movingRoom.roomId]: '@someotheruser:domain', @@ -179,7 +180,7 @@ describe('RoomList', () => { // TODO: Re-enable dragging tests when we support dragging again. describe.skip('does correct optimistic update when dragging from', () => { it('rooms to people', () => { - expectCorrectMove(undefined, 'im.vector.fake.direct'); + expectCorrectMove(undefined, TAG_DM); }); it('rooms to favourites', () => { @@ -194,15 +195,15 @@ describe('RoomList', () => { // Whe running the app live, it updates when some other event occurs (likely the // m.direct arriving) that these tests do not fire. xit('people to rooms', () => { - expectCorrectMove('im.vector.fake.direct', undefined); + expectCorrectMove(TAG_DM, undefined); }); it('people to favourites', () => { - expectCorrectMove('im.vector.fake.direct', 'm.favourite'); + expectCorrectMove(TAG_DM, 'm.favourite'); }); it('people to lowpriority', () => { - expectCorrectMove('im.vector.fake.direct', 'm.lowpriority'); + expectCorrectMove(TAG_DM, 'm.lowpriority'); }); it('low priority to rooms', () => { @@ -210,7 +211,7 @@ describe('RoomList', () => { }); it('low priority to people', () => { - expectCorrectMove('m.lowpriority', 'im.vector.fake.direct'); + expectCorrectMove('m.lowpriority', TAG_DM); }); it('low priority to low priority', () => { @@ -222,7 +223,7 @@ describe('RoomList', () => { }); it('favourites to people', () => { - expectCorrectMove('m.favourite', 'im.vector.fake.direct'); + expectCorrectMove('m.favourite', TAG_DM); }); it('favourites to low priority', () => {