Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -284,19 +297,24 @@ 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: " +
"SiteStore error ${error?.type}: ${error?.message}"
)
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,
)
}

Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -388,7 +414,8 @@ class ApplicationPasswordLoginViewModel @Inject constructor(
emitError(
siteUrl = currentUrlLogin?.siteUrl.orEmpty(),
errorMessage = errorCode,
cause = cause
cause = cause,
reportToSentry = reportToSentry,
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ class ApplicationPasswordLoginHelper @Inject constructor(
return uriLoginWrapper.isUserRejectedAuthorization(rawData)
}

private fun findSiteByUrl(
internal fun findSiteByUrl(
normalizedUrl: String?,
sites: List<SiteModel>
): SiteModel? {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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<String?>(0)
val sites = invocation.getArgument<List<SiteModel>>(1)
sites.firstOrNull { UrlUtils.normalizeUrl(it.url) == normalizedUrl }
}
}

@Test
Expand Down Expand Up @@ -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 {
Expand Down
Loading