From dd16be13b082862ddc5caace105675efc7b86130 Mon Sep 17 00:00:00 2001 From: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com> Date: Thu, 23 Mar 2023 17:52:46 -0700 Subject: [PATCH] Handle PNI+PNI+E164 triple --- ts/ConversationController.ts | 35 ++- .../ConversationController_test.ts | 22 ++ ts/test-mock/pnp/merge_test.ts | 223 ++++++++++-------- 3 files changed, 167 insertions(+), 113 deletions(-) diff --git a/ts/ConversationController.ts b/ts/ConversationController.ts index d99ae22c1c7c..ebc7239c1bd8 100644 --- a/ts/ConversationController.ts +++ b/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> = []; @@ -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, @@ -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)); } diff --git a/ts/test-electron/ConversationController_test.ts b/ts/test-electron/ConversationController_test.ts index f6c9f59074ee..b1dcdc82e84c 100644 --- a/ts/test-electron/ConversationController_test.ts +++ b/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, diff --git a/ts/test-mock/pnp/merge_test.ts b/ts/test-mock/pnp/merge_test.ts index f1a78f3be87f..672ef0428959 100644 --- a/ts/test-mock/pnp/merge_test.ts +++ b/ts/test-mock/pnp/merge_test.ts @@ -101,123 +101,140 @@ describe('pnp/merge', function needsName() { }); for (const finalContact of [UUIDKind.ACI, UUIDKind.PNI]) { - // eslint-disable-next-line no-loop-func - it(`happens via storage service, with notification (${finalContact})`, async () => { - const { phone } = bootstrap; + for (const withNotification of [false, true]) { + const testName = + 'happens via storage service, ' + + `${withNotification ? 'with' : 'without'} notification ` + + `(${finalContact})`; - const window = await app.getWindow(); - const leftPane = window.locator('.left-pane-wrapper'); + // eslint-disable-next-line no-loop-func + it(testName, async () => { + const { phone } = bootstrap; - debug('opening conversation with the aci contact'); - await leftPane - .locator(`[data-testid="${pniContact.device.uuid}"]`) - .click(); + const window = await app.getWindow(); + const leftPane = window.locator('.left-pane-wrapper'); - await window.locator('.module-conversation-hero').waitFor(); - - debug('Send message to ACI'); - { - const composeArea = window.locator( - '.composition-area-wrapper, .conversation .ConversationView' - ); - const compositionInput = composeArea.locator( - '[data-testid=CompositionInput]' - ); - - await compositionInput.type('Hello ACI'); - await compositionInput.press('Enter'); - } - - debug('opening conversation with the pni contact'); - await leftPane - .locator('.module-conversation-list__item--contact-or-conversation') - .first() - .click(); - - await window.locator('.module-conversation-hero').waitFor(); - - debug('Verify starting state'); - { - // No messages - const messages = window.locator('.module-message__text'); - assert.strictEqual(await messages.count(), 0, 'message count'); - - // No notifications - const notifications = window.locator('.SystemMessage'); - assert.strictEqual( - await notifications.count(), - 0, - 'notification count' - ); - } - - debug('Send message to PNI'); - { - const composeArea = window.locator( - '.composition-area-wrapper, .conversation .ConversationView' - ); - const compositionInput = composeArea.locator( - '[data-testid=CompositionInput]' - ); - - await compositionInput.type('Hello PNI'); - await compositionInput.press('Enter'); - } - - if (finalContact === UUIDKind.ACI) { - debug('switching back to ACI conversation'); + debug('opening conversation with the aci contact'); await leftPane .locator(`[data-testid="${pniContact.device.uuid}"]`) .click(); await window.locator('.module-conversation-hero').waitFor(); - } - debug( - 'removing both contacts from storage service, adding one combined contact' - ); - { - const state = await phone.expectStorageState('consistency check'); - await phone.setStorageState( - state.mergeContact(pniContact, { - identityState: Proto.ContactRecord.IdentityState.DEFAULT, - whitelisted: true, - identityKey: pniContact.publicKey.serialize(), - profileKey: pniContact.profileKey.serialize(), - }) + debug('Send message to ACI'); + { + const composeArea = window.locator( + '.composition-area-wrapper, .conversation .ConversationView' + ); + const compositionInput = composeArea.locator( + '[data-testid=CompositionInput]' + ); + + await compositionInput.type('Hello ACI'); + await compositionInput.press('Enter'); + } + + debug('opening conversation with the pni contact'); + await leftPane + .locator('.module-conversation-list__item--contact-or-conversation') + .first() + .click(); + + await window.locator('.module-conversation-hero').waitFor(); + + debug('Verify starting state'); + { + // No messages + const messages = window.locator('.module-message__text'); + assert.strictEqual(await messages.count(), 0, 'message count'); + + // No notifications + const notifications = window.locator('.SystemMessage'); + assert.strictEqual( + await notifications.count(), + 0, + 'notification count' + ); + } + + if (withNotification) { + debug('Send message to PNI'); + const composeArea = window.locator( + '.composition-area-wrapper, .conversation .ConversationView' + ); + const compositionInput = composeArea.locator( + '[data-testid=CompositionInput]' + ); + + await compositionInput.type('Hello PNI'); + await compositionInput.press('Enter'); + } + + if (finalContact === UUIDKind.ACI) { + debug('switching back to ACI conversation'); + await leftPane + .locator(`[data-testid="${pniContact.device.uuid}"]`) + .click(); + + await window.locator('.module-conversation-hero').waitFor(); + } + + debug( + 'removing both contacts from storage service, adding one combined contact' ); - await phone.sendFetchStorage({ - timestamp: bootstrap.getTimestamp(), - }); - } + { + const state = await phone.expectStorageState('consistency check'); + await phone.setStorageState( + state.mergeContact(pniContact, { + identityState: Proto.ContactRecord.IdentityState.DEFAULT, + whitelisted: true, + identityKey: pniContact.publicKey.serialize(), + profileKey: pniContact.profileKey.serialize(), + }) + ); + 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(); + if (withNotification) { + await window + .locator('.module-message__text >> "Hello PNI"') + .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(); + const messages = window.locator('.module-message__text'); + assert.strictEqual( + await messages.count(), + withNotification ? 2 : 1, + 'message count' + ); - const messages = window.locator('.module-message__text'); - assert.strictEqual(await messages.count(), 2, 'message count'); + // One notification - the merge + const notifications = window.locator('.SystemMessage'); + assert.strictEqual( + await notifications.count(), + withNotification ? 1 : 0, + 'notification count' + ); - // One notification - the merge - const notifications = window.locator('.SystemMessage'); - assert.strictEqual( - await notifications.count(), - 1, - 'notification count' - ); - - const first = await notifications.first(); - assert.match( - await first.innerText(), - /Your message history with ACI Contact and their number .* has been merged./ - ); - } - }); + 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 () => {