Skip to content
This repository was archived by the owner on Feb 4, 2025. It is now read-only.

Woo REST API: fix Woo detection for non-eligible users #2628

Merged
merged 2 commits into from
Jan 12, 2023
Merged

Woo REST API: fix Woo detection for non-eligible users #2628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82a9b48
Fix logic of Woo detection to work with non-eligible users
hichamboushaba Jan 10, 2023
ed3c683
Offload fetching sites to a background thread
hichamboushaba Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions fluxc/src/main/java/org/wordpress/android/fluxc/store/SiteStore.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1341,12 +1341,15 @@ open class SiteStore @Inject constructor(
}

suspend fun fetchSite(site: SiteModel): OnSiteChanged {
val updatedSite = if (site.isUsingWpComRestApi) {
siteRestClient.fetchSite(site)
} else {
siteXMLRPCClient.fetchSite(site)
return coroutineEngine.withDefaultContext(T.API, this, "Fetch site") {
val updatedSite = if (site.isUsingWpComRestApi) {
siteRestClient.fetchSite(site)
} else {
siteXMLRPCClient.fetchSite(site)
}

updateSite(updatedSite)
Comment on lines +1344 to +1351
Copy link
Member Author

@hichamboushaba hichamboushaba Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions in the SiteStore are suspendable, and yet, the DB logic wasn't offloaded correctly to the background thread, with this change, we follow the convention that the suspendable functions in the Stores use the default context.

This exact issue was fixed for fetchSites in #2254, and this PR updates the two other fetch functions.

}
return updateSite(updatedSite)
}

suspend fun fetchSites(payload: FetchSitesPayload): OnSiteChanged {
Expand All @@ -1357,7 +1360,9 @@ open class SiteStore @Inject constructor(
}

suspend fun fetchSitesXmlRpc(payload: RefreshSitesXMLRPCPayload): OnSiteChanged {
return updateSites(siteXMLRPCClient.fetchSites(payload.url, payload.username, payload.password))
return coroutineEngine.withDefaultContext(T.API, this, "Fetch sites") {
updateSites(siteXMLRPCClient.fetchSites(payload.url, payload.username, payload.password))
}
}

@Suppress("ForbiddenComment", "SwallowedException")
Expand Down
18 changes: 13 additions & 5 deletions ...in/kotlin/org/wordpress/android/fluxc/network/rest/wpcom/wc/system/WooSystemRestClient.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import com.google.gson.annotations.SerializedName
import org.wordpress.android.fluxc.generated.endpoint.WOOCOMMERCE
import org.wordpress.android.fluxc.generated.endpoint.WPAPI
import org.wordpress.android.fluxc.model.SiteModel
import org.wordpress.android.fluxc.network.BaseRequest.GenericErrorType.NOT_FOUND
import org.wordpress.android.fluxc.network.rest.wpapi.WPAPIResponse
import org.wordpress.android.fluxc.network.rest.wpcom.wc.WooNetwork
import org.wordpress.android.fluxc.network.rest.wpcom.wc.WooPayload
Expand All @@ -14,6 +13,11 @@ import org.wordpress.android.fluxc.utils.toWooPayload
import javax.inject.Inject

class WooSystemRestClient @Inject constructor(private val wooNetwork: WooNetwork) {
companion object {
private const val NOT_FOUND = 404
private const val FORBIDDEN = 403
}

suspend fun fetchInstalledPlugins(site: SiteModel): WooPayload<WCSystemPluginResponse> {
val url = WOOCOMMERCE.system_status.pathV3

Expand Down Expand Up @@ -59,10 +63,14 @@ class WooSystemRestClient @Inject constructor(private val wooNetwork: WooNetwork
return when (response) {
is WPAPIResponse.Success -> WooPayload(true)
is WPAPIResponse.Error -> {
if (response.error.type == NOT_FOUND) {
WooPayload(false)
} else {
WooPayload(response.error.toWooError())
when (response.error.volleyError?.networkResponse?.statusCode) {
NOT_FOUND -> WooPayload(false)
FORBIDDEN -> {
// If we get a 403 status code, we can infer that Woo is installed,
// the user just doesn't have permission to access the Woo API
Copy link
Contributor

@hafizrahman hafizrahman Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to discuss:

when a user doesn't get a permission to access API, is this exclusively because of their user role? Or can it be because of security plugins blocking certain users from using API, for example?

If there can be multiple reasons, do they need to be handled in this PR (maybe be clarified in comments)? I'm mostly asking because this PR seems to be tied to a task specifically about checking user role.

Copy link
Member Author

@hichamboushaba hichamboushaba Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when a user doesn't get a permission to access API, is this exclusively because of their user role? Or can it be because of security plugins blocking certain users from using API, for example?

We don't care about the cause here, our goal of this code is just to know whether Woo is installed or not, what happens later depends on other parts of the app: for example, we handle the role check, but if the cause is different, then the behavior won't be defined, same as what happens when accessing the site using WordPress.com or Jeptack CP.

Another point to keep in mind: this is needed just because we fetch the site using XMLRPC for now, if we get a chance to work on improving this part, we will handle the Woo detection differently, as we will be able to infer whether its present or not from fetching the root /wp-json/ endpoint, which will be used also to fetch site details (check my discussion with Huong here).

Copy link
Contributor

@hafizrahman hafizrahman Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanation!

WooPayload(true)
}
else -> WooPayload(response.error.toWooError())
}
}
}
Expand Down