From 14bc7f469a4068a3e3591a3803c9668b7f3e0b9e Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Tue, 28 Mar 2023 15:36:12 +0200 Subject: [PATCH 1/2] check profiles before starting a DM --- .../views/dialogs/AskInviteAnywayDialog.tsx | 35 ++- src/components/views/dialogs/InviteDialog.tsx | 91 +++++++- src/i18n/strings/en_EN.json | 5 +- src/stores/UserProfilesStore.ts | 61 ++++- src/utils/MultiInviter.ts | 7 +- .../views/dialogs/InviteDialog-test.tsx | 211 ++++++++++++++---- test/stores/UserProfilesStore-test.ts | 63 +++++- 7 files changed, 405 insertions(+), 68 deletions(-) diff --git a/src/components/views/dialogs/AskInviteAnywayDialog.tsx b/src/components/views/dialogs/AskInviteAnywayDialog.tsx index a35971992e2..7e7c1b9ff0d 100644 --- a/src/components/views/dialogs/AskInviteAnywayDialog.tsx +++ b/src/components/views/dialogs/AskInviteAnywayDialog.tsx @@ -22,14 +22,21 @@ import SettingsStore from "../../../settings/SettingsStore"; import { SettingLevel } from "../../../settings/SettingLevel"; import BaseDialog from "./BaseDialog"; +export interface UnknownProfile { + userId: string; + errorText: string; +} + +export type UnknownProfiles = UnknownProfile[]; + export interface AskInviteAnywayDialogProps { - unknownProfileUsers: Array<{ - userId: string; - errorText: string; - }>; + unknownProfileUsers: UnknownProfiles; onInviteAnyways: () => void; onGiveUp: () => void; onFinished: (success: boolean) => void; + description?: string; + inviteNeverWarnLabel?: string; + inviteLabel?: string; } export default function AskInviteAnywayDialog({ @@ -37,6 +44,9 @@ export default function AskInviteAnywayDialog({ onGiveUp, onInviteAnyways, unknownProfileUsers, + description: descriptionProp, + inviteNeverWarnLabel, + inviteLabel, }: AskInviteAnywayDialogProps): JSX.Element { const onInviteClicked = useCallback((): void => { onInviteAnyways(); @@ -60,6 +70,10 @@ export default function AskInviteAnywayDialog({ )); + const description = + descriptionProp ?? + _t("Unable to find profiles for the Matrix IDs listed below - would you like to invite them anyway?"); + return (
-

- {_t( - "Unable to find profiles for the Matrix IDs listed below - " + - "would you like to invite them anyway?", - )} -

+

{description}

    {errorList}
- +
diff --git a/src/components/views/dialogs/InviteDialog.tsx b/src/components/views/dialogs/InviteDialog.tsx index 7ddd9e8b4fc..e637e3081eb 100644 --- a/src/components/views/dialogs/InviteDialog.tsx +++ b/src/components/views/dialogs/InviteDialog.tsx @@ -1,5 +1,5 @@ /* -Copyright 2019 - 2022 The Matrix.org Foundation C.I.C. +Copyright 2019 - 2023 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -20,6 +20,7 @@ import { RoomMember } from "matrix-js-sdk/src/models/room-member"; import { Room } from "matrix-js-sdk/src/models/room"; import { MatrixCall } from "matrix-js-sdk/src/webrtc/call"; import { logger } from "matrix-js-sdk/src/logger"; +import { MatrixError } from "matrix-js-sdk/src/matrix"; import { Icon as InfoIcon } from "../../../../res/img/element-icons/info.svg"; import { Icon as EmailPillAvatarIcon } from "../../../../res/img/icon-email-pill-avatar.svg"; @@ -75,10 +76,38 @@ import Modal from "../../../Modal"; import dis from "../../../dispatcher/dispatcher"; import { privateShouldBeEncrypted } from "../../../utils/rooms"; import { NonEmptyArray } from "../../../@types/common"; +import { UNKNOWN_PROFILE_ERRORS } from "../../../utils/MultiInviter"; +import AskInviteAnywayDialog, { UnknownProfiles } from "./AskInviteAnywayDialog"; +import { SdkContextClass } from "../../../contexts/SDKContext"; +import { UserProfilesStore } from "../../../stores/UserProfilesStore"; // we have a number of types defined from the Matrix spec which can't reasonably be altered here. /* eslint-disable camelcase */ +const extractTargetUnknownProfiles = async ( + targets: Member[], + profilesStores: UserProfilesStore, +): Promise => { + const directoryMembers = targets.filter((t): t is DirectoryMember => t instanceof DirectoryMember); + await Promise.all(directoryMembers.map((t) => profilesStores.getOrFetchProfile(t.userId))); + return directoryMembers.reduce((unknownProfiles: UnknownProfiles, target: DirectoryMember) => { + const lookupError = profilesStores.getProfileLookupError(target.userId); + + if ( + lookupError instanceof MatrixError && + lookupError.errcode && + UNKNOWN_PROFILE_ERRORS.includes(lookupError.errcode) + ) { + unknownProfiles.push({ + userId: target.userId, + errorText: lookupError.data.error || "", + }); + } + + return unknownProfiles; + }, []); +}; + interface Result { userId: string; user: Member; @@ -331,6 +360,7 @@ export default class InviteDialog extends React.PureComponent = createRef(); private unmounted = false; private encryptionByDefault = false; + private profilesStore: UserProfilesStore; public constructor(props: Props) { super(props); @@ -341,6 +371,8 @@ export default class InviteDialog extends React.PureComponent => { + this.setBusy(true); + const targets = this.convertFilter(); + + if (SettingsStore.getValue("promptBeforeInviteUnknownUsers")) { + const unknownProfileUsers = await extractTargetUnknownProfiles(targets, this.profilesStore); + + if (unknownProfileUsers.length) { + this.showAskInviteAnywayDialog(unknownProfileUsers); + return; + } + } + + await this.startDm(); + }; + private startDm = async (): Promise => { - this.setState({ - busy: true, - }); + this.setBusy(true); try { const cli = MatrixClientPeg.get(); @@ -523,6 +573,27 @@ export default class InviteDialog extends React.PureComponent this.startDm(), + onGiveUp: () => { + this.setBusy(false); + }, + description: _t( + "Unable to find profiles for the Matrix IDs listed below - would you like to start a DM anyway?", + ), + inviteNeverWarnLabel: _t("Start DM anyway and never warn me again"), + inviteLabel: _t("Start DM anyway"), + }); + } + private inviteUsers = async (): Promise => { if (this.props.kind !== InviteKind.Invite) return; this.setState({ busy: true }); @@ -639,7 +710,8 @@ export default class InviteDialog extends React.PureComponent 1) { try { - const profile = await MatrixClientPeg.get().getProfileInfo(term); + const profile = await this.profilesStore.getOrFetchProfile(term, { shouldThrow: true }); + if (profile) { // If we have a profile, we have enough information to assume that // the mxid can be invited - add it to the list. We stick it at the @@ -651,8 +723,7 @@ export default class InviteDialog extends React.PureComponent {_t("Some suggestions may be hidden for privacy.")} diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 48c1b629563..9199151963c 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -2707,8 +2707,8 @@ "Get it on F-Droid": "Get it on F-Droid", "App Store® and the Apple logo® are trademarks of Apple Inc.": "App Store® and the Apple logo® are trademarks of Apple Inc.", "Google Play and the Google Play logo are trademarks of Google LLC.": "Google Play and the Google Play logo are trademarks of Google LLC.", - "The following users may not exist": "The following users may not exist", "Unable to find profiles for the Matrix IDs listed below - would you like to invite them anyway?": "Unable to find profiles for the Matrix IDs listed below - would you like to invite them anyway?", + "The following users may not exist": "The following users may not exist", "Invite anyway and never warn me again": "Invite anyway and never warn me again", "Invite anyway": "Invite anyway", "Close dialog": "Close dialog", @@ -2872,6 +2872,9 @@ "Click the button below to confirm your identity.": "Click the button below to confirm your identity.", "Invite by email": "Invite by email", "We couldn't create your DM.": "We couldn't create your DM.", + "Unable to find profiles for the Matrix IDs listed below - would you like to start a DM anyway?": "Unable to find profiles for the Matrix IDs listed below - would you like to start a DM anyway?", + "Start DM anyway and never warn me again": "Start DM anyway and never warn me again", + "Start DM anyway": "Start DM anyway", "Something went wrong trying to invite the users.": "Something went wrong trying to invite the users.", "We couldn't invite those users. Please check the users you want to invite and try again.": "We couldn't invite those users. Please check the users you want to invite and try again.", "A call can only be transferred to a single user.": "A call can only be transferred to a single user.", diff --git a/src/stores/UserProfilesStore.ts b/src/stores/UserProfilesStore.ts index cd4fd7dd5ee..500450f31a7 100644 --- a/src/stores/UserProfilesStore.ts +++ b/src/stores/UserProfilesStore.ts @@ -15,7 +15,14 @@ limitations under the License. */ import { logger } from "matrix-js-sdk/src/logger"; -import { IMatrixProfile, MatrixClient, MatrixEvent, RoomMember, RoomMemberEvent } from "matrix-js-sdk/src/matrix"; +import { + IMatrixProfile, + MatrixClient, + MatrixError, + MatrixEvent, + RoomMember, + RoomMemberEvent, +} from "matrix-js-sdk/src/matrix"; import { LruCache } from "../utils/LruCache"; @@ -23,12 +30,18 @@ const cacheSize = 500; type StoreProfileValue = IMatrixProfile | undefined | null; +interface GetOptions { + /** Whether calling the function shouuld raise an Error. */ + shouldThrow: boolean; +} + /** * This store provides cached access to user profiles. * Listens for membership events and invalidates the cache for a profile on update with different profile values. */ export class UserProfilesStore { private profiles = new LruCache(cacheSize); + private profileLookupErrors = new LruCache(cacheSize); private knownProfiles = new LruCache(cacheSize); public constructor(private client: MatrixClient) { @@ -48,6 +61,32 @@ export class UserProfilesStore { return this.profiles.get(userId); } + /** + * Async shortcut function that returns the profile from cache or + * or fetches it on cache miss. + * + * @param userId - User Id of the profile to get or fetch + * @returns The profile, if cached by the store or fetched from the API. + * Null if the profile does not exist or an error occurred during fetch. + */ + public async getOrFetchProfile(userId: string, options?: GetOptions): Promise { + const cachedProfile = this.profiles.get(userId); + + if (cachedProfile) return cachedProfile; + + return this.fetchProfile(userId, options); + } + + /** + * Get a profile lookup error. + * + * @param userId - User Id for which to get the lookup error + * @returns The lookup error or undefined if there was no error or the profile was not fetched. + */ + public getProfileLookupError(userId: string): MatrixError | undefined { + return this.profileLookupErrors.get(userId); + } + /** * Synchronously get a profile from known users from the store cache. * Known user means that at least one shared room with the user exists. @@ -70,8 +109,8 @@ export class UserProfilesStore { * @returns The profile, if found. * Null if the profile does not exist or there was an error fetching it. */ - public async fetchProfile(userId: string): Promise { - const profile = await this.fetchProfileFromApi(userId); + public async fetchProfile(userId: string, options?: GetOptions): Promise { + const profile = await this.fetchProfileFromApi(userId, options); this.profiles.set(userId, profile); return profile; } @@ -96,17 +135,31 @@ export class UserProfilesStore { return profile; } + public flush(): void { + this.profiles = new LruCache(cacheSize); + this.profileLookupErrors = new LruCache(cacheSize); + this.knownProfiles = new LruCache(cacheSize); + } + /** * Looks up a user profile via API. * * @param userId - User Id for which the profile should be fetched for * @returns The profile information or null on errors */ - private async fetchProfileFromApi(userId: string): Promise { + private async fetchProfileFromApi(userId: string, options?: GetOptions): Promise { try { return (await this.client.getProfileInfo(userId)) ?? null; } catch (e) { logger.warn(`Error retrieving profile for userId ${userId}`, e); + + if (e instanceof MatrixError) { + this.profileLookupErrors.set(userId, e); + } + + if (options?.shouldThrow) { + throw e; + } } return null; diff --git a/src/utils/MultiInviter.ts b/src/utils/MultiInviter.ts index 45de0e2ca08..02cbbe585ba 100644 --- a/src/utils/MultiInviter.ts +++ b/src/utils/MultiInviter.ts @@ -38,7 +38,12 @@ interface IError { errcode: string; } -const UNKNOWN_PROFILE_ERRORS = ["M_NOT_FOUND", "M_USER_NOT_FOUND", "M_PROFILE_UNDISCLOSED", "M_PROFILE_NOT_FOUND"]; +export const UNKNOWN_PROFILE_ERRORS = [ + "M_NOT_FOUND", + "M_USER_NOT_FOUND", + "M_PROFILE_UNDISCLOSED", + "M_PROFILE_NOT_FOUND", +]; export type CompletionStates = Record; diff --git a/test/components/views/dialogs/InviteDialog-test.tsx b/test/components/views/dialogs/InviteDialog-test.tsx index e048240f90c..89be4f80300 100644 --- a/test/components/views/dialogs/InviteDialog-test.tsx +++ b/test/components/views/dialogs/InviteDialog-test.tsx @@ -18,15 +18,28 @@ import React from "react"; import { render, screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { RoomType } from "matrix-js-sdk/src/@types/event"; -import { Room } from "matrix-js-sdk/src/matrix"; +import { MatrixClient, MatrixError, Room } from "matrix-js-sdk/src/matrix"; +import { sleep } from "matrix-js-sdk/src/utils"; +import { mocked, Mocked } from "jest-mock"; import InviteDialog from "../../../../src/components/views/dialogs/InviteDialog"; import { InviteKind } from "../../../../src/components/views/dialogs/InviteDialogTypes"; -import { getMockClientWithEventEmitter, mkMembership, mkMessage, mkRoomCreateEvent } from "../../../test-utils"; +import { + filterConsole, + getMockClientWithEventEmitter, + mkMembership, + mkMessage, + mkRoomCreateEvent, +} from "../../../test-utils"; import DMRoomMap from "../../../../src/utils/DMRoomMap"; import SdkConfig from "../../../../src/SdkConfig"; import { ValidatedServerConfig } from "../../../../src/utils/ValidatedServerConfig"; import { IConfigOptions } from "../../../../src/IConfigOptions"; +import { SdkContextClass } from "../../../../src/contexts/SDKContext"; +import { IProfileInfo } from "../../../../src/hooks/useProfileInfo"; +import { DirectoryMember, startDmOnFirstMessage } from "../../../../src/utils/direct-messages"; +import SettingsStore from "../../../../src/settings/SettingsStore"; +import Modal from "../../../../src/Modal"; const mockGetAccessToken = jest.fn().mockResolvedValue("getAccessToken"); jest.mock("../../../../src/IdentityAuthClient", () => @@ -35,6 +48,12 @@ jest.mock("../../../../src/IdentityAuthClient", () => })), ); +jest.mock("../../../../src/utils/direct-messages", () => ({ + ...jest.requireActual("../../../../src/utils/direct-messages"), + __esModule: true, + startDmOnFirstMessage: jest.fn(), +})); + const getSearchField = () => screen.getByTestId("invite-dialog-input"); const enterIntoSearchField = async (value: string) => { @@ -60,46 +79,73 @@ const expectNoPill = (value: string) => { expect(getSearchField()).toHaveValue(value); }; +const roomId = "!111111111111111111:example.org"; +const aliceId = "@alice:example.org"; +const aliceEmail = "foobar@email.com"; +const bobId = "@bob:example.org"; +const bobEmail = "bobbob@example.com"; // bob@example.com is already used as an example in the invite dialog +const carolId = "@carol:example.com"; + +const aliceProfileInfo: IProfileInfo = { + user_id: aliceId, + display_name: "Alice", +}; + +const bobProfileInfo: IProfileInfo = { + user_id: bobId, + display_name: "Bob", +}; + describe("InviteDialog", () => { - const roomId = "!111111111111111111:example.org"; - const aliceId = "@alice:example.org"; - const aliceEmail = "foobar@email.com"; - const bobId = "@bob:example.org"; - const bobEmail = "bobbob@example.com"; // bob@example.com is already used as an example in the invite dialog - const mockClient = getMockClientWithEventEmitter({ - getUserId: jest.fn().mockReturnValue(bobId), - getSafeUserId: jest.fn().mockReturnValue(bobId), - isGuest: jest.fn().mockReturnValue(false), - getVisibleRooms: jest.fn().mockReturnValue([]), - getRoom: jest.fn(), - getRooms: jest.fn(), - getAccountData: jest.fn(), - getPushActionsForEvent: jest.fn(), - mxcUrlToHttp: jest.fn().mockReturnValue(""), - isRoomEncrypted: jest.fn().mockReturnValue(false), - getProfileInfo: jest.fn().mockRejectedValue({ errcode: "" }), - getIdentityServerUrl: jest.fn(), - searchUserDirectory: jest.fn().mockResolvedValue({}), - lookupThreePid: jest.fn(), - registerWithIdentityServer: jest.fn().mockResolvedValue({ - access_token: "access_token", - token: "token", - }), - getOpenIdToken: jest.fn().mockResolvedValue({}), - getIdentityAccount: jest.fn().mockResolvedValue({}), - getTerms: jest.fn().mockResolvedValue({ policies: [] }), - supportsThreads: jest.fn().mockReturnValue(false), - isInitialSyncComplete: jest.fn().mockReturnValue(true), - getClientWellKnown: jest.fn(), - }); + let mockClient: Mocked; let room: Room; + filterConsole( + "Error retrieving profile for userId @carol:example.com", + "Error retrieving profile for userId @localpart:server.tld", + "Error retrieving profile for userId @localpart:server:tld", + "Starting load of AsyncWrapper for modal", + "[Invite:Recents] Excluding @alice:example.org from recents", + ); + beforeEach(() => { + mockClient = getMockClientWithEventEmitter({ + getUserId: jest.fn().mockReturnValue(bobId), + getSafeUserId: jest.fn().mockReturnValue(bobId), + isGuest: jest.fn().mockReturnValue(false), + getVisibleRooms: jest.fn().mockReturnValue([]), + getRoom: jest.fn(), + getRooms: jest.fn(), + getAccountData: jest.fn(), + getPushActionsForEvent: jest.fn(), + mxcUrlToHttp: jest.fn().mockReturnValue(""), + isRoomEncrypted: jest.fn().mockReturnValue(false), + getProfileInfo: jest.fn().mockImplementation(async (userId: string) => { + if (userId === aliceId) return aliceProfileInfo; + if (userId === bobId) return bobProfileInfo; + + throw new MatrixError({ + errcode: "M_NOT_FOUND", + error: "Profile not found", + }); + }), + getIdentityServerUrl: jest.fn(), + searchUserDirectory: jest.fn().mockResolvedValue({}), + lookupThreePid: jest.fn(), + registerWithIdentityServer: jest.fn().mockResolvedValue({ + access_token: "access_token", + token: "token", + }), + getOpenIdToken: jest.fn().mockResolvedValue({}), + getIdentityAccount: jest.fn().mockResolvedValue({}), + getTerms: jest.fn().mockResolvedValue({ policies: [] }), + supportsThreads: jest.fn().mockReturnValue(false), + isInitialSyncComplete: jest.fn().mockReturnValue(true), + getClientWellKnown: jest.fn().mockResolvedValue({}), + }); SdkConfig.put({ validated_server_config: {} as ValidatedServerConfig } as IConfigOptions); DMRoomMap.makeShared(); jest.clearAllMocks(); - mockClient.getUserId.mockReturnValue(bobId); - mockClient.getClientWellKnown.mockReturnValue({}); room = new Room(roomId, mockClient, mockClient.getSafeUserId()); room.addLiveEvents([ @@ -127,6 +173,14 @@ describe("InviteDialog", () => { }); mockClient.getRooms.mockReturnValue([room]); mockClient.getRoom.mockReturnValue(room); + + SdkContextClass.instance.client = mockClient; + }); + + afterEach(() => { + Modal.closeCurrentModal(); + SdkContextClass.instance.onLoggedOut(); + SdkContextClass.instance.client = undefined; }); afterAll(() => { @@ -232,7 +286,7 @@ describe("InviteDialog", () => { input.focus(); await userEvent.paste(`${bobId} ${aliceEmail}`); - await screen.findByText(bobId); + await screen.findAllByText(bobId); await screen.findByText(aliceEmail); expect(input).toHaveValue(""); }); @@ -291,13 +345,94 @@ describe("InviteDialog", () => { render(); // Start with a MXID → should convert to a pill - await enterIntoSearchField("@carol:example.com"); + await enterIntoSearchField(carolId); expect(screen.queryByText("Invites by email can only be sent one at a time")).not.toBeInTheDocument(); - expectPill("@carol:example.com"); + expectPill(carolId); // Add an email → should not convert to a pill await enterIntoSearchField(bobEmail); expect(screen.getByText("Invites by email can only be sent one at a time")).toBeInTheDocument(); expectNoPill(bobEmail); }); + + it("should start a DM if the profile is available", async () => { + render(); + await enterIntoSearchField(aliceId); + await userEvent.click(screen.getByRole("button", { name: "Go" })); + expect(startDmOnFirstMessage).toHaveBeenCalledWith(mockClient, [ + new DirectoryMember({ + user_id: aliceId, + }), + ]); + }); + + describe("when inviting a user with an unknown profile", () => { + beforeEach(async () => { + render(); + await enterIntoSearchField(carolId); + await userEvent.click(screen.getByRole("button", { name: "Go" })); + // Wait for the »invite anyway« modal to show up + await screen.findByText("The following users may not exist"); + }); + + it("should not start the DM", () => { + expect(startDmOnFirstMessage).not.toHaveBeenCalled(); + }); + + it("should show the »invite anyway« dialog if the profile is not available", () => { + expect(screen.getByText("The following users may not exist")).toBeInTheDocument(); + expect(screen.getByText(`${carolId}: Profile not found`)).toBeInTheDocument(); + }); + + describe("when clicking »Start DM anyway«", () => { + beforeEach(async () => { + await userEvent.click(screen.getByRole("button", { name: "Start DM anyway", exact: true })); + }); + + it("should start the DM", () => { + expect(startDmOnFirstMessage).toHaveBeenCalledWith(mockClient, [ + new DirectoryMember({ + user_id: carolId, + }), + ]); + }); + }); + + describe("when clicking »Close«", () => { + beforeEach(async () => { + mocked(startDmOnFirstMessage).mockClear(); + await userEvent.click(screen.getByRole("button", { name: "Close" })); + }); + + it("should not start the DM", () => { + expect(startDmOnFirstMessage).not.toHaveBeenCalled(); + }); + }); + }); + + describe("when inviting a user with an unknown profile and »promptBeforeInviteUnknownUsers« setting = false", () => { + beforeEach(async () => { + mocked(startDmOnFirstMessage).mockClear(); + jest.spyOn(SettingsStore, "getValue").mockImplementation( + (settingName) => settingName !== "promptBeforeInviteUnknownUsers", + ); + render(); + await enterIntoSearchField(carolId); + await userEvent.click(screen.getByRole("button", { name: "Go" })); + // modal rendering has some weird sleeps - fake timers will mess up the entire test + await sleep(100); + }); + + it("should not show the »invite anyway« dialog", () => { + expect(screen.queryByText("The following users may not exist")).not.toBeInTheDocument(); + }); + + it("should start the DM directly", () => { + expect(startDmOnFirstMessage).toHaveBeenCalledWith(mockClient, [ + new DirectoryMember({ + user_id: carolId, + }), + ]); + }); + }); }); diff --git a/test/stores/UserProfilesStore-test.ts b/test/stores/UserProfilesStore-test.ts index 8f7739501e5..679081c50e5 100644 --- a/test/stores/UserProfilesStore-test.ts +++ b/test/stores/UserProfilesStore-test.ts @@ -15,13 +15,25 @@ limitations under the License. */ import { mocked, Mocked } from "jest-mock"; -import { IMatrixProfile, MatrixClient, MatrixEvent, Room, RoomMemberEvent } from "matrix-js-sdk/src/matrix"; +import { + IMatrixProfile, + MatrixClient, + MatrixError, + MatrixEvent, + Room, + RoomMemberEvent, +} from "matrix-js-sdk/src/matrix"; import { UserProfilesStore } from "../../src/stores/UserProfilesStore"; import { filterConsole, mkRoomMember, mkRoomMemberJoinEvent, stubClient } from "../test-utils"; describe("UserProfilesStore", () => { const userIdDoesNotExist = "@unknown:example.com"; + const userDoesNotExistError = new MatrixError({ + errcode: "M_NOT_FOUND", + error: "Profile not found", + }); + const user1Id = "@user1:example.com"; const user1Profile: IMatrixProfile = { displayname: "User 1", avatar_url: undefined }; const user2Id = "@user2:example.com"; @@ -50,7 +62,7 @@ describe("UserProfilesStore", () => { if (userId === user1Id) return user1Profile; if (userId === user2Id) return user2Profile; - throw new Error("User not found"); + throw userDoesNotExistError; }); }); @@ -70,6 +82,39 @@ describe("UserProfilesStore", () => { expect(profile).toBeNull(); expect(userProfilesStore.getProfile(userIdDoesNotExist)).toBeNull(); }); + + it("when souldThrow = true and there is an error it should raise an error", async () => { + await expect(userProfilesStore.fetchProfile(userIdDoesNotExist, { shouldThrow: true })).rejects.toThrow( + userDoesNotExistError.message, + ); + }); + }); + + describe("getOrFetchProfile", () => { + it("should return a profile from the API and cache it", async () => { + const profile = await userProfilesStore.getOrFetchProfile(user1Id); + expect(profile).toBe(user1Profile); + // same method again + expect(await userProfilesStore.getOrFetchProfile(user1Id)).toBe(user1Profile); + // assert that the profile is cached + expect(userProfilesStore.getProfile(user1Id)).toBe(user1Profile); + }); + }); + + describe("getProfileLookupError", () => { + it("should return undefined if a profile was not fetched", () => { + expect(userProfilesStore.getProfileLookupError(user1Id)).toBeUndefined(); + }); + + it("should return undefined if a profile was successfully fetched", async () => { + await userProfilesStore.fetchProfile(user1Id); + expect(userProfilesStore.getProfileLookupError(user1Id)).toBeUndefined(); + }); + + it("should return the error if there was one", async () => { + await userProfilesStore.fetchProfile(userIdDoesNotExist); + expect(userProfilesStore.getProfileLookupError(userIdDoesNotExist)).toBe(userDoesNotExistError); + }); }); it("getOnlyKnownProfile should return undefined if the profile was not fetched", () => { @@ -121,4 +166,18 @@ describe("UserProfilesStore", () => { }); }); }); + + describe("flush", () => { + it("should clear profiles, known profiles and errors", async () => { + await userProfilesStore.fetchOnlyKnownProfile(user1Id); + await userProfilesStore.fetchProfile(user1Id); + await userProfilesStore.fetchProfile(userIdDoesNotExist); + + userProfilesStore.flush(); + + expect(userProfilesStore.getProfile(user1Id)).toBeUndefined(); + expect(userProfilesStore.getOnlyKnownProfile(user1Id)).toBeUndefined(); + expect(userProfilesStore.getProfileLookupError(userIdDoesNotExist)).toBeUndefined(); + }); + }); }); From d89af7c13ae047af504ba9e3e9573d61900728bc Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Wed, 5 Apr 2023 08:46:53 +0200 Subject: [PATCH 2/2] Invalidate profile error when fetching a profile --- src/stores/UserProfilesStore.ts | 3 ++ test/stores/UserProfilesStore-test.ts | 40 ++++++++++++++++++++++----- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/src/stores/UserProfilesStore.ts b/src/stores/UserProfilesStore.ts index 500450f31a7..54623af11bf 100644 --- a/src/stores/UserProfilesStore.ts +++ b/src/stores/UserProfilesStore.ts @@ -148,6 +148,9 @@ export class UserProfilesStore { * @returns The profile information or null on errors */ private async fetchProfileFromApi(userId: string, options?: GetOptions): Promise { + // invalidate cached profile errors + this.profileLookupErrors.delete(userId); + try { return (await this.client.getProfileInfo(userId)) ?? null; } catch (e) { diff --git a/test/stores/UserProfilesStore-test.ts b/test/stores/UserProfilesStore-test.ts index 679081c50e5..72f36f7c98a 100644 --- a/test/stores/UserProfilesStore-test.ts +++ b/test/stores/UserProfilesStore-test.ts @@ -77,17 +77,43 @@ describe("UserProfilesStore", () => { expect(userProfilesStore.getProfile(user1Id)).toBe(user1Profile); }); - it("for an user that does not exist should return null and cache it", async () => { - const profile = await userProfilesStore.fetchProfile(userIdDoesNotExist); - expect(profile).toBeNull(); - expect(userProfilesStore.getProfile(userIdDoesNotExist)).toBeNull(); - }); - - it("when souldThrow = true and there is an error it should raise an error", async () => { + it("when shouldThrow = true and there is an error it should raise an error", async () => { await expect(userProfilesStore.fetchProfile(userIdDoesNotExist, { shouldThrow: true })).rejects.toThrow( userDoesNotExistError.message, ); }); + + describe("when fetching a profile that does not exist", () => { + let profile: IMatrixProfile | null | undefined; + + beforeEach(async () => { + profile = await userProfilesStore.fetchProfile(userIdDoesNotExist); + }); + + it("should return null", () => { + expect(profile).toBeNull(); + }); + + it("should cache the error and result", () => { + expect(userProfilesStore.getProfile(userIdDoesNotExist)).toBeNull(); + expect(userProfilesStore.getProfileLookupError(userIdDoesNotExist)).toBe(userDoesNotExistError); + }); + + describe("when the profile does not exist and fetching it again", () => { + beforeEach(async () => { + mockClient.getProfileInfo.mockResolvedValue(user1Profile); + profile = await userProfilesStore.fetchProfile(userIdDoesNotExist); + }); + + it("should return the profile", () => { + expect(profile).toBe(user1Profile); + }); + + it("should clear the error", () => { + expect(userProfilesStore.getProfileLookupError(userIdDoesNotExist)).toBeUndefined(); + }); + }); + }); }); describe("getOrFetchProfile", () => {