diff --git a/app/src/main/java/app/closer/domain/usecase/GoogleProfileMerger.kt b/app/src/main/java/app/closer/domain/usecase/GoogleProfileMerger.kt index fb334471..c80b129e 100644 --- a/app/src/main/java/app/closer/domain/usecase/GoogleProfileMerger.kt +++ b/app/src/main/java/app/closer/domain/usecase/GoogleProfileMerger.kt @@ -1,6 +1,6 @@ package app.closer.domain.usecase -import android.util.Log +import app.closer.core.crash.CrashReporter import app.closer.domain.model.GoogleSignInResult import app.closer.domain.model.User import app.closer.domain.model.isSignInStub @@ -11,9 +11,13 @@ import javax.inject.Singleton /** * Seeds a user's profile from what Google already knows (name, photo) at sign-in. * - * Best-effort by design: it fills gaps for a new account and otherwise stays out of the way. It ran - * as identical copies inside both `LoginViewModel` and `SignUpViewModel`, which is how the same bug - * came to exist twice. + * Best-effort by design, and [merge] never throws: by the time it runs, authentication has already + * succeeded, so a cosmetic seed failing must cost nothing — the caller sets its success state on the + * next line, and an escaping exception here would kill a sign-in that actually worked. Failures are + * recorded to [CrashReporter] instead. + * + * It ran as identical copies inside both `LoginViewModel` and `SignUpViewModel`, which is how the + * same bug came to exist twice. * * The bug: it decided "this is a new account" from `runCatching { getUser(uid) }.getOrNull()`, which * returns null both when the user genuinely has no document *and* when the read simply failed. On the @@ -22,9 +26,15 @@ import javax.inject.Singleton */ @Singleton class GoogleProfileMerger @Inject constructor( - private val userRepository: UserRepository + private val userRepository: UserRepository, + private val crashReporter: CrashReporter ) { suspend fun merge(result: GoogleSignInResult) { + runCatching { seedProfile(result) } + .onFailure { crashReporter.recordException(it) } + } + + private suspend fun seedProfile(result: GoogleSignInResult) { val uid = result.uid if (uid.isBlank()) return @@ -32,7 +42,7 @@ class GoogleProfileMerger @Inject constructor( if (read.isFailure) { // Can't tell a new account from an unreadable one. Writing either way risks overwriting a // real profile, and the only thing at stake here is a pre-filled name — so do nothing. - Log.w(TAG, "Couldn't read the profile at Google sign-in; leaving it untouched", read.exceptionOrNull()) + read.exceptionOrNull()?.let { crashReporter.recordException(it) } return } val existing = read.getOrNull() @@ -55,7 +65,7 @@ class GoogleProfileMerger @Inject constructor( // field blank. Filling it in from Google would overwrite the real name of a returning user // whose profile simply hadn't synced yet. Their name is already set — leave it alone. if (existing.isSignInStub) { - Log.w(TAG, "Profile not synced yet at Google sign-in; not seeding name/photo over it") + crashReporter.log("GoogleProfileMerger: profile not synced yet at sign-in; not seeding over it") return } @@ -66,8 +76,4 @@ class GoogleProfileMerger @Inject constructor( userRepository.updatePhotoUrl(uid, result.photoUrl) } } - - private companion object { - const val TAG = "GoogleProfileMerger" - } } diff --git a/app/src/main/java/app/closer/ui/pairing/RecoveryScreen.kt b/app/src/main/java/app/closer/ui/pairing/RecoveryScreen.kt index 75ac1db5..f6cd5d8e 100644 --- a/app/src/main/java/app/closer/ui/pairing/RecoveryScreen.kt +++ b/app/src/main/java/app/closer/ui/pairing/RecoveryScreen.kt @@ -20,6 +20,7 @@ import androidx.compose.material3.Button import androidx.compose.material3.ButtonDefaults import app.closer.ui.components.CloserHeartLoader import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.OutlinedButton import androidx.compose.material3.OutlinedTextField import androidx.compose.material3.OutlinedTextFieldDefaults import androidx.compose.material3.Scaffold @@ -162,7 +163,7 @@ fun RecoveryScreen( // It stays *below* the field rather than replacing it: someone who does have the phrase // (pasted from their partner's message) is unlocked instantly and offline, whereas this // path has to wait on the partner tapping consent. - androidx.compose.material3.OutlinedButton( + OutlinedButton( onClick = onPartnerRestore, enabled = !state.isLoading, modifier = Modifier.fillMaxWidth().heightIn(min = 56.dp), diff --git a/app/src/test/java/app/closer/crypto/RecoveryPhraseNormalizationTest.kt b/app/src/test/java/app/closer/crypto/RecoveryPhraseNormalizationTest.kt index 7cab821d..c065ef65 100644 --- a/app/src/test/java/app/closer/crypto/RecoveryPhraseNormalizationTest.kt +++ b/app/src/test/java/app/closer/crypto/RecoveryPhraseNormalizationTest.kt @@ -1,6 +1,7 @@ package app.closer.crypto import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals import org.junit.Test /** @@ -52,8 +53,8 @@ class RecoveryPhraseNormalizationTest { @Test fun aGenuinelyWrongPhraseStillDiffers() { // Normalisation must not make wrong phrases pass: only the *presentation* is folded. - assert(norm("hunt down gear east lead over drop live more cat") != canonical) - assert(norm("hunt down gear east lead over drop live more") != canonical) + assertNotEquals(canonical, norm("hunt down gear east lead over drop live more cat")) + assertNotEquals(canonical, norm("hunt down gear east lead over drop live more")) } @Test diff --git a/app/src/test/java/app/closer/domain/usecase/GoogleProfileMergerTest.kt b/app/src/test/java/app/closer/domain/usecase/GoogleProfileMergerTest.kt new file mode 100644 index 00000000..362a0086 --- /dev/null +++ b/app/src/test/java/app/closer/domain/usecase/GoogleProfileMergerTest.kt @@ -0,0 +1,130 @@ +package app.closer.domain.usecase + +import app.closer.core.crash.CrashReporter +import app.closer.domain.model.GoogleSignInResult +import app.closer.domain.model.User +import app.closer.domain.repository.UserRepository +import io.mockk.Runs +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.every +import io.mockk.just +import io.mockk.mockk +import io.mockk.slot +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * The merger runs right after a *successful* Google sign-in, from inside the view model's + * `onSuccess` block — so its one hard contract is that it never throws: an escaping exception there + * is an unhandled coroutine exception that kills a sign-in that actually worked. Everything else it + * does is optional polish; failures go to [CrashReporter] and the sign-in proceeds. + * + * The write-safety half (a failed read must not be mistaken for "new account" — C-AUTH-001's + * conflation) is asserted here too, since this class is where that decision now lives. + */ +class GoogleProfileMergerTest { + + private val userRepository: UserRepository = mockk() + private val crashReporter: CrashReporter = mockk(relaxUnitFun = true) + private val merger = GoogleProfileMerger(userRepository, crashReporter) + + private val googleResult = GoogleSignInResult( + uid = "u1", + email = "ava@example.com", + displayName = "Ava", + photoUrl = "https://example.com/a.jpg" + ) + + // Genuinely complete — including the photo. Leaving photoUrl at its "" default here would make + // the merger legitimately try to fill it, and the strict mock's throw would then be swallowed by + // the very runCatching under test: a test that passes for the wrong reason. The mutation check + // caught exactly that. + private val pairedUser = User( + id = "u1", + email = "ava@example.com", + displayName = "enc:v1:name", + photoUrl = "https://example.com/existing.jpg", + sex = "enc:v1:sex", + coupleId = "couple1", + partnerId = "ben", + createdAt = 1_700_000_000_000L + ) + + @Test + fun aFailedReadWritesNothingAndIsRecordedNotThrown() = runTest { + // The C-AUTH-001 conflation: read failure must not read as "new account". + val boom = RuntimeException("firestore unavailable") + coEvery { userRepository.getUser("u1") } throws boom + + merger.merge(googleResult) // must not throw + + coVerify(exactly = 0) { userRepository.createUser(any()) } + coVerify(exactly = 0) { userRepository.updateDisplayName(any(), any()) } + val recorded = slot() + coVerify { crashReporter.recordException(capture(recorded)) } + assertEquals(boom, recorded.captured) + } + + @Test + fun aFailedWriteIsRecordedNotThrown() = runTest { + // Auth already succeeded; a cosmetic seed failing must not kill the sign-in. + coEvery { userRepository.getUser("u1") } returns null + coEvery { userRepository.createUser(any()) } throws RuntimeException("write refused") + + merger.merge(googleResult) // must not throw + + coVerify { crashReporter.recordException(any()) } + } + + @Test + fun aMissingDocumentSeedsANewProfile() = runTest { + coEvery { userRepository.getUser("u1") } returns null + coEvery { userRepository.createUser(any()) } just Runs + + merger.merge(googleResult) + + val created = slot() + coVerify { userRepository.createUser(capture(created)) } + assertEquals("Ava", created.captured.displayName) + assertEquals("ava@example.com", created.captured.email) + } + + @Test + fun aSignInStubIsNotSeededOver() = runTest { + // The FCM-materialised document: exists, nothing on it. Seeding Google's name over it would + // overwrite the real name of a returning user whose profile simply hadn't synced yet. + coEvery { userRepository.getUser("u1") } returns User(id = "u1", createdAt = 0L, lastActiveAt = 0L) + every { crashReporter.log(any()) } just Runs + + merger.merge(googleResult) + + coVerify(exactly = 0) { userRepository.createUser(any()) } + coVerify(exactly = 0) { userRepository.updateDisplayName(any(), any()) } + coVerify(exactly = 0) { userRepository.updatePhotoUrl(any(), any()) } + } + + @Test + fun aCompleteProfileIsLeftAlone() = runTest { + coEvery { userRepository.getUser("u1") } returns pairedUser + + merger.merge(googleResult) + + coVerify(exactly = 0) { userRepository.createUser(any()) } + coVerify(exactly = 0) { userRepository.updateDisplayName(any(), any()) } + coVerify(exactly = 0) { userRepository.updatePhotoUrl(any(), any()) } + } + + @Test + fun onlyTheBlankFieldsAreFilledIn() = runTest { + // A real (non-stub) profile missing just the photo: fill the photo, leave the name. + coEvery { userRepository.getUser("u1") } returns pairedUser.copy(photoUrl = "") + coEvery { userRepository.updatePhotoUrl(any(), any()) } just Runs + + merger.merge(googleResult) + + coVerify { userRepository.updatePhotoUrl("u1", "https://example.com/a.jpg") } + coVerify(exactly = 0) { userRepository.updateDisplayName(any(), any()) } + } +}