From c748e3ceac77322517cefd08be00418ab3509c9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20G=C3=B3rka?= Date: Thu, 4 Apr 2024 10:26:30 +0200 Subject: [PATCH] refactor: init 1to1 conversations (#17207) * refactor: add generic method for initialising 1:1 conversaiton by userid * refactor: get initialised 1:1 for connected user * test: update tests * refactor: init 1:1 conversation * chore: improve logs * refactor: get initialised 1:1 params * refactor: get 1:1 for connection options --- .../ConversationRepository.test.ts | 65 ++++++--- .../conversation/ConversationRepository.ts | 135 ++++++++++-------- src/script/view_model/ActionsViewModel.ts | 6 +- src/script/view_model/ContentViewModel.ts | 2 +- 4 files changed, 121 insertions(+), 87 deletions(-) diff --git a/src/script/conversation/ConversationRepository.test.ts b/src/script/conversation/ConversationRepository.test.ts index d3261811731..4a5ae8a058a 100644 --- a/src/script/conversation/ConversationRepository.test.ts +++ b/src/script/conversation/ConversationRepository.test.ts @@ -377,7 +377,7 @@ describe('ConversationRepository', () => { jest.spyOn(conversationRepository['userState'], 'self').mockReturnValue(selfUser); - const conversationEntity = await conversationRepository.getInitialised1To1Conversation(userEntity); + const conversationEntity = await conversationRepository.getInitialised1To1Conversation(userEntity.qualifiedId); expect(conversationEntity).toBe(newConversationEntity); }); @@ -403,6 +403,8 @@ describe('ConversationRepository', () => { }) as BackendConversation; const connection = new ConnectionEntity(); + connection.userId = otherUserId; + connection.status(ConnectionStatus.ACCEPTED); connection.conversationId = proteus1to1ConversationResponse.qualified_id; otherUser.connection(connection); @@ -413,7 +415,7 @@ describe('ConversationRepository', () => { .spyOn(conversationRepository['conversationService'], 'getConversationById') .mockResolvedValueOnce(proteus1to1ConversationResponse); - const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser); + const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId); expect(conversationEntity?.serialize()).toEqual(proteus1to1Conversation.serialize()); expect(conversationEntity?.readOnlyState()).toEqual(null); @@ -443,6 +445,8 @@ describe('ConversationRepository', () => { }) as BackendConversation; const connection = new ConnectionEntity(); + connection.status(ConnectionStatus.ACCEPTED); + connection.userId = otherUserId; connection.conversationId = proteus1to1ConversationResponse.qualified_id; otherUser.connection(connection); @@ -451,7 +455,7 @@ describe('ConversationRepository', () => { .spyOn(conversationRepository['conversationService'], 'getConversationById') .mockResolvedValueOnce(proteus1to1ConversationResponse); - const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser); + const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId); expect(conversationEntity?.readOnlyState()).toEqual( CONVERSATION_READONLY_STATE.READONLY_ONE_TO_ONE_SELF_UNSUPPORTED_MLS, @@ -492,7 +496,7 @@ describe('ConversationRepository', () => { .spyOn(conversationRepository['conversationService'], 'isMLSGroupEstablishedLocally') .mockResolvedValueOnce(true); - const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser); + const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId); expect(conversationEntity?.serialize()).toEqual(mls1to1Conversation.serialize()); }); @@ -577,7 +581,7 @@ describe('ConversationRepository', () => { jest.spyOn(conversationRepository['conversationService'], 'blacklistConversation'); jest.spyOn(conversationRepository['eventRepository'], 'injectEvent').mockResolvedValueOnce(undefined); - const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser); + const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId); expect(conversationRepository['eventService'].moveEventsToConversation).toHaveBeenCalledWith( proteus1to1Conversation.id, @@ -661,7 +665,7 @@ describe('ConversationRepository', () => { const [mls1to1Conversation] = conversationRepository.mapConversations([establishedMls1to1ConversationResponse]); - const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser); + const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId); expect(container.resolve(Core).service!.conversation.establishMLS1to1Conversation).toHaveBeenCalledWith( mockedGroupId, @@ -741,7 +745,7 @@ describe('ConversationRepository', () => { .spyOn(conversationRepository['conversationService'], 'isMLSGroupEstablishedLocally') .mockResolvedValueOnce(false); - const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser); + const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId); expect(conversationEntity?.serialize()).toEqual(mls1to1Conversation.serialize()); expect(container.resolve(Core).service!.conversation.establishMLS1to1Conversation).toHaveBeenCalled(); @@ -782,7 +786,7 @@ describe('ConversationRepository', () => { .spyOn(conversationRepository['conversationService'], 'isMLSGroupEstablishedLocally') .mockResolvedValueOnce(true); - const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser); + const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId); expect(conversationEntity?.serialize()).toEqual(mls1to1Conversation.serialize()); }); @@ -822,7 +826,7 @@ describe('ConversationRepository', () => { .spyOn(conversationRepository['conversationService'], 'isMLSGroupEstablishedLocally') .mockResolvedValueOnce(false); - const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser); + const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId); expect(conversationEntity?.serialize()).toEqual(mls1to1Conversation.serialize()); expect(conversationEntity?.readOnlyState()).toEqual( @@ -865,7 +869,7 @@ describe('ConversationRepository', () => { .spyOn(conversationRepository['conversationService'], 'isMLSGroupEstablishedLocally') .mockResolvedValueOnce(true); - const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser); + const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId); expect(conversationEntity?.serialize()).toEqual(mls1to1Conversation.serialize()); expect(conversationEntity?.readOnlyState()).toEqual(null); @@ -943,7 +947,9 @@ describe('ConversationRepository', () => { }); await waitFor(() => { - expect(conversationRepository.getInitialised1To1Conversation).toHaveBeenCalledWith(otherUser, true); + expect(conversationRepository.getInitialised1To1Conversation).toHaveBeenCalledWith(otherUser.qualifiedId, { + isLiveUpdate: true, + }); }); }); @@ -990,6 +996,7 @@ describe('ConversationRepository', () => { const connection = new ConnectionEntity(); connection.status(ConnectionStatus.SENT); connection.conversationId = conversation.qualifiedId; + connection.userId = otherUserId; otherUser.connection(connection); jest.spyOn(conversationRepository['conversationService'], 'removeConversationFromBlacklist'); @@ -1186,6 +1193,12 @@ describe('ConversationRepository', () => { it('should map a connection to an existing conversation', () => { conversation_et.type(CONVERSATION_TYPE.ONE_TO_ONE); + const conversationRepository = testFactory.conversation_repository!; + const user = new User('id', 'domain'); + user.connection(connectionEntity); + connectionEntity.userId = user.qualifiedId; + conversationRepository['userState'].users.push(user); + return testFactory.conversation_repository['mapConnection'](connectionEntity).then( (_conversation: Conversation) => { expect(testFactory.conversation_repository['fetchConversationById']).not.toHaveBeenCalled(); @@ -1196,31 +1209,40 @@ describe('ConversationRepository', () => { }); it('should map a connection to a new conversation', () => { + const conversationRepository = testFactory.conversation_repository!; + const user = new User('id1', 'domain1'); + user.connection(connectionEntity); + connectionEntity.userId = user.qualifiedId; + conversationRepository['userState'].users.push(user); + connectionEntity.status(ConnectionStatus.ACCEPTED); - testFactory.conversation_repository['conversationState'].conversations.removeAll(); + conversationRepository['conversationState'].conversations.removeAll(); - return testFactory.conversation_repository['mapConnection'](connectionEntity).then(_conversation => { - expect(testFactory.conversation_repository['fetchConversationById']).toHaveBeenCalled(); + return conversationRepository['mapConnection'](connectionEntity).then(_conversation => { + expect(conversationRepository['fetchConversationById']).toHaveBeenCalled(); expect(testFactory.conversation_service.getConversationById).toHaveBeenCalled(); expect(_conversation.connection()).toBe(connectionEntity); }); }); it('should map a cancelled connection to an existing conversation and filter it', () => { + const conversationRepository = testFactory.conversation_repository!; + const user = new User('id', 'domain'); + user.connection(connectionEntity); + connectionEntity.userId = user.qualifiedId; + conversationRepository['userState'].users.push(user); + conversation_et.type(CONVERSATION_TYPE.ONE_TO_ONE); connectionEntity.status(ConnectionStatus.CANCELLED); - return testFactory.conversation_repository['mapConnection'](connectionEntity).then(_conversation => { + return conversationRepository['mapConnection'](connectionEntity).then(_conversation => { expect(_conversation.connection()).toBe(connectionEntity); expect( - _findConversation(_conversation, testFactory.conversation_repository['conversationState'].conversations), + _findConversation(_conversation, conversationRepository['conversationState'].conversations), ).not.toBeUndefined(); expect( - _findConversation( - _conversation, - testFactory.conversation_repository['conversationState'].filteredConversations, - ), + _findConversation(_conversation, conversationRepository['conversationState'].filteredConversations), ).toBeUndefined(); }); }); @@ -1273,8 +1295,9 @@ describe('ConversationRepository', () => { protocol: ConversationProtocol.MLS, }); - const otherUserId = {id: 'f718410c-3833-479d-bd80-a5df03f38414', domain: 'test-domain'}; + const otherUserId = {id: 'f718410c-3833-479d-bd80-a5df01138411', domain: 'test-domain'}; const otherUser = new User(otherUserId.id, otherUserId.domain); + otherUser.supportedProtocols([ConversationProtocol.PROTEUS, ConversationProtocol.MLS]); conversationRepository['userState'].users.push(otherUser); conversation.participating_user_ids.push(otherUserId); diff --git a/src/script/conversation/ConversationRepository.ts b/src/script/conversation/ConversationRepository.ts index 1cd52048c96..bd443bac833 100644 --- a/src/script/conversation/ConversationRepository.ts +++ b/src/script/conversation/ConversationRepository.ts @@ -169,6 +169,11 @@ export enum CONVERSATION_READONLY_STATE { READONLY_ONE_TO_ONE_OTHER_UNSUPPORTED_MLS = 'READONLY_ONE_TO_ONE_OTHER_UNSUPPORTED_MLS', } +interface GetInitialised1To1ConversationOptions { + isLiveUpdate?: boolean; + shouldRefreshUser?: boolean; +} + export class ConversationRepository { private isBlockingNotificationHandling: boolean; private readonly ephemeralHandler: ConversationEphemeralHandler; @@ -1033,7 +1038,7 @@ export class ConversationRepository { * Update conversation with a user you just unblocked */ private readonly onUnblockUser = async (user_et: User): Promise => { - const conversationEntity = await this.getInitialised1To1Conversation(user_et); + const conversationEntity = await this.getInitialised1To1Conversation(user_et.qualifiedId); if (conversationEntity) { conversationEntity.status(ConversationStatus.CURRENT_MEMBER); } @@ -1278,26 +1283,42 @@ export class ConversationRepository { * If there's no common protocol, it will pick the protocol that is supported by the current user and mark conversation as read-only. * @param userEntity User entity for whom to get the conversation * @param isLiveUpdate Whether the conversation is being initialised because of a live update (e.g. some websocket event) + * @param shouldRefreshUser Whether the user should be refetched from backend before getting the conversation * @returns Resolves with the initialised 1:1 conversation with requested user */ - async getInitialised1To1Conversation(userEntity: User, isLiveUpdate = false): Promise { - const {qualifiedId: otherUserId} = userEntity; + public async getInitialised1To1Conversation( + userId: QualifiedId, + options: GetInitialised1To1ConversationOptions = { + isLiveUpdate: false, + shouldRefreshUser: false, + }, + ): Promise { + const user = await this.userRepository.getUserById(userId); + + const connection = user.connection(); + if (connection) { + this.logger.log(`There's a connection with user ${userId.id}, getting a 1:1 conversation for the connection`); + return this.get1to1ConversationForConnection(connection, options); + } const {protocol, isMLSSupportedByTheOtherUser, isProteusSupportedByTheOtherUser} = - await this.getProtocolFor1to1Conversation(otherUserId); + await this.getProtocolFor1to1Conversation(userId, options.shouldRefreshUser); + this.logger.log( + `Protocol for 1:1 conversation with user ${userId.id} is ${protocol}, isMLSSupportedByTheOtherUser: ${isMLSSupportedByTheOtherUser}, isProteusSupportedByTheOtherUser: ${isProteusSupportedByTheOtherUser}`, + ); - const localMLSConversation = this.conversationState.findMLS1to1Conversation(otherUserId); + const localMLSConversation = this.conversationState.findMLS1to1Conversation(userId); if (protocol === ConversationProtocol.MLS || localMLSConversation) { /** * When mls 1:1 conversation initialisation is triggered by some live update (e.g other user updates their supported protocols), it's very likely that we will also receive a welcome message shortly. * We have to add a delay to make sure the welcome message is not wasted, in case the self client would establish mls group themselves before receiving the welcome. */ - const shouldDelayMLSGroupEstablishment = isLiveUpdate && isMLSSupportedByTheOtherUser; - return this.initMLS1to1Conversation(otherUserId, isMLSSupportedByTheOtherUser, shouldDelayMLSGroupEstablishment); + const shouldDelayMLSGroupEstablishment = options.isLiveUpdate && isMLSSupportedByTheOtherUser; + return this.initMLS1to1Conversation(userId, isMLSSupportedByTheOtherUser, shouldDelayMLSGroupEstablishment); } - const proteusConversation = await this.getOrCreateProteus1To1Conversation(userEntity); + const proteusConversation = await this.getOrCreateProteus1To1Conversation(user); if (!proteusConversation) { return null; @@ -1925,7 +1946,7 @@ export class ConversationRepository { public readonly init1to1Conversation = async ( conversation: Conversation, shouldRefreshUser = false, - ): Promise => { + ): Promise => { if (!conversation.is1to1()) { throw new Error('Conversation is not 1:1'); } @@ -1937,87 +1958,64 @@ export class ConversationRepository { return conversation; } + const otherUser = await this.userRepository.getUserById(otherUserId); + // If the other user is deleted on backend, just open a conversation and do not try to migrate it to mls. + if (otherUser.isDeleted) { + this.logger.warn(`User ${otherUserId.id} is deleted, opening proteus conversation`); + return conversation; + } + this.logger.info( `Initialising 1:1 conversation ${conversation.id} of type ${conversation.type()} with user ${otherUserId.id}`, ); try { - const {protocol, isMLSSupportedByTheOtherUser, isProteusSupportedByTheOtherUser} = - await this.getProtocolFor1to1Conversation(otherUserId, shouldRefreshUser); - this.logger.info( - `Protocol for 1:1 conversation ${conversation.id} with user ${otherUserId.id} is ${protocol}, ${JSON.stringify({ - isMLSSupportedByTheOtherUser, - isProteusSupportedByTheOtherUser, - })}`, - ); - - // When called with mls conversation, we just make sure it is initialised. - if (isMLSConversation(conversation)) { - return this.initMLS1to1Conversation(otherUserId, isMLSSupportedByTheOtherUser); - } - - // If there's local mls conversation, we want to use it - const localMLSConversation = this.conversationState.findMLS1to1Conversation(otherUserId); - - // If mls conversation is already known, we use it - // we never go back to proteus conversation, even if one of the users do not support mls anymore - // (e.g. due to the change of supported protocols in team configuration) - if (localMLSConversation) { - return this.initMLS1to1Conversation(otherUserId, isMLSSupportedByTheOtherUser); - } - - // If both users support mls, we will initialise mls 1:1 conversation (unless it's a proteus 1:1 conversation with a user that has been deleted) - if (protocol === ConversationProtocol.MLS) { - // If the other user is deleted on backend, just open a proteus conversation and do not try to migrate it to mls. - if (isProteusConversation(conversation)) { - const otherUser = await this.userRepository.getUserById(otherUserId); - if (otherUser.isDeleted) { - this.logger.warn(`User ${otherUserId.id} is deleted, opening proteus conversation`); - return conversation; - } - } - - return this.initMLS1to1Conversation(otherUserId, isMLSSupportedByTheOtherUser); - } - - if (protocol === ConversationProtocol.PROTEUS && isProteusConversation(conversation)) { - return this.initProteus1to1Conversation(conversation.qualifiedId, isProteusSupportedByTheOtherUser); - } + return await this.getInitialised1To1Conversation(otherUserId, {shouldRefreshUser}); } catch {} return conversation; }; - private readonly getConnectionConversation = async (connectionEntity: ConnectionEntity, source?: EventSource) => { + private readonly get1to1ConversationForConnection = async ( + connection: ConnectionEntity, + options: GetInitialised1To1ConversationOptions = { + isLiveUpdate: false, + shouldRefreshUser: false, + }, + ): Promise => { // As of how backed works now (August 2023), proteus 1:1 conversations will always be created, even if both users support MLS conversation. // Proteus 1:1 conversation is created right after a connection request is sent. // Therefore, conversationId filed on connectionEntity will always indicate proteus 1:1 conversation. // We need to manually check if mls 1:1 conversation can be used instead. // If mls 1:1 conversation is used, proteus 1:1 conversation will be deleted locally. - const {conversationId: proteusConversationId, userId: otherUserId} = connectionEntity; - const localProteusConversation = this.conversationState.findConversation(proteusConversationId); + const {conversationId: proteusConversationId, userId: otherUserId} = connection; + const localProteusConversation = this.conversationState.findConversation(proteusConversationId) || null; // For connection request, we simply display proteus conversation of type 3 (connect) it will be displayed as a connection request - if (connectionEntity.isOutgoingRequest()) { + if (connection.isOutgoingRequest()) { + this.logger.log( + `Connection request with user ${otherUserId.id}, using proteus conversation ${proteusConversationId.id}`, + ); const proteusConversation = localProteusConversation || (await this.fetchConversationById(proteusConversationId)); proteusConversation.type(CONVERSATION_TYPE.CONNECT); return proteusConversation; } - const isConnectionAccepted = connectionEntity.isConnected(); - // Check what protocol should be used for 1:1 conversation const {protocol, isMLSSupportedByTheOtherUser, isProteusSupportedByTheOtherUser} = - await this.getProtocolFor1to1Conversation(otherUserId); + await this.getProtocolFor1to1Conversation(otherUserId, options.shouldRefreshUser); - const isWebSocketEvent = source === EventSource.WEBSOCKET; - const shouldDelayMLSGroupEstablishment = isWebSocketEvent && isMLSSupportedByTheOtherUser; + const shouldDelayMLSGroupEstablishment = options.isLiveUpdate && isMLSSupportedByTheOtherUser; const localMLSConversation = this.conversationState.findMLS1to1Conversation(otherUserId); + const isConnectionAccepted = connection.isConnected(); // If it's accepted, initialise conversation so it's ready to be used if (isConnectionAccepted) { + this.logger.log( + `Connection with user ${otherUserId.id} is accepted, using protocol ${protocol} for 1:1 conversation`, + ); if (protocol === ConversationProtocol.MLS || localMLSConversation) { return this.initMLS1to1Conversation( otherUserId, @@ -2035,11 +2033,19 @@ export class ConversationRepository { // If we already know mls 1:1 conversation, we use it, even if proteus protocol was now choosen as common, // we do not support switching back to proteus after mls conversation was established, // only proteus -> mls migration is supported, never the other way around. + if (localMLSConversation) { + this.logger.log( + `Connection with user ${otherUserId.id} is not accepted, using already known MLS 1:1 conversation ${localMLSConversation.id}`, + ); return this.initMLS1to1Conversation(otherUserId, isMLSSupportedByTheOtherUser, shouldDelayMLSGroupEstablishment); } - return protocol === ConversationProtocol.PROTEUS ? localProteusConversation : undefined; + this.logger.log( + `Connection with user ${otherUserId.id} is not accepted, defaulting to local proteus 1:1 conversation ${proteusConversationId.id}`, + ); + + return protocol === ConversationProtocol.PROTEUS ? localProteusConversation : null; }; /** @@ -2056,7 +2062,10 @@ export class ConversationRepository { source?: EventSource, ): Promise => { try { - const conversation = await this.getConnectionConversation(connectionEntity, source); + const userId = connectionEntity.userId; + const conversation = await this.getInitialised1To1Conversation(userId, { + isLiveUpdate: source === EventSource.WEBSOCKET, + }); if (!conversation) { return undefined; @@ -2112,7 +2121,7 @@ export class ConversationRepository { return; } - await this.getInitialised1To1Conversation(user, true); + await this.getInitialised1To1Conversation(user.qualifiedId, {isLiveUpdate: true}); }; /** @@ -3964,7 +3973,7 @@ export class ConversationRepository { return; } - await this.initMLS1to1Conversation(otherUserId, true); + await this.getInitialised1To1Conversation(otherUserId, {isLiveUpdate: true}); } /** diff --git a/src/script/view_model/ActionsViewModel.ts b/src/script/view_model/ActionsViewModel.ts index 768ba2d5935..27858307424 100644 --- a/src/script/view_model/ActionsViewModel.ts +++ b/src/script/view_model/ActionsViewModel.ts @@ -336,7 +336,7 @@ export class ActionsViewModel { }; getOrCreate1to1Conversation = async (userEntity: User): Promise => { - const conversationEntity = await this.conversationRepository.getInitialised1To1Conversation(userEntity); + const conversationEntity = await this.conversationRepository.getInitialised1To1Conversation(userEntity.qualifiedId); if (conversationEntity) { return conversationEntity; } @@ -431,7 +431,9 @@ export class ActionsViewModel { primaryAction: { action: async () => { await this.connectionRepository.unblockUser(userEntity); - const conversationEntity = await this.conversationRepository.getInitialised1To1Conversation(userEntity); + const conversationEntity = await this.conversationRepository.getInitialised1To1Conversation( + userEntity.qualifiedId, + ); resolve(); if (conversationEntity) { await this.conversationRepository.updateParticipatingUserEntities(conversationEntity); diff --git a/src/script/view_model/ContentViewModel.ts b/src/script/view_model/ContentViewModel.ts index ddbdff86e64..d67f5c66453 100644 --- a/src/script/view_model/ContentViewModel.ts +++ b/src/script/view_model/ContentViewModel.ts @@ -141,7 +141,7 @@ export class ContentViewModel { private readonly getConversationEntity = async ( conversation: Conversation | string, domain: string | null = null, - ): Promise => { + ): Promise => { const conversationEntity = isConversationEntity(conversation) ? conversation : await this.conversationRepository.getConversationById({domain: domain || '', id: conversation});