Skip to content
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.

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

[Login] Improve error messaging displaying a new error screen when application password is disabled #15031

Open
wants to merge 24 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
cf69dbb
First pass at converting error dialog into an error screen for disabl…
hafizrahman Oct 31, 2024
74f3268
Merge branch 'trunk' into issue/14004-better-application-password-dis…
pmusolino Jan 31, 2025
b292c92
Update navigation logic and button titles in ApplicationPasswordDisab…
pmusolino Jan 31, 2025
170d58e
update: localization keys and message formatting in `ApplicationPassw…
pmusolino Jan 31, 2025
2887491
update: authentication tests and localization keys in `ApplicationPas…
pmusolino Jan 31, 2025
43e803d
clean: removed TODO comment because the track already happen somewher…
pmusolino Jan 31, 2025
965c4d3
update: release-notes 21.7
pmusolino Jan 31, 2025
58faf5e
update: added PR url in 21.7 release notes
pmusolino Jan 31, 2025
79d524a
Merge branch 'trunk' into issue/14004-better-application-password-dis…
pmusolino Feb 3, 2025
3f40c59
fix: remove unused code
pmusolino Feb 3, 2025
9b480ba
Add navigation to previous view controller in application password flow
pmusolino Feb 3, 2025
98810f3
Merge branch 'trunk' into issue/14004-better-application-password-dis…
pmusolino Feb 3, 2025
6467c96
Update WooCommerce/Classes/Authentication/Application Password/Applic…
pmusolino Feb 4, 2025
71d68ee
Update WooCommerce/Classes/Authentication/Navigation Exceptions/Appli…
pmusolino Feb 4, 2025
ec50237
fix: naming
pmusolino Feb 4, 2025
6bdb62d
Add previousViewController parameter for navigation
pmusolino Feb 4, 2025
fc9335d
Add previousViewController parameter to PostSiteCredentialLoginChecke…
pmusolino Feb 4, 2025
3e5b2ad
Merge branch 'trunk' into issue/14004-better-application-password-dis…
pmusolino Feb 5, 2025
daee4c0
feat: remove weak from `ApplicationPasswordAuthorizationWebViewContro…
pmusolino Feb 5, 2025
dc68ebc
update: navigation logic in `ApplicationPasswordDisabledViewModel`: c…
pmusolino Feb 5, 2025
4ae12d2
Merge branch 'trunk' into issue/14004-better-application-password-dis…
pmusolino Feb 5, 2025
9fafc53
Refactor error handling in `ApplicationPasswordAuthorizationWebViewCo…
pmusolino Feb 6, 2025
9e4663e
Merge branch 'trunk' into issue/14004-better-application-password-dis…
pmusolino Feb 7, 2025
2c7f417
Merge branch 'trunk' into issue/14004-better-application-password-dis…
pmusolino Feb 7, 2025
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
5 changes: 5 additions & 0 deletions RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
*** PLEASE FOLLOW THIS FORMAT: [<priority indicator, more stars = higher priority>] <description> [<PR URL>]
*** Use [*****] to indicate smoke tests of all critical flows should be run on the final IPA before release (e.g. major library or OS update).

21.7
-----
- [*] Better error messages if Application Password login is disabled on user's website. [https://github.com/woocommerce/woocommerce-ios/pull/15031]

