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

pmusolino · 2025-01-31T15:14:27Z

Closes: #14004 (and peaMlT-U4)

Description

This pull request introduces improvements to handling the scenario when application passwords are disabled in user's website. The changes involve updating the error handling UI and navigation logic to provide a better user experience. This is a continuation of the job started by @hafizrahman here.

Testing information

Create test site with WooCommerce and no Jetpack, on Jurassic Ninja.
Install and active a plugin like this one https://wordpress.org/plugins/disable-application-passwords/
Try to log in using application password to the test site.
Confirm that the new dialog is shown.
Confirm that tapping "Retry" button returns to two screens before the current one (instructions on how to approve the connection with application password).
Confirm that tapping "Log In With Another Account" returns to the initial screen of the login process.

Screenshots

Before	After

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

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

…ed application password.

…abled-note

…ledViewModel.swift - Modify navigation logic to pop to the third last view controller or root if fewer than three - Update primary button title to "Retry" for retrying application password authorization - Remove outdated primary button title "Log in with WordPress.com"

…ordDisabledViewModel` using Woo mobile guidelines

…swordViewModelTests`

…e else

wpmobilebot · 2025-01-31T15:33:20Z

📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS
	Build Number	pr15031-98810f3
	Version	21.6
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`98810f3`
	App Center Build	WooCommerce - Prototype Builds #12774

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

itsmeichigo

While testing navigation back and forth, I encountered a case where the application password fetch failed with the following log:

Tracked application_password_authorization_url_fetch_failed, properties: [error_domain: Networking.RequestAuthenticatorError, error_description: Alamofire.AFError.requestAdaptationFailed(error: Networking.RequestAuthenticatorError.applicationPasswordNotAvailable), error_code: 1]

In this case, the fetchAuthURL method failed with error and the logic went through to the catch block, which showed an alert instead of the application password not available screen:

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-02-03.at.11.50.29.mp4

We should also check the logic in this catch block to show the new screen if we get the applicationPasswordNotAvailable error.

itsmeichigo · 2025-02-03T05:01:51Z

.../Authentication/Application Password/ApplicationPasswordAuthorizationWebViewController.swift

+        // Pop back two view controllers to remove both error and web view
+        navigationController?.popViewController(animated: false)
+        navigationController?.popViewController(animated: true)


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.

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

.../Authentication/Application Password/ApplicationPasswordAuthorizationWebViewController.swift

itsmeichigo · 2025-02-03T05:09:30Z

...erce/Classes/Authentication/Navigation Exceptions/ApplicationPasswordDisabledViewModel.swift

+        if viewControllers.count >= 3 {
+            navigationController.popToViewController(viewControllers[viewControllers.count - 3], animated: true)
+        } else {
+            navigationController.popToRootViewController(animated: true)
        }


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.

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

…abled-note-take-2

- Modify `ApplicationPasswordAuthorizationWebViewController.swift` to include a reference to the previous view controller for navigation purposes. - Update `AuthenticationManager.swift` to pass the previous view controller to the web view used for application password authorization. - Enhance `ApplicationPasswordDisabledViewModel.swift` to store the previous view controller and use it for navigation. - Adjust `PostSiteCredentialLoginChecker.swift` to capture and utilize the last view controller before the application password flow. - Update tests in `ApplicationPasswordDisabledViewModelTests.swift` to include the previous view controller in the view model initialization.

…abled-note-take-2

pmusolino · 2025-02-03T14:54:44Z

@itsmeichigo this is ready for another check 🙌

itsmeichigo

Thanks for the updates, Paolo. I found a few issues with the new changes in the comments below.

Also, it seems that you missed the comment in my previous round about missing the case when fetching the authentication URL fails with Alamofire.AFError.requestAdaptationFailed(error: Networking.RequestAuthenticatorError.applicationPasswordNotAvailable).

itsmeichigo · 2025-02-04T04:09:23Z

.../Authentication/Application Password/ApplicationPasswordAuthorizationWebViewController.swift

@@ -34,6 +34,9 @@ final class ApplicationPasswordAuthorizationWebViewController: UIViewController
        return indicator
    }()

+    /// The view controller that was presenting the application password flow.
+    private weak var previousViewController: UIViewController?


Just wondering: is there any case where previousViewController can be deallocated when the user is still on ApplicationPasswordAuthorizationWebViewController? I'm curious why the weak reference is necessary here.

itsmeichigo · 2025-02-04T04:11:10Z

.../Authentication/Application Password/ApplicationPasswordAuthorizationWebViewController.swift

@@ -171,6 +189,15 @@ private extension ApplicationPasswordAuthorizationWebViewController {
        }
    }

