diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 4a3e69cfe1..16550aba7a 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -113,6 +113,7 @@ androidx-room-testing = { module = "androidx.room:room-testing", version.ref = " androidx-lifecycle-viewmodel-ktx = { module = "androidx.lifecycle:lifecycle-viewmodel-ktx", version.ref = "lifecycle" } androidx-lifecycle-viewmodel-compose = { module = "androidx.lifecycle:lifecycle-viewmodel-compose", version.ref = "lifecycle" } androidx-lifecycle-runtime-compose = { module = "androidx.lifecycle:lifecycle-runtime-compose", version.ref = "lifecycle" } +androidx-lifecycle-process = { module = "androidx.lifecycle:lifecycle-process", version.ref = "lifecycle" } androidx-navigation-compose = { module = "androidx.navigation:navigation-compose", version.ref = "navigationCompose" } androidx-hilt-navigation-compose = { module = "androidx.hilt:hilt-navigation-compose", version.ref = "hiltNavigationCompose" } diff --git a/onebusaway-android/build.gradle.kts b/onebusaway-android/build.gradle.kts index e53be3b8b3..78e33335c5 100644 --- a/onebusaway-android/build.gradle.kts +++ b/onebusaway-android/build.gradle.kts @@ -428,6 +428,8 @@ dependencies { implementation(libs.androidx.lifecycle.viewmodel.ktx) implementation(libs.androidx.lifecycle.viewmodel.compose) implementation(libs.androidx.lifecycle.runtime.compose) + // ProcessLifecycleOwner: app-foreground (ON_START) trigger for push re-registration (#1957). + implementation(libs.androidx.lifecycle.process) // Navigation-Compose backbone, on the 2.9.x line (minSdk 23). // hilt-navigation-compose bridges hiltViewModel() to @HiltViewModel + SavedStateHandle nav-args. implementation(libs.androidx.navigation.compose) diff --git a/onebusaway-android/src/androidTest/java/org/onebusaway/android/database/oba/LegacyDataImporterTest.kt b/onebusaway-android/src/androidTest/java/org/onebusaway/android/database/oba/LegacyDataImporterTest.kt index 09f26894a3..ca5f54a6c2 100644 --- a/onebusaway-android/src/androidTest/java/org/onebusaway/android/database/oba/LegacyDataImporterTest.kt +++ b/onebusaway-android/src/androidTest/java/org/onebusaway/android/database/oba/LegacyDataImporterTest.kt @@ -32,6 +32,7 @@ import org.onebusaway.android.database.AppDatabase import org.onebusaway.android.database.survey.entity.Study import org.onebusaway.android.database.survey.entity.Survey import org.onebusaway.android.database.widealerts.entity.AlertEntity +import org.onebusaway.android.preferences.PreferencesEditor import org.onebusaway.android.preferences.PreferencesRepository /** @@ -381,4 +382,5 @@ private object NoopPreferences : PreferencesRepository { override fun setLong(key: String, value: Long) = Unit override fun setFloat(keyRes: Int, value: Float) = Unit override fun setFloat(key: String, value: Float) = Unit + override fun edit(block: PreferencesEditor.() -> Unit) = block(this) } diff --git a/onebusaway-android/src/androidTest/java/org/onebusaway/android/preferences/PreferencesRepositoryReadYourWritesTest.kt b/onebusaway-android/src/androidTest/java/org/onebusaway/android/preferences/PreferencesRepositoryReadYourWritesTest.kt index e988f108b3..dc226ce786 100644 --- a/onebusaway-android/src/androidTest/java/org/onebusaway/android/preferences/PreferencesRepositoryReadYourWritesTest.kt +++ b/onebusaway-android/src/androidTest/java/org/onebusaway/android/preferences/PreferencesRepositoryReadYourWritesTest.kt @@ -18,6 +18,7 @@ package org.onebusaway.android.preferences import androidx.datastore.core.DataStore import androidx.datastore.preferences.core.Preferences import androidx.datastore.preferences.core.emptyPreferences +import androidx.datastore.preferences.core.longPreferencesKey import androidx.datastore.preferences.core.preferencesOf import androidx.datastore.preferences.core.stringPreferencesKey import androidx.test.ext.junit.runners.AndroidJUnit4 @@ -39,6 +40,10 @@ import org.junit.runner.RunWith * the cache could revert a newer optimistic write when a *stale* emission of an earlier write arrived * after it — so `setX(B)` immediately followed by `getX()` could return an earlier value `A`. This is * what made the region/custom-URL instrumented tests flake (`expected api.tampa… but was api.pugetsound…`). + * + * Also pins the [PreferencesRepository.edit] batch contract (#1978): all staged keys land in a single + * DataStore commit — the atomicity that multi-slot records (e.g. `PushRegistrationStore`) rely on to + * never tear across a process death. */ @RunWith(AndroidJUnit4::class) class PreferencesRepositoryReadYourWritesTest { @@ -64,6 +69,36 @@ class PreferencesRepositoryReadYourWritesTest { assertEquals("second", repo.getString(key, null)) } + @OptIn(ExperimentalCoroutinesApi::class) + @Test + fun edit_commitsAllStagedKeysInOneDataStoreUpdate() = runTest { + val dataStore = FakeDataStore() + val repo = DefaultPreferencesRepository(InstrumentationRegistry.getInstrumentation().targetContext, dataStore, backgroundScope) + runCurrent() + repo.setString("preexisting", "value") + runCurrent() + val updatesBefore = dataStore.updateCount + + repo.edit { + setString("record_a", "a") + setLong("record_b", 7L) + setString("preexisting", null) // null removes, same as setString + } + + // Read-your-writes holds across the whole batch, synchronously. + assertEquals("a", repo.getString("record_a", null)) + assertEquals(7L, repo.getLong("record_b", 0L)) + assertEquals(null, repo.getString("preexisting", null)) + + // The batch persists as ONE commit — the atomicity multi-slot records depend on — and the + // committed state carries every staged key. + runCurrent() + assertEquals(1, dataStore.updateCount - updatesBefore) + assertEquals("a", dataStore.committed[stringPreferencesKey("record_a")]) + assertEquals(7L, dataStore.committed[longPreferencesKey("record_b")]) + assertEquals(null, dataStore.committed[stringPreferencesKey("preexisting")]) + } + /** * A [DataStore] test double whose committed-state emissions are driven by the test (via [emit]) * rather than emitted automatically on [updateData] — so a test can reproduce DataStore's real commit @@ -72,8 +107,15 @@ class PreferencesRepositoryReadYourWritesTest { private class FakeDataStore(initial: Preferences = emptyPreferences()) : DataStore { private val emissions = MutableSharedFlow(replay = 1, extraBufferCapacity = 64) + /** The state the last [updateData] committed. */ + @Volatile + var committed: Preferences = initial + private set + + /** Number of [updateData] commits so far — one per DataStore edit, however many keys it carries. */ @Volatile - private var committed = initial + var updateCount = 0 + private set init { emissions.tryEmit(initial) @@ -83,6 +125,7 @@ class PreferencesRepositoryReadYourWritesTest { override suspend fun updateData(transform: suspend (t: Preferences) -> Preferences): Preferences { committed = transform(committed) + updateCount++ return committed // deliberately does not emit; the test drives [emit] to model commit latency } diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/api/contract/PushRegistrationWebService.kt b/onebusaway-android/src/main/java/org/onebusaway/android/api/contract/PushRegistrationWebService.kt new file mode 100644 index 0000000000..2f7fa0ac55 --- /dev/null +++ b/onebusaway-android/src/main/java/org/onebusaway/android/api/contract/PushRegistrationWebService.kt @@ -0,0 +1,82 @@ +/* + * Copyright (C) 2026 Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.onebusaway.android.api.contract + +import retrofit2.Response +import retrofit2.http.Field +import retrofit2.http.FormUrlEncoded +import retrofit2.http.HTTP +import retrofit2.http.POST +import retrofit2.http.Query +import retrofit2.http.Url + +/** + * The OBACloud push-registration client (issue #1957) — like [ReminderWebService], it is served from + * the region's sidecar host, not the OBA `where` host, so both calls pass the resolved URL via [Url] + * and this service runs WITHOUT `ApiParamsInterceptor` (the Retrofit base URL is a throwaway). + * + * Registering the device's FCM token proactively (rather than only as a side effect of creating a trip + * alarm) is what lets OBACloud deliver service-alert notifications to riders who enable notifications + * but never set an alarm, capture the device locale, and keep tokens from ageing out at 180 days. + * + * Both calls return `204 No Content` on success, so the body is unused — [Response] carries only the + * status. The registration endpoint is unauthenticated (ownership is possession of the token) and + * rate-limited to 30 requests/minute/IP, so callers must avoid redundant registrations. + */ +interface PushRegistrationWebService { + + /** + * Registers (or refreshes) this device's push token with the region. Form-urlencoded POST to + * `…/regions/{id}/push_registrations` (issue #1957). The server upserts on `(region, token)` and + * overwrites the stored value from each call, so [testDevice] must be sent every time (an omitted + * `test_device` resets it to `false`) — the wire value is the literal `true`/`false` the contract + * documents. [locale] is the device's BCP-47 tag sent as-is; the server maps it to its translation + * catalog itself. + * + * [description] is a human-readable device label that the server requires **only** when + * [testDevice] is true, rejecting the call otherwise with + * `422 {"error":"Unable to register device","messages":["Description can't be blank"]}`. Pass null + * for an ordinary registration and Retrofit omits the field entirely — deliberate, so an ordinary + * rider's device model is never sent. + * + * OBACloud's push-notifications documentation now specifies this field, confirming the behaviour + * this client was originally built against by probing the deployed server: free text "≤255 chars + * identifying the device to admins" (e.g. `"Aaron's iPhone 17"`), "server-enforced when + * `test_device=true` (422 if blank)", and cleared server-side when a device is demoted to + * `test_device=false`. The rider supplies the value, and it is capped at + * `PUSH_DESCRIPTION_MAX_LENGTH` before it reaches here. + */ + @FormUrlEncoded + @POST + suspend fun register( + @Url url: String, + @Field("token") token: String, + @Field("locale") locale: String, + @Field("test_device") testDevice: Boolean, + @Field("description") description: String?, + @Field("operating_system") operatingSystem: String = "android" + ): Response + + /** + * Unregisters this device's push [token] from the region (when the rider opts out of + * notifications). DELETE to `…/regions/{id}/push_registrations` with the token as a query param + * (issue #1957) — the form OBACloud's push-notifications documentation specifies: + * `DELETE /api/v2/regions/{region_id}/push_registrations?token={token}`. It also documents a `404` + * (token never registered) as equivalent to success, which [PushRegistrationClient] treats as such. + */ + @HTTP(method = "DELETE") + suspend fun unregister(@Url url: String, @Query("token") token: String): Response +} diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/api/contract/SidecarUrls.kt b/onebusaway-android/src/main/java/org/onebusaway/android/api/contract/SidecarUrls.kt new file mode 100644 index 0000000000..84c3a0769a --- /dev/null +++ b/onebusaway-android/src/main/java/org/onebusaway/android/api/contract/SidecarUrls.kt @@ -0,0 +1,35 @@ +/* + * Copyright (C) 2026 Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.onebusaway.android.api.contract + +/** + * Assembles a region-scoped **v2** sidecar endpoint URL: + * `{sidecarBaseUrl}{regionsPath}{regionId}/{resource}` — e.g. + * `https://sidecar.onebusaway.org/api/v2/regions/1/alarms`. The one home of that shape, shared by the + * alarms client (`TripInfoRepository`) and `PushRegistrationClient`. (The v1 sidecar endpoints — + * weather, surveys — are a separate pre-existing idiom: each embeds the region id in its own string + * resource via a `regionID` placeholder.) + * + * [regionsPath] is the resolved `R.string.arrivals_reminders_api_endpoint` (`/api/v2/regions/`). It + * stays a parameter because the segment lives in that string resource — overridable per brand, + * though no brand overrides it today — and Context-free callers inject the resolved value. + */ +fun sidecarRegionUrl( + sidecarBaseUrl: String, + regionsPath: String, + regionId: Long, + resource: String +): String = "$sidecarBaseUrl$regionsPath$regionId/$resource" diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/app/Application.kt b/onebusaway-android/src/main/java/org/onebusaway/android/app/Application.kt index 2ef3490d00..41de083076 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/app/Application.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/app/Application.kt @@ -25,6 +25,7 @@ import org.onebusaway.android.app.di.AnalyticsEntryPoint import org.onebusaway.android.app.di.DatabaseEntryPoint import org.onebusaway.android.app.di.FirebaseMessagingEntryPoint import org.onebusaway.android.app.di.PreferencesEntryPoint +import org.onebusaway.android.app.di.PushRegistrationEntryPoint import org.onebusaway.android.app.di.RegionEntryPoint import org.onebusaway.android.notifications.NotificationChannels import org.onebusaway.android.region.RegionSubsystems @@ -73,6 +74,10 @@ class Application : android.app.Application() { PreferencesEntryPoint.get(this).incrementAppLaunchCount() FirebaseMessagingEntryPoint.get(this).fetchAndStoreToken() + + // Keep this device's OBACloud push registration in sync (register on launch / token rotation / + // opt-in, unregister on opt-out) so service alerts reach riders who never set a trip alarm (#1957). + PushRegistrationEntryPoint.get(this).start() } /** diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/app/di/NetworkModule.kt b/onebusaway-android/src/main/java/org/onebusaway/android/app/di/NetworkModule.kt index cf30f46c41..9feaca4d99 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/app/di/NetworkModule.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/app/di/NetworkModule.kt @@ -28,6 +28,7 @@ import okhttp3.logging.HttpLoggingInterceptor import org.onebusaway.android.BuildConfig import org.onebusaway.android.api.contract.BikeWebService import org.onebusaway.android.api.contract.OtpWebService +import org.onebusaway.android.api.contract.PushRegistrationWebService import org.onebusaway.android.api.contract.RegionsWebService import org.onebusaway.android.api.contract.ReminderWebService import org.onebusaway.android.api.contract.SurveyWebService @@ -112,6 +113,14 @@ object NetworkModule { @Singleton fun provideReminderWebService(json: Json): ReminderWebService = plainRetrofit(json).create(ReminderWebService::class.java) + /** + * The OBACloud push-registration client (#1957). Targets the region's sidecar host via `@Url`, so + * like the reminders client it uses a plain client without [ApiParamsInterceptor]. + */ + @Provides + @Singleton + fun providePushRegistrationWebService(json: Json): PushRegistrationWebService = plainRetrofit(json).create(PushRegistrationWebService::class.java) + /** * The OpenTripPlanner trip-planner client. Like [provideBikeWebService] it targets the region's OTP * host via absolute `@Url`, so it uses a plain client without [ApiParamsInterceptor] — but with the diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/app/di/PushRegistrationEntryPoint.kt b/onebusaway-android/src/main/java/org/onebusaway/android/app/di/PushRegistrationEntryPoint.kt new file mode 100644 index 0000000000..d72cf42929 --- /dev/null +++ b/onebusaway-android/src/main/java/org/onebusaway/android/app/di/PushRegistrationEntryPoint.kt @@ -0,0 +1,45 @@ +/* + * Copyright (C) 2026 Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.onebusaway.android.app.di + +import android.content.Context +import dagger.hilt.EntryPoint +import dagger.hilt.InstallIn +import dagger.hilt.android.EntryPointAccessors +import dagger.hilt.components.SingletonComponent +import org.onebusaway.android.push.PushRegistrationManager + +/** + * A Hilt [EntryPoint] that lets code which can't be constructor-injected — `Application.onCreate`, an + * `Activity` lifecycle callback — reach the injected [PushRegistrationManager] singleton (mirrors + * [FirebaseMessagingEntryPoint]). It needs a [Context] only to resolve the singleton graph. + * + * Use it only where injection genuinely isn't available — Hilt-reachable classes should inject + * [PushRegistrationManager] directly. + */ +@EntryPoint +@InstallIn(SingletonComponent::class) +interface PushRegistrationEntryPoint { + + fun pushRegistrationManager(): PushRegistrationManager + + companion object { + /** Resolves the [PushRegistrationManager] singleton from any [context] (its application is used). */ + @JvmStatic + fun get(context: Context): PushRegistrationManager = EntryPointAccessors.fromApplication(context, PushRegistrationEntryPoint::class.java) + .pushRegistrationManager() + } +} diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/preferences/PreferencesRepository.kt b/onebusaway-android/src/main/java/org/onebusaway/android/preferences/PreferencesRepository.kt index 5a68b678a0..d232e75c42 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/preferences/PreferencesRepository.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/preferences/PreferencesRepository.kt @@ -18,6 +18,7 @@ package org.onebusaway.android.preferences import android.content.Context import androidx.annotation.StringRes import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.MutablePreferences import androidx.datastore.preferences.core.Preferences import androidx.datastore.preferences.core.booleanPreferencesKey import androidx.datastore.preferences.core.edit @@ -37,6 +38,22 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import org.onebusaway.android.app.di.AppScope +/** + * The String-keyed setters, split out as the receiver of [PreferencesRepository.edit] so a batch block + * is statically restricted to staging writes. [PreferencesRepository] extends this, so the same methods + * double as the repository's one-shot String-keyed setters. The `@StringRes` key form is deliberately + * omitted here until a batch caller needs it — batching so far only involves const-string record slots. + */ +interface PreferencesEditor { + fun setBoolean(key: String, value: Boolean) + + /** Null removes the key. */ + fun setString(key: String, value: String?) + fun setInt(key: String, value: Int) + fun setLong(key: String, value: Long) + fun setFloat(key: String, value: Float) +} + /** * Injected access to user preferences — the single seam in front of the persisted store (replacing * scattered `Application.getPrefs()` / `PreferenceUtils` reads). @@ -49,9 +66,10 @@ import org.onebusaway.android.app.di.AppScope * Every accessor comes in two key forms. The `@StringRes` overload lets a caller name a pref by its * resource id and stay Context-free / JVM-testable (the implementation resolves it); the `String` * overload handles keys that are const or runtime strings rather than resources (e.g. the - * region-version slots, a map layer's preference key). + * region-version slots, a map layer's preference key). The String-keyed setters are inherited from + * [PreferencesEditor], which also serves as [edit]'s batch receiver. */ -interface PreferencesRepository { +interface PreferencesRepository : PreferencesEditor { /** Emits the current value of the boolean pref [keyRes] and re-emits on every change. */ fun observeBoolean(@StringRes keyRes: Int, default: Boolean): Flow @@ -98,19 +116,28 @@ interface PreferencesRepository { } fun setBoolean(@StringRes keyRes: Int, value: Boolean) - fun setBoolean(key: String, value: Boolean) fun setString(@StringRes keyRes: Int, value: String?) - fun setString(key: String, value: String?) fun setInt(@StringRes keyRes: Int, value: Int) - fun setInt(key: String, value: Int) fun setLong(@StringRes keyRes: Int, value: Long) - fun setLong(key: String, value: Long) fun setFloat(@StringRes keyRes: Int, value: Float) - fun setFloat(key: String, value: Float) + + /** + * Applies every write staged in [block] as **one commit** — for [DefaultPreferencesRepository] a + * single cache swap and a single DataStore edit (one file rewrite + fsync, one `data` emission) + * instead of one per key (#1978). That makes the batch atomic: a multi-slot record written through + * here persists whole or not at all across a process death, where the same slots written via + * individual `setX` calls could tear. Use it whenever several slots form one logical record; + * independent single writes should keep using `setX`. + * + * Deliberately has no default body: the atomicity IS the contract, so each implementation must + * decide how it honors it rather than silently inheriting a per-key replay that can tear. An + * in-memory test fake, which has no commit to tear, honestly implements it as `block(this)`. + */ + fun edit(block: PreferencesEditor.() -> Unit) companion object { /** Preference key for the persisted app-launch counter. Preserves the original value so counts @@ -198,13 +225,48 @@ class DefaultPreferencesRepository @Inject constructor( override fun setFloat(keyRes: Int, value: Float) = setFloat(context.getString(keyRes), value) override fun setFloat(key: String, value: Float) = put(floatPreferencesKey(key), value) + override fun edit(block: PreferencesEditor.() -> Unit) { + val ops = mutableListOf<(MutablePreferences) -> Unit>() + object : PreferencesEditor { + override fun setBoolean(key: String, value: Boolean) { + ops += stage(booleanPreferencesKey(key), value) + } + override fun setString(key: String, value: String?) { + ops += stage(stringPreferencesKey(key), value) + } + override fun setInt(key: String, value: Int) { + ops += stage(intPreferencesKey(key), value) + } + override fun setLong(key: String, value: Long) { + ops += stage(longPreferencesKey(key), value) + } + override fun setFloat(key: String, value: Float) { + ops += stage(floatPreferencesKey(key), value) + } + }.block() + commit { prefs -> ops.forEach { it(prefs) } } + } + /** Apply [value] (or remove the key when null) to the cache immediately, then persist async. */ - private fun put(key: Preferences.Key, value: T?) { - cache = cache.toMutablePreferences().apply { - if (value == null) remove(key) else set(key, value) - }.toPreferences() + private fun put(key: Preferences.Key, value: T?) = commit(stage(key, value)) + + /** A staged write: sets [value] (null removes the key) on whatever preferences it is applied to. */ + private fun stage(key: Preferences.Key, value: T?): (MutablePreferences) -> Unit = { prefs -> if (value == null) prefs.remove(key) else prefs[key] = value } + + // Guards the cache read-modify-write in [commit]: @Volatile gives readers visibility but not + // atomicity, so two unsynchronized commits could snapshot the same cache and the later swap would + // silently drop the earlier one's keys — forever, since the cache is never re-mirrored from + // DataStore. Reads stay lock-free; the async persist stays outside (DataStore serializes its own + // edits). + private val cacheLock = Any() + + /** Apply [op] to the cache in one swap, then persist it as one async DataStore edit. */ + private fun commit(op: (MutablePreferences) -> Unit) { + synchronized(cacheLock) { + cache = cache.toMutablePreferences().also(op).toPreferences() + } scope.launch { - dataStore.edit { prefs -> if (value == null) prefs.remove(key) else prefs[key] = value } + dataStore.edit { prefs -> op(prefs) } } } } diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationClient.kt b/onebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationClient.kt new file mode 100644 index 0000000000..3a6954cb64 --- /dev/null +++ b/onebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationClient.kt @@ -0,0 +1,162 @@ +/* + * Copyright (C) 2026 Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.onebusaway.android.push + +import android.content.Context +import android.util.Log +import com.google.firebase.crashlytics.FirebaseCrashlytics +import dagger.hilt.android.qualifiers.ApplicationContext +import java.net.HttpURLConnection +import javax.inject.Inject +import javax.inject.Singleton +import org.onebusaway.android.R +import org.onebusaway.android.api.contract.PushRegistrationWebService +import org.onebusaway.android.api.contract.sidecarRegionUrl +import org.onebusaway.android.util.runCatchingCancellable +import retrofit2.Response + +/** + * Carries a push-registration server rejection to the remote error channel. A dedicated type so these + * group as their own Crashlytics issue rather than mixing into generic [IllegalStateException]s. + */ +class PushRegistrationException(message: String) : Exception(message) + +/** + * Performs the two push_registrations calls (issue #1957) and answers a single question about each: + * **did the server end up in the state we wanted?** Everything a caller would otherwise have to get + * right about an unreliable network lives here — URL assembly, which non-2xx codes still count as + * success, and which failures are visible where (all log locally; only systematic rejections report + * remotely) — so [PushRegistrationManager] can reason about reconciliation rather than about HTTP. + * + * Nothing here retries. A false return means "not achieved this time"; the manager's next reconcile + * pass tries again, which is the only retry policy the feature needs given it already runs on every + * app foreground. + */ +@Singleton +class PushRegistrationClient internal constructor( + private val service: PushRegistrationWebService, + // The resolved /api/v2/regions/ segment (a string resource, so it can't be a compile-time const). + private val regionsPath: String, + // Seams for the Android-only reporting sinks, so failure handling is assertable from a plain JVM + // test: android.util.Log and Crashlytics are both unavailable there. + private val logWarning: (String, Throwable?) -> Unit, + private val reportError: (Throwable) -> Unit +) { + + /** Production constructor Hilt builds from: resolves the seams from [context] and delegates. */ + @Inject + constructor( + @ApplicationContext context: Context, + service: PushRegistrationWebService + ) : this( + service = service, + regionsPath = context.getString(R.string.arrivals_reminders_api_endpoint), + logWarning = { message, cause -> Log.w(TAG, message, cause) }, + reportError = { FirebaseCrashlytics.getInstance().recordException(it) } + ) + + /** POSTs [target]. True only on a 2xx (204) success. */ + suspend fun register(target: PushRegistration): Boolean = runCatchingCancellable { + val response = service.register( + url = registrationUrl(target), + token = target.token, + locale = target.locale, + testDevice = target.testDevice, + // Required by the server for a test device, omitted otherwise (see the service KDoc). + description = target.description + ) + val registered = response.isSuccessful + if (!registered) reportHttpFailure("register", target, response) + registered + }.onFailure { logTransportFailure("register", target, it) }.getOrDefault(false) + + /** + * DELETEs [previous]. True on 204 **or 404** — the row already being gone is the state we wanted, so + * the caller can clear its record either way. + */ + suspend fun unregister(previous: PushRegistration): Boolean = runCatchingCancellable { + val response = service.unregister(url = registrationUrl(previous), token = previous.token) + val removed = response.isSuccessful || response.code() == HttpURLConnection.HTTP_NOT_FOUND + if (!removed) reportHttpFailure("unregister", previous, response) + removed + }.onFailure { logTransportFailure("unregister", previous, it) }.getOrDefault(false) + + /** `{sidecarBaseUrl}/api/v2/regions/{regionId}/push_registrations`. */ + private fun registrationUrl(registration: PushRegistration): String = sidecarRegionUrl( + sidecarBaseUrl = registration.sidecarBaseUrl, + regionsPath = regionsPath, + regionId = registration.regionId, + resource = "push_registrations" + ) + + /** + * Reports a non-2xx response. Without this an HTTP error would be *invisible*: a failed status is a + * perfectly successful call as far as [runCatchingCancellable] is concerned, so the `onFailure` + * handlers above only ever see transport exceptions. Since a failed call also persists nothing, the + * same doomed request then repeats on every foreground — which is exactly how the missing- + * `description` 422 went unnoticed until a packet capture. + * + * Systematic rejections also go to the remote channel: registrations are the server's ONLY audience + * source for service alerts, so a contract failure — a field added server-side, a schema change — + * must not be visible solely in one developer's logcat. [Transient][isTransient] statuses stay + * local-only, like transport failures. + */ + private fun reportHttpFailure( + operation: String, + registration: PushRegistration, + response: Response + ) { + // The server's error body carries the actionable message (e.g. "Description can't be blank"). + val body = response.errorBody()?.string()?.trim().orEmpty() + val detail = if (body.isEmpty()) "" else " $body" + val message = "${failurePrefix(operation, registration)}: HTTP ${response.code()}$detail" + logWarning(message, null) + if (!isTransient(response.code())) reportError(PushRegistrationException(message)) + } + + /** + * Whether [code] signals a transient condition rather than a systematic rejection: throttling (429 — + * the endpoint is unauthenticated and limited per IP, so on a shared-NAT network — public wifi, + * carrier CGNAT — other clients can spend this device's budget) or server-side trouble (5xx). A + * later reconcile pass retries both, so reporting each occurrence would flood the remote channel + * and drown the systematic signal it exists to carry; only statuses that indicate *this request* + * can never succeed are worth a report. + */ + private fun isTransient(code: Int): Boolean = code == HTTP_TOO_MANY_REQUESTS || code in 500..599 + + /** + * Logs a transport-level failure. Local only — being offline is ordinary and self-healing, and + * reporting every such moment remotely would drown the systematic-rejection signal above. + */ + private fun logTransportFailure( + operation: String, + registration: PushRegistration, + cause: Throwable + ) = logWarning(failurePrefix(operation, registration), cause) + + /** + * The shared opening of every failure message. Identifies the endpoint by region + host — never the + * token, and never the URL, since the unregister URL carries the token as a query param. + */ + private fun failurePrefix(operation: String, registration: PushRegistration): String = "push $operation failed for region ${registration.regionId} at ${registration.sidecarBaseUrl}" + + private companion object { + const val TAG = "PushRegistration" + + /** RFC 6585 Too Many Requests — [HttpURLConnection] predates it and has no constant. */ + const val HTTP_TOO_MANY_REQUESTS = 429 + } +} diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationDecision.kt b/onebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationDecision.kt new file mode 100644 index 0000000000..458df67734 --- /dev/null +++ b/onebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationDecision.kt @@ -0,0 +1,262 @@ +/* + * Copyright (C) 2026 Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.onebusaway.android.push + +import kotlin.time.Duration +import kotlin.time.Duration.Companion.hours +import org.onebusaway.android.region.Region + +/** + * A single push registration on record with OBACloud: the region it targets, the FCM token, and the + * metadata sent alongside it. All properties participate in equality, so a change to *any* of them (a + * rotated token, a moved region, a new device locale, a renamed or cleared test-device label) is a + * change that must be pushed to the server. Pure data — no Android dependencies — so + * [decidePushRegistration] is JVM-unit-testable. + */ +data class PushRegistration( + val regionId: Long, + val sidecarBaseUrl: String, + val token: String, + val locale: String, + /** + * The admin-facing label sent alongside a test-device registration (null for an ordinary one). It + * participates in equality so that renaming the device re-POSTs — and because it can only change + * when the rider edits the setting, it never churns on its own. + */ + val description: String? +) { + + /** + * Whether this registers as a test device. Derived rather than stored: the server requires a + * `description` exactly when `test_device=true` (422 otherwise), so carrying the flag separately + * would admit a pair — flag set, no name — that can only ever be rejected. Deriving it makes that + * rule hold by construction, at the type level, instead of by agreement between call sites. + */ + val testDevice: Boolean get() = description != null +} + +/** + * Re-POST an otherwise-unchanged registration this often, so the server's `last_seen_at` is refreshed + * and its 180-day prune never silently drops this device (issue #1957's "freshness" goal). Matches the + * iOS client's `PushRegistrationManager.refreshInterval`, which keeps a healthy device at roughly one + * POST per day — comfortably inside the endpoint's 30 req/min/IP budget. + */ +val PUSH_REFRESH_INTERVAL: Duration = 24.hours + +/** + * The server's cap on a test device's `description` (OBACloud's push-notifications documentation: + * "Free text ≤255 chars identifying the device to admins"). Enforced client-side — via + * [truncatedToDescriptionCap] — so a long name is truncated rather than POSTed and rejected: a + * `description` that violates this is a 422, and since a failed registration persists nothing, the + * doomed request would otherwise repeat on every foreground. + */ +const val PUSH_DESCRIPTION_MAX_LENGTH = 255 + +/** + * Truncates to at most [PUSH_DESCRIPTION_MAX_LENGTH] UTF-16 units without ever splitting a surrogate + * pair — a bare `take()` counts code units and can cut between a pair's halves, sending a lone + * surrogate (mangled to `?`/U+FFFD by the form encoding) as the final character. When the cut would + * land mid-pair, the pair is dropped whole. + * + * The budget deliberately stays in UTF-16 units: it is the strictest of the plausible readings of the + * server's documented "≤255 chars" (the OBACloud source is private, so its exact counting can't be + * read) — a string of at most N UTF-16 units also has at most N code points, so the result is legal + * under either interpretation. Grapheme clusters (ZWJ emoji sequences, combining marks) can still be + * cut between code points; that renders imperfectly but is valid Unicode and within the server limit. + */ +fun String.truncatedToDescriptionCap(): String = if (length <= PUSH_DESCRIPTION_MAX_LENGTH) { + this +} else { + take(PUSH_DESCRIPTION_MAX_LENGTH).dropLastWhile { it.isHighSurrogate() } +} + +/** + * True when [this] and [other] address the same server row — the region, host, and token a DELETE (or an + * upsert POST) targets. Locale and test-device aren't part of the row's identity, so they don't count. + */ +fun PushRegistration.sameEndpoint(other: PushRegistration): Boolean = regionId == other.regionId && + sidecarBaseUrl == other.sidecarBaseUrl && + token == other.token + +/** + * The registration this device should have, derived from the current inputs. Four-valued, not a + * nullable [PushRegistration], because "no registration" genuinely means three different things here + * and conflating them is exactly how a registration bug leaks: an *unresolved* answer must freeze + * reconciliation (deciding on a half-read state fires spurious DELETEs), an [OptedOut] answer must + * leave the server row alone (issue #1957: an OS-level disable needs no DELETE), and only a + * [NoSidecar] answer actively unregisters what is on record. The splits are enforced at the type + * level — [decidePushRegistration] accepts only [Resolved], so the only way to skip reconciling is an + * explicit match on [Unresolved]. + */ +sealed interface DesiredRegistration { + + /** + * An input that resolves asynchronously (the region flow, the FCM token) hasn't settled yet, so + * nothing can be decided — a later trigger re-runs the reconcile once it has. Never an opt-out: + * this state cannot reach [decidePushRegistration], so it can never DELETE anything. + */ + data object Unresolved : DesiredRegistration + + /** Every input has settled; only these states may be reconciled against the record. */ + sealed interface Resolved : DesiredRegistration + + /** + * The rider has notifications off at the OS level — nothing should be POSTed, and the server row + * (if any) is deliberately left in place. Issue #1957: an OS-level disable "doesn't need a DELETE — + * FCM bounces feed back to the server and it cleans up unregistered tokens itself"; DELETE is + * reserved for an *in-app* opt-out, which this app doesn't have. The iOS client behaves the same + * way (its `PushRegistrationManager` never DELETEs). Deleting here would also risk dropping the + * delivery target of an already-scheduled trip alarm the moment the rider toggles notifications + * off, if the server couples alarm delivery to the registration row. + */ + data object OptedOut : Resolved + + /** + * The current region has no sidecar host to register with, so no registration should exist here — + * and one on record from a *previous* region must be unregistered, or the rider who moved keeps + * receiving the old region's alerts until the server's 180-day prune (the iOS docs sanction exactly + * this old-row DELETE on a region change). + */ + data object NoSidecar : Resolved + + /** Definitively, [target] should be registered. */ + data class Wanted(val target: PushRegistration) : Resolved +} + +/** + * The pure derivation of [DesiredRegistration] from the raw inputs (issue #1957). Every early exit + * names which of the four answers it is — there is no `null` through which "this region has no + * sidecar" (a resolved fact, [DesiredRegistration.NoSidecar]) can masquerade as "the region hasn't + * loaded yet" (an unresolved one) — see [DesiredRegistration] for why that distinction is load-bearing. + * Notifications off at the OS level is the **only opt-out signal** (there is no in-app toggle), it is + * definitive regardless of every other input, and it deliberately does not unregister — see + * [DesiredRegistration.OptedOut]. + * + * The test-device [description][PushRegistration.description] is honoured only once the rider has + * named the device: the server rejects a `test_device=true` registration with a blank `description` + * (422), so an unnamed device registers as an ordinary one rather than POSTing a request guaranteed to + * fail. The iOS client gates its "Test Device Name" the same way — and [PushRegistration.testDevice] + * derives the flag from the name, so that rule holds by construction. The name is capped when it is + * written (`AdvancedSettingsViewModel.onPushTestDeviceNameChanged`); the [PUSH_DESCRIPTION_MAX_LENGTH] + * clamp here is a cheap guard on the wire boundary itself, so no path can put an over-long + * `description` on a request the server would reject outright. + * + * @param region null while the region flow hasn't resolved yet (it is briefly null at cold start — + * see `RegionRepository`). + * @param token the FCM token, or `""` while one hasn't been obtained yet (`userPushId()`'s contract). + * @param locale the device's BCP-47 tag, sent as-is with no normalization. + */ +fun deriveDesiredRegistration( + notificationsEnabled: Boolean, + region: Region?, + token: String, + locale: String, + testDeviceEnabled: Boolean, + testDeviceName: String? +): DesiredRegistration { + if (!notificationsEnabled) return DesiredRegistration.OptedOut + if (region == null) return DesiredRegistration.Unresolved + val base = region.sidecarBaseUrl?.takeIf { it.isNotBlank() } ?: return DesiredRegistration.NoSidecar + if (token.isEmpty()) return DesiredRegistration.Unresolved + val description = testDeviceName + ?.takeIf { testDeviceEnabled } + ?.trim() + ?.truncatedToDescriptionCap() + ?.takeIf { it.isNotEmpty() } + return DesiredRegistration.Wanted( + PushRegistration( + regionId = region.id, + sidecarBaseUrl = base, + token = token, + locale = locale, + description = description + ) + ) +} + +/** + * What the registrar should do to reconcile the [PushRegistration] currently on record with the one the + * device now wants (issue #1957). + */ +sealed interface PushRegistrationAction { + + /** On record and desired already match (or neither exists) — no network call. */ + object NoOp : PushRegistrationAction + + /** POST [target]: either the first registration, or a metadata refresh on the same token+region. */ + data class Register(val target: PushRegistration) : PushRegistrationAction + + /** + * DELETE [previous]: its destination no longer applies (the rider's region has no sidecar). Never + * produced for an OS-level opt-out — see [DesiredRegistration.OptedOut]. + */ + data class Unregister(val previous: PushRegistration) : PushRegistrationAction + + /** + * The token or the region changed while still enabled: DELETE the stale [previous] registration + * (so the old region/token stops receiving) and POST the new [target]. The DELETE is + * **best-effort by design**: a miss is dropped, and the old row lingers until the server's + * 180-day prune — the iOS client's baseline for *every* region change (it never DELETEs, and + * issue #1957 marks the old-row DELETE as optional). A persisted retry queue was deliberately + * rejected as disproportionate to that corner (PR #1958 review). + */ + data class Reregister( + val previous: PushRegistration, + val target: PushRegistration + ) : PushRegistrationAction +} + +/** + * The pure reconciliation decision. [desired] is the registration the device should have — typed as + * [DesiredRegistration.Resolved], so an unresolved input state cannot reach this function and be + * mistaken for either an opt-out or a wanted registration. [last] is the registration last + * successfully sent to the server, or null if none. [sinceLastSent] is how long ago [last] was sent, + * or null when that is unknown (no record, or a record written by a build that predates the + * timestamp) — unknown counts as stale, since a redundant upsert is harmless and losing the device to + * the 180-day prune is not. + * + * Keeping this a pure function (no clock, no I/O, no Android — the caller supplies the elapsed time) + * is deliberate: it is the one place the register/refresh/unregister rules live, and it is + * exhaustively unit-tested. + */ +fun decidePushRegistration( + desired: DesiredRegistration.Resolved, + last: PushRegistration?, + sinceLastSent: Duration? +): PushRegistrationAction = when (desired) { + // The server row is left alone on an OS-level opt-out (issue #1957; iOS parity) — the record is + // kept too, so re-enabling reads as "unchanged" rather than a fresh registration. + DesiredRegistration.OptedOut -> PushRegistrationAction.NoOp + DesiredRegistration.NoSidecar -> + if (last == null) PushRegistrationAction.NoOp else PushRegistrationAction.Unregister(last) + is DesiredRegistration.Wanted -> { + val target = desired.target + when { + last == null -> PushRegistrationAction.Register(target) + // Unchanged, but the server's last_seen_at needs refreshing before the 180-day prune reaches it. + target == last -> + if (sinceLastSent == null || sinceLastSent >= PUSH_REFRESH_INTERVAL) { + PushRegistrationAction.Register(target) + } else { + PushRegistrationAction.NoOp + } + // Same endpoint, only metadata (locale / test flag / description) changed → a plain re-POST upserts it. + target.sameEndpoint(last) -> PushRegistrationAction.Register(target) + // Token or region changed → clean up the stale registration, then register the new one. + else -> PushRegistrationAction.Reregister(previous = last, target = target) + } + } +} diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationManager.kt b/onebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationManager.kt new file mode 100644 index 0000000000..ff7cb0e3c8 --- /dev/null +++ b/onebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationManager.kt @@ -0,0 +1,171 @@ +/* + * Copyright (C) 2026 Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.onebusaway.android.push + +import android.content.Context +import androidx.core.app.NotificationManagerCompat +import androidx.lifecycle.DefaultLifecycleObserver +import androidx.lifecycle.LifecycleOwner +import androidx.lifecycle.ProcessLifecycleOwner +import dagger.hilt.android.qualifiers.ApplicationContext +import java.util.Locale +import java.util.concurrent.atomic.AtomicBoolean +import javax.inject.Inject +import javax.inject.Singleton +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.launch +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock +import org.onebusaway.android.R +import org.onebusaway.android.app.di.AppScope +import org.onebusaway.android.preferences.PreferencesRepository +import org.onebusaway.android.region.RegionRepository + +/** + * Keeps this device's OBACloud push registration (issue #1957) in sync with the app's current state, so + * that service-alert notifications reach every rider who has notifications enabled — not only those who + * happen to create a trip alarm. + * + * A reconciler, in three parts: + * + * 1. **Desired state** ([deriveDesiredRegistration]) — a pure classification of the raw inputs (OS + * notifications-enabled — the opt-in signal; there is no separate in-app toggle — the current + * region, the FCM token, the device locale, and the test-device settings) into wanted / opted-out / + * no-sidecar / not-yet-resolved. This class only gathers the inputs. + * 2. **The decision** ([decidePushRegistration]) — a pure function comparing desired against the record + * of what was last sent, yielding register / re-register / unregister / no-op. + * 3. **Applying it** — this class, which owns only the rule for *what to record given which calls + * succeeded* (the quadrants in [sync]). + * + * The mechanisms underneath are deliberately elsewhere: [PushRegistrationClient] owns talking to the + * endpoint and making failures visible, and [PushRegistrationStore] owns the durable record and its + * commit semantics. Neither retries — every trigger below re-runs [sync], and that is the retry policy. + */ +@Singleton +class PushRegistrationManager internal constructor( + private val client: PushRegistrationClient, + private val store: PushRegistrationStore, + private val regionRepository: RegionRepository, + private val firebaseMessagingManager: FirebaseMessagingManager, + private val prefs: PreferencesRepository, + private val scope: CoroutineScope, + // The one Android-only read left here: NotificationManagerCompat isn't available to a plain JVM + // test, and the opt-in signal is what the whole reconcile hinges on. + private val notificationsEnabled: () -> Boolean +) { + + /** Production constructor Hilt builds from: resolves the seam from [context] and delegates. */ + @Inject + constructor( + @ApplicationContext context: Context, + client: PushRegistrationClient, + store: PushRegistrationStore, + regionRepository: RegionRepository, + firebaseMessagingManager: FirebaseMessagingManager, + prefs: PreferencesRepository, + @AppScope scope: CoroutineScope + ) : this( + client = client, + store = store, + regionRepository = regionRepository, + firebaseMessagingManager = firebaseMessagingManager, + prefs = prefs, + scope = scope, + notificationsEnabled = { NotificationManagerCompat.from(context).areNotificationsEnabled() } + ) + + private val started = AtomicBoolean(false) + + // Serializes reconciliation so overlapping triggers (region change + a token write arriving together) + // can't interleave two syncs and race on the stored record. + private val mutex = Mutex() + + /** + * Begins reconciling in the background, idempotently (safe to call more than once). Kicked from + * `Application.onCreate` (on the main thread). Two triggers, covering every input that can change: + * + * - A collector over the region flow and the preferences this feature reads (the FCM token, the + * test-device flag, and the test-device name), running one [sync] per change. All of them replay + * on launch, and each later change (token rotation, region switch, toggling or naming the test + * device) re-emits. Observing those keys specifically — rather than every app-wide preference + * write — avoids a needless `areNotificationsEnabled()` IPC on unrelated writes. + * - A process-lifecycle `ON_START` observer that [resync]s whenever the app comes to the foreground, + * catching an OS notification-permission change made in system settings (which touches neither a + * preference nor the region, so the collector can't see it) regardless of which activity resumes. + * + * Between them these also drive the [PUSH_REFRESH_INTERVAL] keep-alive: every foreground re-runs + * [sync], which re-POSTs an unchanged registration once its 24h window has elapsed. No separate + * timer or `WorkManager` job is needed — a rider who never opens the app for 180 days has no + * service-alert reach to preserve anyway, and the server prunes exactly that population. + */ + fun start() { + if (started.getAndSet(true)) return + scope.launch { + combine( + regionRepository.region, + prefs.observeString(R.string.firebase_messaging_token, null), + prefs.observeBoolean(R.string.preference_key_push_test_device, false), + prefs.observeString(R.string.preference_key_push_test_device_name, null) + ) { _, _, _, _ -> }.collect { sync() } + } + ProcessLifecycleOwner.get().lifecycle.addObserver(object : DefaultLifecycleObserver { + override fun onStart(owner: LifecycleOwner) = resync() + }) + } + + /** Re-runs reconciliation once, off the main thread; a no-op when nothing changed. */ + fun resync() { + scope.launch { sync() } + } + + /** Reconciles the recorded registration with the currently desired one. */ + private suspend fun sync() = mutex.withLock { + val desired = deriveDesiredRegistration( + notificationsEnabled = notificationsEnabled(), + region = regionRepository.region.value, + token = firebaseMessagingManager.userPushId(), + locale = Locale.getDefault().toLanguageTag(), + testDeviceEnabled = prefs.getBoolean(R.string.preference_key_push_test_device, false), + testDeviceName = prefs.getString(R.string.preference_key_push_test_device_name, null) + ) + // Unresolved (an async input hasn't settled) is the only state that may skip the decision — a + // later trigger re-runs us. See DesiredRegistration for why it must never be read as a resolved + // state (deciding on a half-read state fires spurious DELETEs). + if (desired !is DesiredRegistration.Resolved) return@withLock + + when (val action = decidePushRegistration(desired, store.last(), store.sinceLastSent())) { + PushRegistrationAction.NoOp -> Unit + is PushRegistrationAction.Register -> + if (client.register(action.target)) store.record(action.target) + is PushRegistrationAction.Unregister -> + if (client.unregister(action.previous)) store.clear() + is PushRegistrationAction.Reregister -> applyReregister(action) + } + } + + /** + * The [PushRegistrationAction.Reregister] DELETE and POST as two independent steps, each + * persisting its own outcome exactly like the standalone Unregister/Register branches in [sync]: + * a cleared-but-unrecorded state re-Registers next sync, both calls failing keeps `previous` so + * the whole Reregister retries, and a missed DELETE alone is dropped — best-effort, see + * [PushRegistrationAction.Reregister]. + */ + private suspend fun applyReregister(action: PushRegistrationAction.Reregister) { + if (client.unregister(action.previous)) store.clear() + if (client.register(action.target)) store.record(action.target) + } +} diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationStore.kt b/onebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationStore.kt new file mode 100644 index 0000000000..0106049168 --- /dev/null +++ b/onebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationStore.kt @@ -0,0 +1,107 @@ +/* + * Copyright (C) 2026 Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.onebusaway.android.push + +import javax.inject.Inject +import javax.inject.Singleton +import kotlin.time.Duration +import org.onebusaway.android.preferences.PreferencesRepository +import org.onebusaway.android.time.WallTime + +/** + * The durable memory of what this device has told OBACloud (issue #1957) — the only record of it, + * since the push_registrations API offers no GET: the registration last successfully sent ([last]) + * plus when it was sent ([sinceLastSent]). Needed to address the DELETE of a stale row when the + * endpoint changes or the region loses its sidecar (by which point the current token/region may + * already have changed), to skip redundant POSTs against a 30 req/min/IP endpoint, and to drive the + * [PUSH_REFRESH_INTERVAL] keep-alive. + * + * The record spans several preference slots, with the token as the **presence marker**: a read + * returns nothing unless it is present, and clearing the record nulls it. Every multi-slot write goes + * through [PreferencesRepository.edit], which commits all of the record's slots atomically — in + * process and on disk (#1978) — so the record persists whole or not at all across a process death, + * and a record whose marker is present but whose addressing fields are missing cannot exist. + */ +@Singleton +class PushRegistrationStore internal constructor( + private val prefs: PreferencesRepository, + // Device clock, injectable so the refresh window is drivable from a JVM test. + private val now: () -> WallTime +) { + + @Inject + constructor(prefs: PreferencesRepository) : this(prefs, WallTime::now) + + /** The registration last successfully sent to the server, or null if none is on record. */ + fun last(): PushRegistration? { + val base = prefs.getString(KEY_BASE, null) ?: return null + val token = prefs.getString(KEY_TOKEN, null) ?: return null + return PushRegistration( + regionId = prefs.getLong(KEY_REGION_ID, -1L), + sidecarBaseUrl = base, + token = token, + locale = prefs.getString(KEY_LOCALE, null) ?: "", + description = prefs.getString(KEY_DESCRIPTION, null) + ) + } + + /** + * How long ago [last] was sent, or null if unknown — no record, one written before this slot + * existed, or a device wall clock that has since been set backwards (the elapsed time would read + * negative, which would silence the [PUSH_REFRESH_INTERVAL] keep-alive until the clock caught back + * up — for a large jump, long enough for the server's 180-day prune to drop the device). Unknown + * counts as stale, so the caller re-POSTs and [record] restamps at the new clock, healing the jump. + * + * Purely local elapsed time measured against our own write ([WallTime] on both ends), never against + * a server timestamp, so no server-clock crossing is involved. + */ + fun sinceLastSent(): Duration? { + val sentAtMs = prefs.getLong(KEY_SENT_AT, 0L).takeIf { it > 0L } ?: return null + return (now() - WallTime(sentAtMs)).takeIf { !it.isNegative() } + } + + /** Commits [registration] as the live record and stamps the refresh clock. */ + fun record(registration: PushRegistration) { + prefs.edit { + setLong(KEY_REGION_ID, registration.regionId) + setString(KEY_BASE, registration.sidecarBaseUrl) + setString(KEY_LOCALE, registration.locale) + setString(KEY_DESCRIPTION, registration.description) + setLong(KEY_SENT_AT, now().epochMs) + setString(KEY_TOKEN, registration.token) + } + } + + /** Forgets the live record, so [last] reports nothing on record. */ + fun clear() { + prefs.edit { + setString(KEY_TOKEN, null) + // Dropped with the record it stamped: sinceLastSent() must not report an elapsed time for a + // registration last() says doesn't exist. + setLong(KEY_SENT_AT, 0L) + } + } + + private companion object { + // Internal bookkeeping slots (not user-facing) for the last successful registration. + const val KEY_REGION_ID = "push_reg_region_id" + const val KEY_BASE = "push_reg_base" + const val KEY_TOKEN = "push_reg_token" + const val KEY_LOCALE = "push_reg_locale" + const val KEY_DESCRIPTION = "push_reg_description" + const val KEY_SENT_AT = "push_reg_sent_at" + } +} diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/compose/NotificationPermissionRequest.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/compose/NotificationPermissionRequest.kt new file mode 100644 index 0000000000..5e5c1c68d9 --- /dev/null +++ b/onebusaway-android/src/main/java/org/onebusaway/android/ui/compose/NotificationPermissionRequest.kt @@ -0,0 +1,57 @@ +/* + * Copyright (C) 2026 Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.onebusaway.android.ui.compose + +import android.Manifest +import android.os.Build +import androidx.activity.compose.rememberLauncherForActivityResult +import androidx.activity.result.contract.ActivityResultContracts +import androidx.compose.runtime.Composable +import androidx.compose.runtime.remember +import androidx.compose.ui.platform.LocalContext +import org.onebusaway.android.app.di.PushRegistrationEntryPoint + +/** + * Remembers the in-app POST_NOTIFICATIONS opt-in, returning a `() -> Unit` that requests the + * permission and, once the user responds, resyncs this device's push registration. + * + * The system permission dialog only pauses (never stops) the activity, so PushRegistrationManager's + * `ProcessLifecycleOwner` `ON_START` resync never fires for an in-flow grant. Resyncing straight from + * the result callback registers the device the moment the user opts in — the highest-value moment for + * the push feature — instead of waiting for the next app launch. Wiring this into one shared helper + * means every opt-in site inherits the resync by construction rather than re-deriving it (and risking + * the old fire-and-forget bug). + * + * The returned action is a no-op below API 33, where POST_NOTIFICATIONS is granted implicitly at + * install, so callers can invoke it unconditionally. + */ +@Composable +internal fun rememberNotificationPermissionRequest(): () -> Unit { + val context = LocalContext.current + val launcher = rememberLauncherForActivityResult( + ActivityResultContracts.RequestPermission() + ) { PushRegistrationEntryPoint.get(context).resync() } + // Remembered so the returned action keeps a stable identity across recompositions — callers pass it + // into other composables (see TripDestinations), which could not skip if it were a fresh lambda + // every time. + return remember(launcher) { + { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { + launcher.launch(Manifest.permission.POST_NOTIFICATIONS) + } + } + } +} diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsScreen.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsScreen.kt index e9fbeeb419..e8b063d363 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsScreen.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsScreen.kt @@ -96,6 +96,8 @@ fun AdvancedSettingsRoute( onBack = onBack, onExperimentalToggle = onExperimentalToggle, onDisplayTestAlerts = viewModel::onDisplayTestAlertsChanged, + onPushTestDevice = viewModel::onPushTestDeviceChanged, + onPushTestDeviceName = viewModel::onPushTestDeviceNameChanged, onObaUrlChange = { url -> when (viewModel.onCustomObaApiUrlChanged(url)) { UrlChangeResult.InvalidObaUrl -> { @@ -150,6 +152,8 @@ fun AdvancedSettingsScreen( onBack: () -> Unit, onExperimentalToggle: (Boolean) -> Unit, onDisplayTestAlerts: (Boolean) -> Unit, + onPushTestDevice: (Boolean) -> Unit, + onPushTestDeviceName: (String) -> Boolean, onObaUrlChange: (String) -> Boolean, onOtpUrlChange: (String) -> Boolean, onOtpUrlUsesGraphQlChange: (Boolean) -> Unit, @@ -186,6 +190,30 @@ fun AdvancedSettingsScreen( checked = state.displayTestAlerts, onCheckedChange = onDisplayTestAlerts ) + SwitchPreferenceItem( + title = stringResource(R.string.preferences_push_test_device_title), + // On-but-unnamed is the one state where the toggle silently has no effect (the + // device registers normally until a name is set — see PushRegistrationManager), + // so say exactly that instead of the generic description. + summary = if (state.pushTestDevice && state.pushTestDeviceName == null) { + stringResource(R.string.preferences_push_test_device_unnamed_summary) + } else { + stringResource(R.string.preferences_push_test_device_summary) + }, + checked = state.pushTestDevice, + onCheckedChange = onPushTestDevice + ) + EditTextPreferenceItem( + title = stringResource(R.string.preferences_push_test_device_name_title), + summary = state.pushTestDeviceName + ?: stringResource(R.string.preferences_push_test_device_name_summary), + currentValue = state.pushTestDeviceName, + hint = stringResource(R.string.preferences_push_test_device_name_title), + onValueChange = onPushTestDeviceName, + // The name only matters while registering as a test device, so it follows the toggle. + enabled = state.pushTestDevice, + keyboardType = KeyboardType.Text + ) EditTextPreferenceItem( title = stringResource(R.string.preferences_map_stop_cache_size_title), summary = stringResource( diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModel.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModel.kt index 6cfca53fa5..dc1ca05ad4 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModel.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModel.kt @@ -35,6 +35,8 @@ import org.onebusaway.android.R import org.onebusaway.android.donations.DonationsManager import org.onebusaway.android.map.StopsMapController import org.onebusaway.android.preferences.PreferencesRepository +import org.onebusaway.android.push.PUSH_DESCRIPTION_MAX_LENGTH +import org.onebusaway.android.push.truncatedToDescriptionCap import org.onebusaway.android.region.ApiUrlValidator import org.onebusaway.android.region.Region import org.onebusaway.android.region.RegionRepository @@ -81,6 +83,8 @@ class AdvancedSettingsViewModel @Inject constructor( prefs = AdvancedPrefSnapshot( experimentalRegionsEnabled = prefs.getBoolean(R.string.preference_key_experimental_regions, false), displayTestAlerts = prefs.getBoolean(R.string.preferences_display_test_alerts, false), + pushTestDevice = prefs.getBoolean(R.string.preference_key_push_test_device, false), + pushTestDeviceName = prefs.getString(R.string.preference_key_push_test_device_name, null), customObaApiUrl = prefs.getString(R.string.preference_key_oba_api_url, null), customOtpApiUrl = prefs.getString(R.string.preference_key_otp_api_url, null), customOtpApiUrlUsesGraphQl = prefs.getBoolean( @@ -103,6 +107,25 @@ class AdvancedSettingsViewModel @Inject constructor( fun onDisplayTestAlertsChanged(value: Boolean) = prefs.setBoolean(R.string.preferences_display_test_alerts, value) + /** + * Toggle whether this device registers into OBACloud's test-only push audience (#1957). The write + * lands in the `preference_key_push_test_device` slot that `PushRegistrationManager` reads; its + * reactive collector observes preference changes and re-registers with the flipped `test_device` flag. + */ + fun onPushTestDeviceChanged(value: Boolean) = prefs.setBoolean(R.string.preference_key_push_test_device, value) + + /** + * Set the admin-facing name for this test device (#1957). Blank clears it, which downgrades the + * device to an ordinary registration — the server rejects a test-device registration without a + * name, so `PushRegistrationManager` declines to send one rather than POST a guaranteed failure. + * Always accepted (returns true): any non-blank text is a valid name. + * + * Normalized to the server's [PUSH_DESCRIPTION_MAX_LENGTH] cap here, at the one boundary where this + * value enters persistence, so what is stored is by construction a legal `description` and the name + * shown in settings is the name that goes on the wire. + */ + fun onPushTestDeviceNameChanged(value: String): Boolean = applyPushTestDeviceName(value, prefs) + /** * Apply an edit to the map stop LRU cache size. Returns true if the input was a valid in-range * number (and was persisted); false otherwise so the dialog stays open (reject-on-invalid). @@ -210,6 +233,23 @@ internal fun applyMapStopCacheSize(text: String, prefs: PreferencesRepository): return true } +/** + * The test-device-name edit's domain effect, split from + * [AdvancedSettingsViewModel.onPushTestDeviceNameChanged] so it's free of Android dependencies and + * unit-tested directly (see AdvancedSettingsViewModelTest). + * + * This is the one boundary where the name enters persistence, so it is where the value is normalized to + * OBACloud's [PUSH_DESCRIPTION_MAX_LENGTH] cap: what is stored is by construction a legal `description`, + * which keeps the name shown in settings identical to the one sent on the wire. Blank clears the slot, + * downgrading the device to an ordinary registration. Always accepted — any non-blank text is a valid + * name, so unlike [applyMapStopCacheSize] there is no reject-on-invalid case. + */ +internal fun applyPushTestDeviceName(value: String, prefs: PreferencesRepository): Boolean { + val name = value.trim().truncatedToDescriptionCap().ifEmpty { null } + prefs.setString(R.string.preference_key_push_test_device_name, name) + return true +} + /** * The domain rule that a toggle-initiated experimental-region *change* resets the OTP API version to the * current default ([resetOtpVersion]). The repository's own [RegionRepository.applyRegion] reset covers diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/SettingsUiState.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/SettingsUiState.kt index 1f1419c49a..cc4bba3d11 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/SettingsUiState.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/SettingsUiState.kt @@ -135,6 +135,11 @@ fun buildSettingsUiState( data class AdvancedPrefSnapshot( val experimentalRegionsEnabled: Boolean, val displayTestAlerts: Boolean, + // Registers this device into OBACloud's test-only push audience for previewing service alerts (#1957). + val pushTestDevice: Boolean, + // Admin-facing label for this test device. The server rejects a test-device registration without + // one, so an unnamed device registers as an ordinary device instead (see PushRegistrationManager). + val pushTestDeviceName: String?, val customObaApiUrl: String?, val customOtpApiUrl: String?, // Manual OTP protocol override for the custom URL above (#1780) — the custom-server counterpart @@ -162,6 +167,8 @@ data class AdvancedSettingsUiState( val showExperimentalRegions: Boolean, val experimentalRegionsEnabled: Boolean, val displayTestAlerts: Boolean, + val pushTestDevice: Boolean, + val pushTestDeviceName: String?, val customObaApiUrl: String?, val customOtpApiUrl: String?, val customOtpApiUrlUsesGraphQl: Boolean, @@ -193,6 +200,8 @@ fun buildAdvancedSettingsUiState( showExperimentalRegions = !useFixedRegion, experimentalRegionsEnabled = prefs.experimentalRegionsEnabled, displayTestAlerts = prefs.displayTestAlerts, + pushTestDevice = prefs.pushTestDevice, + pushTestDeviceName = prefs.pushTestDeviceName, customObaApiUrl = prefs.customObaApiUrl, customOtpApiUrl = prefs.customOtpApiUrl, customOtpApiUrlUsesGraphQl = prefs.customOtpApiUrlUsesGraphQl, diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripdetails/DestinationReminder.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripdetails/DestinationReminder.kt index 84fb6aaf28..d956dc8fed 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripdetails/DestinationReminder.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripdetails/DestinationReminder.kt @@ -16,7 +16,6 @@ */ package org.onebusaway.android.ui.tripdetails -import android.Manifest import android.app.Activity import android.content.BroadcastReceiver import android.content.Context @@ -40,7 +39,6 @@ import androidx.compose.ui.platform.LocalResources import androidx.compose.ui.res.colorResource import androidx.compose.ui.res.painterResource import androidx.compose.ui.res.stringResource -import androidx.core.app.ActivityCompat import androidx.core.content.ContextCompat import com.google.android.gms.common.api.ApiException import com.google.android.gms.common.api.ResolvableApiException @@ -57,7 +55,7 @@ import org.onebusaway.android.nav.NavigationService import org.onebusaway.android.preferences.PreferencesRepository import org.onebusaway.android.ui.compose.components.OptOutInfoDialog import org.onebusaway.android.ui.compose.findActivity -import org.onebusaway.android.util.PermissionUtils.NOTIFICATION_PERMISSION_REQUEST +import org.onebusaway.android.ui.compose.rememberNotificationPermissionRequest /** * The destination-reminder flow (set a reminder to alight at a chosen stop), as a reusable Compose @@ -105,6 +103,9 @@ internal fun rememberDestinationReminderAction( } } + // Requests POST_NOTIFICATIONS and resyncs the push registration on the result (see the helper). + val requestNotificationPermission = rememberNotificationPermissionRequest() + fun askUserToTurnLocationOn() { @Suppress("DEPRECATION") val locationRequest = LocationRequest.create().apply { @@ -171,14 +172,7 @@ internal fun rememberDestinationReminderAction( resources.getString(R.string.analytics_label_destination_reminder), resources.getString(R.string.analytics_label_destination_reminder_variant_started) ) - // POST_NOTIFICATIONS is a runtime permission only on API 33+; older versions grant it implicitly. - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { - ActivityCompat.requestPermissions( - activity, - arrayOf(Manifest.permission.POST_NOTIFICATIONS), - NOTIFICATION_PERMISSION_REQUEST - ) - } + requestNotificationPermission() val serviceIntent = setUpNavigationService(position) ?: return startNavigationService(serviceIntent) Toast.makeText( diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripdetails/TripDestinations.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripdetails/TripDestinations.kt index 604a24681e..bd32f07cdf 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripdetails/TripDestinations.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripdetails/TripDestinations.kt @@ -17,12 +17,9 @@ */ package org.onebusaway.android.ui.tripdetails -import android.Manifest -import android.os.Build import android.widget.Toast import androidx.compose.runtime.LaunchedEffect import androidx.compose.ui.platform.LocalContext -import androidx.core.app.ActivityCompat import androidx.hilt.lifecycle.viewmodel.compose.hiltViewModel import androidx.navigation.NavGraphBuilder import androidx.navigation.NavHostController @@ -33,6 +30,7 @@ import org.onebusaway.android.R import org.onebusaway.android.app.di.DatabaseEntryPoint import org.onebusaway.android.app.di.PreferencesEntryPoint import org.onebusaway.android.ui.compose.findActivity +import org.onebusaway.android.ui.compose.rememberNotificationPermissionRequest import org.onebusaway.android.ui.compose.theme.ObaTheme import org.onebusaway.android.ui.dataview.TripTrajectoryRoute import org.onebusaway.android.ui.dataview.TripTrajectoryViewModel @@ -41,7 +39,6 @@ import org.onebusaway.android.ui.tripinfo.TripInfoEvent import org.onebusaway.android.ui.tripinfo.TripInfoRoute import org.onebusaway.android.ui.tripinfo.TripInfoViewModel import org.onebusaway.android.ui.tripinfo.confirmDeleteReminder -import org.onebusaway.android.util.PermissionUtils /** * The trip navigation cluster: a trip's stops + live vehicle ([NavRoutes.TRIP_DETAILS]) and the @@ -168,6 +165,8 @@ fun NavGraphBuilder.tripGraph(navController: NavHostController) { val infoStopId = backStackEntry.arguments?.getString(NavRoutes.ARG_STOP_ID).orEmpty() val infoVm: TripInfoViewModel = hiltViewModel() + // Requests POST_NOTIFICATIONS and resyncs the push registration on the result (see the helper). + val requestNotificationPermission = rememberNotificationPermissionRequest() LaunchedEffect(infoVm) { infoVm.events.collect { event -> when (event) { @@ -192,14 +191,7 @@ fun NavGraphBuilder.tripGraph(navController: NavHostController) { viewModel = infoVm, onBack = { navController.popBackStack() }, onSave = { - // POST_NOTIFICATIONS is a runtime permission only on API 33+; older versions grant it implicitly. - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { - ActivityCompat.requestPermissions( - activity, - arrayOf(Manifest.permission.POST_NOTIFICATIONS), - PermissionUtils.NOTIFICATION_PERMISSION_REQUEST - ) - } + requestNotificationPermission() infoVm.save() }, onDelete = { diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripinfo/TripInfoRepository.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripinfo/TripInfoRepository.kt index b90924df43..7cc65e8cd9 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripinfo/TripInfoRepository.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripinfo/TripInfoRepository.kt @@ -23,6 +23,7 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import org.onebusaway.android.R import org.onebusaway.android.api.contract.ReminderWebService +import org.onebusaway.android.api.contract.sidecarRegionUrl import org.onebusaway.android.database.oba.ImportGate import org.onebusaway.android.database.oba.RouteDao import org.onebusaway.android.database.oba.StopDao @@ -260,10 +261,12 @@ class DefaultTripInfoRepository @Inject constructor( // push registration completes); the legacy builder returned null in that case. val userPushId = firebaseMessagingManager.userPushId() if (userPushId.isEmpty()) return null - val url = base + - context.getString(R.string.arrivals_reminders_api_endpoint) + - region.id + - "/alarms" + val url = sidecarRegionUrl( + sidecarBaseUrl = base, + regionsPath = context.getString(R.string.arrivals_reminders_api_endpoint), + regionId = region.id, + resource = "alarms" + ) // runCatchingCancellable keeps a cancelled save from being reported to the UI as a save failure. return runCatchingCancellable { reminderService.createAlarm( diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/util/PermissionUtils.java b/onebusaway-android/src/main/java/org/onebusaway/android/util/PermissionUtils.java index 49bc944415..bf22d81ed2 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/util/PermissionUtils.java +++ b/onebusaway-android/src/main/java/org/onebusaway/android/util/PermissionUtils.java @@ -23,8 +23,6 @@ public class PermissionUtils { - public static final int NOTIFICATION_PERMISSION_REQUEST = 5; - public static final String[] LOCATION_PERMISSIONS = { Manifest.permission.ACCESS_FINE_LOCATION, Manifest.permission.ACCESS_COARSE_LOCATION }; diff --git a/onebusaway-android/src/main/res/values/donottranslate.xml b/onebusaway-android/src/main/res/values/donottranslate.xml index 36d29b37aa..215cecca1b 100644 --- a/onebusaway-android/src/main/res/values/donottranslate.xml +++ b/onebusaway-android/src/main/res/values/donottranslate.xml @@ -63,6 +63,8 @@ preference_never_show_destination_reminder_beta_dialog preferences_key_user_denied_location_permissions preferences_display_test_alerts + preference_push_test_device + preference_push_test_device_name https://regions.onebusaway.org/regions-v3.json diff --git a/onebusaway-android/src/main/res/values/strings.xml b/onebusaway-android/src/main/res/values/strings.xml index ef21707faf..5d583400ee 100644 --- a/onebusaway-android/src/main/res/values/strings.xml +++ b/onebusaway-android/src/main/res/values/strings.xml @@ -1191,6 +1191,11 @@ More Info Display test alerts Display test-wide alerts for regions + Register as test device + Register this device in the test-only audience for previewing service-alert push notifications. Requires a test device name. + Not in effect yet — set a test device name below. Until then, this device registers normally. + Test device name + Identifies this device to transit agency admins, for example “Sam’s Pixel 9”. Until this is set, the device registers normally instead of as a test device. No description available Close donation card Zoom out diff --git a/onebusaway-android/src/test/java/org/onebusaway/android/push/DeriveDesiredRegistrationTest.kt b/onebusaway-android/src/test/java/org/onebusaway/android/push/DeriveDesiredRegistrationTest.kt new file mode 100644 index 0000000000..335954957c --- /dev/null +++ b/onebusaway-android/src/test/java/org/onebusaway/android/push/DeriveDesiredRegistrationTest.kt @@ -0,0 +1,142 @@ +/* + * Copyright (C) 2026 Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.onebusaway.android.push + +import org.junit.Assert.assertEquals +import org.junit.Test +import org.onebusaway.android.region.Region +import org.onebusaway.android.region.region + +/** + * Unit tests for [deriveDesiredRegistration] — the pure classification of the raw inputs into + * wanted / opted-out / no-sidecar / not-yet-resolved. The load-bearing distinctions: + * [DesiredRegistration.NoSidecar] vs [DesiredRegistration.Unresolved] (`NoSidecar` reconciles a stale + * registration away, `Unresolved` freezes reconciliation until the input settles — collapsing the two + * under a nullable target is exactly the bug that let a region-A registration outlive a switch to a + * sidecar-less region B), and [DesiredRegistration.OptedOut] vs `NoSidecar` (an OS-level opt-out must + * NOT unregister — see [DesiredRegistration.OptedOut] for the rationale). + */ +class DeriveDesiredRegistrationTest { + + private val sidecarRegion: Region = region(1).copy(sidecarBaseUrl = "https://sidecar.example.org") + + private fun derive( + notificationsEnabled: Boolean = true, + region: Region? = sidecarRegion, + token: String = "token-a", + locale: String = "en-US", + testDeviceEnabled: Boolean = false, + testDeviceName: String? = null + ) = deriveDesiredRegistration( + notificationsEnabled = notificationsEnabled, + region = region, + token = token, + locale = locale, + testDeviceEnabled = testDeviceEnabled, + testDeviceName = testDeviceName + ) + + @Test + fun `all inputs resolved yields the wanted registration`() { + assertEquals( + DesiredRegistration.Wanted( + PushRegistration( + regionId = 1L, + sidecarBaseUrl = "https://sidecar.example.org", + token = "token-a", + locale = "en-US", + description = null + ) + ), + derive() + ) + } + + @Test + fun `notifications off is a definitive opt-out regardless of the other inputs`() { + assertEquals(DesiredRegistration.OptedOut, derive(notificationsEnabled = false)) + // Definitive even while the async inputs are still settling: opting out never waits. + assertEquals(DesiredRegistration.OptedOut, derive(notificationsEnabled = false, region = null, token = "")) + } + + @Test + fun `a missing region is unresolved, not no-sidecar`() { + // The region flow is briefly null at cold start (RegionRepository seeds it asynchronously); + // reading that as NoSidecar would DELETE the registration and re-POST it seconds later. + assertEquals(DesiredRegistration.Unresolved, derive(region = null)) + } + + @Test + fun `a region without a sidecar host is definitively no-sidecar, not unresolved`() { + // The regression this type exists to prevent: a resolved region whose sidecarBaseUrl is blank + // (the field defaults to "") is a fact — there is nowhere to be registered — so a registration + // left over from a sidecar region must be reconciled away, not frozen in place. + assertEquals(DesiredRegistration.NoSidecar, derive(region = region(2).copy(sidecarBaseUrl = ""))) + assertEquals(DesiredRegistration.NoSidecar, derive(region = region(2).copy(sidecarBaseUrl = null))) + } + + @Test + fun `a missing FCM token is unresolved`() { + assertEquals(DesiredRegistration.Unresolved, derive(token = "")) + } + + @Test + fun `a named test device carries its description`() { + val desired = derive(testDeviceEnabled = true, testDeviceName = "Sam's Pixel") + assertEquals("Sam's Pixel", (desired as DesiredRegistration.Wanted).target.description) + assertEquals(true, desired.target.testDevice) + } + + @Test + fun `an unnamed test device downgrades to an ordinary registration`() { + // The server 422s test_device=true with a blank description, so an unnamed device must + // register as ordinary rather than POST a request guaranteed to fail (matching iOS). + for (name in listOf(null, "", " ")) { + val desired = derive(testDeviceEnabled = true, testDeviceName = name) + assertEquals(null, (desired as DesiredRegistration.Wanted).target.description) + } + } + + @Test + fun `a device name without the test-device flag is not sent`() { + val desired = derive(testDeviceEnabled = false, testDeviceName = "Sam's Pixel") + assertEquals(null, (desired as DesiredRegistration.Wanted).target.description) + } + + @Test + fun `an over-long device name is truncated to the server's limit`() { + val desired = derive(testDeviceEnabled = true, testDeviceName = "N".repeat(300)) + assertEquals( + "N".repeat(PUSH_DESCRIPTION_MAX_LENGTH), + (desired as DesiredRegistration.Wanted).target.description + ) + } + + @Test + fun `truncation never splits a surrogate pair`() { + // 254 single-unit chars + a two-unit emoji = 256 UTF-16 units: a bare take(255) would cut the + // pair in half, sending a lone surrogate (mangled to '?'/U+FFFD) as the final character. + val desired = derive(testDeviceEnabled = true, testDeviceName = "N".repeat(254) + "😀") + assertEquals("N".repeat(254), (desired as DesiredRegistration.Wanted).target.description) + } + + @Test + fun `an emoji that fits exactly within the limit is kept whole`() { + val name = "N".repeat(PUSH_DESCRIPTION_MAX_LENGTH - 2) + "😀" + val desired = derive(testDeviceEnabled = true, testDeviceName = name) + assertEquals(name, (desired as DesiredRegistration.Wanted).target.description) + } +} diff --git a/onebusaway-android/src/test/java/org/onebusaway/android/push/PushRegistrationDecisionTest.kt b/onebusaway-android/src/test/java/org/onebusaway/android/push/PushRegistrationDecisionTest.kt new file mode 100644 index 0000000000..f3654d40f3 --- /dev/null +++ b/onebusaway-android/src/test/java/org/onebusaway/android/push/PushRegistrationDecisionTest.kt @@ -0,0 +1,146 @@ +/* + * Copyright (C) 2026 Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.onebusaway.android.push + +import kotlin.time.Duration +import kotlin.time.Duration.Companion.days +import kotlin.time.Duration.Companion.hours +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.onebusaway.android.push.DesiredRegistration.NoSidecar +import org.onebusaway.android.push.DesiredRegistration.OptedOut +import org.onebusaway.android.push.DesiredRegistration.Wanted + +/** + * Unit tests for [decidePushRegistration] — the pure register/refresh/unregister reconciliation for OBACloud push + * registration (#1957). Covers each transition between the resolved desired state ([DesiredRegistration.Resolved]) + * and the registration last sent to the server, plus the [PUSH_REFRESH_INTERVAL] keep-alive. + */ +class PushRegistrationDecisionTest { + + private val base = PushRegistration( + regionId = 1L, + sidecarBaseUrl = "https://sidecar.example.org", + token = "token-a", + locale = "en-US", + description = null + ) + + /** Comfortably inside [PUSH_REFRESH_INTERVAL], so the keep-alive never confounds these cases. */ + private val fresh: Duration = 1.hours + + private fun decide( + desired: DesiredRegistration.Resolved, + last: PushRegistration?, + sinceLastSent: Duration? = fresh + ) = decidePushRegistration(desired, last, sinceLastSent) + + @Test + fun `nothing desired and nothing on record is a no-op`() { + assertEquals(PushRegistrationAction.NoOp, decide(OptedOut, last = null)) + assertEquals(PushRegistrationAction.NoOp, decide(NoSidecar, last = null)) + } + + @Test + fun `first registration when nothing is on record`() { + assertEquals(PushRegistrationAction.Register(base), decide(Wanted(base), last = null)) + } + + @Test + fun `opting out leaves the recorded registration and its server row alone`() { + // An OS-level disable never DELETEs — see [DesiredRegistration.OptedOut] for the rationale. + assertEquals(PushRegistrationAction.NoOp, decide(OptedOut, last = base)) + // Stale record while opted out changes nothing: no keep-alive POST either. + assertEquals(PushRegistrationAction.NoOp, decide(OptedOut, last = base, sinceLastSent = 30.days)) + } + + @Test + fun `losing the sidecar unregisters the recorded registration`() { + // Unlike an opt-out, a rider who moved to a sidecar-less region must stop receiving the old + // region's alerts — the sanctioned old-row DELETE on a region change. + assertEquals(PushRegistrationAction.Unregister(base), decide(NoSidecar, last = base)) + } + + @Test + fun `unchanged registration is a no-op`() { + assertEquals(PushRegistrationAction.NoOp, decide(Wanted(base), last = base)) + } + + @Test + fun `a locale change re-posts on the same token and region`() { + val target = base.copy(locale = "es-MX") + assertEquals(PushRegistrationAction.Register(target), decide(Wanted(target), last = base)) + } + + @Test + fun `a test-device flag change re-posts on the same token and region`() { + // Naming the device is what promotes it: testDevice derives from the description. + val target = base.copy(description = "Sam's Pixel") + assertTrue("naming the device must promote it to a test device", target.testDevice) + assertEquals(PushRegistrationAction.Register(target), decide(Wanted(target), last = base)) + } + + @Test + fun `an unchanged registration is re-posted once the refresh interval elapses`() { + // The keep-alive: without this the server's last_seen_at is never refreshed and the 180-day + // prune silently drops a device whose token, region, locale and flags simply never change. + assertEquals( + PushRegistrationAction.Register(base), + decide(Wanted(base), last = base, sinceLastSent = PUSH_REFRESH_INTERVAL) + ) + assertEquals( + PushRegistrationAction.Register(base), + decide(Wanted(base), last = base, sinceLastSent = 30.days) + ) + } + + @Test + fun `an unchanged registration is left alone just inside the refresh interval`() { + assertEquals( + PushRegistrationAction.NoOp, + decide(Wanted(base), last = base, sinceLastSent = PUSH_REFRESH_INTERVAL - 1.hours) + ) + } + + @Test + fun `an unknown last-sent time counts as stale`() { + // A record written before the timestamp slot existed. A redundant upsert is harmless; losing + // the device to the prune is not, so unknown must re-post rather than no-op. + assertEquals( + PushRegistrationAction.Register(base), + decide(Wanted(base), last = base, sinceLastSent = null) + ) + } + + @Test + fun `a token rotation unregisters the old token then registers the new one`() { + val target = base.copy(token = "token-b") + assertEquals( + PushRegistrationAction.Reregister(previous = base, target = target), + decide(Wanted(target), last = base) + ) + } + + @Test + fun `a region change unregisters the old region then registers the new one`() { + val target = base.copy(regionId = 2L, sidecarBaseUrl = "https://other.example.org") + assertEquals( + PushRegistrationAction.Reregister(previous = base, target = target), + decide(Wanted(target), last = base) + ) + } +} diff --git a/onebusaway-android/src/test/java/org/onebusaway/android/push/PushRegistrationManagerTest.kt b/onebusaway-android/src/test/java/org/onebusaway/android/push/PushRegistrationManagerTest.kt new file mode 100644 index 0000000000..98b40421ae --- /dev/null +++ b/onebusaway-android/src/test/java/org/onebusaway/android/push/PushRegistrationManagerTest.kt @@ -0,0 +1,533 @@ +/* + * Copyright (C) 2026 Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.onebusaway.android.push + +import java.io.IOException +import java.util.Locale +import kotlin.time.Duration.Companion.hours +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest +import okhttp3.ResponseBody.Companion.toResponseBody +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.onebusaway.android.R +import org.onebusaway.android.api.contract.PushRegistrationWebService +import org.onebusaway.android.region.FakeRegionRepository +import org.onebusaway.android.region.region +import org.onebusaway.android.testing.FakePreferencesRepository +import org.onebusaway.android.time.WallTime +import retrofit2.Response + +/** + * Drives [PushRegistrationManager.sync] (through the public [PushRegistrationManager.resync] trigger) + * over hand fakes to lock down the reconcile → apply → persist semantics [decidePushRegistration] can't + * see: which endpoint calls fire and, crucially, what the persisted "last registration" record becomes + * after each success/failure. The manager's two Android-only reads (OS notifications-enabled, the + * endpoint-path string) are injected as test seams, so this is a plain JVM test with no Robolectric. + * + * The headline case is [reregister that fully fails keeps previous so the next sync retries the delete]: + * a Reregister whose DELETE and POST both fail must NOT forget `previous`, or the stale region/token is + * orphaned (still pushing) until the 180-day server prune. That path is invisible to the pure decision + * test and is exactly the kind of bug a persist-semantics test catches. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class PushRegistrationManagerTest { + + private var previousLocale: Locale? = null + + @Before + fun pinLocale() { + // sync() sends Locale.getDefault().toLanguageTag(); pin it so the assertion is stable. + previousLocale = Locale.getDefault() + Locale.setDefault(Locale.US) + } + + @After + fun restoreLocale() { + previousLocale?.let { Locale.setDefault(it) } + } + + @Test + fun `registers and persists, then a no-op when nothing changed`() = runTest { + val f = Fixture(this) + f.setToken("T1") + f.sync() + + val call = f.service.registerCalls.single() + assertEquals("https://sidecar.test/api/v2/regions/1/push_registrations", call.url) + assertEquals("T1", call.token) + assertEquals("en-US", call.locale) + assertEquals(true, call.testDevice) + // The server 422s a test_device=true POST without a description. + assertEquals(DEVICE_DESCRIPTION, call.description) + assertEquals("android", call.operatingSystem) + + // Same inputs → decide() yields NoOp; no second POST despite the trigger firing again. + f.sync() + assertEquals(1, f.service.registerCalls.size) + } + + @Test + fun `does nothing when notifications are disabled and nothing is on record`() = runTest { + val f = Fixture(this) + f.setToken("T1") + f.notificationsEnabled = false + + f.sync() + + assertTrue(f.service.registerCalls.isEmpty()) + assertTrue(f.service.unregisterCalls.isEmpty()) + } + + @Test + fun `opt-out leaves the server row alone - FCM bounces and the server prune own that cleanup`() = runTest { + val f = Fixture(this).registered("T1") + + // Rider turns notifications off in system settings; the ON_START resync reconciles. An OS-level + // disable never DELETEs — see [DesiredRegistration.OptedOut] for the rationale. + f.notificationsEnabled = false + f.sync() + + assertTrue("opt-out must not DELETE", f.service.unregisterCalls.isEmpty()) + assertEquals("opt-out must not POST either", 1, f.service.registerCalls.size) + + // Re-enabling within the refresh window: the kept record still matches → pure NoOp, no + // spurious re-POST of an unchanged registration whose server row never went away. + f.notificationsEnabled = true + f.sync() + assertEquals(1, f.service.registerCalls.size) + assertTrue(f.service.unregisterCalls.isEmpty()) + } + + @Test + fun `a failed register persists nothing and the next sync retries`() = runTest { + val f = Fixture(this) + f.setToken("T1") + f.service.onRegister = { throw IOException("offline") } + + f.sync() + assertEquals(1, f.service.registerCalls.size) + + // The failure is surfaced (not swallowed silently) so an on-device report has the cause — but + // only locally: transport failures are ordinary and self-healing, and reporting every offline + // moment remotely would drown the systematic-rejection signal. + assertTrue(f.loggedWarnings.any { it.startsWith("push register failed") }) + assertTrue(f.reportedErrors.isEmpty()) + + // Nothing was persisted, so the retry is a full Register again (not a NoOp). + f.service.onRegister = { Response.success(Unit) } + f.sync() + assertEquals(2, f.service.registerCalls.size) + + // Now it's on record → the third sync is a NoOp. + f.sync() + assertEquals(2, f.service.registerCalls.size) + } + + @Test + fun `token rotation reregisters, deleting the old token and posting the new`() = runTest { + val f = Fixture(this).registered("T1") + + f.setToken("T2") + f.sync() + + assertEquals("T1", f.service.unregisterCalls.single().token) + assertEquals(listOf("T1", "T2"), f.service.registerCalls.map { it.token }) + + // The new token is on record now → NoOp. + f.sync() + assertEquals(2, f.service.registerCalls.size) + } + + @Test + fun `reregister that fully fails keeps previous so the next sync retries the delete`() = runTest { + val f = Fixture(this).registered("T1") + + // Region switch mid-offline: both the DELETE of T1 and the POST of T2 fail. + f.setToken("T2") + f.service.onUnregister = { throw IOException("offline") } + f.service.onRegister = { throw IOException("offline") } + f.sync() + + // Back online: the record must still point at T1 so this sync re-runs the full Reregister + // (DELETE T1 + POST T2). If `previous` had been forgotten, this would be a plain Register(T2) + // and T1 would never be deleted — the orphan bug. + f.service.onUnregister = { Response.success(Unit) } + f.service.onRegister = { Response.success(Unit) } + f.sync() + + assertEquals(2, f.service.unregisterCalls.count { it.token == "T1" }) + + // T2 is finally on record → NoOp, no further POSTs. + val postsSoFar = f.service.registerCalls.size + f.sync() + assertEquals(postsSoFar, f.service.registerCalls.size) + } + + @Test + fun `reregister clears the record when the delete succeeds but the post fails`() = runTest { + val f = Fixture(this).registered("T1") + + // DELETE of T1 lands, POST of T2 fails → the server holds nothing, so the record must clear. + f.setToken("T2") + f.service.onRegister = { throw IOException("offline") } + f.sync() + assertEquals(1, f.service.unregisterCalls.size) + + // Record cleared → the retry is a plain Register(T2), with no further DELETE. + f.service.onRegister = { Response.success(Unit) } + f.sync() + assertEquals(1, f.service.unregisterCalls.size) + assertEquals("T2", f.service.registerCalls.last().token) + } + + @Test + fun `reregister with a failed delete but successful post drops the delete, best-effort`() = runTest { + val f = Fixture(this).registered("T1") + + // Reregister where DELETE(T1) fails but POST(T2) lands: T2 is recorded and the missed DELETE is + // deliberately dropped — the old row falls back to the server's 180-day prune, which is the iOS + // client's baseline for every region change (see applyReregister). + f.setToken("T2") + f.service.onUnregister = { throw IOException("offline") } + f.sync() + + assertEquals(listOf("T1", "T2"), f.service.registerCalls.map { it.token }) + assertEquals(1, f.service.unregisterCalls.count { it.token == "T1" }) + + // T2 is settled on record: further syncs neither retry the DELETE nor re-POST. (onUnregister is + // left throwing, so this pins that no DELETE is even *attempted*.) + f.sync() + assertEquals(1, f.service.unregisterCalls.size) + assertEquals(2, f.service.registerCalls.size) + } + + @Test + fun `unregister treats a 404 as already-gone and clears the record`() = runTest { + val f = Fixture(this).registered("T1") + + // Moving to a sidecar-less region is the path that unregisters (an OS opt-out doesn't). + f.regions.emit(region(2).copy(sidecarBaseUrl = "")) + f.service.onUnregister = { Response.error(404, "".toResponseBody(null)) } + f.sync() + assertEquals(1, f.service.unregisterCalls.size) + + // 404 counts as removed → the record is cleared, so a further resync is a NoOp. + f.sync() + assertEquals(1, f.service.unregisterCalls.size) + } + + @Test + fun `an ordinary registration omits the description`() = runTest { + val f = Fixture(this) + f.prefs.setBoolean(R.string.preference_key_push_test_device, false) + f.setToken("T1") + f.sync() + + // Null means Retrofit drops the field: the server doesn't require it here, and an ordinary + // rider's device name has no business being sent. + val call = f.service.registerCalls.single() + assertEquals(false, call.testDevice) + assertEquals(null, call.description) + } + + @Test + fun `an unnamed test device registers as an ordinary device instead of failing`() = runTest { + val f = Fixture(this) + // Flag on, but no name: the server would 422 a test_device=true POST with a blank description, + // so we must downgrade rather than send a request guaranteed to fail (matching iOS). + f.prefs.setString(R.string.preference_key_push_test_device_name, null) + f.setToken("T1") + f.sync() + + val call = f.service.registerCalls.single() + assertEquals(false, call.testDevice) + assertEquals(null, call.description) + assertTrue("a downgraded registration must still succeed", f.loggedWarnings.isEmpty()) + } + + @Test + fun `an over-long test device name is truncated to the server's limit`() = runTest { + val f = Fixture(this) + // The settings write boundary caps this, so an over-long value can only arrive by writing the + // pref directly (as here). Guard it at the wire anyway: OBACloud 422s a description over 255 + // chars, and a rejected registration persists nothing, so the doomed POST would repeat on + // every foreground. + f.prefs.setString(R.string.preference_key_push_test_device_name, "N".repeat(300)) + f.setToken("T1") + f.sync() + + val call = f.service.registerCalls.single() + assertEquals(true, call.testDevice) + assertEquals("N".repeat(PUSH_DESCRIPTION_MAX_LENGTH), call.description) + } + + @Test + fun `renaming the test device re-posts the new name without a delete`() = runTest { + val f = Fixture(this).registered("T1") + assertEquals(DEVICE_DESCRIPTION, f.service.registerCalls.single().description) + + f.prefs.setString(R.string.preference_key_push_test_device_name, "Renamed Device") + f.sync() + + // Same endpoint (region + host + token), so this upserts in place — no DELETE. + assertEquals("Renamed Device", f.service.registerCalls.last().description) + assertTrue(f.service.unregisterCalls.isEmpty()) + + // Now on record → settled. + f.sync() + assertEquals(2, f.service.registerCalls.size) + } + + @Test + fun `an unchanged registration is re-posted once a day to defeat the 180-day prune`() = runTest { + val f = Fixture(this).registered("T1") + assertEquals(1, f.service.registerCalls.size) + + // Same inputs, an hour later: still a no-op. + f.now += 1.hours + f.sync() + assertEquals(1, f.service.registerCalls.size) + + // Past the refresh window: re-POST so the server's last_seen_at is refreshed. + f.now += PUSH_REFRESH_INTERVAL + f.sync() + assertEquals(2, f.service.registerCalls.size) + assertTrue("the keep-alive must not DELETE anything", f.service.unregisterCalls.isEmpty()) + + // The refresh restamps the clock, so the very next sync is quiet again. + f.sync() + assertEquals(2, f.service.registerCalls.size) + } + + @Test + fun `a backwards device-clock jump refreshes once instead of starving the keep-alive`() = runTest { + val f = Fixture(this).registered("T1") + + // The clock is set back past the record's stamp: elapsed-since-sent would read negative, which + // must count as stale — otherwise the keep-alive stays silent until the clock catches back up, + // within reach of the 180-day prune for a large jump. + f.now -= 2.hours + f.sync() + assertEquals(2, f.service.registerCalls.size) + + // The refresh restamped at the new clock, so the next sync is quiet again. + f.sync() + assertEquals(2, f.service.registerCalls.size) + } + + @Test + fun `a missing region is treated as not-yet-known rather than resolved`() = runTest { + val f = Fixture(this).registered("T1") + + // The region flow is briefly null at cold start (RegionRepository seeds it asynchronously). + // Treating that as no-sidecar would DELETE the registration and re-POST seconds later. + f.regions.emit(null) + f.sync() + + assertTrue("a null region must not trigger a DELETE", f.service.unregisterCalls.isEmpty()) + assertEquals(1, f.service.registerCalls.size) + + // Once the region resolves, the unchanged registration is still on record → no re-POST. + f.regions.emit(region(1).copy(sidecarBaseUrl = "https://sidecar.test")) + f.sync() + assertEquals(1, f.service.registerCalls.size) + } + + @Test + fun `switching to a region without a sidecar unregisters the stale registration`() = runTest { + val f = Fixture(this).registered("T1") + + // Region 2 has no sidecar host (the field defaults to ""). Unlike the null region above, this + // is a *resolved* fact, not a still-settling input — so the region-1 registration must be + // reconciled away, or the device keeps receiving region 1's alerts (and no region-2 ones) + // until the server's 180-day prune. + f.regions.emit(region(2).copy(sidecarBaseUrl = "")) + f.sync() + + val delete = f.service.unregisterCalls.single() + assertEquals("T1", delete.token) + // The DELETE targets the *old* registration's host — the current region has none. + assertTrue(delete.url, delete.url.startsWith("https://sidecar.test")) + + // Record cleared and nothing to register → settled: no re-POST, no duplicate DELETE. + f.sync() + assertEquals(1, f.service.unregisterCalls.size) + assertEquals(1, f.service.registerCalls.size) + } + + @Test + fun `an HTTP error response is logged and reported, not silently swallowed`() = runTest { + val f = Fixture(this) + f.setToken("T1") + // The real regression: a 422 is a *successful call* returning an unsuccessful response, so it + // never reaches runCatching's onFailure. It used to vanish entirely. + f.service.onRegister = { + Response.error(422, """{"error":"Unable to register device"}""".toResponseBody(null)) + } + + f.sync() + + val warning = f.loggedWarnings.single() + assertTrue("status code must be logged: $warning", warning.contains("422")) + assertTrue("server error body must be logged: $warning", warning.contains("Unable to register device")) + // The token must never reach the log. + assertTrue("token must not be logged: $warning", !warning.contains("T1")) + + // Registrations are the server's only audience source, so a systematic rejection must also be + // visible remotely (Crashlytics), not just in one developer's logcat. + val reported = f.reportedErrors.single() + assertTrue("$reported", reported is PushRegistrationException) + assertTrue("$reported", reported.message.orEmpty().contains("422")) + } + + @Test + fun `a throttled register is logged locally but not reported, and retries next sync`() = runTest { + val f = Fixture(this) + f.setToken("T1") + // A 429 says nothing about this client (see [PushRegistrationClient.isTransient]), and since a + // failed register persists nothing, every foreground would re-report it. + f.service.onRegister = { Response.error(429, "".toResponseBody(null)) } + + f.sync() + + assertTrue("throttle must still be logged", f.loggedWarnings.single().contains("429")) + assertTrue("throttle must not be reported remotely", f.reportedErrors.isEmpty()) + + // Transient means the next reconcile retries; once the throttle lifts, registration succeeds. + f.service.onRegister = { Response.success(Unit) } + f.sync() + assertEquals(2, f.service.registerCalls.size) + } + + @Test + fun `a failed unregister response is logged too, and a 5xx is not reported remotely`() = runTest { + val f = Fixture(this).registered("T1") + + f.regions.emit(region(2).copy(sidecarBaseUrl = "")) + f.service.onUnregister = { Response.error(500, "boom".toResponseBody(null)) } + f.sync() + + val warning = f.loggedWarnings.single() + assertTrue("status code must be logged: $warning", warning.contains("500")) + // Server-side trouble is transient and shared-fate across the fleet — local log only. + assertTrue("a 5xx must not be reported remotely", f.reportedErrors.isEmpty()) + } + + /** + * A ready-to-drive manager over fakes: notifications on, one region (id 1 + sidecar), the + * test-device flag on, and no token until a test sets one. [notificationsEnabled] is a mutable flag + * so a test can flip the OS opt-in between syncs. + */ + private class Fixture(private val scope: TestScope) { + val service = FakePushRegistrationWebService() + val prefs = FakePreferencesRepository().apply { + setBoolean(R.string.preference_key_push_test_device, true) + // Named, so the test-device flag is honoured rather than downgraded (see deriveDesiredRegistration). + setString(R.string.preference_key_push_test_device_name, DEVICE_DESCRIPTION) + } + val regions = FakeRegionRepository(region(1).copy(sidecarBaseUrl = "https://sidecar.test")) + var notificationsEnabled = true + + /** Mutable device clock, so a test can jump past [PUSH_REFRESH_INTERVAL]. */ + var now = WallTime(1_000_000_000L) + + /** Server rejections routed to the remote error channel (Crashlytics in production). */ + val reportedErrors = mutableListOf() + + /** Captures every warning the manager logs (android.util.Log isn't mocked under a JVM test). */ + val loggedWarnings = mutableListOf() + + // The real store and client over fakes: these tests are about the reconcile → apply → record + // loop as a whole, so exercising the actual persistence and failure-classification code (rather + // than fakes of it) is the point. + val manager = PushRegistrationManager( + client = PushRegistrationClient( + service = service, + regionsPath = "/api/v2/regions/", + logWarning = { message, _ -> loggedWarnings += message }, + reportError = { reportedErrors += it } + ), + store = PushRegistrationStore(prefs = prefs, now = { now }), + regionRepository = regions, + firebaseMessagingManager = FirebaseMessagingManager(prefs), + prefs = prefs, + scope = scope, + notificationsEnabled = { notificationsEnabled } + ) + + fun setToken(token: String) = prefs.setString(R.string.firebase_messaging_token, token) + + /** Fires the reconcile trigger and drains it — the manager runs sync() off resync()'s launch. */ + fun sync() { + manager.resync() + scope.advanceUntilIdle() + } + + /** Establishes a "[token] already registered and on record" baseline before the scenario. */ + fun registered(token: String): Fixture = apply { + setToken(token) + sync() + } + } + + /** Records every call and returns programmable outcomes (default: success). */ + private class FakePushRegistrationWebService : PushRegistrationWebService { + data class RegisterCall( + val url: String, + val token: String, + val locale: String, + val testDevice: Boolean, + val description: String?, + val operatingSystem: String + ) + + data class UnregisterCall(val url: String, val token: String) + + val registerCalls = mutableListOf() + val unregisterCalls = mutableListOf() + + var onRegister: () -> Response = { Response.success(Unit) } + var onUnregister: () -> Response = { Response.success(Unit) } + + override suspend fun register( + url: String, + token: String, + locale: String, + testDevice: Boolean, + description: String?, + operatingSystem: String + ): Response { + registerCalls += RegisterCall(url, token, locale, testDevice, description, operatingSystem) + return onRegister() + } + + override suspend fun unregister(url: String, token: String): Response { + unregisterCalls += UnregisterCall(url, token) + return onUnregister() + } + } + + private companion object { + /** Stands in for the name the rider types into the "Test device name" setting. */ + const val DEVICE_DESCRIPTION = "Sam's Test Pixel" + } +} diff --git a/onebusaway-android/src/test/java/org/onebusaway/android/push/PushRegistrationStoreTest.kt b/onebusaway-android/src/test/java/org/onebusaway/android/push/PushRegistrationStoreTest.kt new file mode 100644 index 0000000000..a385dadca3 --- /dev/null +++ b/onebusaway-android/src/test/java/org/onebusaway/android/push/PushRegistrationStoreTest.kt @@ -0,0 +1,73 @@ +/* + * Copyright (C) 2026 Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.onebusaway.android.push + +import kotlin.time.Duration.Companion.hours +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test +import org.onebusaway.android.testing.FakePreferencesRepository +import org.onebusaway.android.time.WallTime + +/** + * Unit tests for [PushRegistrationStore]'s clock edges — the [PushRegistrationStore.sinceLastSent] + * contract that [decidePushRegistration]'s keep-alive relies on. The reconcile → apply → persist loop + * as a whole is exercised in [PushRegistrationManagerTest]; these pin the store-level invariants that + * are invisible there because the decision layer happens to tolerate their violation today. + */ +class PushRegistrationStoreTest { + + private val registration = PushRegistration( + regionId = 1L, + sidecarBaseUrl = "https://sidecar.example.org", + token = "token-a", + locale = "en-US", + description = null + ) + + /** Mutable device clock, so a test can move it in either direction. */ + private var now = WallTime(1_000_000_000L) + + private val store = PushRegistrationStore(FakePreferencesRepository()) { now } + + @Test + fun `a backwards clock jump reads as unknown rather than a negative elapsed time`() { + store.record(registration) + + // The rider (or a bad network-time update) sets the device clock back: a negative elapsed time + // would keep the keep-alive comparison false until the clock caught back up — for a large jump, + // long enough for the server's 180-day prune to drop the device. + now -= 2.hours + assertNull("negative elapsed must read as unknown (= stale)", store.sinceLastSent()) + + // Once the clock is past the write again, elapsed time reads normally. + now += 3.hours + assertEquals(1.hours, store.sinceLastSent()) + } + + @Test + fun `clearing the record drops its sent-at timestamp with it`() { + store.record(registration) + now += 1.hours + + store.clear() + + assertNull(store.last()) + // The store must not report an elapsed-since-sent for a registration it says doesn't exist — + // otherwise a later record's freshness could be read against a dead record's stamp. + assertNull(store.sinceLastSent()) + } +} diff --git a/onebusaway-android/src/test/java/org/onebusaway/android/testing/FakePreferencesRepository.kt b/onebusaway-android/src/test/java/org/onebusaway/android/testing/FakePreferencesRepository.kt index 5735bbfa94..feddd7b086 100644 --- a/onebusaway-android/src/test/java/org/onebusaway/android/testing/FakePreferencesRepository.kt +++ b/onebusaway-android/src/test/java/org/onebusaway/android/testing/FakePreferencesRepository.kt @@ -18,6 +18,7 @@ package org.onebusaway.android.testing import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.flowOf +import org.onebusaway.android.preferences.PreferencesEditor import org.onebusaway.android.preferences.PreferencesRepository /** @@ -101,4 +102,8 @@ class FakePreferencesRepository(private val observeValue: Boolean = true) : Pref override fun setFloat(key: String, value: Float) { values[key] = value } + + // In-memory, so there is no commit to tear: replaying the block through the setters honors edit's + // atomicity contract exactly. + override fun edit(block: PreferencesEditor.() -> Unit) = block(this) } diff --git a/onebusaway-android/src/test/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModelTest.kt b/onebusaway-android/src/test/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModelTest.kt index 56fdd62777..451d2a51af 100644 --- a/onebusaway-android/src/test/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModelTest.kt +++ b/onebusaway-android/src/test/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModelTest.kt @@ -26,6 +26,7 @@ import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Test import org.onebusaway.android.R +import org.onebusaway.android.push.PUSH_DESCRIPTION_MAX_LENGTH import org.onebusaway.android.region.FakeRegionRepository import org.onebusaway.android.region.RegionState import org.onebusaway.android.region.RegionStatus @@ -122,6 +123,30 @@ class AdvancedSettingsViewModelTest { assertEquals("nothing was persisted", -1, prefs.getInt(mapStopCacheSize, -1)) } + private val pushTestDeviceName = R.string.preference_key_push_test_device_name + + @Test + fun `a test device name is trimmed and stored, and blank clears it`() { + val prefs = FakePreferencesRepository() + + assertTrue("any non-blank name is accepted", applyPushTestDeviceName(" Sam's Pixel ", prefs)) + assertEquals("Sam's Pixel", prefs.getString(pushTestDeviceName, null)) + + assertTrue("blank is accepted too", applyPushTestDeviceName(" ", prefs)) + assertNull("blank clears the slot, downgrading to an ordinary device", prefs.getString(pushTestDeviceName, null)) + } + + @Test + fun `a test device name is capped at the server's limit before it is stored`() { + val prefs = FakePreferencesRepository() + + // Normalizing here — the only boundary where this value enters persistence — is what keeps the + // stored name a legal `description`, so settings shows exactly what goes on the wire. + applyPushTestDeviceName("N".repeat(300), prefs) + + assertEquals("N".repeat(PUSH_DESCRIPTION_MAX_LENGTH), prefs.getString(pushTestDeviceName, null)) + } + @Test fun `a re-resolve that needs a manual pick defers the OTP reset until the user chooses`() = runTest { val regions = listOf(region(1), region(2)) diff --git a/onebusaway-android/src/test/java/org/onebusaway/android/ui/settings/SettingsUiStateTest.kt b/onebusaway-android/src/test/java/org/onebusaway/android/ui/settings/SettingsUiStateTest.kt index 9a257e3de5..24369ff910 100644 --- a/onebusaway-android/src/test/java/org/onebusaway/android/ui/settings/SettingsUiStateTest.kt +++ b/onebusaway-android/src/test/java/org/onebusaway/android/ui/settings/SettingsUiStateTest.kt @@ -126,6 +126,8 @@ class SettingsUiStateTest { private val advPrefs = AdvancedPrefSnapshot( experimentalRegionsEnabled = false, displayTestAlerts = false, + pushTestDevice = false, + pushTestDeviceName = null, customObaApiUrl = null, customOtpApiUrl = null, customOtpApiUrlUsesGraphQl = false,