From 19a9d76122215b7d76872c5c61f45a07b6398bb5 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Thu, 4 Jun 2026 11:50:00 -0600 Subject: [PATCH 01/12] Unify editor capability detection behind a single per-site detector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces EditorCapabilityDetector — one app-scoped owner of editor REST capability state, exposed as a per-site StateFlow — so the connectivity banner and editor preloader share a single, deduplicated probe instead of each re-deriving the state and racing the same async preconditions. Folds in the authenticated direct-host probe fallback so private Atomic sites detect correctly on trunk. Part of #22942. --- .../org/wordpress/android/AppInitializer.kt | 7 + .../repositories/EditorCapabilityDetector.kt | 158 ++++++++++++ .../ApplicationPasswordViewModelSlice.kt | 20 +- .../SiteConnectivityBannerViewModelSlice.kt | 83 ++---- .../ui/posts/GutenbergEditorPreloader.kt | 14 +- .../EditorCapabilityDetectorTest.kt | 207 +++++++++++++++ .../ApplicationPasswordViewModelSliceTest.kt | 33 +-- ...iteConnectivityBannerViewModelSliceTest.kt | 242 +++++------------- .../ui/posts/GutenbergEditorPreloaderTest.kt | 11 +- 9 files changed, 509 insertions(+), 266 deletions(-) create mode 100644 WordPress/src/main/java/org/wordpress/android/repositories/EditorCapabilityDetector.kt create mode 100644 WordPress/src/test/java/org/wordpress/android/repositories/EditorCapabilityDetectorTest.kt diff --git a/WordPress/src/main/java/org/wordpress/android/AppInitializer.kt b/WordPress/src/main/java/org/wordpress/android/AppInitializer.kt index 300566324748..722d06809f77 100644 --- a/WordPress/src/main/java/org/wordpress/android/AppInitializer.kt +++ b/WordPress/src/main/java/org/wordpress/android/AppInitializer.kt @@ -71,6 +71,7 @@ import org.wordpress.android.networking.ConnectionChangeReceiver import org.wordpress.android.networking.OAuthAuthenticator import org.wordpress.android.networking.RestClientUtils import org.wordpress.android.push.GCMRegistrationScheduler +import org.wordpress.android.repositories.EditorCapabilityDetector import org.wordpress.android.support.ZendeskHelper import org.wordpress.android.ui.ActivityId import org.wordpress.android.ui.debug.cookies.DebugCookieManager @@ -229,6 +230,9 @@ class AppInitializer @Inject constructor( @Inject lateinit var wpApiClientProvider: WpApiClientProvider + @Inject + lateinit var editorCapabilityDetector: EditorCapabilityDetector + @Inject lateinit var openWebLinksWithJetpackHelper: DeepLinkOpenWebLinksWithJetpackHelper @@ -717,6 +721,9 @@ class AppInitializer @Inject constructor( // Clear cached wordpress-rs services and API clients wpServiceProvider.clearAll() wpApiClientProvider.clearAllClients() + + // Drop per-site editor-capability detection state for the signed-out user + editorCapabilityDetector.clear() } /* diff --git a/WordPress/src/main/java/org/wordpress/android/repositories/EditorCapabilityDetector.kt b/WordPress/src/main/java/org/wordpress/android/repositories/EditorCapabilityDetector.kt new file mode 100644 index 000000000000..a8f6484e4709 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/repositories/EditorCapabilityDetector.kt @@ -0,0 +1,158 @@ +package org.wordpress.android.repositories + +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.launch +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.modules.APPLICATION_SCOPE +import org.wordpress.android.util.NetworkUtilsWrapper +import java.util.concurrent.ConcurrentHashMap +import javax.inject.Inject +import javax.inject.Named +import javax.inject.Singleton + +/** + * Single owner of "what does this site's editor REST API support" as an + * observable, per-site state. + * + * Capability detection ([EditorSettingsRepository.fetchEditorCapabilitiesForSite]) + * has two async preconditions on Atomic sites — an application password and a + * recovered REST root — provisioned elsewhere on the My Site screen. Routing + * every consumer (connectivity banner, editor preloader) through this detector + * means one probe per site, shared and deduplicated, instead of each consumer + * re-deriving the same state and racing the same preconditions. + * + * State is keyed by [SiteModel.id] — the local DB row id, stable across the + * process lifetime — mirroring `GutenbergEditorPreloader`. + * + * ## Entry points + * - [stateFor] — the reactive entry point. Returns a shared [StateFlow]; the + * first access starts detection, later accesses reuse the cached result + * (capabilities rarely change). A failed probe is retried on the next access. + * - [awaitProbe] — the one-shot entry point for callers that just need the + * probe to have run (and its capabilities persisted) before continuing. + * - [refresh] — forces a re-probe, bypassing the once-per-site gate + * (pull-to-refresh, banner retry, newly established credentials). + * - [clear] — cancels all work and drops all state; wire into sign-out. + */ +@Singleton +class EditorCapabilityDetector @Inject constructor( + private val editorSettingsRepository: EditorSettingsRepository, + private val networkUtilsWrapper: NetworkUtilsWrapper, + @Named(APPLICATION_SCOPE) private val appScope: CoroutineScope, +) { + private val states = + ConcurrentHashMap>() + private val jobs = ConcurrentHashMap() + + // Sites whose live probe succeeded this process — the dedup gate. Only a + // successful fetch latches; a failed one is left to retry on the next + // access, matching the connectivity banner's previous per-slice behaviour. + // Reset by refresh / clear. + private val probedOk = ConcurrentHashMap.newKeySet() + + /** + * The shared detection state for [site]. The first call starts detection; + * later calls return the same flow without re-probing once it has + * succeeded. Collect it to react to capability changes. + */ + @Synchronized + fun stateFor(site: SiteModel): StateFlow { + val flow = flowFor(site.id) + if (shouldProbe(site.id)) launchDetection(site) + return flow + } + + /** + * Ensures detection has run for [site] (so its capabilities are persisted) + * and returns the settled state. Respects the once-per-site gate; call + * [refresh] first to force a fresh probe. + */ + suspend fun awaitProbe(site: SiteModel): EditorCapabilityDetectionState { + stateFor(site) + jobs[site.id]?.join() + return states[site.id]?.value ?: EditorCapabilityDetectionState.Pending + } + + /** + * Forces a re-probe for [site], bypassing the once-per-site gate. A no-op + * while a probe is already in flight — that probe's result is fresh enough. + */ + @Synchronized + fun refresh(site: SiteModel) { + if (jobs[site.id]?.isActive == true) return + probedOk.remove(site.id) + launchDetection(site) + } + + /** Cancels all in-flight detection and drops all cached state (sign-out). */ + @Synchronized + fun clear() { + jobs.values.forEach { it.cancel() } + jobs.clear() + states.clear() + probedOk.clear() + } + + @Synchronized + private fun launchDetection(site: SiteModel) { + jobs[site.id]?.cancel() + val flow = flowFor(site.id) + jobs[site.id] = appScope.launch { + flow.value = detect(site) + } + } + + private fun flowFor(siteLocalId: Int): MutableStateFlow = + states.getOrPut(siteLocalId) { + MutableStateFlow(EditorCapabilityDetectionState.Pending) + } + + private fun shouldProbe(siteLocalId: Int): Boolean = + jobs[siteLocalId]?.isActive != true && siteLocalId !in probedOk + + private suspend fun detect(site: SiteModel): EditorCapabilityDetectionState { + val ok = editorSettingsRepository.fetchEditorCapabilitiesForSite(site) + if (ok) probedOk.add(site.id) + val hasCache = editorSettingsRepository.hasCachedCapabilities(site) + return when { + ok || hasCache -> EditorCapabilityDetectionState.Ready + editorSettingsRepository.isAwaitingApplicationPassword(site) -> + EditorCapabilityDetectionState.Pending + !networkUtilsWrapper.isNetworkAvailable() -> + EditorCapabilityDetectionState.TransientError + else -> EditorCapabilityDetectionState.Unreachable + } + } +} + +/** + * Observable lifecycle of editor-capability detection for one site — distinct + * from `org.wordpress.android.ui.posts.EditorCapabilityState`, which models a + * resolved settings-row capability. This is the *detection* state the + * connectivity banner and editor preloader subscribe to. + */ +sealed interface EditorCapabilityDetectionState { + /** + * Not determined yet — still probing, or waiting on an application password + * being minted asynchronously. Consumers hold; the banner stays hidden. + */ + data object Pending : EditorCapabilityDetectionState + + /** + * Capabilities are known (freshly detected, or cached from a prior run). + * Read them via [EditorSettingsRepository]'s getters. + */ + data object Ready : EditorCapabilityDetectionState + + /** + * Credentials are present but the transport probe failed — the site looks + * unreachable. The only state that surfaces the connectivity banner. + */ + data object Unreachable : EditorCapabilityDetectionState + + /** A transient failure (e.g. device offline). Retried on the next probe. */ + data object TransientError : EditorCapabilityDetectionState +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt index 3ae1fdaa37c0..83cf7b1bc981 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt @@ -9,14 +9,16 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import org.wordpress.android.R import androidx.annotation.VisibleForTesting +import org.wordpress.android.fluxc.Dispatcher +import org.wordpress.android.fluxc.generated.SiteActionBuilder import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.network.discovery.SelfHostedEndpointFinder import org.wordpress.android.fluxc.network.xmlrpc.site.SiteXMLRPCClient import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider import org.wordpress.android.fluxc.store.SiteStore import org.wordpress.android.fluxc.utils.AppLogWrapper +import org.wordpress.android.repositories.EditorCapabilityDetector import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper -import org.wordpress.android.ui.accounts.login.CredentialsChangedNotifier import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer import org.wordpress.android.ui.mysite.MySiteCardAndItem import org.wordpress.android.ui.mysite.MySiteCardAndItem.Card.QuickLinksItem.QuickLinkItem @@ -39,7 +41,8 @@ class ApplicationPasswordViewModelSlice @Inject constructor( private val selfHostedEndpointFinder: SelfHostedEndpointFinder, private val siteXMLRPCClient: SiteXMLRPCClient, private val siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer, - private val credentialsChangedNotifier: CredentialsChangedNotifier, + private val dispatcher: Dispatcher, + private val editorCapabilityDetector: EditorCapabilityDetector, @Named(IO_THREAD) private val ioDispatcher: CoroutineDispatcher, ) { lateinit var scope: CoroutineScope @@ -111,7 +114,11 @@ class ApplicationPasswordViewModelSlice @Inject constructor( if (!createResult.isError && createResult.credentials != null) { wpApiClientProvider.clearSelfHostedClient(storedSite.id) appLogWrapper.d(AppLog.T.MAIN, "A_P: Headless mint succeeded for ${storedSite.url}") - credentialsChangedNotifier.notifyChanged(storedSite.id) + // The first-login capability probe can lose the race to this async mint. storedSite + // was just mutated in place with the new credentials (SiteStore + // .persistApplicationPasswordCredentials), so re-probe against this exact instance — + // no stale-SiteModel re-read, and capabilities settle without a manual pull-to-refresh. + editorCapabilityDetector.refresh(storedSite) // The mint goes through the Jetpack tunnel and never runs discovery — without this // step, freshly minted Atomic sites end up with working creds but a NULL // wpApiRestUrl in the local DB. Run in the background so the card hides immediately. @@ -254,10 +261,9 @@ class ApplicationPasswordViewModelSlice @Inject constructor( } site.xmlRpcUrl = xmlRpcEndpoint - // Persist only the rediscovered column — mirrors healApiRestUrlIfMissing. A full-row - // updateSite would rewrite ~80 columns from this in-memory model for a one-field change - // (risking clobbering other out-of-band values), so write just xmlRpcUrl. - siteStore.persistXmlRpcUrl(site.id, xmlRpcEndpoint) + dispatcher.dispatch( + SiteActionBuilder.newUpdateSiteAction(site) + ) buildCard(site) } catch ( @Suppress("SwallowedException") diff --git a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSlice.kt b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSlice.kt index 5a93cd7df624..3123e1c5e9d9 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSlice.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSlice.kt @@ -7,83 +7,51 @@ import kotlinx.coroutines.Job import kotlinx.coroutines.launch import org.wordpress.android.R import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.repositories.EditorSettingsRepository -import org.wordpress.android.ui.accounts.login.CredentialsChangedNotifier +import org.wordpress.android.repositories.EditorCapabilityDetectionState +import org.wordpress.android.repositories.EditorCapabilityDetector import org.wordpress.android.ui.mysite.MySiteCardAndItem -import org.wordpress.android.ui.mysite.SelectedSiteRepository -import org.wordpress.android.util.NetworkUtilsWrapper import javax.inject.Inject class SiteConnectivityBannerViewModelSlice @Inject constructor( - private val editorSettingsRepository: EditorSettingsRepository, - private val networkUtilsWrapper: NetworkUtilsWrapper, - private val credentialsChangedNotifier: CredentialsChangedNotifier, - private val selectedSiteRepository: SelectedSiteRepository, + private val editorCapabilityDetector: EditorCapabilityDetector, ) { private lateinit var scope: CoroutineScope - private var currentJob: Job? = null + private var collectJob: Job? = null private var currentSite: SiteModel? = null private val _uiModel = MutableLiveData() val uiModel: LiveData = _uiModel - /* Site capabilities rarely change, so once we've successfully fetched them for a site we - skip subsequent non-user-initiated fetches in this slice's lifetime. Failed fetches do - not populate this set, so a transient network failure recovers on the next onResume. - User-initiated calls (PTR, banner retry) always bypass this gate. */ - private val fetchedCapabilitiesForSite = mutableSetOf() - fun initialize(scope: CoroutineScope) { this.scope = scope - // Re-run detection the moment an application password is established for the selected site - // (e.g. the headless mint finished after our first fetch lost the race), instead of waiting - // for the next resume/refresh. Re-read the selected site so we see the just-persisted - // credentials; isUserInitiated = false so a replayed event is a no-op once cached. - scope.launch { - credentialsChangedNotifier.events.collect { siteLocalId -> - val site = selectedSiteRepository.getSelectedSite() - if (site != null && site.id == siteLocalId) { - fetchCapabilities(site, isUserInitiated = false) - } - } - } } + /** + * Subscribes the banner to [site]'s editor-capability detection state. The + * banner is a thin view over that state — it surfaces only when detection + * reports the site [Unreachable][EditorCapabilityDetectionState.Unreachable]. + * Every other state (probing, pending credentials, offline, ready) leaves it + * hidden, so the dedup, offline-suppression, and pending-credential handling + * that used to live here now belong to the one detector. [isUserInitiated] + * (pull-to-refresh, banner retry) forces a fresh probe. + */ fun fetchCapabilities(site: SiteModel, isUserInitiated: Boolean) { - currentJob?.cancel() + collectJob?.cancel() currentSite = site - currentJob = scope.launch { - if (site.id in fetchedCapabilitiesForSite && !isUserInitiated) { - return@launch - } - val ok = editorSettingsRepository.fetchEditorCapabilitiesForSite(site) - if (ok) { - fetchedCapabilitiesForSite.add(site.id) + if (isUserInitiated) editorCapabilityDetector.refresh(site) + collectJob = scope.launch { + editorCapabilityDetector.stateFor(site).collect { state -> + // Bail if the user switched sites while suspended — postValue is + // not a suspension point, so cancellation alone won't catch this. + if (currentSite?.id != site.id) return@collect + val showBanner = state is EditorCapabilityDetectionState.Unreachable + _uiModel.postValue(if (showBanner) buildBanner() else null) } - val hasCache = editorSettingsRepository.hasCachedCapabilities(site) - // Bail if the user switched sites while we were suspended — postValue - // isn't a suspension point, so cancellation alone won't catch this. - if (currentSite?.id != site.id) return@launch - // Suppress the banner when the device is offline — the global "no - // connection" banner already covers this case, and stacking warnings - // for the same root cause is just noise. - val suppressForOffline = !ok && !networkUtilsWrapper.isNetworkAvailable() - // Atomic sites probe the direct host with an application password that's minted - // asynchronously on this same screen, so a first-login fetch can fail purely because - // the credential isn't ready yet. Treat that as pending, not a connection failure — - // the application-password card owns that state and a later fetch will succeed. - val suppressForPendingAuth = - !ok && editorSettingsRepository.isAwaitingApplicationPassword(site) - // Show the banner only as a last resort — not when detection succeeded, when we have - // cached capabilities, or while a transient non-error state (offline / pending creds) - // already explains the failure. - val suppressBanner = ok || hasCache || suppressForOffline || suppressForPendingAuth - _uiModel.postValue(if (suppressBanner) null else buildBanner()) } } fun clearBanner() { - currentJob?.cancel() + collectJob?.cancel() currentSite = null _uiModel.postValue(null) } @@ -93,10 +61,7 @@ class SiteConnectivityBannerViewModelSlice @Inject constructor( textResource = R.string.site_connectivity_banner_text, imageResource = R.drawable.ic_cloud_off_themed_24dp, onActionClick = { - val site = currentSite - if (site != null && currentJob?.isActive != true) { - fetchCapabilities(site, isUserInitiated = true) - } + currentSite?.let { fetchCapabilities(it, isUserInitiated = true) } }, showLearnMore = false, centerImageVertically = true, diff --git a/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergEditorPreloader.kt b/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergEditorPreloader.kt index 5a9a116b8769..27f3b8632ec5 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergEditorPreloader.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergEditorPreloader.kt @@ -11,7 +11,7 @@ import org.wordpress.android.datasets.SiteSettingsProvider import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.store.AccountStore import org.wordpress.android.modules.BG_THREAD -import org.wordpress.android.repositories.EditorSettingsRepository +import org.wordpress.android.repositories.EditorCapabilityDetector import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer import org.wordpress.android.util.AppLog import org.wordpress.gutenberg.model.EditorDependencies @@ -63,7 +63,7 @@ class GutenbergEditorPreloader @Inject constructor( private val gutenbergKitSettingsBuilder: GutenbergKitSettingsBuilder, private val siteSettingsProvider: SiteSettingsProvider, private val editorServiceProvider: EditorServiceProvider, - private val editorSettingsRepository: EditorSettingsRepository, + private val editorCapabilityDetector: EditorCapabilityDetector, private val siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer, @Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher ) { @@ -99,8 +99,11 @@ class GutenbergEditorPreloader @Inject constructor( siteApiRestUrlRecoverer.discoverApiRootUrl(site.url) ?.let { site.wpApiRestUrl = it } } - editorSettingsRepository - .fetchEditorCapabilitiesForSite(site) + // Detect (and persist) editor capabilities via the shared + // detector so the preloader and connectivity banner can't + // double-probe. We only need the probe to have run before + // building config, so the settled state itself is ignored. + editorCapabilityDetector.awaitProbe(site) // Preloading produces EditorDependencies, which the editor // consumes alongside its own per-launch EditorConfiguration. // Cookies and network-logging are per-launch concerns the @@ -146,6 +149,9 @@ class GutenbergEditorPreloader @Inject constructor( @MainThread fun refreshPreloading(site: SiteModel, scope: CoroutineScope) { clearSite(site) + // Pull-to-refresh: force a fresh capability probe so the awaitProbe in + // preloadIfNeeded re-detects instead of returning the cached result. + editorCapabilityDetector.refresh(site) preloadIfNeeded(site, scope) } diff --git a/WordPress/src/test/java/org/wordpress/android/repositories/EditorCapabilityDetectorTest.kt b/WordPress/src/test/java/org/wordpress/android/repositories/EditorCapabilityDetectorTest.kt new file mode 100644 index 000000000000..ea597c101dd3 --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/repositories/EditorCapabilityDetectorTest.kt @@ -0,0 +1,207 @@ +package org.wordpress.android.repositories + +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.advanceUntilIdle +import org.assertj.core.api.Assertions.assertThat +import org.junit.Before +import org.junit.Test +import org.mockito.Mock +import org.mockito.kotlin.never +import org.mockito.kotlin.times +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever +import org.wordpress.android.BaseUnitTest +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.util.NetworkUtilsWrapper + +private const val TEST_SITE_LOCAL_ID = 7 + +@ExperimentalCoroutinesApi +class EditorCapabilityDetectorTest : BaseUnitTest(StandardTestDispatcher()) { + @Mock + lateinit var editorSettingsRepository: EditorSettingsRepository + + @Mock + lateinit var networkUtilsWrapper: NetworkUtilsWrapper + + private lateinit var site: SiteModel + private lateinit var detector: EditorCapabilityDetector + + @Before + fun setUp() { + site = SiteModel().apply { id = TEST_SITE_LOCAL_ID } + detector = EditorCapabilityDetector( + editorSettingsRepository, + networkUtilsWrapper, + testScope(), + ) + } + + // region state mapping + + @Test + fun `given probe succeeds, then state is Ready`() = test { + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(true) + + val flow = detector.stateFor(site) + advanceUntilIdle() + + assertThat(flow.value).isEqualTo(EditorCapabilityDetectionState.Ready) + } + + @Test + fun `given probe fails but capabilities are cached, then state is Ready`() = test { + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(false) + whenever(editorSettingsRepository.hasCachedCapabilities(site)).thenReturn(true) + + val flow = detector.stateFor(site) + advanceUntilIdle() + + assertThat(flow.value).isEqualTo(EditorCapabilityDetectionState.Ready) + } + + @Test + fun `given probe fails while awaiting an application password, then state is Pending`() = test { + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(false) + whenever(editorSettingsRepository.hasCachedCapabilities(site)).thenReturn(false) + whenever(editorSettingsRepository.isAwaitingApplicationPassword(site)).thenReturn(true) + + val flow = detector.stateFor(site) + advanceUntilIdle() + + assertThat(flow.value).isEqualTo(EditorCapabilityDetectionState.Pending) + } + + @Test + fun `given probe fails while offline, then state is TransientError`() = test { + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(false) + whenever(editorSettingsRepository.hasCachedCapabilities(site)).thenReturn(false) + whenever(editorSettingsRepository.isAwaitingApplicationPassword(site)).thenReturn(false) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(false) + + val flow = detector.stateFor(site) + advanceUntilIdle() + + assertThat(flow.value).isEqualTo(EditorCapabilityDetectionState.TransientError) + } + + @Test + fun `given probe fails online with no pending auth, then state is Unreachable`() = test { + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(false) + whenever(editorSettingsRepository.hasCachedCapabilities(site)).thenReturn(false) + whenever(editorSettingsRepository.isAwaitingApplicationPassword(site)).thenReturn(false) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) + + val flow = detector.stateFor(site) + advanceUntilIdle() + + assertThat(flow.value).isEqualTo(EditorCapabilityDetectionState.Unreachable) + } + + // endregion + + // region deduplication + + @Test + fun `given a prior successful probe, when stateFor is called again, then it does not re-probe`() = test { + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(true) + + detector.stateFor(site) + advanceUntilIdle() + detector.stateFor(site) + advanceUntilIdle() + + verify(editorSettingsRepository, times(1)).fetchEditorCapabilitiesForSite(site) + } + + @Test + fun `given a prior failed probe, when stateFor is called again, then it re-probes`() = test { + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(false, true) + whenever(editorSettingsRepository.hasCachedCapabilities(site)).thenReturn(false) + whenever(editorSettingsRepository.isAwaitingApplicationPassword(site)).thenReturn(false) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) + + val flow = detector.stateFor(site) + advanceUntilIdle() + assertThat(flow.value).isEqualTo(EditorCapabilityDetectionState.Unreachable) + detector.stateFor(site) + advanceUntilIdle() + + verify(editorSettingsRepository, times(2)).fetchEditorCapabilitiesForSite(site) + assertThat(flow.value).isEqualTo(EditorCapabilityDetectionState.Ready) + } + + // endregion + + // region refresh + + @Test + fun `given a prior successful probe, when refresh is called, then it re-probes`() = test { + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(true) + + detector.stateFor(site) + advanceUntilIdle() + detector.refresh(site) + advanceUntilIdle() + + verify(editorSettingsRepository, times(2)).fetchEditorCapabilitiesForSite(site) + } + + @Test + fun `given a probe in flight, when refresh is called, then it does not start a second probe`() = test { + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(true) + + detector.stateFor(site) // StandardTestDispatcher: job is launched but not yet run + detector.refresh(site) // a probe is already in flight — must be a no-op + advanceUntilIdle() + + verify(editorSettingsRepository, times(1)).fetchEditorCapabilitiesForSite(site) + } + + // endregion + + // region awaitProbe + + @Test + fun `given awaitProbe, then it runs detection and returns the settled state`() = test { + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(true) + + val state = detector.awaitProbe(site) + + assertThat(state).isEqualTo(EditorCapabilityDetectionState.Ready) + verify(editorSettingsRepository).fetchEditorCapabilitiesForSite(site) + } + + @Test + fun `given a prior successful probe, when awaitProbe is called again, then it does not re-probe`() = test { + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(true) + + detector.awaitProbe(site) + detector.awaitProbe(site) + + verify(editorSettingsRepository, times(1)).fetchEditorCapabilitiesForSite(site) + } + + // endregion + + // region clear + + @Test + fun `given a probed site, when clear is called, then the next probe runs again`() = test { + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(true) + + detector.awaitProbe(site) + detector.clear() + detector.awaitProbe(site) + + verify(editorSettingsRepository, times(2)).fetchEditorCapabilitiesForSite(site) + } + + @Test + fun `given no interaction, then no probe runs`() = test { + verify(editorSettingsRepository, never()).fetchEditorCapabilitiesForSite(site) + } + + // endregion +} diff --git a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt index 6276f50d4a58..b19afec7af00 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt @@ -19,6 +19,7 @@ import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest import org.mockito.kotlin.mock +import org.wordpress.android.fluxc.Dispatcher import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.model.SitesModel import org.wordpress.android.fluxc.network.BaseRequest.BaseNetworkError @@ -30,8 +31,8 @@ import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider import org.wordpress.android.fluxc.store.SiteStore import org.wordpress.android.fluxc.store.SiteStore.OnApplicationPasswordCreated import org.wordpress.android.fluxc.utils.AppLogWrapper +import org.wordpress.android.repositories.EditorCapabilityDetector import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper -import org.wordpress.android.ui.accounts.login.CredentialsChangedNotifier import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer import org.wordpress.android.ui.mysite.MySiteCardAndItem import kotlin.test.assertNotNull @@ -71,7 +72,10 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { lateinit var siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer @Mock - lateinit var credentialsChangedNotifier: CredentialsChangedNotifier + lateinit var dispatcher: Dispatcher + + @Mock + lateinit var editorCapabilityDetector: EditorCapabilityDetector private lateinit var siteTest: SiteModel @@ -92,7 +96,8 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { selfHostedEndpointFinder, siteXMLRPCClient, siteApiRestUrlRecoverer, - credentialsChangedNotifier, + dispatcher, + editorCapabilityDetector, testDispatcher() ).apply { initialize(testScope()) @@ -177,12 +182,14 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { } @Test - fun `given headless mint succeeds, then notify credentials changed`() = runTest { + fun `given headless mint succeeds, then re-probe editor capabilities for the minted site`() = runTest { stubMintSuccess() applicationPasswordViewModelSlice.buildCard(siteTest) - verify(credentialsChangedNotifier).notifyChanged(TEST_SITE_ID) + // The just-minted credentials live on this exact SiteModel instance, so the detector + // re-probes against it — no stale-SiteModel re-read, capabilities settle without a refresh. + verify(editorCapabilityDetector).refresh(siteTest) } @Test @@ -243,7 +250,6 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { assertNotNull(applicationPasswordCard) verify(siteStore).createApplicationPassword(any()) verify(applicationPasswordLoginHelper).getAuthorizationUrlComplete(eq(TEST_URL)) - verify(credentialsChangedNotifier, never()).notifyChanged(any()) } @Test @@ -375,10 +381,8 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { } @Test - fun `given xmlRpc rediscovery and auth check succeed, then persist the discovered xmlRpcUrl`() = + fun `given xmlRpc rediscovery and auth check succeed, then update site and dispatch`() = runTest { - // @Before seeds siteTest.xmlRpcUrl; clear it so the final assertion proves rediscovery set it. - siteTest.xmlRpcUrl = null val xmlRpcUrl = "https://www.test.com/xmlrpc.php" whenever( selfHostedEndpointFinder @@ -389,17 +393,16 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { eq(xmlRpcUrl), any(), any() ) ).thenReturn(SitesModel(listOf(SiteModel()))) - whenever(siteStore.persistXmlRpcUrl(any(), any())).thenReturn(SiteStore.OnSiteChanged(0)) applicationPasswordViewModelSlice .attemptXmlRpcRediscovery(siteTest) - verify(siteStore).persistXmlRpcUrl(siteTest.id, xmlRpcUrl) + verify(dispatcher).dispatch(any()) assert(siteTest.xmlRpcUrl == xmlRpcUrl) } @Test - fun `given xmlRpc rediscovery succeeds but auth check fails, then do not persist`() = + fun `given xmlRpc rediscovery succeeds but auth check fails, then do not dispatch`() = runTest { siteTest.xmlRpcUrl = null val xmlRpcUrl = "https://www.test.com/xmlrpc.php" @@ -419,12 +422,12 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { applicationPasswordViewModelSlice .attemptXmlRpcRediscovery(siteTest) - verify(siteStore, never()).persistXmlRpcUrl(any(), any()) + verify(dispatcher, never()).dispatch(any()) assert(siteTest.xmlRpcUrl.isNullOrEmpty()) } @Test - fun `given xmlRpc rediscovery fails, then do not persist`() = + fun `given xmlRpc rediscovery fails, then do not dispatch`() = runTest { siteTest.xmlRpcUrl = null whenever( @@ -439,7 +442,7 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { verify(selfHostedEndpointFinder) .verifyOrDiscoverXMLRPCEndpoint(TEST_URL) - verify(siteStore, never()).persistXmlRpcUrl(any(), any()) + verify(dispatcher, never()).dispatch(any()) assert(siteTest.xmlRpcUrl.isNullOrEmpty()) } } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSliceTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSliceTest.kt index 59ed881f498e..280d8135a4da 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSliceTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSliceTest.kt @@ -1,29 +1,24 @@ package org.wordpress.android.ui.mysite.cards.connectivity -import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.test.advanceUntilIdle import org.assertj.core.api.Assertions.assertThat import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mock -import org.mockito.Mockito.lenient import org.mockito.junit.MockitoJUnitRunner -import org.mockito.kotlin.doSuspendableAnswer -import org.mockito.kotlin.eq -import org.mockito.kotlin.times +import org.mockito.kotlin.any +import org.mockito.kotlin.never import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest import org.wordpress.android.R import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.repositories.EditorSettingsRepository -import org.wordpress.android.ui.accounts.login.CredentialsChangedNotifier +import org.wordpress.android.repositories.EditorCapabilityDetectionState +import org.wordpress.android.repositories.EditorCapabilityDetector import org.wordpress.android.ui.mysite.MySiteCardAndItem -import org.wordpress.android.ui.mysite.SelectedSiteRepository -import org.wordpress.android.util.NetworkUtilsWrapper private const val TEST_SITE_LOCAL_ID = 42 @@ -31,18 +26,7 @@ private const val TEST_SITE_LOCAL_ID = 42 @RunWith(MockitoJUnitRunner::class) class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { @Mock - lateinit var editorSettingsRepository: EditorSettingsRepository - - @Mock - lateinit var networkUtilsWrapper: NetworkUtilsWrapper - - @Mock - lateinit var credentialsChangedNotifier: CredentialsChangedNotifier - - @Mock - lateinit var selectedSiteRepository: SelectedSiteRepository - - private val credentialsChangedFlow = MutableSharedFlow(extraBufferCapacity = 1) + lateinit var editorCapabilityDetector: EditorCapabilityDetector private lateinit var siteTest: SiteModel private lateinit var slice: SiteConnectivityBannerViewModelSlice @@ -51,34 +35,23 @@ class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { @Before fun setUp() { siteTest = SiteModel().apply { id = TEST_SITE_LOCAL_ID } - // Default network state is available; tests that need offline override per-test. Lenient - // because tests where the fetch succeeds never reach the network check. - lenient().`when`(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) - whenever(credentialsChangedNotifier.events).thenReturn(credentialsChangedFlow) - slice = SiteConnectivityBannerViewModelSlice( - editorSettingsRepository, - networkUtilsWrapper, - credentialsChangedNotifier, - selectedSiteRepository, - ) + slice = SiteConnectivityBannerViewModelSlice(editorCapabilityDetector) slice.initialize(testScope()) slice.uiModel.observeForever { emittedBanners.add(it) } } - @Test - fun `given fetch succeeds, when fetchCapabilities invoked, then banner is null`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(true) - - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - - assertThat(emittedBanners.last()).isNull() + private fun stubState( + site: SiteModel, + state: EditorCapabilityDetectionState, + ): MutableStateFlow { + val flow = MutableStateFlow(state) + whenever(editorCapabilityDetector.stateFor(site)).thenReturn(flow) + return flow } @Test - fun `given fetch fails with no cache, when fetchCapabilities invoked, then banner is shown`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) + fun `given detection unreachable, when fetchCapabilities invoked, then banner is shown`() = test { + stubState(siteTest, EditorCapabilityDetectionState.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() @@ -89,181 +62,101 @@ class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { } @Test - fun `given fetch fails with no cache but device offline, when fetchCapabilities invoked, then banner is null`() = - test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) - whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(false) - - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - - // Global offline indicator covers this case — suppress to avoid stacked warnings. - assertThat(emittedBanners.last()).isNull() - } - - @Test - fun `given fetch fails but app password pending, when fetchCapabilities invoked, then banner is null`() = - test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.isAwaitingApplicationPassword(siteTest)).thenReturn(true) - - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - - // Credentials are still being minted — pending, not a connection failure. - assertThat(emittedBanners.last()).isNull() - } - - @Test - fun `when credentials change for the selected site, then capabilities are re-fetched`() = test { - whenever(selectedSiteRepository.getSelectedSite()).thenReturn(siteTest) - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(true) + fun `given detection ready, when fetchCapabilities invoked, then banner is null`() = test { + stubState(siteTest, EditorCapabilityDetectionState.Ready) - credentialsChangedFlow.emit(TEST_SITE_LOCAL_ID) + slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() - verify(editorSettingsRepository).fetchEditorCapabilitiesForSite(siteTest) + assertThat(emittedBanners.last()).isNull() } @Test - fun `given fetch fails but cache exists, when fetchCapabilities invoked, then banner is null`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(true) + fun `given detection pending, when fetchCapabilities invoked, then banner is null`() = test { + stubState(siteTest, EditorCapabilityDetectionState.Pending) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() + // Pending = probing or awaiting credentials — never a false "can't connect". assertThat(emittedBanners.last()).isNull() } @Test - fun `given prior successful fetch, when fetchCapabilities invoked again non-user-initiated, then fetch skipped`() = - test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(true) - - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - - verify(editorSettingsRepository, times(1)).fetchEditorCapabilitiesForSite(siteTest) - } - - @Test - fun `given prior failed fetch, when fetchCapabilities invoked again non-user-initiated, then fetch retries`() = - test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false, true) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) - - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - assertThat(emittedBanners.last()).isNotNull - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - - verify(editorSettingsRepository, times(2)).fetchEditorCapabilitiesForSite(siteTest) - assertThat(emittedBanners.last()).isNull() - } - - @Test - fun `given prior successful fetch, when user-initiated fetchCapabilities invoked, then fetch runs again`() = - test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(true) + fun `given detection transient error, when fetchCapabilities invoked, then banner is null`() = test { + stubState(siteTest, EditorCapabilityDetectionState.TransientError) - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - slice.fetchCapabilities(siteTest, isUserInitiated = true) - advanceUntilIdle() + slice.fetchCapabilities(siteTest, isUserInitiated = false) + advanceUntilIdle() - verify(editorSettingsRepository, times(2)).fetchEditorCapabilitiesForSite(siteTest) - } + // Transient (e.g. offline) is covered by the global indicator — don't stack a warning. + assertThat(emittedBanners.last()).isNull() + } @Test - fun `given banner showing, when retry tapped, then fetch runs and bypasses session dedup`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) + fun `given unreachable then recovered to ready, when state changes, then banner clears`() = test { + val flow = stubState(siteTest, EditorCapabilityDetectionState.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() - val banner = emittedBanners.last() as MySiteCardAndItem.Item.SingleActionCard + assertThat(emittedBanners.last()).isNotNull - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(true) - banner.onActionClick() + flow.value = EditorCapabilityDetectionState.Ready advanceUntilIdle() - verify(editorSettingsRepository, times(2)).fetchEditorCapabilitiesForSite(siteTest) assertThat(emittedBanners.last()).isNull() } @Test - fun `when clearBanner invoked, then banner is null`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) + fun `given user-initiated, when fetchCapabilities invoked, then detector is refreshed`() = test { + stubState(siteTest, EditorCapabilityDetectionState.Ready) - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - assertThat(emittedBanners.last()).isNotNull - slice.clearBanner() + slice.fetchCapabilities(siteTest, isUserInitiated = true) advanceUntilIdle() - assertThat(emittedBanners.last()).isNull() + verify(editorCapabilityDetector).refresh(siteTest) } @Test - fun `given two different sites, when fetched in sequence, then both fetches run`() = test { - val otherSite = SiteModel().apply { id = TEST_SITE_LOCAL_ID + 1 } - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(true) - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(otherSite)).thenReturn(true) + fun `given non-user-initiated, when fetchCapabilities invoked, then detector is not refreshed`() = test { + stubState(siteTest, EditorCapabilityDetectionState.Ready) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() - slice.fetchCapabilities(otherSite, isUserInitiated = false) - advanceUntilIdle() - verify(editorSettingsRepository, times(1)).fetchEditorCapabilitiesForSite(eq(siteTest)) - verify(editorSettingsRepository, times(1)).fetchEditorCapabilitiesForSite(eq(otherSite)) + verify(editorCapabilityDetector, never()).refresh(any()) } @Test - fun `given fetch in flight, when clearBanner invoked, then banner stays null after fetch completes`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) + fun `given banner showing, when retry tapped, then detector is refreshed`() = test { + stubState(siteTest, EditorCapabilityDetectionState.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) - slice.clearBanner() advanceUntilIdle() + val banner = emittedBanners.last() as MySiteCardAndItem.Item.SingleActionCard - assertThat(emittedBanners.last()).isNull() + banner.onActionClick() + advanceUntilIdle() + + verify(editorCapabilityDetector).refresh(siteTest) } @Test - fun `given retry in flight, when banner tapped again, then second tap is a no-op`() = test { - val gate = CompletableDeferred() - var callCount = 0 - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).doSuspendableAnswer { - callCount++ - if (callCount == 1) false else gate.await() - } - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) + fun `when clearBanner invoked, then banner is null`() = test { + stubState(siteTest, EditorCapabilityDetectionState.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() - val banner = emittedBanners.last() as MySiteCardAndItem.Item.SingleActionCard - - banner.onActionClick() // first tap — retry suspends on gate - banner.onActionClick() // second tap — should be ignored - gate.complete(true) + assertThat(emittedBanners.last()).isNotNull + slice.clearBanner() advanceUntilIdle() - verify(editorSettingsRepository, times(2)).fetchEditorCapabilitiesForSite(siteTest) + assertThat(emittedBanners.last()).isNull() } @Test - fun `given banner cleared, when retry tapped, then no fetch runs`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) + fun `given banner cleared, when retry tapped, then no refresh runs`() = test { + stubState(siteTest, EditorCapabilityDetectionState.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() @@ -275,27 +168,26 @@ class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { banner.onActionClick() advanceUntilIdle() - verify(editorSettingsRepository, times(1)).fetchEditorCapabilitiesForSite(siteTest) + verify(editorCapabilityDetector, never()).refresh(any()) } @Test - fun `given fetch in flight for site A, when fetch starts for site B, then site A result is discarded`() = test { + fun `given site switched, when old site becomes unreachable, then banner ignores it`() = test { val siteB = SiteModel().apply { id = TEST_SITE_LOCAL_ID + 1 } - val gateA = CompletableDeferred() - // Site A's fetch suspends on a gate so we can interleave site B's call before A completes. - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).doSuspendableAnswer { - gateA.await() - } - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteB)).thenReturn(true) + val flowA = stubState(siteTest, EditorCapabilityDetectionState.Ready) + stubState(siteB, EditorCapabilityDetectionState.Ready) slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() // siteA suspended in fetch + advanceUntilIdle() slice.fetchCapabilities(siteB, isUserInitiated = false) - advanceUntilIdle() // siteB completes; currentSite is now siteB - gateA.complete(false) // release siteA — its result must NOT post a banner advanceUntilIdle() - // No banner card should ever have been emitted for site A. - assertThat(emittedBanners.filterIsInstance()).isEmpty() + // Site A's probe resolves to Unreachable after we've switched to B — must not surface. + flowA.value = EditorCapabilityDetectionState.Unreachable + advanceUntilIdle() + + assertThat( + emittedBanners.filterIsInstance() + ).isEmpty() } } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt index b009b9f3cc2d..40a1895ef3ac 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt @@ -19,7 +19,7 @@ import org.wordpress.android.BaseUnitTest import org.wordpress.android.datasets.SiteSettingsProvider import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.store.AccountStore -import org.wordpress.android.repositories.EditorSettingsRepository +import org.wordpress.android.repositories.EditorCapabilityDetector import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer import org.wordpress.gutenberg.model.EditorAssetBundle import org.wordpress.gutenberg.model.EditorConfiguration @@ -49,7 +49,7 @@ class GutenbergEditorPreloaderTest : lateinit var editorServiceProvider: EditorServiceProvider @Mock - lateinit var editorSettingsRepository: EditorSettingsRepository + lateinit var editorCapabilityDetector: EditorCapabilityDetector @Mock lateinit var siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer @@ -75,7 +75,7 @@ class GutenbergEditorPreloaderTest : gutenbergKitSettingsBuilder = gutenbergKitSettingsBuilder, siteSettingsProvider = siteSettingsProvider, editorServiceProvider = editorServiceProvider, - editorSettingsRepository = editorSettingsRepository, + editorCapabilityDetector = editorCapabilityDetector, siteApiRestUrlRecoverer = siteApiRestUrlRecoverer, bgDispatcher = testDispatcher() ) @@ -194,7 +194,7 @@ class GutenbergEditorPreloaderTest : } @Test - fun `successful preload fetches editor capabilities`() = test { + fun `successful preload detects editor capabilities via the detector`() = test { val site = createSite() enablePreloading(site) stubSuccessfulPreload() @@ -203,8 +203,7 @@ class GutenbergEditorPreloaderTest : preloader.preloadIfNeeded(site, this) advanceUntilIdle() - verify(editorSettingsRepository) - .fetchEditorCapabilitiesForSite(site) + verify(editorCapabilityDetector).awaitProbe(site) } @Test From 91059e151a7e53714a4ad89b96ce540b38f097a4 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Thu, 4 Jun 2026 11:51:40 -0600 Subject: [PATCH 02/12] Add release note for editor capability detection rework --- RELEASE-NOTES.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index f1ec4f3f18b2..ab4a56249f69 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -2,7 +2,7 @@ 26.9 ----- - +* [*] Reworked editor capability detection to be more reliable and prevent a false "Unable to connect to your site" banner on private Atomic sites. 26.8 ----- From 2907e6f3a13d30d64148debbb65b62a0238c6375 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Thu, 4 Jun 2026 12:20:12 -0600 Subject: [PATCH 03/12] Remove CredentialsChangedNotifier event bus, superseded by the detector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #22926's first-login hardening signalled credential establishment through a process-global CredentialsChangedNotifier that the banner collected against a re-read of getSelectedSite() — the staleness race called out in #22942. EditorCapabilityDetector.refresh(storedSite), called from the mint path on the exact mutated SiteModel, replaces that coordination, so drop the notifier and its wiring in ApplicationPasswordLoginHelper. --- .../login/ApplicationPasswordLoginHelper.kt | 2 -- .../login/CredentialsChangedNotifier.kt | 34 ------------------- .../ApplicationPasswordLoginHelperTest.kt | 7 +--- 3 files changed, 1 insertion(+), 42 deletions(-) delete mode 100644 WordPress/src/main/java/org/wordpress/android/ui/accounts/login/CredentialsChangedNotifier.kt 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 5a9c5e493269..7b5c6c01fe0d 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 @@ -46,7 +46,6 @@ class ApplicationPasswordLoginHelper @Inject constructor( private val discoverSuccessWrapper: DiscoverSuccessWrapper, private val crashLogging: CrashLogging, private val wpApiClientProvider: WpApiClientProvider, - private val credentialsChangedNotifier: CredentialsChangedNotifier, ) { private var processedAppPasswordData: String? = null @@ -149,7 +148,6 @@ class ApplicationPasswordLoginHelper @Inject constructor( } wpApiClientProvider.clearSelfHostedClient(site.id) dispatcherWrapper.updateApplicationPassword(site) - credentialsChangedNotifier.notifyChanged(site.id) trackSuccessful(effectiveUrlLogin.siteUrl) trackCreated(creationSource, success = true) processedAppPasswordData = effectiveUrlLogin.siteUrl diff --git a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/CredentialsChangedNotifier.kt b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/CredentialsChangedNotifier.kt deleted file mode 100644 index 0046dc400595..000000000000 --- a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/CredentialsChangedNotifier.kt +++ /dev/null @@ -1,34 +0,0 @@ -package org.wordpress.android.ui.accounts.login - -import kotlinx.coroutines.channels.BufferOverflow -import kotlinx.coroutines.flow.MutableSharedFlow -import kotlinx.coroutines.flow.SharedFlow -import kotlinx.coroutines.flow.asSharedFlow -import javax.inject.Inject -import javax.inject.Singleton - -/** - * App-scoped signal that an application password was newly established for a site — either by the - * headless Jetpack-tunnel mint on the My Site screen or by the interactive application-password - * login. Lets credential-dependent work (e.g. editor capability detection) re-run as soon as the - * password exists, instead of waiting for the next My Site resume/refresh. - * - * Emits the site's local id; collectors should re-read a fresh SiteModel so they observe the - * just-persisted credentials rather than a stale in-memory copy. - */ -@Singleton -class CredentialsChangedNotifier @Inject constructor() { - // replay = 1 so a collector that subscribes just after an emit still sees it — closes the - // emit-before-collect race. DROP_OLDEST keeps tryEmit non-suspending without an unbounded buffer. - private val _events = MutableSharedFlow( - replay = 1, - extraBufferCapacity = 1, - onBufferOverflow = BufferOverflow.DROP_OLDEST, - ) - val events: SharedFlow = _events.asSharedFlow() - - /** Signals that [siteLocalId]'s application-password credentials were just established. */ - fun notifyChanged(siteLocalId: Int) { - _events.tryEmit(siteLocalId) - } -} diff --git a/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelperTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelperTest.kt index 6c97ee23eb3b..b1df5c270090 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelperTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelperTest.kt @@ -76,9 +76,6 @@ class ApplicationPasswordLoginHelperTest : BaseUnitTest() { @Mock lateinit var wpApiClientProvider: WpApiClientProvider - @Mock - lateinit var credentialsChangedNotifier: CredentialsChangedNotifier - private lateinit var applicationPasswordLoginHelper: ApplicationPasswordLoginHelper @Before @@ -95,8 +92,7 @@ class ApplicationPasswordLoginHelperTest : BaseUnitTest() { apiRootUrlCache, discoverSuccessWrapper, crashLogging, - wpApiClientProvider, - credentialsChangedNotifier + wpApiClientProvider ) } @@ -210,7 +206,6 @@ class ApplicationPasswordLoginHelperTest : BaseUnitTest() { verify(siteStore).sites verify(dispatcherWrapper).updateApplicationPassword(eq(siteModel)) verify(wpApiClientProvider).clearSelfHostedClient(eq(siteModel.id)) - verify(credentialsChangedNotifier).notifyChanged(eq(siteModel.id)) } @Test From 2473ae9c57270cc6669c163351165b6cbd789665 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Thu, 4 Jun 2026 13:06:47 -0600 Subject: [PATCH 04/12] Fold provisioning + detection into one single-flight SiteProvisioningSource MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Promotes the EditorCapabilityDetector into SiteProvisioningSource: one per-site, single-flight pipeline that ensures application-password credentials, recovers the REST root, and detects editor capabilities — each stage awaited before the next. Because the capability probe is now structurally downstream of credential provisioning, it can never run before the mint, so the first-login race is gone by construction rather than mitigated. The application-password card, connectivity banner, and editor preloader all render slices of the one SiteReadiness state. The mint/validate mechanics move out of ApplicationPasswordViewModelSlice (now a renderer) into the source's ensureAuth stage; the per-site single-flight subsumes the card's old 409-safety guard. The duplicate wpApiRestUrl heal collapses into the source's recoverRestUrl stage. --- .../org/wordpress/android/AppInitializer.kt | 8 +- .../repositories/EditorCapabilityDetector.kt | 158 ------- .../repositories/SiteProvisioningSource.kt | 252 ++++++++++ .../ApplicationPasswordViewModelSlice.kt | 145 +++--- .../SiteConnectivityBannerViewModelSlice.kt | 25 +- .../ui/posts/GutenbergEditorPreloader.kt | 31 +- .../EditorCapabilityDetectorTest.kt | 207 --------- .../SiteProvisioningSourceTest.kt | 237 ++++++++++ .../ApplicationPasswordViewModelSliceTest.kt | 431 +++--------------- ...iteConnectivityBannerViewModelSliceTest.kt | 86 ++-- .../ui/posts/GutenbergEditorPreloaderTest.kt | 33 +- 11 files changed, 695 insertions(+), 918 deletions(-) delete mode 100644 WordPress/src/main/java/org/wordpress/android/repositories/EditorCapabilityDetector.kt create mode 100644 WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt delete mode 100644 WordPress/src/test/java/org/wordpress/android/repositories/EditorCapabilityDetectorTest.kt create mode 100644 WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt diff --git a/WordPress/src/main/java/org/wordpress/android/AppInitializer.kt b/WordPress/src/main/java/org/wordpress/android/AppInitializer.kt index 722d06809f77..2113fe519073 100644 --- a/WordPress/src/main/java/org/wordpress/android/AppInitializer.kt +++ b/WordPress/src/main/java/org/wordpress/android/AppInitializer.kt @@ -71,7 +71,7 @@ import org.wordpress.android.networking.ConnectionChangeReceiver import org.wordpress.android.networking.OAuthAuthenticator import org.wordpress.android.networking.RestClientUtils import org.wordpress.android.push.GCMRegistrationScheduler -import org.wordpress.android.repositories.EditorCapabilityDetector +import org.wordpress.android.repositories.SiteProvisioningSource import org.wordpress.android.support.ZendeskHelper import org.wordpress.android.ui.ActivityId import org.wordpress.android.ui.debug.cookies.DebugCookieManager @@ -231,7 +231,7 @@ class AppInitializer @Inject constructor( lateinit var wpApiClientProvider: WpApiClientProvider @Inject - lateinit var editorCapabilityDetector: EditorCapabilityDetector + lateinit var siteProvisioningSource: SiteProvisioningSource @Inject lateinit var openWebLinksWithJetpackHelper: DeepLinkOpenWebLinksWithJetpackHelper @@ -722,8 +722,8 @@ class AppInitializer @Inject constructor( wpServiceProvider.clearAll() wpApiClientProvider.clearAllClients() - // Drop per-site editor-capability detection state for the signed-out user - editorCapabilityDetector.clear() + // Drop per-site provisioning + capability state for the signed-out user + siteProvisioningSource.clear() } /* diff --git a/WordPress/src/main/java/org/wordpress/android/repositories/EditorCapabilityDetector.kt b/WordPress/src/main/java/org/wordpress/android/repositories/EditorCapabilityDetector.kt deleted file mode 100644 index a8f6484e4709..000000000000 --- a/WordPress/src/main/java/org/wordpress/android/repositories/EditorCapabilityDetector.kt +++ /dev/null @@ -1,158 +0,0 @@ -package org.wordpress.android.repositories - -import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Job -import kotlinx.coroutines.flow.MutableStateFlow -import kotlinx.coroutines.flow.StateFlow -import kotlinx.coroutines.launch -import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.modules.APPLICATION_SCOPE -import org.wordpress.android.util.NetworkUtilsWrapper -import java.util.concurrent.ConcurrentHashMap -import javax.inject.Inject -import javax.inject.Named -import javax.inject.Singleton - -/** - * Single owner of "what does this site's editor REST API support" as an - * observable, per-site state. - * - * Capability detection ([EditorSettingsRepository.fetchEditorCapabilitiesForSite]) - * has two async preconditions on Atomic sites — an application password and a - * recovered REST root — provisioned elsewhere on the My Site screen. Routing - * every consumer (connectivity banner, editor preloader) through this detector - * means one probe per site, shared and deduplicated, instead of each consumer - * re-deriving the same state and racing the same preconditions. - * - * State is keyed by [SiteModel.id] — the local DB row id, stable across the - * process lifetime — mirroring `GutenbergEditorPreloader`. - * - * ## Entry points - * - [stateFor] — the reactive entry point. Returns a shared [StateFlow]; the - * first access starts detection, later accesses reuse the cached result - * (capabilities rarely change). A failed probe is retried on the next access. - * - [awaitProbe] — the one-shot entry point for callers that just need the - * probe to have run (and its capabilities persisted) before continuing. - * - [refresh] — forces a re-probe, bypassing the once-per-site gate - * (pull-to-refresh, banner retry, newly established credentials). - * - [clear] — cancels all work and drops all state; wire into sign-out. - */ -@Singleton -class EditorCapabilityDetector @Inject constructor( - private val editorSettingsRepository: EditorSettingsRepository, - private val networkUtilsWrapper: NetworkUtilsWrapper, - @Named(APPLICATION_SCOPE) private val appScope: CoroutineScope, -) { - private val states = - ConcurrentHashMap>() - private val jobs = ConcurrentHashMap() - - // Sites whose live probe succeeded this process — the dedup gate. Only a - // successful fetch latches; a failed one is left to retry on the next - // access, matching the connectivity banner's previous per-slice behaviour. - // Reset by refresh / clear. - private val probedOk = ConcurrentHashMap.newKeySet() - - /** - * The shared detection state for [site]. The first call starts detection; - * later calls return the same flow without re-probing once it has - * succeeded. Collect it to react to capability changes. - */ - @Synchronized - fun stateFor(site: SiteModel): StateFlow { - val flow = flowFor(site.id) - if (shouldProbe(site.id)) launchDetection(site) - return flow - } - - /** - * Ensures detection has run for [site] (so its capabilities are persisted) - * and returns the settled state. Respects the once-per-site gate; call - * [refresh] first to force a fresh probe. - */ - suspend fun awaitProbe(site: SiteModel): EditorCapabilityDetectionState { - stateFor(site) - jobs[site.id]?.join() - return states[site.id]?.value ?: EditorCapabilityDetectionState.Pending - } - - /** - * Forces a re-probe for [site], bypassing the once-per-site gate. A no-op - * while a probe is already in flight — that probe's result is fresh enough. - */ - @Synchronized - fun refresh(site: SiteModel) { - if (jobs[site.id]?.isActive == true) return - probedOk.remove(site.id) - launchDetection(site) - } - - /** Cancels all in-flight detection and drops all cached state (sign-out). */ - @Synchronized - fun clear() { - jobs.values.forEach { it.cancel() } - jobs.clear() - states.clear() - probedOk.clear() - } - - @Synchronized - private fun launchDetection(site: SiteModel) { - jobs[site.id]?.cancel() - val flow = flowFor(site.id) - jobs[site.id] = appScope.launch { - flow.value = detect(site) - } - } - - private fun flowFor(siteLocalId: Int): MutableStateFlow = - states.getOrPut(siteLocalId) { - MutableStateFlow(EditorCapabilityDetectionState.Pending) - } - - private fun shouldProbe(siteLocalId: Int): Boolean = - jobs[siteLocalId]?.isActive != true && siteLocalId !in probedOk - - private suspend fun detect(site: SiteModel): EditorCapabilityDetectionState { - val ok = editorSettingsRepository.fetchEditorCapabilitiesForSite(site) - if (ok) probedOk.add(site.id) - val hasCache = editorSettingsRepository.hasCachedCapabilities(site) - return when { - ok || hasCache -> EditorCapabilityDetectionState.Ready - editorSettingsRepository.isAwaitingApplicationPassword(site) -> - EditorCapabilityDetectionState.Pending - !networkUtilsWrapper.isNetworkAvailable() -> - EditorCapabilityDetectionState.TransientError - else -> EditorCapabilityDetectionState.Unreachable - } - } -} - -/** - * Observable lifecycle of editor-capability detection for one site — distinct - * from `org.wordpress.android.ui.posts.EditorCapabilityState`, which models a - * resolved settings-row capability. This is the *detection* state the - * connectivity banner and editor preloader subscribe to. - */ -sealed interface EditorCapabilityDetectionState { - /** - * Not determined yet — still probing, or waiting on an application password - * being minted asynchronously. Consumers hold; the banner stays hidden. - */ - data object Pending : EditorCapabilityDetectionState - - /** - * Capabilities are known (freshly detected, or cached from a prior run). - * Read them via [EditorSettingsRepository]'s getters. - */ - data object Ready : EditorCapabilityDetectionState - - /** - * Credentials are present but the transport probe failed — the site looks - * unreachable. The only state that surfaces the connectivity banner. - */ - data object Unreachable : EditorCapabilityDetectionState - - /** A transient failure (e.g. device offline). Retried on the next probe. */ - data object TransientError : EditorCapabilityDetectionState -} diff --git a/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt new file mode 100644 index 000000000000..e5c8ad7792fd --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt @@ -0,0 +1,252 @@ +package org.wordpress.android.repositories + +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.launch +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider +import org.wordpress.android.fluxc.store.SiteStore +import org.wordpress.android.fluxc.utils.AppLogWrapper +import org.wordpress.android.modules.APPLICATION_SCOPE +import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper +import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer +import org.wordpress.android.ui.mysite.cards.applicationpassword.ApplicationPasswordValidator +import org.wordpress.android.util.AppLog +import org.wordpress.android.util.NetworkUtilsWrapper +import java.util.concurrent.ConcurrentHashMap +import javax.inject.Inject +import javax.inject.Named +import javax.inject.Singleton + +/** + * The single source of truth for getting a site ready to use: it provisions + * application-password credentials, recovers the REST API root, and detects + * editor capabilities — **in that order, each stage awaited before the next**. + * + * Those three things have a genuine ordering dependency (you can't probe the + * REST API of a private Atomic host until you've minted a credential for it), + * and they used to be spread across the connectivity banner, the editor + * preloader, and the application-password card, each triggering its slice of + * the work independently and racing the others. Running them as one serialized + * per-site pipeline makes the race structurally impossible: [detectCapabilities] + * is downstream of [ensureAuth], so the probe can never run before the mint. + * + * Per site there is at most one in-flight pipeline (single-flight, keyed by + * [SiteModel.id]); concurrent callers join it rather than starting a second — + * which also subsumes the application-password card's old single-flight guard + * (two concurrent mints hit a 409 that destroys the winner's credentials). + * + * ## Entry points + * - [stateFor] — reactive: returns a shared [StateFlow]; the first access runs + * the pipeline, later accesses reuse a [SiteReadiness.Ready] result. + * - [await] — one-shot: runs the pipeline (if needed) and returns the result. + * - [invalidate] — forces a re-run, bypassing the once-per-site gate + * (pull-to-refresh, retry). + * - [clear] — cancels all work and drops all state; wire into sign-out. + */ +@Singleton +@Suppress("LongParameterList") +class SiteProvisioningSource @Inject constructor( + private val siteStore: SiteStore, + private val applicationPasswordLoginHelper: ApplicationPasswordLoginHelper, + private val applicationPasswordValidator: ApplicationPasswordValidator, + private val wpApiClientProvider: WpApiClientProvider, + private val siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer, + private val editorSettingsRepository: EditorSettingsRepository, + private val networkUtilsWrapper: NetworkUtilsWrapper, + private val appLogWrapper: AppLogWrapper, + @Named(APPLICATION_SCOPE) private val appScope: CoroutineScope, +) { + private val states = ConcurrentHashMap>() + private val jobs = ConcurrentHashMap() + + // Sites whose pipeline reached Ready this process — the dedup gate. Only a fully-ready site + // latches; auth-needed / unreachable / transient outcomes are left to re-run on the next + // access (so a later resume re-mints or re-probes). Reset by invalidate / clear. + private val ready = ConcurrentHashMap.newKeySet() + + /** + * The shared readiness state for [site]. The first call starts the pipeline; + * later calls return the same flow without re-running once it reached + * [SiteReadiness.Ready]. Collect it to react to provisioning / capability changes. + */ + @Synchronized + fun stateFor(site: SiteModel): StateFlow { + val flow = flowFor(site.id) + if (shouldRun(site.id)) launchPipeline(site) + return flow + } + + /** + * Runs the pipeline for [site] (if it hasn't reached Ready) and returns the + * settled readiness. Respects the once-per-site gate; call [invalidate] first + * to force a fresh run. + */ + suspend fun await(site: SiteModel): SiteReadiness { + stateFor(site) + jobs[site.id]?.join() + return states[site.id]?.value ?: SiteReadiness.Probing + } + + /** + * Forces a re-run for [site], bypassing the once-per-site gate. A no-op while + * a run is already in flight — that run already reflects current state. + */ + @Synchronized + fun invalidate(site: SiteModel) { + if (jobs[site.id]?.isActive == true) return + ready.remove(site.id) + launchPipeline(site) + } + + /** Cancels all in-flight pipelines and drops all cached state (sign-out). */ + @Synchronized + fun clear() { + jobs.values.forEach { it.cancel() } + jobs.clear() + states.clear() + ready.clear() + } + + @Synchronized + private fun launchPipeline(site: SiteModel) { + jobs[site.id]?.cancel() + val flow = flowFor(site.id) + jobs[site.id] = appScope.launch { + val readiness = runPipeline(site) + flow.value = readiness + if (readiness is SiteReadiness.Ready) ready.add(site.id) + } + } + + private fun flowFor(siteLocalId: Int): MutableStateFlow = + states.getOrPut(siteLocalId) { MutableStateFlow(SiteReadiness.Probing) } + + private fun shouldRun(siteLocalId: Int): Boolean = + jobs[siteLocalId]?.isActive != true && siteLocalId !in ready + + private suspend fun runPipeline(site: SiteModel): SiteReadiness { + // Re-read from the store so we provision against the persisted SiteModel and mutate that + // instance in place — later stages (URL recovery, capability probe) see the fresh creds. + val storedSite = siteStore.sites.firstOrNull { it.id == site.id } ?: site + return when (val auth = ensureAuth(storedSite)) { + SiteAuthState.Provisioned -> { + recoverRestUrl(storedSite) + detectCapabilities(storedSite) + } + else -> SiteReadiness.NeedsAuth(auth) + } + } + + /** + * Stage 1 — ensure the site has working application-password credentials. + * Validates stored creds with Basic auth against the direct host; on a + * confirmed rejection wipes them and mints fresh ones via the FluxC Jetpack + * tunnel (the only path that works for Atomic / Jetpack-WPCom-REST sites). + */ + private suspend fun ensureAuth(site: SiteModel): SiteAuthState { + val hadCredentials = !applicationPasswordLoginHelper.siteHasBadCredentials(site) + if (hadCredentials) { + when (applicationPasswordValidator.validate(site)) { + ApplicationPasswordValidator.Outcome.Valid -> + return SiteAuthState.Provisioned + ApplicationPasswordValidator.Outcome.NetworkUnavailable -> { + // Don't punish flaky networks — treat as in-progress and retry next run. + appLogWrapper.d(AppLog.T.MAIN, "A_P: Validation network error for ${site.url}") + return SiteAuthState.Provisioning + } + ApplicationPasswordValidator.Outcome.Invalid -> { + // Stored creds are stale (revoked, deleted) — clear them so the mint below + // creates fresh ones, and invalidate the cached client. + appLogWrapper.d(AppLog.T.MAIN, "A_P: Stored creds invalid for ${site.url}, clearing") + siteStore.deleteStoredApplicationPasswordCredentials(site) + wpApiClientProvider.clearSelfHostedClient(site.id) + } + } + } + val createResult = siteStore.createApplicationPassword(site) + if (!createResult.isError && createResult.credentials != null) { + wpApiClientProvider.clearSelfHostedClient(site.id) + appLogWrapper.d(AppLog.T.MAIN, "A_P: Headless mint succeeded for ${site.url}") + return SiteAuthState.Provisioned + } + appLogWrapper.d( + AppLog.T.MAIN, + "A_P: Headless mint failed for ${site.url} (notSupported=${createResult.error?.notSupported})" + ) + return SiteAuthState.Unprovisionable(hadCredentials = hadCredentials) + } + + /** + * Stage 2 — recover the REST API root for Atomic sites minted through the + * Jetpack tunnel, which never runs discovery and so leaves `wpApiRestUrl` + * null. One owner for the heal that the card and preloader used to duplicate. + */ + private suspend fun recoverRestUrl(site: SiteModel) { + if (!site.wpApiRestUrl.isNullOrEmpty()) return + siteApiRestUrlRecoverer.discoverApiRootUrl(site.url)?.let { apiRootUrl -> + site.wpApiRestUrl = apiRootUrl + siteApiRestUrlRecoverer.persistApiRootUrl(site.id, apiRootUrl) + } + } + + /** + * Stage 3 — probe the REST API for editor-capability support and persist it. + * Reached only once auth is [SiteAuthState.Provisioned], so credentials are + * guaranteed present: a failure here is a real transport problem, not a + * pending mint. + */ + private suspend fun detectCapabilities(site: SiteModel): SiteReadiness { + val ok = editorSettingsRepository.fetchEditorCapabilitiesForSite(site) + val hasCache = editorSettingsRepository.hasCachedCapabilities(site) + return when { + ok || hasCache -> SiteReadiness.Ready + !networkUtilsWrapper.isNetworkAvailable() -> SiteReadiness.TransientError + else -> SiteReadiness.Unreachable + } + } +} + +/** + * Whether a site's application password is usable. Owned by [SiteProvisioningSource]; + * rendered by the application-password card. + */ +sealed interface SiteAuthState { + /** Credentials are usable (validated, or freshly minted). */ + data object Provisioned : SiteAuthState + + /** Not usable yet, but not a terminal failure — a mint is implied / a transient + * validation error occurred. The card stays hidden; the next run retries. */ + data object Provisioning : SiteAuthState + + /** Terminal: the mint failed. [hadCredentials] distinguishes a re-authentication + * (creds went bad) from a first-time authentication prompt. */ + data class Unprovisionable(val hadCredentials: Boolean) : SiteAuthState +} + +/** + * The combined per-site readiness the [SiteProvisioningSource] exposes. The + * connectivity banner renders [Unreachable], the application-password card + * renders [NeedsAuth], and the editor preloader awaits a non-[Probing] value — + * each a slice of the one state, so they can't disagree. + */ +sealed interface SiteReadiness { + /** The pipeline is running and hasn't produced a result yet. */ + data object Probing : SiteReadiness + + /** Stopped at the auth stage — credentials aren't usable. Carries the + * [SiteAuthState] so the card can pick re-auth vs. first-auth. */ + data class NeedsAuth(val auth: SiteAuthState) : SiteReadiness + + /** Provisioned and editor capabilities are known (detected or cached). */ + data object Ready : SiteReadiness + + /** Provisioned, but the capability probe failed — the site looks unreachable. + * The only state that surfaces the connectivity banner. */ + data object Unreachable : SiteReadiness + + /** Provisioned, but a transient failure (e.g. offline). Retried on the next run. */ + data object TransientError : SiteReadiness +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt index 83cf7b1bc981..226f93355c84 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt @@ -14,12 +14,12 @@ import org.wordpress.android.fluxc.generated.SiteActionBuilder import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.network.discovery.SelfHostedEndpointFinder import org.wordpress.android.fluxc.network.xmlrpc.site.SiteXMLRPCClient -import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider import org.wordpress.android.fluxc.store.SiteStore import org.wordpress.android.fluxc.utils.AppLogWrapper -import org.wordpress.android.repositories.EditorCapabilityDetector +import org.wordpress.android.repositories.SiteAuthState +import org.wordpress.android.repositories.SiteProvisioningSource +import org.wordpress.android.repositories.SiteReadiness import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper -import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer import org.wordpress.android.ui.mysite.MySiteCardAndItem import org.wordpress.android.ui.mysite.MySiteCardAndItem.Card.QuickLinksItem.QuickLinkItem import org.wordpress.android.ui.mysite.SiteNavigationAction @@ -32,17 +32,25 @@ import org.wordpress.android.viewmodel.Event import javax.inject.Inject import javax.inject.Named +/** + * Renders the application-password card from the site's readiness. Credential + * provisioning (validate / mint) now lives in [SiteProvisioningSource]; this + * slice is a view over the auth slice of that state: + * + * - [SiteAuthState.Unprovisionable] → a re-authentication banner (creds went + * bad) or a first-time "authenticate" card, looked up lazily. + * - provisioned (any non-[SiteReadiness.NeedsAuth] terminal state) → hidden, + * except true self-hosted sites missing an XML-RPC endpoint, which get the + * XML-RPC-disabled card plus a background rediscovery attempt. + */ class ApplicationPasswordViewModelSlice @Inject constructor( private val applicationPasswordLoginHelper: ApplicationPasswordLoginHelper, private val siteStore: SiteStore, private val appLogWrapper: AppLogWrapper, - private val wpApiClientProvider: WpApiClientProvider, - private val applicationPasswordValidator: ApplicationPasswordValidator, private val selfHostedEndpointFinder: SelfHostedEndpointFinder, private val siteXMLRPCClient: SiteXMLRPCClient, - private val siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer, + private val siteProvisioningSource: SiteProvisioningSource, private val dispatcher: Dispatcher, - private val editorCapabilityDetector: EditorCapabilityDetector, @Named(IO_THREAD) private val ioDispatcher: CoroutineDispatcher, ) { lateinit var scope: CoroutineScope @@ -60,102 +68,52 @@ class ApplicationPasswordViewModelSlice @Inject constructor( val uiModelMutable = MutableLiveData() val uiModel: LiveData = uiModelMutable - // Single-flight guard: buildCard is invoked from onResume / refresh / onSitePicked, which can - // fire close together. Without this, two coroutines both pass the "creds missing" check in - // ApplicationPasswordsManager and issue two server-side mints. Worse, the 409 conflict handler - // then deletes-and-recreates the winner's password, so the losing racer destroys working creds. - private var buildJob: Job? = null + private var collectJob: Job? = null + private var currentSite: SiteModel? = null fun buildCard(siteModel: SiteModel) { - if (buildJob?.isActive == true) { - appLogWrapper.d( - AppLog.T.MAIN, - "A_P: Skipping buildCard for ${siteModel.url} - previous run still in flight" - ) - return - } - buildJob = scope.launch { - val storedSite = siteStore.sites.firstOrNull { it.id == siteModel.id } ?: siteModel - val hadCreds = !applicationPasswordLoginHelper.siteHasBadCredentials(storedSite) - - // Step 1: if we already have stored creds, validate them with Basic auth against the - // direct host. This actually exercises the application password (unlike - // WpApiClientProvider.getWpApiClient, which routes WPCom-flagged sites through the - // bearer-token path and would not catch a revoked password). - if (hadCreds) { - when (applicationPasswordValidator.validate(storedSite)) { - ApplicationPasswordValidator.Outcome.Valid -> { - // Heal in the background so the card hides immediately on a slow network. - scope.launch { healApiRestUrlIfMissing(storedSite) } - handleValidAuth(storedSite) - return@launch - } - ApplicationPasswordValidator.Outcome.NetworkUnavailable -> { - // Don't punish flaky networks — leave the card hidden and try again next time. - uiModelMutable.postValue(null) - appLogWrapper.d(AppLog.T.MAIN, "A_P: Validation network error for ${storedSite.url}") - return@launch - } - ApplicationPasswordValidator.Outcome.Invalid -> { - // Stored creds are stale (revoked, deleted, etc.) — clear them so the next - // mint creates fresh ones, and invalidate the cached client. - appLogWrapper.d(AppLog.T.MAIN, "A_P: Stored creds invalid for ${storedSite.url}, clearing") - siteStore.deleteStoredApplicationPasswordCredentials(storedSite) - wpApiClientProvider.clearSelfHostedClient(storedSite.id) - } - } - } - - // Step 2: mint a fresh application password via the FluxC Jetpack tunnel. wordpress-rs - // can't do this today — the WP.com REST proxy doesn't expose the application-passwords - // endpoint under /wp/v2/sites/{id}/... (see Automattic/wordpress-rs#1350) — so FluxC's - // Jetpack-tunnel client is the only working path for Atomic / Jetpack-WPCom-REST sites. - val createResult = siteStore.createApplicationPassword(storedSite) - if (!createResult.isError && createResult.credentials != null) { - wpApiClientProvider.clearSelfHostedClient(storedSite.id) - appLogWrapper.d(AppLog.T.MAIN, "A_P: Headless mint succeeded for ${storedSite.url}") - // The first-login capability probe can lose the race to this async mint. storedSite - // was just mutated in place with the new credentials (SiteStore - // .persistApplicationPasswordCredentials), so re-probe against this exact instance — - // no stale-SiteModel re-read, and capabilities settle without a manual pull-to-refresh. - editorCapabilityDetector.refresh(storedSite) - // The mint goes through the Jetpack tunnel and never runs discovery — without this - // step, freshly minted Atomic sites end up with working creds but a NULL - // wpApiRestUrl in the local DB. Run in the background so the card hides immediately. - scope.launch { healApiRestUrlIfMissing(storedSite) } - handleValidAuth(storedSite) - return@launch - } - appLogWrapper.d( - AppLog.T.MAIN, - "A_P: Headless mint failed for ${storedSite.url} (notSupported=" + - "${createResult.error?.notSupported})" - ) - - // Step 3: mint failed. If we started with creds, show the reauth banner; otherwise the - // standard "authenticate" card. Either way, discovery is required to populate the URL. - if (hadCreds) { - buildReauthenticationBanner(storedSite) - } else { - buildAuthenticationCard(storedSite) + collectJob?.cancel() + currentSite = siteModel + collectJob = scope.launch { + siteProvisioningSource.stateFor(siteModel).collect { readiness -> + // Bail if the user switched sites while suspended — postValue is not a + // suspension point, so cancellation alone won't catch this. + if (currentSite?.id != siteModel.id) return@collect + renderCard(siteModel, readiness) } } } - private suspend fun healApiRestUrlIfMissing(site: SiteModel) { - if (!site.wpApiRestUrl.isNullOrEmpty()) return - siteApiRestUrlRecoverer.discoverApiRootUrl(site.url)?.let { apiRootUrl -> - site.wpApiRestUrl = apiRootUrl - siteApiRestUrlRecoverer.persistApiRootUrl(site.id, apiRootUrl) + private suspend fun renderCard(site: SiteModel, readiness: SiteReadiness) { + when (readiness) { + is SiteReadiness.NeedsAuth -> when (val auth = readiness.auth) { + is SiteAuthState.Unprovisionable -> + if (auth.hadCredentials) buildReauthenticationBanner(site) else buildAuthenticationCard(site) + SiteAuthState.Provisioning -> { + // Mint in flight / transient validation error — hide and let the next run retry. + uiModelMutable.postValue(null) + appLogWrapper.d(AppLog.T.MAIN, "A_P: Provisioning in progress for ${site.url}") + } + SiteAuthState.Provisioned -> Unit // unreachable: Provisioned never wraps in NeedsAuth + } + // Any terminal provisioned state — the credentials are usable, so the only card left to + // show is the self-hosted XML-RPC fallback. Capability outcome (Ready/Unreachable) is the + // connectivity banner's concern, not this card's. + SiteReadiness.Ready, + SiteReadiness.Unreachable, + SiteReadiness.TransientError -> handleProvisioned(site) + SiteReadiness.Probing -> Unit // leave the card unchanged while the pipeline runs } } - private fun handleValidAuth(site: SiteModel) { + private fun handleProvisioned(site: SiteModel) { + // Re-read the stored site so we see credentials/endpoints the pipeline just persisted. + val storedSite = siteStore.sites.firstOrNull { it.id == site.id } ?: site // Only true self-hosted sites need the XML-RPC fallback path — Atomic and Jetpack-WPCom-REST // sites talk REST end-to-end and don't need XML-RPC. - if (!site.isUsingWpComRestApi && site.xmlRpcUrl.isNullOrEmpty()) { - buildXmlRpcDisabledCard(site) - attemptXmlRpcRediscovery(site) + if (!storedSite.isUsingWpComRestApi && storedSite.xmlRpcUrl.isNullOrEmpty()) { + buildXmlRpcDisabledCard(storedSite) + attemptXmlRpcRediscovery(storedSite) } else { uiModelMutable.postValue(null) appLogWrapper.d(AppLog.T.MAIN, "A_P: Hiding card for ${site.url} - authenticated") @@ -264,7 +222,8 @@ class ApplicationPasswordViewModelSlice @Inject constructor( dispatcher.dispatch( SiteActionBuilder.newUpdateSiteAction(site) ) - buildCard(site) + // Endpoint recovered — hide the XML-RPC-disabled card. + uiModelMutable.postValue(null) } catch ( @Suppress("SwallowedException") e: SelfHostedEndpointFinder.DiscoveryException diff --git a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSlice.kt b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSlice.kt index 3123e1c5e9d9..fc17191982db 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSlice.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSlice.kt @@ -7,13 +7,13 @@ import kotlinx.coroutines.Job import kotlinx.coroutines.launch import org.wordpress.android.R import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.repositories.EditorCapabilityDetectionState -import org.wordpress.android.repositories.EditorCapabilityDetector +import org.wordpress.android.repositories.SiteProvisioningSource +import org.wordpress.android.repositories.SiteReadiness import org.wordpress.android.ui.mysite.MySiteCardAndItem import javax.inject.Inject class SiteConnectivityBannerViewModelSlice @Inject constructor( - private val editorCapabilityDetector: EditorCapabilityDetector, + private val siteProvisioningSource: SiteProvisioningSource, ) { private lateinit var scope: CoroutineScope private var collectJob: Job? = null @@ -27,24 +27,23 @@ class SiteConnectivityBannerViewModelSlice @Inject constructor( } /** - * Subscribes the banner to [site]'s editor-capability detection state. The - * banner is a thin view over that state — it surfaces only when detection - * reports the site [Unreachable][EditorCapabilityDetectionState.Unreachable]. - * Every other state (probing, pending credentials, offline, ready) leaves it - * hidden, so the dedup, offline-suppression, and pending-credential handling - * that used to live here now belong to the one detector. [isUserInitiated] - * (pull-to-refresh, banner retry) forces a fresh probe. + * Subscribes the banner to [site]'s readiness. The banner is a thin view over + * that state — it surfaces only when the site is provisioned but the capability + * probe failed ([SiteReadiness.Unreachable]). Every other state (probing, needs + * auth, offline, ready) leaves it hidden: when credentials are the problem the + * application-password card owns it, and the banner stays out of the way. + * [isUserInitiated] (pull-to-refresh, retry) forces a fresh run. */ fun fetchCapabilities(site: SiteModel, isUserInitiated: Boolean) { collectJob?.cancel() currentSite = site - if (isUserInitiated) editorCapabilityDetector.refresh(site) + if (isUserInitiated) siteProvisioningSource.invalidate(site) collectJob = scope.launch { - editorCapabilityDetector.stateFor(site).collect { state -> + siteProvisioningSource.stateFor(site).collect { readiness -> // Bail if the user switched sites while suspended — postValue is // not a suspension point, so cancellation alone won't catch this. if (currentSite?.id != site.id) return@collect - val showBanner = state is EditorCapabilityDetectionState.Unreachable + val showBanner = readiness is SiteReadiness.Unreachable _uiModel.postValue(if (showBanner) buildBanner() else null) } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergEditorPreloader.kt b/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergEditorPreloader.kt index 27f3b8632ec5..e90c5e510f0d 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergEditorPreloader.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergEditorPreloader.kt @@ -10,9 +10,9 @@ import kotlinx.coroutines.launch import org.wordpress.android.datasets.SiteSettingsProvider import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.store.AccountStore +import org.wordpress.android.fluxc.store.SiteStore import org.wordpress.android.modules.BG_THREAD -import org.wordpress.android.repositories.EditorCapabilityDetector -import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer +import org.wordpress.android.repositories.SiteProvisioningSource import org.wordpress.android.util.AppLog import org.wordpress.gutenberg.model.EditorDependencies import java.util.concurrent.ConcurrentHashMap @@ -63,8 +63,8 @@ class GutenbergEditorPreloader @Inject constructor( private val gutenbergKitSettingsBuilder: GutenbergKitSettingsBuilder, private val siteSettingsProvider: SiteSettingsProvider, private val editorServiceProvider: EditorServiceProvider, - private val editorCapabilityDetector: EditorCapabilityDetector, - private val siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer, + private val siteProvisioningSource: SiteProvisioningSource, + private val siteStore: SiteStore, @Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher ) { private sealed class PreloadState { @@ -95,15 +95,14 @@ class GutenbergEditorPreloader @Inject constructor( val siteId = site.id val job = scope.launch(bgDispatcher) { try { - if (site.wpApiRestUrl.isNullOrEmpty()) { - siteApiRestUrlRecoverer.discoverApiRootUrl(site.url) - ?.let { site.wpApiRestUrl = it } - } - // Detect (and persist) editor capabilities via the shared - // detector so the preloader and connectivity banner can't - // double-probe. We only need the probe to have run before - // building config, so the settled state itself is ignored. - editorCapabilityDetector.awaitProbe(site) + // The provisioning source mints creds, recovers the REST root, and + // detects capabilities — one shared, deduplicated run, so the preloader + // and connectivity banner can't double-probe. We only need it to have + // run; re-read the provisioned site so the config points at the + // recovered REST root. + siteProvisioningSource.await(site) + val provisionedSite = siteStore.sites + .firstOrNull { it.id == siteId } ?: site // Preloading produces EditorDependencies, which the editor // consumes alongside its own per-launch EditorConfiguration. // Cookies and network-logging are per-launch concerns the @@ -111,7 +110,7 @@ class GutenbergEditorPreloader @Inject constructor( // defaults here. val config = gutenbergKitSettingsBuilder .buildPostConfiguration( - site = site, + site = provisionedSite, accessToken = accountStore.accessToken, cookies = emptyMap(), isNetworkLoggingEnabled = false, @@ -149,9 +148,9 @@ class GutenbergEditorPreloader @Inject constructor( @MainThread fun refreshPreloading(site: SiteModel, scope: CoroutineScope) { clearSite(site) - // Pull-to-refresh: force a fresh capability probe so the awaitProbe in + // Pull-to-refresh: force a fresh provisioning run so the await in // preloadIfNeeded re-detects instead of returning the cached result. - editorCapabilityDetector.refresh(site) + siteProvisioningSource.invalidate(site) preloadIfNeeded(site, scope) } diff --git a/WordPress/src/test/java/org/wordpress/android/repositories/EditorCapabilityDetectorTest.kt b/WordPress/src/test/java/org/wordpress/android/repositories/EditorCapabilityDetectorTest.kt deleted file mode 100644 index ea597c101dd3..000000000000 --- a/WordPress/src/test/java/org/wordpress/android/repositories/EditorCapabilityDetectorTest.kt +++ /dev/null @@ -1,207 +0,0 @@ -package org.wordpress.android.repositories - -import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.test.StandardTestDispatcher -import kotlinx.coroutines.test.advanceUntilIdle -import org.assertj.core.api.Assertions.assertThat -import org.junit.Before -import org.junit.Test -import org.mockito.Mock -import org.mockito.kotlin.never -import org.mockito.kotlin.times -import org.mockito.kotlin.verify -import org.mockito.kotlin.whenever -import org.wordpress.android.BaseUnitTest -import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.util.NetworkUtilsWrapper - -private const val TEST_SITE_LOCAL_ID = 7 - -@ExperimentalCoroutinesApi -class EditorCapabilityDetectorTest : BaseUnitTest(StandardTestDispatcher()) { - @Mock - lateinit var editorSettingsRepository: EditorSettingsRepository - - @Mock - lateinit var networkUtilsWrapper: NetworkUtilsWrapper - - private lateinit var site: SiteModel - private lateinit var detector: EditorCapabilityDetector - - @Before - fun setUp() { - site = SiteModel().apply { id = TEST_SITE_LOCAL_ID } - detector = EditorCapabilityDetector( - editorSettingsRepository, - networkUtilsWrapper, - testScope(), - ) - } - - // region state mapping - - @Test - fun `given probe succeeds, then state is Ready`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(true) - - val flow = detector.stateFor(site) - advanceUntilIdle() - - assertThat(flow.value).isEqualTo(EditorCapabilityDetectionState.Ready) - } - - @Test - fun `given probe fails but capabilities are cached, then state is Ready`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(site)).thenReturn(true) - - val flow = detector.stateFor(site) - advanceUntilIdle() - - assertThat(flow.value).isEqualTo(EditorCapabilityDetectionState.Ready) - } - - @Test - fun `given probe fails while awaiting an application password, then state is Pending`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(site)).thenReturn(false) - whenever(editorSettingsRepository.isAwaitingApplicationPassword(site)).thenReturn(true) - - val flow = detector.stateFor(site) - advanceUntilIdle() - - assertThat(flow.value).isEqualTo(EditorCapabilityDetectionState.Pending) - } - - @Test - fun `given probe fails while offline, then state is TransientError`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(site)).thenReturn(false) - whenever(editorSettingsRepository.isAwaitingApplicationPassword(site)).thenReturn(false) - whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(false) - - val flow = detector.stateFor(site) - advanceUntilIdle() - - assertThat(flow.value).isEqualTo(EditorCapabilityDetectionState.TransientError) - } - - @Test - fun `given probe fails online with no pending auth, then state is Unreachable`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(site)).thenReturn(false) - whenever(editorSettingsRepository.isAwaitingApplicationPassword(site)).thenReturn(false) - whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) - - val flow = detector.stateFor(site) - advanceUntilIdle() - - assertThat(flow.value).isEqualTo(EditorCapabilityDetectionState.Unreachable) - } - - // endregion - - // region deduplication - - @Test - fun `given a prior successful probe, when stateFor is called again, then it does not re-probe`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(true) - - detector.stateFor(site) - advanceUntilIdle() - detector.stateFor(site) - advanceUntilIdle() - - verify(editorSettingsRepository, times(1)).fetchEditorCapabilitiesForSite(site) - } - - @Test - fun `given a prior failed probe, when stateFor is called again, then it re-probes`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(false, true) - whenever(editorSettingsRepository.hasCachedCapabilities(site)).thenReturn(false) - whenever(editorSettingsRepository.isAwaitingApplicationPassword(site)).thenReturn(false) - whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) - - val flow = detector.stateFor(site) - advanceUntilIdle() - assertThat(flow.value).isEqualTo(EditorCapabilityDetectionState.Unreachable) - detector.stateFor(site) - advanceUntilIdle() - - verify(editorSettingsRepository, times(2)).fetchEditorCapabilitiesForSite(site) - assertThat(flow.value).isEqualTo(EditorCapabilityDetectionState.Ready) - } - - // endregion - - // region refresh - - @Test - fun `given a prior successful probe, when refresh is called, then it re-probes`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(true) - - detector.stateFor(site) - advanceUntilIdle() - detector.refresh(site) - advanceUntilIdle() - - verify(editorSettingsRepository, times(2)).fetchEditorCapabilitiesForSite(site) - } - - @Test - fun `given a probe in flight, when refresh is called, then it does not start a second probe`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(true) - - detector.stateFor(site) // StandardTestDispatcher: job is launched but not yet run - detector.refresh(site) // a probe is already in flight — must be a no-op - advanceUntilIdle() - - verify(editorSettingsRepository, times(1)).fetchEditorCapabilitiesForSite(site) - } - - // endregion - - // region awaitProbe - - @Test - fun `given awaitProbe, then it runs detection and returns the settled state`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(true) - - val state = detector.awaitProbe(site) - - assertThat(state).isEqualTo(EditorCapabilityDetectionState.Ready) - verify(editorSettingsRepository).fetchEditorCapabilitiesForSite(site) - } - - @Test - fun `given a prior successful probe, when awaitProbe is called again, then it does not re-probe`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(true) - - detector.awaitProbe(site) - detector.awaitProbe(site) - - verify(editorSettingsRepository, times(1)).fetchEditorCapabilitiesForSite(site) - } - - // endregion - - // region clear - - @Test - fun `given a probed site, when clear is called, then the next probe runs again`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(site)).thenReturn(true) - - detector.awaitProbe(site) - detector.clear() - detector.awaitProbe(site) - - verify(editorSettingsRepository, times(2)).fetchEditorCapabilitiesForSite(site) - } - - @Test - fun `given no interaction, then no probe runs`() = test { - verify(editorSettingsRepository, never()).fetchEditorCapabilitiesForSite(site) - } - - // endregion -} diff --git a/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt b/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt new file mode 100644 index 000000000000..a695ed3b6582 --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt @@ -0,0 +1,237 @@ +package org.wordpress.android.repositories + +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.StandardTestDispatcher +import org.assertj.core.api.Assertions.assertThat +import org.junit.Before +import org.junit.Test +import org.mockito.Mock +import org.mockito.kotlin.any +import org.mockito.kotlin.eq +import org.mockito.kotlin.never +import org.mockito.kotlin.times +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever +import org.wordpress.android.BaseUnitTest +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.fluxc.network.BaseRequest.BaseNetworkError +import org.wordpress.android.fluxc.network.BaseRequest.GenericErrorType +import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.ApplicationPasswordCredentials +import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider +import org.wordpress.android.fluxc.store.SiteStore +import org.wordpress.android.fluxc.store.SiteStore.OnApplicationPasswordCreated +import org.wordpress.android.fluxc.utils.AppLogWrapper +import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper +import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer +import org.wordpress.android.ui.mysite.cards.applicationpassword.ApplicationPasswordValidator +import org.wordpress.android.util.NetworkUtilsWrapper + +private const val TEST_SITE_LOCAL_ID = 7 + +@ExperimentalCoroutinesApi +class SiteProvisioningSourceTest : BaseUnitTest(StandardTestDispatcher()) { + @Mock lateinit var siteStore: SiteStore + @Mock lateinit var applicationPasswordLoginHelper: ApplicationPasswordLoginHelper + @Mock lateinit var applicationPasswordValidator: ApplicationPasswordValidator + @Mock lateinit var wpApiClientProvider: WpApiClientProvider + @Mock lateinit var siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer + @Mock lateinit var editorSettingsRepository: EditorSettingsRepository + @Mock lateinit var networkUtilsWrapper: NetworkUtilsWrapper + @Mock lateinit var appLogWrapper: AppLogWrapper + + private lateinit var site: SiteModel + private lateinit var source: SiteProvisioningSource + + @Before + fun setUp() { + site = SiteModel().apply { + id = TEST_SITE_LOCAL_ID + url = "https://test.example.com" + // A non-null REST root so recoverRestUrl short-circuits unless a test clears it. + wpApiRestUrl = "https://test.example.com/wp-json" + } + source = SiteProvisioningSource( + siteStore, + applicationPasswordLoginHelper, + applicationPasswordValidator, + wpApiClientProvider, + siteApiRestUrlRecoverer, + editorSettingsRepository, + networkUtilsWrapper, + appLogWrapper, + testScope(), + ) + } + + private fun stubHasStoredCredentials(value: Boolean) = + whenever(applicationPasswordLoginHelper.siteHasBadCredentials(any())).thenReturn(!value) + + private suspend fun stubValidate(outcome: ApplicationPasswordValidator.Outcome) = + whenever(applicationPasswordValidator.validate(any())).thenReturn(outcome) + + private suspend fun stubMintSuccess() = + whenever(siteStore.createApplicationPassword(any())).thenReturn( + OnApplicationPasswordCreated(site, ApplicationPasswordCredentials("user", "pass", uuid = "u")) + ) + + private suspend fun stubMintFailure() = + whenever(siteStore.createApplicationPassword(any())).thenReturn( + OnApplicationPasswordCreated(site, BaseNetworkError(GenericErrorType.UNKNOWN, "fail"), notSupported = false) + ) + + private suspend fun stubCapabilityProbe(ok: Boolean, cached: Boolean = false) { + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(any())).thenReturn(ok) + // `ok || hasCache` short-circuits, so the cache is only read (and only needs stubbing) + // when the live probe failed — stubbing it on success would be an unnecessary stub. + if (!ok) whenever(editorSettingsRepository.hasCachedCapabilities(any())).thenReturn(cached) + } + + // region auth stage + + @Test + fun `given valid stored credentials, then provisioned and ready`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + + assertThat(source.await(site)).isEqualTo(SiteReadiness.Ready) + verify(siteStore, never()).createApplicationPassword(any()) + } + + @Test + fun `given no credentials, then mints and is ready`() = test { + stubHasStoredCredentials(false) + stubMintSuccess() + stubCapabilityProbe(ok = true) + + assertThat(source.await(site)).isEqualTo(SiteReadiness.Ready) + verify(siteStore).createApplicationPassword(any()) + } + + @Test + fun `given mint fails with no prior credentials, then needs first-time auth`() = test { + stubHasStoredCredentials(false) + stubMintFailure() + + val result = source.await(site) + + assertThat(result).isEqualTo(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = false))) + // Auth failed — the capability probe must not run. + verify(editorSettingsRepository, never()).fetchEditorCapabilitiesForSite(any()) + } + + @Test + fun `given stored credentials that fail to mint, then needs re-auth`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Invalid) + stubMintFailure() + + val result = source.await(site) + + assertThat(result).isEqualTo(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = true))) + verify(siteStore).deleteStoredApplicationPasswordCredentials(any()) + } + + @Test + fun `given a transient validation error, then provisioning and no mint or probe`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.NetworkUnavailable) + + val result = source.await(site) + + assertThat(result).isEqualTo(SiteReadiness.NeedsAuth(SiteAuthState.Provisioning)) + verify(siteStore, never()).createApplicationPassword(any()) + verify(editorSettingsRepository, never()).fetchEditorCapabilitiesForSite(any()) + } + + // endregion + + // region url recovery + capability stages + + @Test + fun `given provisioned with a missing REST root, then it recovers and persists the url`() = test { + site.wpApiRestUrl = "" + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + whenever(siteApiRestUrlRecoverer.discoverApiRootUrl(site.url)) + .thenReturn("https://test.example.com/custom-rest") + stubCapabilityProbe(ok = true) + + source.await(site) + + verify(siteApiRestUrlRecoverer).persistApiRootUrl(eq(site.id), eq("https://test.example.com/custom-rest")) + } + + @Test + fun `given probe fails while offline, then transient error`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = false) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(false) + + assertThat(source.await(site)).isEqualTo(SiteReadiness.TransientError) + } + + @Test + fun `given probe fails while online, then unreachable`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = false) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) + + assertThat(source.await(site)).isEqualTo(SiteReadiness.Unreachable) + } + + @Test + fun `given probe fails but capabilities are cached, then ready`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = false, cached = true) + + assertThat(source.await(site)).isEqualTo(SiteReadiness.Ready) + } + + // endregion + + // region single-flight / dedup / invalidate / clear + + @Test + fun `given a prior ready run, when awaited again, then it does not re-run`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + + source.await(site) + source.await(site) + + verify(applicationPasswordValidator, times(1)).validate(any()) + } + + @Test + fun `given a prior ready run, when invalidated, then it re-runs`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + + source.await(site) + source.invalidate(site) + source.await(site) + + verify(applicationPasswordValidator, times(2)).validate(any()) + } + + @Test + fun `given a ready site, when cleared, then the next run re-runs`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + + source.await(site) + source.clear() + source.await(site) + + verify(applicationPasswordValidator, times(2)).validate(any()) + } + + // endregion +} diff --git a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt index b19afec7af00..58b4ddc425ad 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt @@ -1,9 +1,9 @@ package org.wordpress.android.ui.mysite.cards.applicationpassword import junit.framework.TestCase.assertNull -import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.flow.MutableStateFlow +import org.assertj.core.api.Assertions.assertThat import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -11,438 +11,143 @@ import org.mockito.Mock import org.mockito.MockitoAnnotations import org.mockito.junit.MockitoJUnitRunner import org.mockito.kotlin.any -import org.mockito.kotlin.doSuspendableAnswer 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 import org.wordpress.android.BaseUnitTest -import org.mockito.kotlin.mock +import org.wordpress.android.R import org.wordpress.android.fluxc.Dispatcher import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.fluxc.model.SitesModel -import org.wordpress.android.fluxc.network.BaseRequest.BaseNetworkError -import org.wordpress.android.fluxc.network.BaseRequest.GenericErrorType import org.wordpress.android.fluxc.network.discovery.SelfHostedEndpointFinder import org.wordpress.android.fluxc.network.xmlrpc.site.SiteXMLRPCClient -import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.ApplicationPasswordCredentials -import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider import org.wordpress.android.fluxc.store.SiteStore -import org.wordpress.android.fluxc.store.SiteStore.OnApplicationPasswordCreated import org.wordpress.android.fluxc.utils.AppLogWrapper -import org.wordpress.android.repositories.EditorCapabilityDetector +import org.wordpress.android.repositories.SiteAuthState +import org.wordpress.android.repositories.SiteProvisioningSource +import org.wordpress.android.repositories.SiteReadiness import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper -import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer import org.wordpress.android.ui.mysite.MySiteCardAndItem -import kotlin.test.assertNotNull private const val TEST_URL = "https://www.test.com" -private const val TEST_SITE_NAME = "My Site" private const val TEST_SITE_ID = 1 -private const val TEST_SITE_ICON = "http://site.com/icon.jpg" -private const val TEST_URL_AUTH = "https://www.test.com/auth" -private const val TEST_URL_AUTH_SUFFIX = "?app_name=android-jetpack-client&success_url=callback://callback" +private const val TEST_AUTH_URL = "https://www.test.com/auth" @ExperimentalCoroutinesApi @RunWith(MockitoJUnitRunner::class) class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { - @Mock - lateinit var applicationPasswordLoginHelper: ApplicationPasswordLoginHelper - - @Mock - lateinit var siteStore: SiteStore - - @Mock - lateinit var appLogWrapper: AppLogWrapper - - @Mock - lateinit var wpApiClientProvider: WpApiClientProvider - - @Mock - lateinit var applicationPasswordValidator: ApplicationPasswordValidator - - @Mock - lateinit var selfHostedEndpointFinder: SelfHostedEndpointFinder - - @Mock - lateinit var siteXMLRPCClient: SiteXMLRPCClient - - @Mock - lateinit var siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer - - @Mock - lateinit var dispatcher: Dispatcher - - @Mock - lateinit var editorCapabilityDetector: EditorCapabilityDetector + @Mock lateinit var applicationPasswordLoginHelper: ApplicationPasswordLoginHelper + @Mock lateinit var siteStore: SiteStore + @Mock lateinit var appLogWrapper: AppLogWrapper + @Mock lateinit var selfHostedEndpointFinder: SelfHostedEndpointFinder + @Mock lateinit var siteXMLRPCClient: SiteXMLRPCClient + @Mock lateinit var siteProvisioningSource: SiteProvisioningSource + @Mock lateinit var dispatcher: Dispatcher private lateinit var siteTest: SiteModel - - private var applicationPasswordCard: MySiteCardAndItem? = null - - private lateinit var applicationPasswordViewModelSlice: ApplicationPasswordViewModelSlice + private var card: MySiteCardAndItem? = null + private lateinit var slice: ApplicationPasswordViewModelSlice @Before fun setUp() { MockitoAnnotations.openMocks(this) - - applicationPasswordViewModelSlice = ApplicationPasswordViewModelSlice( + slice = ApplicationPasswordViewModelSlice( applicationPasswordLoginHelper, siteStore, appLogWrapper, - wpApiClientProvider, - applicationPasswordValidator, selfHostedEndpointFinder, siteXMLRPCClient, - siteApiRestUrlRecoverer, + siteProvisioningSource, dispatcher, - editorCapabilityDetector, - testDispatcher() - ).apply { - initialize(testScope()) - } + testDispatcher(), + ).apply { initialize(testScope()) } siteTest = SiteModel().apply { id = TEST_SITE_ID url = TEST_URL - name = TEST_SITE_NAME - iconUrl = TEST_SITE_ICON - siteId = TEST_SITE_ID.toLong() - apiRestUsernamePlain = "testuser" - apiRestPasswordPlain = "testpass" - // Mark xmlRpcUrl so handleValidAuth's XML-RPC-disabled fallback doesn't fire — that - // path is exercised by the dedicated xmlRpcRediscovery tests below. - xmlRpcUrl = "https://www.test.com/xmlrpc.php" + // A WP.com-REST site by default, so a provisioned site hides the card (no XML-RPC path). + setIsWPCom(true) } - - applicationPasswordCard = null - applicationPasswordViewModelSlice.uiModel.observeForever { card -> - applicationPasswordCard = card - } - - // By default, treat the site as having no stored credentials. Tests that exercise the - // validate-stored-creds path override this. - whenever(applicationPasswordLoginHelper.siteHasBadCredentials(any())).thenReturn(true) + card = null + slice.uiModel.observeForever { card = it } } - private suspend fun stubMintFailure(notSupported: Boolean = false) { - whenever(siteStore.createApplicationPassword(any())).thenReturn( - OnApplicationPasswordCreated( - siteTest, - BaseNetworkError(GenericErrorType.UNKNOWN, "fail"), - notSupported = notSupported, - ) - ) + private fun stubReadiness(readiness: SiteReadiness): MutableStateFlow { + val flow = MutableStateFlow(readiness) + whenever(siteProvisioningSource.stateFor(siteTest)).thenReturn(flow) + return flow } - private suspend fun stubMintSuccess() { - whenever(siteStore.createApplicationPassword(any())).thenReturn( - OnApplicationPasswordCreated( - siteTest, - ApplicationPasswordCredentials("user", "pass", uuid = "u") - ) - ) - } - - @Test - fun `given proper site, when api discovery is success, then add the application password card`() = runTest { - stubMintFailure() + private suspend fun stubAuthorized() = whenever(applicationPasswordLoginHelper.getAuthorizationUrlComplete(eq(TEST_URL))) - .thenReturn( - ApplicationPasswordLoginHelper.DiscoveryResult.Authorized("$TEST_URL_AUTH$TEST_URL_AUTH_SUFFIX") - ) - - applicationPasswordViewModelSlice.buildCard(siteTest) - - assertNotNull(applicationPasswordCard) - verify(applicationPasswordLoginHelper).getAuthorizationUrlComplete(eq(TEST_URL)) - } - - @Test - fun `given login scenario, when api discovery is empty, then show no card`() = runTest { - stubMintFailure() - whenever(applicationPasswordLoginHelper.getAuthorizationUrlComplete(eq(TEST_URL))) - .thenReturn(ApplicationPasswordLoginHelper.DiscoveryResult.Failed("test discovery failure")) - - applicationPasswordViewModelSlice.buildCard(siteTest) - - assertNull(applicationPasswordCard) - verify(applicationPasswordLoginHelper).getAuthorizationUrlComplete(eq(TEST_URL)) - } - - @Test - fun `given headless mint succeeds, then hide card and skip discovery`() = runTest { - stubMintSuccess() - - applicationPasswordViewModelSlice.buildCard(siteTest) - - assertNull(applicationPasswordCard) - verify(siteStore).createApplicationPassword(any()) - verify(applicationPasswordLoginHelper, never()).getAuthorizationUrlComplete(any()) - } - - @Test - fun `given headless mint succeeds, then re-probe editor capabilities for the minted site`() = runTest { - stubMintSuccess() - - applicationPasswordViewModelSlice.buildCard(siteTest) - - // The just-minted credentials live on this exact SiteModel instance, so the detector - // re-probes against it — no stale-SiteModel re-read, capabilities settle without a refresh. - verify(editorCapabilityDetector).refresh(siteTest) - } + .thenReturn(ApplicationPasswordLoginHelper.DiscoveryResult.Authorized(TEST_AUTH_URL)) @Test - fun `given headless mint succeeds, card hides without waiting for the recoverer`() = runTest { - stubMintSuccess() - val recoverGate = CompletableDeferred() - whenever(siteApiRestUrlRecoverer.discoverApiRootUrl(any())) - .doSuspendableAnswer { recoverGate.await(); null } - - applicationPasswordViewModelSlice.buildCard(siteTest) + fun `given unprovisionable without prior creds, then show the create card`() = test { + stubReadiness(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = false))) + stubAuthorized() - // Card has been hidden even though the recoverer is still suspended on the gate. - assertNull(applicationPasswordCard) - verify(siteApiRestUrlRecoverer).discoverApiRootUrl(siteTest.url) + slice.buildCard(siteTest) - // Release the recoverer so the test scope doesn't carry a dangling coroutine. - recoverGate.complete(Unit) + assertThat(card).isInstanceOf(MySiteCardAndItem.Card.QuickLinksItem::class.java) } @Test - fun `given valid stored creds, card hides without waiting for the recoverer`() = runTest { - whenever(applicationPasswordLoginHelper.siteHasBadCredentials(any())).thenReturn(false) - whenever(siteStore.sites).thenReturn( - listOf( - SiteModel().apply { - id = siteTest.id - url = TEST_URL - apiRestUsernamePlain = "user" - apiRestPasswordPlain = "password" - xmlRpcUrl = siteTest.xmlRpcUrl - } - ) - ) - whenever(applicationPasswordValidator.validate(any())) - .thenReturn(ApplicationPasswordValidator.Outcome.Valid) - val recoverGate = CompletableDeferred() - whenever(siteApiRestUrlRecoverer.discoverApiRootUrl(any())) - .doSuspendableAnswer { recoverGate.await(); null } + fun `given unprovisionable with prior creds, then show the reauthentication card`() = test { + stubReadiness(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = true))) + stubAuthorized() - applicationPasswordViewModelSlice.buildCard(siteTest) + slice.buildCard(siteTest) - assertNull(applicationPasswordCard) - verify(siteApiRestUrlRecoverer).discoverApiRootUrl(TEST_URL) - - recoverGate.complete(Unit) + val banner = card as MySiteCardAndItem.Item.SingleActionCard + assertThat(banner.textResource).isEqualTo(R.string.application_password_reauthentication_banner) } @Test - fun `given headless mint returns NotSupported, then fall back to discovery`() = runTest { - stubMintFailure(notSupported = true) + fun `given unprovisionable but discovery fails, then no card`() = test { + stubReadiness(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = false))) whenever(applicationPasswordLoginHelper.getAuthorizationUrlComplete(eq(TEST_URL))) - .thenReturn( - ApplicationPasswordLoginHelper.DiscoveryResult.Authorized("$TEST_URL_AUTH$TEST_URL_AUTH_SUFFIX") - ) + .thenReturn(ApplicationPasswordLoginHelper.DiscoveryResult.Failed("bad discovery")) - applicationPasswordViewModelSlice.buildCard(siteTest) + slice.buildCard(siteTest) - assertNotNull(applicationPasswordCard) - verify(siteStore).createApplicationPassword(any()) - verify(applicationPasswordLoginHelper).getAuthorizationUrlComplete(eq(TEST_URL)) + assertNull(card) } @Test - fun `given site already authenticated and validation succeeds, then show no card`() = runTest { - whenever(applicationPasswordLoginHelper.siteHasBadCredentials(any())).thenReturn(false) - whenever(siteStore.sites).thenReturn( - listOf( - SiteModel().apply { - id = siteTest.id - url = TEST_URL - apiRestUsernamePlain = "user" - apiRestPasswordPlain = "password" - xmlRpcUrl = siteTest.xmlRpcUrl - } - ) - ) - whenever(applicationPasswordValidator.validate(any())) - .thenReturn(ApplicationPasswordValidator.Outcome.Valid) + fun `given provisioning, then no card`() = test { + stubReadiness(SiteReadiness.NeedsAuth(SiteAuthState.Provisioning)) - applicationPasswordViewModelSlice.buildCard(siteTest) + slice.buildCard(siteTest) - assertNull(applicationPasswordCard) - verify(applicationPasswordValidator).validate(any()) - verify(siteStore, never()).createApplicationPassword(any()) - verify(applicationPasswordLoginHelper, times(0)).getAuthorizationUrlComplete(any()) + assertNull(card) } @Test - fun `given stored creds invalid, clear them and try headless mint`() = runTest { - whenever(applicationPasswordLoginHelper.siteHasBadCredentials(any())).thenReturn(false) - whenever(siteStore.sites).thenReturn( - listOf( - SiteModel().apply { - id = siteTest.id - url = TEST_URL - apiRestUsernamePlain = "stale-user" - apiRestPasswordPlain = "stale-pass" - xmlRpcUrl = siteTest.xmlRpcUrl - } - ) - ) - whenever(applicationPasswordValidator.validate(any())) - .thenReturn(ApplicationPasswordValidator.Outcome.Invalid) - stubMintSuccess() - - applicationPasswordViewModelSlice.buildCard(siteTest) + fun `given ready on a WPCom-REST site, then no card`() = test { + stubReadiness(SiteReadiness.Ready) - assertNull(applicationPasswordCard) - verify(siteStore).deleteStoredApplicationPasswordCredentials(any()) - // clearSelfHostedClient is invoked twice — once on invalidation, once after the fresh mint - verify(wpApiClientProvider, times(2)).clearSelfHostedClient(siteTest.id) - verify(siteStore).createApplicationPassword(any()) - } + slice.buildCard(siteTest) - @Test - fun `given stored creds invalid and mint fails, show reauthentication banner`() = runTest { - whenever(applicationPasswordLoginHelper.siteHasBadCredentials(any())).thenReturn(false) - whenever(siteStore.sites).thenReturn( - listOf( - SiteModel().apply { - id = siteTest.id - url = TEST_URL - apiRestUsernamePlain = "stale-user" - apiRestPasswordPlain = "stale-pass" - xmlRpcUrl = siteTest.xmlRpcUrl - } - ) - ) - whenever(applicationPasswordValidator.validate(any())) - .thenReturn(ApplicationPasswordValidator.Outcome.Invalid) - stubMintFailure(notSupported = true) - whenever(applicationPasswordLoginHelper.getAuthorizationUrlComplete(eq(TEST_URL))) - .thenReturn( - ApplicationPasswordLoginHelper.DiscoveryResult.Authorized("$TEST_URL_AUTH$TEST_URL_AUTH_SUFFIX") - ) - - applicationPasswordViewModelSlice.buildCard(siteTest) - - assertNotNull(applicationPasswordCard) - // Reauth banner uses SingleActionCard (not the QuickLinksItem create card) - assert(applicationPasswordCard is MySiteCardAndItem.Item.SingleActionCard) - } - - @Test - fun `given validation hits a network error, leave the card hidden without re-minting`() = runTest { - whenever(applicationPasswordLoginHelper.siteHasBadCredentials(any())).thenReturn(false) - whenever(siteStore.sites).thenReturn( - listOf( - SiteModel().apply { - id = siteTest.id - url = TEST_URL - apiRestUsernamePlain = "user" - apiRestPasswordPlain = "pass" - xmlRpcUrl = siteTest.xmlRpcUrl - } - ) - ) - whenever(applicationPasswordValidator.validate(any())) - .thenReturn(ApplicationPasswordValidator.Outcome.NetworkUnavailable) - - applicationPasswordViewModelSlice.buildCard(siteTest) - - assertNull(applicationPasswordCard) - verify(siteStore, never()).createApplicationPassword(any()) - verify(siteStore, never()).deleteStoredApplicationPasswordCredentials(any()) + assertNull(card) + verify(applicationPasswordLoginHelper, never()).getAuthorizationUrlComplete(any()) } @Test - fun `concurrent buildCard calls coalesce to a single mint`() = runTest { - // Gate the mint so the first buildCard suspends mid-call and the second arrives while it's - // still in flight. Without the single-flight guard, both calls would issue separate - // server-side mints and race the 409 conflict handler in ApplicationPasswordsManager. - val mintGate = CompletableDeferred() - whenever(siteStore.createApplicationPassword(any())).doSuspendableAnswer { - mintGate.await() - OnApplicationPasswordCreated( - siteTest, - ApplicationPasswordCredentials("user", "pass", uuid = "u") - ) + fun `given ready on a self-hosted site missing XML-RPC, then show the XML-RPC disabled card`() = test { + siteTest = SiteModel().apply { + id = TEST_SITE_ID + url = TEST_URL + // Not WP.com-REST and no XML-RPC endpoint — the one case the card still surfaces. } + stubReadiness(SiteReadiness.Ready) + // Let rediscovery fail so the card stays put for the assertion. + whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(TEST_URL)) + .thenThrow(mock()) - applicationPasswordViewModelSlice.buildCard(siteTest) - applicationPasswordViewModelSlice.buildCard(siteTest) - - mintGate.complete(Unit) - advanceUntilIdle() + slice.buildCard(siteTest) - verify(siteStore, times(1)).createApplicationPassword(any()) + val xmlRpcCard = card as MySiteCardAndItem.Item.SingleActionCard + assertThat(xmlRpcCard.textResource).isEqualTo(R.string.xmlrpc_disabled_card_text) } - - @Test - fun `given xmlRpc rediscovery and auth check succeed, then update site and dispatch`() = - runTest { - val xmlRpcUrl = "https://www.test.com/xmlrpc.php" - whenever( - selfHostedEndpointFinder - .verifyOrDiscoverXMLRPCEndpoint(TEST_URL) - ).thenReturn(xmlRpcUrl) - whenever( - siteXMLRPCClient.fetchSites( - eq(xmlRpcUrl), any(), any() - ) - ).thenReturn(SitesModel(listOf(SiteModel()))) - - applicationPasswordViewModelSlice - .attemptXmlRpcRediscovery(siteTest) - - verify(dispatcher).dispatch(any()) - assert(siteTest.xmlRpcUrl == xmlRpcUrl) - } - - @Test - fun `given xmlRpc rediscovery succeeds but auth check fails, then do not dispatch`() = - runTest { - siteTest.xmlRpcUrl = null - val xmlRpcUrl = "https://www.test.com/xmlrpc.php" - whenever( - selfHostedEndpointFinder - .verifyOrDiscoverXMLRPCEndpoint(TEST_URL) - ).thenReturn(xmlRpcUrl) - val errorResult = SitesModel().apply { - error = mock() - } - whenever( - siteXMLRPCClient.fetchSites( - eq(xmlRpcUrl), any(), any() - ) - ).thenReturn(errorResult) - - applicationPasswordViewModelSlice - .attemptXmlRpcRediscovery(siteTest) - - verify(dispatcher, never()).dispatch(any()) - assert(siteTest.xmlRpcUrl.isNullOrEmpty()) - } - - @Test - fun `given xmlRpc rediscovery fails, then do not dispatch`() = - runTest { - siteTest.xmlRpcUrl = null - whenever( - selfHostedEndpointFinder - .verifyOrDiscoverXMLRPCEndpoint(TEST_URL) - ).thenThrow( - mock() - ) - - applicationPasswordViewModelSlice - .attemptXmlRpcRediscovery(siteTest) - - verify(selfHostedEndpointFinder) - .verifyOrDiscoverXMLRPCEndpoint(TEST_URL) - verify(dispatcher, never()).dispatch(any()) - assert(siteTest.xmlRpcUrl.isNullOrEmpty()) - } } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSliceTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSliceTest.kt index 280d8135a4da..a1e9fa9b3d51 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSliceTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSliceTest.kt @@ -16,8 +16,9 @@ import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest import org.wordpress.android.R import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.repositories.EditorCapabilityDetectionState -import org.wordpress.android.repositories.EditorCapabilityDetector +import org.wordpress.android.repositories.SiteAuthState +import org.wordpress.android.repositories.SiteProvisioningSource +import org.wordpress.android.repositories.SiteReadiness import org.wordpress.android.ui.mysite.MySiteCardAndItem private const val TEST_SITE_LOCAL_ID = 42 @@ -26,7 +27,7 @@ private const val TEST_SITE_LOCAL_ID = 42 @RunWith(MockitoJUnitRunner::class) class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { @Mock - lateinit var editorCapabilityDetector: EditorCapabilityDetector + lateinit var siteProvisioningSource: SiteProvisioningSource private lateinit var siteTest: SiteModel private lateinit var slice: SiteConnectivityBannerViewModelSlice @@ -35,23 +36,23 @@ class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { @Before fun setUp() { siteTest = SiteModel().apply { id = TEST_SITE_LOCAL_ID } - slice = SiteConnectivityBannerViewModelSlice(editorCapabilityDetector) + slice = SiteConnectivityBannerViewModelSlice(siteProvisioningSource) slice.initialize(testScope()) slice.uiModel.observeForever { emittedBanners.add(it) } } - private fun stubState( + private fun stubReadiness( site: SiteModel, - state: EditorCapabilityDetectionState, - ): MutableStateFlow { - val flow = MutableStateFlow(state) - whenever(editorCapabilityDetector.stateFor(site)).thenReturn(flow) + readiness: SiteReadiness, + ): MutableStateFlow { + val flow = MutableStateFlow(readiness) + whenever(siteProvisioningSource.stateFor(site)).thenReturn(flow) return flow } @Test - fun `given detection unreachable, when fetchCapabilities invoked, then banner is shown`() = test { - stubState(siteTest, EditorCapabilityDetectionState.Unreachable) + fun `given unreachable, when fetchCapabilities invoked, then banner is shown`() = test { + stubReadiness(siteTest, SiteReadiness.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() @@ -62,8 +63,8 @@ class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { } @Test - fun `given detection ready, when fetchCapabilities invoked, then banner is null`() = test { - stubState(siteTest, EditorCapabilityDetectionState.Ready) + fun `given ready, when fetchCapabilities invoked, then banner is null`() = test { + stubReadiness(siteTest, SiteReadiness.Ready) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() @@ -72,64 +73,73 @@ class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { } @Test - fun `given detection pending, when fetchCapabilities invoked, then banner is null`() = test { - stubState(siteTest, EditorCapabilityDetectionState.Pending) + fun `given needs auth, when fetchCapabilities invoked, then banner is null`() = test { + // Credentials are the problem — the application-password card owns it, banner stays hidden. + stubReadiness(siteTest, SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = false))) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() - // Pending = probing or awaiting credentials — never a false "can't connect". assertThat(emittedBanners.last()).isNull() } @Test - fun `given detection transient error, when fetchCapabilities invoked, then banner is null`() = test { - stubState(siteTest, EditorCapabilityDetectionState.TransientError) + fun `given probing, when fetchCapabilities invoked, then banner is null`() = test { + stubReadiness(siteTest, SiteReadiness.Probing) + + slice.fetchCapabilities(siteTest, isUserInitiated = false) + advanceUntilIdle() + + assertThat(emittedBanners.last()).isNull() + } + + @Test + fun `given transient error, when fetchCapabilities invoked, then banner is null`() = test { + stubReadiness(siteTest, SiteReadiness.TransientError) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() - // Transient (e.g. offline) is covered by the global indicator — don't stack a warning. assertThat(emittedBanners.last()).isNull() } @Test fun `given unreachable then recovered to ready, when state changes, then banner clears`() = test { - val flow = stubState(siteTest, EditorCapabilityDetectionState.Unreachable) + val flow = stubReadiness(siteTest, SiteReadiness.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() assertThat(emittedBanners.last()).isNotNull - flow.value = EditorCapabilityDetectionState.Ready + flow.value = SiteReadiness.Ready advanceUntilIdle() assertThat(emittedBanners.last()).isNull() } @Test - fun `given user-initiated, when fetchCapabilities invoked, then detector is refreshed`() = test { - stubState(siteTest, EditorCapabilityDetectionState.Ready) + fun `given user-initiated, when fetchCapabilities invoked, then source is invalidated`() = test { + stubReadiness(siteTest, SiteReadiness.Ready) slice.fetchCapabilities(siteTest, isUserInitiated = true) advanceUntilIdle() - verify(editorCapabilityDetector).refresh(siteTest) + verify(siteProvisioningSource).invalidate(siteTest) } @Test - fun `given non-user-initiated, when fetchCapabilities invoked, then detector is not refreshed`() = test { - stubState(siteTest, EditorCapabilityDetectionState.Ready) + fun `given non-user-initiated, when fetchCapabilities invoked, then source is not invalidated`() = test { + stubReadiness(siteTest, SiteReadiness.Ready) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() - verify(editorCapabilityDetector, never()).refresh(any()) + verify(siteProvisioningSource, never()).invalidate(any()) } @Test - fun `given banner showing, when retry tapped, then detector is refreshed`() = test { - stubState(siteTest, EditorCapabilityDetectionState.Unreachable) + fun `given banner showing, when retry tapped, then source is invalidated`() = test { + stubReadiness(siteTest, SiteReadiness.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() @@ -138,12 +148,12 @@ class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { banner.onActionClick() advanceUntilIdle() - verify(editorCapabilityDetector).refresh(siteTest) + verify(siteProvisioningSource).invalidate(siteTest) } @Test fun `when clearBanner invoked, then banner is null`() = test { - stubState(siteTest, EditorCapabilityDetectionState.Unreachable) + stubReadiness(siteTest, SiteReadiness.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() @@ -155,8 +165,8 @@ class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { } @Test - fun `given banner cleared, when retry tapped, then no refresh runs`() = test { - stubState(siteTest, EditorCapabilityDetectionState.Unreachable) + fun `given banner cleared, when retry tapped, then no invalidate runs`() = test { + stubReadiness(siteTest, SiteReadiness.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() @@ -164,26 +174,24 @@ class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { slice.clearBanner() advanceUntilIdle() - // Simulate a tap that landed before LiveData propagated the null clear. banner.onActionClick() advanceUntilIdle() - verify(editorCapabilityDetector, never()).refresh(any()) + verify(siteProvisioningSource, never()).invalidate(any()) } @Test fun `given site switched, when old site becomes unreachable, then banner ignores it`() = test { val siteB = SiteModel().apply { id = TEST_SITE_LOCAL_ID + 1 } - val flowA = stubState(siteTest, EditorCapabilityDetectionState.Ready) - stubState(siteB, EditorCapabilityDetectionState.Ready) + val flowA = stubReadiness(siteTest, SiteReadiness.Ready) + stubReadiness(siteB, SiteReadiness.Ready) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() slice.fetchCapabilities(siteB, isUserInitiated = false) advanceUntilIdle() - // Site A's probe resolves to Unreachable after we've switched to B — must not surface. - flowA.value = EditorCapabilityDetectionState.Unreachable + flowA.value = SiteReadiness.Unreachable advanceUntilIdle() assertThat( diff --git a/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt index 40a1895ef3ac..17ec4557b451 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt @@ -19,8 +19,8 @@ import org.wordpress.android.BaseUnitTest import org.wordpress.android.datasets.SiteSettingsProvider import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.store.AccountStore -import org.wordpress.android.repositories.EditorCapabilityDetector -import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer +import org.wordpress.android.fluxc.store.SiteStore +import org.wordpress.android.repositories.SiteProvisioningSource import org.wordpress.gutenberg.model.EditorAssetBundle import org.wordpress.gutenberg.model.EditorConfiguration import org.wordpress.gutenberg.model.EditorDependencies @@ -49,10 +49,10 @@ class GutenbergEditorPreloaderTest : lateinit var editorServiceProvider: EditorServiceProvider @Mock - lateinit var editorCapabilityDetector: EditorCapabilityDetector + lateinit var siteProvisioningSource: SiteProvisioningSource @Mock - lateinit var siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer + lateinit var siteStore: SiteStore private val editorDependencies = EditorDependencies.empty @@ -75,8 +75,8 @@ class GutenbergEditorPreloaderTest : gutenbergKitSettingsBuilder = gutenbergKitSettingsBuilder, siteSettingsProvider = siteSettingsProvider, editorServiceProvider = editorServiceProvider, - editorCapabilityDetector = editorCapabilityDetector, - siteApiRestUrlRecoverer = siteApiRestUrlRecoverer, + siteProvisioningSource = siteProvisioningSource, + siteStore = siteStore, bgDispatcher = testDispatcher() ) } @@ -194,7 +194,7 @@ class GutenbergEditorPreloaderTest : } @Test - fun `successful preload detects editor capabilities via the detector`() = test { + fun `successful preload runs the provisioning source`() = test { val site = createSite() enablePreloading(site) stubSuccessfulPreload() @@ -203,7 +203,7 @@ class GutenbergEditorPreloaderTest : preloader.preloadIfNeeded(site, this) advanceUntilIdle() - verify(editorCapabilityDetector).awaitProbe(site) + verify(siteProvisioningSource).await(site) } @Test @@ -482,21 +482,4 @@ class GutenbergEditorPreloaderTest : // endregion - // region wpApiRestUrl recovery - - @Test - fun `successful preload invokes discovery only — slice owns persistence`() = test { - val site = createSite() - enablePreloading(site) - stubSuccessfulPreload() - stubEditorService() - - preloader.preloadIfNeeded(site, this) - advanceUntilIdle() - - verify(siteApiRestUrlRecoverer).discoverApiRootUrl(site.url) - verify(siteApiRestUrlRecoverer, never()).persistApiRootUrl(any(), any()) - } - - // endregion } From e688348c685887b8e2c33bbf7143bfc563f8b01a Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Thu, 4 Jun 2026 14:00:15 -0600 Subject: [PATCH 05/12] Read fresh / write targeted per stage; run XML-RPC recovery in parallel Removes the threaded, mutated SiteModel from the provisioning pipeline: each stage now reads the site fresh by local id and writes back only the column it changed (persistApiRootUrl / a new persistXmlRpcUrl), so the parallel branches can't clobber one another and there's no stale-model write (#22905). With writes targeted, XML-RPC endpoint recovery moves out of the application-password card into the pipeline as a parallel branch off ensureAuth -- independent of the REST capability probe, since it needs only the credentials. The card becomes a pure renderer that reads the recovered xmlRpcUrl fresh. Adds SiteSqlUtils.updateXmlRpcUrl and a SiteXmlRpcUrlRecoverer mirroring SiteApiRestUrlRecoverer. --- .../repositories/SiteProvisioningSource.kt | 121 +++++++++++------- .../accounts/login/SiteXmlRpcUrlRecoverer.kt | 64 +++++++++ .../ApplicationPasswordViewModelSlice.kt | 68 ++-------- .../SiteProvisioningSourceTest.kt | 44 ++++++- .../login/SiteXmlRpcUrlRecovererTest.kt | 96 ++++++++++++++ .../ApplicationPasswordViewModelSliceTest.kt | 30 ++--- 6 files changed, 290 insertions(+), 133 deletions(-) create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecoverer.kt create mode 100644 WordPress/src/test/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecovererTest.kt diff --git a/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt index e5c8ad7792fd..1e1dbab82863 100644 --- a/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt +++ b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt @@ -2,6 +2,8 @@ package org.wordpress.android.repositories import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job +import kotlinx.coroutines.async +import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.launch @@ -12,6 +14,7 @@ import org.wordpress.android.fluxc.utils.AppLogWrapper import org.wordpress.android.modules.APPLICATION_SCOPE import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer +import org.wordpress.android.ui.accounts.login.SiteXmlRpcUrlRecoverer import org.wordpress.android.ui.mysite.cards.applicationpassword.ApplicationPasswordValidator import org.wordpress.android.util.AppLog import org.wordpress.android.util.NetworkUtilsWrapper @@ -22,28 +25,35 @@ import javax.inject.Singleton /** * The single source of truth for getting a site ready to use: it provisions - * application-password credentials, recovers the REST API root, and detects - * editor capabilities — **in that order, each stage awaited before the next**. + * application-password credentials, recovers the REST API root, recovers the + * XML-RPC endpoint (self-hosted), and detects editor capabilities. * - * Those three things have a genuine ordering dependency (you can't probe the - * REST API of a private Atomic host until you've minted a credential for it), - * and they used to be spread across the connectivity banner, the editor - * preloader, and the application-password card, each triggering its slice of - * the work independently and racing the others. Running them as one serialized - * per-site pipeline makes the race structurally impossible: [detectCapabilities] - * is downstream of [ensureAuth], so the probe can never run before the mint. + * Capability detection can't run until a credential exists (you can't probe a + * private Atomic host's REST API unauthenticated), so **auth is awaited first**; + * after that, the REST-capability branch and the XML-RPC branch are independent + * and run **in parallel**. Routing every consumer (connectivity banner, editor + * preloader, application-password card) through this one pipeline means the + * first-login race is structurally impossible — the probe is downstream of the + * mint — and there's one shared, deduplicated run per site instead of each + * consumer racing the others. + * + * ### No model is held across stages + * Stages take a **`siteLocalId`**, read the `SiteModel` fresh from the store at + * the point of use, and write back **only the one column they changed** + * (`persistApiRootUrl` / `persistXmlRpcUrl`). Nothing keeps a mutated `SiteModel` + * around, so the two parallel branches can't clobber each other and there's no + * stale-model write (see #22905). The passed `SiteModel` is used for its id only. * * Per site there is at most one in-flight pipeline (single-flight, keyed by - * [SiteModel.id]); concurrent callers join it rather than starting a second — - * which also subsumes the application-password card's old single-flight guard - * (two concurrent mints hit a 409 that destroys the winner's credentials). + * [SiteModel.id]); concurrent callers join it — which also subsumes the + * application-password card's old single-flight guard (two concurrent mints hit + * a 409 that destroys the winner's credentials). * * ## Entry points - * - [stateFor] — reactive: returns a shared [StateFlow]; the first access runs - * the pipeline, later accesses reuse a [SiteReadiness.Ready] result. + * - [stateFor] — reactive: returns a shared [StateFlow]; first access runs the + * pipeline, later accesses reuse a [SiteReadiness.Ready] result. * - [await] — one-shot: runs the pipeline (if needed) and returns the result. - * - [invalidate] — forces a re-run, bypassing the once-per-site gate - * (pull-to-refresh, retry). + * - [invalidate] — forces a re-run (pull-to-refresh, retry). * - [clear] — cancels all work and drops all state; wire into sign-out. */ @Singleton @@ -54,6 +64,7 @@ class SiteProvisioningSource @Inject constructor( private val applicationPasswordValidator: ApplicationPasswordValidator, private val wpApiClientProvider: WpApiClientProvider, private val siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer, + private val siteXmlRpcUrlRecoverer: SiteXmlRpcUrlRecoverer, private val editorSettingsRepository: EditorSettingsRepository, private val networkUtilsWrapper: NetworkUtilsWrapper, private val appLogWrapper: AppLogWrapper, @@ -64,18 +75,18 @@ class SiteProvisioningSource @Inject constructor( // Sites whose pipeline reached Ready this process — the dedup gate. Only a fully-ready site // latches; auth-needed / unreachable / transient outcomes are left to re-run on the next - // access (so a later resume re-mints or re-probes). Reset by invalidate / clear. + // access. Reset by invalidate / clear. private val ready = ConcurrentHashMap.newKeySet() /** * The shared readiness state for [site]. The first call starts the pipeline; * later calls return the same flow without re-running once it reached - * [SiteReadiness.Ready]. Collect it to react to provisioning / capability changes. + * [SiteReadiness.Ready]. Only [SiteModel.id] is read from [site]. */ @Synchronized fun stateFor(site: SiteModel): StateFlow { val flow = flowFor(site.id) - if (shouldRun(site.id)) launchPipeline(site) + if (shouldRun(site.id)) launchPipeline(site.id) return flow } @@ -98,7 +109,7 @@ class SiteProvisioningSource @Inject constructor( fun invalidate(site: SiteModel) { if (jobs[site.id]?.isActive == true) return ready.remove(site.id) - launchPipeline(site) + launchPipeline(site.id) } /** Cancels all in-flight pipelines and drops all cached state (sign-out). */ @@ -111,13 +122,13 @@ class SiteProvisioningSource @Inject constructor( } @Synchronized - private fun launchPipeline(site: SiteModel) { - jobs[site.id]?.cancel() - val flow = flowFor(site.id) - jobs[site.id] = appScope.launch { - val readiness = runPipeline(site) + private fun launchPipeline(siteLocalId: Int) { + jobs[siteLocalId]?.cancel() + val flow = flowFor(siteLocalId) + jobs[siteLocalId] = appScope.launch { + val readiness = runPipeline(siteLocalId) flow.value = readiness - if (readiness is SiteReadiness.Ready) ready.add(site.id) + if (readiness is SiteReadiness.Ready) ready.add(siteLocalId) } } @@ -127,39 +138,40 @@ class SiteProvisioningSource @Inject constructor( private fun shouldRun(siteLocalId: Int): Boolean = jobs[siteLocalId]?.isActive != true && siteLocalId !in ready - private suspend fun runPipeline(site: SiteModel): SiteReadiness { - // Re-read from the store so we provision against the persisted SiteModel and mutate that - // instance in place — later stages (URL recovery, capability probe) see the fresh creds. - val storedSite = siteStore.sites.firstOrNull { it.id == site.id } ?: site - return when (val auth = ensureAuth(storedSite)) { - SiteAuthState.Provisioned -> { - recoverRestUrl(storedSite) - detectCapabilities(storedSite) + private suspend fun runPipeline(siteLocalId: Int): SiteReadiness = + when (val auth = ensureAuth(siteLocalId)) { + SiteAuthState.Provisioned -> coroutineScope { + // Post-auth, the REST-capability chain and the XML-RPC recovery are independent — + // each reads the site fresh and writes only its own column — so run them in + // parallel. recoverRestUrl precedes detectCapabilities within its branch because + // the probe needs the recovered REST root. + val capabilities = async { recoverRestUrl(siteLocalId); detectCapabilities(siteLocalId) } + val xmlRpc = async { recoverXmlRpc(siteLocalId) } + xmlRpc.await() + capabilities.await() } else -> SiteReadiness.NeedsAuth(auth) } - } /** * Stage 1 — ensure the site has working application-password credentials. * Validates stored creds with Basic auth against the direct host; on a * confirmed rejection wipes them and mints fresh ones via the FluxC Jetpack - * tunnel (the only path that works for Atomic / Jetpack-WPCom-REST sites). + * tunnel. The mint persists the credentials, so later stages read them back. */ - private suspend fun ensureAuth(site: SiteModel): SiteAuthState { + private suspend fun ensureAuth(siteLocalId: Int): SiteAuthState { + val site = siteStore.getSiteByLocalId(siteLocalId) + ?: return SiteAuthState.Unprovisionable(hadCredentials = false) val hadCredentials = !applicationPasswordLoginHelper.siteHasBadCredentials(site) if (hadCredentials) { when (applicationPasswordValidator.validate(site)) { ApplicationPasswordValidator.Outcome.Valid -> return SiteAuthState.Provisioned ApplicationPasswordValidator.Outcome.NetworkUnavailable -> { - // Don't punish flaky networks — treat as in-progress and retry next run. appLogWrapper.d(AppLog.T.MAIN, "A_P: Validation network error for ${site.url}") return SiteAuthState.Provisioning } ApplicationPasswordValidator.Outcome.Invalid -> { - // Stored creds are stale (revoked, deleted) — clear them so the mint below - // creates fresh ones, and invalidate the cached client. appLogWrapper.d(AppLog.T.MAIN, "A_P: Stored creds invalid for ${site.url}, clearing") siteStore.deleteStoredApplicationPasswordCredentials(site) wpApiClientProvider.clearSelfHostedClient(site.id) @@ -180,15 +192,29 @@ class SiteProvisioningSource @Inject constructor( } /** - * Stage 2 — recover the REST API root for Atomic sites minted through the - * Jetpack tunnel, which never runs discovery and so leaves `wpApiRestUrl` - * null. One owner for the heal that the card and preloader used to duplicate. + * Stage 2a — recover the REST API root for Atomic sites minted through the + * Jetpack tunnel (which never runs discovery and leaves `wpApiRestUrl` null). + * Persists the one column; the capability probe re-reads it. */ - private suspend fun recoverRestUrl(site: SiteModel) { + private suspend fun recoverRestUrl(siteLocalId: Int) { + val site = siteStore.getSiteByLocalId(siteLocalId) ?: return if (!site.wpApiRestUrl.isNullOrEmpty()) return siteApiRestUrlRecoverer.discoverApiRootUrl(site.url)?.let { apiRootUrl -> - site.wpApiRestUrl = apiRootUrl - siteApiRestUrlRecoverer.persistApiRootUrl(site.id, apiRootUrl) + siteApiRestUrlRecoverer.persistApiRootUrl(siteLocalId, apiRootUrl) + } + } + + /** + * Stage 2b (parallel) — recover the XML-RPC endpoint for true self-hosted + * sites that don't have one. Discovers + authenticates against it, and on + * success persists the one column; the application-password card re-reads it. + */ + private suspend fun recoverXmlRpc(siteLocalId: Int) { + val site = siteStore.getSiteByLocalId(siteLocalId) ?: return + // WP.com / Atomic / Jetpack-WPCom-REST sites talk REST end-to-end and don't use XML-RPC. + if (site.isUsingWpComRestApi || !site.xmlRpcUrl.isNullOrEmpty()) return + siteXmlRpcUrlRecoverer.discoverAndVerifyXmlRpcUrl(site)?.let { endpoint -> + siteXmlRpcUrlRecoverer.persistXmlRpcUrl(siteLocalId, endpoint) } } @@ -198,7 +224,8 @@ class SiteProvisioningSource @Inject constructor( * guaranteed present: a failure here is a real transport problem, not a * pending mint. */ - private suspend fun detectCapabilities(site: SiteModel): SiteReadiness { + private suspend fun detectCapabilities(siteLocalId: Int): SiteReadiness { + val site = siteStore.getSiteByLocalId(siteLocalId) ?: return SiteReadiness.Unreachable val ok = editorSettingsRepository.fetchEditorCapabilitiesForSite(site) val hasCache = editorSettingsRepository.hasCachedCapabilities(site) return when { diff --git a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecoverer.kt b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecoverer.kt new file mode 100644 index 000000000000..b44dbefcf4e7 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecoverer.kt @@ -0,0 +1,64 @@ +package org.wordpress.android.ui.accounts.login + +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.withContext +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.fluxc.network.discovery.SelfHostedEndpointFinder +import org.wordpress.android.fluxc.network.xmlrpc.site.SiteXMLRPCClient +import org.wordpress.android.fluxc.persistence.SiteSqlUtils +import org.wordpress.android.fluxc.utils.AppLogWrapper +import org.wordpress.android.modules.BG_THREAD +import org.wordpress.android.util.AppLog +import javax.inject.Inject +import javax.inject.Named +import javax.inject.Singleton + +/** + * Heals [SiteModel.xmlRpcUrl] for true self-hosted sites whose XML-RPC endpoint was never + * discovered (or was previously gated). The REST counterpart is [SiteApiRestUrlRecoverer]; this + * follows the same shape so callers never hold a mutated [SiteModel]: + * + * - [discoverAndVerifyXmlRpcUrl] discovers the endpoint and confirms it works with an authenticated + * call, returning the verified URL (or `null` if discovery/verification fails). + * - [persistXmlRpcUrl] writes only that one column to the DB row for `localId`. + */ +@Singleton +class SiteXmlRpcUrlRecoverer @Inject constructor( + private val selfHostedEndpointFinder: SelfHostedEndpointFinder, + private val siteXMLRPCClient: SiteXMLRPCClient, + private val siteSqlUtils: SiteSqlUtils, + private val appLogWrapper: AppLogWrapper, + @param:Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher, +) { + @Suppress("SwallowedException") + suspend fun discoverAndVerifyXmlRpcUrl(site: SiteModel): String? = withContext(bgDispatcher) { + try { + val endpoint = selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url) + val result = siteXMLRPCClient.fetchSites( + endpoint, + site.apiRestUsernamePlain, + site.apiRestPasswordPlain, + ) + if (result.isError) { + appLogWrapper.w(AppLog.T.API, "XML-RPC verification failed for ${site.url}") + null + } else { + endpoint + } + } catch (e: SelfHostedEndpointFinder.DiscoveryException) { + appLogWrapper.w(AppLog.T.API, "XML-RPC discovery failed for ${site.url}") + null + } + } + + suspend fun persistXmlRpcUrl(localId: Int, xmlRpcUrl: String): Boolean = withContext(bgDispatcher) { + val rowsUpdated = siteSqlUtils.updateXmlRpcUrl(localId, xmlRpcUrl) + if (rowsUpdated == 0) { + appLogWrapper.w(AppLog.T.API, "Cannot persist xmlRpcUrl: no site with localId=$localId") + false + } else { + appLogWrapper.d(AppLog.T.API, "Persisted xmlRpcUrl=$xmlRpcUrl for localId=$localId") + true + } + } +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt index 226f93355c84..38545070abac 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt @@ -2,18 +2,11 @@ package org.wordpress.android.ui.mysite.cards.applicationpassword import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData -import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.launch -import kotlinx.coroutines.withContext import org.wordpress.android.R -import androidx.annotation.VisibleForTesting -import org.wordpress.android.fluxc.Dispatcher -import org.wordpress.android.fluxc.generated.SiteActionBuilder import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.fluxc.network.discovery.SelfHostedEndpointFinder -import org.wordpress.android.fluxc.network.xmlrpc.site.SiteXMLRPCClient import org.wordpress.android.fluxc.store.SiteStore import org.wordpress.android.fluxc.utils.AppLogWrapper import org.wordpress.android.repositories.SiteAuthState @@ -27,31 +20,26 @@ import org.wordpress.android.ui.pages.SnackbarMessageHolder import org.wordpress.android.ui.utils.ListItemInteraction import org.wordpress.android.ui.utils.UiString.UiStringRes import org.wordpress.android.util.AppLog -import org.wordpress.android.modules.IO_THREAD import org.wordpress.android.viewmodel.Event import javax.inject.Inject -import javax.inject.Named /** - * Renders the application-password card from the site's readiness. Credential - * provisioning (validate / mint) now lives in [SiteProvisioningSource]; this - * slice is a view over the auth slice of that state: + * Renders the application-password card from the site's readiness. All the + * provisioning mechanics — validate, mint, REST-root recovery, XML-RPC + * recovery — live in [SiteProvisioningSource]; this slice is a thin view over + * the result: * * - [SiteAuthState.Unprovisionable] → a re-authentication banner (creds went * bad) or a first-time "authenticate" card, looked up lazily. * - provisioned (any non-[SiteReadiness.NeedsAuth] terminal state) → hidden, - * except true self-hosted sites missing an XML-RPC endpoint, which get the - * XML-RPC-disabled card plus a background rediscovery attempt. + * except true self-hosted sites whose XML-RPC endpoint the pipeline couldn't + * recover, which get the XML-RPC-disabled card. */ class ApplicationPasswordViewModelSlice @Inject constructor( private val applicationPasswordLoginHelper: ApplicationPasswordLoginHelper, private val siteStore: SiteStore, private val appLogWrapper: AppLogWrapper, - private val selfHostedEndpointFinder: SelfHostedEndpointFinder, - private val siteXMLRPCClient: SiteXMLRPCClient, private val siteProvisioningSource: SiteProvisioningSource, - private val dispatcher: Dispatcher, - @Named(IO_THREAD) private val ioDispatcher: CoroutineDispatcher, ) { lateinit var scope: CoroutineScope @@ -107,13 +95,11 @@ class ApplicationPasswordViewModelSlice @Inject constructor( } private fun handleProvisioned(site: SiteModel) { - // Re-read the stored site so we see credentials/endpoints the pipeline just persisted. - val storedSite = siteStore.sites.firstOrNull { it.id == site.id } ?: site - // Only true self-hosted sites need the XML-RPC fallback path — Atomic and Jetpack-WPCom-REST - // sites talk REST end-to-end and don't need XML-RPC. + // Read fresh: the pipeline's parallel XML-RPC branch may have just recovered the endpoint. + val storedSite = siteStore.getSiteByLocalId(site.id) ?: site + // Only true self-hosted sites need XML-RPC; if the pipeline couldn't recover it, surface it. if (!storedSite.isUsingWpComRestApi && storedSite.xmlRpcUrl.isNullOrEmpty()) { buildXmlRpcDisabledCard(storedSite) - attemptXmlRpcRediscovery(storedSite) } else { uiModelMutable.postValue(null) appLogWrapper.d(AppLog.T.MAIN, "A_P: Hiding card for ${site.url} - authenticated") @@ -197,42 +183,6 @@ class ApplicationPasswordViewModelSlice @Inject constructor( ) } - @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) - internal fun attemptXmlRpcRediscovery(site: SiteModel) { - scope.launch { - try { - val xmlRpcEndpoint = withContext(ioDispatcher) { - selfHostedEndpointFinder - .verifyOrDiscoverXMLRPCEndpoint(site.url) - } - - // Verify with an authenticated call - val result = withContext(ioDispatcher) { - siteXMLRPCClient.fetchSites( - xmlRpcEndpoint, - site.apiRestUsernamePlain, - site.apiRestPasswordPlain - ) - } - if (result.isError) { - return@launch - } - - site.xmlRpcUrl = xmlRpcEndpoint - dispatcher.dispatch( - SiteActionBuilder.newUpdateSiteAction(site) - ) - // Endpoint recovered — hide the XML-RPC-disabled card. - uiModelMutable.postValue(null) - } catch ( - @Suppress("SwallowedException") - e: SelfHostedEndpointFinder.DiscoveryException - ) { - // XML-RPC rediscovery failed; card remains visible - } - } - } - private fun onClick(site: SiteModel, alternativeUrl: String) { _onNavigation.postValue( Event( diff --git a/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt b/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt index a695ed3b6582..0c5fae5796de 100644 --- a/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt @@ -23,6 +23,7 @@ import org.wordpress.android.fluxc.store.SiteStore.OnApplicationPasswordCreated import org.wordpress.android.fluxc.utils.AppLogWrapper import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer +import org.wordpress.android.ui.accounts.login.SiteXmlRpcUrlRecoverer import org.wordpress.android.ui.mysite.cards.applicationpassword.ApplicationPasswordValidator import org.wordpress.android.util.NetworkUtilsWrapper @@ -35,6 +36,7 @@ class SiteProvisioningSourceTest : BaseUnitTest(StandardTestDispatcher()) { @Mock lateinit var applicationPasswordValidator: ApplicationPasswordValidator @Mock lateinit var wpApiClientProvider: WpApiClientProvider @Mock lateinit var siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer + @Mock lateinit var siteXmlRpcUrlRecoverer: SiteXmlRpcUrlRecoverer @Mock lateinit var editorSettingsRepository: EditorSettingsRepository @Mock lateinit var networkUtilsWrapper: NetworkUtilsWrapper @Mock lateinit var appLogWrapper: AppLogWrapper @@ -47,15 +49,19 @@ class SiteProvisioningSourceTest : BaseUnitTest(StandardTestDispatcher()) { site = SiteModel().apply { id = TEST_SITE_LOCAL_ID url = "https://test.example.com" - // A non-null REST root so recoverRestUrl short-circuits unless a test clears it. + // Non-null REST root + XML-RPC url so both recovery branches short-circuit unless a + // test clears them — keeping the auth/capability tests focused. wpApiRestUrl = "https://test.example.com/wp-json" + xmlRpcUrl = "https://test.example.com/xmlrpc.php" } + whenever(siteStore.getSiteByLocalId(TEST_SITE_LOCAL_ID)).thenReturn(site) source = SiteProvisioningSource( siteStore, applicationPasswordLoginHelper, applicationPasswordValidator, wpApiClientProvider, siteApiRestUrlRecoverer, + siteXmlRpcUrlRecoverer, editorSettingsRepository, networkUtilsWrapper, appLogWrapper, @@ -81,8 +87,7 @@ class SiteProvisioningSourceTest : BaseUnitTest(StandardTestDispatcher()) { private suspend fun stubCapabilityProbe(ok: Boolean, cached: Boolean = false) { whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(any())).thenReturn(ok) - // `ok || hasCache` short-circuits, so the cache is only read (and only needs stubbing) - // when the live probe failed — stubbing it on success would be an unnecessary stub. + // `ok || hasCache` short-circuits, so only stub the cache when the live probe failed. if (!ok) whenever(editorSettingsRepository.hasCachedCapabilities(any())).thenReturn(cached) } @@ -116,7 +121,6 @@ class SiteProvisioningSourceTest : BaseUnitTest(StandardTestDispatcher()) { val result = source.await(site) assertThat(result).isEqualTo(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = false))) - // Auth failed — the capability probe must not run. verify(editorSettingsRepository, never()).fetchEditorCapabilitiesForSite(any()) } @@ -144,9 +148,18 @@ class SiteProvisioningSourceTest : BaseUnitTest(StandardTestDispatcher()) { verify(editorSettingsRepository, never()).fetchEditorCapabilitiesForSite(any()) } + @Test + fun `given the site is gone from the store, then unprovisionable`() = test { + whenever(siteStore.getSiteByLocalId(TEST_SITE_LOCAL_ID)).thenReturn(null) + + val result = source.await(site) + + assertThat(result).isEqualTo(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = false))) + } + // endregion - // region url recovery + capability stages + // region recovery + capability stages @Test fun `given provisioned with a missing REST root, then it recovers and persists the url`() = test { @@ -162,6 +175,27 @@ class SiteProvisioningSourceTest : BaseUnitTest(StandardTestDispatcher()) { verify(siteApiRestUrlRecoverer).persistApiRootUrl(eq(site.id), eq("https://test.example.com/custom-rest")) } + @Test + fun `given a provisioned self-hosted site without XML-RPC, then it recovers and persists xmlRpcUrl`() = test { + val selfHosted = SiteModel().apply { + id = TEST_SITE_LOCAL_ID + url = "https://selfhosted.example.com" + wpApiRestUrl = "https://selfhosted.example.com/wp-json" // REST branch short-circuits + // not WP.com and no xmlRpcUrl → the XML-RPC branch runs + } + whenever(siteStore.getSiteByLocalId(TEST_SITE_LOCAL_ID)).thenReturn(selfHosted) + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + whenever(siteXmlRpcUrlRecoverer.discoverAndVerifyXmlRpcUrl(selfHosted)) + .thenReturn("https://selfhosted.example.com/xmlrpc.php") + + source.await(selfHosted) + + verify(siteXmlRpcUrlRecoverer) + .persistXmlRpcUrl(eq(TEST_SITE_LOCAL_ID), eq("https://selfhosted.example.com/xmlrpc.php")) + } + @Test fun `given probe fails while offline, then transient error`() = test { stubHasStoredCredentials(true) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecovererTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecovererTest.kt new file mode 100644 index 000000000000..d895c67265ab --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecovererTest.kt @@ -0,0 +1,96 @@ +package org.wordpress.android.ui.accounts.login + +import kotlinx.coroutines.ExperimentalCoroutinesApi +import org.assertj.core.api.Assertions.assertThat +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mock +import org.mockito.junit.MockitoJUnitRunner +import org.mockito.kotlin.any +import org.mockito.kotlin.eq +import org.mockito.kotlin.mock +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever +import org.wordpress.android.BaseUnitTest +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.fluxc.model.SitesModel +import org.wordpress.android.fluxc.network.BaseRequest.BaseNetworkError +import org.wordpress.android.fluxc.network.BaseRequest.GenericErrorType +import org.wordpress.android.fluxc.network.discovery.SelfHostedEndpointFinder +import org.wordpress.android.fluxc.network.xmlrpc.site.SiteXMLRPCClient +import org.wordpress.android.fluxc.persistence.SiteSqlUtils +import org.wordpress.android.fluxc.utils.AppLogWrapper + +private const val ENDPOINT = "https://selfhosted.example.com/xmlrpc.php" +private const val SITE_LOCAL_ID = 5 + +@ExperimentalCoroutinesApi +@RunWith(MockitoJUnitRunner::class) +class SiteXmlRpcUrlRecovererTest : BaseUnitTest() { + @Mock lateinit var selfHostedEndpointFinder: SelfHostedEndpointFinder + @Mock lateinit var siteXMLRPCClient: SiteXMLRPCClient + @Mock lateinit var siteSqlUtils: SiteSqlUtils + @Mock lateinit var appLogWrapper: AppLogWrapper + + private lateinit var site: SiteModel + private lateinit var recoverer: SiteXmlRpcUrlRecoverer + + @Before + fun setUp() { + site = SiteModel().apply { + id = SITE_LOCAL_ID + url = "https://selfhosted.example.com" + apiRestUsernamePlain = "user" + apiRestPasswordPlain = "pass" + } + recoverer = SiteXmlRpcUrlRecoverer( + selfHostedEndpointFinder, + siteXMLRPCClient, + siteSqlUtils, + appLogWrapper, + testDispatcher(), + ) + } + + @Test + fun `given discovery and authenticated verify succeed, then returns the endpoint`() = test { + whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url)).thenReturn(ENDPOINT) + whenever(siteXMLRPCClient.fetchSites(eq(ENDPOINT), any(), any())) + .thenReturn(SitesModel(listOf(SiteModel()))) + + assertThat(recoverer.discoverAndVerifyXmlRpcUrl(site)).isEqualTo(ENDPOINT) + } + + @Test + fun `given discovery throws, then returns null`() = test { + whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url)) + .thenThrow(mock()) + + assertThat(recoverer.discoverAndVerifyXmlRpcUrl(site)).isNull() + } + + @Test + fun `given the authenticated verify errors, then returns null`() = test { + whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url)).thenReturn(ENDPOINT) + whenever(siteXMLRPCClient.fetchSites(eq(ENDPOINT), any(), any())) + .thenReturn(SitesModel().apply { error = BaseNetworkError(GenericErrorType.UNKNOWN, "x") }) + + assertThat(recoverer.discoverAndVerifyXmlRpcUrl(site)).isNull() + } + + @Test + fun `given persist updates a row, then returns true`() = test { + whenever(siteSqlUtils.updateXmlRpcUrl(eq(SITE_LOCAL_ID), eq(ENDPOINT))).thenReturn(1) + + assertThat(recoverer.persistXmlRpcUrl(SITE_LOCAL_ID, ENDPOINT)).isTrue + verify(siteSqlUtils).updateXmlRpcUrl(eq(SITE_LOCAL_ID), eq(ENDPOINT)) + } + + @Test + fun `given persist matches no row, then returns false`() = test { + whenever(siteSqlUtils.updateXmlRpcUrl(eq(SITE_LOCAL_ID), eq(ENDPOINT))).thenReturn(0) + + assertThat(recoverer.persistXmlRpcUrl(SITE_LOCAL_ID, ENDPOINT)).isFalse + } +} diff --git a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt index 58b4ddc425ad..090966a4b3e1 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt @@ -12,16 +12,12 @@ import org.mockito.MockitoAnnotations import org.mockito.junit.MockitoJUnitRunner import org.mockito.kotlin.any import org.mockito.kotlin.eq -import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest import org.wordpress.android.R -import org.wordpress.android.fluxc.Dispatcher import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.fluxc.network.discovery.SelfHostedEndpointFinder -import org.wordpress.android.fluxc.network.xmlrpc.site.SiteXMLRPCClient import org.wordpress.android.fluxc.store.SiteStore import org.wordpress.android.fluxc.utils.AppLogWrapper import org.wordpress.android.repositories.SiteAuthState @@ -40,10 +36,7 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { @Mock lateinit var applicationPasswordLoginHelper: ApplicationPasswordLoginHelper @Mock lateinit var siteStore: SiteStore @Mock lateinit var appLogWrapper: AppLogWrapper - @Mock lateinit var selfHostedEndpointFinder: SelfHostedEndpointFinder - @Mock lateinit var siteXMLRPCClient: SiteXMLRPCClient @Mock lateinit var siteProvisioningSource: SiteProvisioningSource - @Mock lateinit var dispatcher: Dispatcher private lateinit var siteTest: SiteModel private var card: MySiteCardAndItem? = null @@ -56,17 +49,11 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { applicationPasswordLoginHelper, siteStore, appLogWrapper, - selfHostedEndpointFinder, - siteXMLRPCClient, siteProvisioningSource, - dispatcher, - testDispatcher(), ).apply { initialize(testScope()) } siteTest = SiteModel().apply { id = TEST_SITE_ID url = TEST_URL - // A WP.com-REST site by default, so a provisioned site hides the card (no XML-RPC path). - setIsWPCom(true) } card = null slice.uiModel.observeForever { card = it } @@ -126,6 +113,9 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { @Test fun `given ready on a WPCom-REST site, then no card`() = test { stubReadiness(SiteReadiness.Ready) + whenever(siteStore.getSiteByLocalId(TEST_SITE_ID)).thenReturn( + SiteModel().apply { id = TEST_SITE_ID; url = TEST_URL; setIsWPCom(true) } + ) slice.buildCard(siteTest) @@ -134,16 +124,12 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { } @Test - fun `given ready on a self-hosted site missing XML-RPC, then show the XML-RPC disabled card`() = test { - siteTest = SiteModel().apply { - id = TEST_SITE_ID - url = TEST_URL - // Not WP.com-REST and no XML-RPC endpoint — the one case the card still surfaces. - } + fun `given ready on a self-hosted site whose XML-RPC stayed unrecovered, then show the disabled card`() = test { stubReadiness(SiteReadiness.Ready) - // Let rediscovery fail so the card stays put for the assertion. - whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(TEST_URL)) - .thenThrow(mock()) + // Not WP.com-REST and still no XML-RPC endpoint after the pipeline's recovery attempt. + whenever(siteStore.getSiteByLocalId(TEST_SITE_ID)).thenReturn( + SiteModel().apply { id = TEST_SITE_ID; url = TEST_URL } + ) slice.buildCard(siteTest) From 4861cb8238fcfb25ec7e99dcee2d1e67d6b24e75 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Thu, 4 Jun 2026 14:26:36 -0600 Subject: [PATCH 06/12] Satisfy detekt and checkstyle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - @Suppress("ReturnCount") on ensureAuth — its five returns are each a distinct auth outcome; a single-return rewrite would read worse. - Drop a stray blank line before a brace left from removing a test region. --- .../wordpress/android/repositories/SiteProvisioningSource.kt | 3 +++ .../wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt index 1e1dbab82863..a2a1f0ef662d 100644 --- a/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt +++ b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt @@ -159,6 +159,9 @@ class SiteProvisioningSource @Inject constructor( * confirmed rejection wipes them and mints fresh ones via the FluxC Jetpack * tunnel. The mint persists the credentials, so later stages read them back. */ + // Each return is a distinct auth outcome (missing site, valid, transient, minted, failed); + // collapsing to one return would thread a result through nested branches and read worse. + @Suppress("ReturnCount") private suspend fun ensureAuth(siteLocalId: Int): SiteAuthState { val site = siteStore.getSiteByLocalId(siteLocalId) ?: return SiteAuthState.Unprovisionable(hadCredentials = false) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt index 17ec4557b451..c5989af09870 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt @@ -481,5 +481,4 @@ class GutenbergEditorPreloaderTest : } // endregion - } From c46b58e75d772c4e84b9f2529c3eda9a9e05dba8 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Thu, 4 Jun 2026 14:41:28 -0600 Subject: [PATCH 07/12] Rename recover stages to recoverRestUrlIfNeeded / recoverXmlRpcIfNeeded The IfNeeded suffix makes the short-circuit (skip when the field is already present / not applicable) clear at the call site. --- .../android/repositories/SiteProvisioningSource.kt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt index a2a1f0ef662d..e5b191b30ccd 100644 --- a/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt +++ b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt @@ -143,10 +143,10 @@ class SiteProvisioningSource @Inject constructor( SiteAuthState.Provisioned -> coroutineScope { // Post-auth, the REST-capability chain and the XML-RPC recovery are independent — // each reads the site fresh and writes only its own column — so run them in - // parallel. recoverRestUrl precedes detectCapabilities within its branch because + // parallel. recoverRestUrlIfNeeded precedes detectCapabilities within its branch because // the probe needs the recovered REST root. - val capabilities = async { recoverRestUrl(siteLocalId); detectCapabilities(siteLocalId) } - val xmlRpc = async { recoverXmlRpc(siteLocalId) } + val capabilities = async { recoverRestUrlIfNeeded(siteLocalId); detectCapabilities(siteLocalId) } + val xmlRpc = async { recoverXmlRpcIfNeeded(siteLocalId) } xmlRpc.await() capabilities.await() } @@ -199,7 +199,7 @@ class SiteProvisioningSource @Inject constructor( * Jetpack tunnel (which never runs discovery and leaves `wpApiRestUrl` null). * Persists the one column; the capability probe re-reads it. */ - private suspend fun recoverRestUrl(siteLocalId: Int) { + private suspend fun recoverRestUrlIfNeeded(siteLocalId: Int) { val site = siteStore.getSiteByLocalId(siteLocalId) ?: return if (!site.wpApiRestUrl.isNullOrEmpty()) return siteApiRestUrlRecoverer.discoverApiRootUrl(site.url)?.let { apiRootUrl -> @@ -212,7 +212,7 @@ class SiteProvisioningSource @Inject constructor( * sites that don't have one. Discovers + authenticates against it, and on * success persists the one column; the application-password card re-reads it. */ - private suspend fun recoverXmlRpc(siteLocalId: Int) { + private suspend fun recoverXmlRpcIfNeeded(siteLocalId: Int) { val site = siteStore.getSiteByLocalId(siteLocalId) ?: return // WP.com / Atomic / Jetpack-WPCom-REST sites talk REST end-to-end and don't use XML-RPC. if (site.isUsingWpComRestApi || !site.xmlRpcUrl.isNullOrEmpty()) return From 86d8d7c180a209460022d0dbfc67ca99d54ae721 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Thu, 4 Jun 2026 15:02:46 -0600 Subject: [PATCH 08/12] Don't gate capability detection behind a mint for WP.com Simple sites WP.com Simple sites are proxy-served and OAuth-authed; the application-password mint returns NotSupported for them, so the pipeline was returning Unprovisionable and never reaching capability detection (which works fine through the proxy) -- a regression vs. the old ungated probe. ensureAuth now short-circuits them to a new SiteAuthState.NotApplicable (treated like Provisioned), and recoverRestUrlIfNeeded skips them too. --- .../repositories/SiteProvisioningSource.kt | 14 ++++++++++++-- .../ApplicationPasswordViewModelSlice.kt | 3 ++- .../repositories/SiteProvisioningSourceTest.kt | 17 +++++++++++++++++ 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt index e5b191b30ccd..d946304f9280 100644 --- a/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt +++ b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt @@ -140,7 +140,7 @@ class SiteProvisioningSource @Inject constructor( private suspend fun runPipeline(siteLocalId: Int): SiteReadiness = when (val auth = ensureAuth(siteLocalId)) { - SiteAuthState.Provisioned -> coroutineScope { + SiteAuthState.Provisioned, SiteAuthState.NotApplicable -> coroutineScope { // Post-auth, the REST-capability chain and the XML-RPC recovery are independent — // each reads the site fresh and writes only its own column — so run them in // parallel. recoverRestUrlIfNeeded precedes detectCapabilities within its branch because @@ -165,6 +165,10 @@ class SiteProvisioningSource @Inject constructor( private suspend fun ensureAuth(siteLocalId: Int): SiteAuthState { val site = siteStore.getSiteByLocalId(siteLocalId) ?: return SiteAuthState.Unprovisionable(hadCredentials = false) + // WP.com Simple sites are fully proxied and OAuth-bearer-authed — no application password + // applies (the mint returns NotSupported). Capability detection works through the proxy, so + // treat them as ready instead of blocking detection behind a mint that can never run. + if (site.isWPComSimpleSite) return SiteAuthState.NotApplicable val hadCredentials = !applicationPasswordLoginHelper.siteHasBadCredentials(site) if (hadCredentials) { when (applicationPasswordValidator.validate(site)) { @@ -201,7 +205,9 @@ class SiteProvisioningSource @Inject constructor( */ private suspend fun recoverRestUrlIfNeeded(siteLocalId: Int) { val site = siteStore.getSiteByLocalId(siteLocalId) ?: return - if (!site.wpApiRestUrl.isNullOrEmpty()) return + // WP.com Simple sites are proxy-served — no direct REST host to recover (their wpApiRestUrl + // is legitimately null), so don't burn a discovery call on them. + if (site.isWPComSimpleSite || !site.wpApiRestUrl.isNullOrEmpty()) return siteApiRestUrlRecoverer.discoverApiRootUrl(site.url)?.let { apiRootUrl -> siteApiRestUrlRecoverer.persistApiRootUrl(siteLocalId, apiRootUrl) } @@ -247,6 +253,10 @@ sealed interface SiteAuthState { /** Credentials are usable (validated, or freshly minted). */ data object Provisioned : SiteAuthState + /** No application password applies — a WP.com Simple site, which is proxy-served and + * OAuth-bearer-authed. Treated like [Provisioned]: capability detection runs via the proxy. */ + data object NotApplicable : SiteAuthState + /** Not usable yet, but not a terminal failure — a mint is implied / a transient * validation error occurred. The card stays hidden; the next run retries. */ data object Provisioning : SiteAuthState diff --git a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt index 38545070abac..69a1cc998b86 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt @@ -82,7 +82,8 @@ class ApplicationPasswordViewModelSlice @Inject constructor( uiModelMutable.postValue(null) appLogWrapper.d(AppLog.T.MAIN, "A_P: Provisioning in progress for ${site.url}") } - SiteAuthState.Provisioned -> Unit // unreachable: Provisioned never wraps in NeedsAuth + SiteAuthState.Provisioned, SiteAuthState.NotApplicable -> + Unit // never wrap in NeedsAuth — they proceed to detection } // Any terminal provisioned state — the credentials are usable, so the only card left to // show is the self-hosted XML-RPC fallback. Capability outcome (Ready/Unreachable) is the diff --git a/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt b/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt index 0c5fae5796de..aa44d02a5e42 100644 --- a/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt @@ -157,6 +157,23 @@ class SiteProvisioningSourceTest : BaseUnitTest(StandardTestDispatcher()) { assertThat(result).isEqualTo(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = false))) } + @Test + fun `given a WPCom Simple site, then it detects capabilities without minting`() = test { + val simple = SiteModel().apply { + id = TEST_SITE_LOCAL_ID + url = "https://simple.wordpress.com" + setIsWPCom(true) // isWPComSimpleSite = isWPCom && !isWPComAtomic + wpApiRestUrl = "https://simple.wordpress.com/wp-json" + } + whenever(siteStore.getSiteByLocalId(TEST_SITE_LOCAL_ID)).thenReturn(simple) + stubCapabilityProbe(ok = true) + + assertThat(source.await(simple)).isEqualTo(SiteReadiness.Ready) + // No application password applies — it must not validate or mint, just probe via the proxy. + verify(applicationPasswordValidator, never()).validate(any()) + verify(siteStore, never()).createApplicationPassword(any()) + } + // endregion // region recovery + capability stages From 166309b26b4214fc2bd158c564e4a0abbe8ea27e Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Thu, 4 Jun 2026 15:17:46 -0600 Subject: [PATCH 09/12] Probe the direct host for Jetpack capability detection, not the proxy The route-support probe only sent Atomic sites to the direct host; Jetpack WPCom-REST sites fell through to the WP.com proxy. Since the proxy and the direct host advertise different route lists (the #22879 premise), Jetpack sites got the wrong answer. Broaden the predicate to isUsingWpComRestApi && !isWPComSimpleSite (Atomic + Jetpack); the proxy is only for minting the application password. --- .../repositories/EditorSettingsRepository.kt | 11 +++---- .../EditorSettingsRepositoryTest.kt | 30 +++++++++++++++++++ 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/repositories/EditorSettingsRepository.kt b/WordPress/src/main/java/org/wordpress/android/repositories/EditorSettingsRepository.kt index bce5a2d3dcf4..d57b7b8c44f9 100644 --- a/WordPress/src/main/java/org/wordpress/android/repositories/EditorSettingsRepository.kt +++ b/WordPress/src/main/java/org/wordpress/android/repositories/EditorSettingsRepository.kt @@ -108,11 +108,12 @@ class EditorSettingsRepository @Inject constructor( private suspend fun fetchRouteSupport( site: SiteModel ): Boolean = try { - // For Atomic sites the editor fetches `wp-block-editor/v1/settings` - // from the direct host — proxy and direct host can advertise - // different route lists, so detection has to probe the direct host - // too. See #22879. - if (site.isWPComAtomic) { + // Atomic and Jetpack-WPCom-REST sites have their own REST host that the editor talks to + // directly — the WP.com proxy and the direct host advertise different route lists, so + // detection has to probe the direct host too. The proxy is only for minting the application + // password. WP.com Simple sites have no direct host (the WP.com REST API *is* their API), + // and self-hosted sites are already direct via the configured client. See #22879. + if (site.isUsingWpComRestApi && !site.isWPComSimpleSite) { fetchRouteSupportViaDirectHostDiscovery(site) } else { fetchRouteSupportViaConfiguredClient(site) diff --git a/WordPress/src/test/java/org/wordpress/android/repositories/EditorSettingsRepositoryTest.kt b/WordPress/src/test/java/org/wordpress/android/repositories/EditorSettingsRepositoryTest.kt index c4371b8675fb..a3bd6301355c 100644 --- a/WordPress/src/test/java/org/wordpress/android/repositories/EditorSettingsRepositoryTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/repositories/EditorSettingsRepositoryTest.kt @@ -229,6 +229,36 @@ class EditorSettingsRepositoryTest : BaseUnitTest() { verify(wpApiClientProvider, never()).getWpApiClient(atomicSite) } + @Test + fun `jetpack site probes the direct host, not the WP_com proxy`() = + runTest { + val jetpackSite = SiteModel().apply { + id = 7 + url = "https://jetpack.example.com" + // isUsingWpComRestApi via Jetpack, but not Atomic and not WP.com Simple → direct host. + setIsJetpackConnected(true) + setOrigin(SiteModel.ORIGIN_WPCOM_REST) + } + mockDiscoverySuccess( + siteUrl = jetpackSite.url, + hasEditorSettings = true, + hasEditorAssets = true + ) + whenever(themeRepository.fetchCurrentTheme(jetpackSite)) + .thenReturn(buildTheme(isBlockTheme = false)) + + val result = + repository.fetchEditorCapabilitiesForSite(jetpackSite) + + assertThat(result).isTrue() + verify(appPrefsWrapper) + .setSiteSupportsEditorSettings(jetpackSite, true) + verify(appPrefsWrapper) + .setSiteSupportsEditorAssets(jetpackSite, true) + // The proxy is only for minting — capability detection goes direct. + verify(wpApiClientProvider, never()).getWpApiClient(jetpackSite) + } + @Test fun `atomic site returns false when discovery fails`() = runTest { From bab2b773704035d53fd9ea48ccc38b01b316dd2d Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 5 Jun 2026 11:47:00 -0600 Subject: [PATCH 10/12] Carry minted credentials to the capability probe as a value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a first provision of an Atomic site, the My Site screen showed a false "Unable to connect to your site" banner. `ensureAuth` minted an application password, but `detectCapabilities` re-read the `SiteModel` fresh and a concurrent whole-row site write (`insertOrUpdateSite` uses `UpdateAllExceptId`) had clobbered the just-encrypted credential columns before that read (#22905). With no credentials present, the authenticated direct-host probe was skipped, the unauthenticated discovery failed against the private host, and the probe reported the site unreachable. `ensureAuth` now returns the credentials it obtained in an internal `AuthResult`, and `detectCapabilities` overlays that immutable value onto its own coroutine-local `SiteModel` copy. The stages still read fresh and write only their own column — no `SiteModel` is shared or mutated across the parallel detect/`recoverXmlRpc` branches, so there's no race. Verified on-device against a private Atomic site: the authenticated direct-host probe runs and the banner is gone. --- .../repositories/SiteProvisioningSource.kt | 92 ++++++++++++++----- .../SiteProvisioningSourceTest.kt | 36 ++++++++ 2 files changed, 104 insertions(+), 24 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt index d946304f9280..24a2cf187b21 100644 --- a/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt +++ b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt @@ -138,45 +138,55 @@ class SiteProvisioningSource @Inject constructor( private fun shouldRun(siteLocalId: Int): Boolean = jobs[siteLocalId]?.isActive != true && siteLocalId !in ready - private suspend fun runPipeline(siteLocalId: Int): SiteReadiness = - when (val auth = ensureAuth(siteLocalId)) { + private suspend fun runPipeline(siteLocalId: Int): SiteReadiness { + val auth = ensureAuth(siteLocalId) + return when (auth.state) { SiteAuthState.Provisioned, SiteAuthState.NotApplicable -> coroutineScope { - // Post-auth, the REST-capability chain and the XML-RPC recovery are independent — - // each reads the site fresh and writes only its own column — so run them in - // parallel. recoverRestUrlIfNeeded precedes detectCapabilities within its branch because - // the probe needs the recovered REST root. - val capabilities = async { recoverRestUrlIfNeeded(siteLocalId); detectCapabilities(siteLocalId) } + // Post-auth, the REST-capability chain and the XML-RPC recovery are independent — each + // reads the site fresh and writes only its own column — so run them in parallel. + // recoverRestUrlIfNeeded precedes detectCapabilities within its branch because the probe + // needs the recovered REST root. The mint's credentials reach the probe as an immutable + // value (auth.credentials), never via a shared mutated SiteModel — the fresh-read columns + // can't be trusted to carry them (transient, and clobberable by a concurrent write, #22905). + val capabilities = async { + recoverRestUrlIfNeeded(siteLocalId) + detectCapabilities(siteLocalId, auth.credentials) + } val xmlRpc = async { recoverXmlRpcIfNeeded(siteLocalId) } xmlRpc.await() capabilities.await() } - else -> SiteReadiness.NeedsAuth(auth) + else -> SiteReadiness.NeedsAuth(auth.state) } + } /** - * Stage 1 — ensure the site has working application-password credentials. - * Validates stored creds with Basic auth against the direct host; on a - * confirmed rejection wipes them and mints fresh ones via the FluxC Jetpack - * tunnel. The mint persists the credentials, so later stages read them back. + * Stage 1 — ensure the site has working application-password credentials, and + * return them. Validates stored creds with Basic auth against the direct host; + * on a confirmed rejection wipes them and mints fresh ones via the FluxC Jetpack + * tunnel. The credentials are returned in the [AuthResult] so [detectCapabilities] + * can authenticate with them without trusting a fresh re-read — the plain columns + * are transient and a concurrent whole-row site write can clobber the encrypted + * ones mid-run (#22905). */ // Each return is a distinct auth outcome (missing site, valid, transient, minted, failed); // collapsing to one return would thread a result through nested branches and read worse. @Suppress("ReturnCount") - private suspend fun ensureAuth(siteLocalId: Int): SiteAuthState { + private suspend fun ensureAuth(siteLocalId: Int): AuthResult { val site = siteStore.getSiteByLocalId(siteLocalId) - ?: return SiteAuthState.Unprovisionable(hadCredentials = false) + ?: return AuthResult(SiteAuthState.Unprovisionable(hadCredentials = false)) // WP.com Simple sites are fully proxied and OAuth-bearer-authed — no application password // applies (the mint returns NotSupported). Capability detection works through the proxy, so // treat them as ready instead of blocking detection behind a mint that can never run. - if (site.isWPComSimpleSite) return SiteAuthState.NotApplicable + if (site.isWPComSimpleSite) return AuthResult(SiteAuthState.NotApplicable) val hadCredentials = !applicationPasswordLoginHelper.siteHasBadCredentials(site) if (hadCredentials) { when (applicationPasswordValidator.validate(site)) { ApplicationPasswordValidator.Outcome.Valid -> - return SiteAuthState.Provisioned + return AuthResult(SiteAuthState.Provisioned, site.provisionedCredentials()) ApplicationPasswordValidator.Outcome.NetworkUnavailable -> { appLogWrapper.d(AppLog.T.MAIN, "A_P: Validation network error for ${site.url}") - return SiteAuthState.Provisioning + return AuthResult(SiteAuthState.Provisioning) } ApplicationPasswordValidator.Outcome.Invalid -> { appLogWrapper.d(AppLog.T.MAIN, "A_P: Stored creds invalid for ${site.url}, clearing") @@ -185,23 +195,24 @@ class SiteProvisioningSource @Inject constructor( } } } + // createApplicationPassword mutates this local `site` with the freshly minted plain credentials. val createResult = siteStore.createApplicationPassword(site) if (!createResult.isError && createResult.credentials != null) { wpApiClientProvider.clearSelfHostedClient(site.id) appLogWrapper.d(AppLog.T.MAIN, "A_P: Headless mint succeeded for ${site.url}") - return SiteAuthState.Provisioned + return AuthResult(SiteAuthState.Provisioned, site.provisionedCredentials()) } appLogWrapper.d( AppLog.T.MAIN, "A_P: Headless mint failed for ${site.url} (notSupported=${createResult.error?.notSupported})" ) - return SiteAuthState.Unprovisionable(hadCredentials = hadCredentials) + return AuthResult(SiteAuthState.Unprovisionable(hadCredentials = hadCredentials)) } /** * Stage 2a — recover the REST API root for Atomic sites minted through the * Jetpack tunnel (which never runs discovery and leaves `wpApiRestUrl` null). - * Persists the one column; the capability probe re-reads it. + * Persists the one column; the capability probe (sequenced after it) re-reads it. */ private suspend fun recoverRestUrlIfNeeded(siteLocalId: Int) { val site = siteStore.getSiteByLocalId(siteLocalId) ?: return @@ -229,12 +240,25 @@ class SiteProvisioningSource @Inject constructor( /** * Stage 3 — probe the REST API for editor-capability support and persist it. - * Reached only once auth is [SiteAuthState.Provisioned], so credentials are - * guaranteed present: a failure here is a real transport problem, not a - * pending mint. + * Reached only once auth is [SiteAuthState.Provisioned] / [SiteAuthState.NotApplicable]. + * [credentials] (from [ensureAuth]) are overlaid onto this run-local copy so the + * authenticated direct-host probe can run even when the fresh read doesn't reflect + * the mint; a failure here is then a real transport problem, not a pending mint. */ - private suspend fun detectCapabilities(siteLocalId: Int): SiteReadiness { + private suspend fun detectCapabilities( + siteLocalId: Int, + credentials: ProvisionedCredentials?, + ): SiteReadiness { val site = siteStore.getSiteByLocalId(siteLocalId) ?: return SiteReadiness.Unreachable + // Overlay the credentials ensureAuth obtained. The fresh read can't be trusted to carry them: + // the plain columns are transient (never persisted), and a concurrent whole-row site write + // during My Site load can clobber the encrypted ones before this read (#22905). This copy is + // local to this coroutine — not shared with the parallel XML-RPC stage — so the overlay is + // race-free. + credentials?.let { + if (site.apiRestUsernamePlain.isNullOrEmpty()) site.apiRestUsernamePlain = it.username + if (site.apiRestPasswordPlain.isNullOrEmpty()) site.apiRestPasswordPlain = it.password + } val ok = editorSettingsRepository.fetchEditorCapabilitiesForSite(site) val hasCache = editorSettingsRepository.hasCachedCapabilities(site) return when { @@ -245,6 +269,26 @@ class SiteProvisioningSource @Inject constructor( } } +/** + * Snapshot of the application-password credentials [SiteProvisioningSource.ensureAuth] obtained, + * handed forward to the capability probe as an immutable value rather than via a shared, mutated + * [SiteModel] — so the parallel stages never race on it. + */ +private data class ProvisionedCredentials(val username: String, val password: String) + +private fun SiteModel.provisionedCredentials(): ProvisionedCredentials? { + val user = apiRestUsernamePlain + val pass = apiRestPasswordPlain + return if (!user.isNullOrEmpty() && !pass.isNullOrEmpty()) ProvisionedCredentials(user, pass) else null +} + +/** + * [SiteProvisioningSource.ensureAuth]'s outcome: the public [SiteAuthState] plus, when provisioned, + * the credentials to hand to the capability probe. Internal so the credentials never leak into the + * UI-facing [SiteReadiness] / [SiteAuthState]. + */ +private data class AuthResult(val state: SiteAuthState, val credentials: ProvisionedCredentials? = null) + /** * Whether a site's application password is usable. Owned by [SiteProvisioningSource]; * rendered by the application-password card. diff --git a/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt b/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt index aa44d02a5e42..d9e140b48a5c 100644 --- a/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt @@ -7,6 +7,7 @@ import org.junit.Before import org.junit.Test import org.mockito.Mock import org.mockito.kotlin.any +import org.mockito.kotlin.argThat import org.mockito.kotlin.eq import org.mockito.kotlin.never import org.mockito.kotlin.times @@ -91,6 +92,14 @@ class SiteProvisioningSourceTest : BaseUnitTest(StandardTestDispatcher()) { if (!ok) whenever(editorSettingsRepository.hasCachedCapabilities(any())).thenReturn(cached) } + private fun provisionableSiteCopy() = SiteModel().apply { + id = TEST_SITE_LOCAL_ID + url = "https://test.example.com" + // Non-null REST root + XML-RPC url so both recovery branches short-circuit. + wpApiRestUrl = "https://test.example.com/wp-json" + xmlRpcUrl = "https://test.example.com/xmlrpc.php" + } + // region auth stage @Test @@ -174,6 +183,33 @@ class SiteProvisioningSourceTest : BaseUnitTest(StandardTestDispatcher()) { verify(siteStore, never()).createApplicationPassword(any()) } + @Test + fun `carries minted credentials to the probe even when the re-read lacks them`() = test { + // ensureAuth reads one copy and the mint sets credentials on it; detectCapabilities re-reads a + // SEPARATE copy that — simulating a concurrent whole-row clobber (#22905) — carries none. The + // probe must still receive a credentialed site, because the pipeline hands the mint's creds + // forward as an immutable value rather than relying on the re-read or a shared, race-prone model. + val authCopy = provisionableSiteCopy() + val credentiallessReread = provisionableSiteCopy() + whenever(siteStore.getSiteByLocalId(TEST_SITE_LOCAL_ID)).thenReturn(authCopy, credentiallessReread) + stubHasStoredCredentials(false) + whenever(siteStore.createApplicationPassword(any())).thenAnswer { invocation -> + // The real createApplicationPassword sets the plain credentials on the passed site. + (invocation.arguments[0] as SiteModel).apply { + apiRestUsernamePlain = "user" + apiRestPasswordPlain = "pass" + } + OnApplicationPasswordCreated(authCopy, ApplicationPasswordCredentials("user", "pass", uuid = "u")) + } + stubCapabilityProbe(ok = true) + + source.await(site) + + verify(editorSettingsRepository).fetchEditorCapabilitiesForSite( + argThat { apiRestUsernamePlain == "user" && apiRestPasswordPlain == "pass" } + ) + } + // endregion // region recovery + capability stages From 9b38f59d041a173513a9d11bc3809545ed923747 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Sat, 13 Jun 2026 10:26:21 -0600 Subject: [PATCH 11/12] Contain unexpected throws in SiteXmlRpcUrlRecoverer discoverAndVerifyXmlRpcUrl caught only DiscoveryException, so a RuntimeException from the discovery/verify path would escape the async, cancel the whole provisioning coroutine, and reach appScope (no SupervisorJob/handler). Catch CancellationException + generic Exception like the sibling SiteApiRestUrlRecoverer, so any failure degrades to the XML-RPC-disabled card and retries next run. --- .../accounts/login/SiteXmlRpcUrlRecoverer.kt | 18 ++++++++++++++++-- .../login/SiteXmlRpcUrlRecovererTest.kt | 13 ++++++++++++- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecoverer.kt b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecoverer.kt index b44dbefcf4e7..6278d15fc424 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecoverer.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecoverer.kt @@ -12,6 +12,7 @@ import org.wordpress.android.util.AppLog import javax.inject.Inject import javax.inject.Named import javax.inject.Singleton +import kotlin.coroutines.cancellation.CancellationException /** * Heals [SiteModel.xmlRpcUrl] for true self-hosted sites whose XML-RPC endpoint was never @@ -19,7 +20,8 @@ import javax.inject.Singleton * follows the same shape so callers never hold a mutated [SiteModel]: * * - [discoverAndVerifyXmlRpcUrl] discovers the endpoint and confirms it works with an authenticated - * call, returning the verified URL (or `null` if discovery/verification fails). + * call using the site's application-password credentials, returning the verified URL (or `null` + * if discovery/verification fails). * - [persistXmlRpcUrl] writes only that one column to the DB row for `localId`. */ @Singleton @@ -30,7 +32,7 @@ class SiteXmlRpcUrlRecoverer @Inject constructor( private val appLogWrapper: AppLogWrapper, @param:Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher, ) { - @Suppress("SwallowedException") + @Suppress("SwallowedException", "TooGenericExceptionCaught") suspend fun discoverAndVerifyXmlRpcUrl(site: SiteModel): String? = withContext(bgDispatcher) { try { val endpoint = selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url) @@ -45,9 +47,21 @@ class SiteXmlRpcUrlRecoverer @Inject constructor( } else { endpoint } + } catch (e: CancellationException) { + throw e } catch (e: SelfHostedEndpointFinder.DiscoveryException) { + // Expected when the site has no reachable XML-RPC endpoint — surfaces as the + // XML-RPC-disabled card (xmlRpcUrl stays empty) and retries on the next run. appLogWrapper.w(AppLog.T.API, "XML-RPC discovery failed for ${site.url}") null + } catch (e: Exception) { + // Best-effort recovery must never let an unexpected throw escape and cancel the + // provisioning pipeline (mirrors SiteApiRestUrlRecoverer). Same null -> disabled-card surface. + appLogWrapper.e( + AppLog.T.API, + "XML-RPC discovery threw for ${site.url}: ${e::class.simpleName}: ${e.message}" + ) + null } } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecovererTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecovererTest.kt index d895c67265ab..3ce345c0fc41 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecovererTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecovererTest.kt @@ -56,7 +56,8 @@ class SiteXmlRpcUrlRecovererTest : BaseUnitTest() { @Test fun `given discovery and authenticated verify succeed, then returns the endpoint`() = test { whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url)).thenReturn(ENDPOINT) - whenever(siteXMLRPCClient.fetchSites(eq(ENDPOINT), any(), any())) + // The site's stored credentials are forwarded to the authenticated verify call. + whenever(siteXMLRPCClient.fetchSites(eq(ENDPOINT), eq("user"), eq("pass"))) .thenReturn(SitesModel(listOf(SiteModel()))) assertThat(recoverer.discoverAndVerifyXmlRpcUrl(site)).isEqualTo(ENDPOINT) @@ -70,6 +71,16 @@ class SiteXmlRpcUrlRecovererTest : BaseUnitTest() { assertThat(recoverer.discoverAndVerifyXmlRpcUrl(site)).isNull() } + @Test + fun `given discovery throws an unexpected exception, then returns null`() = test { + // A non-DiscoveryException (e.g. a RuntimeException from the network/parse path) must be + // contained, not propagated — otherwise it cancels the whole provisioning pipeline. + whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url)) + .thenThrow(RuntimeException("unexpected")) + + assertThat(recoverer.discoverAndVerifyXmlRpcUrl(site)).isNull() + } + @Test fun `given the authenticated verify errors, then returns null`() = test { whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url)).thenReturn(ENDPOINT) From 3f20dc4fdd6bd48940a2184d7fc3d212be608e6e Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Sat, 13 Jun 2026 10:26:30 -0600 Subject: [PATCH 12/12] Drop credential forwarding now that app-password columns are single-writer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #22947 made the app-password columns single-writer — excluded from the generic full-row update, written only by updateApplicationPasswordCredentials — so a fresh getSiteByLocalId after a mint reliably carries the credentials and a concurrent site write can't clobber them (#22905). The pipeline no longer needs to thread them as a value: remove ProvisionedCredentials, AuthResult.credentials, and the detectCapabilities overlay. ensureAuth returns a bare SiteAuthState; detectCapabilities and recoverXmlRpcIfNeeded read the credentials off the fresh read. --- .../repositories/SiteProvisioningSource.kt | 86 ++++++------------- .../SiteProvisioningSourceTest.kt | 36 -------- 2 files changed, 26 insertions(+), 96 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt index 24a2cf187b21..78704b09428b 100644 --- a/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt +++ b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt @@ -140,53 +140,49 @@ class SiteProvisioningSource @Inject constructor( private suspend fun runPipeline(siteLocalId: Int): SiteReadiness { val auth = ensureAuth(siteLocalId) - return when (auth.state) { + return when (auth) { SiteAuthState.Provisioned, SiteAuthState.NotApplicable -> coroutineScope { // Post-auth, the REST-capability chain and the XML-RPC recovery are independent — each // reads the site fresh and writes only its own column — so run them in parallel. // recoverRestUrlIfNeeded precedes detectCapabilities within its branch because the probe - // needs the recovered REST root. The mint's credentials reach the probe as an immutable - // value (auth.credentials), never via a shared mutated SiteModel — the fresh-read columns - // can't be trusted to carry them (transient, and clobberable by a concurrent write, #22905). + // needs the recovered REST root. Both branches read the credentials fresh from the store: + // the mint persists them via a single writer that the generic full-row update can no + // longer clobber (#22947), so the re-read is now trustworthy (#22905). val capabilities = async { recoverRestUrlIfNeeded(siteLocalId) - detectCapabilities(siteLocalId, auth.credentials) + detectCapabilities(siteLocalId) } val xmlRpc = async { recoverXmlRpcIfNeeded(siteLocalId) } xmlRpc.await() capabilities.await() } - else -> SiteReadiness.NeedsAuth(auth.state) + else -> SiteReadiness.NeedsAuth(auth) } } /** - * Stage 1 — ensure the site has working application-password credentials, and - * return them. Validates stored creds with Basic auth against the direct host; - * on a confirmed rejection wipes them and mints fresh ones via the FluxC Jetpack - * tunnel. The credentials are returned in the [AuthResult] so [detectCapabilities] - * can authenticate with them without trusting a fresh re-read — the plain columns - * are transient and a concurrent whole-row site write can clobber the encrypted - * ones mid-run (#22905). + * Stage 1 — ensure the site has working application-password credentials. Validates stored creds + * with Basic auth against the direct host; on a confirmed rejection wipes them and mints fresh + * ones via the FluxC Jetpack tunnel. The mint persists the credentials (single-writer, #22947), so + * the downstream stages read them back from a fresh [SiteModel] rather than having them threaded. */ // Each return is a distinct auth outcome (missing site, valid, transient, minted, failed); // collapsing to one return would thread a result through nested branches and read worse. @Suppress("ReturnCount") - private suspend fun ensureAuth(siteLocalId: Int): AuthResult { + private suspend fun ensureAuth(siteLocalId: Int): SiteAuthState { val site = siteStore.getSiteByLocalId(siteLocalId) - ?: return AuthResult(SiteAuthState.Unprovisionable(hadCredentials = false)) + ?: return SiteAuthState.Unprovisionable(hadCredentials = false) // WP.com Simple sites are fully proxied and OAuth-bearer-authed — no application password // applies (the mint returns NotSupported). Capability detection works through the proxy, so // treat them as ready instead of blocking detection behind a mint that can never run. - if (site.isWPComSimpleSite) return AuthResult(SiteAuthState.NotApplicable) + if (site.isWPComSimpleSite) return SiteAuthState.NotApplicable val hadCredentials = !applicationPasswordLoginHelper.siteHasBadCredentials(site) if (hadCredentials) { when (applicationPasswordValidator.validate(site)) { - ApplicationPasswordValidator.Outcome.Valid -> - return AuthResult(SiteAuthState.Provisioned, site.provisionedCredentials()) + ApplicationPasswordValidator.Outcome.Valid -> return SiteAuthState.Provisioned ApplicationPasswordValidator.Outcome.NetworkUnavailable -> { appLogWrapper.d(AppLog.T.MAIN, "A_P: Validation network error for ${site.url}") - return AuthResult(SiteAuthState.Provisioning) + return SiteAuthState.Provisioning } ApplicationPasswordValidator.Outcome.Invalid -> { appLogWrapper.d(AppLog.T.MAIN, "A_P: Stored creds invalid for ${site.url}, clearing") @@ -195,18 +191,19 @@ class SiteProvisioningSource @Inject constructor( } } } - // createApplicationPassword mutates this local `site` with the freshly minted plain credentials. + // createApplicationPassword mints and persists the credentials; downstream stages read them + // back from a fresh SiteModel. val createResult = siteStore.createApplicationPassword(site) if (!createResult.isError && createResult.credentials != null) { wpApiClientProvider.clearSelfHostedClient(site.id) appLogWrapper.d(AppLog.T.MAIN, "A_P: Headless mint succeeded for ${site.url}") - return AuthResult(SiteAuthState.Provisioned, site.provisionedCredentials()) + return SiteAuthState.Provisioned } appLogWrapper.d( AppLog.T.MAIN, "A_P: Headless mint failed for ${site.url} (notSupported=${createResult.error?.notSupported})" ) - return AuthResult(SiteAuthState.Unprovisionable(hadCredentials = hadCredentials)) + return SiteAuthState.Unprovisionable(hadCredentials = hadCredentials) } /** @@ -226,8 +223,10 @@ class SiteProvisioningSource @Inject constructor( /** * Stage 2b (parallel) — recover the XML-RPC endpoint for true self-hosted - * sites that don't have one. Discovers + authenticates against it, and on - * success persists the one column; the application-password card re-reads it. + * sites that don't have one. Discovers + authenticates against it with the + * site's application-password credentials (which work for XML-RPC just as for + * REST), and on success persists the one column; the application-password card + * re-reads it. */ private suspend fun recoverXmlRpcIfNeeded(siteLocalId: Int) { val site = siteStore.getSiteByLocalId(siteLocalId) ?: return @@ -241,24 +240,11 @@ class SiteProvisioningSource @Inject constructor( /** * Stage 3 — probe the REST API for editor-capability support and persist it. * Reached only once auth is [SiteAuthState.Provisioned] / [SiteAuthState.NotApplicable]. - * [credentials] (from [ensureAuth]) are overlaid onto this run-local copy so the - * authenticated direct-host probe can run even when the fresh read doesn't reflect - * the mint; a failure here is then a real transport problem, not a pending mint. + * Reads the site fresh: the mint has already persisted the credentials (single-writer, #22947), + * so a probe failure here is a real transport problem, not a pending mint. */ - private suspend fun detectCapabilities( - siteLocalId: Int, - credentials: ProvisionedCredentials?, - ): SiteReadiness { + private suspend fun detectCapabilities(siteLocalId: Int): SiteReadiness { val site = siteStore.getSiteByLocalId(siteLocalId) ?: return SiteReadiness.Unreachable - // Overlay the credentials ensureAuth obtained. The fresh read can't be trusted to carry them: - // the plain columns are transient (never persisted), and a concurrent whole-row site write - // during My Site load can clobber the encrypted ones before this read (#22905). This copy is - // local to this coroutine — not shared with the parallel XML-RPC stage — so the overlay is - // race-free. - credentials?.let { - if (site.apiRestUsernamePlain.isNullOrEmpty()) site.apiRestUsernamePlain = it.username - if (site.apiRestPasswordPlain.isNullOrEmpty()) site.apiRestPasswordPlain = it.password - } val ok = editorSettingsRepository.fetchEditorCapabilitiesForSite(site) val hasCache = editorSettingsRepository.hasCachedCapabilities(site) return when { @@ -269,26 +255,6 @@ class SiteProvisioningSource @Inject constructor( } } -/** - * Snapshot of the application-password credentials [SiteProvisioningSource.ensureAuth] obtained, - * handed forward to the capability probe as an immutable value rather than via a shared, mutated - * [SiteModel] — so the parallel stages never race on it. - */ -private data class ProvisionedCredentials(val username: String, val password: String) - -private fun SiteModel.provisionedCredentials(): ProvisionedCredentials? { - val user = apiRestUsernamePlain - val pass = apiRestPasswordPlain - return if (!user.isNullOrEmpty() && !pass.isNullOrEmpty()) ProvisionedCredentials(user, pass) else null -} - -/** - * [SiteProvisioningSource.ensureAuth]'s outcome: the public [SiteAuthState] plus, when provisioned, - * the credentials to hand to the capability probe. Internal so the credentials never leak into the - * UI-facing [SiteReadiness] / [SiteAuthState]. - */ -private data class AuthResult(val state: SiteAuthState, val credentials: ProvisionedCredentials? = null) - /** * Whether a site's application password is usable. Owned by [SiteProvisioningSource]; * rendered by the application-password card. diff --git a/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt b/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt index d9e140b48a5c..aa44d02a5e42 100644 --- a/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt @@ -7,7 +7,6 @@ import org.junit.Before import org.junit.Test import org.mockito.Mock import org.mockito.kotlin.any -import org.mockito.kotlin.argThat import org.mockito.kotlin.eq import org.mockito.kotlin.never import org.mockito.kotlin.times @@ -92,14 +91,6 @@ class SiteProvisioningSourceTest : BaseUnitTest(StandardTestDispatcher()) { if (!ok) whenever(editorSettingsRepository.hasCachedCapabilities(any())).thenReturn(cached) } - private fun provisionableSiteCopy() = SiteModel().apply { - id = TEST_SITE_LOCAL_ID - url = "https://test.example.com" - // Non-null REST root + XML-RPC url so both recovery branches short-circuit. - wpApiRestUrl = "https://test.example.com/wp-json" - xmlRpcUrl = "https://test.example.com/xmlrpc.php" - } - // region auth stage @Test @@ -183,33 +174,6 @@ class SiteProvisioningSourceTest : BaseUnitTest(StandardTestDispatcher()) { verify(siteStore, never()).createApplicationPassword(any()) } - @Test - fun `carries minted credentials to the probe even when the re-read lacks them`() = test { - // ensureAuth reads one copy and the mint sets credentials on it; detectCapabilities re-reads a - // SEPARATE copy that — simulating a concurrent whole-row clobber (#22905) — carries none. The - // probe must still receive a credentialed site, because the pipeline hands the mint's creds - // forward as an immutable value rather than relying on the re-read or a shared, race-prone model. - val authCopy = provisionableSiteCopy() - val credentiallessReread = provisionableSiteCopy() - whenever(siteStore.getSiteByLocalId(TEST_SITE_LOCAL_ID)).thenReturn(authCopy, credentiallessReread) - stubHasStoredCredentials(false) - whenever(siteStore.createApplicationPassword(any())).thenAnswer { invocation -> - // The real createApplicationPassword sets the plain credentials on the passed site. - (invocation.arguments[0] as SiteModel).apply { - apiRestUsernamePlain = "user" - apiRestPasswordPlain = "pass" - } - OnApplicationPasswordCreated(authCopy, ApplicationPasswordCredentials("user", "pass", uuid = "u")) - } - stubCapabilityProbe(ok = true) - - source.await(site) - - verify(editorSettingsRepository).fetchEditorCapabilitiesForSite( - argThat { apiRestUsernamePlain == "user" && apiRestPasswordPlain == "pass" } - ) - } - // endregion // region recovery + capability stages