Let's expand this comment to make it clear that we deliberately ignore all other error cases as non actionable and not worthy of informing the user about. Thanks!

I update the comment in 545094c.

Hmm, can isSocialLogin be false while in onSocialChanged(). I'm under the impression that this callback is only called when we are in the Social login mode, right?

I think this was another copy/paste error from Login2FaFragment. I removed that conditional in b9bf704.

It seems there is a doFinishLogin() call right after the switch anyway (wrapped in a isSocialLogin conditional that I asked about below anyway) so, is it really needed here?

Nope, it's not needed. I missed that when I moved the doFinishLogin statement to the isSocialLogin conditional. I remove the redundant statement in 33bc5c3.

Not sure, do we really mean to pass null as the idToken to the new instance here or should we use the provided idToken parameter instead? I think that Scenario No1 fails to complete the account connection because of this.

Yep, this is why the Social Disconnected / Two-Factor Disabled scenario failed as you mentioned. It was a copy/paste error on my part. I fixed that in f2df3e8.

The "current" site list will need to be grabbed before the login actually finishes and the updated sites list is fetched from the server. See here for example.

It's not super clear but this list acts as an ignore-list at the login epilogue screen, to make the epilogue only show the brand new sites that got enabled by this login (there might be already added .org sites before this login). With the current state of the PR, the epilogue screen shows an empty list in the scenario No2 (Social connected but no 2FA).

The mLoginListener.loggedInViaSocialAccount(SiteUtils.getCurrentSiteIds(mSiteStore, false)); statement was meant to mimic similar statements like mLoginListener.loggedInViaPassword(mOldSitesIDs);. I wasn't aware the ArrayList<Integer> oldSiteIds parameter needed to be determined before onLoginFinished. Would adding a mOldSitesIds variable which is assigned when the Google login process is started be sufficient?

At the start of the Google login process should do it, yeap. Note that in that case the list will need to be preserved to survive rotations.

I added the variable and state preservation in 836e1b3.

Verified it working now @theck13 👍

Minor but, these isXXX variables need their Hungarian notation :). We haven't dropped that rule yet for Java wpandroid, right?

There are more places in this PR with non-hungarian form of isXXX variables and those need adjustment as well. Thanks!

There are 100+ instances of boolean [m](\w)+; and 78 instances of boolean [^m](\w)+; variables in the WordPress codebase so it's not clear what the standard should be. Let's not worry about it for now and leave it as is. Those will be cleaned up when we implement code style automation in the near future.

I think this PR is not the place to debate codestyle guideline changes but I see what you mean.

Let's keep it as is here (non-hungarian notation in the isXXX fields) but I'd highly suggest we stick to the current styleguide in subsequent PRs until it gets revised. Manually enforcing the styleguide in PR review (instead of say, using checkstyle) is already laborious so, let's not add debating on top of it 😸 . Thanks!

It seems there are 3 modes of operation in regards to asking for 2FA (please, let me know if I understood this wrongly):

1. The plain old non-social login mode
2. The social login+connect mode
3. The social login (already connected) mode

I think the needs2fa method name doesn't serve its purpose enough now that there are overloaded versions of it. Let's rename each of the overloads to better portray their functionality/usage.

While at it, I'd recommend accompany them with a dedicated (and renamed) newInstance helper method in Login2FaFragment. Currently in the PR, one of the newInstances accepts the isSocialLoginConnect param but I think that's confusing. Let's break that one out to 2 separate helper methods, 3 all in all, to better match the modes. Thanks!

There are multiple instances which two-factor authentication is required as you mentioned and each has a different set of parameters. That seems like a good case to use overloaded methods. I didn't rename the interface methods since one is strictly non-social, one is either social or not, and one is strictly social.

Since you can get to the two-factor login screen from the email/password screen two different ways (i.e. non-social login, social login with connect), the isSocialLoginConnect parameter is needed.

