diff --git a/app/src/main/kotlin/dev/privacyllc/period/feature/export/ExportMapping.kt b/app/src/main/kotlin/dev/privacyllc/period/feature/export/ExportMapping.kt index 97d3ec1..42db5d6 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/feature/export/ExportMapping.kt +++ b/app/src/main/kotlin/dev/privacyllc/period/feature/export/ExportMapping.kt @@ -21,7 +21,7 @@ import dev.privacyllc.period.core.export.ExportedSettings * - **`onboardingCompleted`** — application state, not a fact about the user. * Nobody's archive is improved by knowing they finished a wizard. * - * `checkInCount` is not on `UserPreferences` at all; it is stored separately and + * The check-in tally is not on `UserPreferences` at all; it is stored separately and * is likewise app state. */ internal fun UserPreferences.toExportedSettings(): ExportedSettings = ExportedSettings( diff --git a/app/src/main/kotlin/dev/privacyllc/period/notifications/NotificationActionHandler.kt b/app/src/main/kotlin/dev/privacyllc/period/notifications/NotificationActionHandler.kt index 860e0c3..deb4366 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/notifications/NotificationActionHandler.kt +++ b/app/src/main/kotlin/dev/privacyllc/period/notifications/NotificationActionHandler.kt @@ -41,10 +41,10 @@ class NotificationActionHandler @Inject constructor( // still opens the app; it just does not guess what she meant. return when (ReminderAction.fromWireName(action)) { ReminderAction.STARTED -> { + // The question is answered. Nothing resets a counter here: the + // count is stored against the period it was asked about, so this + // new start makes the old count read as zero on its own. repository.confirmPeriodStart(today, PeriodRecordSource.NOTIFICATION_CONFIRMATION) - // The question is answered, so the app is willing to ask again - // next cycle rather than staying permanently quiet. - preferences.resetCheckIns() true } diff --git a/app/src/main/kotlin/dev/privacyllc/period/notifications/ReminderCoordinator.kt b/app/src/main/kotlin/dev/privacyllc/period/notifications/ReminderCoordinator.kt index 6032840..53c6875 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/notifications/ReminderCoordinator.kt +++ b/app/src/main/kotlin/dev/privacyllc/period/notifications/ReminderCoordinator.kt @@ -40,22 +40,25 @@ class ReminderCoordinator @Inject constructor( /** * Watch for anything that changes what a reminder should say or when. * - * The two chains are built by [scheduleUpdates] and [checkInResets] rather - * than inline, so a test can hand them a flow that throws. Without that - * seam the only way to reach the `catch` below is a repository failure, - * and `CycleRepository` is final with an internal constructor — which is - * the right shape for a data boundary and the wrong shape for faking. + * [scheduleUpdates] is built rather than inlined so a test can hand it a + * flow that throws. Without that seam the only way to reach the `catch` + * below is a repository failure, and `CycleRepository` is final with an + * internal constructor — which is the right shape for a data boundary and + * the wrong shape for faking. + * + * There used to be a second chain here, resetting the check-in count when a + * period was confirmed. It is gone, and nothing replaced it: the count is + * now stored against the period it was asked about, so a new period starts + * it over by arithmetic instead of by an event. See + * `UserPreferencesRepository.recordCheckIn` — the chain fired its first + * value in every process, including the one WorkManager starts to run the + * reminder, which wiped the count moments before the worker read it. */ fun start(scope: CoroutineScope) { scheduleUpdates( forecastDates = repository.forecast.map { it?.mostLikelyStartDate }, preferences = preferences.preferences, ).launchIn(scope) - - checkInResets( - confirmedStarts = repository.confirmedPeriods - .map { periods -> periods.maxOfOrNull { it.startDate } }, - ).launchIn(scope) } /** @@ -102,31 +105,10 @@ class ReminderCoordinator @Inject constructor( .onEach { (_, time, enabled) -> apply(time, enabled) } .catch { } - /** - * A confirmed period answers the question the check-ins were asking, so the - * count resets and the app is willing to ask again next cycle. - * - * Without this, somebody who ignored three check-ins once would never be - * asked again — the stopping rule would become permanent. - * - * Catches for the same reason as [scheduleUpdates], and separately from it: - * one collection failing must not stop the other, which is the whole point - * of them being two chains rather than one. - */ - internal fun checkInResets(confirmedStarts: Flow): Flow<*> = - confirmedStarts - .distinctUntilChanged() - .onEach { latest -> if (latest != null) onPeriodConfirmed(latest) } - .catch { } - private suspend fun apply(time: LocalTime, enabled: Boolean) { if (enabled) scheduler.schedule(time) else scheduler.cancel() } - private suspend fun onPeriodConfirmed(@Suppress("UNUSED_PARAMETER") start: LocalDate) { - preferences.resetCheckIns() - } - private fun anyEnabled(p: UserPreferences) = p.periodApproachingEnabled || p.periodExpectedTodayEnabled || p.didItStartEnabled || p.periodEndCheckInEnabled || p.fertileWindowReminderEnabled || p.ovulationReminderEnabled diff --git a/app/src/test/kotlin/dev/privacyllc/period/feature/lock/LockSettingsViewModelTest.kt b/app/src/test/kotlin/dev/privacyllc/period/feature/lock/LockSettingsViewModelTest.kt index d1157a6..b973eb0 100644 --- a/app/src/test/kotlin/dev/privacyllc/period/feature/lock/LockSettingsViewModelTest.kt +++ b/app/src/test/kotlin/dev/privacyllc/period/feature/lock/LockSettingsViewModelTest.kt @@ -112,8 +112,14 @@ class LockSettingsViewModelTest { Dispatchers.resetMain() } + /** + * Generous on purpose. Every PIN here costs a real PBKDF2 derivation at + * 210,000 iterations, and a test that asks for three of them can pass alone + * and time out when the whole suite runs beside it. This budget is for + * catching a genuine hang, not for measuring the crypto. + */ private fun await(predicate: suspend () -> Boolean) = runBlocking { - withTimeout(5_000) { while (!predicate()) delay(10) } + withTimeout(30_000) { while (!predicate()) delay(10) } } private fun pin(value: String) = value.toCharArray() diff --git a/app/src/test/kotlin/dev/privacyllc/period/notifications/ReminderCoordinatorTest.kt b/app/src/test/kotlin/dev/privacyllc/period/notifications/ReminderCoordinatorTest.kt index 4bacf65..ef6b753 100644 --- a/app/src/test/kotlin/dev/privacyllc/period/notifications/ReminderCoordinatorTest.kt +++ b/app/src/test/kotlin/dev/privacyllc/period/notifications/ReminderCoordinatorTest.kt @@ -9,6 +9,9 @@ import dev.privacyllc.period.core.datastore.UserPreferences import dev.privacyllc.period.core.datastore.UserPreferencesRepository import dev.privacyllc.period.core.notifications.ReminderScheduler import dev.privacyllc.period.domain.prediction.PersonalPredictionEngine +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.runBlocking @@ -39,14 +42,16 @@ import java.time.Clock class ReminderCoordinatorTest { private lateinit var coordinator: ReminderCoordinator + private lateinit var preferences: UserPreferencesRepository @Before fun setUp() { val context = ApplicationProvider.getApplicationContext() + preferences = UserPreferencesRepository( + PreferenceDataStoreFactory.create { context.preferencesDataStoreFile("coordinator_test") }, + ) coordinator = ReminderCoordinator( repository = CycleData.repository(context, PersonalPredictionEngine(), Clock.systemUTC()), - preferences = UserPreferencesRepository( - PreferenceDataStoreFactory.create { context.preferencesDataStoreFile("coordinator_test") }, - ), + preferences = preferences, scheduler = ReminderScheduler(context), ) } @@ -71,21 +76,44 @@ class ReminderCoordinatorTest { ).collect { } } - @Test fun `a throwing history flow does not escape the check-in chain`() = runBlocking { - coordinator.checkInResets( - confirmedStarts = flow { throw IllegalStateException("the database could not be opened") }, - ).collect { } - } - /** * The positive control, so a `catch` that swallowed everything would not - * pass the three tests above by accident. A null latest start is the - * no-history case, which reaches the collector without touching the - * scheduler or the preferences store. + * pass the two tests above by accident. + * + * A repeated key must collapse: the forecast object is rebuilt on every + * recalculation even when it says the same thing, and rescheduling on each + * of those would churn WorkManager for nothing. */ @Test fun `a healthy flow still reaches the collector`() = runBlocking { var emissions = 0 - coordinator.checkInResets(confirmedStarts = flowOf(null, null)).collect { emissions++ } + coordinator.scheduleUpdates( + forecastDates = flowOf(null, null), + preferences = flowOf(UserPreferences.Defaults), + ).collect { emissions++ } assertEquals("distinctUntilChanged should collapse the repeated value", 1, emissions) } + + /** + * The check-in count is not the coordinator's business any more, and this + * is the regression that made it so. + * + * The old chain reset the count when the newest confirmed start changed — + * except `distinctUntilChanged` only dedups within one collection, and + * `PeriodApplication` starts the coordinator in EVERY process, including the + * one WorkManager spawns to run the reminder. So the first emission always + * passed, the count was wiped moments before the worker read it, and §30's + * "do not nag forever" could never be reached. + * + * Starting twice on the same history now leaves the tally alone, because + * nothing is watching the history at all. + */ + @Test fun `starting twice on the same history leaves the check-in tally alone`() = runBlocking { + preferences.recordCheckIn(periodId = 7L) + preferences.recordCheckIn(periodId = 7L) + + coordinator.start(CoroutineScope(Dispatchers.Unconfined)) + coordinator.start(CoroutineScope(Dispatchers.Unconfined)) + + assertEquals(2, preferences.checkInTally.first().countFor(7L)) + } } diff --git a/core/datastore/src/main/kotlin/dev/privacyllc/period/core/datastore/UserPreferences.kt b/core/datastore/src/main/kotlin/dev/privacyllc/period/core/datastore/UserPreferences.kt index 25f2dae..121a77e 100644 --- a/core/datastore/src/main/kotlin/dev/privacyllc/period/core/datastore/UserPreferences.kt +++ b/core/datastore/src/main/kotlin/dev/privacyllc/period/core/datastore/UserPreferences.kt @@ -72,3 +72,18 @@ data class UserPreferences( val Defaults = UserPreferences() } } + +/** + * How many times the app has asked "did your period start?" — and about which + * period it was asking. + * + * The pair is the point. A bare count had to be reset when the question became + * moot, and the reset ran in every process that started the app, wiping it + * moments before the reminder worker read it. Carrying the period's id means + * the count answers itself: a count for a period that is no longer the newest + * is not a count at all. + */ +data class CheckInTally(val count: Int, val periodId: Long?) { + /** The count if it belongs to [periodId], otherwise none — see the class KDoc. */ + fun countFor(periodId: Long?): Int = if (periodId != null && this.periodId == periodId) count else 0 +} diff --git a/core/datastore/src/main/kotlin/dev/privacyllc/period/core/datastore/UserPreferencesRepository.kt b/core/datastore/src/main/kotlin/dev/privacyllc/period/core/datastore/UserPreferencesRepository.kt index 86e1d02..aeeb387 100644 --- a/core/datastore/src/main/kotlin/dev/privacyllc/period/core/datastore/UserPreferencesRepository.kt +++ b/core/datastore/src/main/kotlin/dev/privacyllc/period/core/datastore/UserPreferencesRepository.kt @@ -5,6 +5,7 @@ import androidx.datastore.preferences.core.Preferences import androidx.datastore.preferences.core.booleanPreferencesKey import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.core.intPreferencesKey +import androidx.datastore.preferences.core.longPreferencesKey import androidx.datastore.preferences.core.stringPreferencesKey import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.catch @@ -63,13 +64,29 @@ class UserPreferencesRepository( * the process dies. Reset when a period is confirmed, which is the event * that makes the question moot. */ - val checkInCount: Flow = dataStore.data + val checkInTally: Flow = dataStore.data .catch { cause -> if (cause is IOException) emit(EMPTY) else throw cause } - .map { it[Keys.CheckInCount] ?: 0 } + .map { CheckInTally(it[Keys.CheckInCount] ?: 0, it[Keys.CheckInPeriodId]) } - suspend fun recordCheckIn() = edit { it[Keys.CheckInCount] = (it[Keys.CheckInCount] ?: 0) + 1 } - - suspend fun resetCheckIns() = edit { it[Keys.CheckInCount] = 0 } + /** + * Count one check-in against the period it was asked about. + * + * A different period starts the count again, which is what the old + * `resetCheckIns()` was trying to achieve and could not: it was called from + * a flow that re-emitted its first value in **every** process — including + * the one WorkManager starts to run the reminder — so the count was wiped + * moments before the worker read it, and §30's "do not nag forever" could + * never be reached. + * + * Keying the count instead of resetting it removes the timing entirely. + * There is no chain to fire, nothing to race, and nothing to get wrong in a + * second process. + */ + suspend fun recordCheckIn(periodId: Long) = edit { + val same = it[Keys.CheckInPeriodId] == periodId + it[Keys.CheckInCount] = if (same) (it[Keys.CheckInCount] ?: 0) + 1 else 1 + it[Keys.CheckInPeriodId] = periodId + } /** * Entitlement, mirrored from Google Play. @@ -124,6 +141,17 @@ class UserPreferencesRepository( val DidItStart = booleanPreferencesKey("reminder_did_it_start") val PeriodEndCheckIn = booleanPreferencesKey("reminder_period_end_check_in") val CheckInCount = intPreferencesKey("check_in_count") + + /** + * Which period the count above belongs to. + * + * A row id, never a date. The same rule that keeps dates out of + * `PeriodRecord.toString()` applies to anything at rest that a crash + * reporter or a backup could pick up: an id says a record exists, a date + * says when somebody bled. Delete My Data leaves an id matching nothing, + * which correctly reads as a count of zero. + */ + val CheckInPeriodId = longPreferencesKey("check_in_period_id") val FertileReminder = booleanPreferencesKey("fertile_window_reminder_enabled") val OvulationReminder = booleanPreferencesKey("ovulation_reminder_enabled") val BiometricLock = booleanPreferencesKey("biometric_lock_enabled") diff --git a/core/datastore/src/test/kotlin/dev/privacyllc/period/core/datastore/UserPreferencesRepositoryTest.kt b/core/datastore/src/test/kotlin/dev/privacyllc/period/core/datastore/UserPreferencesRepositoryTest.kt index aaad9a7..d375f96 100644 --- a/core/datastore/src/test/kotlin/dev/privacyllc/period/core/datastore/UserPreferencesRepositoryTest.kt +++ b/core/datastore/src/test/kotlin/dev/privacyllc/period/core/datastore/UserPreferencesRepositoryTest.kt @@ -187,4 +187,52 @@ class UserPreferencesRepositoryTest { assertTrue(p.incognitoLauncherEnabled) assertTrue(p.adsRemoved) } + + // ----------------------------------------------------------------------- + // The check-in tally + // ----------------------------------------------------------------------- + + @Test + fun `check-ins are counted against the period they were asked about`() = scope.runTest { + repo.recordCheckIn(periodId = 7L) + repo.recordCheckIn(periodId = 7L) + + assertEquals(2, repo.checkInTally.first().countFor(7L)) + } + + @Test + fun `a different period starts the count again`() = scope.runTest { + repo.recordCheckIn(periodId = 7L) + repo.recordCheckIn(periodId = 7L) + repo.recordCheckIn(periodId = 8L) + + // This is what the old resetCheckIns() was for, and it happens here by + // arithmetic rather than by an event that had to fire at the right + // moment — which it did not, in the process WorkManager starts. + assertEquals(1, repo.checkInTally.first().countFor(8L)) + } + + @Test + fun `a count for one period reads as nothing for another`() = scope.runTest { + repo.recordCheckIn(periodId = 7L) + repo.recordCheckIn(periodId = 7L) + repo.recordCheckIn(periodId = 7L) + + assertEquals(0, repo.checkInTally.first().countFor(8L)) + // And with no period at all — a fresh install, or after Delete My Data, + // where the stored id matches nothing that exists. + assertEquals(0, repo.checkInTally.first().countFor(null)) + } + + @Test + fun `the tally survives being read in another process`() = scope.runTest { + repo.recordCheckIn(periodId = 7L) + repo.recordCheckIn(periodId = 7L) + + // A second repository over the same file is what the reminder worker's + // process holds. The count is state, not a session: §30's stopping rule + // is meaningless if it starts over whenever a process does. + val elsewhere = UserPreferencesRepository(store) + assertEquals(2, elsewhere.checkInTally.first().countFor(7L)) + } } diff --git a/core/notifications/src/main/kotlin/dev/privacyllc/period/core/notifications/ReminderWorker.kt b/core/notifications/src/main/kotlin/dev/privacyllc/period/core/notifications/ReminderWorker.kt index 18785cd..bc723f7 100644 --- a/core/notifications/src/main/kotlin/dev/privacyllc/period/core/notifications/ReminderWorker.kt +++ b/core/notifications/src/main/kotlin/dev/privacyllc/period/core/notifications/ReminderWorker.kt @@ -76,6 +76,10 @@ class ReminderWorker @AssistedInject constructor( val notYet = repository.notYetObservations.first() val fertility = FertilityEstimate.from(forecast) + // The check-in count belongs to a period, not to the app. See + // UserPreferencesRepository.recordCheckIn. + val latestPeriodId = periods.maxByOrNull { it.startDate }?.id + val status = CycleStatusRules.statusFor( periods = periods, forecast = forecast, @@ -88,7 +92,7 @@ class ReminderWorker @AssistedInject constructor( val decision = ReminderRules.decide( status = status, preferences = prefs.toReminderPreferences(), - checkInsSoFar = preferences.checkInCount.first(), + checkInsSoFar = preferences.checkInTally.first().countFor(latestPeriodId), fertility = fertility, today = today, ) @@ -115,8 +119,9 @@ class ReminderWorker @AssistedInject constructor( contentIntent = openApp(), ) // Counted, so the next wake-up sees a number past the limit and - // sends nothing. - preferences.recordCheckIn() + // sends nothing. Against the period it was asked about, so the + // next period starts the count over on its own. + latestPeriodId?.let { preferences.recordCheckIn(it) } return Result.success() } @@ -147,7 +152,7 @@ class ReminderWorker @AssistedInject constructor( if (decision.kind == ReminderKind.DID_IT_START || decision.kind == ReminderKind.PERIOD_EXPECTED_TODAY ) { - preferences.recordCheckIn() + latestPeriodId?.let { preferences.recordCheckIn(it) } } return Result.success() } diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 06d44f3..37e432b 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -157,6 +157,27 @@ health app a crash mid-write is adjacent to losing what the user just entered, and a message they can read beats a process that vanished. The message carries the exception *type* and never a record's contents — §45. +### The check-in count belongs to a period, not to the app + +§30 says the app must stop asking "did your period start?" after a few +unanswered check-ins, and start again next cycle. That was a counter plus a +reset, and the reset ran in a flow watching the newest confirmed start — +`distinctUntilChanged`, whose first emission always passes, in a coordinator +`PeriodApplication` starts in **every** process. Including the one WorkManager +spawns to run the reminder, moments before the worker reads the count. The +stopping rule could never be reached. + +The count is now stored with the row id of the period it was asked about +(`check_in_period_id`), and reads as zero for any other. A new period starts it +over by arithmetic instead of by an event that has to fire at the right moment +in the right process; there is no chain to race and nothing for a second process +to get wrong. Delete My Data leaves an id matching nothing, which is correctly +no count at all. + +An id, never a date — the rule that keeps dates out of `PeriodRecord.toString()` +applies to anything at rest a backup or a crash reporter could pick up. An id +says a record exists; a date says when somebody bled. + ### One forecast stands at a time, and backfill is not a prediction Two rules about `prediction_records` that are easy to get wrong and expensive to