Skip to content

Commit

Permalink
fix: Viewing an archived conversation should not automatically “unarc…
Browse files Browse the repository at this point in the history 
…hive” it (#15629)

* fix: self leave conversation shouldn't be archieved automatically

* fix: backup password hint

* fix: refactor showConversation method

* fix: don't unarchieve when an archieved conversation viewed and stay at archieve view

* fix: don't unarchive when a new message is received

* fix: open the archived view directly for archived conversation notification click

* feat: select archived conversation

* fix: call openConversations conditionally

* feat: remove shouldunaricve function which removes unarchive on events like new message, member join

* fix: remove double subscription of openConversation

* feat: use archivedState property to determine conversation type

* fix: remove unarchive on memeber join event

* fix: pipeline test

* Update src/script/notification/NotificationRepository.ts

Co-authored-by: Thomas Belin <[email protected]>

* fix: code review comments

---------

Co-authored-by: Thomas Belin <[email protected]>
  • Loading branch information
@arjita-mitra @atomrc
arjita-mitra and atomrc authored Oct 23, 2023
1 parent 154c054 commit dc9fef4
Show file tree
Hide file tree
Showing 7 changed files with 103 additions and 297 deletions.
45 changes: 2 additions & 43 deletions src/script/conversation/ConversationRepository.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,6 @@ type IncomingEvent = ConversationEvent | ClientConversationEvent;

export class ConversationRepository {
private isBlockingNotificationHandling: boolean;
private readonly conversationsWithNewEvents: Map<any, any>;
private readonly ephemeralHandler: ConversationEphemeralHandler;
public readonly conversationLabelRepository: ConversationLabelRepository;
public readonly conversationRoleRepository: ConversationRoleRepository;
Expand Down Expand Up @@ -284,7 +283,6 @@ export class ConversationRepository {
}

this.isBlockingNotificationHandling = true;
this.conversationsWithNewEvents = new Map();

this.teamState.isTeam.subscribe(() => this.mapGuestStatusSelf());

Expand Down Expand Up @@ -1431,9 +1429,6 @@ export class ConversationRepository {
const isFetchingFromStream = handlingState !== NOTIFICATION_HANDLING_STATE.WEB_SOCKET;

if (this.isBlockingNotificationHandling !== isFetchingFromStream) {
if (!isFetchingFromStream) {
this.checkChangedConversations();
}
this.isBlockingNotificationHandling = isFetchingFromStream;
this.logger.info(`Block handling of conversation events: ${this.isBlockingNotificationHandling}`);
}
Expand Down Expand Up @@ -1933,16 +1928,6 @@ export class ConversationRepository {
this.onMemberUpdate(conversationEntity, response);
}

private checkChangedConversations() {
this.conversationsWithNewEvents.forEach(conversationEntity => {
if (conversationEntity.shouldUnarchive()) {
this.unarchiveConversation(conversationEntity, false, ConversationRepository.eventFromStreamMessage);
}
});

this.conversationsWithNewEvents.clear();
}

/**
* Clears conversation content from view and the database.
*
Expand Down Expand Up @@ -2137,19 +2122,10 @@ export class ConversationRepository {
const onEventPromise = isConversationCreate
? Promise.resolve(null)
: this.getConversationById(conversationId, true);
let previouslyArchived = false;

return onEventPromise
.then((conversationEntity: Conversation) => {
if (conversationEntity) {
// Check if conversation was archived
previouslyArchived = conversationEntity.is_archived();
const isPastMemberStatus = conversationEntity.status() === ConversationStatus.PAST_MEMBER;
const isMemberJoinType = type === CONVERSATION_EVENT.MEMBER_JOIN;

if (previouslyArchived && isPastMemberStatus && isMemberJoinType) {
this.unarchiveConversation(conversationEntity, false, ConversationRepository.eventFromStreamMessage);
}
const isBackendTimestamp = eventSource !== EventSource.INJECTED;

const eventsToSkip: (CLIENT_CONVERSATION_EVENT | CONVERSATION_EVENT)[] = [
Expand All @@ -2175,7 +2151,7 @@ export class ConversationRepository {
)
.then((entityObject = {} as EntityObject) => {
if (type !== CONVERSATION_EVENT.MEMBER_JOIN && type !== CONVERSATION_EVENT.MEMBER_LEAVE) {
this.handleConversationNotification(entityObject as EntityObject, eventSource, previouslyArchived);
this.handleConversationNotification(entityObject as EntityObject, eventSource);
}
})
.catch((error: BaseError) => {
Expand Down Expand Up @@ -2410,14 +2386,9 @@ export class ConversationRepository {
*
* @param entityObject Object containing the conversation and the message that are targeted by the event
* @param eventSource Source of event
* @param previouslyArchived `true` if the previous state of the conversation was archived
* @returns Resolves when the conversation was updated
*/
private async handleConversationNotification(
entityObject: EntityObject,
eventSource: EventSource,
previouslyArchived: boolean,
) {
private async handleConversationNotification(entityObject: EntityObject, eventSource: EventSource) {
const {conversationEntity, messageEntity} = entityObject;

if (conversationEntity) {
Expand All @@ -2439,18 +2410,6 @@ export class ConversationRepository {
conversationEntity.cleared_timestamp(0);
}
}

// Check if event needs to be un-archived
if (previouslyArchived) {
// Add to check for un-archiving at the end of stream handling
if (eventFromStream) {
return this.conversationsWithNewEvents.set(conversationEntity.id, conversationEntity);
}

if (eventFromWebSocket && conversationEntity.shouldUnarchive()) {
return this.unarchiveConversation(conversationEntity, false, 'event from WebSocket');
}
}
}
}

Expand Down
157 changes: 0 additions & 157 deletions src/script/entity/Conversation.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,17 @@

import {ConnectionStatus} from '@wireapp/api-client/lib/connection/';
import {CONVERSATION_TYPE} from '@wireapp/api-client/lib/conversation/';
import {CONVERSATION_EVENT} from '@wireapp/api-client/lib/event/';

import {ClientEntity} from 'src/script/client/ClientEntity';
import {ConnectionMapper} from 'src/script/connection/ConnectionMapper';
import {ConversationMapper} from 'src/script/conversation/ConversationMapper';
import {NOTIFICATION_STATE} from 'src/script/conversation/NotificationSetting';
import 'src/script/localization/Localizer';
import {CALL_MESSAGE_TYPE} from 'src/script/message/CallMessageType';
import {MentionEntity} from 'src/script/message/MentionEntity';
import {StatusType} from 'src/script/message/StatusType';
import {createUuid} from 'Util/uuid';

import {Conversation} from './Conversation';
import {CallMessage} from './message/CallMessage';
import {ContentMessage} from './message/ContentMessage';
import {MemberMessage} from './message/MemberMessage';
import {Message} from './message/Message';
import {PingMessage} from './message/PingMessage';
import {Text} from './message/Text';
Expand Down Expand Up @@ -897,158 +892,6 @@ describe('Conversation', () => {
});
});

describe('shouldUnarchive', () => {
let timestamp: number = undefined;
let contentMessage: ContentMessage = undefined;
let mutedTimestampMessage: PingMessage = undefined;
let outdatedMessage: PingMessage = undefined;
let pingMessage: PingMessage = undefined;
let selfMentionMessage: ContentMessage = undefined;
const conversationEntity = new Conversation(createUuid());

const selfUserEntity = new User(createUuid(), null);
selfUserEntity.isMe = true;
selfUserEntity.inTeam(true);
conversationEntity.selfUser(selfUserEntity);

beforeEach(() => {
timestamp = Date.now();
conversationEntity.archivedTimestamp(timestamp);
conversationEntity.archivedState(true);

mutedTimestampMessage = new PingMessage();
mutedTimestampMessage.timestamp(timestamp);

outdatedMessage = new PingMessage();
outdatedMessage.timestamp(timestamp - 100);

contentMessage = new ContentMessage();
contentMessage.assets([new Text('id', 'Hello there')]);
contentMessage.timestamp(timestamp + 100);

pingMessage = new PingMessage();
pingMessage.timestamp(timestamp + 200);

selfMentionMessage = new ContentMessage();
const mentionEntity = new MentionEntity(0, 7, selfUserEntity.id, selfUserEntity.domain);
const textAsset = new Text('id', '@Gregor, Hello there');
textAsset.mentions.push(mentionEntity);
selfMentionMessage.assets([textAsset]);
selfMentionMessage.timestamp(timestamp + 300);
});

afterEach(() => conversationEntity.messages_unordered.removeAll());

it('returns false if conversation is not archived', () => {
conversationEntity.archivedState(false);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(outdatedMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(mutedTimestampMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(contentMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(pingMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(selfMentionMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
});

it('returns false if conversation is in no notification state', () => {
conversationEntity.mutedState(NOTIFICATION_STATE.NOTHING);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(outdatedMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(mutedTimestampMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(contentMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(pingMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(selfMentionMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
});

it('returns expected value if conversation is in only mentions notifications state', () => {
conversationEntity.mutedState(NOTIFICATION_STATE.MENTIONS_AND_REPLIES);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(outdatedMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(mutedTimestampMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(contentMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(pingMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(selfMentionMessage);

expect(conversationEntity.shouldUnarchive()).toBe(true);
});

it('returns expected value if conversation is in everything notifications state', () => {
conversationEntity.mutedState(NOTIFICATION_STATE.EVERYTHING);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(outdatedMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(mutedTimestampMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
conversationEntity.messages_unordered.push(contentMessage);

expect(conversationEntity.shouldUnarchive()).toBe(true);
conversationEntity.messages_unordered.removeAll();

const memberLeaveMessage = new MemberMessage();
memberLeaveMessage.type = CONVERSATION_EVENT.MEMBER_LEAVE;
memberLeaveMessage.timestamp(timestamp + 100);
conversationEntity.messages_unordered.push(memberLeaveMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);

const callMessage = new CallMessage(CALL_MESSAGE_TYPE.ACTIVATED);
callMessage.timestamp(timestamp + 200);
conversationEntity.messages_unordered.push(callMessage);

expect(conversationEntity.shouldUnarchive()).toBe(true);
conversationEntity.messages_unordered.removeAll();
conversationEntity.messages_unordered.push(memberLeaveMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
const memberJoinMessage = new MemberMessage();
memberJoinMessage.type = CONVERSATION_EVENT.MEMBER_JOIN;
memberJoinMessage.timestamp(timestamp + 200);
conversationEntity.messages_unordered.push(memberJoinMessage);

expect(conversationEntity.shouldUnarchive()).toBe(false);
const selfJoinMessage = new MemberMessage();
selfJoinMessage.type = CONVERSATION_EVENT.MEMBER_JOIN;
selfJoinMessage.userIds.push({domain: selfUserEntity.domain, id: selfUserEntity.id});
selfJoinMessage.timestamp(timestamp + 200);
conversationEntity.messages_unordered.push(selfJoinMessage);

expect(conversationEntity.shouldUnarchive()).toBe(true);
});
});

describe('_incrementTimeOnly', () => {
it('should update only to newer timestamps', () => {
//@ts-ignore
Expand Down
33 changes: 0 additions & 33 deletions src/script/entity/Conversation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ import {truncate} from 'Util/StringUtil';

import {CallMessage} from './message/CallMessage';
import type {ContentMessage} from './message/ContentMessage';
import type {MemberMessage} from './message/MemberMessage';
import type {Message} from './message/Message';
import {PingMessage} from './message/PingMessage';
import type {User} from './User';
Expand Down Expand Up @@ -827,38 +826,6 @@ export class Conversation {
this.messages_unordered.removeAll();
}

shouldUnarchive(): boolean {
if (!this.archivedState() || this.showNotificationsNothing()) {
return false;
}

const isNewerMessage = (messageEntity: Message) => messageEntity.timestamp() > this.archivedTimestamp();

const {allEvents, allMessages, selfMentions, selfReplies} = this.unreadState();
if (this.showNotificationsMentionsAndReplies()) {
const mentionsAndReplies = selfMentions.concat(selfReplies);
return mentionsAndReplies.some(isNewerMessage);
}

const hasNewMessage = allMessages.some(isNewerMessage);
if (hasNewMessage) {
return true;
}

return allEvents.some(messageEntity => {
if (!isNewerMessage(messageEntity)) {
return false;
}

const isCallActivation = messageEntity.isCall() && messageEntity.isActivation();
const isMemberJoin = messageEntity.isMember() && (messageEntity as MemberMessage).isMemberJoin();
const wasSelfUserAdded =
isMemberJoin && (messageEntity as MemberMessage).isUserAffected(this.selfUser().qualifiedId);

return isCallActivation || wasSelfUserAdded;
});
}

/**
* Checks for message duplicates.
*
Expand Down
4 changes: 2 additions & 2 deletions src/script/page/LeftSidebar/panels/Archive.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,6 @@ const Archive = ({
]);

const onClickConversation = async (conversation: Conversation) => {
await conversationRepository.unarchiveConversation(conversation, true, 'opened conversation from archive');
onClose();
amplify.publish(WebAppEvents.CONVERSATION.SHOW, conversation, {});
};

Expand All @@ -68,6 +66,7 @@ const Archive = ({

const {currentFocus, handleKeyDown, resetConversationFocus} = useConversationFocus(conversations);

const isActiveConversation = (conversation: Conversation) => conversationState.isActiveConversation(conversation);
return (
<ListWrapper id="archive" header={t('archiveHeader')} onClose={onClose}>
<h2 className="visually-hidden">{t('archiveHeader')}</h2>
Expand All @@ -85,6 +84,7 @@ const Archive = ({
conversation={conversation}
onJoinCall={answerCall}
showJoinButton={false}
isSelected={isActiveConversation}
/>
))}
</ul>
Expand Down
4 changes: 0 additions & 4 deletions src/script/view_model/ActionsViewModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -352,10 +352,6 @@ export class ActionsViewModel {
};

private readonly openConversation = async (conversationEntity: Conversation): Promise<void> => {
if (conversationEntity.is_archived()) {
await this.conversationRepository.unarchiveConversation(conversationEntity, true);
}

if (conversationEntity.is_cleared()) {
conversationEntity.cleared_timestamp(0);
}
Expand Down
Loading

0 comments on commit dc9fef4

Please sign in to comment.