Skip to content

REST API: Check for Woo after authentication #8525

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 23 commits into from
Jan 4, 2023
Merged

REST API: Check for Woo after authentication #8525

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
3bf6772
Enable application password for fetching site plugin details
itsmeichigo Jan 2, 2023
6845228
Check for woo plugin after authentication
itsmeichigo Jan 2, 2023
d8b8e64
Show alert when Woo check fails
itsmeichigo Jan 2, 2023
1047167
Use default alert for site credential login failures
itsmeichigo Jan 2, 2023
cd1fb1c
Revert changes to FancyAlertViewController
itsmeichigo Jan 2, 2023
3b201ca
Remove formatting for no Woo error
itsmeichigo Jan 2, 2023
2295109
Remove alert title for site credential failure
itsmeichigo Jan 2, 2023
736e59b
Move login checker to a new file
itsmeichigo Jan 3, 2023
deecb1c
Revert "Enable application password for fetching site plugin details"
itsmeichigo Jan 3, 2023
a141eec
Add woo check to settings remote and store
itsmeichigo Jan 3, 2023
6c527d6
Check woo with site settings
itsmeichigo Jan 3, 2023
aec51ca
Simplify code with less guard let self
itsmeichigo Jan 3, 2023
5939c7f
Refactor PostSiteCredentialLoginChecker for testability
itsmeichigo Jan 3, 2023
81a4e03
Add tests for PostSiteCredentialLoginCheckerTests's application passw…
itsmeichigo Jan 3, 2023
3754169
Add tests for role eligibility check
itsmeichigo Jan 3, 2023
2697edd
Revert "Add woo check to settings remote and store"
itsmeichigo Jan 3, 2023
e2eebbd
Check for woo in WordPressSite
itsmeichigo Jan 3, 2023
eaf0b7a
Check for Woo by fetching WordPressSite
itsmeichigo Jan 3, 2023
e33605d
Remove redundant line
itsmeichigo Jan 3, 2023
2499757
Add tests for woo check
itsmeichigo Jan 3, 2023
1f78ccd
Add namespaces to the mock response for wordpress site
itsmeichigo Jan 3, 2023
c9a5fa3
Add more checks to WordPressSiteMapperTests
itsmeichigo Jan 3, 2023
4d2afb8
Fix unit test failure for WordPressSiteStoreTests
itsmeichigo Jan 3, 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
2 changes: 1 addition & 1 deletion Networking/Networking/Remote/SitePluginsRemote.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public class SitePluginsRemote: Remote {
pluginName: String,
completion: @escaping (Result<SitePlugin, Error>) -> Void) {
let path = String(format: "%@/%@", Constants.sitePluginsPath, pluginName)
let request = JetpackRequest(wooApiVersion: .none, method: .get, siteID: siteID, path: path, parameters: nil)
let request = JetpackRequest(wooApiVersion: .none, method: .get, siteID: siteID, path: path, parameters: nil, availableAsRESTRequest: true)
let mapper = SitePluginMapper(siteID: siteID)
enqueue(request, mapper: mapper, completion: completion)
}
Expand Down
88 changes: 77 additions & 11 deletions WooCommerce/Classes/Authentication/AuthenticationManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import KeychainAccess
import WordPressAuthenticator
import WordPressKit
import Yosemite
import WordPressUI
import class Networking.UserAgent
import enum Experiments.ABTest
import struct Networking.Settings
Expand All @@ -12,6 +11,7 @@ import protocol Storage.StorageManagerType
import protocol Networking.ApplicationPasswordUseCase
import class Networking.DefaultApplicationPasswordUseCase
import enum Networking.ApplicationPasswordUseCaseError
import enum Alamofire.AFError

