Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Fix displayed user in notifications for virtual rooms #6666

Closed
wants to merge 11 commits into from
14 changes: 12 additions & 2 deletions src/Notifier.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import RoomViewStore from "./stores/RoomViewStore";
import UserActivity from "./UserActivity";
Copy link
Member Author

@anoadragon453 anoadragon453 Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to rephrase the PR title to have it look like a bug fix in the changelog please?

@turt2live I'm actually not sure if this is a bug fix, since notifications never had support for virtual -> native user mapping in the first place.

And it is user visible... so I suppose this would be an enhancement, with a changelog entry of "Show details of the native room in notifications when new events occur in virtual rooms." Does that sound sensible?

My only worry is that this will make no sense to 99% of users 😋

import { mediaFromMxc } from "./customisations/Media";
import ErrorDialog from "./components/views/dialogs/ErrorDialog";
import CallHandler from "./CallHandler";

/*
* Dispatches:
Expand Down Expand Up @@ -385,8 +386,17 @@ export const Notifier = {
}
},

_evaluateEvent: function(ev: MatrixEvent) {
const room = MatrixClientPeg.get().getRoom(ev.getRoomId());
_evaluateEvent: function(ev) {
let roomId = ev.getRoomId();
if (CallHandler.instance.getSupportsVirtualRooms()) {
// Attempt to translate a virtual room to a native one
const nativeRoomId = window.mxVoipUserMapper.nativeRoomForVirtualRoom(roomId);
if (nativeRoomId) {
roomId = nativeRoomId;
}
}
const room = MatrixClientPeg.get().getRoom(roomId);

const actions = MatrixClientPeg.get().getPushActionsForEvent(ev);
if (actions?.notify) {
if (RoomViewStore.getRoomId() === room.roomId &&
Expand Down
71 changes: 69 additions & 2 deletions src/TextForEvent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ import {
PollStartEvent,
} from "matrix-events-sdk";
import { LOCATION_EVENT_TYPE } from "matrix-js-sdk/src/@types/location";
import { RoomMember } from 'matrix-js-sdk/src/models/room-member';

import CallHandler from './CallHandler';
import { _t } from './languageHandler';
import * as Roles from './Roles';
import { isValid3pidInvite } from "./RoomInvite";
Expand All @@ -46,8 +48,73 @@ import AccessibleButton from './components/views/elements/AccessibleButton';
import RightPanelStore from './stores/right-panel/RightPanelStore';
import UserIdentifierCustomisations from './customisations/UserIdentifier';

export function getSenderName(event: MatrixEvent): string {
return event.sender?.name ?? event.getSender() ?? _t("Someone");
/**
* Attempt to retrieve the displayname of a user from an event. If the user is a virtual user,
* return the displayname of the native user instead. In either case, if a displayname cannot
* be found, simply return the MXID of the user instead.
*
* @param event The event to extract the user from.
*/
export function getSenderName(event: MatrixEvent): Promise<string> {
// Retrieve the room ID from the event
const potentiallyVirtualRoom = MatrixClientPeg.get().getRoom(event.getRoomId());

// Check if the event is from a virtual room
if (
!CallHandler.instance.getSupportsVirtualRooms() ||
!window.mxVoipUserMapper.isVirtualRoom(potentiallyVirtualRoom)
) {
// If not, simply extract the sender information from the incoming event
return Promise.resolve(event.sender?.name ?? event.getSender() ?? _t("Someone"));
}

// Otherwise, assume the caller is a virtual user and attempt to look up the corresponding
// native user

// We can avoid a potentially expensive virtual -> native user lookup by checking if the native room
// only has two people in it. If so, then the other user in that room is likely the matching native user.
const nativeRoomId = window.mxVoipUserMapper.nativeRoomForVirtualRoom(potentiallyVirtualRoom.roomId);
const nativeRoom = MatrixClientPeg.get().getRoom(nativeRoomId);
const nativeRoomjoinedMembers = nativeRoom.getJoinedMembers();

const getSenderNameForUserId = (userId: string): Promise<string> => {
return MatrixClientPeg.get().getProfileInfo(userId).then((resp) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't matter in the long run, but we try to use async/await in newer code to avoid confusing promise semantics - up to the developer on whether that is clearer or not though

if (resp.displayname) {
return resp.displayname;
}

return userId;
}).catch((e) => {
console.error('Error getting native user profile', e);
return userId;
});
};

