From b416e15cbdbf7239bbb7d88f0ca86beeffcba5f4 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 2 Feb 2023 13:22:30 +0000 Subject: [PATCH] Tests for RoomListStore's predecessor handling (#10046) --- src/stores/room-list/RoomListStore.ts | 76 +++++++++------- test/stores/room-list/RoomListStore-test.ts | 97 ++++++++++++++++++++- 2 files changed, 138 insertions(+), 35 deletions(-) diff --git a/src/stores/room-list/RoomListStore.ts b/src/stores/room-list/RoomListStore.ts index 6369afb350..39d66aa9bf 100644 --- a/src/stores/room-list/RoomListStore.ts +++ b/src/stores/room-list/RoomListStore.ts @@ -18,6 +18,7 @@ import { MatrixClient } from "matrix-js-sdk/src/client"; import { Room } from "matrix-js-sdk/src/models/room"; import { logger } from "matrix-js-sdk/src/logger"; import { EventType } from "matrix-js-sdk/src/@types/event"; +import { RoomState } from "matrix-js-sdk/src/matrix"; import SettingsStore from "../../settings/SettingsStore"; import { DefaultTagID, OrderedDefaultTagIDs, RoomUpdateCause, TagID } from "./models"; @@ -267,44 +268,55 @@ export class RoomListStoreClass extends AsyncStoreWithClient implements } this.updateFn.trigger(); } else if (payload.action === "MatrixActions.Room.myMembership") { - const membershipPayload = payload; // TODO: Type out the dispatcher types - const oldMembership = getEffectiveMembership(membershipPayload.oldMembership); - const newMembership = getEffectiveMembership(membershipPayload.membership); - if (oldMembership !== EffectiveMembership.Join && newMembership === EffectiveMembership.Join) { - // If we're joining an upgraded room, we'll want to make sure we don't proliferate - // the dead room in the list. - const createEvent = membershipPayload.room.currentState.getStateEvents(EventType.RoomCreate, ""); - if (createEvent && createEvent.getContent()["predecessor"]) { - const prevRoom = this.matrixClient.getRoom(createEvent.getContent()["predecessor"]["room_id"]); - if (prevRoom) { - const isSticky = this.algorithm.stickyRoom === prevRoom; - if (isSticky) { - this.algorithm.setStickyRoom(null); - } + this.onDispatchMyMembership(payload); + return; + } + } - // Note: we hit the algorithm instead of our handleRoomUpdate() function to - // avoid redundant updates. - this.algorithm.handleRoomUpdate(prevRoom, RoomUpdateCause.RoomRemoved); + /** + * Handle a MatrixActions.Room.myMembership event from the dispatcher. + * + * Public for test. + */ + public async onDispatchMyMembership(membershipPayload: any): Promise { + // TODO: Type out the dispatcher types so membershipPayload is not any + const oldMembership = getEffectiveMembership(membershipPayload.oldMembership); + const newMembership = getEffectiveMembership(membershipPayload.membership); + if (oldMembership !== EffectiveMembership.Join && newMembership === EffectiveMembership.Join) { + // If we're joining an upgraded room, we'll want to make sure we don't proliferate + // the dead room in the list. + const roomState: RoomState = membershipPayload.room.currentState; + const createEvent = roomState.getStateEvents(EventType.RoomCreate, ""); + if (createEvent && createEvent.getContent()["predecessor"]) { + const prevRoom = this.matrixClient.getRoom(createEvent.getContent()["predecessor"]["room_id"]); + if (prevRoom) { + const isSticky = this.algorithm.stickyRoom === prevRoom; + if (isSticky) { + this.algorithm.setStickyRoom(null); } + + // Note: we hit the algorithm instead of our handleRoomUpdate() function to + // avoid redundant updates. + this.algorithm.handleRoomUpdate(prevRoom, RoomUpdateCause.RoomRemoved); } - - await this.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.NewRoom); - this.updateFn.trigger(); - return; } - if (oldMembership !== EffectiveMembership.Invite && newMembership === EffectiveMembership.Invite) { - await this.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.NewRoom); - this.updateFn.trigger(); - return; - } + await this.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.NewRoom); + this.updateFn.trigger(); + return; + } - // If it's not a join, it's transitioning into a different list (possibly historical) - if (oldMembership !== newMembership) { - await this.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.PossibleTagChange); - this.updateFn.trigger(); - return; - } + if (oldMembership !== EffectiveMembership.Invite && newMembership === EffectiveMembership.Invite) { + await this.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.NewRoom); + this.updateFn.trigger(); + return; + } + + // If it's not a join, it's transitioning into a different list (possibly historical) + if (oldMembership !== newMembership) { + await this.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.PossibleTagChange); + this.updateFn.trigger(); + return; } } diff --git a/test/stores/room-list/RoomListStore-test.ts b/test/stores/room-list/RoomListStore-test.ts index 04b99a8035..7ceb6393a2 100644 --- a/test/stores/room-list/RoomListStore-test.ts +++ b/test/stores/room-list/RoomListStore-test.ts @@ -14,14 +14,55 @@ See the License for the specific language governing permissions and limitations under the License. */ +import { EventType, MatrixEvent, Room } from "matrix-js-sdk/src/matrix"; + +import { MatrixDispatcher } from "../../../src/dispatcher/dispatcher"; import { ListAlgorithm, SortAlgorithm } from "../../../src/stores/room-list/algorithms/models"; -import { OrderedDefaultTagIDs } from "../../../src/stores/room-list/models"; +import { OrderedDefaultTagIDs, RoomUpdateCause } from "../../../src/stores/room-list/models"; import RoomListStore, { RoomListStoreClass } from "../../../src/stores/room-list/RoomListStore"; -import { stubClient } from "../../test-utils"; +import { stubClient, upsertRoomStateEvents } from "../../test-utils"; describe("RoomListStore", () => { + const client = stubClient(); + const roomWithCreatePredecessorId = "!roomid:example.com"; + const roomNoPredecessorId = "!roomnopreid:example.com"; + const oldRoomId = "!oldroomid:example.com"; + const userId = "@user:example.com"; + const createWithPredecessor = new MatrixEvent({ + type: EventType.RoomCreate, + sender: userId, + room_id: roomWithCreatePredecessorId, + content: { + predecessor: { room_id: oldRoomId, event_id: "tombstone_event_id" }, + }, + event_id: "$create", + state_key: "", + }); + const createNoPredecessor = new MatrixEvent({ + type: EventType.RoomCreate, + sender: userId, + room_id: roomWithCreatePredecessorId, + content: {}, + event_id: "$create", + state_key: "", + }); + const roomWithCreatePredecessor = new Room(roomWithCreatePredecessorId, client, userId, {}); + upsertRoomStateEvents(roomWithCreatePredecessor, [createWithPredecessor]); + const roomNoPredecessor = new Room(roomNoPredecessorId, client, userId, {}); + upsertRoomStateEvents(roomNoPredecessor, [createNoPredecessor]); + const oldRoom = new Room(oldRoomId, client, userId, {}); + client.getRoom = jest.fn().mockImplementation((roomId) => { + switch (roomId) { + case roomWithCreatePredecessorId: + return roomWithCreatePredecessor; + case oldRoomId: + return oldRoom; + default: + return null; + } + }); + beforeAll(async () => { - const client = stubClient(); await (RoomListStore.instance as RoomListStoreClass).makeReady(client); }); @@ -32,4 +73,54 @@ describe("RoomListStore", () => { it.each(OrderedDefaultTagIDs)("defaults to activity ordering for %s=", (tagId) => { expect(RoomListStore.instance.getListOrder(tagId)).toBe(ListAlgorithm.Importance); }); + + function createStore(): { store: RoomListStoreClass; handleRoomUpdate: jest.Mock } { + const fakeDispatcher = { register: jest.fn() } as unknown as MatrixDispatcher; + const store = new RoomListStoreClass(fakeDispatcher); + // @ts-ignore accessing private member to set client + store.readyStore.matrixClient = client; + const handleRoomUpdate = jest.fn(); + // @ts-ignore accessing private member to mock it + store.algorithm.handleRoomUpdate = handleRoomUpdate; + + return { store, handleRoomUpdate }; + } + + it("Removes old room if it finds a predecessor in the create event", () => { + // Given a store we can spy on + const { store, handleRoomUpdate } = createStore(); + + // When we tell it we joined a new room that has an old room as + // predecessor in the create event + const payload = { + oldMembership: "invite", + membership: "join", + room: roomWithCreatePredecessor, + }; + store.onDispatchMyMembership(payload); + + // Then the old room is removed + expect(handleRoomUpdate).toHaveBeenCalledWith(oldRoom, RoomUpdateCause.RoomRemoved); + + // And the new room is added + expect(handleRoomUpdate).toHaveBeenCalledWith(roomWithCreatePredecessor, RoomUpdateCause.NewRoom); + }); + + it("Does not remove old room if there is no predecessor in the create event", () => { + // Given a store we can spy on + const { store, handleRoomUpdate } = createStore(); + + // When we tell it we joined a new room with no predecessor + const payload = { + oldMembership: "invite", + membership: "join", + room: roomNoPredecessor, + }; + store.onDispatchMyMembership(payload); + + // Then the new room is added + expect(handleRoomUpdate).toHaveBeenCalledWith(roomNoPredecessor, RoomUpdateCause.NewRoom); + // And no other updates happen + expect(handleRoomUpdate).toHaveBeenCalledTimes(1); + }); });