From fdc34761067ea3d0112f2036e73fd9d9409bf629 Mon Sep 17 00:00:00 2001 From: Chris Svenningsen Date: Sat, 10 Oct 2020 07:25:17 -0700 Subject: [PATCH] Fix a few pinned chat sync issues --- ts/ConversationController.ts | 33 +++++ ts/model-types.d.ts | 1 - ts/models/conversations.ts | 17 +-- ts/services/storageRecordOps.ts | 42 +++++- ts/state/ducks/conversations.ts | 1 - ts/state/selectors/conversations.ts | 7 +- ts/test/state/selectors/conversations_test.ts | 120 ++++++++++++++++++ ts/views/conversation_view.ts | 4 +- 8 files changed, 205 insertions(+), 20 deletions(-) diff --git a/ts/ConversationController.ts b/ts/ConversationController.ts index 2c2e433cb7..70c3ddef29 100644 --- a/ts/ConversationController.ts +++ b/ts/ConversationController.ts @@ -732,4 +732,37 @@ export class ConversationController { return this._initialPromise; } + + getPinnedConversationIds(): Array { + let pinnedConversationIds = window.storage.get>( + 'pinnedConversationIds' + ); + + // If pinnedConversationIds is missing, we're upgrading from + // a previous version and need to backfill storage from pinned + // conversation models. + if (pinnedConversationIds === undefined) { + window.log.info( + 'getPinnedConversationIds: no pinned conversations in storage' + ); + + const modelPinnedConversationIds = this._conversations + .filter(conversation => conversation.get('isPinned')) + // pinIndex is a deprecated field. We now rely on the order of + // the ids in storage, which is synced with the AccountRecord. + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + .sort((a, b) => (a.get('pinIndex') || 0) - (b.get('pinIndex') || 0)) + .map(conversation => conversation.get('id')); + + window.log.info( + `getPinnedConversationIds: falling back to ${modelPinnedConversationIds.length} pinned models` + ); + + window.storage.put('pinnedConversationIds', modelPinnedConversationIds); + pinnedConversationIds = modelPinnedConversationIds; + } + + return pinnedConversationIds; + } } diff --git a/ts/model-types.d.ts b/ts/model-types.d.ts index 78cb02615e..5ab5a0afd2 100644 --- a/ts/model-types.d.ts +++ b/ts/model-types.d.ts @@ -153,7 +153,6 @@ export type ConversationAttributesType = { messageCountBeforeMessageRequests: number; messageRequestResponseType: number; muteExpiresAt: number; - pinIndex?: number; profileAvatar: WhatIsThis; profileKeyCredential: string | null; profileKeyVersion: string | null; diff --git a/ts/models/conversations.ts b/ts/models/conversations.ts index 77f24a2a9d..e4d8e4214b 100644 --- a/ts/models/conversations.ts +++ b/ts/models/conversations.ts @@ -1098,7 +1098,6 @@ export class ConversationModel extends window.Backbone.Model< muteExpiresAt: this.get('muteExpiresAt')!, name: this.get('name')!, phoneNumber: this.getNumber()!, - pinIndex: this.get('pinIndex'), profileName: this.getProfileName()!, sharedGroupNames: this.get('sharedGroupNames')!, shouldShowDraft, @@ -4073,17 +4072,16 @@ export class ConversationModel extends window.Backbone.Model< window.storage.get>('pinnedConversationIds', []) ); + pinnedConversationIds.add(this.id); + + this.writePinnedConversations([...pinnedConversationIds]); + this.set('isPinned', true); - this.set('pinIndex', pinnedConversationIds.size); window.Signal.Data.updateConversation(this.attributes); if (this.get('isArchived')) { this.setArchived(false); } - - pinnedConversationIds.add(this.id); - - this.writePinnedConversations([...pinnedConversationIds]); } unpin(): void { @@ -4093,13 +4091,12 @@ export class ConversationModel extends window.Backbone.Model< window.storage.get>('pinnedConversationIds', []) ); - this.set('isPinned', false); - this.set('pinIndex', undefined); - window.Signal.Data.updateConversation(this.attributes); - pinnedConversationIds.delete(this.id); this.writePinnedConversations([...pinnedConversationIds]); + + this.set('isPinned', false); + window.Signal.Data.updateConversation(this.attributes); } writePinnedConversations(pinnedConversationIds: Array): void { diff --git a/ts/services/storageRecordOps.ts b/ts/services/storageRecordOps.ts index e088ebe8b4..6489d2f36e 100644 --- a/ts/services/storageRecordOps.ts +++ b/ts/services/storageRecordOps.ts @@ -147,7 +147,7 @@ export async function toAccountRecord( window.storage.get('typingIndicators') ); accountRecord.linkPreviews = Boolean(window.storage.get('linkPreviews')); - accountRecord.pinnedConversations = window.storage + const pinnedConversations = window.storage .get>('pinnedConversationIds', []) .map(id => { const pinnedConversation = window.ConversationController.get(id); @@ -197,6 +197,11 @@ export async function toAccountRecord( pinnedConversationClass !== undefined ); + window.log.info( + `toAccountRecord: sending ${pinnedConversations.length} pinned conversations` + ); + + accountRecord.pinnedConversations = pinnedConversations; applyUnknownFields(accountRecord, conversation); return accountRecord; @@ -580,8 +585,33 @@ export async function mergeAccountRecord( } if (remotelyPinnedConversationClasses) { - const locallyPinnedConversations = window.ConversationController._conversations.filter( - conversation => Boolean(conversation.get('isPinned')) + const modelPinnedConversations = window + .getConversations() + .filter(conversation => Boolean(conversation.get('isPinned'))); + + const modelPinnedConversationIds = modelPinnedConversations.map( + conversation => conversation.get('id') + ); + + const missingStoragePinnedConversationIds = window.ConversationController.getPinnedConversationIds().filter( + id => !modelPinnedConversationIds.includes(id) + ); + + if (missingStoragePinnedConversationIds.length !== 0) { + window.log.info( + 'mergeAccountRecord: pinnedConversationIds in storage does not match pinned Conversation models' + ); + } + + const locallyPinnedConversations = modelPinnedConversations.concat( + missingStoragePinnedConversationIds + .map(conversationId => + window.ConversationController.get(conversationId) + ) + .filter( + (conversation): conversation is ConversationModel => + conversation !== undefined + ) ); window.log.info( @@ -677,12 +707,12 @@ export async function mergeAccountRecord( ); conversationsToUnpin.forEach(conversation => { - conversation.set({ isPinned: false, pinIndex: undefined }); + conversation.set({ isPinned: false }); updateConversation(conversation.attributes); }); - remotelyPinnedConversations.forEach((conversation, index) => { - conversation.set({ isPinned: true, pinIndex: index }); + remotelyPinnedConversations.forEach(conversation => { + conversation.set({ isPinned: true }); updateConversation(conversation.attributes); }); diff --git a/ts/state/ducks/conversations.ts b/ts/state/ducks/conversations.ts index e76166a6fa..169dd85846 100644 --- a/ts/state/ducks/conversations.ts +++ b/ts/state/ducks/conversations.ts @@ -55,7 +55,6 @@ export type ConversationType = { text: string; }; phoneNumber?: string; - pinIndex?: number; membersCount?: number; muteExpiresAt?: number; type: ConversationTypeType; diff --git a/ts/state/selectors/conversations.ts b/ts/state/selectors/conversations.ts index 5c8c0261b6..4e3c06ba88 100644 --- a/ts/state/selectors/conversations.ts +++ b/ts/state/selectors/conversations.ts @@ -158,7 +158,12 @@ export const _getLeftPaneLists = ( conversations.sort(comparator); archivedConversations.sort(comparator); - pinnedConversations.sort((a, b) => (a.pinIndex || 0) - (b.pinIndex || 0)); + + const pinnedConversationIds = window.ConversationController.getPinnedConversationIds(); + pinnedConversations.sort( + (a, b) => + pinnedConversationIds.indexOf(a.id) - pinnedConversationIds.indexOf(b.id) + ); return { conversations, archivedConversations, pinnedConversations }; }; diff --git a/ts/test/state/selectors/conversations_test.ts b/ts/test/state/selectors/conversations_test.ts index ed8006e7c5..defae1920c 100644 --- a/ts/test/state/selectors/conversations_test.ts +++ b/ts/test/state/selectors/conversations_test.ts @@ -1,4 +1,5 @@ import { assert } from 'chai'; +import * as sinon from 'sinon'; import { ConversationLookupType } from '../../../state/ducks/conversations'; import { @@ -7,6 +8,27 @@ import { } from '../../../state/selectors/conversations'; describe('state/selectors/conversations', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const globalAsAny = global as any; + + beforeEach(function beforeEach() { + this.oldWindow = globalAsAny.window; + globalAsAny.window = {}; + + window.ConversationController = { + getPinnedConversationIds: sinon.stub().returns([]), + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any; + }); + + afterEach(function afterEach() { + if (this.oldWindow === undefined) { + delete globalAsAny.window; + } else { + globalAsAny.window = this.oldWindow; + } + }); + describe('#getLeftPaneList', () => { it('sorts conversations based on timestamp then by intl-friendly title', () => { const data: ConversationLookupType = { @@ -140,5 +162,103 @@ describe('state/selectors/conversations', () => { assert.strictEqual(conversations[3].name, 'C'); assert.strictEqual(conversations[4].name, 'No timestamp'); }); + + describe('given pinned conversations', () => { + beforeEach(() => { + (window.ConversationController + .getPinnedConversationIds as sinon.SinonStub).returns([ + 'pin1', + 'pin2', + 'pin3', + ]); + }); + + it('sorts pinned conversations based on order in storage', () => { + const data: ConversationLookupType = { + pin2: { + id: 'pin2', + e164: '+18005551111', + activeAt: Date.now(), + name: 'Pin Two', + timestamp: 30, + inboxPosition: 30, + phoneNumber: 'notused', + isArchived: false, + isPinned: true, + + type: 'direct', + isMe: false, + lastUpdated: Date.now(), + title: 'Pin Two', + unreadCount: 1, + isSelected: false, + typingContact: { + name: 'Someone There', + color: 'blue', + phoneNumber: '+18005551111', + }, + + acceptedMessageRequest: true, + }, + pin3: { + id: 'pin3', + e164: '+18005551111', + activeAt: Date.now(), + name: 'Pin Three', + timestamp: 30, + inboxPosition: 30, + phoneNumber: 'notused', + isArchived: false, + isPinned: true, + + type: 'direct', + isMe: false, + lastUpdated: Date.now(), + title: 'Pin Three', + unreadCount: 1, + isSelected: false, + typingContact: { + name: 'Someone There', + color: 'blue', + phoneNumber: '+18005551111', + }, + + acceptedMessageRequest: true, + }, + pin1: { + id: 'pin1', + e164: '+18005551111', + activeAt: Date.now(), + name: 'Pin One', + timestamp: 30, + inboxPosition: 30, + phoneNumber: 'notused', + isArchived: false, + isPinned: true, + + type: 'direct', + isMe: false, + lastUpdated: Date.now(), + title: 'Pin One', + unreadCount: 1, + isSelected: false, + typingContact: { + name: 'Someone There', + color: 'blue', + phoneNumber: '+18005551111', + }, + + acceptedMessageRequest: true, + }, + }; + + const comparator = _getConversationComparator(); + const { pinnedConversations } = _getLeftPaneLists(data, comparator); + + assert.strictEqual(pinnedConversations[0].name, 'Pin One'); + assert.strictEqual(pinnedConversations[1].name, 'Pin Two'); + assert.strictEqual(pinnedConversations[2].name, 'Pin Three'); + }); + }); }); }); diff --git a/ts/views/conversation_view.ts b/ts/views/conversation_view.ts index ed4f71b3b6..c55496c978 100644 --- a/ts/views/conversation_view.ts +++ b/ts/views/conversation_view.ts @@ -416,7 +416,9 @@ Whisper.ConversationView = Whisper.View.extend({ setPin(value: boolean) { if (value) { - if (window.storage.get('pinnedConversationIds', []).length >= 4) { + const pinnedConversationIds = window.ConversationController.getPinnedConversationIds(); + + if (pinnedConversationIds.length >= 4) { this.showToast(Whisper.PinnedConversationsFullToast); return; }