REST API: Check for Woo after authentication #8525

itsmeichigo · 2023-01-02T12:30:59Z

Part of #8453

Description

This PR adds a check for Woo after authenticating the user with site credentials only. Following Hicham's suggestion in a comment below, the check is handled by calling the logged in site's root endpoint and checking for any namespaces starting with wc/.

For simplicity, we are only showing an alert when Woo is not installed for now. To improve the UX, we'll need to handle Woo installation in the future.

Also, for readability and testability, all the checks for #8453 are now moved to a separate class named PostSiteCredentialLoginChecker. This along with the unit tests cause the PR to be slightly larger, sorry if this causes any inconvenience for reviewing.

I've also replaced the used of the FancyAlertController with UIAlertController for a more intuitive look.

Tracks events will be added in a subsequent PR.

Testing instructions

1. Without active WooCommerce:
Prerequisite: Make sure that application password is enabled in your test site. Deactivate or uninstall WooCommerce from the site.

Enable the feature flag applicationPasswordAuthenticationForSiteCredentialLogin and build the app.
Log out of the app if needed.
On the prologue screen, select "Enter your site address". If this option is not available, disable simplified login and build the app again.
Enter your test store address and select "Sign in with site credentials".
Enter the correct credentials for a user account with an eligible role and select "Continue".
Notice that an alert will then be displayed asking to install WooCommerce to the site.

2. With issue fetching site information:
Prerequisite: Make sure that application password is enabled on your site.

Enable the feature flag applicationPasswordAuthenticationForSiteCredentialLogin and build the app.
Log out of the app if needed.
On the prologue screen, select "Enter your site address". If this option is not available, disable simplified login and build the app again.
Enter your test store address and select "Sign in with site credentials".
Enter the correct credentials for a user account with an eligible role and select "Continue".
Use Proxyman to intercept the site fetch request (/wp-json) or disable your network connection when this request is made.
Notice that an alert will then be displayed saying there is a problem fetching the WooCommerce plugin.
Tap the "Retry" button on the alert, notice that the alert is dismissed. If the issue persists, the same alert should be presented again.
Tap the "Log In With Another Account" button. Notice that you are navigated to the prologue screen to log in again.

Please feel free to test again with a site that has WooCommerce activated. You should be able to get to the home screen after logging in.

Screenshots

Woo not active	Problem fetching site

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

wpmobilebot · 2023-01-02T12:54:27Z

You can test the changes from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr8525-4d2afb8 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

hichamboushaba · 2023-01-02T14:23:25Z

WooCommerce/Classes/Authentication/AuthenticationManager.swift