21.6
-----
- [*] Payments: improve retry handling when card reader updates fail due to low battery [https://github.com/woocommerce/woocommerce-ios/pull/14990]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import Yosemite
/// View model for `ApplicationPasswordAuthorizationWebViewController`.
///
final class ApplicationPasswordAuthorizationViewModel {
private let siteURL: String
let siteURL: String
private let stores: StoresManager

init(siteURL: String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,20 @@ private extension ApplicationPasswordAuthorizationWebViewController {
guard let url = try await viewModel.fetchAuthURL() else {
DDLogError("⛔️ No authorization URL found for application passwords")
analytics.track(.applicationPasswordAuthorizationURLNotAvailable)
return showErrorAlert(message: Localization.applicationPasswordDisabled)

let errorUI = applicationPasswordDisabledUI(for: viewModel.siteURL)
// When the error view controller is popped, also pop the web view
errorUI.navigationItem.leftBarButtonItem = UIBarButtonItem(
image: UIImage(systemName: "chevron.backward"),
style: .plain,
target: self,
action: #selector(popBothControllers)
)

// Push instead of present
navigationController?.pushViewController(errorUI, animated: true)

return
}
loadAuthorizationPage(url: url)
} catch {
Expand All @@ -171,6 +184,12 @@ private extension ApplicationPasswordAuthorizationWebViewController {
}
}

@objc private func popBothControllers() {
// Pop back two view controllers to remove both error and web view
navigationController?.popViewController(animated: false)
navigationController?.popViewController(animated: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is hacky and prone to error. How about looking for the index of ApplicationPasswordTutorialViewController in the navigation stack, and pop to the index before that?

A better option would be to send the view controller before the application password flow to this view controller via presentApplicationPasswordWebView in AuthenticationManager.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with you and with your second suggestion, so I applied the new changes in this commit: 9b480ba

}

func loadAuthorizationPage(url: URL) {
let parameters: [URLQueryItem] = [
URLQueryItem(name: Constants.Query.appName, value: appName),
Expand Down Expand Up @@ -220,6 +239,14 @@ private extension ApplicationPasswordAuthorizationWebViewController {
}
present(alertController, animated: true)
}

/// The error screen to be displayed when the user tries to log in with site credentials
/// with application password disabled.
///
func applicationPasswordDisabledUI(for siteURL: String) -> UIViewController {
let viewModel = ApplicationPasswordDisabledViewModel(siteURL: siteURL)
return ULErrorViewController(viewModel: viewModel)
}
}

extension ApplicationPasswordAuthorizationWebViewController: WKNavigationDelegate {
Expand Down Expand Up @@ -254,6 +281,13 @@ extension ApplicationPasswordAuthorizationWebViewController: WKNavigationDelegat
}
}

extension ApplicationPasswordAuthorizationWebViewController: UIAdaptivePresentationControllerDelegate {
func presentationControllerDidDismiss(_ presentationController: UIPresentationController) {
// This will be called when the error UI is dismissed
navigationController?.dismiss(animated: true)
}
}

pmusolino marked this conversation as resolved.
Show resolved Hide resolved
private extension ApplicationPasswordAuthorizationWebViewController {
enum Constants {
static let successURL = "woocommerce://application-password"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,15 @@ import WordPressAuthenticator
/// modeling an error when application password is disabled.
///
struct ApplicationPasswordDisabledViewModel: ULErrorViewModel {
init(siteURL: String) {
init(siteURL: String,
authentication: Authentication = ServiceLocator.authenticationManager) {
self.siteURL = siteURL
self.authentication = authentication
}

let siteURL: String
let image: UIImage = .errorImage // TODO: update this if needed
let authentication: Authentication
let image: UIImage = .errorImage

var text: NSAttributedString {
let font: UIFont = .body
Expand All @@ -20,7 +23,7 @@ struct ApplicationPasswordDisabledViewModel: ULErrorViewModel {
attributes: [.font: boldFont])
let message = NSMutableAttributedString(string: Localization.errorMessage)

message.replaceFirstOccurrence(of: "%@", with: boldSiteAddress)
message.replaceFirstOccurrence(of: "%1$@", with: boldSiteAddress)

return message
}
Expand All @@ -34,14 +37,18 @@ struct ApplicationPasswordDisabledViewModel: ULErrorViewModel {
let secondaryButtonTitle = Localization.secondaryButtonTitle

func viewDidLoad(_ viewController: UIViewController?) {
// TODO: add tracks if necessary
}

// Navigates back to the third last view controller in the stack if possible,
// otherwise it navigates to the root view controller.
func didTapPrimaryButton(in viewController: UIViewController?) {
guard let viewController else {
return
guard let navigationController = viewController?.navigationController else { return }
let viewControllers = navigationController.viewControllers
if viewControllers.count >= 3 {
navigationController.popToViewController(viewControllers[viewControllers.count - 3], animated: true)
} else {
navigationController.popToRootViewController(animated: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you want to pop to the previous view controller, popToRoot seems incorrect. Should it be popViewController?

Copy link
Member Author

Choose a reason for hiding this comment

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

When I implemented it, during my testing process (at least in every test I did), the previous view controller immediately before the error screen was the loading screen—the one with just a loader in the middle of the view. If we pop back to it (by calling popViewController), the user would remain stuck there, so we need to go back further to ensure a better user experience.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. From my understanding, you already sent the previousViewController for the application password flow, so this should not happen. Also, you are sending nil for the site credential login flow, so tapping "Try Again" should navigate back to the login screen instead of the prologue screen IMO. Is there any other edge case we should be aware of?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, in theory, there are no other edge cases and I didn't find any other specific edge cases so I'm putting popViewController. 👍

}
Copy link
Contributor

Choose a reason for hiding this comment

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

The number 3 seems to be a magic number, it's not clear why we have to do this. Please consider my suggestion above about looking for the index of a specific view controller to pop to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now I'm using the previousVC like in the comment above, please feel free to check changes here: 9b480ba 🙏

WordPressAuthenticator.showLoginForJustWPCom(from: viewController)
}

func didTapSecondaryButton(in viewController: UIViewController?) {
Expand All @@ -55,28 +62,48 @@ struct ApplicationPasswordDisabledViewModel: ULErrorViewModel {
}
WebviewHelper.launch(Constants.applicationPasswordLink, with: viewController)
}

var rightBarButtonItemTitle: String? {
return Localization.helpButtonTitle
}

func didTapRightBarButtonItem(in viewController: UIViewController?) {
guard let viewController else {
return
}
authentication.presentSupport(from: viewController, screen: .noWooError)
}
}

private extension ApplicationPasswordDisabledViewModel {
enum Localization {
static let errorMessage = NSLocalizedString(
"It seems that your site %@ has Application Password disabled. Please enable it to use the WooCommerce app.",
"applicationPasswordDisabled.errorMessage",
value: "It seems that your site %1$@ has Application Password disabled. Please enable it to use the WooCommerce app.",
comment: "An error message displayed when the user tries to log in to the app with site credentials but has application password disabled. " +
"Reads like: It seems that your site google.com has Application Password disabled. " +
"Please enable it to use the WooCommerce app."
)
static let primaryButtonTitle = NSLocalizedString(
"applicationPasswordDisabled.retry.buttonTitle",
value: "Retry",
comment: "Button to retry fetching application password authorization if application password is disabled"
)
static let secondaryButtonTitle = NSLocalizedString(
"Log In With Another Account",
"applicationPasswordDisabled.secondaryButtonTitle",
value: "Log In With Another Account",
comment: "Action button that will restart the login flow."
+ "Presented when the user tries to log in to the app with site credentials but has application password disabled."
)
static let auxiliaryButtonTitle = NSLocalizedString(
"What is Application Password?",
"applicationPasswordDisabled.auxiliaryButtonTitle",
value: "What is Application Password?",
comment: "Button that will navigate to a web page explaining Application Password"
)
static let primaryButtonTitle = NSLocalizedString(
"Log in with WordPress.com",
comment: "Button that will navigate to the authentication flow with WP.com"
static let helpButtonTitle = NSLocalizedString(
"applicationPasswordDisabled.helpButtonTitle",
value: "Help",
comment: "Button that will navigate to the support area"
)
}
enum Constants {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ final class ApplicationPasswordDisabledViewModelTests: XCTestCase {
func test_viewmodel_provides_expected_error_message() {
// Given
let viewModel = ApplicationPasswordDisabledViewModel(siteURL: testURL)
let expectation = Expectations.errorMessage.replacingOccurrences(of: "%@", with: "test.com")
let expectation = Expectations.errorMessage.replacingOccurrences(of: "%1$@", with: "test.com")

// When
let errorMessage = viewModel.text.string
Expand Down Expand Up @@ -129,18 +129,30 @@ final class ApplicationPasswordDisabledViewModelTests: XCTestCase {
XCTAssertTrue(navigationController.presentedViewController is SFSafariViewController)
}

func test_didTapPrimaryButton_presents_LoginNavigationController() {
func test_didTapPrimaryButton_navigates_to_correct_view_controller() {
// Given
let viewModel = ApplicationPasswordDisabledViewModel(siteURL: testURL)
let viewController1 = UIViewController()
let viewController2 = UIViewController()
let viewController3 = UIViewController()

navigationController.viewControllers = [viewController1, viewController2, viewController3]
window.rootViewController = navigationController
window.makeKeyAndVisible()

XCTAssertEqual(viewController3.navigationController, navigationController, "viewController3 should be part of the navigationController's stack")

// When
viewModel.didTapPrimaryButton(in: navigationController)
viewModel.didTapPrimaryButton(in: viewController3)

// Then
waitUntil {
self.navigationController.presentedViewController != nil
waitUntil(timeout: Constants.expectationTimeout) {
return self.navigationController.viewControllers.count == 1 &&
self.navigationController.topViewController === viewController1
}
XCTAssertTrue(navigationController.presentedViewController is LoginNavigationController)

// Then
XCTAssertEqual(navigationController.viewControllers.count, 1, "After the action, navigationController's stack should contain only one view controller")
XCTAssertEqual(navigationController.topViewController, viewController1, "topViewController should be viewController1")
}
}

Expand All @@ -149,23 +161,27 @@ private extension ApplicationPasswordDisabledViewModelTests {
static let image = UIImage.errorImage

static let errorMessage = NSLocalizedString(
"It seems that your site %@ has Application Password disabled. Please enable it to use the WooCommerce app.",
"applicationPasswordDisabled.errorMessage",
value: "It seems that your site %1$@ has Application Password disabled. Please enable it to use the WooCommerce app.",
comment: "An error message displayed when the user tries to log in to the app with site credentials but has application password disabled. " +
"Reads like: It seems that your site google.com has Application Password disabled. " +
"Please enable it to use the WooCommerce app."
)
static let secondaryButtonTitle = NSLocalizedString(
"Log In With Another Account",
"applicationPasswordDisabled.secondaryButtonTitle",
value: "Log In With Another Account",
comment: "Action button that will restart the login flow."
+ "Presented when the user tries to log in to the app with site credentials but has application password disabled."
)
static let auxiliaryButtonTitle = NSLocalizedString(
"What is Application Password?",
"applicationPasswordDisabled.auxiliaryButtonTitle",
value: "What is Application Password?",
comment: "Button that will navigate to a web page explaining Application Password"
)
static let primaryButtonTitle = NSLocalizedString(
"Log in with WordPress.com",
comment: "Button that will navigate to the authentication flow with WP.com"
"applicationPasswordDisabled.retry.buttonTitle",
value: "Retry",
comment: "Button to retry fetching application password authorization if application password is disabled"
)
}
}