Fix a few pinned chat sync issues

ts/ConversationController.ts

@@ -732,4 +732,37 @@ export class ConversationController {
 
     return this._initialPromise;
   }
+
+  getPinnedConversationIds(): Array<string> {
+    let pinnedConversationIds = window.storage.get<Array<string>>(
+      '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;
+  }
 }

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;

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<Array<string>>('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<Array<string>>('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<string>): void {

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<Array<string>>('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(
+    const modelPinnedConversations = window
-      conversation => Boolean(conversation.get('isPinned'))
+      .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) => {
+    remotelyPinnedConversations.forEach(conversation => {
-      conversation.set({ isPinned: true, pinIndex: index });
+      conversation.set({ isPinned: true });
       updateConversation(conversation.attributes);
     });
 

ts/state/ducks/conversations.ts

@@ -55,7 +55,6 @@ export type ConversationType = {
     text: string;
   };
   phoneNumber?: string;
-  pinIndex?: number;
   membersCount?: number;
   muteExpiresAt?: number;
   type: ConversationTypeType;

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 };
 };

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');
+      });
+    });
   });
 });

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;
       }