Add new methods to WPAuthenticatorDelegate to allow host apps to handle errors #525
Conversation
| /// | ||
| func shouldPresentLoginEpilogue(isJetpackLogin: Bool) -> Bool | ||
|
|
||
| /// Indicates the Host app wants to handle and display a given error. |
There was a problem hiding this comment.
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.
Thanks for the info! After this is merged, I'll create a WP PR to address this.
| } | ||
| } | ||
|
|
||
| override func displayRemoteError(_ error: Error) { |
There was a problem hiding this comment.
This might not be necessary. In fact, I would argue that it would be good to avoid doing it.
There was a problem hiding this comment.
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? 😄
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
And I just found a test case for WooCommerce where these overrides are necessary: a Pressable store with Jetpack installed but not activated
There was a problem hiding this comment.
Oh! OK, that makes more sense. 😄 Thank you sir!
|
|
||
| let err = self.originalErrorOrError(error: error as NSError) | ||
|
|
||
| if self.authenticationDelegate.shouldDisplayError(err) { |
There was a problem hiding this comment.
This is what actually inserts the custom UI for the case when the url provided for login with site address is not a WordPress site. This code would use some cleanup, but I hope it helps illustrate how this solution would work
|
|
||
| /// Signals the Host app that there is an error that needs to be handled. | ||
| /// | ||
| func handleError(_ error: Error, onCompletion: @escaping (WordPressAuthenticatorResult) -> Void) |
There was a problem hiding this comment.
The parameter in the completion block might just be of type UIViewController. After all, host apps would only call this method if they actually want to inject a custom error UI
| } | ||
|
|
||
| override func displayRemoteError(_ error: Error) { | ||
| guard authenticationDelegate.shouldHandleError(error) == true else { |
There was a problem hiding this comment.
Can this just be guard authenticationDelegate.shouldHandleError(error) else {?
There was a problem hiding this comment.
iirc, this is part of Woo's coding style. I could be wrong though.
There was a problem hiding this comment.
Yes, it's probably muscle memory. I'll update it
|
Thanks for the review @mindgraffiti @ScoutHarris I bumped the podspec, updated the guard clause and hopefully clarified my comment about the override. I'll merge when CI is ✅ |
Closes #530
Ref: p91TBi-3Rn-p2
Changes
WPAuthenticatorDelegateto allow host apps to decide if they want to handle errors.How to test