Issue/8225 fix google sign up #8252
Conversation
|
@theck13 for example 2fa: I'm sure @malinajirka has more usecases he can describe here. |
|
Are there specific devices and/or Android version(s)? |
|
The 2fa bug was happening to me on my Pixel 2 with Android Pie and on my emulator with Android 8. I know Jirka was testing the Google flow also on an old Samsung device. |
|
I don't remember all the scenarios, but I'll try to list at least some of them. Not all are easily reproducible -> I just kept trying to signing in/up. One of the infinite dialog cases
Broken flow - happening mostly on low cost devices with limited memory (Samsung Galaxy Lite 2 - Android 4.4).
Broken flow - weird state which I'm not sure how to reproduce, but happens from time to time (happened on both Pixel 2, Android 9 and Samsung Galaxy Lite 2, Android 4.4 phones)
Generic error message (2fa code can be generated just once per minute)
|
|
Are all four of those scenarios fixed with these changes? It seems like the first set is due to some backend server confusion and it's not clear where that is fixed with these changes. The second set sounds like I need to use and low-memory device with Android 4.4. Where can I find how that is fixed with these changes? The third set sounds inconsistent and due to another backend issue. Which changes address that? The fourth set seems to be the only one for certain to reproduce. Were there changes to update the error message or was something else changed? It's hard to follow what changes correspond to each issue. I'm interested in specific steps for devices and/or Android versions because we tested this previously and they were verified as I mentioned. |
| getActivity().getSupportFragmentManager().beginTransaction().remove(this).commit(); | ||
| finishSignUp(); | ||
| } else { | ||
| AppLog.d(T.MAIN, "GOOGLE SIGNUP: onSocialChanged - shouldn't happen - google sign in success"); |
There was a problem hiding this comment.
Following flow is "fixed" with this else branch -> the user is redirected to "Login with WordPress password is shown" and when they try to log in a valid error message is shown (something like your account seems to be deleted, contact our support if you haven't deleted your account).
One of the infinite dialog cases
1. Delete your WordPress account
2. Try the Google sign-up with the same email address
3. Generic error -> "There was some troubles connecting with the Google account" is shown
| @@ -158,14 +164,6 @@ protected void onCreate(Bundle savedInstanceState) { | |||
|
|
|||
| @Override | |||
| public void onSaveInstanceState(Bundle outState) { | |||
| SignupGoogleFragment signupGoogleFragment; | |||
There was a problem hiding this comment.
Following flow is fixed here -> this change requires to change a lot of other code - > mainly the finishSignupFlow needs to be invoked in all scenarios when the flow is finished.
Broken flow - happening mostly on low cost devices with limited memory (Samsung Galaxy Lite 2 - Android 4.4).
1. Click on "Sign Up With Google"
2. Account chooser is shown
3. Select an account
4a. Sometimes an infinite dialog which can't be canceled is shown
4b. Sometimes the dialog disappears, but nothing else happens (I think the activity's onSaveInstanceState gets called -> the GoogleSignUp fragment is removed)
There was a problem hiding this comment.
I think following flow got fixed as part of this change as well. It sometimes happens in the production version of the app but never happened on this branch. What might have also helped to fix this issue is, that we always disconnect the GoogleApiClient when the activityResult is returned.
Broken flow - weird state which I'm not sure how to reproduce, but happens from time to time (happened on both Pixel 2, Android 9 and Samsung Galaxy Lite 2, Android 4.4 phones)
1. Click on "Sign Up with Google"
2. Semi-transparent grey overlay is shown
3. Nothing else happens
| // Response does not return error when two-factor authentication is required. | ||
| } else if (event.requiresTwoStepAuth) { | ||
| // Response does not return error when two-factor authentication is required. | ||
| } else if (event.requiresTwoStepAuth || "sms".equals(event.notificationSent)) { |
There was a problem hiding this comment.
Following flow is fixed here and in the same else branch in the SignUpGoogleFragment.
Generic error message (2fa code can be generated just once per minute)
1. Login to a WordPress account with 2fa in a browser
2. Open the app
3. Click on "Log in" button
4. Click on "Log in with Google"
5. Generic error -> "There was some troubles connecting with the Google account" is shown
|
I agree you'll need a low cost device to test the second set. However, I've tested it extensively after the changes and afaik it works.
I've tried to add some in-code comments pointing to specific lines. However, I'd suggest to review and test the code without keeping focus on which line fixes which issue.
Unfortunately, I don't have more info than what I provided in my previous comment. I agree it's not exactly specific, but both Vojta and I had troubles when we tried to sign-up in the production version of the app and I believe it works pretty well now. I think one of the reasons why it worked during the tests is that most people test it with same type of a phone. Maybe, it's also possible the Google Services got updated since the feature was tested - who knows. |
|
Last thing I'd like to add. I definitely don't want to give an impression that I think this issue is someones fault (developer's, reviewer's, tester's or anyone's else). We've all experienced releasing a code which worked and was tested, but turned out to have some issues in the future. I'm looking forward to see if these changes will have any impact on percentage of successful sign up/in :-). |
| @@ -761,6 +749,24 @@ public void onGoogleSignupFinished(String name, String email, String photoUrl, S | |||
| finish(); | |||
| } | |||
|
|
|||
| @Override public void onGoogleSignupError(String msg) { | |||
There was a problem hiding this comment.
The @Override annotation should be on a separate line from the onGoogleSignupError method for code consistency.
| dialog.show(this.getSupportFragmentManager(), GOOGLE_ERROR_DIALOG_TAG); | ||
| } | ||
|
|
||
| @Override public void onPositiveClicked(@NotNull String instanceTag) { |
There was a problem hiding this comment.
The @Override annotation should be on a separate line from the onPositiveClicked method for code consistency.
| @@ -76,7 +80,7 @@ | |||
|
|
|||
| public class LoginActivity extends AppCompatActivity implements ConnectionCallbacks, OnConnectionFailedListener, | |||
| Callback, LoginListener, GoogleListener, LoginPrologueListener, SignupSheetListener, | |||
| HasSupportFragmentInjector { | |||
| HasSupportFragmentInjector, BasicDialogPositiveClickInterface { | |||
There was a problem hiding this comment.
Is the BasicDialogPositiveClickInterface implementation necessary since the onPositiveClicked method does nothing (i.e. the dialog is dismissed)?
There was a problem hiding this comment.
BasicDialogFragments verifies in the onAttach method that the parent activity implements the interface and throws an exception if it doesn't.
| @@ -2066,6 +2066,7 @@ | |||
| <string name="login_error_button">Close</string> | |||
| <string name="login_error_email_not_found">The Google account \'%s\' doesn\'t match any existing account on WordPress.com.</string> | |||
| <string name="login_error_generic">There was some trouble connecting with the Google account.</string> | |||
| <string name="login_error_sms_throttled">Too many attempts on sending SMS verification code. Try again in a minute.</string> | |||
There was a problem hiding this comment.
Was this copied from the web or iOS? If not, has it been reviewed by Editorial? If not, let's add the [Status] Needs Copy Review label and check with them.
There was a problem hiding this comment.
Tweaks for 2 and 4:
- There's no WordPress.com account matching this Google account.
- We've made too many attempts to send an SMS verification code -- take a break, and request a new one in a minute.
On the "There was some trouble..." is there something we can tell them about what to do next? Wait a few minutes and try again?
There was a problem hiding this comment.
Thank you @michelleweber !
On the "There was some trouble..." is there something we can tell them about what to do next? Wait a few minutes and try again?
Nope, it's an unexpected state and we don't know if there is something wrong with their account/our app/server or it just haven't worked out because of a temporary outage.
| @Override | ||
| public void onDetach() { | ||
| super.onDetach(); | ||
| @Override public void onDestroy() { |
There was a problem hiding this comment.
The @Override annotation should be on a separate line from the onDestroy method for code consistency.
| break; | ||
| } | ||
| // Response does not return error when two-factor authentication is required. | ||
| } else if (event.requiresTwoStepAuth) { | ||
| } else if (event.requiresTwoStepAuth || "sms".equals(event.notificationSent)) { |
There was a problem hiding this comment.
Let's replace the hard-coded "sms" with a constant like Login2FaFragment.TWO_FACTOR_TYPE_SMS.
| // Response does not return error when two-factor authentication is required. | ||
| } else if (event.requiresTwoStepAuth) { | ||
| // Response does not return error when two-factor authentication is required. | ||
| } else if (event.requiresTwoStepAuth || "sms".equals(event.notificationSent)) { |
There was a problem hiding this comment.
Let's replace the hard-coded "sms" with a constant like Login2FaFragment.TWO_FACTOR_TYPE_SMS.
| getActivity().getSupportFragmentManager().beginTransaction().remove(this).commit(); | ||
| finishSignUp(); | ||
| } else { | ||
| AppLog.d(T.MAIN, "GOOGLE SIGNUP: onSocialChanged - shouldn't happen - google sign in success"); |
There was a problem hiding this comment.
For better differentiation between the two, we used Google login or signup rather than signin. Let's update sign in to signup for consistency.
| mAnalyticsListener.trackSignupSocialToLogin(); | ||
| mLoginListener.showSignupToLoginMessage(); | ||
| // Dispatch social login action to retrieve data required for two-factor authentication. | ||
| PushSocialPayload payload = new PushSocialPayload(mIdToken, SERVICE_TYPE_GOOGLE); | ||
| AppLog.d(T.MAIN, | ||
| "GOOGLE SIGNUP: onSocialChanged - error - two step authentication - dispatching " |
There was a problem hiding this comment.
The first part of the string can be on the previous line with the second part can be on the second since it makes the line longer than 120 characters.
| mDispatcher.unregister(this); | ||
| getActivity().getSupportFragmentManager().beginTransaction().remove(this).commit(); | ||
| finishSignUp(); | ||
| } else { |
There was a problem hiding this comment.
We shouldn't make the assumption all scenarios that aren't an error and not two-factor authentication are a deleted WordPress account. It's possible that's not the case and we're directing them to login, which may be incorrect and confusing to the user. If we can ensure it's a deleted account, we should catch that case, show an appropriate message, and track that event specifically. If we can't, let's keep the generic error message and remove the SignupGoogleFragment to fix the indefinite dialog scenario.
Also, we're triggering the trackSignupSocialAccountsNeedConnecting statistic from the handleUserExists method, which should only be called when we're certain the user exists and their WordPress/Google accounts need to be connected. That's not true if they deleted their account.
There was a problem hiding this comment.
I don't think we are making the assumption all scenarios that aren't an error and not two-factor authentication are a deleted WordPress account. We are making an assumption that all scenarios that aren't an error or not two-factor authentication are successful. I've renamed the handleUserExists to loginViaSocialAccount to make it more clear. Wdyt?
|
Thanks @malinajirka for wrangling this. :) Can I ask a favor? Authentication has quite a bit of complicated logic (as I'm sure you've seen by now) and we want to be very cautious about introducing a new regression in the course of fixing another. Testing all the different flows is very time consuming and with the more things a single PR fixes the easier it becomes to start to miss things during testing. Some of the glitches being fixed in this PR sound very tricky to reproduce, making the reviewer's job a bit harder. With that in mind, I'm wondering if there is a way to break this PR into two or more smaller more targeted PRs where testing could also be more focused and less likely to overlook anything? What do you think? I understand that some changes resolve multiple issues so not everything can be broken up, but any separation that makes the code more focused would be helpful. |
|
Thanks @aerych for your comment. I've considered breaking this PR into more smaller PRs, but I decided having everything in one PR makes more sense to me for the following reasons
|
|
Thank you @theck13 !!! I've fixed most of the issues you've pointed out. I have left comments for those I haven't fixed. I believe it's ready for another round. Thank you!! |
|
Crash: Android API 24 (7.0) Samsung Galaxy S6 Steps:
(NOTE - get same result for existing or new google account connection) Notable Log messages: Exception: |
|
Thanks @AmandaRiu !!! I tested the app on Samsung Galaxy S6 Edge, Android 7.0 and it works as expected:(. The exception is there as well, so it most probably isn't related. Are you aware of any settings on your phone which might "cause" the issue -> such as "Don't keep activities" in developer options is on or anything similar? |
|
I tested the following scenarios:
Scenarios 1-5 worked exactly as expected. The last scenario connecting Jetpack did not work as expected. Here's what I expected:
Here's what happened:
I tested the same scenario in the latest 10.8 alpha (alpha-126) and the flow worked as expected there (without the magic link flow, but as mentioned I think that's expected), so this issue seems to be specific to this PR. Tested on Nexus 9, Android 7.1.1 |
|
Turns out my errors were due to google services not being properly configured in my development environment. Once that issue was remedied, signup with google worked like a charm on my Samsung Galaxy s6! |
|
@planarvoid and I have just tested the @rachelmcr has confirmed she reproduced the bug on the current Alpha version, so it seems the bug wasn't introduced as part of this PR. She'll submit a new GitHub issue. Thank you Rachel for all your help!! @theck13 With all the new info, I believe we can finally merge it 🎉 . Thank you for reviewing such a complicated PR;-)! |
Fixes #8225 #8223
@planarvoid and I were working on this issue together, so I believe it'd be good for someone else to review this PR.
Issues
It'd be hard to list all the things that have been broken. Some of them were happening in rare scenarios. We have tried to cover all the use-cases and test them extensively.
It's hard to connect the fixes with the issues, some of them fix more than one thing. Please feel free to ask if you have any doubts.
To test:
Sign up screen
State: New account (non 2fa)
State: Existing WordPress account not connected to Google
State: Existing WordPress account connected to Google
State: Existing WordPress account with 2fa not connected to Google
State: Existing WordPress account with 2fa connected to Google
Sign in screen
State: Non existing account (non 2fa)
State: Existing WordPress account not connected to Google
State: Existing WordPress account connected to Google
State: Existing WordPress account with 2fa not connected to Google
State: Existing WordPress account with 2fa connected to Google
2. Open the app
3. Click on "Log in"
4. Click on "Login with Google"
5. Wait until an account chooser appears
6. Select an account
7. 2FA verification screen is shown
8. Enter the 2FA code
9. Epilogue is shown