Fix displayed user in notifications for virtual rooms #6666
Conversation
anoadragon453
force-pushed
the
anoa/virtual_rooms_notifications
branch
3 times, most recently
from
725f223 to
6bb8dc3
Compare
anoadragon453
added
the
T-Defect
This commit first checks when a notification comes in if it originated from a virtual room. If so, check via the VoipUserMapper to if the call was generated by a virtual user. If yes, get the corresponding native user and show their information in the notification instead.
Performing a virtual user -> native user lookup is rather expensive. It requires making a third-party lookup API call in the background. We would rather not need to do so every time a call comes in. Translating a virtual room to a native room is rather cheap in contrast - the client already knows about it. Thus, we can make an optimisation by checking if the corresponding native room has only two users in it. If so, we can likely assume that whichever user in the native that's not us, is probably the same user that calls us via the virtual room. Thus in that case, we just return that user's profile information.
Most of this commit can be safely ignored.
anoadragon453
force-pushed
the
anoa/virtual_rooms_notifications
branch
from
6bb8dc3 to
45bb2df
Compare
src/TextForEvent.tsx
Outdated
| * | ||
| * @param event The event to extract the user from. | ||
| */ | ||
| function getSenderNameFromPotentiallyVirtualUser(event: MatrixEvent) { |
| const nativeRoomjoinedMembers = nativeRoom.getJoinedMembers(); | ||
|
|
||
| const getSenderNameForUserId = (userId) => { | ||
| return MatrixClientPeg.get().getProfileInfo(userId).then((resp) => { |
There was a problem hiding this comment.
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
| return roomMember.userId !== myUserId; | ||
| }); | ||
|
|
||
| // If a native user was found, return their profile information |
There was a problem hiding this comment.
and if not? The function ends up returning undefined - would be good to make that explicit
| @@ -37,6 +37,7 @@ import RoomViewStore from "./stores/RoomViewStore"; | |||
| import UserActivity from "./UserActivity"; | |||
There was a problem hiding this comment.
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 😋
This has now had a round of testing from @chagai95 (thanks!). Bug 1
We seem to be failing to pull out a string somewhere. Bug 2
Though @chagai95 has shown interest in instead displaying Either way, the title of the notification needs some fixing. |
|
@anoadragon453 I'm afraid I don't quite know where this PR is at. Is it stuck with me or with you? (or neither and needs taking over?) |
|
I think I can answer this, @dbkr is going to take this over but it'll take some time because there are some other higher priority tickets. |
dbkr
changed the title
Co-authored-by: Travis Ralston <travisr@matrix.org>
Co-authored-by: Travis Ralston <travisr@matrix.org>
|
Turns out the reason the type were unhappy is because we added async method calls but everything here is sync :/ On second thoughts, I'm going to make new PRs for this: there are two distinct changes in here so there's no need for them to block each other. |
When virtual room support is enabled, calls to native matrix users who
also have a third-party, bridged identity - commonly known as a "virtual
user" - will go into a separate, "virtual" room that is hidden from the
client. Whereas text messages will be sent directly between native
users over Matrix.
Due to this multi-room split, we have to use the VoipUserMapper class to
translate between native and virtual users. It does so by performing a
third-party lookup of either the native user's ID, or the virtual user's
ID, receiving the other ID in return from an application service.
The bug that's being fixed here is that when a call comes in from a
virtual user (in the virtual room), Element will send a notification
saying "Virtual User placed a video call". Clicking it will also bring
you to the virtual room, both of which is undesirable. Instead, when a
call appears in a virtual room, ideally the notification would show the
profile information of the native user, as well as clicking the
notification should take you to the native room.
Note: This has not yet been tested.
Signed-off-by: Andrew Morgan <andrewm@element.io>Here's what your changelog entry will look like:
🐛 Bug Fixes
Preview: https://61fac6426710be1a1b71d40e--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.