Check only isJetpackConnected when checking Jetpack status for Woo

[hichamboushaba]

Currently, when we get the /connect/site-info response for a site, we check the three flags isJetpackActive, hasJetpack and isJetpackConnected, this was implemented just recently (https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/68), while before we were using the field mConnectSiteInfoCalculatedHasJetpack which reads only isJetpackConnected for non-atomic sites (reason as explained in the comments p99K0U-1vO-p2#comment-3574).

This PR restores this behavior, as it's needed for sites that uses Jetpack Connection Package, since, for those, Jetpack is not installed, and hasJetpack will be likely false*

*The backend is broken currently, and returns true for the fields hasJetpack and isJetpackActive, but it's a known bug, that will be fixed later.

Tests

This just restores the behavior to what was done before https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/67 for Jetpack connected sites, so hopefully it shouldn't break anything. Just confirming that the login works with an atomic site and a self hosted site should be enough.

Woo PR: https://github.com/woocommerce/woocommerce-android/pull/5099

[malinajirka]

Thanks for working on this @hichamboushaba! 🙇‍♂️

I'm not sure I fully understand the issue here, so before I dive into testing, I'd like to clarify a couple things.

The PR you are referring to 68 didn't remove usage of mConnectSiteInfoCalculatedHasJetpack -> it only added an extra check to run xml-dpc discovery only when we are not sure if Jetpack is installed&active&connected.

The current code (before this PR) checks the three flags -> if the condition is false, it starts xmldpc discovery and then when xmlrpc is enabled invokes gotConnectedSiteInfo() here. This change was introduced in 67. The "mConnectSiteInfoCalculatedHasJetpack" is passed to the parent app here.

Could you please elaborate or ideally provide a testing steps of what exactly doesn't work with the current code? Also, if we merge this PR, won't we re-introduce the issues which were fixed in the mentioned PRs? Wdyt?

[malinajirka]

One more thing, you mentioned while before we were using the field mConnectSiteInfoCalculatedHasJetpack (aka hasJetpack) but I can't find this in the git history - could you please send me a link to what you are referring to? Thank you!

[hichamboushaba]

One more thing, you mentioned while before we were using the field mConnectSiteInfoCalculatedHasJetpack (aka hasJetpack) but I can't find this in the git history - could you please send me a link to what you are referring to? Thank you!

Let's start with the last question, as it will make the first question easier to answer 🙂, before the changes of both #67 and #68, this is what we've been doing, we calculate the hasJetpack according to this: https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/blob/8bca6c6d2f39970318d8ec9c501c063595d9bfac/WordPressLoginFlow/src/main/java/org/wordpress/android/login/LoginSiteAddressFragment.java#L452-L457 this logic doesn't check isJetpackActive at all, and checks hasJetpack only for atomic sites, for the rest it only uses isJetpackConnected, this calculated value is then passed to the parent app, to decide whether continue the login or not, so we were using the calculated hasJetpack in the parent app. With the changes of #67, we started the XMLRPC discovery regardless of the value of the calculated hasJetpack, and with #68, we introduced the check on the other values that we didn't before, the three flags from the site-info: hasJetpack, isJetpackActive and isJetpackConnected. This PR just restores the old behavior of only checking the calculated hasJetpack, if it's true, then there is no difference from the old logic, and if it's false, then it would run the XMLRPC discovery that was added.

The PR you are referring to 68 didn't remove usage of mConnectSiteInfoCalculatedHasJetpack -> it only added an extra check to run xml-dpc discovery only when we are not sure if Jetpack is installed&active&connected.

As explained above, we don't care if Jetpack is installed&active&connected, we calculate hasJetpack using only isJetpackConnected for non-atomic sites, so we should just keep checking it before running the XMLRPC discovery.

[malinajirka]

The current logic for hasJetpack is exactly the same - here, it hasn't changed. Before #67 and #68 the app run gEtConnectedSiteInfo -> gOtConnectedSiteInfo after the PRs the app runs gEtConnectedSiteInfo -> discovery -> gOtConnectedSiteInfo. The checks for isJetpackActive and isJetpackConnected are used just to know if it's necessary to run discovery -> but after the discovery is run, we still call gOtConnectedSiteInfo with the original hasJetpack no matter what are the values of isJetpackActive and isJetpackConnected. AFAIU Jetpack needs xmlrpc anyway, so running an extra check if XMLRPC is enabled shouldn't cause any issues, right?

With the changes of #67, we started the XMLRPC discovery regardless of the value of the calculated hasJetpack, and with #68, we introduced the check on the other values that we didn't before, the three flags from the site-info: hasJetpack, isJetpackActive and isJetpackConnected.

I believe that's not the case as explained in the above paragraph. We still use the original hasJetpack and we pass it to the client app -> we just pass it after we check that XMLRPC is enabled.

This PR just restores the old behavior of only checking the calculated hasJetpack, if it's true, then there is no difference from the old logic, and if it's false, then it would run the XMLRPC discovery that was added.

I'm still missing the part why we want to restore it to the original behavior. Is there a bug in the current code we are trying to fix? If we restore it to the original behavior, won't we re-introduce the issues which were fixed in #67 and #68?

[malinajirka]

@hichamboushaba and I jumped on a brief call and we found the root cause of confusion.

The motivation for this change is simply to prepare the codebase for introduction of support for Jetpack CP sites.

The source of confusion is that there is a difference between siteInfo.hasJetpack and hasJetpack aka CalculatedHasJetpack. hasJetpack aka CalculatedHasJetpack mirrors siteInfo.isJetpackConnected for non-wpcom sites as per this line.

I merged the tables mentioned in this convo and added

• calculatedHasJetpack column
• Before this PR: xmlrpc discovery will run
• After this PR: xmlrpc discovery will run

Image for easier overview -

Source of the table:		xmlrpc disable plugin	xmlrpc disabled .htaccess	Web: jetpack installed	Web: Jetpack Active	Web: Jetpack connected	siteInfo.isJetpackActive	siteInfo.isJetpackConnected	siteInfo.hasJetpack	.calculatedHasJetpack	Before this PR: xmlrpc discovery will run	After this PR: xmlrpc discovery will run
a	-	-	Y	-	-	-	-	true	-	YES	YES
b	-	-	Y	Y	-	true	-	true	-	YES	YES
c	-	-	Y	Y	Y	true	true	true	true	NO	NO
d	Y	-	Y	-	-	-	-	true	-	YES	YES
e	Y	-	Y	Y	-	true	-	true	-	YES	YES
f	Y	-	Y	Y	Y	true	true	true	true	NO	NO
g	-	-	-	-	-	-	-	-	-	YES	YES
h	Y	-	-	-	-	-	-	-	-	YES	YES
i	-	Y	Y	Y	Y	-	-	true	-	YES	YES
j	-	Y	Y	Y	-	-	-	true	-	YES	YES
k	-	Y	Y	-	-	-	-	true	-	YES	YES
l	-	Y	-	-	-	-	-	-	-	YES	YES

As you can see in the last two columns, the behavior shouldn't change at all. This PR therefore technically doesn't change anything. It'll become useful when we introduce support for Jetpack CP sites.

[malinajirka]

We decided to wait for the Monday codefreeze and merge the PR after. I'll also work on extracting the logic into a separate class and writing unit tests for it, so we can more easily touch this logic without worrying about regressions in the future.