-
Notifications
You must be signed in to change notification settings - Fork 11
Add new methods to WPAuthenticatorDelegate to allow host apps to handle errors #525
Changes from all commits
d454512
765e7af
8252043
43cc632
5ceeeb3
7caf417
68a5c13
a7921d7
b65d8fa
b4d8c44
4ed1276
258c284
a94a620
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 |
|---|---|---|
|
|
@@ -140,6 +140,17 @@ final class SiteAddressViewController: LoginViewController { | |
| } | ||
| } | ||
|
|
||
| override func displayRemoteError(_ error: Error) { | ||
|
Contributor
Author
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. This might not be necessary. In fact, I would argue that it would be good to avoid doing it.
Contributor
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. I'm sorry, but I'm confused by this. It appears you added this override method to implement your error handling. Why do you not agree with what you just did? 😄
Contributor
Author
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. Sorry, that was terrible wording on my side. Let me rephrase my comment: "I am not 100% sure that this is absolutely necessary. So far I have not seen this override being used in any of the tests I have run using WooCommerce. That does not mean it won't happen as I continue testing. I would prefer if, eventually, we noticed that overriding this method is actually unnecessary and we could get rid of the override"
Contributor
Author
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. And I just found a test case for WooCommerce where these overrides are necessary: a Pressable store with Jetpack installed but not activated
Contributor
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. Oh! OK, that makes more sense. 😄 Thank you sir! |
||
| guard authenticationDelegate.shouldHandleError(error) else { | ||
| super.displayRemoteError(error) | ||
| return | ||
| } | ||
|
|
||
| authenticationDelegate.handleError(error) { customUI in | ||
| self.navigationController?.pushViewController(customUI, animated: true) | ||
| } | ||
| } | ||
|
|
||
| /// Reload the tableview and show errors, if any. | ||
| /// | ||
| override func displayError(message: String, moveVoiceOverFocus: Bool = false) { | ||
|
|
@@ -351,6 +362,19 @@ private extension SiteAddressViewController { | |
| } | ||
| } | ||
|
|
||
| /// Push a custom view controller, provided by a host app, to the navigation stack | ||
| func pushCustomUI(_ customUI: UIViewController) { | ||
| /// Assign the help button of the newly injected UI to the same help button we are currently displaying | ||
| /// We are making a somewhat big assumption here: the chrome of the new UI we insert would look like the UI | ||
| /// WPAuthenticator is already displaying. Which is risky, but also kind of makes sense, considering | ||
| /// we are also pushing that injected UI to the current navigation controller. | ||
| if WordPressAuthenticator.shared.delegate?.supportActionEnabled == true { | ||
| customUI.navigationItem.rightBarButtonItems = self.navigationItem.rightBarButtonItems | ||
| } | ||
|
|
||
| self.navigationController?.pushViewController(customUI, animated: true) | ||
| } | ||
|
|
||
| // MARK: - Private Constants | ||
|
|
||
| /// Rows listed in the order they were created. | ||
|
|
@@ -419,6 +443,16 @@ private extension SiteAddressViewController { | |
|
|
||
| let err = self.originalErrorOrError(error: error as NSError) | ||
|
|
||
| /// Check if the host app wants to provide custom UI to handle the error. | ||
| /// If it does, insert the custom UI provided by the host app and exit early | ||
| if self.authenticationDelegate.shouldHandleError(err) { | ||
| self.authenticationDelegate.handleError(err) { customUI in | ||
| self.pushCustomUI(customUI) | ||
| } | ||
|
|
||
| return | ||
| } | ||
|
|
||
| if let xmlrpcValidatorError = err as? WordPressOrgXMLRPCValidatorError { | ||
| self.displayError(message: xmlrpcValidatorError.localizedDescription, moveVoiceOverFocus: true) | ||
|
|
||
|
|
@@ -480,14 +514,7 @@ private extension SiteAddressViewController { | |
|
|
||
| self.showWPUsernamePassword() | ||
| case let .injectViewController(customUI): | ||
| /// Assign the help button of the newly injected UI to the same help button we are currently displaying | ||
| /// We are making a somewhat big assumption here: the chrome of the new UI we insert would look like the UI | ||
| /// WPAuthenticator is already displaying. Which is risky, but also kind of makes sense, considering | ||
| /// we are also pushing that injected UI to the current navigation controller. | ||
| if WordPressAuthenticator.shared.delegate?.supportActionEnabled == true { | ||
| customUI.navigationItem.rightBarButtonItems = self.navigationItem.rightBarButtonItems | ||
| } | ||
| self.navigationController?.pushViewController(customUI, animated: true) | ||
| self.pushCustomUI(customUI) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
These two methods would be new. This would break WordPress, but the fix is simple: for host apps that are good with WPAuthenticator's error handling, shoudlDisplayError would just return false, and handleError would do nothing
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.
Thanks for the info! After this is merged, I'll create a WP PR to address this.