diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/MegaphoneDatabase.kt b/app/src/main/java/org/thoughtcrime/securesms/database/MegaphoneDatabase.kt index 372dfccded..ea8a27855e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/MegaphoneDatabase.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/MegaphoneDatabase.kt @@ -1,13 +1,14 @@ package org.thoughtcrime.securesms.database import android.app.Application +import androidx.annotation.VisibleForTesting +import androidx.sqlite.db.SupportSQLiteDatabase import net.zetetic.database.sqlcipher.SQLiteDatabase import net.zetetic.database.sqlcipher.SQLiteOpenHelper import org.signal.core.util.delete import org.signal.core.util.forEach import org.signal.core.util.insertInto import org.signal.core.util.logging.Log -import org.signal.core.util.logging.Log.tag import org.signal.core.util.requireBoolean import org.signal.core.util.requireInt import org.signal.core.util.requireLong @@ -25,7 +26,7 @@ import kotlin.concurrent.Volatile /** * IMPORTANT: Writes should only be made through [org.thoughtcrime.securesms.megaphone.MegaphoneRepository]. */ -class MegaphoneDatabase( +open class MegaphoneDatabase( application: Application, databaseSecret: DatabaseSecret ) : SQLiteOpenHelper( @@ -42,17 +43,45 @@ class MegaphoneDatabase( SignalDatabaseOpenHelper { companion object { - private val TAG = tag(MegaphoneDatabase::class.java) + private val TAG = Log.tag(MegaphoneDatabase::class.java) - private const val DATABASE_VERSION = 1 + private const val DATABASE_VERSION = 2 private const val DATABASE_NAME = "signal-megaphone.db" private const val TABLE_NAME = "megaphone" private const val ID = "_id" + + /** + * The event name, which is a key we use to tie it to views and whatnot. + */ private const val EVENT = "event" - private const val INTERACTION_COUNT = "seen_count" - private const val LAST_INTERACTION_TIMESTAMP = "last_seen" + + /** + * How many times a megaphone was interacted with. This is most commonly the "snooze" count. + */ + private const val INTERACTION_COUNT = "interaction_count" + + /** + * The last time a megaphone was interacted with. This is most commonly the "snooze" timestamp. + */ + private const val LAST_INTERACTION_TIMESTAMP = "last_interaction_timestamp" + + /** + * The timestamp of when the megaphone was first shown to the user. + */ private const val FIRST_VISIBLE = "first_visible" + + /** + * The timestamp of then when the last "view cycle" started. For instance, if a megaphone was + * snoozed and then shown again, this will be the timestamp of when it was first shown again. + * It is *not* updated every time a megaphone is seen, just at the start of the view cycle. + * This is largely used to determine when to auto-snooze a megaphone. + */ + private const val LAST_VISIBLE = "last_visible" + + /** + * Whether a megaphone has been fully completed. When it's finished, it'll never be shown again. + */ private const val FINISHED = "finished" const val CREATE_TABLE: String = """CREATE TABLE $TABLE_NAME( @@ -61,6 +90,7 @@ class MegaphoneDatabase( $INTERACTION_COUNT INTEGER, $LAST_INTERACTION_TIMESTAMP INTEGER, $FIRST_VISIBLE INTEGER, + $LAST_VISIBLE INTEGER DEFAULT 0, $FINISHED INTEGER )""" @@ -81,6 +111,10 @@ class MegaphoneDatabase( } } + @get:VisibleForTesting + internal open val database: SupportSQLiteDatabase + get() = writableDatabase + override fun onCreate(db: SQLiteDatabase) { Log.i(TAG, "onCreate()") db.execSQL(CREATE_TABLE) @@ -88,6 +122,12 @@ class MegaphoneDatabase( override fun onUpgrade(db: SQLiteDatabase?, oldVersion: Int, newVersion: Int) { Log.i(TAG, "onUpgrade($oldVersion, $newVersion)") + + if (oldVersion < 2) { + db!!.execSQL("ALTER TABLE $TABLE_NAME ADD COLUMN $LAST_VISIBLE INTEGER DEFAULT 0") + db.execSQL("ALTER TABLE $TABLE_NAME RENAME COLUMN seen_count TO interaction_count") + db.execSQL("ALTER TABLE $TABLE_NAME RENAME COLUMN last_seen TO last_interaction_timestamp") + } } override fun onOpen(db: SQLiteDatabase) { @@ -96,7 +136,7 @@ class MegaphoneDatabase( } fun insert(events: Collection) { - writableDatabase.withinTransaction { db -> + database.withinTransaction { db -> for (event in events) { db.insertInto(TABLE_NAME) .values(EVENT to event.key) @@ -108,7 +148,7 @@ class MegaphoneDatabase( fun getAllAndDeleteMissing(): MutableList { val records: MutableList = mutableListOf() - writableDatabase.withinTransaction { db -> + database.withinTransaction { db -> val missingKeys: MutableSet = mutableSetOf() db.select() @@ -119,6 +159,7 @@ class MegaphoneDatabase( val interactionCount = cursor.requireInt(INTERACTION_COUNT) val lastInteractionTime = cursor.requireLong(LAST_INTERACTION_TIMESTAMP) val firstVisible = cursor.requireLong(FIRST_VISIBLE) + val lastVisible = cursor.requireLong(LAST_VISIBLE) val finished = cursor.requireBoolean(FINISHED) if (Megaphones.Event.hasKey(event)) { @@ -127,6 +168,7 @@ class MegaphoneDatabase( interactionCount = interactionCount, lastInteractionTime = lastInteractionTime, firstVisible = firstVisible, + lastVisible = lastVisible, finished = finished ) } else { @@ -146,26 +188,35 @@ class MegaphoneDatabase( } fun markFirstVisible(event: Megaphones.Event, time: Long) { - writableDatabase + database .update(TABLE_NAME) .values(FIRST_VISIBLE to time) .where("$EVENT = ?", event.key) .run() } + fun markLastVisible(event: Megaphones.Event, time: Long) { + database + .update(TABLE_NAME) + .values(LAST_VISIBLE to time) + .where("$EVENT = ?", event.key) + .run() + } + fun markInteractedWith(event: Megaphones.Event, interactionCount: Int, lastInteractionTimestamp: Long) { - writableDatabase + database .update(TABLE_NAME) .values( INTERACTION_COUNT to interactionCount, - LAST_INTERACTION_TIMESTAMP to lastInteractionTimestamp + LAST_INTERACTION_TIMESTAMP to lastInteractionTimestamp, + LAST_VISIBLE to 0L ) .where("$EVENT = ?", event.key) .run() } fun markFinished(event: Megaphones.Event) { - writableDatabase + database .update(TABLE_NAME) .values(FINISHED to 1) .where("$EVENT = ?", event.key) @@ -173,7 +224,7 @@ class MegaphoneDatabase( } fun delete(event: Megaphones.Event) { - writableDatabase + database .delete(TABLE_NAME) .where("$EVENT = ?", event.key) .run() diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/model/MegaphoneRecord.kt b/app/src/main/java/org/thoughtcrime/securesms/database/model/MegaphoneRecord.kt index 0ad31ca7e3..3cdcd2dba1 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/model/MegaphoneRecord.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/model/MegaphoneRecord.kt @@ -7,5 +7,6 @@ data class MegaphoneRecord( val interactionCount: Int, val lastInteractionTime: Long, val firstVisible: Long, + val lastVisible: Long, val finished: Boolean ) diff --git a/app/src/main/java/org/thoughtcrime/securesms/megaphone/MegaphoneRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/megaphone/MegaphoneRepository.kt index 14268cf4fb..ca42648e3f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/megaphone/MegaphoneRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/megaphone/MegaphoneRepository.kt @@ -4,9 +4,11 @@ import android.app.Application import androidx.annotation.AnyThread import androidx.annotation.WorkerThread import org.signal.core.util.concurrent.SignalExecutors +import org.signal.core.util.logging.Log import org.thoughtcrime.securesms.database.MegaphoneDatabase import org.thoughtcrime.securesms.database.model.MegaphoneRecord import java.util.concurrent.Executor +import kotlin.time.Duration.Companion.days /** * Synchronization of data structures is done using a serial executor. Do not access or change @@ -19,6 +21,11 @@ class MegaphoneRepository(private val context: Application) { private var enabled = false + companion object { + private val TAG = Log.tag(MegaphoneRepository::class.java) + private val MAX_DISPLAY_DURATION = 3.days.inWholeMilliseconds + } + init { executor.execute { this.init() @@ -44,12 +51,33 @@ class MegaphoneRepository(private val context: Application) { } } + /** + * Note that if the next megaphone we'd choose needs to be auto-snoozed, this will result in an "off" cycle, where no megaphone will be shown. + * We could choose to keep looking, but given that auto-snooze is intended to give the user a break from megaphones, it's probably for the best that we take + * at least one cycle off. + */ @AnyThread fun getNextMegaphone(callback: Callback) { executor.execute { if (enabled) { init() - callback.onResult(Megaphones.getNextMegaphone(context, databaseCache)) + + val currentTime = System.currentTimeMillis() + val next = Megaphones.getNextMegaphone(context, databaseCache) + + if (next != null) { + val record = getRecord(next.event) + if (record.lastVisible > 0 && currentTime - record.lastVisible > MAX_DISPLAY_DURATION) { + Log.i(TAG, "Auto-snoozing ${next.event} after being visible for ${currentTime - record.lastVisible}ms without interaction.") + database.markInteractedWith(next.event, record.interactionCount + 1, currentTime) + enabled = false + resetDatabaseCache() + callback.onResult(null) + return@execute + } + } + + callback.onResult(next) } else { callback.onResult(null) } @@ -61,8 +89,17 @@ class MegaphoneRepository(private val context: Application) { val time = System.currentTimeMillis() executor.execute { - if (getRecord(event).firstVisible == 0L) { + val record = getRecord(event) + var changed = false + if (record.firstVisible == 0L) { database.markFirstVisible(event, time) + changed = true + } + if (record.lastVisible == 0L) { + database.markLastVisible(event, time) + changed = true + } + if (changed) { resetDatabaseCache() } } diff --git a/app/src/test/java/org/thoughtcrime/securesms/database/MegaphoneDatabaseTest.kt b/app/src/test/java/org/thoughtcrime/securesms/database/MegaphoneDatabaseTest.kt new file mode 100644 index 0000000000..7235161560 --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/database/MegaphoneDatabaseTest.kt @@ -0,0 +1,138 @@ +package org.thoughtcrime.securesms.database + +import android.app.Application +import androidx.sqlite.db.SupportSQLiteDatabase +import androidx.test.core.app.ApplicationProvider +import assertk.assertThat +import assertk.assertions.containsExactlyInAnyOrder +import assertk.assertions.hasSize +import assertk.assertions.isEqualTo +import assertk.assertions.isFalse +import assertk.assertions.isTrue +import assertk.assertions.single +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import org.thoughtcrime.securesms.crypto.DatabaseSecret +import org.thoughtcrime.securesms.megaphone.Megaphones +import org.thoughtcrime.securesms.testing.JdbcSqliteDatabase + +@RunWith(RobolectricTestRunner::class) +@Config(manifest = Config.NONE, application = Application::class) +class MegaphoneDatabaseTest { + + private lateinit var jdbcDatabase: JdbcSqliteDatabase + private lateinit var db: MegaphoneDatabase + + @Before + fun setUp() { + jdbcDatabase = JdbcSqliteDatabase.createInMemory() + jdbcDatabase.execSQL(MegaphoneDatabase.CREATE_TABLE) + + val backing: SupportSQLiteDatabase = jdbcDatabase + db = object : MegaphoneDatabase( + ApplicationProvider.getApplicationContext(), + DatabaseSecret(ByteArray(32)) + ) { + override val database: SupportSQLiteDatabase = backing + } + } + + @After + fun tearDown() { + jdbcDatabase.close() + } + + @Test + fun `insert adds events`() { + db.insert(listOf(Megaphones.Event.PINS_FOR_ALL, Megaphones.Event.NOTIFICATIONS)) + + assertThat(db.getAllAndDeleteMissing().map { it.event }) + .containsExactlyInAnyOrder(Megaphones.Event.PINS_FOR_ALL, Megaphones.Event.NOTIFICATIONS) + } + + @Test + fun `insert ignores duplicate events`() { + db.insert(listOf(Megaphones.Event.PINS_FOR_ALL)) + db.insert(listOf(Megaphones.Event.PINS_FOR_ALL)) + + assertThat(db.getAllAndDeleteMissing()).hasSize(1) + } + + @Test + fun `markFirstVisible sets firstVisible on the matching event only`() { + db.insert(listOf(Megaphones.Event.PINS_FOR_ALL, Megaphones.Event.NOTIFICATIONS)) + + db.markFirstVisible(Megaphones.Event.PINS_FOR_ALL, 12345L) + + val records = db.getAllAndDeleteMissing().associateBy { it.event } + assertThat(records[Megaphones.Event.PINS_FOR_ALL]!!.firstVisible).isEqualTo(12345L) + assertThat(records[Megaphones.Event.NOTIFICATIONS]!!.firstVisible).isEqualTo(0L) + } + + @Test + fun `markLastVisible sets lastVisible`() { + db.insert(listOf(Megaphones.Event.PINS_FOR_ALL)) + + db.markLastVisible(Megaphones.Event.PINS_FOR_ALL, 67890L) + + assertThat(db.getAllAndDeleteMissing()).single().transform { it.lastVisible }.isEqualTo(67890L) + } + + @Test + fun `markInteractedWith updates interaction fields and clears lastVisible`() { + db.insert(listOf(Megaphones.Event.PINS_FOR_ALL)) + db.markLastVisible(Megaphones.Event.PINS_FOR_ALL, 67890L) + + db.markInteractedWith(Megaphones.Event.PINS_FOR_ALL, interactionCount = 3, lastInteractionTimestamp = 99999L) + + val record = db.getAllAndDeleteMissing().single() + assertThat(record.interactionCount).isEqualTo(3) + assertThat(record.lastInteractionTime).isEqualTo(99999L) + assertThat(record.lastVisible).isEqualTo(0L) + } + + @Test + fun `markInteractedWith does not affect firstVisible`() { + db.insert(listOf(Megaphones.Event.PINS_FOR_ALL)) + db.markFirstVisible(Megaphones.Event.PINS_FOR_ALL, 12345L) + + db.markInteractedWith(Megaphones.Event.PINS_FOR_ALL, interactionCount = 1, lastInteractionTimestamp = 99999L) + + assertThat(db.getAllAndDeleteMissing()).single().transform { it.firstVisible }.isEqualTo(12345L) + } + + @Test + fun `markFinished sets finished`() { + db.insert(listOf(Megaphones.Event.PINS_FOR_ALL)) + + db.markFinished(Megaphones.Event.PINS_FOR_ALL) + + assertThat(db.getAllAndDeleteMissing()).single().transform { it.finished }.isTrue() + } + + @Test + fun `delete removes only the targeted event`() { + db.insert(listOf(Megaphones.Event.PINS_FOR_ALL, Megaphones.Event.NOTIFICATIONS)) + + db.delete(Megaphones.Event.PINS_FOR_ALL) + + assertThat(db.getAllAndDeleteMissing().map { it.event }) + .containsExactlyInAnyOrder(Megaphones.Event.NOTIFICATIONS) + } + + @Test + fun `freshly inserted record has zeroed counters and unfinished state`() { + db.insert(listOf(Megaphones.Event.PINS_FOR_ALL)) + + val record = db.getAllAndDeleteMissing().single() + assertThat(record.interactionCount).isEqualTo(0) + assertThat(record.lastInteractionTime).isEqualTo(0L) + assertThat(record.firstVisible).isEqualTo(0L) + assertThat(record.lastVisible).isEqualTo(0L) + assertThat(record.finished).isFalse() + } +} diff --git a/app/src/test/java/org/thoughtcrime/securesms/megaphone/BackupUpsellScheduleTest.kt b/app/src/test/java/org/thoughtcrime/securesms/megaphone/BackupUpsellScheduleTest.kt index 9d17ad8afc..dc7a5b5f51 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/megaphone/BackupUpsellScheduleTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/megaphone/BackupUpsellScheduleTest.kt @@ -118,8 +118,8 @@ class BackupUpsellScheduleTest { return BackupUpsellSchedule(records, *gaps) } - private fun record(event: Event, seenCount: Int = 1, lastSeen: Long = 0, firstVisible: Long = 0, finished: Boolean = false): MegaphoneRecord { - return MegaphoneRecord(event, seenCount, lastSeen, firstVisible, finished) + private fun record(event: Event, seenCount: Int = 1, lastSeen: Long = 0, firstVisible: Long = 0, lastVisible: Long = 0, finished: Boolean = false): MegaphoneRecord { + return MegaphoneRecord(event, seenCount, lastSeen, firstVisible, lastVisible, finished) } private fun emptyRecords(): Map {