From 07049a3f351393a5fe73cf020d11633ab3d42303 Mon Sep 17 00:00:00 2001 From: Song Eric Yu Li Date: Fri, 30 Jan 2026 00:53:36 -0800 Subject: [PATCH 1/6] fix: review reminders schema v2 migration ...plus better unit tests Review Reminders Reverted the quick-and-dirty schema migration crash fix for onlyNotifyIfNoReviews and added a proper schema migration. Created a schema migration using the established system for review reminder schema migrations. Moved all subclasses of ReviewReminderSchema to ReviewRemindersDatabase so that ReviewReminderSchema can be made sealed. Improved unit tests and added regression tests to ensure that all future changes to ReviewReminder are caught. --- .../anki/reviewreminders/ReviewReminder.kt | 2 +- .../ReviewRemindersDatabase.kt | 107 +++- .../ReviewRemindersDatabaseTest.kt | 466 +++++++++++------- 3 files changed, 383 insertions(+), 192 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt index 3f1b017ae0c6..0bf200171768 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt @@ -197,7 +197,7 @@ data class ReviewReminder private constructor( val scope: ReviewReminderScope, var enabled: Boolean, val profileID: String, - val onlyNotifyIfNoReviews: Boolean = false, + val onlyNotifyIfNoReviews: Boolean, ) : Parcelable, ReviewReminderSchema { companion object { diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt index 3a6ed9bbfd14..3d778b0a2b21 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt @@ -100,13 +100,14 @@ object ReviewRemindersDatabase { /** * Current [ReviewReminder] schema version. Whenever [ReviewReminder] is modified, this integer MUST be incremented. * - * Version 1: 3 August 2025 + * Version 1: 3 August 2025 - Initial version + * Version 2: 25 January 2026 - Added [ReviewReminder.onlyNotifyIfNoReviews] * * @see [oldReviewReminderSchemasForMigration] * @see [ReviewReminder] */ @VisibleForTesting - var schemaVersion = ReviewReminderSchemaVersion(1) + var schemaVersion = ReviewReminderSchemaVersion(2) /** * A map of all old [ReviewReminderSchema]s that [ReviewRemindersDatabase.performSchemaMigration] will attempt to migrate old @@ -123,7 +124,8 @@ object ReviewRemindersDatabase { @VisibleForTesting var oldReviewReminderSchemasForMigration: Map> = mapOf( - ReviewReminderSchemaVersion(1) to ReviewReminder::class, // Most up to date version + ReviewReminderSchemaVersion(1) to ReviewReminderSchemaV1::class, + ReviewReminderSchemaVersion(2) to ReviewReminder::class, // Most up to date version ) /** @@ -207,6 +209,7 @@ object ReviewRemindersDatabase { jsonString: String, deckKeyForMigrationPurposes: String, ): HashMap { + Timber.v("Decoding review reminders JSON string: $jsonString") val storedReviewRemindersMap = Json.decodeFromString(jsonString) return if (storedReviewRemindersMap.version.value != schemaVersion.value) { performSchemaMigration( @@ -340,7 +343,8 @@ value class ReviewReminderSchemaVersion( * @see [ReviewRemindersDatabase.performSchemaMigration]. * @see [ReviewReminder] */ -interface ReviewReminderSchema { +@Serializable +sealed interface ReviewReminderSchema { /** * All review reminders must have an identifying ID. * This is necessary to facilitate migrations. See the implementation of [ReviewRemindersDatabase.performSchemaMigration] for details. @@ -352,3 +356,98 @@ interface ReviewReminderSchema { */ fun migrate(): ReviewReminderSchema } + +/** + * Version 1 of [ReviewReminderSchema]. Updated to Version 2 by adding [ReviewReminder.onlyNotifyIfNoReviews]. + */ +@Serializable +data class ReviewReminderSchemaV1( + override val id: ReviewReminderId, + val time: ReviewReminderTime, + val cardTriggerThreshold: ReviewReminderCardTriggerThreshold, + val scope: ReviewReminderScope, + var enabled: Boolean, + val profileID: String, + val onlyNotifyIfNoReviews: Boolean = false, +) : ReviewReminderSchema { + override fun migrate(): ReviewReminderSchema = + ReviewReminder.createReviewReminder( + time = time, + cardTriggerThreshold = cardTriggerThreshold, + scope = scope, + enabled = enabled, + profileID = profileID, + onlyNotifyIfNoReviews = onlyNotifyIfNoReviews, + ) +} + +/** + * Schema migration settings for testing purposes. + * Consult this as an example of how to save old schemas and define their [ReviewReminderSchema.migrate] methods. + */ +object TestingReviewReminderMigrationSettings { + /** + * A sample old review reminder schema. Perhaps this was how the [ReviewReminder] data class was originally implemented. + * We would like to test the code that checks if review reminders stored on the device adhere to an old, outdated schema. + * In particular, does the code correctly migrate the serialized data class strings to the updated, current version of [ReviewReminder]? + */ + @Serializable + data class ReviewReminderTestSchemaVersionOne( + override val id: ReviewReminderId, + val hour: Int, + val minute: Int, + val cardTriggerThreshold: Int, + val did: DeckId, + val enabled: Boolean = true, + ) : ReviewReminderSchema { + override fun migrate(): ReviewReminderSchema = + ReviewReminderTestSchemaVersionTwo( + id = this.id, + time = VersionTwoDataClasses.ReviewReminderTime(hour, minute), + snoozeAmount = 1, + cardTriggerThreshold = this.cardTriggerThreshold, + did = this.did, + enabled = enabled, + ) + } + + /** + * Here's an example of how you can handle renamed fields in a data class stored as part of a [ReviewReminder]. + * Otherwise, there's a namespace collision with [ReviewReminderTime]. + * + * This class will be serialized into "ReviewReminderTime(timeHour=#, timeMinute=#)", which otherwise might conflict + * with the updated definition of [ReviewReminderTime], which is serialized as "ReviewReminderTime(hour=#, minute=#)". + * When we read the outdated schema from the disk, we need to tell the deserializer that it is reading a + * [VersionTwoDataClasses.ReviewReminderTime] rather than a [ReviewReminderTime], even though the names are the same. + * + * @see ReviewReminderTestSchemaVersionTwo + */ + object VersionTwoDataClasses { + @Serializable + data class ReviewReminderTime( + val timeHour: Int, + val timeMinute: Int, + ) + } + + /** + * Another example of an old review reminder schema. See [ReviewReminderTestSchemaVersionOne] for more details. + */ + @Serializable + data class ReviewReminderTestSchemaVersionTwo( + override val id: ReviewReminderId, + val time: VersionTwoDataClasses.ReviewReminderTime, + val snoozeAmount: Int, + val cardTriggerThreshold: Int, + val did: DeckId, + val enabled: Boolean = true, + ) : ReviewReminderSchema { + override fun migrate(): ReviewReminder = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(this.time.timeHour, this.time.timeMinute), + cardTriggerThreshold = ReviewReminderCardTriggerThreshold(this.cardTriggerThreshold), + scope = if (this.did == -1L) ReviewReminderScope.Global else ReviewReminderScope.DeckSpecific(this.did), + enabled = enabled, + ) + } +} diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt index b1f3a3c74066..174da14aa097 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt @@ -19,11 +19,12 @@ package com.ichi2.anki.reviewreminders import androidx.core.content.edit import androidx.test.ext.junit.runners.AndroidJUnit4 import com.ichi2.anki.RobolectricTest -import com.ichi2.anki.libanki.DeckId -import kotlinx.serialization.Serializable +import kotlinx.serialization.InternalSerializationApi +import kotlinx.serialization.KSerializer import kotlinx.serialization.SerializationException +import kotlinx.serialization.builtins.MapSerializer import kotlinx.serialization.json.Json -import org.hamcrest.CoreMatchers +import kotlinx.serialization.serializer import org.hamcrest.Description import org.hamcrest.Matcher import org.hamcrest.MatcherAssert.assertThat @@ -38,77 +39,6 @@ import org.junit.Test import org.junit.runner.RunWith import kotlin.reflect.full.memberProperties -/** - * Schema migration settings for testing purposes. - * Consult this as an example of how to save old schemas and define their [ReviewReminderSchema.migrate] methods. - */ -object TestingReviewReminderMigrationSettings { - /** - * A sample old review reminder schema. Perhaps this was how the [ReviewReminder] data class was originally implemented. - * We would like to test the code that checks if review reminders stored on the device adhere to an old, outdated schema. - * In particular, does the code correctly migrate the serialized data class strings to the updated, current version of [ReviewReminder]? - */ - @Serializable - data class ReviewReminderSchemaVersionOne( - override val id: ReviewReminderId, - val hour: Int, - val minute: Int, - val cardTriggerThreshold: Int, - val did: DeckId, - val enabled: Boolean = true, - ) : ReviewReminderSchema { - override fun migrate(): ReviewReminderSchema = - ReviewReminderSchemaVersionTwo( - id = this.id, - time = VersionTwoDataClasses.ReviewReminderTime(hour, minute), - snoozeAmount = 1, - cardTriggerThreshold = this.cardTriggerThreshold, - did = this.did, - enabled = enabled, - ) - } - - /** - * Here's an example of how you can handle renamed fields in a data class stored as part of a [ReviewReminder]. - * Otherwise, there's a namespace collision with [ReviewReminderTime]. - * - * This class will be serialized into "ReviewReminderTime(timeHour=#, timeMinute=#)", which otherwise might conflict - * with the updated definition of [ReviewReminderTime], which is serialized as "ReviewReminderTime(hour=#, minute=#)". - * When we read the outdated schema from the disk, we need to tell the deserializer that it is reading a - * [VersionTwoDataClasses.ReviewReminderTime] rather than a [ReviewReminderTime], even though the names are the same. - * - * @see ReviewReminderSchemaVersionTwo - */ - object VersionTwoDataClasses { - @Serializable - data class ReviewReminderTime( - val timeHour: Int, - val timeMinute: Int, - ) - } - - /** - * Another example of an old review reminder schema. See [ReviewReminderSchemaVersionOne] for more details. - */ - @Serializable - data class ReviewReminderSchemaVersionTwo( - override val id: ReviewReminderId, - val time: VersionTwoDataClasses.ReviewReminderTime, - val snoozeAmount: Int, - val cardTriggerThreshold: Int, - val did: DeckId, - val enabled: Boolean = true, - ) : ReviewReminderSchema { - override fun migrate(): ReviewReminder = - ReviewReminder.createReviewReminder( - time = ReviewReminderTime(this.time.timeHour, this.time.timeMinute), - cardTriggerThreshold = ReviewReminderCardTriggerThreshold(this.cardTriggerThreshold), - scope = if (this.did == -1L) ReviewReminderScope.Global else ReviewReminderScope.DeckSpecific(this.did), - enabled = enabled, - ) - } -} - /** * If tests in this file have failed, it may be because you have updated [ReviewReminder]! * Please read the documentation of [ReviewReminder] carefully and ensure you have implemented @@ -370,18 +300,22 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { /** * If this test has failed, please ensure the review reminder schema version and old schemas in the review reminder - * migration chain are set correctly. + * migration chain are set correctly. If you've written a new migration, please also write a new test in this file + * to prove your migration works! + * + * This test is designed to fail and be updated every time the schema is changed + * to ensure developers know what they are doing and to remind them to write migration tests. */ @Test fun `current schema version points to ReviewReminder`() { - assertThat(ReviewRemindersDatabase.schemaVersion.value, equalTo(1)) + assertThat(ReviewRemindersDatabase.schemaVersion.value, equalTo(2)) assertThat( ReviewRemindersDatabase .oldReviewReminderSchemasForMigration .keys .last() .value, - equalTo(1), + equalTo(2), ) assertThat( ReviewRemindersDatabase @@ -392,6 +326,150 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { ) } + /** + * If this test has failed, you have likely updated [ReviewReminder] without writing a migration! + * Please write a migration (see [ReviewReminder] and the tests in this file for more information), + * update the latest schema version (see [ReviewRemindersDatabase]), add a new test in this file + * to prove your migration works, and update this test to the new latest schema version. + * + * To get a raw string, add a log to [ReviewRemindersDatabase.decodeJson] to print out its input string. + * + * This test is designed to fail and be updated every time the schema is changed + * to ensure developers know what they are doing and to remind them to write migration tests. + */ + @Test + fun `raw ReviewReminder string can be deserialized without throwing`() { + val rawString = + """ + { + "version":2, + "remindersMapJson":"{\"0\":{\"id\":0,\"time\":{\"hour\":9,\"minute\":0},\"cardTriggerThreshold\":5,\"scope\":{\"type\":\"com.ichi2.anki.reviewreminders.ReviewReminderScope.DeckSpecific\",\"did\":12345},\"enabled\":false,\"profileID\":\"\",\"onlyNotifyIfNoReviews\":false},\"1\":{\"id\":1,\"time\":{\"hour\":10,\"minute\":30},\"cardTriggerThreshold\":10,\"scope\":{\"type\":\"com.ichi2.anki.reviewreminders.ReviewReminderScope.DeckSpecific\",\"did\":12345},\"enabled\":true,\"profileID\":\"\",\"onlyNotifyIfNoReviews\":false}}" + } + """.trimIndent() + + val storedReviewRemindersMap = Json.decodeFromString(rawString) + val mapSerializer = MapSerializer(ReviewReminderId.serializer(), ReviewReminder.serializer()) + Json.decodeFromString(mapSerializer, storedReviewRemindersMap.remindersMapJson) + } + + /** + * If this test has failed, you have likely updated [ReviewReminder]'s schema! Please ensure you've written + * a migration for this schema change (see [ReviewReminder]) and write a test in this file to prove your migration works. + * + * This test is designed to fail and be updated every time the schema is changed + * to ensure developers know what they are doing and to remind them to write migration tests. + */ + @Test + fun `ReviewReminder properties and types are the expected values`() { + val expectedPropertiesWithTypes = + mapOf( + "id" to ReviewReminderId::class, + "time" to ReviewReminderTime::class, + "cardTriggerThreshold" to ReviewReminderCardTriggerThreshold::class, + "scope" to ReviewReminderScope::class, + "enabled" to Boolean::class, + "profileID" to String::class, + "onlyNotifyIfNoReviews" to Boolean::class, + ) + + val actualPropertiesWithTypes = + ReviewReminder::class + .memberProperties + .associate { it.name to it.returnType.classifier } + + assertThat(actualPropertiesWithTypes, equalTo(expectedPropertiesWithTypes)) + } + + /** + * A single test case for migration testing. + * @see assertMigrationsWork + */ + private data class MigrationTestCase( + val inputVersion: ReviewReminderSchemaVersion, + val input: ReviewReminderSchema, + val expectedOutput: ReviewReminder, + ) + + /** + * Helper function for performing migrations and asserting they work as expected. + * + * In order to create a unified helper function for doing this which can accept arbitrary subclasses + * of [ReviewReminderSchema] and get their serializers at runtime, we need to opt into + * the internal serialization API. Since this is a test-only function, this should be acceptable. + */ + @OptIn(InternalSerializationApi::class) + private fun assertMigrationsWork(vararg testCases: MigrationTestCase) { + // Group + val groupedByScope = + testCases.groupBy { + when (val scope = it.expectedOutput.scope) { + is ReviewReminderScope.DeckSpecific -> scope.did + is ReviewReminderScope.Global -> null + } + } + + // Write + groupedByScope.forEach { (did, casesInScope) -> + // Reading and writing is done per scope, so all test cases in a scope will have the same input version + if (casesInScope.map { it.inputVersion }.toSet().size != 1) { + throw IllegalArgumentException("All test cases in a scope must have the same input version and type") + } + val version = casesInScope.first().inputVersion + val inputType = ReviewRemindersDatabase.oldReviewReminderSchemasForMigration[version]!! + + // We need an unchecked runtime cast to allow this helper to operate on arbitrary subclasses of ReviewReminderSchema + @Suppress("UNCHECKED_CAST") + val inputSerializer = inputType.serializer() as KSerializer + val mapSerializer = MapSerializer(ReviewReminderId.serializer(), inputSerializer) + + val inputMap = casesInScope.associate { it.input.id to it.input } + val packagedInput = + ReviewRemindersDatabase.StoredReviewRemindersMap( + version, + Json.encodeToString(mapSerializer, inputMap), + ) + + val key = + if (did != null) { + ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did + } else { + ReviewRemindersDatabase.APP_WIDE_KEY + } + ReviewRemindersDatabase.remindersSharedPrefs.edit(commit = true) { + putString(key, Json.encodeToString(packagedInput)) + } + } + + // Read and assert + groupedByScope.forEach { (did, casesInScope) -> + val retrievedReminders = + if (did != null) { + ReviewRemindersDatabase.getRemindersForDeck(did) + } else { + ReviewRemindersDatabase.getAllAppWideReminders() + } + + // We ignore ID because the migration process will generate new review reminders from scratch during the migration + // ID is a private, inaccessible property + // Instead, we only check that the ID matches the key in the map; all other properties can be compared normally + retrievedReminders.forEach { (id, reminder) -> + assertThat(id, equalTo(reminder.id)) + } + assertThat( + retrievedReminders.values, + containsEqualReviewRemindersInAnyOrderIgnoringId( + casesInScope.map { it.expectedOutput }, + ), + ) + } + + // Shared Preferences should not contain any random corrupted keys after or due to the migration process + assertThat( + ReviewRemindersDatabase.remindersSharedPrefs.all.size, + equalTo(groupedByScope.size), + ) + } + @Test fun `review reminder schema migration works`() { // Save existing mocks @@ -401,128 +479,142 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { ReviewRemindersDatabase.schemaVersion = ReviewReminderSchemaVersion(3) ReviewRemindersDatabase.oldReviewReminderSchemasForMigration = mapOf( - ReviewReminderSchemaVersion(1) to TestingReviewReminderMigrationSettings.ReviewReminderSchemaVersionOne::class, - ReviewReminderSchemaVersion(2) to TestingReviewReminderMigrationSettings.ReviewReminderSchemaVersionTwo::class, + ReviewReminderSchemaVersion(1) to TestingReviewReminderMigrationSettings.ReviewReminderTestSchemaVersionOne::class, + ReviewReminderSchemaVersion(2) to TestingReviewReminderMigrationSettings.ReviewReminderTestSchemaVersionTwo::class, ReviewReminderSchemaVersion(3) to ReviewReminder::class, ) - // To spice things up, some will be version one... - val versionOneDummyDeckSpecificRemindersForDeckOne = - mapOf( - ReviewReminderId(0) to - TestingReviewReminderMigrationSettings.ReviewReminderSchemaVersionOne( - ReviewReminderId(0), - 9, - 0, - 5, - did1, - false, + + assertMigrationsWork( + // To spice things up, some will be version one... + MigrationTestCase( + inputVersion = ReviewReminderSchemaVersion(1), + input = + TestingReviewReminderMigrationSettings.ReviewReminderTestSchemaVersionOne( + id = ReviewReminderId(0), + hour = 9, + minute = 0, + cardTriggerThreshold = 5, + did = did1, + enabled = false, ), - ReviewReminderId(1) to - TestingReviewReminderMigrationSettings.ReviewReminderSchemaVersionOne( - ReviewReminderId(1), - 10, - 30, - 10, - did1, + expectedOutput = dummyDeckSpecificRemindersForDeckOne[ReviewReminderId(0)]!!, + ), + MigrationTestCase( + inputVersion = ReviewReminderSchemaVersion(1), + input = + TestingReviewReminderMigrationSettings.ReviewReminderTestSchemaVersionOne( + id = ReviewReminderId(1), + hour = 10, + minute = 30, + cardTriggerThreshold = 10, + did = did1, ), - ) - // ...and some will be version two - val versionTwoDummyDeckSpecificRemindersForDeckTwo = - mapOf( - ReviewReminderId(2) to - TestingReviewReminderMigrationSettings.ReviewReminderSchemaVersionTwo( - ReviewReminderId(2), - TestingReviewReminderMigrationSettings.VersionTwoDataClasses.ReviewReminderTime(10, 30), - 1, - 10, - did2, + expectedOutput = dummyDeckSpecificRemindersForDeckOne[ReviewReminderId(1)]!!, + ), + // ...and some will be version two... + MigrationTestCase( + inputVersion = ReviewReminderSchemaVersion(2), + input = + TestingReviewReminderMigrationSettings.ReviewReminderTestSchemaVersionTwo( + id = ReviewReminderId(2), + time = TestingReviewReminderMigrationSettings.VersionTwoDataClasses.ReviewReminderTime(10, 30), + snoozeAmount = 1, + cardTriggerThreshold = 10, + did = did2, + enabled = true, ), - ReviewReminderId(3) to - TestingReviewReminderMigrationSettings.ReviewReminderSchemaVersionTwo( - ReviewReminderId(3), - TestingReviewReminderMigrationSettings.VersionTwoDataClasses.ReviewReminderTime(12, 30), - 1, - 20, - did2, + expectedOutput = dummyDeckSpecificRemindersForDeckTwo[ReviewReminderId(2)]!!, + ), + MigrationTestCase( + inputVersion = ReviewReminderSchemaVersion(2), + input = + TestingReviewReminderMigrationSettings.ReviewReminderTestSchemaVersionTwo( + id = ReviewReminderId(3), + time = TestingReviewReminderMigrationSettings.VersionTwoDataClasses.ReviewReminderTime(12, 30), + snoozeAmount = 1, + cardTriggerThreshold = 20, + did = did2, ), - ) - val versionOneDummyAppWideReminders = - mapOf( - ReviewReminderId(4) to - TestingReviewReminderMigrationSettings.ReviewReminderSchemaVersionOne( - ReviewReminderId(4), - 9, - 0, - 5, - -1L, + expectedOutput = dummyDeckSpecificRemindersForDeckTwo[ReviewReminderId(3)]!!, + ), + // ...and some will be app-wide for good measure + MigrationTestCase( + inputVersion = ReviewReminderSchemaVersion(1), + input = + TestingReviewReminderMigrationSettings.ReviewReminderTestSchemaVersionOne( + id = ReviewReminderId(4), + hour = 9, + minute = 0, + cardTriggerThreshold = 5, + did = -1L, ), - ReviewReminderId(5) to - TestingReviewReminderMigrationSettings.ReviewReminderSchemaVersionOne( - ReviewReminderId(5), - 10, - 30, - 10, - -1L, + expectedOutput = dummyAppWideReminders[ReviewReminderId(4)]!!, + ), + MigrationTestCase( + inputVersion = ReviewReminderSchemaVersion(1), + input = + TestingReviewReminderMigrationSettings.ReviewReminderTestSchemaVersionOne( + id = ReviewReminderId(5), + hour = 10, + minute = 30, + cardTriggerThreshold = 10, + did = -1L, ), - ) - - val packagedDeckOneReminders = - ReviewRemindersDatabase.StoredReviewRemindersMap( - ReviewReminderSchemaVersion(1), - Json.encodeToString(versionOneDummyDeckSpecificRemindersForDeckOne), - ) - val packagedDeckTwoReminders = - ReviewRemindersDatabase.StoredReviewRemindersMap( - ReviewReminderSchemaVersion(2), - Json.encodeToString(versionTwoDummyDeckSpecificRemindersForDeckTwo), - ) - val packagedGlobalReminders = - ReviewRemindersDatabase.StoredReviewRemindersMap( - ReviewReminderSchemaVersion(1), - Json.encodeToString(versionOneDummyAppWideReminders), - ) - - ReviewRemindersDatabase.remindersSharedPrefs.edit(commit = true) { - putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, Json.encodeToString(packagedDeckOneReminders)) - putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did2, Json.encodeToString(packagedDeckTwoReminders)) - putString(ReviewRemindersDatabase.APP_WIDE_KEY, Json.encodeToString(packagedGlobalReminders)) - } - - val retrievedDeckOneReminders = ReviewRemindersDatabase.getRemindersForDeck(did1) - val retrievedDeckTwoReminders = ReviewRemindersDatabase.getRemindersForDeck(did2) - val retrievedGlobalReminders = ReviewRemindersDatabase.getAllAppWideReminders() - - retrievedDeckOneReminders.forEach { (id, reminder) -> - assertThat(id, equalTo(reminder.id)) - } - retrievedDeckTwoReminders.forEach { (id, reminder) -> - assertThat(id, equalTo(reminder.id)) - } - retrievedGlobalReminders.forEach { (id, reminder) -> - assertThat(id, equalTo(reminder.id)) - } - - // We ignore ID because the migration process will generate new review reminders from scratch during the migration; ID is a private, inaccessible property - assertThat( - retrievedDeckOneReminders.values, - containsEqualReviewRemindersInAnyOrderIgnoringId(dummyDeckSpecificRemindersForDeckOne.values), - ) - assertThat( - retrievedDeckTwoReminders.values, - containsEqualReviewRemindersInAnyOrderIgnoringId(dummyDeckSpecificRemindersForDeckTwo.values), + expectedOutput = dummyAppWideReminders[ReviewReminderId(5)]!!, + ), ) - assertThat( - retrievedGlobalReminders.values, - containsEqualReviewRemindersInAnyOrderIgnoringId(dummyAppWideReminders.values), - ) - - // Shared Preferences should not contain any random corrupted keys after or due to the migration process - // There should be three: two for the specific decks, one for app-wide - val sharedPrefsSize = ReviewRemindersDatabase.remindersSharedPrefs.all.size - assertThat(sharedPrefsSize, CoreMatchers.equalTo(3)) // Reset mocks ReviewRemindersDatabase.schemaVersion = savedSchemaVersion ReviewRemindersDatabase.oldReviewReminderSchemasForMigration = savedOldReviewReminderSchemasForMigration } + + @Test + fun `review reminder v1 to v2 migration works`() { + assertMigrationsWork( + MigrationTestCase( + inputVersion = ReviewReminderSchemaVersion(1), + input = + ReviewReminderSchemaV1( + id = ReviewReminderId(0), + time = ReviewReminderTime(9, 0), + cardTriggerThreshold = ReviewReminderCardTriggerThreshold(5), + scope = ReviewReminderScope.DeckSpecific(did1), + enabled = true, + profileID = "", + ), + expectedOutput = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(9, 0), + cardTriggerThreshold = ReviewReminderCardTriggerThreshold(5), + scope = ReviewReminderScope.DeckSpecific(did1), + enabled = true, + profileID = "", + onlyNotifyIfNoReviews = false, + ), + ), + MigrationTestCase( + inputVersion = ReviewReminderSchemaVersion(1), + input = + ReviewReminderSchemaV1( + id = ReviewReminderId(1), + time = ReviewReminderTime(10, 30), + cardTriggerThreshold = ReviewReminderCardTriggerThreshold(10), + scope = ReviewReminderScope.Global, + enabled = false, + profileID = "", + onlyNotifyIfNoReviews = true, + ), + expectedOutput = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(10, 30), + cardTriggerThreshold = ReviewReminderCardTriggerThreshold(10), + scope = ReviewReminderScope.Global, + enabled = false, + profileID = "", + onlyNotifyIfNoReviews = true, + ), + ), + ) + } } From 3a4b44bab48db2eb64d087123f7b9940474b2177 Mon Sep 17 00:00:00 2001 From: Song Eric Yu Li Date: Sun, 1 Feb 2026 17:13:50 -0800 Subject: [PATCH 2/6] refactor: ReviewReminderGroup Previously, ReviewRemindersDatabase used "HashMap" a lot. Arthur suggested making a small abstraction over this to 1. get rid of the verbosity that comes with saying "HashMap" over and over, and 2. limiting the functionality provided by "HashMap" to a more clearly-defined, clean, limited interface. I've implemented that here. The trickle-down effects of this abstraction cause multiple files, ex. tests, to change. I've also moved all the ReviewReminderSchema including old ReviewReminder schemas into a new, separate file to make ReviewRemindersDatabase shorter and cleaner. Previously, you could use ReviewRemindersDatabase.editRemindersForDeck and ReviewRemindersDatabase.editAllAppWideReminders to write deck-specific reminders to the app-wide key or vice versa. None of the code currently does so, but it really should not be allowed at all. I've added a `require` clause so that it causes a runtime exception if attempted, with unit tests. A better solution would probably be to create two separate kinds of ReviewReminder, one of which is DeckSpecific and the other of which is AppWide, so that trying to write one kind to the wrong key causes a compile-time error, but that would require a massive refactor of ReviewReminder and the schema migration system and, in my opinion, be overly complicated. --- .../reviewreminders/ReviewReminderGroup.kt | 119 ++++++++ .../reviewreminders/ReviewReminderSchema.kt | 159 +++++++++++ .../ReviewRemindersDatabase.kt | 261 +++++------------- .../anki/reviewreminders/ScheduleReminders.kt | 47 ++-- .../anki/services/AlarmManagerService.kt | 3 +- .../ReviewRemindersDatabaseTest.kt | 89 +++--- .../anki/services/AlarmManagerServiceTest.kt | 20 +- .../anki/services/NotificationServiceTest.kt | 25 +- 8 files changed, 454 insertions(+), 269 deletions(-) create mode 100644 AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderGroup.kt create mode 100644 AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderSchema.kt diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderGroup.kt b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderGroup.kt new file mode 100644 index 000000000000..de66f6f1f9aa --- /dev/null +++ b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderGroup.kt @@ -0,0 +1,119 @@ +/* + * Copyright (c) 2026 Eric Li + * + * This program is free software; you can redistribute it and/or modify it under + * the terms of the GNU General Public License as published by the Free Software + * Foundation; either version 3 of the License, or (at your option) any later + * version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT ANY + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A + * PARTICULAR PURPOSE. See the GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see . + */ + +package com.ichi2.anki.reviewreminders + +import kotlinx.serialization.SerializationException +import kotlinx.serialization.json.Json + +/** + * A group of review reminders, all for the same [ReviewReminderScope], + * persisted as a single JSON string in SharedPreferences. + * + * Essentially a wrapper around a HashMap of [ReviewReminderId] to [ReviewReminder], + * explicitly defined to restrict what can be done with the interface and to eliminate the + * need to verbosely type out "HashMap" everywhere. + * + * A HashMap is used to allow for O(1) access to individual reminders by [ReviewReminderId]. + */ +class ReviewReminderGroup( + private val underlyingMap: HashMap, +) { + constructor() : this(HashMap()) + + constructor(map: Map) : this(HashMap(map)) + + /** + * Manually construct a [ReviewReminderGroup] from key-value pairs. + */ + constructor(vararg pairs: Pair) : this( + buildMap { pairs.forEach { put(it.first, it.second) } }, + ) + + /** + * Merge multiple [ReviewReminderGroup]s into one. + * [ReviewReminderId] should be unique, so there should be no collisions. + */ + constructor(vararg groups: ReviewReminderGroup) : this( + buildMap { groups.forEach { putAll(it.underlyingMap) } }, + ) + + /** + * Constructs a [ReviewReminderGroup] from a serialized JSON string. + * + * @throws SerializationException If the stored string is not a valid JSON string. + * @throws IllegalArgumentException If the decoded reminders map is not a HashMap<[ReviewReminderId], [ReviewReminder]>. + */ + constructor(serializedString: String) : this( + Json.decodeFromString>(serializedString), + ) + + /** + * Serializes this [ReviewReminderGroup] to a JSON string for storage. + */ + fun serializeToString(): String = Json.encodeToString(underlyingMap) + + /** + * Merge two [ReviewReminderGroup]s into one. + * [ReviewReminderId] should be unique, so there should be no collisions. + */ + operator fun plus(other: ReviewReminderGroup): ReviewReminderGroup = ReviewReminderGroup(underlyingMap + other.underlyingMap) + + operator fun get(id: ReviewReminderId) = underlyingMap[id] + + operator fun set( + id: ReviewReminderId, + reminder: ReviewReminder, + ) { + underlyingMap[id] = reminder + } + + fun isEmpty(): Boolean = underlyingMap.isEmpty() + + fun remove(id: ReviewReminderId) { + underlyingMap.remove(id) + } + + fun forEach(action: (ReviewReminderId, ReviewReminder) -> Unit) { + underlyingMap.forEach { (id, reminder) -> action(id, reminder) } + } + + /** + * Get the values of the underlying map, removing the [ReviewReminderId] keys. + */ + fun getRemindersList(): List = underlyingMap.values.toList() + + /** + * Toggles whether a [ReviewReminder] is enabled. + */ + fun toggleEnabled(id: ReviewReminderId) { + underlyingMap[id]?.apply { enabled = !enabled } + } + + override fun equals(other: Any?): Boolean = other is ReviewReminderGroup && other.underlyingMap == this.underlyingMap + + override fun hashCode(): Int = underlyingMap.hashCode() +} + +/** + * Convenience extension constructor for merging a list of [ReviewReminderGroup]s into one. + */ +fun List.mergeAll() = ReviewReminderGroup(*this.toTypedArray()) + +/** + * Convenience typealias for the mutation functions passed to editors of [ReviewReminderGroup]. + */ +typealias ReviewReminderGroupEditor = (ReviewReminderGroup) -> ReviewReminderGroup diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderSchema.kt b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderSchema.kt new file mode 100644 index 000000000000..90693590e53e --- /dev/null +++ b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderSchema.kt @@ -0,0 +1,159 @@ +/* + * Copyright (c) 2026 Eric Li + * + * This program is free software; you can redistribute it and/or modify it under + * the terms of the GNU General Public License as published by the Free Software + * Foundation; either version 3 of the License, or (at your option) any later + * version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT ANY + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A + * PARTICULAR PURPOSE. See the GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see . + */ + +package com.ichi2.anki.reviewreminders + +import com.ichi2.anki.libanki.DeckId +import com.ichi2.anki.reviewreminders.ReviewRemindersDatabase.StoredReviewReminderGroup +import kotlinx.serialization.Serializable + +/** + * Inline value class for review reminder schema versions. + * @see [StoredReviewReminderGroup] + * @see [ReviewReminder] + */ +@JvmInline +@Serializable +value class ReviewReminderSchemaVersion( + val value: Int, +) { + init { + require(value >= 1) { "Review reminder schema version must be >= 1" } + // We do not check that it is <= SCHEMA_VERSION here because then declaring SCHEMA_VERSION would be circular + } +} + +/** + * When [ReviewReminder] is updated by a developer, implement this interface in a new data class which + * has the same fields as the old version of [ReviewReminder], then implement the [migrate] method which + * transforms old [ReviewReminder]s to new [ReviewReminder]s. Also ensure that the previous [ReviewReminderSchema] + * in the migration version chain ([ReviewRemindersDatabase.oldReviewReminderSchemasForMigration]) has its [migrate] method + * edited to return instances of the newly-created [ReviewReminderSchema]. Then, increment [ReviewRemindersDatabase.schemaVersion]. + * + * Data classes implementing this interface should be marked as @Serializable. Any new types defined for ReviewReminderSchemas + * should also be marked as @Serializable. + * + * @see [ReviewRemindersDatabase.performSchemaMigration]. + * @see [ReviewReminder] + */ +@Serializable +sealed interface ReviewReminderSchema { + /** + * All review reminders must have an identifying ID. + * This is necessary to facilitate migrations. See the implementation of [ReviewRemindersDatabase.performSchemaMigration] for details. + */ + val id: ReviewReminderId + + /** + * Transforms this [ReviewReminderSchema] to the next version of the [ReviewReminderSchema]. + */ + fun migrate(): ReviewReminderSchema +} + +/** + * Version 1 of [ReviewReminderSchema]. Updated to Version 2 by adding [ReviewReminder.onlyNotifyIfNoReviews]. + */ +@Serializable +data class ReviewReminderSchemaV1( + override val id: ReviewReminderId, + val time: ReviewReminderTime, + val cardTriggerThreshold: ReviewReminderCardTriggerThreshold, + val scope: ReviewReminderScope, + var enabled: Boolean, + val profileID: String, + val onlyNotifyIfNoReviews: Boolean = false, +) : ReviewReminderSchema { + override fun migrate(): ReviewReminderSchema = + ReviewReminder.createReviewReminder( + time = time, + cardTriggerThreshold = cardTriggerThreshold, + scope = scope, + enabled = enabled, + profileID = profileID, + onlyNotifyIfNoReviews = onlyNotifyIfNoReviews, + ) +} + +/** + * Schema migration settings for testing purposes. + * Consult this as an example of how to save old schemas and define their [ReviewReminderSchema.migrate] methods. + */ +object TestingReviewReminderMigrationSettings { + /** + * A sample old review reminder schema. Perhaps this was how the [ReviewReminder] data class was originally implemented. + * We would like to test the code that checks if review reminders stored on the device adhere to an old, outdated schema. + * In particular, does the code correctly migrate the serialized data class strings to the updated, current version of [ReviewReminder]? + */ + @Serializable + data class ReviewReminderTestSchemaVersionOne( + override val id: ReviewReminderId, + val hour: Int, + val minute: Int, + val cardTriggerThreshold: Int, + val did: DeckId, + val enabled: Boolean = true, + ) : ReviewReminderSchema { + override fun migrate(): ReviewReminderSchema = + ReviewReminderTestSchemaVersionTwo( + id = this.id, + time = VersionTwoDataClasses.ReviewReminderTime(hour, minute), + snoozeAmount = 1, + cardTriggerThreshold = this.cardTriggerThreshold, + did = this.did, + enabled = enabled, + ) + } + + /** + * Here's an example of how you can handle renamed fields in a data class stored as part of a [ReviewReminder]. + * Otherwise, there's a namespace collision with [ReviewReminderTime]. + * + * This class will be serialized into "ReviewReminderTime(timeHour=#, timeMinute=#)", which otherwise might conflict + * with the updated definition of [ReviewReminderTime], which is serialized as "ReviewReminderTime(hour=#, minute=#)". + * When we read the outdated schema from the disk, we need to tell the deserializer that it is reading a + * [VersionTwoDataClasses.ReviewReminderTime] rather than a [ReviewReminderTime], even though the names are the same. + * + * @see ReviewReminderTestSchemaVersionTwo + */ + object VersionTwoDataClasses { + @Serializable + data class ReviewReminderTime( + val timeHour: Int, + val timeMinute: Int, + ) + } + + /** + * Another example of an old review reminder schema. See [ReviewReminderTestSchemaVersionOne] for more details. + */ + @Serializable + data class ReviewReminderTestSchemaVersionTwo( + override val id: ReviewReminderId, + val time: VersionTwoDataClasses.ReviewReminderTime, + val snoozeAmount: Int, + val cardTriggerThreshold: Int, + val did: DeckId, + val enabled: Boolean = true, + ) : ReviewReminderSchema { + override fun migrate(): ReviewReminder = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(this.time.timeHour, this.time.timeMinute), + cardTriggerThreshold = ReviewReminderCardTriggerThreshold(this.cardTriggerThreshold), + scope = if (this.did == -1L) ReviewReminderScope.Global else ReviewReminderScope.DeckSpecific(this.did), + enabled = enabled, + ) + } +} diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt index 3d778b0a2b21..dde1e7a46b37 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt @@ -22,7 +22,6 @@ import androidx.annotation.VisibleForTesting import androidx.core.content.edit import com.ichi2.anki.AnkiDroidApp import com.ichi2.anki.libanki.DeckId -import com.ichi2.anki.reviewreminders.ReviewRemindersDatabase.StoredReviewRemindersMap import kotlinx.serialization.InternalSerializationApi import kotlinx.serialization.Serializable import kotlinx.serialization.SerializationException @@ -68,20 +67,20 @@ object ReviewRemindersDatabase { /** * Key in SharedPreferences for retrieving deck-specific reminders. * Should have deck ID appended to its end, ex. "deck_12345". - * Its value is a HashMap<[ReviewReminderId], [ReviewReminder]> serialized as a JSON String. + * Its value is a [ReviewReminderGroup] serialized as a JSON String. */ @VisibleForTesting const val DECK_SPECIFIC_KEY = "deck_" /** * Key in SharedPreferences for retrieving app-wide reminders. - * Its value is a HashMap<[ReviewReminderId], [ReviewReminder]> serialized as a JSON String. + * Its value is a [ReviewReminderGroup] serialized as a JSON String. */ @VisibleForTesting const val APP_WIDE_KEY = "app_wide" /** - * The form in which HashMap<[ReviewReminderId], [ReviewReminder]> are actually written to SharedPreferences. + * The form in which [ReviewReminderGroup] are actually written to SharedPreferences. * This allows us to check the version of [ReviewReminder] stored before trying to deserialize the JSON string, * allowing us to carefully handle schema migration. Otherwise, if an older version of [ReviewReminder] is encoded * and we try to decode it into a newer form of [ReviewReminder], an error will be thrown. @@ -89,10 +88,13 @@ object ReviewRemindersDatabase { * We assume that the version is accurate; e.x. if the version is 3, then the [ReviewReminder] stored is indeed * of schema version 3. This should be safe to assume since writing this data class to SharedPreferences is an * atomic operation. + * + * @param version The version of the [remindersMapJson] which is stored. + * @param remindersMapJson The stored [ReviewReminderGroup]'s underlying hashmap as a serialized JSON string. */ @Serializable @VisibleForTesting - data class StoredReviewRemindersMap( + data class StoredReviewReminderGroup( val version: ReviewReminderSchemaVersion, val remindersMapJson: String, ) @@ -138,26 +140,26 @@ object ReviewRemindersDatabase { * dynamically via [oldReviewReminderSchemasForMigration] rather than at compile-time. * The possible schemas to deserialize from are inputted dynamically so that unit tests are possible. * - * @param encodedReviewRemindersKey The key with which the [encodedReviewRemindersMap] is stored in SharedPreferences, + * @param encodedReviewReminderKey The key with which the [encodedReviewReminderGroup] is stored in SharedPreferences, * used for writing the migrated map back into SharedPreferences. - * @param encodedReviewRemindersMap The encoded review reminders map to migrate. - * @param fromVersion The schema version of [encodedReviewRemindersMap]. + * @param encodedReviewReminderGroup The encoded review reminders map to migrate. + * @param fromVersion The schema version of [encodedReviewReminderGroup]. * @param toVersion The schema version of the new review reminders map. * * @throws SerializationException If the [fromVersion] is less than 1 or greater than [schemaVersion], or if the - * [encodedReviewRemindersMap] is not a valid JSON string, or if the final result of migration is somehow not a [ReviewReminder]. - * @throws IllegalArgumentException If the [encodedReviewRemindersMap] is not actually of version [fromVersion], + * [encodedReviewReminderGroup] is not a valid JSON string, or if the final result of migration is somehow not a [ReviewReminder]. + * @throws IllegalArgumentException If the [encodedReviewReminderGroup] is not actually of version [fromVersion], * or if the [fromVersion] is not in [oldReviewReminderSchemasForMigration]. * * @see [ReviewReminder] */ @OptIn(InternalSerializationApi::class) private fun performSchemaMigration( - encodedReviewRemindersKey: String, - encodedReviewRemindersMap: String, + encodedReviewReminderKey: String, + encodedReviewReminderGroup: String, fromVersion: ReviewReminderSchemaVersion, toVersion: ReviewReminderSchemaVersion = schemaVersion, - ): HashMap { + ): ReviewReminderGroup { Timber.i("Beginning migration from $fromVersion to $toVersion") if (fromVersion.value < 1 || fromVersion.value > toVersion.value @@ -170,7 +172,7 @@ object ReviewRemindersDatabase { oldReviewReminderSchemasForMigration[fromVersion] ?: throw IllegalArgumentException("Review reminder schema version not found: $fromVersion") val mapDeserializer = MapSerializer(ReviewReminderId.serializer(), oldSchema.serializer()) - val mapDecoded = Json.decodeFromString(mapDeserializer, encodedReviewRemindersMap) + val mapDecoded = Json.decodeFromString(mapDeserializer, encodedReviewReminderGroup) // Migrate step by step var currentMap = mapDecoded @@ -187,100 +189,108 @@ object ReviewRemindersDatabase { } // Write to SharedPreferences, then return deserialized map - val finalMap = - currentMap.mapValues { (_, value) -> - value as? ReviewReminder ?: throw SerializationException("Expected ReviewReminder, got ${value::class.qualifiedName}") - } - val jsonString = encodeJson(finalMap) + val finalGroup = + ReviewReminderGroup( + currentMap.mapValues { (_, value) -> + value as? ReviewReminder ?: throw SerializationException("Expected ReviewReminder, got ${value::class.qualifiedName}") + }, + ) remindersSharedPrefs.edit { - putString(encodedReviewRemindersKey, jsonString) + putString(encodedReviewReminderKey, encodeJson(finalGroup)) } - return HashMap(finalMap) + return finalGroup } /** - * Decode an encoded HashMap<[ReviewReminderId], [ReviewReminder]> JSON string which has been stored as a [StoredReviewRemindersMap]. + * Decode an encoded [ReviewReminderGroup] JSON string which has been stored as a [StoredReviewReminderGroup]. * @see Json.decodeFromString * @throws SerializationException If the stored string is not a valid JSON string. - * @throws IllegalArgumentException If the decoded reminders map is not a HashMap<[ReviewReminderId], [ReviewReminder]>, + * @throws IllegalArgumentException If the decoded reminders map is not a [ReviewReminderGroup], * and no valid schema migrations exist. */ private fun decodeJson( jsonString: String, deckKeyForMigrationPurposes: String, - ): HashMap { + ): ReviewReminderGroup { Timber.v("Decoding review reminders JSON string: $jsonString") - val storedReviewRemindersMap = Json.decodeFromString(jsonString) - return if (storedReviewRemindersMap.version.value != schemaVersion.value) { + val storedReviewReminderGroup = Json.decodeFromString(jsonString) + return if (storedReviewReminderGroup.version.value != schemaVersion.value) { performSchemaMigration( deckKeyForMigrationPurposes, - storedReviewRemindersMap.remindersMapJson, - storedReviewRemindersMap.version, + storedReviewReminderGroup.remindersMapJson, + storedReviewReminderGroup.version, schemaVersion, ) } else { - Json.decodeFromString>(storedReviewRemindersMap.remindersMapJson) + ReviewReminderGroup(storedReviewReminderGroup.remindersMapJson) } } /** - * Encode a Map<[ReviewReminderId], [ReviewReminder]> as a [StoredReviewRemindersMap] JSON string. + * Encode a [ReviewReminderGroup] as a [StoredReviewReminderGroup] JSON string. * @see Json.encodeToString - * @throws SerializationException If the stored string is somehow not a valid JSON string, even though the input parameter is type-checked. */ - private fun encodeJson(reminders: Map): String = - Json.encodeToString(StoredReviewRemindersMap.serializer(), StoredReviewRemindersMap(schemaVersion, Json.encodeToString(reminders))) + private fun encodeJson(reminders: ReviewReminderGroup): String = + Json.encodeToString(StoredReviewReminderGroup.serializer(), StoredReviewReminderGroup(schemaVersion, reminders.serializeToString())) /** * Get the [ReviewReminder]s for a specific key. * @throws SerializationException If the value associated with this key is not valid JSON string. - * @throws IllegalArgumentException If the decoded reminders map is not a HashMap<[ReviewReminderId], [ReviewReminder]>. + * @throws IllegalArgumentException If the decoded reminders map is not a [ReviewReminderGroup]. */ - private fun getRemindersForKey(key: String): HashMap { - val jsonString = remindersSharedPrefs.getString(key, null) ?: return hashMapOf() + private fun getRemindersForKey(key: String): ReviewReminderGroup { + val jsonString = remindersSharedPrefs.getString(key, null) ?: return ReviewReminderGroup() return decodeJson(jsonString, deckKeyForMigrationPurposes = key) } /** * Get the [ReviewReminder]s for a specific deck. * @throws SerializationException If the reminders map has not been stored in SharedPreferences as a valid JSON string. - * @throws IllegalArgumentException If the decoded reminders map is not a HashMap<[ReviewReminderId], [ReviewReminder]>. + * @throws IllegalArgumentException If the decoded reminders map is not a [ReviewReminderGroup]. */ - fun getRemindersForDeck(did: DeckId): HashMap = getRemindersForKey(DECK_SPECIFIC_KEY + did) + fun getRemindersForDeck(did: DeckId): ReviewReminderGroup = getRemindersForKey(DECK_SPECIFIC_KEY + did) /** * Get the app-wide [ReviewReminder]s. * @throws SerializationException If the reminders map has not been stored in SharedPreferences as a valid JSON string. - * @throws IllegalArgumentException If the decoded reminders map is not a HashMap<[ReviewReminderId], [ReviewReminder]>. + * @throws IllegalArgumentException If the decoded reminders map is not a [ReviewReminderGroup]. */ - fun getAllAppWideReminders(): HashMap = getRemindersForKey(APP_WIDE_KEY) + fun getAllAppWideReminders(): ReviewReminderGroup = getRemindersForKey(APP_WIDE_KEY) /** * Get all [ReviewReminder]s that are associated with a specific deck, all in a single flattened map. * @throws SerializationException If the reminders maps have not been stored in SharedPreferences as valid JSON strings. - * @throws IllegalArgumentException If the decoded reminders maps are not instances of HashMap<[ReviewReminderId], [ReviewReminder]>. + * @throws IllegalArgumentException If the decoded reminders maps are not instances of [ReviewReminderGroup]. */ - fun getAllDeckSpecificReminders(): HashMap = + fun getAllDeckSpecificReminders(): ReviewReminderGroup = remindersSharedPrefs .all .filterKeys { it.startsWith(DECK_SPECIFIC_KEY) } - .flatMap { (key, value) -> decodeJson(value.toString(), deckKeyForMigrationPurposes = key).entries } - .associateTo(hashMapOf()) { it.toPair() } + .map { (key, value) -> decodeJson(value.toString(), deckKeyForMigrationPurposes = key) } + .mergeAll() /** * Edit the [ReviewReminder]s for a specific key. Deletes the review reminders map for this key if, after the editing operation, * no review reminders remain. * @param key - * @param reminderEditor A lambda that takes the current map and returns the updated map. + * @param editor A lambda that takes the current map and returns the updated map. + * @param expectedScope The expected scope of all review reminders in the resulting map. * @throws SerializationException If the current reminders map has not been stored in SharedPreferences as a valid JSON string. - * @throws IllegalArgumentException If the decoded current reminders map is not a HashMap<[ReviewReminderId], [ReviewReminder]>. + * @throws IllegalArgumentException If the decoded current reminders map is not a [ReviewReminderGroup], + * or if the result of applying the [editor] contains review reminders of a scope other than [expectedScope]. */ private fun editRemindersForKey( key: String, - reminderEditor: (HashMap) -> Map, + editor: ReviewReminderGroupEditor, + expectedScope: ReviewReminderScope, ) { val existingReminders = getRemindersForKey(key) - val updatedReminders = reminderEditor(existingReminders) + val updatedReminders = editor(existingReminders) + + require(updatedReminders.getRemindersList().all { it.scope == expectedScope }) { + "Tried to write review reminders of an unexpected, incompatible scope to scope: $expectedScope" + } + remindersSharedPrefs.edit { if (updatedReminders.isEmpty()) { remove(key) @@ -294,160 +304,23 @@ object ReviewRemindersDatabase { * Edit the [ReviewReminder]s for a specific deck. * This assumes the resulting map contains only reminders of scope [ReviewReminderScope.DeckSpecific]. * @param did - * @param reminderEditor A lambda that takes the current map and returns the updated map. + * @param editor A lambda that takes the current map and returns the updated map. * @throws SerializationException If the current reminders map has not been stored in SharedPreferences as a valid JSON string. - * @throws IllegalArgumentException If the decoded current reminders map is not a HashMap<[ReviewReminderId], [ReviewReminder]>. + * @throws IllegalArgumentException If the decoded current reminders map is not a [ReviewReminderGroup], + * or if the result of applying the [editor] contains review reminders of a scope other than [ReviewReminderScope.DeckSpecific]. */ fun editRemindersForDeck( did: DeckId, - reminderEditor: (HashMap) -> Map, - ) = editRemindersForKey(DECK_SPECIFIC_KEY + did, reminderEditor) + editor: ReviewReminderGroupEditor, + ) = editRemindersForKey(DECK_SPECIFIC_KEY + did, editor, ReviewReminderScope.DeckSpecific(did)) /** * Edit the app-wide [ReviewReminder]s. * This assumes the resulting map contains only reminders of scope [ReviewReminderScope.Global]. - * @param reminderEditor A lambda that takes the current map and returns the updated map. + * @param editor A lambda that takes the current map and returns the updated map. * @throws SerializationException If the current reminders map has not been stored in SharedPreferences as a valid JSON string. - * @throws IllegalArgumentException If the decoded current reminders map is not a HashMap<[ReviewReminderId], [ReviewReminder]>. + * @throws IllegalArgumentException If the decoded current reminders map is not a [ReviewReminderGroup], + * or if the result of applying the [editor] contains review reminders of a scope other than [ReviewReminderScope.Global]. */ - fun editAllAppWideReminders(reminderEditor: (HashMap) -> Map) = - editRemindersForKey(APP_WIDE_KEY, reminderEditor) -} - -/** - * Inline value class for review reminder schema versions. - * @see [StoredReviewRemindersMap] - * @see [ReviewReminder] - */ -@JvmInline -@Serializable -value class ReviewReminderSchemaVersion( - val value: Int, -) { - init { - require(value >= 1) { "Review reminder schema version must be >= 1" } - // We do not check that it is <= SCHEMA_VERSION here because then declaring SCHEMA_VERSION would be circular - } -} - -/** - * When [ReviewReminder] is updated by a developer, implement this interface in a new data class which - * has the same fields as the old version of [ReviewReminder], then implement the [migrate] method which - * transforms old [ReviewReminder]s to new [ReviewReminder]s. Also ensure that the previous [ReviewReminderSchema] - * in the migration version chain ([ReviewRemindersDatabase.oldReviewReminderSchemasForMigration]) has its [migrate] method - * edited to return instances of the newly-created [ReviewReminderSchema]. Then, increment [ReviewRemindersDatabase.schemaVersion]. - * - * Data classes implementing this interface should be marked as @Serializable. Any new types defined for ReviewReminderSchemas - * should also be marked as @Serializable. - * - * @see [ReviewRemindersDatabase.performSchemaMigration]. - * @see [ReviewReminder] - */ -@Serializable -sealed interface ReviewReminderSchema { - /** - * All review reminders must have an identifying ID. - * This is necessary to facilitate migrations. See the implementation of [ReviewRemindersDatabase.performSchemaMigration] for details. - */ - val id: ReviewReminderId - - /** - * Transforms this [ReviewReminderSchema] to the next version of the [ReviewReminderSchema]. - */ - fun migrate(): ReviewReminderSchema -} - -/** - * Version 1 of [ReviewReminderSchema]. Updated to Version 2 by adding [ReviewReminder.onlyNotifyIfNoReviews]. - */ -@Serializable -data class ReviewReminderSchemaV1( - override val id: ReviewReminderId, - val time: ReviewReminderTime, - val cardTriggerThreshold: ReviewReminderCardTriggerThreshold, - val scope: ReviewReminderScope, - var enabled: Boolean, - val profileID: String, - val onlyNotifyIfNoReviews: Boolean = false, -) : ReviewReminderSchema { - override fun migrate(): ReviewReminderSchema = - ReviewReminder.createReviewReminder( - time = time, - cardTriggerThreshold = cardTriggerThreshold, - scope = scope, - enabled = enabled, - profileID = profileID, - onlyNotifyIfNoReviews = onlyNotifyIfNoReviews, - ) -} - -/** - * Schema migration settings for testing purposes. - * Consult this as an example of how to save old schemas and define their [ReviewReminderSchema.migrate] methods. - */ -object TestingReviewReminderMigrationSettings { - /** - * A sample old review reminder schema. Perhaps this was how the [ReviewReminder] data class was originally implemented. - * We would like to test the code that checks if review reminders stored on the device adhere to an old, outdated schema. - * In particular, does the code correctly migrate the serialized data class strings to the updated, current version of [ReviewReminder]? - */ - @Serializable - data class ReviewReminderTestSchemaVersionOne( - override val id: ReviewReminderId, - val hour: Int, - val minute: Int, - val cardTriggerThreshold: Int, - val did: DeckId, - val enabled: Boolean = true, - ) : ReviewReminderSchema { - override fun migrate(): ReviewReminderSchema = - ReviewReminderTestSchemaVersionTwo( - id = this.id, - time = VersionTwoDataClasses.ReviewReminderTime(hour, minute), - snoozeAmount = 1, - cardTriggerThreshold = this.cardTriggerThreshold, - did = this.did, - enabled = enabled, - ) - } - - /** - * Here's an example of how you can handle renamed fields in a data class stored as part of a [ReviewReminder]. - * Otherwise, there's a namespace collision with [ReviewReminderTime]. - * - * This class will be serialized into "ReviewReminderTime(timeHour=#, timeMinute=#)", which otherwise might conflict - * with the updated definition of [ReviewReminderTime], which is serialized as "ReviewReminderTime(hour=#, minute=#)". - * When we read the outdated schema from the disk, we need to tell the deserializer that it is reading a - * [VersionTwoDataClasses.ReviewReminderTime] rather than a [ReviewReminderTime], even though the names are the same. - * - * @see ReviewReminderTestSchemaVersionTwo - */ - object VersionTwoDataClasses { - @Serializable - data class ReviewReminderTime( - val timeHour: Int, - val timeMinute: Int, - ) - } - - /** - * Another example of an old review reminder schema. See [ReviewReminderTestSchemaVersionOne] for more details. - */ - @Serializable - data class ReviewReminderTestSchemaVersionTwo( - override val id: ReviewReminderId, - val time: VersionTwoDataClasses.ReviewReminderTime, - val snoozeAmount: Int, - val cardTriggerThreshold: Int, - val did: DeckId, - val enabled: Boolean = true, - ) : ReviewReminderSchema { - override fun migrate(): ReviewReminder = - ReviewReminder.createReviewReminder( - time = ReviewReminderTime(this.time.timeHour, this.time.timeMinute), - cardTriggerThreshold = ReviewReminderCardTriggerThreshold(this.cardTriggerThreshold), - scope = if (this.did == -1L) ReviewReminderScope.Global else ReviewReminderScope.DeckSpecific(this.did), - enabled = enabled, - ) - } + fun editAllAppWideReminders(editor: ReviewReminderGroupEditor) = editRemindersForKey(APP_WIDE_KEY, editor, ReviewReminderScope.Global) } diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt index 04e69dfdcd0e..0ec00556c72a 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt @@ -101,7 +101,7 @@ class ScheduleReminders : * use [triggerUIUpdate]. Note that editing this map does not also automatically write to the database. * Writing to the database must be done separately. */ - private lateinit var reminders: HashMap + private lateinit var reminders: ReviewReminderGroup /** * Retrieving deck names for a given deck ID in [retrieveDeckNameFromID] requires a call to the collection. @@ -188,11 +188,11 @@ class ScheduleReminders : catchDatabaseExceptions { when (val scope = scheduleRemindersScope) { is ReviewReminderScope.Global -> { - HashMap(ReviewRemindersDatabase.getAllAppWideReminders() + ReviewRemindersDatabase.getAllDeckSpecificReminders()) + ReviewRemindersDatabase.getAllAppWideReminders() + ReviewRemindersDatabase.getAllDeckSpecificReminders() } is ReviewReminderScope.DeckSpecific -> ReviewRemindersDatabase.getRemindersForDeck(scope.did) } - } ?: hashMapOf() + } ?: ReviewReminderGroup() triggerUIUpdate() Timber.d("Database review reminders successfully loaded") } @@ -267,9 +267,10 @@ class ScheduleReminders : * [ReviewRemindersDatabase.editAllAppWideReminders] which deletes the given review reminder. */ private fun deleteReminder(reminder: ReviewReminder) = - { reminders: HashMap -> - reminders.remove(reminder.id) - reminders + { reminders: ReviewReminderGroup -> + reminders.apply { + remove(reminder.id) + } } /** @@ -278,9 +279,22 @@ class ScheduleReminders : * exists or inserts it if it doesn't (an "upsert" operation) */ private fun upsertReminder(reminder: ReviewReminder) = - { reminders: HashMap -> - reminders[reminder.id] = reminder - reminders + { reminders: ReviewReminderGroup -> + reminders.apply { + this[reminder.id] = reminder + } + } + + /** + * Lambda that can be fed into [ReviewRemindersDatabase.editRemindersForDeck] or + * [ReviewRemindersDatabase.editAllAppWideReminders] which toggles whether the given review reminder + * is enabled. + */ + private fun toggleReminder(reminder: ReviewReminder) = + { reminders: ReviewReminderGroup -> + reminders.apply { + toggleEnabled(reminder.id) + } } /** @@ -368,19 +382,12 @@ class ScheduleReminders : val reminder = reminders[id] ?: return val newState = !reminder.enabled - val performToggle: - (HashMap) -> Map = - { reminders -> - reminders[id]?.enabled = newState - reminders - } - // Update database launchCatchingTask { catchDatabaseExceptions { when (scope) { - is ReviewReminderScope.Global -> ReviewRemindersDatabase.editAllAppWideReminders(performToggle) - is ReviewReminderScope.DeckSpecific -> ReviewRemindersDatabase.editRemindersForDeck(scope.did, performToggle) + is ReviewReminderScope.Global -> ReviewRemindersDatabase.editAllAppWideReminders(toggleReminder(reminder)) + is ReviewReminderScope.DeckSpecific -> ReviewRemindersDatabase.editRemindersForDeck(scope.did, toggleReminder(reminder)) } } } @@ -424,7 +431,7 @@ class ScheduleReminders : /** * [AddEditReminderDialog] requires a [DeckSelectionDialog.DeckSelectionListener] to catch changes to - * the [com.ichi2.anki.DeckSpinnerSelection]. However, [AddEditReminderDialog] is removed from the + * the [DeckSelectionDialog]. However, [AddEditReminderDialog] is removed from the * fragment stack when the [DeckSelectionDialog] appears, so we set [ScheduleReminders] as the listener * and forward data to [AddEditReminderDialog] when a deck is selected. */ @@ -445,7 +452,7 @@ class ScheduleReminders : private fun triggerUIUpdate() { val listToDisplay = reminders - .values + .getRemindersList() .sortedBy { it.time.toSecondsFromMidnight() } .toList() adapter.submitList(listToDisplay) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/services/AlarmManagerService.kt b/AnkiDroid/src/main/java/com/ichi2/anki/services/AlarmManagerService.kt index d8244de8060d..456e239580cd 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/services/AlarmManagerService.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/services/AlarmManagerService.kt @@ -224,7 +224,7 @@ class AlarmManagerService : BroadcastReceiver() { Timber.d("scheduleAllEnabledReviewReminderNotifications") val allReviewRemindersAsMap = ReviewRemindersDatabase.getAllAppWideReminders() + ReviewRemindersDatabase.getAllDeckSpecificReminders() - val enabledReviewReminders = allReviewRemindersAsMap.values.filter { it.enabled } + val enabledReviewReminders = allReviewRemindersAsMap.getRemindersList().filter { it.enabled } for (reviewReminder in enabledReviewReminders) { scheduleReviewReminderNotification(context, reviewReminder) } @@ -277,6 +277,7 @@ class AlarmManagerService : BroadcastReceiver() { * To extend the notifications created by AnkiDroid, add more functionality to the body of this method. */ fun scheduleAllNotifications(context: Context) { + // currently, the only scheduled notifications supported by AnkiDroid are review reminder notifications scheduleAllEnabledReviewReminderNotifications(context) } diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt index 174da14aa097..db4e076c96e8 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt @@ -28,7 +28,6 @@ import kotlinx.serialization.serializer import org.hamcrest.Description import org.hamcrest.Matcher import org.hamcrest.MatcherAssert.assertThat -import org.hamcrest.Matchers.anEmptyMap import org.hamcrest.Matchers.equalTo import org.hamcrest.Matchers.hasItem import org.hamcrest.Matchers.not @@ -49,8 +48,9 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { private val did1 = 12345L private val did2 = 67890L + private val emptyReminderGroup = ReviewReminderGroup() private val dummyDeckSpecificRemindersForDeckOne = - mapOf( + ReviewReminderGroup( ReviewReminderId(0) to ReviewReminder.createReviewReminder( ReviewReminderTime(9, 0), @@ -66,7 +66,7 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { ), ) private val dummyDeckSpecificRemindersForDeckTwo = - mapOf( + ReviewReminderGroup( ReviewReminderId(2) to ReviewReminder.createReviewReminder( ReviewReminderTime(10, 30), @@ -82,7 +82,7 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { ), ) private val dummyAppWideReminders = - mapOf( + ReviewReminderGroup( ReviewReminderId(4) to ReviewReminder.createReviewReminder( ReviewReminderTime(9, 0), @@ -108,9 +108,9 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { } @Test - fun `getRemindersForDeck should return empty map when no reminders exist`() { + fun `getRemindersForDeck should return empty group when no reminders exist`() { val reminders = ReviewRemindersDatabase.getRemindersForDeck(did1) - assertThat(reminders, anEmptyMap()) + assertThat(reminders, equalTo(emptyReminderGroup)) } @Test @@ -121,9 +121,9 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { } @Test - fun `getAllDeckSpecificReminders should return empty map when no reminders exist`() { + fun `getAllDeckSpecificReminders should return empty group when no reminders exist`() { val reminders = ReviewRemindersDatabase.getAllDeckSpecificReminders() - assertThat(reminders, anEmptyMap()) + assertThat(reminders, equalTo(emptyReminderGroup)) } @Test @@ -138,9 +138,9 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { } @Test - fun `getAllAppWideReminders should return empty map when no reminders exist`() { + fun `getAllAppWideReminders should return empty group when no reminders exist`() { val reminders = ReviewRemindersDatabase.getAllAppWideReminders() - assertThat(reminders, anEmptyMap()) + assertThat(reminders, equalTo(emptyReminderGroup)) } @Test @@ -151,7 +151,7 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { } @Test(expected = SerializationException::class) - fun `getRemindersForDeck should throw SerializationException if JSON string for StoredReviewReminder is corrupted`() { + fun `getRemindersForDeck should throw SerializationException if JSON string for StoredReviewReminderGroup is corrupted`() { ReviewRemindersDatabase.remindersSharedPrefs.edit { putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, "corrupted_and_invalid_json_string") } @@ -159,8 +159,8 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { } @Test(expected = IllegalArgumentException::class) - fun `getRemindersForDeck should throw IllegalArgumentException if JSON string is not a StoredReviewReminder`() { - val randomObject = Pair("not a map of", "review reminders") + fun `getRemindersForDeck should throw IllegalArgumentException if JSON string is not a StoredReviewReminderGroup`() { + val randomObject = Pair("not a group of", "review reminders") ReviewRemindersDatabase.remindersSharedPrefs.edit { putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, Json.encodeToString(randomObject)) } @@ -169,19 +169,19 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { @Test(expected = SerializationException::class) fun `getRemindersForDeck should throw SerializationException if JSON string for review reminder is corrupted`() { - val corruptedStoredReviewReminder = - ReviewRemindersDatabase.StoredReviewRemindersMap( + val corruptedStoredReviewReminderGroup = + ReviewRemindersDatabase.StoredReviewReminderGroup( ReviewRemindersDatabase.schemaVersion, "corrupted_and_invalid_json_string", ) ReviewRemindersDatabase.remindersSharedPrefs.edit { - putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, Json.encodeToString(corruptedStoredReviewReminder)) + putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, Json.encodeToString(corruptedStoredReviewReminderGroup)) } ReviewRemindersDatabase.getRemindersForDeck(did1) } @Test(expected = SerializationException::class) - fun `getAllAppWideReminders should throw SerializationException if JSON string for StoredReviewReminder is corrupted`() { + fun `getAllAppWideReminders should throw SerializationException if JSON string for StoredReviewReminderGroup is corrupted`() { ReviewRemindersDatabase.remindersSharedPrefs.edit { putString(ReviewRemindersDatabase.APP_WIDE_KEY, "corrupted_and_invalid_json_string") } @@ -189,8 +189,8 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { } @Test(expected = IllegalArgumentException::class) - fun `getAllAppWideReminders should throw IllegalArgumentException if JSON string is not a StoredReviewReminder`() { - val randomObject = Pair("not a map of", "review reminders") + fun `getAllAppWideReminders should throw IllegalArgumentException if JSON string is not a StoredReviewReminderGroup`() { + val randomObject = Pair("not a group of", "review reminders") ReviewRemindersDatabase.remindersSharedPrefs.edit { putString(ReviewRemindersDatabase.APP_WIDE_KEY, Json.encodeToString(randomObject)) } @@ -199,19 +199,19 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { @Test(expected = SerializationException::class) fun `getAllAppWideReminders should throw SerializationException if JSON string for review reminder is corrupted`() { - val corruptedStoredReviewReminder = - ReviewRemindersDatabase.StoredReviewRemindersMap( + val corruptedStoredReviewReminderGroup = + ReviewRemindersDatabase.StoredReviewReminderGroup( ReviewRemindersDatabase.schemaVersion, "corrupted_and_invalid_json_string", ) ReviewRemindersDatabase.remindersSharedPrefs.edit { - putString(ReviewRemindersDatabase.APP_WIDE_KEY, Json.encodeToString(corruptedStoredReviewReminder)) + putString(ReviewRemindersDatabase.APP_WIDE_KEY, Json.encodeToString(corruptedStoredReviewReminderGroup)) } ReviewRemindersDatabase.getAllAppWideReminders() } @Test(expected = SerializationException::class) - fun `getAllDeckSpecificReminders should throw SerializationException if JSON string for StoredReviewReminder is corrupted`() { + fun `getAllDeckSpecificReminders should throw SerializationException if JSON string for StoredReviewReminderGroup is corrupted`() { ReviewRemindersDatabase.remindersSharedPrefs.edit { putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, "corrupted_and_invalid_json_string") } @@ -219,8 +219,8 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { } @Test(expected = IllegalArgumentException::class) - fun `getAllDeckSpecificReminders should throw IllegalArgumentException if JSON string is not a StoredReviewReminder`() { - val randomObject = Pair("not a map of", "review reminders") + fun `getAllDeckSpecificReminders should throw IllegalArgumentException if JSON string is not a StoredReviewReminderGroup`() { + val randomObject = Pair("not a group of", "review reminders") ReviewRemindersDatabase.remindersSharedPrefs.edit { putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, Json.encodeToString(randomObject)) } @@ -229,13 +229,13 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { @Test(expected = SerializationException::class) fun `getAllDeckSpecificReminders should throw SerializationException if JSON string for review reminder is corrupted`() { - val corruptedStoredReviewReminder = - ReviewRemindersDatabase.StoredReviewRemindersMap( + val corruptedStoredReviewReminderGroup = + ReviewRemindersDatabase.StoredReviewReminderGroup( ReviewRemindersDatabase.schemaVersion, "corrupted_and_invalid_json_string", ) ReviewRemindersDatabase.remindersSharedPrefs.edit { - putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, Json.encodeToString(corruptedStoredReviewReminder)) + putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, Json.encodeToString(corruptedStoredReviewReminderGroup)) } ReviewRemindersDatabase.getAllDeckSpecificReminders() } @@ -243,9 +243,9 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { @Test fun `editRemindersForDeck should delete SharedPreferences key if no reminders are returned`() { ReviewRemindersDatabase.editRemindersForDeck(did1) { dummyDeckSpecificRemindersForDeckOne } - ReviewRemindersDatabase.editRemindersForDeck(did1) { emptyMap() } + ReviewRemindersDatabase.editRemindersForDeck(did1) { ReviewReminderGroup() } val attemptedRetrieval = ReviewRemindersDatabase.getRemindersForDeck(did1) - assertThat(attemptedRetrieval, anEmptyMap()) + assertThat(attemptedRetrieval, equalTo(emptyReminderGroup)) assertThat( ReviewRemindersDatabase.remindersSharedPrefs.all.keys, not(hasItem(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1)), @@ -255,15 +255,30 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { @Test fun `editAllAppWideReminders should delete SharedPreferences key if no reminders are returned`() { ReviewRemindersDatabase.editAllAppWideReminders { dummyAppWideReminders } - ReviewRemindersDatabase.editAllAppWideReminders { emptyMap() } + ReviewRemindersDatabase.editAllAppWideReminders { ReviewReminderGroup() } val attemptedRetrieval = ReviewRemindersDatabase.getAllAppWideReminders() - assertThat(attemptedRetrieval, anEmptyMap()) + assertThat(attemptedRetrieval, equalTo(emptyReminderGroup)) assertThat( ReviewRemindersDatabase.remindersSharedPrefs.all.keys, not(hasItem(ReviewRemindersDatabase.APP_WIDE_KEY)), ) } + @Test(expected = IllegalArgumentException::class) + fun `editAllAppWideReminders with deck specific reminders should throw IllegalArgumentException`() { + ReviewRemindersDatabase.editAllAppWideReminders { dummyDeckSpecificRemindersForDeckOne } + } + + @Test(expected = IllegalArgumentException::class) + fun `editRemindersForDeck with app wide reminders should throw IllegalArgumentException`() { + ReviewRemindersDatabase.editRemindersForDeck(did1) { dummyAppWideReminders } + } + + @Test(expected = IllegalArgumentException::class) + fun `editRemindersForDeck with reminders for different deck should throw IllegalArgumentException`() { + ReviewRemindersDatabase.editRemindersForDeck(did1) { dummyDeckSpecificRemindersForDeckTwo } + } + /** * When review reminders are migrated to the new schema, the reminders' IDs will be recreated from scratch. * Thus, validation that our tests succeeded should ignore [ReviewReminder.id]. @@ -347,9 +362,9 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { } """.trimIndent() - val storedReviewRemindersMap = Json.decodeFromString(rawString) + val storedReviewReminderGroup = Json.decodeFromString(rawString) val mapSerializer = MapSerializer(ReviewReminderId.serializer(), ReviewReminder.serializer()) - Json.decodeFromString(mapSerializer, storedReviewRemindersMap.remindersMapJson) + Json.decodeFromString(mapSerializer, storedReviewReminderGroup.remindersMapJson) } /** @@ -424,7 +439,7 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { val inputMap = casesInScope.associate { it.input.id to it.input } val packagedInput = - ReviewRemindersDatabase.StoredReviewRemindersMap( + ReviewRemindersDatabase.StoredReviewReminderGroup( version, Json.encodeToString(mapSerializer, inputMap), ) @@ -452,11 +467,11 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { // We ignore ID because the migration process will generate new review reminders from scratch during the migration // ID is a private, inaccessible property // Instead, we only check that the ID matches the key in the map; all other properties can be compared normally - retrievedReminders.forEach { (id, reminder) -> + retrievedReminders.forEach { id, reminder -> assertThat(id, equalTo(reminder.id)) } assertThat( - retrievedReminders.values, + retrievedReminders.getRemindersList(), containsEqualReviewRemindersInAnyOrderIgnoringId( casesInScope.map { it.expectedOutput }, ), diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/services/AlarmManagerServiceTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/services/AlarmManagerServiceTest.kt index 4e5bd96d08e8..200ce394fd71 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/services/AlarmManagerServiceTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/services/AlarmManagerServiceTest.kt @@ -31,7 +31,9 @@ import com.ichi2.anki.RobolectricTest import com.ichi2.anki.common.time.MockTime import com.ichi2.anki.common.time.TimeManager import com.ichi2.anki.reviewreminders.ReviewReminder +import com.ichi2.anki.reviewreminders.ReviewReminderGroup import com.ichi2.anki.reviewreminders.ReviewReminderId +import com.ichi2.anki.reviewreminders.ReviewReminderScope import com.ichi2.anki.reviewreminders.ReviewReminderTime import com.ichi2.anki.reviewreminders.ReviewRemindersDatabase import io.mockk.every @@ -138,18 +140,26 @@ class AlarmManagerServiceTest : RobolectricTest() { runTest { val did1 = addDeck("Deck1") val did2 = addDeck("Deck2") - val reminder1 = ReviewReminder.createReviewReminder(time = ReviewReminderTime(9, 0)) - val reminder2 = ReviewReminder.createReviewReminder(time = ReviewReminderTime(10, 0)) + val reminder1 = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(9, 0), + scope = ReviewReminderScope.DeckSpecific(did1), + ) + val reminder2 = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(10, 0), + scope = ReviewReminderScope.DeckSpecific(did2), + ) val reminder3 = ReviewReminder.createReviewReminder(time = ReviewReminderTime(11, 0)) val disabledReminder = ReviewReminder.createReviewReminder( time = ReviewReminderTime(11, 0), enabled = false, ) - ReviewRemindersDatabase.editRemindersForDeck(did1) { mapOf(ReviewReminderId(0) to reminder1) } - ReviewRemindersDatabase.editRemindersForDeck(did2) { mapOf(ReviewReminderId(1) to reminder2) } + ReviewRemindersDatabase.editRemindersForDeck(did1) { ReviewReminderGroup(ReviewReminderId(0) to reminder1) } + ReviewRemindersDatabase.editRemindersForDeck(did2) { ReviewReminderGroup(ReviewReminderId(1) to reminder2) } ReviewRemindersDatabase.editAllAppWideReminders { - mapOf( + ReviewReminderGroup( ReviewReminderId(2) to reminder3, ReviewReminderId(3) to disabledReminder, ) diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt index f0791f601f1f..8ca44c23ab7c 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt @@ -30,6 +30,7 @@ import com.ichi2.anki.common.time.TimeManager import com.ichi2.anki.libanki.DeckId import com.ichi2.anki.reviewreminders.ReviewReminder import com.ichi2.anki.reviewreminders.ReviewReminderCardTriggerThreshold +import com.ichi2.anki.reviewreminders.ReviewReminderGroup import com.ichi2.anki.reviewreminders.ReviewReminderId import com.ichi2.anki.reviewreminders.ReviewReminderScope import com.ichi2.anki.reviewreminders.ReviewReminderTime @@ -82,13 +83,13 @@ class NotificationServiceTest : RobolectricTest() { private fun createAndSaveDummyDeckSpecificReminder(did: DeckId): ReviewReminder { val reviewReminder = createTestReminder(deckId = did, thresholdInt = 1) - ReviewRemindersDatabase.editRemindersForDeck(did) { mapOf(ReviewReminderId(0) to reviewReminder) } + ReviewRemindersDatabase.editRemindersForDeck(did) { ReviewReminderGroup(ReviewReminderId(0) to reviewReminder) } return reviewReminder } private fun createAndSaveDummyAppWideReminder(): ReviewReminder { val reviewReminder = createTestReminder(thresholdInt = 1) - ReviewRemindersDatabase.editAllAppWideReminders { mapOf(ReviewReminderId(1) to reviewReminder) } + ReviewRemindersDatabase.editAllAppWideReminders { ReviewReminderGroup(ReviewReminderId(1) to reviewReminder) } return reviewReminder } @@ -111,8 +112,8 @@ class NotificationServiceTest : RobolectricTest() { } val reviewReminderDeckSpecific = createTestReminder(deckId = did1, thresholdInt = 3) val reviewReminderAppWide = createTestReminder(thresholdInt = 3) - ReviewRemindersDatabase.editRemindersForDeck(did1) { mapOf(ReviewReminderId(0) to reviewReminderDeckSpecific) } - ReviewRemindersDatabase.editAllAppWideReminders { mapOf(ReviewReminderId(1) to reviewReminderAppWide) } + ReviewRemindersDatabase.editRemindersForDeck(did1) { ReviewReminderGroup(ReviewReminderId(0) to reviewReminderDeckSpecific) } + ReviewRemindersDatabase.editAllAppWideReminders { ReviewReminderGroup(ReviewReminderId(1) to reviewReminderAppWide) } triggerDummyReminderNotification(reviewReminderDeckSpecific) triggerDummyReminderNotification(reviewReminderAppWide) @@ -191,8 +192,8 @@ class NotificationServiceTest : RobolectricTest() { col.sched.answerCard(col.sched.card!!, CardAnswer.Rating.GOOD) val reviewReminderDeckSpecific = createTestReminder(deckId = did1, thresholdInt = 1, onlyNotifyIfNoReviews = true) val reviewReminderAppWide = createTestReminder(thresholdInt = 1, onlyNotifyIfNoReviews = true) - ReviewRemindersDatabase.editRemindersForDeck(did1) { mapOf(ReviewReminderId(0) to reviewReminderDeckSpecific) } - ReviewRemindersDatabase.editAllAppWideReminders { mapOf(ReviewReminderId(1) to reviewReminderAppWide) } + ReviewRemindersDatabase.editRemindersForDeck(did1) { ReviewReminderGroup(ReviewReminderId(0) to reviewReminderDeckSpecific) } + ReviewRemindersDatabase.editAllAppWideReminders { ReviewReminderGroup(ReviewReminderId(1) to reviewReminderAppWide) } triggerDummyReminderNotification(reviewReminderDeckSpecific) triggerDummyReminderNotification(reviewReminderAppWide) @@ -208,8 +209,8 @@ class NotificationServiceTest : RobolectricTest() { } val reviewReminderDeckSpecific = createTestReminder(deckId = did1, thresholdInt = 1, onlyNotifyIfNoReviews = true) val reviewReminderAppWide = createTestReminder(thresholdInt = 1, onlyNotifyIfNoReviews = true) - ReviewRemindersDatabase.editRemindersForDeck(did1) { mapOf(ReviewReminderId(0) to reviewReminderDeckSpecific) } - ReviewRemindersDatabase.editAllAppWideReminders { mapOf(ReviewReminderId(1) to reviewReminderAppWide) } + ReviewRemindersDatabase.editRemindersForDeck(did1) { ReviewReminderGroup(ReviewReminderId(0) to reviewReminderDeckSpecific) } + ReviewRemindersDatabase.editAllAppWideReminders { ReviewReminderGroup(ReviewReminderId(1) to reviewReminderAppWide) } triggerDummyReminderNotification(reviewReminderDeckSpecific) triggerDummyReminderNotification(reviewReminderAppWide) @@ -240,8 +241,8 @@ class NotificationServiceTest : RobolectricTest() { val reviewReminderDeckSpecific = createTestReminder(deckId = did1, thresholdInt = 1, onlyNotifyIfNoReviews = true) val reviewReminderAppWide = createTestReminder(thresholdInt = 1, onlyNotifyIfNoReviews = true) - ReviewRemindersDatabase.editRemindersForDeck(did1) { mapOf(ReviewReminderId(0) to reviewReminderDeckSpecific) } - ReviewRemindersDatabase.editAllAppWideReminders { mapOf(ReviewReminderId(1) to reviewReminderAppWide) } + ReviewRemindersDatabase.editRemindersForDeck(did1) { ReviewReminderGroup(ReviewReminderId(0) to reviewReminderDeckSpecific) } + ReviewRemindersDatabase.editAllAppWideReminders { ReviewReminderGroup(ReviewReminderId(1) to reviewReminderAppWide) } triggerDummyReminderNotification(reviewReminderDeckSpecific) triggerDummyReminderNotification(reviewReminderAppWide) @@ -365,8 +366,8 @@ class NotificationServiceTest : RobolectricTest() { } val reviewReminderOne = createTestReminder(deckId = did1, thresholdInt = 1) val reviewReminderTwo = createTestReminder(deckId = did2, thresholdInt = 1) - ReviewRemindersDatabase.editRemindersForDeck(did1) { mapOf(ReviewReminderId(0) to reviewReminderOne) } - ReviewRemindersDatabase.editRemindersForDeck(did2) { mapOf(ReviewReminderId(1) to reviewReminderTwo) } + ReviewRemindersDatabase.editRemindersForDeck(did1) { ReviewReminderGroup(ReviewReminderId(0) to reviewReminderOne) } + ReviewRemindersDatabase.editRemindersForDeck(did2) { ReviewReminderGroup(ReviewReminderId(1) to reviewReminderTwo) } val slotOne = slot() val slotTwo = slot() From 80bf2f06e0b62dd806138c2c9a5476193be403cb Mon Sep 17 00:00:00 2001 From: Song Eric Yu Li Date: Sun, 1 Feb 2026 22:12:02 -0800 Subject: [PATCH 3/6] feat(reminders): deserialization error preference A new SharedPreference. Review reminders are deserialized and have their alarms scheduled when the device starts, but if the deserialization process fails and no valid migrations are available, the error can be put into this string so that the next time the user opens the app, an error dialog can be shown to inform them of the issue. --- AnkiDroid/src/main/java/com/ichi2/anki/settings/Prefs.kt | 8 ++++++++ AnkiDroid/src/main/res/values/preferences.xml | 1 + 2 files changed, 9 insertions(+) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/settings/Prefs.kt b/AnkiDroid/src/main/java/com/ichi2/anki/settings/Prefs.kt index 016e4c14f08d..2e7273d4a7d7 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/settings/Prefs.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/settings/Prefs.kt @@ -296,6 +296,14 @@ open class PrefsRepository( */ var reminderNotifsRequestShown by booleanPref(R.string.reminder_notifs_request_shown_key, defaultValue = false) + /** + * A list of all recent deserialization errors that have occurred when trying to load review reminders from storage. + * For example, review reminders are deserialized and have their alarms scheduled when the device starts, but + * if the deserialization process fails and no valid migrations are available, the error can be put into this string + * so that the next time the user opens the app, an error dialog can be shown to inform them of the issue. + */ + var reviewReminderDeserializationErrors by stringPref(R.string.review_reminder_deserialization_errors_key) + // *************************************** Permissions ************************************** // // Flags for whether the system UI dialog for requesting certain permissions has been shown before. diff --git a/AnkiDroid/src/main/res/values/preferences.xml b/AnkiDroid/src/main/res/values/preferences.xml index b3696cade493..fc202b5bf026 100644 --- a/AnkiDroid/src/main/res/values/preferences.xml +++ b/AnkiDroid/src/main/res/values/preferences.xml @@ -213,6 +213,7 @@ reviewRemindersScreen reviewRemindersNextFreeId reminderNotifsRequestShown + reviewReminderDeserializationErrors notificationsPermissionRequested recordAudioPermissionRequested From 16e85ab42db4f1f82a099663d2b5c520e00fb6af Mon Sep 17 00:00:00 2001 From: Song Eric Yu Li Date: Thu, 5 Feb 2026 22:53:42 -0800 Subject: [PATCH 4/6] feat(reminders): handle review reminder deserialization errors gracefully Previously, if for some reason the review reminder system failed to deserialize or migrate review reminders, it would block the entire app from working due to trying to set review reminder alarms on launch. Obviously, a small side feature like review reminders should not block the entire app's function. This change substantially cleans up ReviewRemindersDatabase. If a deserialization error happens and no migration works, the offending ReviewReminderGroup is deleted, an error message is stored to SharedPreferences, and a crash report is sent. Then, the next time the app is opened and DeckPicker is opened, all accumulated error messages up to that point are shown and then cleared from SharedPreferences. We need to store the error messages and show them later on because it's possible for a review reminder database access to happen on device boot, not just when the app is open. If a deserialization error happens on ScheduleReminders, a wrapper function immediately shows the error message. Updates and streamlines tests for deserialization errors. --- .../main/java/com/ichi2/anki/DeckPicker.kt | 5 + .../ReviewRemindersDatabase.kt | 124 ++++++++---- .../anki/reviewreminders/ScheduleReminders.kt | 27 +-- .../ReviewRemindersDatabaseTest.kt | 177 +++++++++++------- 4 files changed, 211 insertions(+), 122 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt index 90a655016d64..c86f5e853ff0 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt @@ -158,6 +158,7 @@ import com.ichi2.anki.preferences.AdvancedSettingsFragment import com.ichi2.anki.preferences.PreferencesActivity import com.ichi2.anki.preferences.sharedPrefs import com.ichi2.anki.receiver.SdCardReceiver +import com.ichi2.anki.reviewreminders.ReviewRemindersDatabase import com.ichi2.anki.servicelayer.PreferenceUpgradeService import com.ichi2.anki.servicelayer.ScopedStorageService import com.ichi2.anki.settings.Prefs @@ -600,6 +601,10 @@ open class DeckPicker : checkWebviewVersion(this) + // If a review reminder deserialization error has recently occurred + // (ex. on app boot, when the app opened, etc.), inform the user via a dialog + ReviewRemindersDatabase.checkDeserializationErrors(this) + setFragmentResultListener(REQUEST_KEY) { _, bundle -> when (CustomStudyAction.fromBundle(bundle)) { CustomStudyAction.CUSTOM_STUDY_SESSION -> { diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt index dde1e7a46b37..f1009972b67f 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt @@ -21,7 +21,10 @@ import android.content.SharedPreferences import androidx.annotation.VisibleForTesting import androidx.core.content.edit import com.ichi2.anki.AnkiDroidApp +import com.ichi2.anki.CrashReportService import com.ichi2.anki.libanki.DeckId +import com.ichi2.anki.settings.Prefs +import com.ichi2.anki.showError import kotlinx.serialization.InternalSerializationApi import kotlinx.serialization.Serializable import kotlinx.serialization.SerializationException @@ -203,31 +206,55 @@ object ReviewRemindersDatabase { /** * Decode an encoded [ReviewReminderGroup] JSON string which has been stored as a [StoredReviewReminderGroup]. - * @see Json.decodeFromString - * @throws SerializationException If the stored string is not a valid JSON string. - * @throws IllegalArgumentException If the decoded reminders map is not a [ReviewReminderGroup], + * Deletes the stored review reminders and returns an empty [ReviewReminderGroup] if deserialization fails * and no valid schema migrations exist. + * + * @see Json.decodeFromString */ private fun decodeJson( jsonString: String, - deckKeyForMigrationPurposes: String, + jsonStringKey: String, ): ReviewReminderGroup { Timber.v("Decoding review reminders JSON string: $jsonString") - val storedReviewReminderGroup = Json.decodeFromString(jsonString) - return if (storedReviewReminderGroup.version.value != schemaVersion.value) { - performSchemaMigration( - deckKeyForMigrationPurposes, - storedReviewReminderGroup.remindersMapJson, - storedReviewReminderGroup.version, - schemaVersion, - ) - } else { - ReviewReminderGroup(storedReviewReminderGroup.remindersMapJson) + try { + val storedReviewReminderGroup = Json.decodeFromString(jsonString) + return if (storedReviewReminderGroup.version.value != schemaVersion.value) { + performSchemaMigration( + jsonStringKey, + storedReviewReminderGroup.remindersMapJson, + storedReviewReminderGroup.version, + schemaVersion, + ) + } else { + ReviewReminderGroup(storedReviewReminderGroup.remindersMapJson) + } + } catch (e: Exception) { + when (e) { + is SerializationException, + is IllegalArgumentException, + -> { + // Log error, it will be displayed to the user either immediately if the app is open or when they next open the app if not + val errorString = "Encountered (${e.message}) while parsing $jsonString" + Prefs.reviewReminderDeserializationErrors = Prefs.reviewReminderDeserializationErrors.orEmpty() + "[$errorString]" + Timber.e(e, errorString) + CrashReportService.sendExceptionReport( + e, + origin = "ReviewRemindersDatabase:decodeJson", + additionalInfo = jsonString, + ) + + // Delete corrupted value, then return an empty group + remindersSharedPrefs.edit { remove(jsonStringKey) } + return ReviewReminderGroup() + } + else -> throw e + } } } /** * Encode a [ReviewReminderGroup] as a [StoredReviewReminderGroup] JSON string. + * * @see Json.encodeToString */ private fun encodeJson(reminders: ReviewReminderGroup): String = @@ -235,49 +262,51 @@ object ReviewRemindersDatabase { /** * Get the [ReviewReminder]s for a specific key. - * @throws SerializationException If the value associated with this key is not valid JSON string. - * @throws IllegalArgumentException If the decoded reminders map is not a [ReviewReminderGroup]. + * Deletes the stored review reminders and returns an empty [ReviewReminderGroup] if deserialization fails + * and no valid schema migrations exist. */ private fun getRemindersForKey(key: String): ReviewReminderGroup { val jsonString = remindersSharedPrefs.getString(key, null) ?: return ReviewReminderGroup() - return decodeJson(jsonString, deckKeyForMigrationPurposes = key) + return decodeJson(jsonString, jsonStringKey = key) } /** * Get the [ReviewReminder]s for a specific deck. - * @throws SerializationException If the reminders map has not been stored in SharedPreferences as a valid JSON string. - * @throws IllegalArgumentException If the decoded reminders map is not a [ReviewReminderGroup]. + * Deletes the stored review reminders and returns an empty [ReviewReminderGroup] if deserialization fails + * and no valid schema migrations exist. */ fun getRemindersForDeck(did: DeckId): ReviewReminderGroup = getRemindersForKey(DECK_SPECIFIC_KEY + did) /** * Get the app-wide [ReviewReminder]s. - * @throws SerializationException If the reminders map has not been stored in SharedPreferences as a valid JSON string. - * @throws IllegalArgumentException If the decoded reminders map is not a [ReviewReminderGroup]. + * Deletes the stored review reminders and returns an empty [ReviewReminderGroup] if deserialization fails + * and no valid schema migrations exist. */ fun getAllAppWideReminders(): ReviewReminderGroup = getRemindersForKey(APP_WIDE_KEY) /** * Get all [ReviewReminder]s that are associated with a specific deck, all in a single flattened map. - * @throws SerializationException If the reminders maps have not been stored in SharedPreferences as valid JSON strings. - * @throws IllegalArgumentException If the decoded reminders maps are not instances of [ReviewReminderGroup]. + * If the stored review reminders for any specific deck fail to deserialize and no valid schema migrations exist, + * those reminders are deleted and not included in the resulting [ReviewReminderGroup]. */ fun getAllDeckSpecificReminders(): ReviewReminderGroup = remindersSharedPrefs .all .filterKeys { it.startsWith(DECK_SPECIFIC_KEY) } - .map { (key, value) -> decodeJson(value.toString(), deckKeyForMigrationPurposes = key) } + .map { (key, value) -> decodeJson(value.toString(), jsonStringKey = key) } .mergeAll() /** - * Edit the [ReviewReminder]s for a specific key. Deletes the review reminders map for this key if, after the editing operation, - * no review reminders remain. + * Edit the [ReviewReminder]s for a specific key. + * Deletes the review reminders map for this key if, after the editing operation, no review reminders remain. + * If the stored review reminders for any specific deck fail to deserialize and no valid schema migrations exist, + * those reminders are deleted and not included in the resulting [ReviewReminderGroup]. + * * @param key * @param editor A lambda that takes the current map and returns the updated map. * @param expectedScope The expected scope of all review reminders in the resulting map. - * @throws SerializationException If the current reminders map has not been stored in SharedPreferences as a valid JSON string. - * @throws IllegalArgumentException If the decoded current reminders map is not a [ReviewReminderGroup], - * or if the result of applying the [editor] contains review reminders of a scope other than [expectedScope]. + * + * @throws IllegalArgumentException If the result of applying the [editor] contains review reminders of a scope other than [expectedScope]. */ private fun editRemindersForKey( key: String, @@ -302,12 +331,12 @@ object ReviewRemindersDatabase { /** * Edit the [ReviewReminder]s for a specific deck. - * This assumes the resulting map contains only reminders of scope [ReviewReminderScope.DeckSpecific]. + * * @param did * @param editor A lambda that takes the current map and returns the updated map. - * @throws SerializationException If the current reminders map has not been stored in SharedPreferences as a valid JSON string. - * @throws IllegalArgumentException If the decoded current reminders map is not a [ReviewReminderGroup], - * or if the result of applying the [editor] contains review reminders of a scope other than [ReviewReminderScope.DeckSpecific]. + * + * @throws IllegalArgumentException If the result of applying the [editor] contains review reminders + * of a scope other than [ReviewReminderScope.DeckSpecific]. */ fun editRemindersForDeck( did: DeckId, @@ -316,11 +345,32 @@ object ReviewRemindersDatabase { /** * Edit the app-wide [ReviewReminder]s. - * This assumes the resulting map contains only reminders of scope [ReviewReminderScope.Global]. + * * @param editor A lambda that takes the current map and returns the updated map. - * @throws SerializationException If the current reminders map has not been stored in SharedPreferences as a valid JSON string. - * @throws IllegalArgumentException If the decoded current reminders map is not a [ReviewReminderGroup], - * or if the result of applying the [editor] contains review reminders of a scope other than [ReviewReminderScope.Global]. + * + * @throws IllegalArgumentException If the result of applying the [editor] contains review reminders + * of a scope other than [ReviewReminderScope.Global]. */ fun editAllAppWideReminders(editor: ReviewReminderGroupEditor) = editRemindersForKey(APP_WIDE_KEY, editor, ReviewReminderScope.Global) + + /** + * Shows an error message dialog if a review reminder deserialization error has recently happened. + * Checks if a deserialization error has recently occurred by checking if anything is present in + * [Prefs.reviewReminderDeserializationErrors], emptying the preference after reading it. + * + * @param context A valid themed context (ie. not applicationContext) to display the error dialog in. + */ + fun checkDeserializationErrors(context: Context) { + Prefs.reviewReminderDeserializationErrors?.let { errorString -> + if (errorString.isNotEmpty()) { + context.showError( + message = + "An error occurred while loading your review reminders, corrupted reminders have been deleted. " + + "Details:\n\n$errorString", + crashReportData = null, // Crash reports are sent when the error is first encountered + ) + Prefs.reviewReminderDeserializationErrors = "" + } + } + } } diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt index 0ec00556c72a..d0a11af5b044 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt @@ -34,7 +34,6 @@ import androidx.recyclerview.widget.DividerItemDecoration import androidx.recyclerview.widget.LinearLayoutManager import com.google.android.material.snackbar.Snackbar import com.ichi2.anki.CollectionManager.withCol -import com.ichi2.anki.CrashReportData.Companion.toCrashReportData import com.ichi2.anki.R import com.ichi2.anki.SingleFragmentActivity import com.ichi2.anki.canUserAccessDeck @@ -45,7 +44,6 @@ import com.ichi2.anki.libanki.DeckId import com.ichi2.anki.model.SelectableDeck import com.ichi2.anki.services.AlarmManagerService import com.ichi2.anki.settings.Prefs -import com.ichi2.anki.showError import com.ichi2.anki.snackbar.BaseSnackbarBuilderProvider import com.ichi2.anki.snackbar.SnackbarBuilder import com.ichi2.anki.snackbar.showSnackbar @@ -54,7 +52,6 @@ import com.ichi2.anki.withProgress import com.ichi2.utils.Permissions import com.ichi2.utils.Permissions.requestPermissionThroughDialogOrSettings import dev.androidbroadcast.vbpd.viewBinding -import kotlinx.serialization.SerializationException import timber.log.Timber /** @@ -518,28 +515,14 @@ class ScheduleReminders : */ const val DECK_SELECTION_RESULT_REQUEST_KEY = "reminder_deck_selection_result_request_key" - private const val SERIALIZATION_ERROR_MESSAGE = - "Something went wrong. A serialization error was encountered while working with review reminders." - private const val DATA_TYPE_ERROR_MESSAGE = - "Something went wrong. An unexpected data type was found while working with review reminders." - /** - * Wrapper for database access. - * Shows an error dialog if [SerializationException]s or [IllegalArgumentException]s are thrown. + * Wrapper for database access in this fragment. + * Shows an error dialog via [ReviewRemindersDatabase.checkDeserializationErrors] if there are deserialization errors. * Shows a progress dialog if database access takes a long time. */ - private suspend fun Fragment.catchDatabaseExceptions(block: () -> T): T? = - try { - Timber.d("Attempting ReviewRemindersDatabase operation") - withProgress { block() } - } catch (e: SerializationException) { - Timber.e("JSON Serialization error occurred") - requireContext().showError("$SERIALIZATION_ERROR_MESSAGE: $e", e.toCrashReportData(requireContext())) - null - } catch (e: IllegalArgumentException) { - Timber.e("JSON Illegal argument exception occurred") - requireContext().showError("$DATA_TYPE_ERROR_MESSAGE: $e", e.toCrashReportData(requireContext())) - null + private suspend fun Fragment.catchDatabaseExceptions(block: suspend () -> T): T? = + withProgress { block() }.also { + ReviewRemindersDatabase.checkDeserializationErrors(requireContext()) } /** diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt index db4e076c96e8..d529dedf06c6 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt @@ -21,7 +21,6 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import com.ichi2.anki.RobolectricTest import kotlinx.serialization.InternalSerializationApi import kotlinx.serialization.KSerializer -import kotlinx.serialization.SerializationException import kotlinx.serialization.builtins.MapSerializer import kotlinx.serialization.json.Json import kotlinx.serialization.serializer @@ -150,94 +149,146 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { assertThat(storedReminders, equalTo(dummyAppWideReminders)) } - @Test(expected = SerializationException::class) - fun `getRemindersForDeck should throw SerializationException if JSON string for StoredReviewReminderGroup is corrupted`() { + /** + * Helper function to test how the database handles corrupted JSON strings. + * It should delete only the accessed ones, not throw an exception, and return an empty reminder group. + * + * @param corruptedValue the corrupted JSON string to be inserted into SharedPreferences for did1, did2, and the app-wide key + * @param expectedDeletedKeys the set of keys that should no longer be in SharedPreferences after the access + * @param access a lambda that accesses either deck-specific or app-wide reminders, which should trigger the deletion of the corrupted keys + */ + private fun corruptedRemindersTest( + corruptedValue: String, + expectedDeletedKeys: Set, + inputKeys: Set = + setOf( + ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, + ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did2, + ReviewRemindersDatabase.APP_WIDE_KEY, + ), + access: () -> ReviewReminderGroup, + ) { ReviewRemindersDatabase.remindersSharedPrefs.edit { - putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, "corrupted_and_invalid_json_string") + inputKeys.forEach { key -> + putString(key, corruptedValue) + } } - ReviewRemindersDatabase.getRemindersForDeck(did1) + val reminders = access() + assertThat(reminders, equalTo(emptyReminderGroup)) + val remainingKeys = ReviewRemindersDatabase.remindersSharedPrefs.all.keys + assertThat(remainingKeys + expectedDeletedKeys, equalTo(inputKeys)) } - @Test(expected = IllegalArgumentException::class) - fun `getRemindersForDeck should throw IllegalArgumentException if JSON string is not a StoredReviewReminderGroup`() { - val randomObject = Pair("not a group of", "review reminders") - ReviewRemindersDatabase.remindersSharedPrefs.edit { - putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, Json.encodeToString(randomObject)) + @Test + fun `getRemindersForDeck should delete reminders if JSON string for StoredReviewReminderGroup is corrupted`() { + corruptedRemindersTest( + corruptedValue = "corrupted_and_invalid_json_string", + expectedDeletedKeys = setOf(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1), + ) { + ReviewRemindersDatabase.getRemindersForDeck(did1) } - ReviewRemindersDatabase.getRemindersForDeck(did1) } - @Test(expected = SerializationException::class) - fun `getRemindersForDeck should throw SerializationException if JSON string for review reminder is corrupted`() { - val corruptedStoredReviewReminderGroup = - ReviewRemindersDatabase.StoredReviewReminderGroup( - ReviewRemindersDatabase.schemaVersion, - "corrupted_and_invalid_json_string", - ) - ReviewRemindersDatabase.remindersSharedPrefs.edit { - putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, Json.encodeToString(corruptedStoredReviewReminderGroup)) + @Test + fun `getRemindersForDeck should delete reminders if JSON string is not a StoredReviewReminderGroup`() { + corruptedRemindersTest( + corruptedValue = Json.encodeToString(Pair("not a group of", "review reminders")), + expectedDeletedKeys = setOf(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did2), + ) { + ReviewRemindersDatabase.getRemindersForDeck(did2) } - ReviewRemindersDatabase.getRemindersForDeck(did1) } - @Test(expected = SerializationException::class) - fun `getAllAppWideReminders should throw SerializationException if JSON string for StoredReviewReminderGroup is corrupted`() { - ReviewRemindersDatabase.remindersSharedPrefs.edit { - putString(ReviewRemindersDatabase.APP_WIDE_KEY, "corrupted_and_invalid_json_string") + @Test + fun `getRemindersForDeck should delete reminders if JSON string for review reminder is corrupted`() { + corruptedRemindersTest( + corruptedValue = + Json.encodeToString( + ReviewRemindersDatabase.StoredReviewReminderGroup( + ReviewRemindersDatabase.schemaVersion, + "corrupted_and_invalid_json_string", + ), + ), + expectedDeletedKeys = setOf(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1), + ) { + ReviewRemindersDatabase.getRemindersForDeck(did1) } - ReviewRemindersDatabase.getAllAppWideReminders() } - @Test(expected = IllegalArgumentException::class) - fun `getAllAppWideReminders should throw IllegalArgumentException if JSON string is not a StoredReviewReminderGroup`() { - val randomObject = Pair("not a group of", "review reminders") - ReviewRemindersDatabase.remindersSharedPrefs.edit { - putString(ReviewRemindersDatabase.APP_WIDE_KEY, Json.encodeToString(randomObject)) + @Test + fun `getAllAppWideReminders should delete reminders if JSON string for StoredReviewReminderGroup is corrupted`() { + corruptedRemindersTest( + corruptedValue = "corrupted_and_invalid_json_string", + expectedDeletedKeys = setOf(ReviewRemindersDatabase.APP_WIDE_KEY), + ) { + ReviewRemindersDatabase.getAllAppWideReminders() } - ReviewRemindersDatabase.getAllAppWideReminders() } - @Test(expected = SerializationException::class) - fun `getAllAppWideReminders should throw SerializationException if JSON string for review reminder is corrupted`() { - val corruptedStoredReviewReminderGroup = - ReviewRemindersDatabase.StoredReviewReminderGroup( - ReviewRemindersDatabase.schemaVersion, - "corrupted_and_invalid_json_string", - ) - ReviewRemindersDatabase.remindersSharedPrefs.edit { - putString(ReviewRemindersDatabase.APP_WIDE_KEY, Json.encodeToString(corruptedStoredReviewReminderGroup)) + @Test + fun `getAllAppWideReminders should delete reminders if JSON string is not a StoredReviewReminderGroup`() { + corruptedRemindersTest( + corruptedValue = Json.encodeToString(Pair("not a group of", "review reminders")), + expectedDeletedKeys = setOf(ReviewRemindersDatabase.APP_WIDE_KEY), + ) { + ReviewRemindersDatabase.getAllAppWideReminders() } - ReviewRemindersDatabase.getAllAppWideReminders() } - @Test(expected = SerializationException::class) - fun `getAllDeckSpecificReminders should throw SerializationException if JSON string for StoredReviewReminderGroup is corrupted`() { - ReviewRemindersDatabase.remindersSharedPrefs.edit { - putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, "corrupted_and_invalid_json_string") + @Test + fun `getAllAppWideReminders should delete reminders if JSON string for review reminder is corrupted`() { + corruptedRemindersTest( + corruptedValue = + Json.encodeToString( + ReviewRemindersDatabase.StoredReviewReminderGroup( + ReviewRemindersDatabase.schemaVersion, + "corrupted_and_invalid_json_string", + ), + ), + expectedDeletedKeys = setOf(ReviewRemindersDatabase.APP_WIDE_KEY), + ) { + ReviewRemindersDatabase.getAllAppWideReminders() } - ReviewRemindersDatabase.getAllDeckSpecificReminders() } - @Test(expected = IllegalArgumentException::class) - fun `getAllDeckSpecificReminders should throw IllegalArgumentException if JSON string is not a StoredReviewReminderGroup`() { - val randomObject = Pair("not a group of", "review reminders") - ReviewRemindersDatabase.remindersSharedPrefs.edit { - putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, Json.encodeToString(randomObject)) + @Test + fun `getAllDeckSpecificReminders should delete reminders if JSON string for StoredReviewReminderGroup is corrupted`() { + corruptedRemindersTest( + corruptedValue = "corrupted_and_invalid_json_string", + expectedDeletedKeys = setOf(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did2), + ) { + ReviewRemindersDatabase.getAllDeckSpecificReminders() } - ReviewRemindersDatabase.getAllDeckSpecificReminders() } - @Test(expected = SerializationException::class) - fun `getAllDeckSpecificReminders should throw SerializationException if JSON string for review reminder is corrupted`() { - val corruptedStoredReviewReminderGroup = - ReviewRemindersDatabase.StoredReviewReminderGroup( - ReviewRemindersDatabase.schemaVersion, - "corrupted_and_invalid_json_string", - ) - ReviewRemindersDatabase.remindersSharedPrefs.edit { - putString(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, Json.encodeToString(corruptedStoredReviewReminderGroup)) + @Test + fun `getAllDeckSpecificReminders should delete reminders if JSON string is not a StoredReviewReminderGroup`() { + corruptedRemindersTest( + corruptedValue = Json.encodeToString(Pair("not a group of", "review reminders")), + expectedDeletedKeys = setOf(ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did2), + ) { + ReviewRemindersDatabase.getAllDeckSpecificReminders() + } + } + + @Test + fun `getAllDeckSpecificReminders should delete reminders if JSON string for review reminder is corrupted`() { + corruptedRemindersTest( + corruptedValue = + Json.encodeToString( + ReviewRemindersDatabase.StoredReviewReminderGroup( + ReviewRemindersDatabase.schemaVersion, + "corrupted_and_invalid_json_string", + ), + ), + expectedDeletedKeys = + setOf( + ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did1, + ReviewRemindersDatabase.DECK_SPECIFIC_KEY + did2, + ), + ) { + ReviewRemindersDatabase.getAllDeckSpecificReminders() } - ReviewRemindersDatabase.getAllDeckSpecificReminders() } @Test From 2e2345a74cac7a35bbaf5213f09c241a979ec827 Mon Sep 17 00:00:00 2001 From: Song Eric Yu Li Date: Sun, 8 Feb 2026 18:05:38 -0800 Subject: [PATCH 5/6] refactor: Move ReviewReminderDatabase lambdas A future commit will use `upsertReminder` outside of ScheduleReminders, so they've been moved to ReviewRemindersDatabase. --- .../ReviewRemindersDatabase.kt | 35 +++++++++++++++++++ .../anki/reviewreminders/ScheduleReminders.kt | 35 ------------------- 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt index f1009972b67f..dcc1b19991a0 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt @@ -374,3 +374,38 @@ object ReviewRemindersDatabase { } } } + +/** + * Lambda that can be fed into [ReviewRemindersDatabase.editRemindersForDeck] or + * [ReviewRemindersDatabase.editAllAppWideReminders] which deletes the given review reminder. + */ +fun deleteReminder(reminder: ReviewReminder) = + { reminders: ReviewReminderGroup -> + reminders.apply { + remove(reminder.id) + } + } + +/** + * Lambda that can be fed into [ReviewRemindersDatabase.editRemindersForDeck] or + * [ReviewRemindersDatabase.editAllAppWideReminders] which updates the given review reminder if it + * exists or inserts it if it doesn't (an "upsert" operation) + */ +fun upsertReminder(reminder: ReviewReminder) = + { reminders: ReviewReminderGroup -> + reminders.apply { + this[reminder.id] = reminder + } + } + +/** + * Lambda that can be fed into [ReviewRemindersDatabase.editRemindersForDeck] or + * [ReviewRemindersDatabase.editAllAppWideReminders] which toggles whether the given review reminder + * is enabled. + */ +fun toggleReminder(reminder: ReviewReminder) = + { reminders: ReviewReminderGroup -> + reminders.apply { + toggleEnabled(reminder.id) + } + } diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt index d0a11af5b044..880ac8db3bcb 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt @@ -259,41 +259,6 @@ class ScheduleReminders : } } - /** - * Lambda that can be fed into [ReviewRemindersDatabase.editRemindersForDeck] or - * [ReviewRemindersDatabase.editAllAppWideReminders] which deletes the given review reminder. - */ - private fun deleteReminder(reminder: ReviewReminder) = - { reminders: ReviewReminderGroup -> - reminders.apply { - remove(reminder.id) - } - } - - /** - * Lambda that can be fed into [ReviewRemindersDatabase.editRemindersForDeck] or - * [ReviewRemindersDatabase.editAllAppWideReminders] which updates the given review reminder if it - * exists or inserts it if it doesn't (an "upsert" operation) - */ - private fun upsertReminder(reminder: ReviewReminder) = - { reminders: ReviewReminderGroup -> - reminders.apply { - this[reminder.id] = reminder - } - } - - /** - * Lambda that can be fed into [ReviewRemindersDatabase.editRemindersForDeck] or - * [ReviewRemindersDatabase.editAllAppWideReminders] which toggles whether the given review reminder - * is enabled. - */ - private fun toggleReminder(reminder: ReviewReminder) = - { reminders: ReviewReminderGroup -> - reminders.apply { - toggleEnabled(reminder.id) - } - } - /** * Update the RecyclerView with the new or modified reminder. * @see handleAddEditDialogResult From d23b368f69628e754ef983366b7800923c59ad21 Mon Sep 17 00:00:00 2001 From: Song Eric Yu Li Date: Sun, 15 Feb 2026 17:30:46 -0800 Subject: [PATCH 6/6] feat(reminders): fire immediately if notification is late Review Reminders We currently schedule all stored review reminders for their next upcoming alarm time when the application process starts up, such as when the app initially launches (we also have a BOOT_COMPLETED receiver). However, a notification "firing" is not just a single moment in time; since we don't have permissions to use proper alarm-clock-style alarms, the best we can do is a 10 minute `setWindow`, which means "firing" a notification is a non-deterministic event that happens some time in the ten minutes after a review reminder's scheduled notification time. Consider the following set of actions: 1. The user creates a review reminder for 1:00 pm, its first notification is scheduled for 1:00 pm on Monday 2. At 1:00, we enter the `setWindow` period 3. At 1:03, the notification still has not fired; curious as to why, the user opens up their app 4. This causes `scheduleAllNotifications` to trigger, which reschedules the review reminder notification for the next upcoming time, which is 1:00 pm on Tuesday 5. ... We've skipped a notification! To solution for this is to copy most notification systems: we record when the latest notification time was ourselves (it's impossible to read AlarmManager's currently-scheduled alarms programmatically) and check whenever scheduling a notification if the review reminder's notification has fired since its most recent scheduled time. If not, we immediately fire a notification. This also helps with missed notifications in general: if, for example, the user has had their device off for 6 hours and then turns it on, all alarms that fired during that interval should immediately show. This commit also creates unit tests surrounding this feature and enhances the readability of NotificationServiceTest. The ReviewReminderSchema is migrated from v2 to v3. --- .../anki/reviewreminders/ReviewReminder.kt | 36 ++ .../reviewreminders/ReviewReminderSchema.kt | 25 ++ .../ReviewRemindersDatabase.kt | 6 +- .../anki/services/AlarmManagerService.kt | 49 ++- .../anki/services/NotificationService.kt | 20 +- .../reviewreminders/ReviewReminderTest.kt | 159 ++++++++ .../ReviewRemindersDatabaseTest.kt | 76 +++- .../anki/services/AlarmManagerServiceTest.kt | 240 +++++++++-- .../anki/services/NotificationServiceTest.kt | 377 ++++++++---------- .../testutils/ext/ReviewRemindersDatabase.kt | 40 ++ .../ichi2/anki/libanki/testutils/AnkiTest.kt | 37 ++ 11 files changed, 804 insertions(+), 261 deletions(-) create mode 100644 AnkiDroid/src/test/java/com/ichi2/testutils/ext/ReviewRemindersDatabase.kt diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt index 0bf200171768..0d533a73ca20 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt @@ -22,6 +22,7 @@ import android.text.format.DateFormat import com.ichi2.anki.CollectionManager.withCol import com.ichi2.anki.common.time.TimeManager import com.ichi2.anki.libanki.DeckId +import com.ichi2.anki.libanki.EpochMilliseconds import com.ichi2.anki.settings.Prefs import kotlinx.parcelize.IgnoredOnParcel import kotlinx.parcelize.Parcelize @@ -185,6 +186,9 @@ sealed class ReviewReminderScope : Parcelable { * @param enabled Whether the review reminder's notifications are active or disabled. * @param profileID ID representing the profile which created this review reminder, as review reminders for * multiple profiles might be active simultaneously. + * @param latestNotifTime The time at which this review reminder last attempted to fire a routine daily (non-snooze) + * notification, in epoch milliseconds, or the time at which it was created if no notification has ever been fired. + * See [shouldImmediatelyFire]. * @param onlyNotifyIfNoReviews If true, only notify the user if this scope has not been reviewed today yet. */ @Serializable @@ -196,6 +200,7 @@ data class ReviewReminder private constructor( val cardTriggerThreshold: ReviewReminderCardTriggerThreshold, val scope: ReviewReminderScope, var enabled: Boolean, + var latestNotifTime: EpochMilliseconds, val profileID: String, val onlyNotifyIfNoReviews: Boolean, ) : Parcelable, @@ -219,11 +224,42 @@ data class ReviewReminder private constructor( cardTriggerThreshold, scope, enabled, + latestNotifTime = TimeManager.time.calendar().timeInMillis, profileID, onlyNotifyIfNoReviews, ) } + /** + * Updates [latestNotifTime] to the current time. + * This should be called whenever this review reminder attempts to fire a routine daily (non-snooze) notification. + */ + fun updateLatestNotifTime() { + latestNotifTime = TimeManager.time.calendar().timeInMillis + } + + /** + * Checks if this review reminder has tried to fire a routine daily (non-snooze) notification in the time between + * its latest scheduled firing time and now. If not, this method returns true, indicating that a notification + * should be immediately fired for this review reminder. + */ + fun shouldImmediatelyFire(): Boolean { + val (hour, minute) = this.time + + val currentTimestamp = TimeManager.time.calendar() + val latestScheduledTimestamp = currentTimestamp.clone() as Calendar + latestScheduledTimestamp.apply { + set(Calendar.HOUR_OF_DAY, hour) + set(Calendar.MINUTE, minute) + set(Calendar.SECOND, 0) + if (after(currentTimestamp)) { + add(Calendar.DAY_OF_YEAR, -1) + } + } + + return latestNotifTime < latestScheduledTimestamp.timeInMillis + } + /** * This is the up-to-date schema, we cannot migrate to a newer version. */ diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderSchema.kt b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderSchema.kt index 90693590e53e..c4787a96b74f 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderSchema.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderSchema.kt @@ -75,6 +75,31 @@ data class ReviewReminderSchemaV1( var enabled: Boolean, val profileID: String, val onlyNotifyIfNoReviews: Boolean = false, +) : ReviewReminderSchema { + override fun migrate(): ReviewReminderSchema = + ReviewReminderSchemaV2( + id = id, + time = time, + cardTriggerThreshold = cardTriggerThreshold, + scope = scope, + enabled = enabled, + profileID = profileID, + onlyNotifyIfNoReviews = onlyNotifyIfNoReviews, + ) +} + +/** + * Version 2 of [ReviewReminderSchema]. Updated to Version 3 by adding [ReviewReminder.latestNotifTime]. + */ +@Serializable +data class ReviewReminderSchemaV2( + override val id: ReviewReminderId, + val time: ReviewReminderTime, + val cardTriggerThreshold: ReviewReminderCardTriggerThreshold, + val scope: ReviewReminderScope, + var enabled: Boolean, + val profileID: String, + val onlyNotifyIfNoReviews: Boolean, ) : ReviewReminderSchema { override fun migrate(): ReviewReminderSchema = ReviewReminder.createReviewReminder( diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt index dcc1b19991a0..869238328686 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt @@ -107,12 +107,13 @@ object ReviewRemindersDatabase { * * Version 1: 3 August 2025 - Initial version * Version 2: 25 January 2026 - Added [ReviewReminder.onlyNotifyIfNoReviews] + * Version 3: 8 February 2026 - Added [ReviewReminder.latestNotifTime] * * @see [oldReviewReminderSchemasForMigration] * @see [ReviewReminder] */ @VisibleForTesting - var schemaVersion = ReviewReminderSchemaVersion(2) + var schemaVersion = ReviewReminderSchemaVersion(3) /** * A map of all old [ReviewReminderSchema]s that [ReviewRemindersDatabase.performSchemaMigration] will attempt to migrate old @@ -130,7 +131,8 @@ object ReviewRemindersDatabase { var oldReviewReminderSchemasForMigration: Map> = mapOf( ReviewReminderSchemaVersion(1) to ReviewReminderSchemaV1::class, - ReviewReminderSchemaVersion(2) to ReviewReminder::class, // Most up to date version + ReviewReminderSchemaVersion(2) to ReviewReminderSchemaV2::class, + ReviewReminderSchemaVersion(3) to ReviewReminder::class, // Most up to date version ) /** diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/services/AlarmManagerService.kt b/AnkiDroid/src/main/java/com/ichi2/anki/services/AlarmManagerService.kt index 456e239580cd..a4c07b8269de 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/services/AlarmManagerService.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/services/AlarmManagerService.kt @@ -22,13 +22,16 @@ import android.app.PendingIntent import android.content.BroadcastReceiver import android.content.Context import android.content.Intent +import androidx.annotation.VisibleForTesting import androidx.core.app.PendingIntentCompat import androidx.core.content.getSystemService import androidx.core.os.BundleCompat import com.ichi2.anki.R import com.ichi2.anki.common.time.TimeManager import com.ichi2.anki.reviewreminders.ReviewReminder +import com.ichi2.anki.reviewreminders.ReviewReminderScope import com.ichi2.anki.reviewreminders.ReviewRemindersDatabase +import com.ichi2.anki.reviewreminders.upsertReminder import com.ichi2.anki.showThemedToast import timber.log.Timber import java.util.Calendar @@ -65,7 +68,8 @@ class AlarmManagerService : BroadcastReceiver() { * by at much this amount of time. We set it to 10 minutes, which is the minimum allowable duration * according to [the docs](https://developer.android.com/reference/android/app/AlarmManager). */ - private val WINDOW_LENGTH_MS: Long = 10.minutes.inWholeMilliseconds + @VisibleForTesting + val WINDOW_LENGTH_MS: Long = 10.minutes.inWholeMilliseconds /** * Shows error messages if an error occurs when scheduling review reminders via AlarmManager. @@ -132,6 +136,11 @@ class AlarmManagerService : BroadcastReceiver() { * Queues a review reminder to have its notification fired at its specified time. Does not check * if the review reminder is enabled or not, the caller must handle this. * + * If the review reminder has failed to fire a notification at its most recent specified time for some + * reason (ex. if the device was off, or if the OS delayed the notification for some reason), + * a notification will be fired immediately and no alarm will be immediately scheduled, as the + * notification should automatically trigger the scheduling of the next alarm. + * * Note that this only schedules the next upcoming notification, using [AlarmManager.setWindow] * rather than [AlarmManager.setRepeating]. This is because [AlarmManager.setRepeating] sometimes * postpones alarm firings for long periods of time, with intervals as long as one hour observed @@ -164,6 +173,13 @@ class AlarmManagerService : BroadcastReceiver() { ) ?: return Timber.v("Pending intent for ${reviewReminder.id} is $pendingIntent") + if (reviewReminder.shouldImmediatelyFire()) { + immediatelyFireNotification(context, reviewReminder) + // Once the notification has fired, it will automatically trigger the setting of the next alarm + // so we can return immediately + return + } + val currentTimestamp = TimeManager.time.calendar() val alarmTimestamp = currentTimestamp.clone() as Calendar alarmTimestamp.apply { @@ -186,6 +202,37 @@ class AlarmManagerService : BroadcastReceiver() { } } + /** + * Immediately fires a review reminder notification for a review reminder, which in turn then schedules the next notification. + * Used when a review reminder's notification has been delayed and failed to fire for some reason. + */ + private fun immediatelyFireNotification( + context: Context, + reviewReminder: ReviewReminder, + ) { + Timber.d("Review reminder ${reviewReminder.id} should have fired already, sending notification immediately") + + // Immediately (redundantly) record this latest routine notification-firing attempt's timestamp + // to prevent this from being triggered multiple times in rapid succession + reviewReminder.updateLatestNotifTime() + when (val scope = reviewReminder.scope) { + is ReviewReminderScope.DeckSpecific -> + ReviewRemindersDatabase.editRemindersForDeck( + scope.did, + upsertReminder(reviewReminder), + ) + is ReviewReminderScope.Global -> ReviewRemindersDatabase.editAllAppWideReminders(upsertReminder(reviewReminder)) + } + + val immediateNotificationIntent = + NotificationService.getIntent( + context, + reviewReminder, + NotificationService.NotificationServiceAction.ScheduleRecurringNotifications, + ) + context.sendBroadcast(immediateNotificationIntent) + } + /** * Deletes any scheduled notifications for this review reminder. Does not actually delete the * review reminder itself from anywhere, only deletes any queued alarms for the review reminder. diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/services/NotificationService.kt b/AnkiDroid/src/main/java/com/ichi2/anki/services/NotificationService.kt index ae094e1a43f8..35df9eeb05c3 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/services/NotificationService.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/services/NotificationService.kt @@ -20,6 +20,7 @@ import android.content.BroadcastReceiver import android.content.Context import android.content.Intent import android.graphics.Color +import androidx.annotation.VisibleForTesting import androidx.core.app.NotificationCompat import androidx.core.app.PendingIntentCompat import androidx.core.content.getSystemService @@ -37,6 +38,8 @@ import com.ichi2.anki.preferences.PENDING_NOTIFICATIONS_ONLY import com.ichi2.anki.preferences.sharedPrefs import com.ichi2.anki.reviewreminders.ReviewReminder import com.ichi2.anki.reviewreminders.ReviewReminderScope +import com.ichi2.anki.reviewreminders.ReviewRemindersDatabase +import com.ichi2.anki.reviewreminders.upsertReminder import com.ichi2.anki.runGloballyWithTimeout import com.ichi2.anki.settings.Prefs import com.ichi2.anki.utils.ext.allDecksCounts @@ -71,7 +74,8 @@ class NotificationService : BroadcastReceiver() { /** * Extra key for sending a review reminder as an extra to this broadcast receiver. */ - private const val EXTRA_REVIEW_REMINDER = "notification_service_review_reminder" + @VisibleForTesting + const val EXTRA_REVIEW_REMINDER = "notification_service_review_reminder" /** * Timeout for the process of sending a review reminder notification. @@ -417,8 +421,20 @@ class NotificationService : BroadcastReceiver() { ) ?: return Timber.d("onReceive: ${reviewReminder.id}") - // Schedule the next instance of this review reminder notification if this is a recurring notification + // If this is a recurring notification... if (action == NotificationServiceAction.ScheduleRecurringNotifications.actionString) { + // Record this latest routine notification-firing attempt's timestamp + reviewReminder.updateLatestNotifTime() + when (val scope = reviewReminder.scope) { + is ReviewReminderScope.DeckSpecific -> + ReviewRemindersDatabase.editRemindersForDeck( + scope.did, + upsertReminder(reviewReminder), + ) + is ReviewReminderScope.Global -> ReviewRemindersDatabase.editAllAppWideReminders(upsertReminder(reviewReminder)) + } + + // Schedule the next routine notification-firing Timber.d("Scheduling next review reminder notification") AlarmManagerService.scheduleReviewReminderNotification(context, reviewReminder) } diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewReminderTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewReminderTest.kt index cfa8551c65e6..2b6fe02bdffc 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewReminderTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewReminderTest.kt @@ -19,24 +19,52 @@ package com.ichi2.anki.reviewreminders import androidx.core.content.edit import androidx.test.ext.junit.runners.AndroidJUnit4 import com.ichi2.anki.RobolectricTest +import com.ichi2.anki.common.time.MockTime +import com.ichi2.anki.common.time.TimeManager +import com.ichi2.anki.settings.Prefs import org.hamcrest.MatcherAssert.assertThat import org.hamcrest.Matchers.equalTo import org.junit.After import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import java.util.Calendar +import kotlin.time.Duration.Companion.days +import kotlin.time.Duration.Companion.hours +import kotlin.time.Duration.Companion.minutes @RunWith(AndroidJUnit4::class) class ReviewReminderTest : RobolectricTest() { + companion object { + /** + * A mock time for a consistent date and a specific time: noon in the local time zone (not UTC! this is required because + * the subject-under-test calculates times based on the local time zone, hence the convoluted construction of this mock). + */ + private val mockTime = + MockTime( + initTime = + MockTime(2024, 0, 1, 0, 0, 0, 0, 0) + .calendar() + .apply { + set(Calendar.HOUR_OF_DAY, 12) + set(Calendar.MINUTE, 0) + set(Calendar.SECOND, 0) + }.timeInMillis, + step = 1000, + ) + } + @Before override fun setUp() { super.setUp() + Prefs.reviewReminderNextFreeId = 0 ReviewRemindersDatabase.remindersSharedPrefs.edit { clear() } } @After override fun tearDown() { super.tearDown() + Prefs.reviewReminderNextFreeId = 0 ReviewRemindersDatabase.remindersSharedPrefs.edit { clear() } } @@ -47,4 +75,135 @@ class ReviewReminderTest : RobolectricTest() { assertThat(reminder.id, equalTo(ReviewReminderId(i))) } } + + @Test + fun `latestNotifTime is set at creation`() { + val timeBeforeCreation = TimeManager.time.calendar().timeInMillis + val reviewReminder = ReviewReminder.createReviewReminder(time = ReviewReminderTime(hour = 1, minute = 0)) + val timeAfterCreation = TimeManager.time.calendar().timeInMillis + + assertThat(reviewReminder.latestNotifTime in timeBeforeCreation..timeAfterCreation, equalTo(true)) + } + + @Test + fun `updateLatestNotifTime works correctly`() { + val reviewReminder = ReviewReminder.createReviewReminder(time = ReviewReminderTime(hour = 1, minute = 0)) + + val timeBeforeUpdate = TimeManager.time.calendar().timeInMillis + reviewReminder.updateLatestNotifTime() + val timeAfterUpdate = TimeManager.time.calendar().timeInMillis + + assertThat(reviewReminder.latestNotifTime in timeBeforeUpdate..timeAfterUpdate, equalTo(true)) + } + + @Test + fun `notification should immediately fire if there was no scheduled firing`() { + shouldImmediatelyFireTest( + currentTimeOffsetFromWindowStartMs = 6.hours.inWholeMilliseconds.toInt(), + lastFiringOffsetFromWindowStartMs = (-1).minutes.inWholeMilliseconds.toInt(), + shouldImmediatelyFire = true, + ) + } + + @Test + fun `notification should not immediately fire if there was a scheduled firing`() { + shouldImmediatelyFireTest( + currentTimeOffsetFromWindowStartMs = 6.hours.inWholeMilliseconds.toInt(), + lastFiringOffsetFromWindowStartMs = 1.minutes.inWholeMilliseconds.toInt(), + shouldImmediatelyFire = false, + ) + } + + @Test + fun `notification should immediately fire if scheduled firing time is recent and there was no scheduled firing`() { + shouldImmediatelyFireTest( + currentTimeOffsetFromWindowStartMs = 2.minutes.inWholeMilliseconds.toInt(), + lastFiringOffsetFromWindowStartMs = (-1).minutes.inWholeMilliseconds.toInt(), + shouldImmediatelyFire = true, + ) + } + + @Test + fun `notification should not immediately fire if scheduled firing time is recent and there was a scheduled firing`() { + shouldImmediatelyFireTest( + currentTimeOffsetFromWindowStartMs = 1.minutes.inWholeMilliseconds.toInt(), + lastFiringOffsetFromWindowStartMs = 0, + shouldImmediatelyFire = false, + ) + } + + @Test + fun `notification should immediately fire if latest firing was a long time ago`() { + shouldImmediatelyFireTest( + currentTimeOffsetFromWindowStartMs = 0, + lastFiringOffsetFromWindowStartMs = (-2).days.inWholeMilliseconds.toInt(), + shouldImmediatelyFire = true, + ) + } + + @Test + fun `notification should immediately fire even if next window is approaching if there was no scheduled firing`() { + shouldImmediatelyFireTest( + currentTimeOffsetFromWindowStartMs = (-1).minutes.inWholeMilliseconds.toInt(), + lastFiringOffsetFromWindowStartMs = (-25).hours.inWholeMilliseconds.toInt(), + shouldImmediatelyFire = true, + ) + } + + @Test + fun `notification should not immediately fire if scheduled firing was just late`() { + shouldImmediatelyFireTest( + currentTimeOffsetFromWindowStartMs = 23.hours.inWholeMilliseconds.toInt(), + lastFiringOffsetFromWindowStartMs = 22.hours.inWholeMilliseconds.toInt(), + shouldImmediatelyFire = false, + ) + } + + @Test + fun `notification should immediately fire if scheduled firing is now and latest firing was in the past`() { + shouldImmediatelyFireTest( + currentTimeOffsetFromWindowStartMs = 0, + lastFiringOffsetFromWindowStartMs = (-1).minutes.inWholeMilliseconds.toInt(), + shouldImmediatelyFire = true, + ) + } + + @Test + fun `notification should not immediately fire if scheduled firing is now and latest firing was just now`() { + shouldImmediatelyFireTest( + currentTimeOffsetFromWindowStartMs = 0, + lastFiringOffsetFromWindowStartMs = 0, + shouldImmediatelyFire = false, + ) + } + + private fun shouldImmediatelyFireTest( + currentTimeOffsetFromWindowStartMs: Int, + lastFiringOffsetFromWindowStartMs: Int, + shouldImmediatelyFire: Boolean, + ) { + val reviewReminder = ReviewReminder.createReviewReminder(time = ReviewReminderTime(hour = 1, minute = 0)) + + val currentTime = mockTime.calendar().clone() as Calendar + currentTime.apply { + set(Calendar.HOUR_OF_DAY, 1) + set(Calendar.MINUTE, 0) + set(Calendar.SECOND, 0) + add(Calendar.MILLISECOND, currentTimeOffsetFromWindowStartMs) + } + TimeManager.resetWith(MockTime(currentTime.timeInMillis, step = 1000)) + + val lastFiring = mockTime.calendar().clone() as Calendar + lastFiring.apply { + set(Calendar.HOUR_OF_DAY, 1) + set(Calendar.MINUTE, 0) + set(Calendar.SECOND, 0) + add(Calendar.MILLISECOND, lastFiringOffsetFromWindowStartMs) + } + reviewReminder.latestNotifTime = lastFiring.timeInMillis + + assertThat(reviewReminder.shouldImmediatelyFire(), equalTo(shouldImmediatelyFire)) + + TimeManager.reset() + } } diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt index d529dedf06c6..364c039e0aad 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt @@ -19,6 +19,7 @@ package com.ichi2.anki.reviewreminders import androidx.core.content.edit import androidx.test.ext.junit.runners.AndroidJUnit4 import com.ichi2.anki.RobolectricTest +import com.ichi2.anki.libanki.EpochMilliseconds import kotlinx.serialization.InternalSerializationApi import kotlinx.serialization.KSerializer import kotlinx.serialization.builtins.MapSerializer @@ -331,23 +332,27 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { } /** - * When review reminders are migrated to the new schema, the reminders' IDs will be recreated from scratch. - * Thus, validation that our tests succeeded should ignore [ReviewReminder.id]. + * When review reminders are migrated to the new schema, the reminders' IDs and latestNotifTimes will be recreated from scratch. + * Thus, validation that our tests succeeded should ignore these fields. * This custom Hamcrest matcher performs this validation using reflection. */ - private fun containsEqualReviewRemindersInAnyOrderIgnoringId(expected: Collection): Matcher> = + private fun containsEqualReviewRemindersExcludingVolatileFields( + expected: Collection, + ): Matcher> = object : TypeSafeMatcher>() { override fun describeTo(description: Description) { description.appendValue(expected) } override fun matchesSafely(actual: Iterable): Boolean { + val volatileFields = setOf("id", "latestNotifTime") + val expectedSet = expected .map { e -> ReviewReminder::class .memberProperties - .filterNot { it.name == "id" } + .filterNot { it.name in volatileFields } .associateWith { it.get(e) } }.toSet() @@ -356,7 +361,7 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { .map { a -> ReviewReminder::class .memberProperties - .filterNot { it.name == "id" } + .filterNot { it.name in volatileFields } .associateWith { it.get(a) } }.toSet() @@ -374,14 +379,14 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { */ @Test fun `current schema version points to ReviewReminder`() { - assertThat(ReviewRemindersDatabase.schemaVersion.value, equalTo(2)) + assertThat(ReviewRemindersDatabase.schemaVersion.value, equalTo(3)) assertThat( ReviewRemindersDatabase .oldReviewReminderSchemasForMigration .keys .last() .value, - equalTo(2), + equalTo(3), ) assertThat( ReviewRemindersDatabase @@ -408,8 +413,8 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { val rawString = """ { - "version":2, - "remindersMapJson":"{\"0\":{\"id\":0,\"time\":{\"hour\":9,\"minute\":0},\"cardTriggerThreshold\":5,\"scope\":{\"type\":\"com.ichi2.anki.reviewreminders.ReviewReminderScope.DeckSpecific\",\"did\":12345},\"enabled\":false,\"profileID\":\"\",\"onlyNotifyIfNoReviews\":false},\"1\":{\"id\":1,\"time\":{\"hour\":10,\"minute\":30},\"cardTriggerThreshold\":10,\"scope\":{\"type\":\"com.ichi2.anki.reviewreminders.ReviewReminderScope.DeckSpecific\",\"did\":12345},\"enabled\":true,\"profileID\":\"\",\"onlyNotifyIfNoReviews\":false}}" + "version":3, + "remindersMapJson":"{\"22\":{\"id\":22,\"time\":{\"hour\":14,\"minute\":16},\"cardTriggerThreshold\":1,\"scope\":{\"type\":\"com.ichi2.anki.reviewreminders.ReviewReminderScope.Global\"},\"enabled\":true,\"latestNotifTime\":1771193761002,\"profileID\":\"\",\"onlyNotifyIfNoReviews\":false}}" } """.trimIndent() @@ -434,6 +439,7 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { "cardTriggerThreshold" to ReviewReminderCardTriggerThreshold::class, "scope" to ReviewReminderScope::class, "enabled" to Boolean::class, + "latestNotifTime" to EpochMilliseconds::class, "profileID" to String::class, "onlyNotifyIfNoReviews" to Boolean::class, ) @@ -523,7 +529,7 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { } assertThat( retrievedReminders.getRemindersList(), - containsEqualReviewRemindersInAnyOrderIgnoringId( + containsEqualReviewRemindersExcludingVolatileFields( casesInScope.map { it.expectedOutput }, ), ) @@ -683,4 +689,54 @@ class ReviewRemindersDatabaseTest : RobolectricTest() { ), ) } + + @Test + fun `review reminder v2 to v3 migration works`() { + assertMigrationsWork( + MigrationTestCase( + inputVersion = ReviewReminderSchemaVersion(2), + input = + ReviewReminderSchemaV2( + id = ReviewReminderId(0), + time = ReviewReminderTime(10, 30), + cardTriggerThreshold = ReviewReminderCardTriggerThreshold(10), + scope = ReviewReminderScope.Global, + enabled = true, + profileID = "", + onlyNotifyIfNoReviews = false, + ), + expectedOutput = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(10, 30), + cardTriggerThreshold = ReviewReminderCardTriggerThreshold(10), + scope = ReviewReminderScope.Global, + enabled = true, + profileID = "", + onlyNotifyIfNoReviews = false, + ), + ), + MigrationTestCase( + inputVersion = ReviewReminderSchemaVersion(2), + input = + ReviewReminderSchemaV2( + id = ReviewReminderId(1), + time = ReviewReminderTime(12, 0), + cardTriggerThreshold = ReviewReminderCardTriggerThreshold(20), + scope = ReviewReminderScope.DeckSpecific(did2), + enabled = false, + profileID = "", + onlyNotifyIfNoReviews = true, + ), + expectedOutput = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(12, 0), + cardTriggerThreshold = ReviewReminderCardTriggerThreshold(20), + scope = ReviewReminderScope.DeckSpecific(did2), + enabled = false, + profileID = "", + onlyNotifyIfNoReviews = true, + ), + ), + ) + } } diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/services/AlarmManagerServiceTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/services/AlarmManagerServiceTest.kt index 200ce394fd71..0c8561a3d18f 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/services/AlarmManagerServiceTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/services/AlarmManagerServiceTest.kt @@ -30,28 +30,47 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import com.ichi2.anki.RobolectricTest import com.ichi2.anki.common.time.MockTime import com.ichi2.anki.common.time.TimeManager +import com.ichi2.anki.libanki.EpochMilliseconds import com.ichi2.anki.reviewreminders.ReviewReminder -import com.ichi2.anki.reviewreminders.ReviewReminderGroup -import com.ichi2.anki.reviewreminders.ReviewReminderId import com.ichi2.anki.reviewreminders.ReviewReminderScope import com.ichi2.anki.reviewreminders.ReviewReminderTime import com.ichi2.anki.reviewreminders.ReviewRemindersDatabase +import com.ichi2.anki.services.NotificationService.Companion.EXTRA_REVIEW_REMINDER +import com.ichi2.testutils.ext.storeReminders import io.mockk.every import io.mockk.mockk import io.mockk.mockkStatic import io.mockk.unmockkAll import io.mockk.verify +import org.hamcrest.MatcherAssert.assertThat +import org.hamcrest.Matchers.equalTo import org.junit.After import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import java.util.Calendar +import kotlin.time.Duration.Companion.days import kotlin.time.Duration.Companion.minutes @RunWith(AndroidJUnit4::class) class AlarmManagerServiceTest : RobolectricTest() { companion object { - private val mockTime = MockTime(2024, 0, 1, 12, 0, 0, 0, 0) + /** + * Set the mock time to a consistent date and a specific time: noon in the local time zone (not UTC! this is required because + * the subject-under-test calculates times based on the local time zone, hence the convoluted construction of this mock). + */ + private val mockTime = + MockTime( + initTime = + MockTime(2024, 0, 1, 0, 0, 0, 0, 0) + .calendar() + .apply { + set(Calendar.HOUR_OF_DAY, 12) + set(Calendar.MINUTE, 0) + set(Calendar.SECOND, 0) + }.timeInMillis, + step = 0, + ) } private lateinit var context: Context @@ -67,7 +86,10 @@ class AlarmManagerServiceTest : RobolectricTest() { notificationManager = mockk(relaxed = true) every { context.getSystemService() } returns alarmManager every { context.getSystemService() } returns notificationManager + reviewReminder = ReviewReminder.createReviewReminder(time = ReviewReminderTime(20, 0)) + reviewReminder.latestNotifTime = mockTime.intTimeMS() // Ensure the reminder is ready to have its future instance scheduled + TimeManager.resetWith(mockTime) ReviewRemindersDatabase.remindersSharedPrefs.edit { clear() } } @@ -91,7 +113,7 @@ class AlarmManagerServiceTest : RobolectricTest() { alarmManager.setWindow( AlarmManager.RTC_WAKEUP, expectedSchedulingTime.timeInMillis, - 10.minutes.inWholeMilliseconds, + AlarmManagerService.WINDOW_LENGTH_MS, any(), ) } @@ -111,7 +133,7 @@ class AlarmManagerServiceTest : RobolectricTest() { alarmManager.setWindow( AlarmManager.RTC_WAKEUP, expectedSchedulingTime.timeInMillis, - 10.minutes.inWholeMilliseconds, + AlarmManagerService.WINDOW_LENGTH_MS, any(), ) } @@ -135,38 +157,192 @@ class AlarmManagerServiceTest : RobolectricTest() { verify { alarmManager.cancel(pendingIntent) } } + /** + * A single test case for alarm scheduling testing. + * @see scheduleAllNotificationsTest + */ + private data class ScheduleAllNotificationsTestCase( + val reminder: ReviewReminder, + val shouldImmediatelySetAlarm: Boolean, + val shouldImmediatelyFireNotif: Boolean, + val latestNotifTime: EpochMilliseconds? = null, + ) + + /** + * Helper function that tries scheduling multiple reminders and verifies the outcome for each. + */ + private fun scheduleAllNotificationsTest(vararg testCases: ScheduleAllNotificationsTestCase) { + require(testCases.map { it.reminder.time }.toSet().size == testCases.size) { + "All test cases must have unique reminder times to facilitate alarm scheduling verification" + } + + testCases.forEach { case -> + if (case.latestNotifTime != null) { + case.reminder.latestNotifTime = case.latestNotifTime + } + } + val previousLatestNotifTimes = testCases.associate { it.reminder.id to it.reminder.latestNotifTime } + + ReviewRemindersDatabase.storeReminders(*testCases.map { it.reminder }.toTypedArray()) + val currentTimestamp = mockTime.calendar().clone() as Calendar + + AlarmManagerService.scheduleAllNotifications(context) + + testCases.forEach { case -> + val expectedSchedulingTime = mockTime.calendar().clone() as Calendar + expectedSchedulingTime.apply { + set(Calendar.HOUR_OF_DAY, case.reminder.time.hour) + set(Calendar.MINUTE, case.reminder.time.minute) + set(Calendar.SECOND, 0) + if (before(currentTimestamp)) { + add(Calendar.DAY_OF_YEAR, 1) + } + } + + val alarmSettingCallsExpected = if (case.shouldImmediatelySetAlarm) 1 else 0 + verify(exactly = alarmSettingCallsExpected) { + alarmManager.setWindow( + AlarmManager.RTC_WAKEUP, + expectedSchedulingTime.timeInMillis, + 10.minutes.inWholeMilliseconds, + any(), + ) + } + } + + val (testCasesWithFiring, testCasesWithoutFiring) = testCases.partition { it.shouldImmediatelyFireNotif } + val expectedFired = testCasesWithFiring.map { it.reminder }.toSet() + val expectedNotFired = testCasesWithoutFiring.map { it.reminder }.toSet() + + val capturedIntents = mutableListOf() + verify(exactly = expectedFired.size) { + context.sendBroadcast(capture(capturedIntents)) + } + val actuallyFired = + capturedIntents + .map { intent -> + BundleCompat.getParcelable( + intent.extras!!, + EXTRA_REVIEW_REMINDER, + ReviewReminder::class.java, + )!! + }.toSet() + val actuallyFiredIds = actuallyFired.map { it.id }.toSet() + val notFired = testCases.map { it.reminder }.filterNot { it.id in actuallyFiredIds }.toSet() + + // The actually fired reminders have a modified latestNotifTime, so we can't directly compare the reminder objects + assertThat((actuallyFired + notFired).map { it.id }.toSet(), equalTo(testCases.map { it.reminder.id }.toSet())) + assertThat(expectedFired.map { it.id }.toSet(), equalTo(actuallyFired.map { it.id }.toSet())) + + // But we can compare unfired reminders directly since they should be unchanged + assertThat(expectedNotFired, equalTo(notFired)) + + // Validate latestNotifTime has changed for fired reminders + val expectedFiredCreationTimes = expectedFired.associate { it.id to previousLatestNotifTimes[it.id]!! } + val notificationTimes = actuallyFired.associate { it.id to it.latestNotifTime } + assertThat(expectedFiredCreationTimes.keys.toSet(), equalTo(notificationTimes.keys.toSet())) + expectedFiredCreationTimes.forEach { (id, createdAt) -> + val notifTime = notificationTimes[id]!! + assertThat(notifTime > createdAt, equalTo(true)) + } + + // Validate stored reminders + val stored = ReviewRemindersDatabase.getAllAppWideReminders() + ReviewRemindersDatabase.getAllDeckSpecificReminders() + val (storedFired, storedNotFired) = stored.getRemindersList().partition { it.id in actuallyFired.map { r -> r.id } } + assertThat(storedFired.toSet(), equalTo(actuallyFired)) + assertThat(storedNotFired.toSet(), equalTo(notFired)) + } + @Test fun `scheduleAllNotifications schedules reminders for all enabled reminders in database`() = runTest { val did1 = addDeck("Deck1") val did2 = addDeck("Deck2") - val reminder1 = - ReviewReminder.createReviewReminder( - time = ReviewReminderTime(9, 0), - scope = ReviewReminderScope.DeckSpecific(did1), - ) - val reminder2 = - ReviewReminder.createReviewReminder( - time = ReviewReminderTime(10, 0), - scope = ReviewReminderScope.DeckSpecific(did2), - ) - val reminder3 = ReviewReminder.createReviewReminder(time = ReviewReminderTime(11, 0)) - val disabledReminder = - ReviewReminder.createReviewReminder( - time = ReviewReminderTime(11, 0), - enabled = false, - ) - ReviewRemindersDatabase.editRemindersForDeck(did1) { ReviewReminderGroup(ReviewReminderId(0) to reminder1) } - ReviewRemindersDatabase.editRemindersForDeck(did2) { ReviewReminderGroup(ReviewReminderId(1) to reminder2) } - ReviewRemindersDatabase.editAllAppWideReminders { - ReviewReminderGroup( - ReviewReminderId(2) to reminder3, - ReviewReminderId(3) to disabledReminder, - ) - } + scheduleAllNotificationsTest( + ScheduleAllNotificationsTestCase( + reminder = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(9, 0), + scope = ReviewReminderScope.DeckSpecific(did1), + ), + shouldImmediatelySetAlarm = true, + shouldImmediatelyFireNotif = false, + ), + ScheduleAllNotificationsTestCase( + reminder = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(10, 0), + scope = ReviewReminderScope.DeckSpecific(did2), + ), + shouldImmediatelySetAlarm = true, + shouldImmediatelyFireNotif = false, + ), + ScheduleAllNotificationsTestCase( + reminder = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(11, 0), + ), + shouldImmediatelySetAlarm = true, + shouldImmediatelyFireNotif = false, + ), + ScheduleAllNotificationsTestCase( + reminder = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(12, 0), + enabled = false, + ), + shouldImmediatelySetAlarm = false, + shouldImmediatelyFireNotif = false, + ), + ) + } - AlarmManagerService.scheduleAllNotifications(context) - verify(exactly = 3) { alarmManager.setWindow(AlarmManager.RTC_WAKEUP, any(), 10.minutes.inWholeMilliseconds, any()) } + @Test + fun `scheduleAllNotifications immediately fires notification for reminders which missed scheduled firings`() = + runTest { + val did1 = addDeck("Deck1") + scheduleAllNotificationsTest( + ScheduleAllNotificationsTestCase( + reminder = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(11, 58), // Last scheduled at 2 minutes earlier + scope = ReviewReminderScope.DeckSpecific(did1), + ), + shouldImmediatelySetAlarm = false, + shouldImmediatelyFireNotif = true, // Should fire immediately, because... + latestNotifTime = mockTime.intTimeMS() - 10.minutes.inWholeMilliseconds, // ...latest firing was 10 minutes ago + ), + ScheduleAllNotificationsTestCase( + reminder = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(11, 59), // Last scheduled at 1 minute earlier + scope = ReviewReminderScope.Global, + ), + shouldImmediatelySetAlarm = true, + shouldImmediatelyFireNotif = false, // Should not fire immediately, because... + latestNotifTime = mockTime.intTimeMS(), // ...a notification just fired + ), + ScheduleAllNotificationsTestCase( + reminder = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(12, 1), // Last scheduled almost a full day ago + scope = ReviewReminderScope.Global, + ), + shouldImmediatelySetAlarm = true, + shouldImmediatelyFireNotif = false, // Should not fire immediately, because... + latestNotifTime = mockTime.intTimeMS() - 10.minutes.inWholeMilliseconds, // ...latest firing was 10 minutes ago + ), + ScheduleAllNotificationsTestCase( + reminder = + ReviewReminder.createReviewReminder( + time = ReviewReminderTime(12, 2), // Last scheduled almost a full day ago + scope = ReviewReminderScope.DeckSpecific(did1), + ), + shouldImmediatelySetAlarm = false, + shouldImmediatelyFireNotif = true, // Should fire immediately, because... + latestNotifTime = mockTime.intTimeMS() - 2.days.inWholeMilliseconds, // ...latest firing was 2 days ago + ), + ) } @Test @@ -183,7 +359,7 @@ class AlarmManagerServiceTest : RobolectricTest() { alarmManager.setWindow( AlarmManager.RTC_WAKEUP, mockTime.intTimeMS() + 5.minutes.inWholeMilliseconds, - 10.minutes.inWholeMilliseconds, + AlarmManagerService.WINDOW_LENGTH_MS, any(), ) } diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt index 8ca44c23ab7c..8efe0e28484f 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt @@ -28,14 +28,16 @@ import com.ichi2.anki.RobolectricTest import com.ichi2.anki.common.time.MockTime import com.ichi2.anki.common.time.TimeManager import com.ichi2.anki.libanki.DeckId +import com.ichi2.anki.libanki.EpochMilliseconds import com.ichi2.anki.reviewreminders.ReviewReminder import com.ichi2.anki.reviewreminders.ReviewReminderCardTriggerThreshold -import com.ichi2.anki.reviewreminders.ReviewReminderGroup -import com.ichi2.anki.reviewreminders.ReviewReminderId -import com.ichi2.anki.reviewreminders.ReviewReminderScope +import com.ichi2.anki.reviewreminders.ReviewReminderScope.DeckSpecific +import com.ichi2.anki.reviewreminders.ReviewReminderScope.Global import com.ichi2.anki.reviewreminders.ReviewReminderTime import com.ichi2.anki.reviewreminders.ReviewRemindersDatabase import com.ichi2.anki.settings.Prefs +import com.ichi2.testutils.ext.storeReminders +import io.mockk.CapturingSlot import io.mockk.every import io.mockk.mockk import io.mockk.mockkObject @@ -45,6 +47,7 @@ import io.mockk.unmockkAll import io.mockk.verify import org.hamcrest.MatcherAssert.assertThat import org.hamcrest.Matchers.equalTo +import org.hamcrest.Matchers.not import org.junit.After import org.junit.Before import org.junit.Test @@ -54,8 +57,8 @@ import kotlin.time.Duration.Companion.days @RunWith(AndroidJUnit4::class) class NotificationServiceTest : RobolectricTest() { companion object { - private val yesterday = MockTime(TimeManager.time.intTimeMS() - 1.days.inWholeMilliseconds) - private val today = MockTime(TimeManager.time.intTimeMS()) + private val yesterday = MockTime(TimeManager.time.intTimeMS() - 1.days.inWholeMilliseconds, step = 1000) + private val today = MockTime(TimeManager.time.intTimeMS(), step = 1000) } private lateinit var context: Context @@ -81,192 +84,126 @@ class NotificationServiceTest : RobolectricTest() { ReviewRemindersDatabase.remindersSharedPrefs.edit { clear() } } - private fun createAndSaveDummyDeckSpecificReminder(did: DeckId): ReviewReminder { - val reviewReminder = createTestReminder(deckId = did, thresholdInt = 1) - ReviewRemindersDatabase.editRemindersForDeck(did) { ReviewReminderGroup(ReviewReminderId(0) to reviewReminder) } - return reviewReminder - } - - private fun createAndSaveDummyAppWideReminder(): ReviewReminder { - val reviewReminder = createTestReminder(thresholdInt = 1) - ReviewRemindersDatabase.editAllAppWideReminders { ReviewReminderGroup(ReviewReminderId(1) to reviewReminder) } - return reviewReminder - } - - private fun triggerDummyReminderNotification(reviewReminder: ReviewReminder) { - val intent = - NotificationService.getIntent( - context, - reviewReminder, - NotificationService.NotificationServiceAction.ScheduleRecurringNotifications, - ) - NotificationService().onReceive(context, intent) - } - @Test fun `onReceive with less cards than card threshold should not fire notification but schedule next`() = runTest { - val did1 = addDeck("Deck", setAsSelected = true) - addNotes(2).forEach { - it.firstCard().update { did = did1 } - } + val did1 = addDeck("Deck", setAsSelected = true).withNotes(count = 2) val reviewReminderDeckSpecific = createTestReminder(deckId = did1, thresholdInt = 3) val reviewReminderAppWide = createTestReminder(thresholdInt = 3) - ReviewRemindersDatabase.editRemindersForDeck(did1) { ReviewReminderGroup(ReviewReminderId(0) to reviewReminderDeckSpecific) } - ReviewRemindersDatabase.editAllAppWideReminders { ReviewReminderGroup(ReviewReminderId(1) to reviewReminderAppWide) } + val deckSpecificCreationTime = reviewReminderDeckSpecific.latestNotifTime + val appWideCreationTime = reviewReminderAppWide.latestNotifTime + ReviewRemindersDatabase.storeReminders(reviewReminderDeckSpecific, reviewReminderAppWide) triggerDummyReminderNotification(reviewReminderDeckSpecific) triggerDummyReminderNotification(reviewReminderAppWide) - verify(exactly = 0) { notificationManager.notify(any(), any(), any()) } - verify( - exactly = 1, - ) { AlarmManagerService.scheduleReviewReminderNotification(context, reviewReminderDeckSpecific) } - verify( - exactly = 1, - ) { AlarmManagerService.scheduleReviewReminderNotification(context, reviewReminderAppWide) } + verifyNoNotifsSent() + verifyNextNotifScheduled(reviewReminderDeckSpecific) + verifyNextNotifScheduled(reviewReminderAppWide) + reviewReminderDeckSpecific.verifyLatestNotifTime(previousTime = deckSpecificCreationTime, shouldHaveUpdated = true) + reviewReminderAppWide.verifyLatestNotifTime(previousTime = appWideCreationTime, shouldHaveUpdated = true) } @Test fun `onReceive with happy path for single deck should fire notification and schedule next`() = runTest { - val did1 = addDeck("Deck", setAsSelected = true) - addNotes(2).forEach { - it.firstCard().update { did = did1 } - } + val did1 = addDeck("Deck", setAsSelected = true).withNotes(count = 2) val reviewReminder = createAndSaveDummyDeckSpecificReminder(did1) + val creationTime = reviewReminder.latestNotifTime triggerDummyReminderNotification(reviewReminder) - verify( - exactly = 1, - ) { notificationManager.notify(NotificationService.REVIEW_REMINDER_NOTIFICATION_TAG, reviewReminder.id.value, any()) } - verify( - exactly = 1, - ) { AlarmManagerService.scheduleReviewReminderNotification(context, reviewReminder) } + verifyNotifSent(reviewReminder) + verifyNextNotifScheduled(reviewReminder) + reviewReminder.verifyLatestNotifTime(previousTime = creationTime, shouldHaveUpdated = true) } @Test fun `onReceive with happy path for global reminder should fire notification and schedule next`() = runTest { - val did1 = addDeck("Deck1") - val did2 = addDeck("Deck2") - addNotes(2).forEach { - it.firstCard().update { did = did1 } - } - addNotes(2).forEach { - it.firstCard().update { did = did2 } - } + addDeck("Deck1").withNotes(count = 2) + addDeck("Deck2").withNotes(count = 2) val reviewReminder = createTestReminder(thresholdInt = 4) + val creationTime = reviewReminder.latestNotifTime triggerDummyReminderNotification(reviewReminder) - verify( - exactly = 1, - ) { notificationManager.notify(NotificationService.REVIEW_REMINDER_NOTIFICATION_TAG, reviewReminder.id.value, any()) } - verify( - exactly = 1, - ) { AlarmManagerService.scheduleReviewReminderNotification(context, reviewReminder) } + verifyNotifSent(reviewReminder) + verifyNextNotifScheduled(reviewReminder) + reviewReminder.verifyLatestNotifTime(previousTime = creationTime, shouldHaveUpdated = true) } @Test fun `onReceive with non-existent deck should not fire notification but schedule next`() = runTest { val reviewReminder = createTestReminder(deckId = 9999) + val creationTime = reviewReminder.latestNotifTime triggerDummyReminderNotification(reviewReminder) - verify(exactly = 0) { notificationManager.notify(any(), any(), any()) } - verify( - exactly = 1, - ) { AlarmManagerService.scheduleReviewReminderNotification(context, reviewReminder) } + verifyNoNotifsSent() + verifyNextNotifScheduled(reviewReminder) + reviewReminder.verifyLatestNotifTime(previousTime = creationTime, shouldHaveUpdated = true) } @Test fun `onReceive with reviews today and onlyNotifyIfNoReviews is true should not fire notification`() = runTest { - val did1 = addDeck("Deck", setAsSelected = true) - addNotes(1).forEach { - it.firstCard().update { did = did1 } - } + val did1 = addDeck("Deck", setAsSelected = true).withNote() col.sched.answerCard(col.sched.card!!, CardAnswer.Rating.GOOD) val reviewReminderDeckSpecific = createTestReminder(deckId = did1, thresholdInt = 1, onlyNotifyIfNoReviews = true) val reviewReminderAppWide = createTestReminder(thresholdInt = 1, onlyNotifyIfNoReviews = true) - ReviewRemindersDatabase.editRemindersForDeck(did1) { ReviewReminderGroup(ReviewReminderId(0) to reviewReminderDeckSpecific) } - ReviewRemindersDatabase.editAllAppWideReminders { ReviewReminderGroup(ReviewReminderId(1) to reviewReminderAppWide) } + ReviewRemindersDatabase.storeReminders(reviewReminderDeckSpecific, reviewReminderAppWide) triggerDummyReminderNotification(reviewReminderDeckSpecific) triggerDummyReminderNotification(reviewReminderAppWide) - verify(exactly = 0) { notificationManager.notify(any(), any(), any()) } + + verifyNoNotifsSent() } @Test fun `onReceive with no reviews ever and onlyNotifyIfNoReviews is true should fire notification`() = runTest { - val did1 = addDeck("Deck", setAsSelected = true) - addNotes(1).forEach { - it.firstCard().update { did = did1 } - } + val did1 = addDeck("Deck", setAsSelected = true).withNote() val reviewReminderDeckSpecific = createTestReminder(deckId = did1, thresholdInt = 1, onlyNotifyIfNoReviews = true) val reviewReminderAppWide = createTestReminder(thresholdInt = 1, onlyNotifyIfNoReviews = true) - ReviewRemindersDatabase.editRemindersForDeck(did1) { ReviewReminderGroup(ReviewReminderId(0) to reviewReminderDeckSpecific) } - ReviewRemindersDatabase.editAllAppWideReminders { ReviewReminderGroup(ReviewReminderId(1) to reviewReminderAppWide) } + ReviewRemindersDatabase.storeReminders(reviewReminderDeckSpecific, reviewReminderAppWide) triggerDummyReminderNotification(reviewReminderDeckSpecific) triggerDummyReminderNotification(reviewReminderAppWide) - verify( - exactly = 1, - ) { - notificationManager.notify( - NotificationService.REVIEW_REMINDER_NOTIFICATION_TAG, - reviewReminderDeckSpecific.id.value, - any(), - ) - } - verify( - exactly = 1, - ) { notificationManager.notify(NotificationService.REVIEW_REMINDER_NOTIFICATION_TAG, reviewReminderAppWide.id.value, any()) } + + verifyNotifSent(reviewReminderDeckSpecific) + verifyNotifSent(reviewReminderAppWide) } @Test fun `onReceive with review yesterday but none today and onlyNotifyIfNoReviews is true should fire notification`() = runTest { TimeManager.resetWith(yesterday) // Wind back time and perform the review - val did1 = addDeck("Deck", setAsSelected = true) - addNotes(1).forEach { - it.firstCard().update { did = did1 } - } + val did1 = addDeck("Deck", setAsSelected = true).withNote() col.sched.answerCard(col.sched.card!!, CardAnswer.Rating.GOOD) TimeManager.resetWith(today) // Reset time to present - val reviewReminderDeckSpecific = createTestReminder(deckId = did1, thresholdInt = 1, onlyNotifyIfNoReviews = true) - val reviewReminderAppWide = createTestReminder(thresholdInt = 1, onlyNotifyIfNoReviews = true) - ReviewRemindersDatabase.editRemindersForDeck(did1) { ReviewReminderGroup(ReviewReminderId(0) to reviewReminderDeckSpecific) } - ReviewRemindersDatabase.editAllAppWideReminders { ReviewReminderGroup(ReviewReminderId(1) to reviewReminderAppWide) } + val reviewReminderDeckSpecific = + createTestReminder(deckId = did1, thresholdInt = 1, onlyNotifyIfNoReviews = true) + val reviewReminderAppWide = + createTestReminder(thresholdInt = 1, onlyNotifyIfNoReviews = true) + ReviewRemindersDatabase.storeReminders( + reviewReminderDeckSpecific, + reviewReminderAppWide, + ) triggerDummyReminderNotification(reviewReminderDeckSpecific) triggerDummyReminderNotification(reviewReminderAppWide) - verify( - exactly = 1, - ) { - notificationManager.notify( - NotificationService.REVIEW_REMINDER_NOTIFICATION_TAG, - reviewReminderDeckSpecific.id.value, - any(), - ) - } - verify( - exactly = 1, - ) { notificationManager.notify(NotificationService.REVIEW_REMINDER_NOTIFICATION_TAG, reviewReminderAppWide.id.value, any()) } + + verifyNotifSent(reviewReminderDeckSpecific) + verifyNotifSent(reviewReminderAppWide) } @Test fun `onReceive with onlyNotifyIfNoReviews is false should always fire notification`() = runTest { - val did1 = addDeck("Deck", setAsSelected = true) - addNotes(1).forEach { - it.firstCard().update { did = did1 } - } + val did1 = addDeck("Deck", setAsSelected = true).withNote() val reviewReminderDeckSpecific = createAndSaveDummyDeckSpecificReminder(did1) val reviewReminderAppWide = createAndSaveDummyAppWideReminder() @@ -276,136 +213,71 @@ class NotificationServiceTest : RobolectricTest() { triggerDummyReminderNotification(reviewReminderDeckSpecific) triggerDummyReminderNotification(reviewReminderAppWide) - verify( - exactly = 2, - ) { - notificationManager.notify( - NotificationService.REVIEW_REMINDER_NOTIFICATION_TAG, - reviewReminderDeckSpecific.id.value, - any(), - ) - } - verify( - exactly = 2, - ) { notificationManager.notify(NotificationService.REVIEW_REMINDER_NOTIFICATION_TAG, reviewReminderAppWide.id.value, any()) } + verifyNotifSent(reviewReminderDeckSpecific, times = 2) + verifyNotifSent(reviewReminderAppWide, times = 2) } @Test fun `onReceive with blocked collection should not fire notification but schedule next`() = runTest { - val did1 = addDeck("Deck", setAsSelected = true) - addNotes(2).forEach { - it.firstCard().update { did = did1 } - } + val did1 = addDeck("Deck", setAsSelected = true).withNotes(count = 2) val reviewReminderDeckSpecific = createAndSaveDummyDeckSpecificReminder(did1) val reviewReminderAppWide = createAndSaveDummyAppWideReminder() + val deckSpecificCreationTime = reviewReminderDeckSpecific.latestNotifTime + val appWideCreationTime = reviewReminderAppWide.latestNotifTime CollectionManager.emulatedOpenFailure = CollectionManager.CollectionOpenFailure.LOCKED triggerDummyReminderNotification(reviewReminderDeckSpecific) triggerDummyReminderNotification(reviewReminderAppWide) - verify(exactly = 0) { notificationManager.notify(any(), any(), any()) } - verify( - exactly = 1, - ) { AlarmManagerService.scheduleReviewReminderNotification(context, reviewReminderDeckSpecific) } - verify( - exactly = 1, - ) { AlarmManagerService.scheduleReviewReminderNotification(context, reviewReminderAppWide) } + verifyNoNotifsSent() + verifyNextNotifScheduled(reviewReminderDeckSpecific) + verifyNextNotifScheduled(reviewReminderAppWide) + reviewReminderDeckSpecific.verifyLatestNotifTime(previousTime = deckSpecificCreationTime, shouldHaveUpdated = true) + reviewReminderAppWide.verifyLatestNotifTime(previousTime = appWideCreationTime, shouldHaveUpdated = true) } @Test fun `onReceive with snoozed notification should fire notification but not schedule next`() = runTest { - val did1 = addDeck("Deck", setAsSelected = true) - addNotes(2).forEach { - it.firstCard().update { did = did1 } - } + val did1 = addDeck("Deck", setAsSelected = true).withNotes(count = 2) val reviewReminderDeckSpecific = createAndSaveDummyDeckSpecificReminder(did1) val reviewReminderAppWide = createAndSaveDummyAppWideReminder() + val deckSpecificCreationTime = reviewReminderDeckSpecific.latestNotifTime + val appWideCreationTime = reviewReminderAppWide.latestNotifTime - val intentDeckSpecific = - NotificationService.getIntent( - context, - reviewReminderDeckSpecific, - NotificationService.NotificationServiceAction.SnoozeNotification, - ) - val intentAppWide = - NotificationService.getIntent( - context, - reviewReminderAppWide, - NotificationService.NotificationServiceAction.SnoozeNotification, - ) + val intentDeckSpecific = reviewReminderDeckSpecific.getNotifIntent(NotifIntent.SNOOZE) + val intentAppWide = reviewReminderAppWide.getNotifIntent(NotifIntent.SNOOZE) NotificationService().onReceive(context, intentDeckSpecific) NotificationService().onReceive(context, intentAppWide) - verify( - exactly = 1, - ) { - notificationManager.notify( - NotificationService.REVIEW_REMINDER_NOTIFICATION_TAG, - reviewReminderDeckSpecific.id.value, - any(), - ) - } - verify( - exactly = 1, - ) { notificationManager.notify(NotificationService.REVIEW_REMINDER_NOTIFICATION_TAG, reviewReminderAppWide.id.value, any()) } - verify(exactly = 0) { AlarmManagerService.scheduleReviewReminderNotification(context, any()) } + verifyNotifSent(reviewReminderDeckSpecific) + verifyNotifSent(reviewReminderAppWide) + verifyNextNotifNotScheduled() + reviewReminderDeckSpecific.verifyLatestNotifTime(previousTime = deckSpecificCreationTime, shouldHaveUpdated = false) + reviewReminderAppWide.verifyLatestNotifTime(previousTime = appWideCreationTime, shouldHaveUpdated = false) } @Test fun `snooze actions of different notifications and different intervals should be different`() = runTest { - val did1 = addDeck("Deck1") - val did2 = addDeck("Deck2") - addNotes(2).forEach { - it.firstCard().update { did = did1 } - } - addNotes(2).forEach { - it.firstCard().update { did = did2 } - } + val did1 = addDeck("Deck1").withNotes(count = 2) + val did2 = addDeck("Deck2").withNotes(count = 2) val reviewReminderOne = createTestReminder(deckId = did1, thresholdInt = 1) val reviewReminderTwo = createTestReminder(deckId = did2, thresholdInt = 1) - ReviewRemindersDatabase.editRemindersForDeck(did1) { ReviewReminderGroup(ReviewReminderId(0) to reviewReminderOne) } - ReviewRemindersDatabase.editRemindersForDeck(did2) { ReviewReminderGroup(ReviewReminderId(1) to reviewReminderTwo) } + ReviewRemindersDatabase.storeReminders(reviewReminderOne, reviewReminderTwo) val slotOne = slot() val slotTwo = slot() - val intentOne = - NotificationService.getIntent( - context, - reviewReminderOne, - NotificationService.NotificationServiceAction.ScheduleRecurringNotifications, - ) + val intentOne = reviewReminderOne.getNotifIntent(NotifIntent.RECURRING) NotificationService().onReceive(context, intentOne) - val intentTwo = - NotificationService.getIntent( - context, - reviewReminderTwo, - NotificationService.NotificationServiceAction.ScheduleRecurringNotifications, - ) + val intentTwo = reviewReminderTwo.getNotifIntent(NotifIntent.RECURRING) NotificationService().onReceive(context, intentTwo) - verify( - exactly = 1, - ) { - notificationManager.notify( - NotificationService.REVIEW_REMINDER_NOTIFICATION_TAG, - reviewReminderOne.id.value, - capture(slotOne), - ) - } - verify( - exactly = 1, - ) { - notificationManager.notify( - NotificationService.REVIEW_REMINDER_NOTIFICATION_TAG, - reviewReminderTwo.id.value, - capture(slotTwo), - ) - } + verifyNotifSent(reviewReminderOne, slot = slotOne) + verifyNotifSent(reviewReminderTwo, slot = slotTwo) val snoozeIntents = setOf( @@ -417,12 +289,25 @@ class NotificationServiceTest : RobolectricTest() { assertThat(snoozeIntents.size, equalTo(4)) } + private fun createAndSaveDummyDeckSpecificReminder(did: DeckId): ReviewReminder { + val reviewReminder = createTestReminder(deckId = did, thresholdInt = 1) + ReviewRemindersDatabase.storeReminders(reviewReminder) + return reviewReminder + } + + private fun createAndSaveDummyAppWideReminder(): ReviewReminder { + val reviewReminder = createTestReminder(thresholdInt = 1) + ReviewRemindersDatabase.storeReminders(reviewReminder) + return reviewReminder + } + + private fun triggerDummyReminderNotification(reviewReminder: ReviewReminder) { + val intent = reviewReminder.getNotifIntent(NotifIntent.RECURRING) + NotificationService().onReceive(context, intent) + } + /** * Helper method for creating a review reminder to minimize verbosity in this file. - * - * @param deckId If specified, the reminder will be deck-specific to this deck ID. If null, it will be app-wide. - * @param thresholdInt The card trigger threshold as an integer. - * @param onlyNotifyIfNoReviews Whether the reminder should only notify if there are no reviews today. */ private fun createTestReminder( deckId: DeckId? = null, @@ -431,7 +316,71 @@ class NotificationServiceTest : RobolectricTest() { ) = ReviewReminder.createReviewReminder( time = ReviewReminderTime(hour = 12, minute = 0), cardTriggerThreshold = ReviewReminderCardTriggerThreshold(thresholdInt), - scope = if (deckId != null) ReviewReminderScope.DeckSpecific(deckId) else ReviewReminderScope.Global, + scope = if (deckId != null) DeckSpecific(deckId) else Global, onlyNotifyIfNoReviews = onlyNotifyIfNoReviews, ) + + private fun verifyNoNotifsSent() { + verify(exactly = 0) { notificationManager.notify(any(), any(), any()) } + } + + private fun verifyNotifSent( + reminder: ReviewReminder, + times: Int = 1, + slot: CapturingSlot? = null, + ) { + if (slot != null) { + verify( + exactly = times, + ) { notificationManager.notify(NotificationService.REVIEW_REMINDER_NOTIFICATION_TAG, reminder.id.value, capture(slot)) } + } else { + verify( + exactly = times, + ) { notificationManager.notify(NotificationService.REVIEW_REMINDER_NOTIFICATION_TAG, reminder.id.value, any()) } + } + } + + private fun verifyNextNotifNotScheduled() { + verify(exactly = 0) { AlarmManagerService.scheduleReviewReminderNotification(any(), any()) } + } + + private fun verifyNextNotifScheduled(reminder: ReviewReminder) { + verify( + exactly = 1, + ) { AlarmManagerService.scheduleReviewReminderNotification(context, reminder) } + } + + private fun ReviewReminder.verifyLatestNotifTime( + previousTime: EpochMilliseconds, + shouldHaveUpdated: Boolean, + ) { + val storedReminderGroup = + when (scope) { + is DeckSpecific -> ReviewRemindersDatabase.getRemindersForDeck(scope.did) + is Global -> ReviewRemindersDatabase.getAllAppWideReminders() + } + val storedReminder = storedReminderGroup.getRemindersList().find { it.id == id }!! + + if (shouldHaveUpdated) { + assertThat(previousTime, not(equalTo(storedReminder.latestNotifTime))) + } else { + assertThat(previousTime, equalTo(storedReminder.latestNotifTime)) + } + } + + /** + * Convenience enum class to minimize verbosity in test methods when using [getNotifIntent]. + */ + private enum class NotifIntent { + RECURRING, + SNOOZE, + } + + private fun ReviewReminder.getNotifIntent(action: NotifIntent) = + when (action) { + NotifIntent.RECURRING -> NotificationService.NotificationServiceAction.ScheduleRecurringNotifications + NotifIntent.SNOOZE -> NotificationService.NotificationServiceAction.SnoozeNotification + }.let { action -> + NotificationService.getIntent(context, this, action) + } } diff --git a/AnkiDroid/src/test/java/com/ichi2/testutils/ext/ReviewRemindersDatabase.kt b/AnkiDroid/src/test/java/com/ichi2/testutils/ext/ReviewRemindersDatabase.kt new file mode 100644 index 000000000000..f9b507a409ed --- /dev/null +++ b/AnkiDroid/src/test/java/com/ichi2/testutils/ext/ReviewRemindersDatabase.kt @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2026 Eric Li + * + * This program is free software; you can redistribute it and/or modify it under + * the terms of the GNU General Public License as published by the Free Software + * Foundation; either version 3 of the License, or (at your option) any later + * version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT ANY + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A + * PARTICULAR PURPOSE. See the GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see . + */ + +package com.ichi2.testutils.ext + +import com.ichi2.anki.reviewreminders.ReviewReminder +import com.ichi2.anki.reviewreminders.ReviewReminderGroup +import com.ichi2.anki.reviewreminders.ReviewReminderScope.DeckSpecific +import com.ichi2.anki.reviewreminders.ReviewReminderScope.Global +import com.ichi2.anki.reviewreminders.ReviewRemindersDatabase + +fun ReviewRemindersDatabase.storeReminders(vararg reminders: ReviewReminder) { + reminders.forEach { reminder -> + when (reminder.scope) { + is DeckSpecific -> { + editRemindersForDeck(reminder.scope.did) { reminders -> + reminders + ReviewReminderGroup(reminder.id to reminder) + } + } + is Global -> { + editAllAppWideReminders { reminders -> + reminders + ReviewReminderGroup(reminder.id to reminder) + } + } + } + } +} diff --git a/libanki/testutils/src/main/java/com/ichi2/anki/libanki/testutils/AnkiTest.kt b/libanki/testutils/src/main/java/com/ichi2/anki/libanki/testutils/AnkiTest.kt index 9e0ef2f52dbc..000f62a9e9c6 100644 --- a/libanki/testutils/src/main/java/com/ichi2/anki/libanki/testutils/AnkiTest.kt +++ b/libanki/testutils/src/main/java/com/ichi2/anki/libanki/testutils/AnkiTest.kt @@ -229,6 +229,43 @@ interface AnkiTest { front: String = "Front", ): List = List(count) { addBasicNote(front = front) } + /** + * Adds [count] notes into the specified [queueType] of the provided deck. + */ + fun addNoteToDeck( + deckId: DeckId, + count: Int = 1, + queueType: QueueType = QueueType.New, + ) = addNotes(count).forEach { + it.firstCard().update { + did = deckId + queue = queueType + } + } + + /** + * Convenience method for chaining [addDeck] and [addNoteToDeck]. + * + * Usage: `val deckId = addDeck("My Deck").withNotes(count = 5, queueType = QueueType.New)` + */ + fun DeckId.withNotes( + count: Int, + queueType: QueueType = QueueType.New, + ): DeckId = + this.apply { + addNoteToDeck(this, count = count, queueType = queueType) + } + + /** + * Convenience method for chaining [addDeck] and [addNoteToDeck]. + * + * Usage: `val deckId = addDeck("My Deck").withNote(queueType = QueueType.New)` + */ + fun DeckId.withNote(queueType: QueueType = QueueType.New): DeckId = + this.apply { + addNoteToDeck(this, count = 1, queueType = queueType) + } + fun Note.moveToDeck( deckName: String, createDeckIfMissing: Boolean = true,