feat(reminders): database overhaul#20325
Conversation
...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.
ericli3690
added
the
GSoC
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive database overhaul for the review reminders feature, addressing critical bugs related to schema migration and adding support for detecting missed notifications. The changes fix issue #20163 where users upgrading from earlier versions encountered deserialization errors due to the missing onlyNotifyIfNoReviews field, and issue #20170 by adding extensive regression tests.
Changes:
- Implements schema migration chain (V1 → V2 → V3) with proper fallback handling for corrupted data
- Adds
ReviewReminderGroupabstraction to replace verbose HashMap usage throughout the codebase - Introduces
latestNotifTimetracking andshouldImmediatelyFire()logic to detect and immediately fire missed notifications when the app is launched - Significantly expands test coverage with 5 new/updated test files covering migration paths, immediate firing logic, and error handling
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| libanki/testutils/src/main/java/com/ichi2/anki/libanki/testutils/AnkiTest.kt | Adds test helper methods withNotes() and withNote() for convenient deck+note creation |
| AnkiDroid/src/test/java/com/ichi2/testutils/ext/ReviewRemindersDatabase.kt | New test extension providing storeReminders() helper for test setup |
| AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt | Refactored tests with new verification helpers and latestNotifTime validation |
| AnkiDroid/src/test/java/com/ichi2/anki/services/AlarmManagerServiceTest.kt | Adds comprehensive tests for immediate notification firing logic |
| AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt | Extensively refactored tests for corruption handling, migration verification, and schema canary tests |
| AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewReminderTest.kt | Adds tests for latestNotifTime and shouldImmediatelyFire() logic |
| AnkiDroid/src/main/res/values/preferences.xml | Adds preference key for storing deserialization errors |
| AnkiDroid/src/main/java/com/ichi2/anki/settings/Prefs.kt | Adds reviewReminderDeserializationErrors preference property |
| AnkiDroid/src/main/java/com/ichi2/anki/services/NotificationService.kt | Updates latestNotifTime when recurring notifications fire |
| AnkiDroid/src/main/java/com/ichi2/anki/services/AlarmManagerService.kt | Implements immediate firing logic for missed notifications |
| AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt | Updates to use ReviewReminderGroup abstraction and improved error handling |
| AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt | Core database changes including migration logic, error handling, and scope validation |
| AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderSchema.kt | New file containing schema version definitions and migration implementations |
| AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderGroup.kt | New abstraction class wrapping HashMap for cleaner API |
| AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt | Adds latestNotifTime field and shouldImmediatelyFire() method |
| AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt | Checks for and displays deserialization errors on app start |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -> { | ||
| // 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]" |
There was a problem hiding this comment.
The error accumulation pattern concatenates error strings directly without any delimiter between them except brackets: Prefs.reviewReminderDeserializationErrors.orEmpty() + "[$errorString]". If multiple errors occur, this will create a continuous string like [error1][error2][error3] which may be difficult to read in the error dialog.
Consider adding newlines or other formatting between errors for better readability, such as: Prefs.reviewReminderDeserializationErrors.orEmpty() + "\n[$errorString]" or using a more structured format like JSON array.
| Prefs.reviewReminderDeserializationErrors = Prefs.reviewReminderDeserializationErrors.orEmpty() + "[$errorString]" | |
| val existingErrors = Prefs.reviewReminderDeserializationErrors.orEmpty() | |
| Prefs.reviewReminderDeserializationErrors = | |
| if (existingErrors.isEmpty()) { | |
| "[$errorString]" | |
| } else { | |
| existingErrors + "\n[$errorString]" | |
| } |
There was a problem hiding this comment.
Unnecessary imo since this dialog should never show up if everything goes according to plan; we mostly just want the text to be copy-pastable to the forums etc. if something bad does end up happening. Readability is not my biggest concern -- reviewers, let me know if you think otherwise
| override fun migrate(): ReviewReminderSchema = | ||
| ReviewReminder.createReviewReminder( | ||
| time = time, | ||
| cardTriggerThreshold = cardTriggerThreshold, | ||
| scope = scope, | ||
| enabled = enabled, | ||
| profileID = profileID, | ||
| onlyNotifyIfNoReviews = onlyNotifyIfNoReviews, | ||
| ) |
There was a problem hiding this comment.
When migrating from V2 to V3, the migrate() method calls ReviewReminder.createReviewReminder() which initializes latestNotifTime to the current time (migration time). This can cause a missed notification if the migration happens after the reminder's scheduled time for today.
For example, if a user has a reminder scheduled for 9:00 AM and upgrades the app at 10:00 AM:
- Migration sets
latestNotifTime = 10:00 AM(current time) shouldImmediatelyFire()calculateslatestScheduledTimestamp = 9:00 AM today- Since
latestNotifTime (10:00 AM) > latestScheduledTimestamp (9:00 AM), it returns false - The reminder will not fire today, even though it should have
Consider initializing latestNotifTime to 0 or a very old timestamp during migration (e.g., TimeManager.time.calendar().apply { add(Calendar.YEAR, -1) }.timeInMillis) to ensure any past-due notifications fire immediately after upgrade.
There was a problem hiding this comment.
A good point, but if we set the latestNotifTime to 0 or 1 year ago or something like that during the migration, then every reminder will fire all at once once the migration is complete. Not sure if that's desirable. In my opinion dropping one notification during an update is fine -- this problem will only affect users who update the app exactly during the 10-minute firing window of a review reminder.
|
|
||
| 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<ReviewReminder> = underlyingMap.values.toList() | ||
|
|
||
| /** | ||
| * Toggles whether a [ReviewReminder] is enabled. | ||
| */ | ||
| fun toggleEnabled(id: ReviewReminderId) { | ||
| underlyingMap[id]?.apply { enabled = !enabled } | ||
| } |
There was a problem hiding this comment.
The ReviewReminderGroup class exposes a mutable underlying HashMap through methods like set(), remove(), and toggleEnabled(). However, this class is not thread-safe, and the review reminder system has multiple points where reminders are read and written (AlarmManagerService, NotificationService, ScheduleReminders UI).
Since SharedPreferences operations are atomic but the read-modify-write pattern used with editRemindersForDeck and editAllAppWideReminders is not, there's potential for race conditions where concurrent updates could overwrite each other. Consider either:
- Adding explicit synchronization around database operations
- Documenting that all access must happen on a single thread
- Making ReviewReminderGroup immutable with copy-on-write semantics
There was a problem hiding this comment.
I personally feel like adding explicit synchronization to the review reminder database file is overkill. Reviewers, please let me know if you disagree!
Previously, ReviewRemindersDatabase used "HashMap<ReviewReminderId, ReviewReminder>" a lot. Arthur suggested making a small abstraction over this to 1. get rid of the verbosity that comes with saying "HashMap<ReviewReminderId, ReviewReminder>" over and over, and 2. limiting the functionality provided by "HashMap<ReviewReminderId, ReviewReminder>" 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.
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.
…ully 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.
A future commit will use `upsertReminder` outside of ScheduleReminders, so they've been moved to ReviewRemindersDatabase.
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.
ericli3690
force-pushed
the
ericli3690-only-notify-migration
branch
from
b6be949 to
d23b368
Compare
1 participant
A bit of a big PR, but in my defense most of this is tests!
Error dialog (this is not going to be very user-facing, if all goes well no end users will ever need to see this dialog):
Purpose / Description
Field 'onlyNotifyIfNoReviews' is required for type with serial name 'com.ichi2.anki.reviewreminders.ReviewReminder', but it was missing#20163 by adding a review reminder schema migration from v1 to v2.HashMap<ReviewReminderId, ReviewReminder>type in ReviewRemindersDatabase into a new interface, ReviewReminderGroup.Fixes
Field 'onlyNotifyIfNoReviews' is required for type with serial name 'com.ichi2.anki.reviewreminders.ReviewReminder', but it was missing#20163Approach
Major reorganization of the ReviewReminderDatabase file. Creating a
ReviewReminderGroupclass was Arthur's idea!For immediately firing missed notifications, the method employed is the standard one after discussing with Mike (i.e. store the timestamp every time a reminder's notifications attempt to fire, if no scheduled firing is detected while the app is scheduling reminders ex. on boot up, then immediately fire). We're not going to provide this as a configurable option like some other notification apps do because instantly firing missed notifications is a good default.
How Has This Been Tested?
Incrementally installing:
onlyNotifyIfNoReviewschange, then these changes, andonlyNotifyIfNoReviewschange, then these changesall works.
Intentionally corrupting the reminders does not block the app, the offending review reminders are just deleted.
Tested on a physical Samsung S23, API 34.
Learning
Ran into a bunch of headaches around testing, Kotlin's deserialization library and type safety. Went through three separate ideas for solving the "opening the app causes a notification to be dropped" bug, got 90% of the way to the optimal solution independently, then asked Mike and realized I should have asked Mike earlier haha
Checklist