Handle PNI+PNI+E164 triple

ts/ConversationController.ts

@@ -470,17 +470,20 @@ export class ConversationController {
   } {
     const dataProvided = [];
     if (providedAci) {
-      dataProvided.push('aci');
+      dataProvided.push(`aci=${providedAci}`);
     }
     if (e164) {
-      dataProvided.push('e164');
+      dataProvided.push(`e164=${e164}`);
     }
     if (providedPni) {
-      dataProvided.push('pni');
+      dataProvided.push(`pni=${providedPni}`);
     }
-    const logId = `maybeMergeContacts/${reason}/${dataProvided.join('+')}`;
+    const logId = `maybeMergeContacts/${reason}/${dataProvided.join(',')}`;
 
-    const aci = providedAci ? UUID.cast(providedAci) : undefined;
+    const aci =
+      providedAci && providedAci !== providedPni
+        ? UUID.cast(providedAci)
+        : undefined;
     const pni = providedPni ? UUID.cast(providedPni) : undefined;
     const mergePromises: Array<Promise<void>> = [];
 
@@ -517,7 +520,8 @@ export class ConversationController {
       if (!match) {
         if (targetConversation) {
           log.info(
-            `${logId}: No match for ${key}, applying to target conversation`
+            `${logId}: No match for ${key}, applying to target ` +
+              `conversation - ${targetConversation.idForLogging()}`
           );
           // Note: This line might erase a known e164 or PNI
           applyChangeToConversation(targetConversation, {
@@ -609,7 +613,8 @@ export class ConversationController {
         // Clear the value on the current match, since it belongs on targetConversation!
         //   Note: we need to do the remove first, because it will clear the lookup!
         log.info(
-          `${logId}: Clearing ${key} on match, and adding it to target conversation`
+          `${logId}: Clearing ${key} on match, and adding it to target ` +
+            `conversation - ${targetConversation.idForLogging()}`
         );
         const change: Pick<
           Partial<ConversationAttributesType>,
@@ -635,7 +640,7 @@ export class ConversationController {
         if (willMerge) {
           log.warn(
             `${logId}: Removing old conversation which matched on ${key}. ` +
-              'Merging with target conversation.'
+              `Merging with target conversation - ${targetConversation.idForLogging()}`
           );
           mergePromises.push(
             mergeOldAndNew({
@@ -649,7 +654,10 @@ export class ConversationController {
         }
       } else if (targetConversation && !targetConversation?.get(key)) {
         // This is mostly for the situation where PNI was erased when updating e164
-        log.debug(`${logId}: Re-adding ${key} on target conversation`);
+        log.debug(
+          `${logId}: Re-adding ${key} on target conversation - ` +
+            `${targetConversation.idForLogging()}`
+        );
         applyChangeToConversation(targetConversation, {
           [key]: value,
         });
@@ -1075,6 +1083,8 @@ export class ConversationController {
 
     // Note: we explicitly don't want to update V2 groups
 
+    const obsoleteHadMessages = (obsolete.get('messageCount') ?? 0) > 0;
+
     log.warn(`${logId}: Delete the obsolete conversation from the database`);
     await removeConversation(obsoleteId);
 
@@ -1112,7 +1122,12 @@ export class ConversationController {
     const titleIsUseful = Boolean(
       obsoleteTitleInfo && getTitleNoDefault(obsoleteTitleInfo)
     );
-    if (obsoleteTitleInfo && titleIsUseful && !fromPniSignature) {
+    if (
+      obsoleteTitleInfo &&
+      titleIsUseful &&
+      !fromPniSignature &&
+      obsoleteHadMessages
+    ) {
       drop(current.addConversationMerge(obsoleteTitleInfo));
     }
 

ts/test-electron/ConversationController_test.ts

@@ -571,6 +571,28 @@ describe('ConversationController', () => {
 
         assert.strictEqual(result?.id, initial?.id, 'result and initial match');
       });
+      it('adds PNI to conversation with e164+ACI', () => {
+        const initial = create('initial', {
+          aci: ACI_1,
+          e164: E164_1,
+        });
+
+        const { conversation: result } =
+          window.ConversationController.maybeMergeContacts({
+            mergeOldAndNew,
+            aci: PNI_1,
+            pni: PNI_1,
+            e164: E164_1,
+            reason,
+          });
+        expectPropsAndLookups(result, 'result', {
+          uuid: ACI_1,
+          e164: E164_1,
+          pni: PNI_1,
+        });
+
+        assert.strictEqual(result?.id, initial?.id, 'result and initial match');
+      });
       it('replaces PNI in conversation with all data', () => {
         const initial = create('initial', {
           aci: ACI_1,

ts/test-mock/pnp/merge_test.ts

@@ -101,8 +101,14 @@ describe('pnp/merge', function needsName() {
   });
 
   for (const finalContact of [UUIDKind.ACI, UUIDKind.PNI]) {
+    for (const withNotification of [false, true]) {
+      const testName =
+        'happens via storage service, ' +
+        `${withNotification ? 'with' : 'without'} notification ` +
+        `(${finalContact})`;
+
       // eslint-disable-next-line no-loop-func
-    it(`happens via storage service, with notification (${finalContact})`, async () => {
+      it(testName, async () => {
         const { phone } = bootstrap;
 
         const window = await app.getWindow();
@@ -151,8 +157,8 @@ describe('pnp/merge', function needsName() {
           );
         }
 
+        if (withNotification) {
           debug('Send message to PNI');
-      {
           const composeArea = window.locator(
             '.composition-area-wrapper, .conversation .ConversationView'
           );
@@ -189,36 +195,47 @@ describe('pnp/merge', function needsName() {
           await phone.sendFetchStorage({
             timestamp: bootstrap.getTimestamp(),
           });
+          await app.waitForManifestVersion(state.version);
         }
 
-      // wait for desktop to process these changes
-      await window.locator('.SystemMessage').waitFor();
-
         debug('Verify final state');
         {
           // Should have both PNI and ACI messages
-        await window.locator('.module-message__text >> "Hello ACI"').waitFor();
-        await window.locator('.module-message__text >> "Hello PNI"').waitFor();
+          await window
+            .locator('.module-message__text >> "Hello ACI"')
+            .waitFor();
+          if (withNotification) {
+            await window
+              .locator('.module-message__text >> "Hello PNI"')
+              .waitFor();
+          }
 
           const messages = window.locator('.module-message__text');
-        assert.strictEqual(await messages.count(), 2, 'message count');
+          assert.strictEqual(
+            await messages.count(),
+            withNotification ? 2 : 1,
+            'message count'
+          );
 
           // One notification - the merge
           const notifications = window.locator('.SystemMessage');
           assert.strictEqual(
             await notifications.count(),
-          1,
+            withNotification ? 1 : 0,
             'notification count'
           );
 
+          if (withNotification) {
             const first = await notifications.first();
             assert.match(
               await first.innerText(),
               /Your message history with ACI Contact and their number .* has been merged./
             );
           }
+        }
       });
     }
+  }
 
   it('accepts storage service contact splitting', async () => {
     const { phone } = bootstrap;