Allowing to continue login flow for self hosted sites that have Jetpack. #98
Conversation
Self hosted sites that have jet pack return with a successful response from the fetch site so we don't show the "logout" prompt for those sites.
|
@aerych , @mindgraffiti , I took a shoot a this as more users are experiencing difficulties logging in: Can you think of any issues with allowing the flow to continue with this added condition? |
|
@aerych , @mindgraffiti would you mind taking a look? |
|
Sorry Yael! This one got lost over the holiday shuffle. Taking a look. |
aerych
left a comment
aerych
left a comment
There was a problem hiding this comment.
Tested and confirmed the bug. I was trying to understand how it was happening when I noticed some other odd things. I'm not 💯 sure this is the best patch. More details in my comment.
| let successBlock: (WordPressComSiteInfo) -> Void = { [weak self] siteInfo in | ||
| self?.loginFields.meta.siteInfo = siteInfo | ||
| if WordPressAuthenticator.shared.delegate?.allowWPComLogin == false { | ||
| if WordPressAuthenticator.shared.delegate?.allowWPComLogin == false && !siteInfo.hasJetpack { |
There was a problem hiding this comment.
So... there are a few things going on in fetchSiteInfo and its associated code paths that seem off to me. For example, the else block in the conditional following the success block is never executed. I suspect there might be more work to be done in this method. That said....
I think this change might introduce a hidden bug -- one that is not immediately obvious nor experienced but could be if something else changed. The logic here says: "If the delegate does not allow a wpcom login and the site does not have Jetpack, then show the prompt". This works for WordPress.com sites since they do not have Jetpack since the conditional evalutes to false. This works for self-hosted sites that have Jetpack since the conditional evaluates to true. This would fail for self-hosted sites without Jetpack if it was ever evaluated for one. Currently the code path for a self-hosted non-Jetpack site does not encounter this check since the API call results in the failure block being executed. Subtle, eh?
I wonder if a more robust fix would be to just call the unauthenticated endpoint. The connect/site-info results include a filed for isWordPressDotCom which could be checked first. Then if true the delegate could be consulted about whether to allow a dotcom login. Its a larger change but maybe worth it to clarify the work being done (and potentially clean up the other code paths that go through here.)
Thoughts?
There was a problem hiding this comment.
This would fail for self-hosted sites without Jetpack if it was ever evaluated for one
Oh! I didn't realize that.
I'm going to give your proposed solution a try and see if it makes more sense. I love the idea of cleaning this method up a bit.
according to code review. Tested with self hosted, self hosted with JP, WP site and private WP site.
|
@aerych , refactored |
aerych
left a comment
aerych
left a comment
There was a problem hiding this comment.
Heya Yael! Looking good! Everything tested well. I had two nitpicks but otherwise 
| }) | ||
| self.configureViewLoading(false) | ||
| if siteInfo.isWPCom { | ||
| if WordPressAuthenticator.shared.delegate?.allowWPComLogin == true { |
There was a problem hiding this comment.
Loving the change ❤️.
One nitpick, maybe we could tweak this to do the work with fewer conditionals? Maybe something like
if siteInfo.isWPCom && WordPressAuthenticator.shared.delegate?.allowWPComLogin == false {
// Hey, you have to log out of your existing WP.com account before logging into another one.
self.promptUserToLogoutBeforeConnectingWPComSite()
return;
}
self.presentNextControllerIfPossible(siteInfo: siteInfo)
| /// | ||
| public let icon: String | ||
|
|
||
| public let isWPCom: Bool |
There was a problem hiding this comment.
For consistency could we add a comment describing the property? Its pretty obvious but 🤷♂.
Self hosted sites that have jet pack return with a successful response from the fetch site so we don't show the "logout" prompt for those sites.
Before:
After:
