From ccee7577f7978b4d7c87ab114935bf5a87e4e9de Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Fri, 11 Nov 2022 12:14:13 -0500 Subject: [PATCH] Do not double-insert change number events. --- ...cipientDatabaseTest_getAndPossiblyMerge.kt | 45 ++------------- .../securesms/database/RecipientDatabase.kt | 5 -- .../securesms/jobs/JobManagerFactories.java | 2 +- .../jobs/RecipientChangedNumberJob.kt | 57 ------------------- 4 files changed, 5 insertions(+), 104 deletions(-) delete mode 100644 app/src/main/java/org/thoughtcrime/securesms/jobs/RecipientChangedNumberJob.kt diff --git a/app/src/androidTest/java/org/thoughtcrime/securesms/database/RecipientDatabaseTest_getAndPossiblyMerge.kt b/app/src/androidTest/java/org/thoughtcrime/securesms/database/RecipientDatabaseTest_getAndPossiblyMerge.kt index b3c11b92f..ff11cb9e4 100644 --- a/app/src/androidTest/java/org/thoughtcrime/securesms/database/RecipientDatabaseTest_getAndPossiblyMerge.kt +++ b/app/src/androidTest/java/org/thoughtcrime/securesms/database/RecipientDatabaseTest_getAndPossiblyMerge.kt @@ -12,7 +12,6 @@ import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.signal.core.util.CursorUtil -import org.signal.core.util.ThreadUtil import org.signal.libsignal.protocol.IdentityKey import org.signal.libsignal.protocol.SignalProtocolAddress import org.signal.libsignal.protocol.state.SessionRecord @@ -26,9 +25,7 @@ import org.thoughtcrime.securesms.database.model.Mention import org.thoughtcrime.securesms.database.model.MessageId import org.thoughtcrime.securesms.database.model.MessageRecord import org.thoughtcrime.securesms.database.model.ReactionRecord -import org.thoughtcrime.securesms.dependencies.ApplicationDependencies import org.thoughtcrime.securesms.groups.GroupId -import org.thoughtcrime.securesms.jobs.RecipientChangedNumberJob import org.thoughtcrime.securesms.keyvalue.AccountValues import org.thoughtcrime.securesms.keyvalue.KeyValueDataSet import org.thoughtcrime.securesms.keyvalue.KeyValueStore @@ -229,9 +226,6 @@ class RecipientDatabaseTest_getAndPossiblyMerge { /** Merge two different users into one. You should prefer the ACI user. Not shown: merging threads, dropping e164 sessions, etc. */ @Test fun getAndPossiblyMerge_bothAciAndE164MapToExistingUser_aciAndE164_merge() { - val changeNumberListener = ChangeNumberListener() - changeNumberListener.enqueue() - val existingAciId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, null) val existingE164Id: RecipientId = recipientDatabase.getAndPossiblyMerge(null, E164_A) @@ -245,16 +239,12 @@ class RecipientDatabaseTest_getAndPossiblyMerge { val existingE164Recipient = Recipient.resolved(existingE164Id) assertEquals(mergedId, existingE164Recipient.id) - changeNumberListener.waitForJobManager() - assertFalse(changeNumberListener.numberChangeWasEnqueued) + // TODO [greyson] Change number } /** Same as [getAndPossiblyMerge_bothAciAndE164MapToExistingUser_aciAndE164_merge], but with a number change. */ @Test fun getAndPossiblyMerge_bothAciAndE164MapToExistingUser_aciAndE164_merge_changedNumber() { - val changeNumberListener = ChangeNumberListener() - changeNumberListener.enqueue() - val existingAciId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, E164_B) val existingE164Id: RecipientId = recipientDatabase.getAndPossiblyMerge(null, E164_A) @@ -268,16 +258,12 @@ class RecipientDatabaseTest_getAndPossiblyMerge { val existingE164Recipient = Recipient.resolved(existingE164Id) assertEquals(retrievedId, existingE164Recipient.id) - changeNumberListener.waitForJobManager() - assert(changeNumberListener.numberChangeWasEnqueued) + // TODO [greyson] Change number } /** No new rules here, just a more complex scenario to show how different rules interact. */ @Test fun getAndPossiblyMerge_bothAciAndE164MapToExistingUser_aciAndE164_complex() { - val changeNumberListener = ChangeNumberListener() - changeNumberListener.enqueue() - val existingId1: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, E164_B) val existingId2: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_B, E164_A) @@ -292,8 +278,7 @@ class RecipientDatabaseTest_getAndPossiblyMerge { assertEquals(ACI_B, existingRecipient2.requireServiceId()) assertFalse(existingRecipient2.hasE164()) - changeNumberListener.waitForJobManager() - assert(changeNumberListener.numberChangeWasEnqueued) + // TODO [greyson] Change number } /** @@ -383,9 +368,6 @@ class RecipientDatabaseTest_getAndPossiblyMerge { /** Verifying a case where a change number job is expected to be enqueued. */ @Test fun getAndPossiblyMerge_aciMapsToExistingUserButE164DoesNot_changedNumber() { - val changeNumberListener = ChangeNumberListener() - changeNumberListener.enqueue() - val existingId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, E164_A) val retrievedId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, E164_B) @@ -395,8 +377,7 @@ class RecipientDatabaseTest_getAndPossiblyMerge { assertEquals(ACI_A, retrievedRecipient.requireServiceId()) assertEquals(E164_B, retrievedRecipient.requireE164()) - changeNumberListener.waitForJobManager() - assert(changeNumberListener.numberChangeWasEnqueued) + // TODO [greyson] Change number } /** High trust lets you merge two different users into one. You should prefer the ACI user. Not shown: merging threads, dropping e164 sessions, etc. */ @@ -630,24 +611,6 @@ class RecipientDatabaseTest_getAndPossiblyMerge { val threadId: Long ) - private class ChangeNumberListener { - - var numberChangeWasEnqueued = false - private set - - fun waitForJobManager() { - ApplicationDependencies.getJobManager().flush() - ThreadUtil.sleep(500) - } - - fun enqueue() { - ApplicationDependencies.getJobManager().addListener( - { job -> job.factoryKey == RecipientChangedNumberJob.KEY }, - { _, _ -> numberChangeWasEnqueued = true } - ) - } - } - companion object { val ACI_A = ACI.from(UUID.fromString("3436efbe-5a76-47fa-a98a-7e72c948a82e")) val ACI_B = ACI.from(UUID.fromString("8de7f691-0b60-4a68-9cd9-ed2f8453f9ed")) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.kt b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.kt index e6ae665ed..0df1aa16b 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.kt @@ -75,7 +75,6 @@ import org.thoughtcrime.securesms.groups.GroupId.V1 import org.thoughtcrime.securesms.groups.GroupId.V2 import org.thoughtcrime.securesms.groups.v2.ProfileKeySet import org.thoughtcrime.securesms.groups.v2.processing.GroupsV2StateProcessor -import org.thoughtcrime.securesms.jobs.RecipientChangedNumberJob import org.thoughtcrime.securesms.jobs.RequestGroupV2InfoJob import org.thoughtcrime.securesms.jobs.RetrieveProfileJob import org.thoughtcrime.securesms.keyvalue.SignalStore @@ -490,10 +489,6 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) : StorageSyncHelper.scheduleSyncForDataChange() RecipientId.clearCache() } - - if (result.changedNumberId != null) { - ApplicationDependencies.getJobManager().add(RecipientChangedNumberJob(result.changedNumberId!!)) - } } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java index 8691d137c..8adf9731a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java @@ -157,7 +157,6 @@ public final class JobManagerFactories { put(PushProcessMessageJob.KEY, new PushProcessMessageJob.Factory()); put(PushTextSendJob.KEY, new PushTextSendJob.Factory()); put(ReactionSendJob.KEY, new ReactionSendJob.Factory()); - put(RecipientChangedNumberJob.KEY, new RecipientChangedNumberJob.Factory()); put(RefreshAttributesJob.KEY, new RefreshAttributesJob.Factory()); put(RefreshOwnProfileJob.KEY, new RefreshOwnProfileJob.Factory()); put(RemoteConfigRefreshJob.KEY, new RemoteConfigRefreshJob.Factory()); @@ -254,6 +253,7 @@ public final class JobManagerFactories { put("RotateSignedPreKeyJob", new PreKeysSyncJob.Factory()); put("CreateSignedPreKeyJob", new PreKeysSyncJob.Factory()); put("RefreshPreKeysJob", new PreKeysSyncJob.Factory()); + put("RecipientChangedNumberJob", new FailingJob.Factory()); }}; } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/RecipientChangedNumberJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/RecipientChangedNumberJob.kt deleted file mode 100644 index 84c062725..000000000 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/RecipientChangedNumberJob.kt +++ /dev/null @@ -1,57 +0,0 @@ -package org.thoughtcrime.securesms.jobs - -import org.signal.core.util.logging.Log -import org.thoughtcrime.securesms.database.SignalDatabase -import org.thoughtcrime.securesms.jobmanager.Data -import org.thoughtcrime.securesms.jobmanager.Job -import org.thoughtcrime.securesms.recipients.Recipient -import org.thoughtcrime.securesms.recipients.RecipientId - -/** - * Insert change number update items in all threads (1:1 and group) with [recipientId]. - */ -class RecipientChangedNumberJob(parameters: Parameters, private val recipientId: RecipientId) : BaseJob(parameters) { - - constructor(recipientId: RecipientId) : this( - Parameters.Builder().setQueue("RecipientChangedNumberJob_${recipientId.toQueueKey()}").build(), - recipientId - ) - - override fun serialize(): Data { - return Data.Builder() - .putString(KEY_RECIPIENT_ID, recipientId.serialize()) - .build() - } - - override fun getFactoryKey(): String { - return KEY - } - - override fun onRun() { - val recipient: Recipient = Recipient.resolved(recipientId) - - if (!recipient.isBlocked && !recipient.isGroup && !recipient.isSelf) { - Log.i(TAG, "Writing a number change event.") - SignalDatabase.sms.insertNumberChangeMessages(recipient.id) - } else { - Log.i(TAG, "Number changed but not relevant. blocked: ${recipient.isBlocked} isGroup: ${recipient.isGroup} isSelf: ${recipient.isSelf}") - } - } - - override fun onShouldRetry(e: Exception): Boolean = false - - override fun onFailure() = Unit - - class Factory : Job.Factory { - override fun create(parameters: Parameters, data: Data): RecipientChangedNumberJob { - return RecipientChangedNumberJob(parameters, RecipientId.from(data.getString(KEY_RECIPIENT_ID))) - } - } - - companion object { - const val KEY = "RecipientChangedNumberJob" - - private val TAG = Log.tag(RecipientChangedNumberJob::class.java) - private const val KEY_RECIPIENT_ID = "recipient_id" - } -}