Skip to content

Commit

Permalink
runfix: mls 1:1 accept flicker (#17505)
Browse files Browse the repository at this point in the history 
* refactor: improve naming

* runfix: resolve 1:1 conversation when receiving member join

* runfix: resolve 1:1 on welcome message immediately

* test: update tests

* test: test naming

* test: resolve 1:1 on member join event in 1:1 conversation
  • Loading branch information
@PatrykBuniX
PatrykBuniX authored Jun 4, 2024
1 parent 6a17aaf commit 37f8509
Show file tree
Hide file tree
Showing 3 changed files with 108 additions and 33 deletions.
105 changes: 86 additions & 19 deletions src/script/conversation/ConversationRepository.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ describe('ConversationRepository', () => {
});
});

describe('getInitialised1To1Conversation', () => {
describe('resolve1To1Conversation', () => {
beforeEach(() => {
testFactory.conversation_repository['conversationState'].conversations([]);
testFactory.conversation_repository['userState'].users([]);
Expand Down Expand Up @@ -419,7 +419,7 @@ describe('ConversationRepository', () => {

jest.spyOn(conversationRepository['userState'], 'self').mockReturnValue(selfUser);

const conversationEntity = await conversationRepository.getInitialised1To1Conversation(userEntity.qualifiedId);
const conversationEntity = await conversationRepository.resolve1To1Conversation(userEntity.qualifiedId);

expect(conversationEntity).toBe(newConversationEntity);
});
Expand Down Expand Up @@ -457,7 +457,7 @@ describe('ConversationRepository', () => {
.spyOn(conversationRepository['conversationService'], 'getConversationById')
.mockResolvedValueOnce(proteus1to1ConversationResponse);

const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId);
const conversationEntity = await conversationRepository.resolve1To1Conversation(otherUser.qualifiedId);

expect(conversationEntity?.serialize()).toEqual(proteus1to1Conversation.serialize());
expect(conversationEntity?.readOnlyState()).toEqual(null);
Expand Down Expand Up @@ -497,7 +497,7 @@ describe('ConversationRepository', () => {
.spyOn(conversationRepository['conversationService'], 'getConversationById')
.mockResolvedValueOnce(proteus1to1ConversationResponse);

const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId);
const conversationEntity = await conversationRepository.resolve1To1Conversation(otherUser.qualifiedId);

expect(conversationEntity?.readOnlyState()).toEqual(
CONVERSATION_READONLY_STATE.READONLY_ONE_TO_ONE_SELF_UNSUPPORTED_MLS,
Expand Down Expand Up @@ -538,7 +538,7 @@ describe('ConversationRepository', () => {
.spyOn(conversationRepository['conversationService'], 'isMLSGroupEstablishedLocally')
.mockResolvedValueOnce(true);

const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId);
const conversationEntity = await conversationRepository.resolve1To1Conversation(otherUser.qualifiedId);

expect(conversationEntity?.serialize()).toEqual(mls1to1Conversation.serialize());
});
Expand Down Expand Up @@ -623,7 +623,7 @@ describe('ConversationRepository', () => {
jest.spyOn(conversationRepository['conversationService'], 'blacklistConversation');
jest.spyOn(conversationRepository['eventRepository'], 'injectEvent').mockResolvedValueOnce(undefined);

const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId);
const conversationEntity = await conversationRepository.resolve1To1Conversation(otherUser.qualifiedId);

expect(conversationRepository['eventService'].moveEventsToConversation).toHaveBeenCalledWith(
proteus1to1Conversation.qualifiedId,
Expand Down Expand Up @@ -707,7 +707,7 @@ describe('ConversationRepository', () => {

const [mls1to1Conversation] = conversationRepository.mapConversations([establishedMls1to1ConversationResponse]);

const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId);
const conversationEntity = await conversationRepository.resolve1To1Conversation(otherUser.qualifiedId);

expect(container.resolve(Core).service!.conversation.establishMLS1to1Conversation).toHaveBeenCalledWith(
mockedGroupId,
Expand Down Expand Up @@ -773,7 +773,7 @@ describe('ConversationRepository', () => {
selfUser.supportedProtocols([ConversationProtocol.PROTEUS, ConversationProtocol.MLS]);
jest.spyOn(conversationRepository['userState'], 'self').mockReturnValueOnce(selfUser);

await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId);
await conversationRepository.resolve1To1Conversation(otherUser.qualifiedId);

expect(container.resolve(Core).service!.conversation.establishMLS1to1Conversation).toHaveBeenCalled();
});
Expand Down Expand Up @@ -827,7 +827,7 @@ describe('ConversationRepository', () => {
.spyOn(conversationRepository['conversationService'], 'isMLSGroupEstablishedLocally')
.mockResolvedValueOnce(false);

const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId);
const conversationEntity = await conversationRepository.resolve1To1Conversation(otherUser.qualifiedId);

expect(conversationEntity?.serialize()).toEqual(mls1to1Conversation.serialize());
expect(container.resolve(Core).service!.conversation.establishMLS1to1Conversation).toHaveBeenCalled();
Expand Down Expand Up @@ -868,7 +868,7 @@ describe('ConversationRepository', () => {
.spyOn(conversationRepository['conversationService'], 'isMLSGroupEstablishedLocally')
.mockResolvedValueOnce(true);

const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId);
const conversationEntity = await conversationRepository.resolve1To1Conversation(otherUser.qualifiedId);

expect(conversationEntity?.serialize()).toEqual(mls1to1Conversation.serialize());
});
Expand Down Expand Up @@ -908,7 +908,7 @@ describe('ConversationRepository', () => {
.spyOn(conversationRepository['conversationService'], 'isMLSGroupEstablishedLocally')
.mockResolvedValueOnce(false);

const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId);
const conversationEntity = await conversationRepository.resolve1To1Conversation(otherUser.qualifiedId);

expect(conversationEntity?.serialize()).toEqual(mls1to1Conversation.serialize());
expect(conversationEntity?.readOnlyState()).toEqual(
Expand Down Expand Up @@ -957,7 +957,7 @@ describe('ConversationRepository', () => {
.spyOn(conversationRepository['conversationService'], 'isMLSGroupEstablishedLocally')
.mockResolvedValueOnce(false);

const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId);
const conversationEntity = await conversationRepository.resolve1To1Conversation(otherUser.qualifiedId);

expect(conversationEntity?.serialize()).toEqual(mls1to1Conversation.serialize());
expect(conversationEntity?.readOnlyState()).toEqual(
Expand Down Expand Up @@ -1000,7 +1000,7 @@ describe('ConversationRepository', () => {
.spyOn(conversationRepository['conversationService'], 'isMLSGroupEstablishedLocally')
.mockResolvedValueOnce(true);

const conversationEntity = await conversationRepository.getInitialised1To1Conversation(otherUser.qualifiedId);
const conversationEntity = await conversationRepository.resolve1To1Conversation(otherUser.qualifiedId);

expect(conversationEntity?.serialize()).toEqual(mls1to1Conversation.serialize());
expect(conversationEntity?.readOnlyState()).toEqual(null);
Expand Down Expand Up @@ -1070,15 +1070,15 @@ describe('ConversationRepository', () => {
configurable: true,
});

jest.spyOn(conversationRepository, 'getInitialised1To1Conversation');
jest.spyOn(conversationRepository, 'resolve1To1Conversation');

userRepository.emit('supportedProtocolsUpdated', {
user: otherUser,
supportedProtocols: [ConversationProtocol.PROTEUS, ConversationProtocol.MLS],
});

await waitFor(() => {
expect(conversationRepository.getInitialised1To1Conversation).toHaveBeenCalledWith(otherUser.qualifiedId, {
expect(conversationRepository.resolve1To1Conversation).toHaveBeenCalledWith(otherUser.qualifiedId, {
isLiveUpdate: true,
});
});
Expand All @@ -1091,7 +1091,7 @@ describe('ConversationRepository', () => {
const otherUserId = {id: 'f718410c-3833-479d-bd80-a5df03f38414', domain: 'test-domain'};
const otherUser = new User(otherUserId.id, otherUserId.domain);

jest.spyOn(conversationRepository, 'getInitialised1To1Conversation');
jest.spyOn(conversationRepository, 'resolve1To1Conversation');

conversationRepository['conversationState'].conversations([]);

Expand All @@ -1101,7 +1101,7 @@ describe('ConversationRepository', () => {
});

await waitFor(() => {
expect(conversationRepository.getInitialised1To1Conversation).not.toHaveBeenCalled();
expect(conversationRepository.resolve1To1Conversation).not.toHaveBeenCalled();
});
});
});
Expand Down Expand Up @@ -1749,6 +1749,8 @@ describe('ConversationRepository', () => {
Promise.resolve(true),
);

conversation_et = _generateConversation();

memberJoinEvent = {
conversation: conversation_et.id,
data: {
Expand Down Expand Up @@ -1841,12 +1843,29 @@ describe('ConversationRepository', () => {
},
);

it('should ignore member-join event when joining a 1to1 conversation', () => {
it('should ignore member-join event when accepting a 1to1 conversation', () => {
const selfUser = generateUser();
const conversation = _generateConversation({
id: {id: 'one2one-id', domain: 'one2one-domain'},
type: CONVERSATION_TYPE.ONE_TO_ONE,
});

const memberJoinEvent = {
conversation: conversation.id,
data: {
user_ids: [selfUser.id],
},
from: selfUser.id,
time: '2015-04-27T11:42:31.475Z',
type: CONVERSATION_EVENT.MEMBER_JOIN,
} as ConversationMemberJoinEvent;

const conversationRepo = testFactory.conversation_repository!;
conversationRepo['conversationState'].conversations.push(conversation);

// conversation has a corresponding pending connection
const connectionEntity = new ConnectionEntity();
connectionEntity.conversationId = conversation_et.qualifiedId;
connectionEntity.conversationId = conversation.qualifiedId;
connectionEntity.userId = {domain: '', id: ''};
connectionEntity.status(ConnectionStatus.PENDING);
testFactory.connection_repository!.addConnectionEntity(connectionEntity);
Expand All @@ -1858,6 +1877,54 @@ describe('ConversationRepository', () => {
expect(conversationRepo.updateParticipatingUserEntities).not.toHaveBeenCalled();
});
});

it('should resolve 1:1 conversation on member-join event', () => {
const selfUser = generateUser();
const conversation = _generateConversation({
id: {id: 'one2one2-id', domain: 'one2one2-domain'},
type: CONVERSATION_TYPE.ONE_TO_ONE,
});

const otherUser = generateUser();

conversation.participating_user_ids.push(otherUser.qualifiedId);

const memberJoinEvent = {
conversation: conversation.id,
data: {
user_ids: [selfUser.id],
},
from: selfUser.id,
time: '2015-04-27T11:42:31.475Z',
type: CONVERSATION_EVENT.MEMBER_JOIN,
} as ConversationMemberJoinEvent;

const conversationRepo = testFactory.conversation_repository!;
const userRepo = testFactory.user_repository!;
conversationRepo['conversationState'].conversations.push(conversation);

jest.spyOn(conversationRepo, 'resolve1To1Conversation').mockResolvedValueOnce(conversation);

jest
.spyOn(conversationRepo['conversationService'], 'getConversationById')
.mockResolvedValue(generateAPIConversation({}));

jest.spyOn(userRepo, 'getUserById').mockResolvedValue(selfUser);

const connectionEntity = new ConnectionEntity();
connectionEntity.conversationId = conversation.qualifiedId;
connectionEntity.userId = otherUser.qualifiedId;
connectionEntity.status(ConnectionStatus.SENT);
testFactory.connection_repository!.addConnectionEntity(connectionEntity);

spyOn(conversationRepo!['userState'], 'self').and.returnValue(selfUser);

return conversationRepo['handleConversationEvent'](memberJoinEvent).then(() => {
expect(conversationRepo['onMemberJoin']).toHaveBeenCalled();
expect(conversationRepo.updateParticipatingUserEntities).toHaveBeenCalled();
expect(conversationRepo.resolve1To1Conversation).toHaveBeenCalled();
});
});
});

describe('conversation.message-delete', () => {
Expand Down
27 changes: 18 additions & 9 deletions src/script/conversation/ConversationRepository.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ export enum CONVERSATION_READONLY_STATE {
READONLY_ONE_TO_ONE_NO_KEY_PACKAGES = 'READONLY_ONE_TO_ONE_NO_KEY_PACKAGES',
}

interface GetInitialised1To1ConversationOptions {
interface Resolve1To1ConversationOptions {
isLiveUpdate?: boolean;
shouldRefreshUser?: boolean;
mls?: {allowUnestablished?: boolean};
Expand Down Expand Up @@ -1041,7 +1041,7 @@ export class ConversationRepository {
* Update conversation with a user you just unblocked
*/
private readonly onUnblockUser = async (user_et: User): Promise<void> => {
const conversationEntity = await this.getInitialised1To1Conversation(user_et.qualifiedId);
const conversationEntity = await this.resolve1To1Conversation(user_et.qualifiedId);
if (conversationEntity) {
conversationEntity.status(ConversationStatus.CURRENT_MEMBER);
}
Expand Down Expand Up @@ -1298,9 +1298,9 @@ export class ConversationRepository {
* @param knownConversationId Known conversation ID - if provided, we will try to find the conversation with this exact ID (needed for proteus 1:1 conversation with a team member)
* @returns Resolves with the initialised 1:1 conversation with requested user
*/
public async getInitialised1To1Conversation(
public async resolve1To1Conversation(
userId: QualifiedId,
options: GetInitialised1To1ConversationOptions = {
options: Resolve1To1ConversationOptions = {
isLiveUpdate: false,
shouldRefreshUser: false,
},
Expand Down Expand Up @@ -1942,15 +1942,15 @@ export class ConversationRepository {
);

try {
return await this.getInitialised1To1Conversation(otherUserId, {shouldRefreshUser}, conversation.qualifiedId);
return await this.resolve1To1Conversation(otherUserId, {shouldRefreshUser}, conversation.qualifiedId);
} catch {}

return conversation;
};

private readonly get1to1ConversationForConnection = async (
connection: ConnectionEntity,
options: GetInitialised1To1ConversationOptions = {
options: Resolve1To1ConversationOptions = {
isLiveUpdate: false,
shouldRefreshUser: false,
},
Expand Down Expand Up @@ -2037,7 +2037,7 @@ export class ConversationRepository {
): Promise<Conversation | undefined> => {
try {
const userId = connectionEntity.userId;
const conversation = await this.getInitialised1To1Conversation(userId, {
const conversation = await this.resolve1To1Conversation(userId, {
isLiveUpdate: source === EventSource.WEBSOCKET,
});

Expand Down Expand Up @@ -2095,7 +2095,7 @@ export class ConversationRepository {
return;
}

await this.getInitialised1To1Conversation(user.qualifiedId, {isLiveUpdate: true});
await this.resolve1To1Conversation(user.qualifiedId, {isLiveUpdate: true});
};

/**
Expand Down Expand Up @@ -3543,6 +3543,15 @@ export class ConversationRepository {
});
}

const is1to1Conversation = conversationEntity.is1to1() || conversationEntity.isRequest();

if (is1to1Conversation) {
const otherUserId = conversationEntity.participating_user_ids()[0];
if (otherUserId) {
await this.resolve1To1Conversation(otherUserId, {isLiveUpdate: true});
}
}

// Self user is a creator of the event
const isFromSelf = eventJson.from === this.userState.self().id;

Expand Down Expand Up @@ -3947,7 +3956,7 @@ export class ConversationRepository {
return;
}

await this.getInitialised1To1Conversation(otherUserId, {isLiveUpdate: true});
await this.resolve1To1Conversation(otherUserId);
}

/**
Expand Down
9 changes: 4 additions & 5 deletions src/script/view_model/ActionsViewModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -336,10 +336,9 @@ export class ActionsViewModel {
};

getOrCreate1to1Conversation = async (userEntity: User): Promise<Conversation> => {
const conversationEntity = await this.conversationRepository.getInitialised1To1Conversation(
userEntity.qualifiedId,
{mls: {allowUnestablished: false}},
);
const conversationEntity = await this.conversationRepository.resolve1To1Conversation(userEntity.qualifiedId, {
mls: {allowUnestablished: false},
});
if (conversationEntity) {
return conversationEntity;
}
Expand Down Expand Up @@ -434,7 +433,7 @@ export class ActionsViewModel {
primaryAction: {
action: async () => {
await this.connectionRepository.unblockUser(userEntity);
const conversationEntity = await this.conversationRepository.getInitialised1To1Conversation(
const conversationEntity = await this.conversationRepository.resolve1To1Conversation(
userEntity.qualifiedId,
);
resolve();
Expand Down

0 comments on commit 37f8509

Please sign in to comment.