-
Notifications
You must be signed in to change notification settings - Fork 114
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
base: trunk
Are you sure you want to change the base?
[Login] Improve error messaging displaying a new error screen when application password is disabled #15031
Changes from 8 commits
cf69dbb
74f3268
b292c92
170d58e
2887491
43e803d
965c4d3
58faf5e
79d524a
3f40c59
9b480ba
98810f3
6467c96
71d68ee
ec50237
6bdb62d
fc9335d
3e5b2ad
daee4c0
dc68ebc
4ae12d2
9fafc53
9e4663e
2c7f417
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
} | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you want to pop to the previous view controller, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation. From my understanding, you already sent the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now I'm using the |
||
WordPressAuthenticator.showLoginForJustWPCom(from: viewController) | ||
} | ||
|
||
func didTapSecondaryButton(in viewController: UIViewController?) { | ||
|
@@ -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 { | ||
|
There was a problem hiding this comment.
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
inAuthenticationManager
.There was a problem hiding this comment.
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