Skip to content

Commit

Permalink
Merge pull request #13308 from woocommerce/issue/3974-remove-xmlrpc-a…
Browse files Browse the repository at this point in the history
…ccount-mismatch

[Login] Remove dependency on XMLRPC for the Account Mismatch flow
  • Loading branch information
hichamboushaba authored Jan 17, 2025
2 parents bbebf6c + 1e6f712 commit 51f62d3
Show file tree
Hide file tree
Showing 4 changed files with 161 additions and 132 deletions.
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
- [**] Enhanced WebView to dynamically scale receipt content, ensuring optimal fit across all device screen sizes. [https://github.com/woocommerce/woocommerce-android/pull/13266]
- [*] Fixes missing text in Blaze Campaigns card on larger display and font sizes [https://github.com/woocommerce/woocommerce-android/pull/13300]
- [*] Puerto Rico is now available in the list of countries that are supported by in-person payments [https://github.com/woocommerce/woocommerce-android/pull/13200]
- [Internal] [*] XMLRPC is not needed now for Jetpack Connection during an Account Mismatch flow [https://github.com/woocommerce/woocommerce-android/pull/13308]
- [*] Removed the outdated feedback survey for shipping labels [https://github.com/woocommerce/woocommerce-android/pull/13319]

21.4
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,10 @@ class AccountMismatchErrorViewModel @Inject constructor(
val site = site.await() ?: error("The site is not cached")
accountMismatchRepository.fetchJetpackConnectedEmail(site).fold(
onSuccess = { email ->
// Remove the WPAPI site from DB before continuing
// This ensure we don't have a duplicate site with a wrong origin
accountMismatchRepository.removeSiteFromDB(site)

val isUserAuthenticated = accountRepository.isUserLoggedIn() &&
accountRepository.getUserAccount()?.email == email
triggerEvent(OnJetpackConnectedEvent(email, isAuthenticated = isUserAuthenticated))
Expand Down
Original file line number Diff line number Diff line change
@@ -1,36 +1,33 @@
package com.woocommerce.android.ui.login.accountmismatch

import com.woocommerce.android.OnChangedException
import com.woocommerce.android.ui.login.WPApiSiteRepository
import com.woocommerce.android.util.WooLog
import com.woocommerce.android.util.awaitAny
import com.woocommerce.android.util.awaitEvent
import com.woocommerce.android.util.dispatchAndAwait
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.async
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.withContext
import org.wordpress.android.fluxc.Dispatcher
import org.wordpress.android.fluxc.generated.AuthenticationActionBuilder
import org.wordpress.android.fluxc.generated.SiteActionBuilder
import org.wordpress.android.fluxc.model.SiteModel
import org.wordpress.android.fluxc.store.AccountStore.OnAuthenticationChanged
import org.wordpress.android.fluxc.store.AccountStore.OnDiscoveryResponse
import org.wordpress.android.fluxc.model.jetpack.JetpackUser
import org.wordpress.android.fluxc.store.JetpackStore
import org.wordpress.android.fluxc.store.SiteStore
import org.wordpress.android.fluxc.store.SiteStore.OnSiteChanged
import org.wordpress.android.fluxc.store.SiteStore.RefreshSitesXMLRPCPayload
import org.wordpress.android.login.util.SiteUtils
import javax.inject.Inject

class AccountMismatchRepository @Inject constructor(
private val jetpackStore: JetpackStore,
private val siteStore: SiteStore,
private val wpApiSiteRepository: WPApiSiteRepository,
private val dispatcher: Dispatcher
) {
suspend fun getSiteByUrl(url: String): SiteModel? = withContext(Dispatchers.IO) {
SiteUtils.getSiteByMatchingUrl(siteStore, url)
}

fun removeSiteFromDB(site: SiteModel) {
dispatcher.dispatch(SiteActionBuilder.newRemoveSiteAction(site))
}

suspend fun fetchJetpackConnectionUrl(site: SiteModel): Result<String> {
WooLog.d(WooLog.T.LOGIN, "Fetching Jetpack Connection URL")
val result = jetpackStore.fetchJetpackConnectionUrl(site, autoRegisterSiteIfNeeded = true)
Expand All @@ -39,6 +36,7 @@ class AccountMismatchRepository @Inject constructor(
WooLog.w(WooLog.T.LOGIN, "Fetching Jetpack Connection URL failed: ${result.error.message}")
Result.failure(OnChangedException(result.error, result.error.message))
}

else -> {
WooLog.d(WooLog.T.LOGIN, "Jetpack connection URL fetched successfully")
Result.success(result.url)
Expand All @@ -48,21 +46,21 @@ class AccountMismatchRepository @Inject constructor(

suspend fun fetchJetpackConnectedEmail(site: SiteModel): Result<String> {
WooLog.d(WooLog.T.LOGIN, "Fetching email of Jetpack User")
val result = jetpackStore.fetchJetpackUser(site)
return when {
result.isError -> {
WooLog.w(WooLog.T.LOGIN, "Fetching Jetpack User failed error: $result.error.message")
Result.failure(OnChangedException(result.error, result.error.message))
}
result.user?.wpcomEmail.isNullOrEmpty() -> {
WooLog.w(WooLog.T.LOGIN, "Cannot find Jetpack Email in response")
Result.failure(Exception("Email missing from response"))
}
else -> {
WooLog.d(WooLog.T.LOGIN, "Jetpack User fetched successfully")
Result.success(result.user!!.wpcomEmail)

return fetchJetpackUser(site)
.onFailure {
WooLog.w(WooLog.T.LOGIN, "Fetching Jetpack User failed error: ${it.message}")
}.mapCatching {
val wpcomEmail = it?.wpcomEmail
if (wpcomEmail.isNullOrEmpty()) {
WooLog.w(WooLog.T.LOGIN, "Cannot find Jetpack Email in response")
@Suppress("TooGenericExceptionThrown")
throw Exception("Email missing from response")
} else {
WooLog.d(WooLog.T.LOGIN, "Jetpack User fetched successfully")
wpcomEmail
}
}
}
}

suspend fun checkJetpackConnection(
Expand All @@ -72,121 +70,39 @@ class AccountMismatchRepository @Inject constructor(
): Result<JetpackConnectionStatus> {
WooLog.d(WooLog.T.LOGIN, "Checking Jetpack Connection status for site $siteUrl")

return discoverXMLRPCAddress(siteUrl)
.mapCatching { xmlrpcEndpoint ->
val xmlrpcSite = fetchXMLRPCSite(siteUrl, xmlrpcEndpoint, username, password).getOrThrow()
when {
xmlrpcSite.jetpackUserEmail.isNullOrEmpty() -> {
WooLog.d(WooLog.T.LOGIN, "Jetpack site is not connected to a WPCom account")
JetpackConnectionStatus.NotConnected
}
else -> {
WooLog.d(
tag = WooLog.T.LOGIN,
message = "Jetpack site is connected to different WPCom account:" +
" ${xmlrpcSite.jetpackUserEmail}"
)
JetpackConnectionStatus.ConnectedToDifferentAccount(xmlrpcSite.jetpackUserEmail)
}
}
}
}

/**
* Represents Jetpack Connection status for the current wp-admin account
*/
sealed interface JetpackConnectionStatus {
object NotConnected : JetpackConnectionStatus
data class ConnectedToDifferentAccount(val wpcomEmail: String) : JetpackConnectionStatus
}

private suspend fun discoverXMLRPCAddress(siteUrl: String): Result<String> {
WooLog.d(WooLog.T.LOGIN, "Running discovery to fetch XMLRPC endpoint for site $siteUrl")

val action = AuthenticationActionBuilder.newDiscoverEndpointAction(siteUrl)
val event: OnDiscoveryResponse = dispatcher.dispatchAndAwait(action)

return if (event.isError) {
WooLog.w(WooLog.T.LOGIN, "XMLRPC Discovery failed, error: ${event.error}")
Result.failure(OnChangedException(event.error, event.error.name))
} else {
WooLog.d(
tag = WooLog.T.LOGIN,
message = "XMLRPC Discovery succeeded, xmrlpc endpoint: ${event.xmlRpcEndpoint}"
)
Result.success(event.xmlRpcEndpoint)
val site = wpApiSiteRepository.fetchSite(siteUrl, username, password).getOrElse {
WooLog.w(WooLog.T.LOGIN, "Site fetch failed, error: ${it.message}")
return Result.failure(it)
}
}

@Suppress("ReturnCount")
private suspend fun fetchXMLRPCSite(
siteUrl: String,
xmlrpcEndpoint: String,
username: String,
password: String
): Result<SiteModel> = coroutineScope {
val authenticationTask = async {
dispatcher.awaitEvent<OnAuthenticationChanged>().also { event ->
if (event.isError) {
WooLog.w(
tag = WooLog.T.LOGIN,
message = "Authenticating to XMLRPC site $xmlrpcEndpoint failed, " +
"error: ${event.error.message}"
)
}
return fetchJetpackUser(site).onFailure {
WooLog.w(WooLog.T.LOGIN, "Jetpack User fetch failed, error: ${it.message}")
}.map {
if (it?.isConnected != true) {
WooLog.w(WooLog.T.LOGIN, "Account is not connected to a WPCom account")
JetpackConnectionStatus.NotConnected
} else {
WooLog.d(WooLog.T.LOGIN, "Account is connected to different WPCom account: ${it.wpcomEmail}")
JetpackConnectionStatus.ConnectedToDifferentAccount(it.wpcomEmail)
}
}
val siteTask = async {
val selfHostedPayload = RefreshSitesXMLRPCPayload(
username = username,
password = password,
url = xmlrpcEndpoint
)
dispatcher.dispatchAndAwait<RefreshSitesXMLRPCPayload, OnSiteChanged>(
action = SiteActionBuilder.newFetchSitesXmlRpcAction(
selfHostedPayload
)
).also { event ->
if (event.isError) {
WooLog.w(
tag = WooLog.T.LOGIN,
message = "XMLRPC site $xmlrpcEndpoint fetch failed, error: ${event.error.message}"
)
} else {
WooLog.d(WooLog.T.LOGIN, "XMLRPC site $xmlrpcEndpoint fetch succeeded")
}
}
}

val tasks = listOf(authenticationTask, siteTask)

val event = tasks.awaitAny()

if (event.isError) return@coroutineScope Result.failure(OnChangedException(event.error))

return@coroutineScope fetchSiteOptions(siteUrl)
}

private suspend fun fetchSiteOptions(siteUrl: String): Result<SiteModel> {
WooLog.d(WooLog.T.LOGIN, "Fetch XMLRPC site options")
val site = getSiteByUrl(siteUrl) ?: run {
WooLog.e(WooLog.T.LOGIN, "XMLRPC site null after fetching!!")
return Result.failure(IllegalStateException("XMLRPC Site not found"))
}

val fetchSiteResult = siteStore.fetchSite(site)
return when {
fetchSiteResult.isError -> {
WooLog.w(
tag = WooLog.T.LOGIN,
message = "XMLRPC site $siteUrl fetch failed, error: ${fetchSiteResult.error.message}"
)
Result.failure(OnChangedException(fetchSiteResult.error, fetchSiteResult.error.message))
}
else -> {
WooLog.d(WooLog.T.LOGIN, "XMLRPC site $siteUrl fetch succeeded")
Result.success(getSiteByUrl(siteUrl)!!)
private suspend fun fetchJetpackUser(site: SiteModel): Result<JetpackUser?> {
return jetpackStore.fetchJetpackUser(site).let {
if (it.isError) {
Result.failure(OnChangedException(it.error, it.error.message))
} else {
Result.success(it.user)
}
}
}

/**
* Represents Jetpack Connection status for the current wp-admin account
*/
sealed interface JetpackConnectionStatus {
data object NotConnected : JetpackConnectionStatus
data class ConnectedToDifferentAccount(val wpcomEmail: String) : JetpackConnectionStatus
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
package com.woocommerce.android.ui.login.accountmismatch

import com.woocommerce.android.FakeDispatcher
import com.woocommerce.android.ui.login.WPApiSiteRepository
import com.woocommerce.android.viewmodel.BaseUnitTest
import kotlinx.coroutines.ExperimentalCoroutinesApi
import org.assertj.core.api.Assertions.assertThat
import org.junit.Test
import org.mockito.kotlin.any
import org.mockito.kotlin.eq
import org.mockito.kotlin.mock
import org.mockito.kotlin.whenever
import org.wordpress.android.fluxc.model.SiteModel
import org.wordpress.android.fluxc.model.jetpack.JetpackUser
import org.wordpress.android.fluxc.store.JetpackStore
import org.wordpress.android.fluxc.store.SiteStore

@OptIn(ExperimentalCoroutinesApi::class)
class AccountMismatchRepositoryTest : BaseUnitTest() {
private val jetpackStore = mock<JetpackStore>()
private val siteStore = mock<SiteStore>()
private val wpApiSiteRepository = mock<WPApiSiteRepository> {
onBlocking { fetchSite(any(), any(), any()) }.thenReturn(Result.success(SiteModel()))
}
private val dispatcher = FakeDispatcher()

private val sut = AccountMismatchRepository(
jetpackStore = jetpackStore,
siteStore = siteStore,
wpApiSiteRepository = wpApiSiteRepository,
dispatcher = dispatcher
)

@Test
fun `given a non-connected Jetpack Account, when fetching status, then return correct status`() = testBlocking {
whenever(jetpackStore.fetchJetpackUser(any(), eq(false)))
.thenReturn(JetpackStore.JetpackUserResult(createJetpackUser(isConnected = false)))

val result = sut.checkJetpackConnection(
siteUrl = "https://example.com",
username = "username",
password = "password"
)

assertThat(result.getOrNull()).isEqualTo(AccountMismatchRepository.JetpackConnectionStatus.NotConnected)
}

@Test
fun `given a null jetpack user, when fetching connection status, then assume non-connected`() = testBlocking {
whenever(jetpackStore.fetchJetpackUser(any(), eq(false)))
.thenReturn(JetpackStore.JetpackUserResult(null))

val result = sut.checkJetpackConnection(
siteUrl = "https://example.com",
username = "username",
password = "password"
)

assertThat(result.getOrNull()).isEqualTo(AccountMismatchRepository.JetpackConnectionStatus.NotConnected)
}

@Test
fun `given a connected Jetpack Account, when fetching status, then return correct status`() = testBlocking {
whenever(jetpackStore.fetchJetpackUser(any(), eq(false)))
.thenReturn(JetpackStore.JetpackUserResult(createJetpackUser(isConnected = true, wpcomEmail = "email")))

val result = sut.checkJetpackConnection(
siteUrl = "https://example.com",
username = "username",
password = "password"
)

assertThat(result.getOrNull())
.isEqualTo(AccountMismatchRepository.JetpackConnectionStatus.ConnectedToDifferentAccount("email"))
}

@Test
fun `given a correctly connected Jetpack account, when fetching email, then return it`() = testBlocking {
whenever(jetpackStore.fetchJetpackUser(any(), eq(false)))
.thenReturn(JetpackStore.JetpackUserResult(createJetpackUser(isConnected = true, wpcomEmail = "email")))

val result = sut.fetchJetpackConnectedEmail(SiteModel())

assertThat(result.getOrNull()).isEqualTo("email")
}

@Test
fun `given an issue with Jetpack account, when fetching email, then return error`() = testBlocking {
whenever(jetpackStore.fetchJetpackUser(any(), eq(false)))
.thenReturn(JetpackStore.JetpackUserResult(createJetpackUser(isConnected = true, wpcomEmail = "")))

val result = sut.fetchJetpackConnectedEmail(SiteModel())

assertThat(result.isFailure).isTrue()
}

private fun createJetpackUser(
isConnected: Boolean = false,
wpcomEmail: String = ""
) = JetpackUser(
isConnected = isConnected,
wpcomEmail = wpcomEmail,
isMaster = false,
username = "username",
wpcomId = 1L,
wpcomUsername = "wpcomUsername"
)
}

0 comments on commit 51f62d3

Please sign in to comment.