diff --git a/WordPress/src/main/java/org/wordpress/android/ui/accounts/applicationpassword/ApplicationPasswordLoginViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/accounts/applicationpassword/ApplicationPasswordLoginViewModel.kt index c872f169dcfe..11603a639520 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/accounts/applicationpassword/ApplicationPasswordLoginViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/accounts/applicationpassword/ApplicationPasswordLoginViewModel.kt @@ -27,6 +27,7 @@ import org.wordpress.android.util.AppLog import org.wordpress.android.util.UrlUtils import com.automattic.android.tracks.crashlogging.CrashLogging import org.wordpress.android.util.crashlogging.sendReportWithTag +import java.io.IOException import javax.inject.Inject import javax.inject.Named @@ -202,10 +203,19 @@ class ApplicationPasswordLoginViewModel @Inject constructor( applicationPasswordLoginHelper.trackStoringFailed( siteUrl, "fetch_sites_exception", creationSource ) - emitError(siteUrl = siteUrl, errorMessage = e.message, cause = e) + emitError( + siteUrl = siteUrl, + errorMessage = e.message, + cause = e, + reportToSentry = !e.isRecoverableNetworkOrDiscoveryError(), + ) } } + private fun Throwable.isRecoverableNetworkOrDiscoveryError(): Boolean = + generateSequence(this as Throwable?) { it.cause } + .any { it is IOException || it is SelfHostedEndpointFinder.DiscoveryException } + private suspend fun discoverAndDispatchFetchSite( username: String, password: String, @@ -252,11 +262,14 @@ class ApplicationPasswordLoginViewModel @Inject constructor( private suspend fun emitError( siteUrl: String, errorMessage: String? = null, - cause: Throwable? = null + cause: Throwable? = null, + reportToSentry: Boolean = true, ) { - val exception = cause - ?: Exception("Application password login failed: $errorMessage") - crashLogging.sendReportWithTag(exception, AppLog.T.MAIN) + if (reportToSentry) { + val exception = cause + ?: Exception("Application password login failed: $errorMessage") + crashLogging.sendReportWithTag(exception, AppLog.T.MAIN) + } _onFinishedEvent.emit( NavigationActionData( showSiteSelector = false, @@ -284,6 +297,7 @@ class ApplicationPasswordLoginViewModel @Inject constructor( private suspend fun handleSiteChangedError(event: OnSiteChanged) { waitingForFetchedSite = false val error = event.error + val errorTypeName = error?.type?.name ?: "unknown" appLogWrapper.e( AppLog.T.MAIN, "A_P: onSiteChanged failed: " + @@ -291,12 +305,16 @@ class ApplicationPasswordLoginViewModel @Inject constructor( ) applicationPasswordLoginHelper.trackStoringFailed( currentUrlLogin?.siteUrl, - "site_changed_failed", + "site_changed_failed:$errorTypeName", creationSource ) + // Don't report SiteStore errors to Sentry — every transient network + // failure, auth failure, or backend hiccup ends up here, drowning out + // genuine bugs. The reason is preserved in analytics + AppLog above. emitError( siteUrl = currentUrlLogin?.siteUrl.orEmpty(), - errorMessage = "site_store_error" + errorMessage = "site_store_error", + reportToSentry = false, ) } @@ -307,24 +325,31 @@ class ApplicationPasswordLoginViewModel @Inject constructor( UrlUtils.normalizeUrl(currentUrlLogin?.siteUrl) val site = try { - siteStore.sites.firstOrNull { - UrlUtils.normalizeUrl(it.url) == normalizedUrl - } + // Reuse the helper's tolerant matching (exact, then ignoring scheme + // and www) so site_not_found doesn't fire on cosmetic URL differences + // between the callback URL and the stored SiteModel. + applicationPasswordLoginHelper.findSiteByUrl( + normalizedUrl, siteStore.sites + ) } catch (e: Exception) { logAndEmitSiteChangedError( logMessage = "exception reading sites from DB: " + e.stackTraceToString(), errorCode = "db_read_exception", - cause = e + cause = e, ) return } val validationError = validateSiteChanged(event, site) if (validationError != null) { + // These validation outcomes (site_not_found, no_rows_affected, + // empty_credentials) are user-recoverable login failures, not bugs; + // analytics + AppLog are enough. logAndEmitSiteChangedError( logMessage = validationError.logMessage, - errorCode = validationError.errorCode + errorCode = validationError.errorCode, + reportToSentry = false, ) } else { val resolvedSite = site ?: return @@ -374,7 +399,8 @@ class ApplicationPasswordLoginViewModel @Inject constructor( private suspend fun logAndEmitSiteChangedError( logMessage: String, errorCode: String, - cause: Throwable? = null + cause: Throwable? = null, + reportToSentry: Boolean = true, ) { appLogWrapper.e( AppLog.T.MAIN, @@ -388,7 +414,8 @@ class ApplicationPasswordLoginViewModel @Inject constructor( emitError( siteUrl = currentUrlLogin?.siteUrl.orEmpty(), errorMessage = errorCode, - cause = cause + cause = cause, + reportToSentry = reportToSentry, ) } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelper.kt b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelper.kt index b5d8d2f398f8..38d85c05b5d3 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelper.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelper.kt @@ -285,7 +285,7 @@ class ApplicationPasswordLoginHelper @Inject constructor( return uriLoginWrapper.isUserRejectedAuthorization(rawData) } - private fun findSiteByUrl( + internal fun findSiteByUrl( normalizedUrl: String?, sites: List ): SiteModel? { diff --git a/WordPress/src/test/java/org/wordpress/android/ui/accounts/applicationpassword/ApplicationPasswordLoginViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/accounts/applicationpassword/ApplicationPasswordLoginViewModelTest.kt index a89497e9acd6..3c0fe57b7704 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/accounts/applicationpassword/ApplicationPasswordLoginViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/accounts/applicationpassword/ApplicationPasswordLoginViewModelTest.kt @@ -12,6 +12,7 @@ import org.mockito.kotlin.any import org.mockito.kotlin.argThat import org.mockito.kotlin.eq import org.mockito.kotlin.mock +import org.mockito.kotlin.never import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever @@ -24,6 +25,8 @@ import org.wordpress.android.fluxc.store.SiteStore import org.wordpress.android.fluxc.utils.AppLogWrapper import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper.StoreCredentialsResult +import org.wordpress.android.util.UrlUtils +import java.io.IOException import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertTrue @@ -77,6 +80,14 @@ class ApplicationPasswordLoginViewModelTest : BaseUnitTest() { crashLogging ) whenever(applicationPasswordLoginHelper.getSiteUrlLoginFromRawData(rawData)).thenReturn(urlLogin) + // Mirror the helper's exact-match behavior so existing tests that stub + // siteStore.sites still find their fixture site by URL. Tests that need + // tolerant or no-match behavior can override this stub. + whenever(applicationPasswordLoginHelper.findSiteByUrl(any(), any())).thenAnswer { invocation -> + val normalizedUrl = invocation.getArgument(0) + val sites = invocation.getArgument>(1) + sites.firstOrNull { UrlUtils.normalizeUrl(it.url) == normalizedUrl } + } } @Test @@ -533,6 +544,109 @@ class ApplicationPasswordLoginViewModelTest : BaseUnitTest() { } } + @Test + fun `given onSiteChanged with error, then no Sentry report is sent`() = runTest { + // Given + setupFetchSitesFlow() + val siteError = SiteStore.SiteError( + SiteStore.SiteErrorType.GENERIC_ERROR, "boom" + ) + + // When + viewModel.onFinishedEvent.test { + viewModel.setupSite(rawData) + viewModel.onSiteChanged(SiteStore.OnSiteChanged(0, siteError)) + + // Then — error surfaces in UI but is not noised into Sentry + val result = awaitItem() + assertTrue(result.isError) + assertEquals("site_store_error", result.errorMessage) + verify(crashLogging, never()).sendReport(any(), any(), any()) + cancelAndIgnoreRemainingEvents() + } + } + + @Test + fun `given site_not_found validation, then no Sentry report is sent`() = runTest { + // Given — site list is empty so validateSiteChanged hits site_not_found + setupFetchSitesFlow() + whenever(siteStore.sites).thenReturn(emptyList()) + + // When + viewModel.onFinishedEvent.test { + viewModel.setupSite(rawData) + viewModel.onSiteChanged(SiteStore.OnSiteChanged(rowsAffected = 1)) + + // Then + val result = awaitItem() + assertTrue(result.isError) + assertEquals("site_not_found", result.errorMessage) + verify(crashLogging, never()).sendReport(any(), any(), any()) + cancelAndIgnoreRemainingEvents() + } + } + + @Test + fun `given fetchSites with IOException cause, then no Sentry report is sent`() = runTest { + // Given — verifyOrDiscoverXMLRPCEndpoint only declares DiscoveryException, + // so use an unchecked exception with an IOException cause to exercise the + // cause-chain traversal in isRecoverableNetworkOrDiscoveryError. + whenever( + applicationPasswordLoginHelper + .storeApplicationPasswordCredentialsFrom(eq(urlLogin), any()) + ).thenReturn(StoreCredentialsResult.SiteNotFound) + whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(any())) + .thenThrow(RuntimeException("wrapped", IOException("offline"))) + + // When + viewModel.onFinishedEvent.test { + viewModel.setupSite(rawData) + + // Then + val result = awaitItem() + assertTrue(result.isError) + verify(crashLogging, never()).sendReport(any(), any(), any()) + cancelAndIgnoreRemainingEvents() + } + } + + @Test + fun `given onSiteChanged success and site matched via tolerant URL match, then emit success`() = runTest { + // Given — stored URL has www. prefix while callback URL does not. Without + // the tolerant fallback the lookup would fail with site_not_found. + val storedSite = SiteModel().apply { + apiRestUsernamePlain = urlLogin.user + apiRestPasswordPlain = urlLogin.password + url = "https://www.example.com" + } + whenever(siteStore.hasSite()).thenReturn(true) + whenever(siteStore.sites).thenReturn(listOf(storedSite)) + whenever(applicationPasswordLoginHelper.findSiteByUrl(any(), any())) + .thenReturn(storedSite) + whenever( + applicationPasswordLoginHelper + .storeApplicationPasswordCredentialsFrom(eq(urlLogin), any()) + ).thenReturn(StoreCredentialsResult.SiteNotFound) + whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(any())) + .thenReturn("https://example.com/xmlrpc.php") + + // When + viewModel.onFinishedEvent.test { + viewModel.setupSite(rawData) + viewModel.onSiteChanged( + SiteStore.OnSiteChanged( + rowsAffected = 1, + updatedSites = listOf(storedSite) + ) + ) + + // Then + val result = awaitItem() + assertFalse(result.isError) + cancelAndIgnoreRemainingEvents() + } + } + @Test fun `given DiscoveryException, when fetchSites, then dispatch WPAPI fallback`() = runTest {