+
+    func checkWooInstallation(in navigationController: UINavigationController,
+                              onSuccess: @escaping () -> Void) {
+        let action = SitePluginAction.getPluginDetails(siteID: WooConstants.placeholderStoreID, pluginName: Constants.wooPluginName) { result in


@itsmeichigo I was just taking a look to compare how the implementation differs between the two platforms, and if I'm understanding things correctly, you are using the endpoint /wp/v2/plugins to check if Woo is installed here, I don't think this can work for all cases, users with the role "shop manager" can't use this endpoint.

As detailed in the internal doc pe5sF9-U3-p2, there are two ways to check if Woo is installed for all the users. Currently, in Android we are re-using the same check as Jetpack CP sites (meaning /wc/v3/settings), but if we decide to remove XMLRPC, we might try to move to use the root endpoint /wp-json/ in order to reduce the number of requests we send.

Thanks for looking into my code @hichamboushaba! Could you clarify how we can use the root endpoint to check for Woo please? And how does it have anything to do with XMLRPC?

Could you clarify how we can use the root endpoint to check for Woo please?

This would be the same as we do for checking the Woo API version (here), the root endpoint returns an array of the available namespaces of APIs in the site, and if it contains /wc/{something}, we can infer that the site has WooCommerce.

And how does it have anything to do with XMLRPC?

Sorry for not being clear about this, this is specific to Android, currently the site data is being fetched using XMLRPC, so as long as we don't touch this, it made sense to me to reuse the same WooCommerce check that we use for Jetpack CP sites. But if we decide to remove XMLRPC, we will use the root endpoint to fetch site data, so in order to save one request, I would use this same request to infer if Woo is installed or not too, and stop using /wc/v3/settings.

Thanks for the detailed explanation Hicham! I'll use the root endpoint to check too, for simplicity 🙇

This reverts commit 3bf6772.

…ord check

This reverts commit a141eec.

selanthiraiyan

LGTM 🚢

I left a couple of non-blocking nits.

selanthiraiyan · 2023-01-04T03:14:07Z

WooCommerce/Classes/Authentication/PostSiteCredentialLoginChecker.swift

+            self?.checkRoleEligibility(in: navigationController) {
+                self?.checkWooInstallation(for: siteURL, in: navigationController, onSuccess: onSuccess)


❓ Can we send these requests in parallel? Do we have to wait for the role eligibility response before checking Woo installation?

I think we want to avoid presenting a few alerts at the same time in case there is more than one issue with the checks. If we want to call everything concurrently, we'll need another mechanism to check the results and prioritize the alerts we want to display. For simplicity, I'm using nested closures for now, I hope it's alright.

I agree. 👍

selanthiraiyan · 2023-01-04T03:32:13Z

WooCommerce/WooCommerceTests/Authentication/PostSiteCredentialLoginCheckerTests.swift

+
+/// MOCK: application password use case
+///
+private final class MockApplicationPasswordUseCase: ApplicationPasswordUseCase {


Nit: Could we move this to a separate file and use it here and in RequestAuthenticatorTests?

I'm not sure if we should import NetworkingTests from WooCommerceTests? Or should we move this code to Networking` instead? @selanthiraiyan

Oh, I missed that these files reside in separate frameworks. Let us keep them separate. 😅

itsmeichigo added 6 commits January 2, 2023 18:39

Enable application password for fetching site plugin details

3bf6772

Check for woo plugin after authentication

6845228

Show alert when Woo check fails

d8b8e64

Use default alert for site credential login failures

1047167

Revert changes to FancyAlertViewController

cd1fb1c

Remove formatting for no Woo error

3b201ca

itsmeichigo added type: task An internally driven task. feature: login Related to any part of the log in or sign in flow, or authentication. status: feature-flagged Behind a feature flag. Milestone is not strongly held. labels Jan 2, 2023

Remove alert title for site credential failure

2295109

hichamboushaba reviewed Jan 2, 2023

View reviewed changes

itsmeichigo added 13 commits January 3, 2023 11:23

Move login checker to a new file

736e59b

Revert "Enable application password for fetching site plugin details"

deecb1c

This reverts commit 3bf6772.

Add woo check to settings remote and store

a141eec

Check woo with site settings

6c527d6

Simplify code with less guard let self

aec51ca

Refactor PostSiteCredentialLoginChecker for testability

5939c7f

Add tests for PostSiteCredentialLoginCheckerTests's application passw…

81a4e03

…ord check

Add tests for role eligibility check

3754169

Revert "Add woo check to settings remote and store"

2697edd

This reverts commit a141eec.

Check for woo in WordPressSite

e2eebbd

Check for Woo by fetching WordPressSite

eaf0b7a

Remove redundant line

e33605d

Add tests for woo check

2499757

itsmeichigo marked this pull request as ready for review January 3, 2023 09:12

itsmeichigo requested a review from selanthiraiyan January 3, 2023 09:12

itsmeichigo added this to the 11.8 milestone Jan 3, 2023

itsmeichigo added 2 commits January 3, 2023 18:56

Add namespaces to the mock response for wordpress site

1f78ccd

Add more checks to WordPressSiteMapperTests

c9a5fa3

Fix unit test failure for WordPressSiteStoreTests

4d2afb8

Base automatically changed from feat/8453-role-eligibility to trunk January 3, 2023 11:27

selanthiraiyan self-assigned this Jan 3, 2023

selanthiraiyan approved these changes Jan 4, 2023

View reviewed changes

itsmeichigo merged commit 5a0b846 into trunk Jan 4, 2023

itsmeichigo deleted the feat/8453-woo-check branch January 4, 2023 06:32

hichamboushaba mentioned this pull request Jan 12, 2023

Woo REST API: fix Woo detection for non-eligible users wordpress-mobile/WordPress-FluxC-Android#2628

Merged

		self?.checkRoleEligibility(in: navigationController) {
		self?.checkWooInstallation(for: siteURL, in: navigationController, onSuccess: onSuccess)

REST API: Check for Woo after authentication #8525

REST API: Check for Woo after authentication #8525

Uh oh!

Conversation

itsmeichigo commented Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Screenshots

Uh oh!

wpmobilebot commented Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

selanthiraiyan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

itsmeichigo commented Jan 2, 2023 •

edited

Loading

wpmobilebot commented Jan 2, 2023 •

edited

Loading