/// Encapsulates all of the interactions with the WordPress Authenticator
///
Expand Down Expand Up @@ -743,7 +743,7 @@ private extension AuthenticationManager {
/// The error screen to be displayed when the user tries to enter a site without WooCommerce.
///
func noWooUI(for site: Site,
with matcher: ULAccountMatcher,
with matcher: ULAccountMatcher = .init(),
navigationController: UINavigationController,
onStorePickerDismiss: @escaping () -> Void) -> UIViewController {
let viewModel = NoWooErrorViewModel(
Expand Down Expand Up @@ -836,9 +836,11 @@ private extension AuthenticationManager {
guard let self else { return }
self.checkRoleEligibility(in: navigationController) { [weak self] in
guard let self else { return }
// TODO: check for Woo
// navigates to home screen immediately with a placeholder store ID
self.startStorePicker(with: WooConstants.placeholderStoreID, in: navigationController)
self.checkWooInstallation(in: navigationController) { [weak self] in
guard let self else { return }
// navigates to home screen immediately with a placeholder store ID
self.startStorePicker(with: WooConstants.placeholderStoreID, in: navigationController)
}
}
}
}
Expand All @@ -862,12 +864,12 @@ private extension AuthenticationManager {
// show generic error
await MainActor.run {
DDLogError("⛔️ Error generating application password: \(error)")
let alert = FancyAlertViewController.makeSiteCredentialLoginAlert(
let alert = self.siteCredentialLoginAlert(
message: Localization.applicationPasswordError,
retryAction: { [weak self] in
onRetry: { [weak self] in
self?.checkApplicationPassword(for: siteURL, with: useCase, in: navigationController, onSuccess: onSuccess)
},
restartLoginAction: {
onRestartLogin: {
ServiceLocator.stores.deauthenticate()
navigationController.popToRootViewController(animated: true)
}
Expand Down Expand Up @@ -896,12 +898,12 @@ private extension AuthenticationManager {
} else {
// show generic error
DDLogError("⛔️ Error checking role eligibility: \(error)")
let alert = FancyAlertViewController.makeSiteCredentialLoginAlert(
let alert = self.siteCredentialLoginAlert(
message: Localization.roleEligibilityCheckError,
retryAction: { [weak self] in
onRetry: { [weak self] in
self?.checkRoleEligibility(in: navigationController, onSuccess: onSuccess)
},
restartLoginAction: {
onRestartLogin: {
ServiceLocator.stores.deauthenticate()
navigationController.popToRootViewController(animated: true)
}
Expand All @@ -928,6 +930,57 @@ private extension AuthenticationManager {
}
navigationController.show(errorViewController, sender: self)
}

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

@hichamboushaba hichamboushaba Jan 2, 2023

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

@itsmeichigo itsmeichigo Jan 3, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Member

@hichamboushaba hichamboushaba Jan 3, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@itsmeichigo itsmeichigo Jan 3, 2023

Choose a reason for hiding this comment

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

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

var errorMessage: String?
switch result {
case .success(let plugin):
if plugin.status == .active {
return onSuccess()
} else {
errorMessage = Localization.noWooError
}
case .failure(let error):
DDLogError("⛔️ Error checking Woo: \(error)")
if case .responseValidationFailed(reason: .unacceptableStatusCode(code: 404)) = error as? AFError {
errorMessage = Localization.noWooError
} else {
errorMessage = Localization.wooCheckError
}
}
if let errorMessage {
let alert = self.siteCredentialLoginAlert(message: errorMessage,
onRestartLogin: {
ServiceLocator.stores.deauthenticate()
navigationController.popToRootViewController(animated: true)
}
)
navigationController.present(alert, animated: true)
}
}
ServiceLocator.stores.dispatch(action)
}

func siteCredentialLoginAlert(message: String,
onRetry: (() -> Void)? = nil,
onRestartLogin: @escaping () -> Void) -> UIAlertController {
let alert = UIAlertController(title: message,
message: nil,
preferredStyle: .alert)
if let onRetry {
let retryAction = UIAlertAction(title: Localization.retryButton, style: .default) { _ in
onRetry()
}
alert.addAction(retryAction)
}
let restartAction = UIAlertAction(title: Localization.restartLoginButton, style: .cancel) { _ in
onRestartLogin()
}
alert.addAction(restartAction)
return alert
}
}

private extension AuthenticationManager {
Expand All @@ -940,6 +993,19 @@ private extension AuthenticationManager {
"Error fetching user information.",
comment: "Error message displayed when user information cannot be fetched after authentication."
)
static let noWooError = NSLocalizedString(
"It looks like this is not a WooCommerce site.",
comment: "Message explaining that the site entered doesn't have WooCommerce installed or activated."
)
static let wooCheckError = NSLocalizedString(
"Error checking for the WooCommerce plugin.",
comment: "Error message displayed when the WooCommerce plugin detail cannot be fetched after authentication"
)
static let retryButton = NSLocalizedString("Try Again", comment: "Button to refetch application password for the current site")
static let restartLoginButton = NSLocalizedString("Log In With Another Account", comment: "Button to restart the login flow.")
}
enum Constants {
static let wooPluginName = "woocommerce/woocommerce"
}
}

Expand Down
35 changes: 0 additions & 35 deletions WooCommerce/Classes/ViewRelated/Fancy Alerts/FancyAlertViewController+UnifiedLogin.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,21 +53,6 @@ extension FancyAlertViewController {
let controller = FancyAlertViewController.controllerWithConfiguration(configuration: config)
return controller
}

static func makeSiteCredentialLoginAlert(message: String,
retryAction: @escaping () -> Void,
restartLoginAction: @escaping () -> Void) -> FancyAlertViewController {
let retryButton = makeRetryButtonConfig(retryAction: retryAction)
let loginButton = makeLoginButtonConfig(loginAction: restartLoginAction)
let config = FancyAlertViewController.Config(titleText: Localization.cannotLogin,
bodyText: message,
headerImage: nil,
dividerPosition: .top,
defaultButton: loginButton,
cancelButton: retryButton)
let controller = FancyAlertViewController.controllerWithConfiguration(configuration: config)
return controller
}
}


Expand Down Expand Up @@ -122,12 +107,6 @@ private extension FancyAlertViewController {
comment: "Description of alert for suggestion on how to connect to a WP.com site" +
"Presented when a user logs in with an email that does not have access to a WP.com site"
)
static let retryButton = NSLocalizedString("Try Again", comment: "Button to refetch application password for the current site")
static let retryLoginButton = NSLocalizedString("Log In With Another Account", comment: "Button to restart the login flow.")
static let cannotLogin = NSLocalizedString(
"Cannot log in",
comment: "Title of the alert displayed when application password cannot be fetched after authentication"
)
}

enum Strings {
Expand All @@ -145,20 +124,6 @@ private extension FancyAlertViewController {
}
}

static func makeRetryButtonConfig(retryAction: @escaping () -> Void) -> FancyAlertViewController.Config.ButtonConfig {
return FancyAlertViewController.Config.ButtonConfig(Localization.retryButton) { controller, _ in
controller.dismiss(animated: true)
retryAction()
}
}

static func makeLoginButtonConfig(loginAction: @escaping () -> Void) -> FancyAlertViewController.Config.ButtonConfig {
return FancyAlertViewController.Config.ButtonConfig(Localization.retryLoginButton) { controller, _ in
controller.dismiss(animated: true)
loginAction()
}
}

static func makeLearnMoreAboutJetpackButtonConfig(analytics: Analytics) -> FancyAlertViewController.Config.ButtonConfig {
return FancyAlertViewController.Config.ButtonConfig(Localization.learnMore) { controller, _ in
guard let url = URL(string: Strings.whatsJetpackURLString) else {
Expand Down