LoginListener interface methods are deliberately descriptive in an attempt to address Login's complexity and correspond to the different flow or subflows so, let's follow suit here. Example: See loggedInViaSignup(list) vs loggedInViaPassword(list). Those two could be defined overloaded but are not and instead, their names correspond to the login flow/subflow.

I didn't rename the interface methods since one is strictly non-social, one is either social or not, and one is strictly social.

Since you can get to the two-factor login screen from the email/password screen two different ways (i.e. non-social login, social login with connect), the isSocialLoginConnect parameter is needed.

I might be missing something here so, in an attempt to understand the code better I prepared this commit on a branch with the method names "de-overloaded". Can you have a look and check if it's missing any functionality? Let's adopt something similar if it's functionally sound. Thanks!

In particular, is there a need for that second overloaded version of needs2fa to be both for social and non-social?

Yes, the second version of the needs2fa overloaded methods is needed for social and non-social instances currently. It was designed that way so the first version could remain unaltered and files irrelevant to social login could also remained unaltered (i.e. LoginUsernamePasswordFragment).

I compromised with 954b09f.

ARG_2FA_TYPE_AUTHENTICATOR, ARG_2FA_TYPE_BACKUP and ARG_2FA_TYPE_SMS don't seem to be arguments passed to the fragment so, their naming is confusing. Instead, they seem to be values for the internal mType variable. Let's rename them to drop the ARG_ prefix and move them to a different section of the file. Alternatively, we can make them an Enum for additional clarity and utility.

I update the constant names in 0f6eddf.

The mType variable seems like the API is bleeding some of its internal mechanics. I mean, the UI or the Fragment in general doesn't need the type other than to serve the API calls. It also seems that we can always deduce the type by examining the nonce (its length) so, I'd recommend we remove the instance variable and just use some nonce-to-type helper method.

That will also remove the need to persist the instance variable on rotation, which I think is currently needed.

By the way, can't the type be just hidden in FluxC without the UI needing to specify it in the call?

The mType cannot be deduced from the nonce length. The mType is determined by the code length.

The mType doesn't need to be persisted on device rotation since it is set with setAuthCodeTypeAndNonce before passing it to PushSocialAuthPayload. I can remove it from onCreate and remove the ARG_2FA_TYPE constant.

I didn't want to put that level of logic in FluxC. The network layer should not care about app-level logic. It should just take parameters, make network calls, and return responses.

In principal yes, the networking lib should only handle network stuff and not that much business logic, but FluxC is not just a networking library by any measure. Instead, it tries to hide away many WP/WPCOM details, exposing simple and consistent interfaces to wpandroid and others.

In this case, the 2FA type looks like noise that the mobile client shouldn't even know/care about at all. Unless it serves some purpose to have it exposed to the user, we should hide it inside FluxC (if possible). But, this is not something this PR needs to block on anyway. So, let's keep it as is but, I would still recommend opening a ticket in FluxC to hide the 2FA type from the caller. I think some collaboration with the backend developers will also be needed to fix this. Thanks!

The mType doesn't need to be persisted on device rotation since it is set with setAuthCodeTypeAndNonce before passing it to PushSocialAuthPayload. I can remove it from onCreate and remove the ARG_2FA_TYPE constant.

Nice, yeah, let's remove it from onCreate (and newInstance I guess) since it gets overwritten anyway before the PushSocialAuthPayload call.

I removed the unnecessary code for two-factor authentication type in 40c1b33.

Oh, I think we overdid it here. mType doesn't need to be initialised from fragment's argument but we do need to preserve it on rotation so its value is correct when the onSocialChanged response arrives. Rotation may have happened after the PushSocialAuthPayload call but before the response comes back.

I added state preservation for mType in b71176d.

I hinted in an earlier comment about this so, I think we should just deduce the type from the event.error.nonce without relying on a global instance variable, unless of course there is more to it. Is there some special reason to rely on a global var here?

As I mentioned in my reply, we can't deduce the type from the nonce.