let nativeUser: RoomMember;
if (nativeRoomjoinedMembers.length === 2) {
// Assume the other user in the native room is the native user.
// Find and pull them from the room's joined member list
const myUserId = MatrixClientPeg.get().getUserId();
nativeUser = nativeRoomjoinedMembers.find((roomMember) => {
return roomMember.userId !== myUserId;
});

// If a native user was found, return their profile information
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and if not? The function ends up returning undefined - would be good to make that explicit

if (nativeUser) {
return getSenderNameForUserId(nativeUser.userId);
}
} else {
// Perform a virtual to native user third-party lookup
return window.mxVoipUserMapper.virtualUserToNativeUser(event.sender.userId).then((nativeUserId) => {
// If a native user was found, return their profile information
if (nativeUserId) {
return getSenderNameForUserId(nativeUser.userId);
}
}).catch((e) => {
console.error('Error lookup up native user for virtual user', e);
return event.sender.userId;
});
}
}

// These functions are frequently used just to check whether an event has
Expand Down
13 changes: 13 additions & 0 deletions src/VoipUserMapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,19 @@ export default class VoipUserMapper {
return results[0].userid;
}

/*
* Given a virtual user, attempt to find a corresponding native user via a call to
* the third-party lookup API.
*
* @params userId The MXID of the virtual user.
* @returns Resolves to the native user ID.
*/
public async virtualUserToNativeUser(userId: string): Promise<string> {
const results = await CallHandler.instance.sipNativeLookup(userId);
if (results.length === 0 || !results[0].fields.lookup_success) return null;
return results[0].userid;
}

public async getOrCreateVirtualRoomForRoom(roomId: string): Promise<string> {
const userId = DMRoomMap.shared().getUserIdForRoomId(roomId);
if (!userId) return null;
Expand Down
18 changes: 14 additions & 4 deletions test/TextForEvent-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import renderer from 'react-test-renderer';

import { getSenderName, textForEvent } from "../src/TextForEvent";
import SettingsStore from "../src/settings/SettingsStore";
import { createTestClient } from './test-utils';
import { createTestClient, stubClient } from './test-utils';
import { MatrixClientPeg } from '../src/MatrixClientPeg';
import UserIdentifierCustomisations from '../src/customisations/UserIdentifier';

Expand Down Expand Up @@ -61,15 +61,25 @@ function renderComponent(component): string {
}

describe('TextForEvent', () => {
beforeEach(() => {
stubClient();
});

describe("getSenderName()", () => {
function makeFakeEvent(baseEvent: object): MatrixEvent {
return Object.assign({
getRoomId: () => 'a_room_id',
}, baseEvent) as MatrixEvent;
}

it("Prefers sender.name", () => {
expect(getSenderName({ sender: { name: "Alice" } } as MatrixEvent)).toBe("Alice");
expect(getSenderName(makeFakeEvent({ sender: { name: "Alice" } }))).toBe("Alice");
});
it("Handles missing sender", () => {
expect(getSenderName({ getSender: () => "Alice" } as MatrixEvent)).toBe("Alice");
expect(getSenderName(makeFakeEvent({ getSender: () => "Alice" }))).toBe("Alice");
});
it("Handles missing sender and get sender", () => {
expect(getSenderName({ getSender: () => undefined } as MatrixEvent)).toBe("Someone");
expect(getSenderName(makeFakeEvent({ getSender: () => undefined }))).toBe("Someone");
});
});

Expand Down