+    /// Pops to the previous view controller (if provided) or pops one level otherwise.
+    @objc private func navigateToPreviousViewController() {
+        if let previous = previousViewController, let nav = navigationController {


Super nit: we can simplify the if let checks now.

Suggested change

if let previous = previousViewController, let nav = navigationController {

if let previousViewController, let navigationController {

itsmeichigo · 2025-02-04T04:15:06Z

...erce/Classes/Authentication/Navigation Exceptions/ApplicationPasswordDisabledViewModel.swift

+    let image: UIImage = .errorImage
+
+    // The VC that was showing before the application password flow. This is used to navigate back without guesswork.
+    let previousViewController: UIViewController?


Related to a question above: the previousViewController reference is kept strongly here. Is it intentional to keep the weak reference in ApplicationPasswordAuthorizationWebViewController but strong here?

itsmeichigo · 2025-02-04T04:15:47Z

...erce/Classes/Authentication/Navigation Exceptions/ApplicationPasswordDisabledViewModel.swift

-        guard let viewController else {
-            return
+        guard let navigationController = viewController?.navigationController else { return }
+        if let previousViewController = previousViewController {


Super nit:

Suggested change

if let previousViewController = previousViewController {

if let previousViewController {

itsmeichigo · 2025-02-04T04:25:43Z

WooCommerce/Classes/Authentication/PostSiteCredentialLoginChecker.swift

+                    // show application password disabled error, and catch the last view controller that was showing before the Application Password flow.
+                    let previousViewController = navigationController.viewControllers.dropLast().last


What is the exact controller that you are navigating to here? Please note that the PostSiteCredentialLoginChecker can be triggered from two places: (1) after site credential login (if application password is not required), and (2) after application password login.

For (1), I'd suggest sending nil as the previous view controller to the PostSiteCredentialLoginChecker, so that the error page would simply pop itself upon navigating back.

For (2), I'd suggest sending the previous view controller from the application password entry point in AuthenticationManager

itsmeichigo · 2025-02-04T04:28:32Z

...erce/Classes/Authentication/Navigation Exceptions/ApplicationPasswordDisabledViewModel.swift

+        if let previousViewController = previousViewController {
+            navigationController.popToViewController(previousViewController, animated: true)
+        } else {
+            navigationController.popToRootViewController(animated: true)


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

hafizrahman and others added 7 commits October 31, 2024 16:23

First pass at converting error dialog into an error screen for disabl…

cf69dbb

…ed application password.

Merge branch 'trunk' into issue/14004-better-application-password-dis…

74f3268

…abled-note

update: localization keys and message formatting in `ApplicationPassw…

170d58e

…ordDisabledViewModel` using Woo mobile guidelines

update: authentication tests and localization keys in `ApplicationPas…

2887491

…swordViewModelTests`

clean: removed TODO comment because the track already happen somewher…

43e803d

…e else

update: release-notes 21.7

965c4d3

pmusolino added the feature: login Related to any part of the log in or sign in flow, or authentication. label Jan 31, 2025

pmusolino added this to the 21.7 milestone Jan 31, 2025

pmusolino requested review from selanthiraiyan, hichamboushaba and itsmeichigo January 31, 2025 15:14

pmusolino mentioned this pull request Jan 31, 2025

[Login] Improve error messaging when application password is disabled. #14269

Closed

4 tasks

update: added PR url in 21.7 release notes

58faf5e

itsmeichigo self-assigned this Feb 3, 2025

itsmeichigo reviewed Feb 3, 2025

View reviewed changes

pmusolino added 4 commits February 3, 2025 14:34

Merge branch 'trunk' into issue/14004-better-application-password-dis…

79d524a

…abled-note-take-2

fix: remove unused code

3f40c59

Merge branch 'trunk' into issue/14004-better-application-password-dis…

98810f3

…abled-note-take-2

pmusolino requested a review from itsmeichigo February 3, 2025 14:54

itsmeichigo requested changes Feb 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pmusolino commented Jan 31, 2025

wpmobilebot commented Jan 31, 2025 •

edited

Loading

itsmeichigo left a comment

itsmeichigo Feb 3, 2025

pmusolino Feb 3, 2025

itsmeichigo Feb 3, 2025

pmusolino Feb 3, 2025

pmusolino commented Feb 3, 2025

itsmeichigo left a comment •

edited

Loading

itsmeichigo Feb 4, 2025

itsmeichigo Feb 4, 2025

itsmeichigo Feb 4, 2025

itsmeichigo Feb 4, 2025

itsmeichigo Feb 4, 2025

itsmeichigo Feb 4, 2025

	if let previous = previousViewController, let nav = navigationController {
	if let previousViewController, let navigationController {

	if let previousViewController = previousViewController {
	if let previousViewController {

		// show application password disabled error, and catch the last view controller that was showing before the Application Password flow.
		let previousViewController = navigationController.viewControllers.dropLast().last

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

Are you sure you want to change the base?

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

Conversation

pmusolino commented Jan 31, 2025

Description

Testing information

Screenshots

Reviewer (or Author, in the case of optional code reviews):

wpmobilebot commented Jan 31, 2025 • edited Loading

itsmeichigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmusolino commented Feb 3, 2025

itsmeichigo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpmobilebot commented Jan 31, 2025 •

edited

Loading

itsmeichigo left a comment •

edited

Loading