Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue/8225 fix google sign up #8252

Merged
merged 13 commits into from
Sep 10, 2018
Merged

Issue/8225 fix google sign up #8252

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
baab409
Fix google sign-up
malinajirka Aug 28, 2018
db9fc44
Fix signup/in flows
malinajirka Aug 28, 2018
7b1e4b2
Fix google sign in flow
malinajirka Aug 29, 2018
a23b2f6
Trigger 2fa when the authentication SMS has been already sent during …
planarvoid Aug 29, 2018
8451c7a
Add a meaningful error message when the 2fa loggin is stopped because…
planarvoid Aug 29, 2018
b8bf0f0
Fix checkstyle
malinajirka Aug 29, 2018
bd154e9
Fix build - missing string in the base app
malinajirka Aug 29, 2018
3f9560c
Fix log msgs
malinajirka Aug 30, 2018
e6dbc86
Refactoring
malinajirka Aug 31, 2018
982431c
Refactoring
malinajirka Aug 31, 2018
e880cc6
Checkstyle fix
malinajirka Aug 31, 2018
fef35d7
Fix text of sms throttled error
malinajirka Aug 31, 2018
992e5d7
Fix text of email not found error
malinajirka Aug 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 29 additions & 23 deletions WordPress/src/main/java/org/wordpress/android/ui/accounts/LoginActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.android.gms.common.api.GoogleApiClient.ConnectionCallbacks;
import com.google.android.gms.common.api.GoogleApiClient.OnConnectionFailedListener;

import org.jetbrains.annotations.NotNull;
import org.wordpress.android.R;
import org.wordpress.android.WordPress;
import org.wordpress.android.analytics.AnalyticsTracker;
Expand Down Expand Up @@ -51,9 +52,12 @@
import org.wordpress.android.ui.accounts.login.LoginPrologueFragment;
import org.wordpress.android.ui.accounts.login.LoginPrologueListener;
import org.wordpress.android.ui.notifications.services.NotificationsUpdateServiceStarter;
import org.wordpress.android.ui.posts.BasicFragmentDialog;
import org.wordpress.android.ui.posts.BasicFragmentDialog.BasicDialogPositiveClickInterface;
import org.wordpress.android.ui.reader.services.update.ReaderUpdateLogic;
import org.wordpress.android.ui.reader.services.update.ReaderUpdateServiceStarter;
import org.wordpress.android.util.AppLog;
import org.wordpress.android.util.AppLog.T;
import org.wordpress.android.util.CrashlyticsUtils;
import org.wordpress.android.util.LanguageUtils;
import org.wordpress.android.util.LocaleManager;
Expand All @@ -76,7 +80,7 @@

public class LoginActivity extends AppCompatActivity implements ConnectionCallbacks, OnConnectionFailedListener,
Callback, LoginListener, GoogleListener, LoginPrologueListener, SignupSheetListener,
HasSupportFragmentInjector {
HasSupportFragmentInjector, BasicDialogPositiveClickInterface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the BasicDialogPositiveClickInterface implementation necessary since the onPositiveClicked method does nothing (i.e. the dialog is dismissed)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BasicDialogFragments verifies in the onAttach method that the parent activity implements the interface and throws an exception if it doesn't.

public static final String ARG_JETPACK_CONNECT_SOURCE = "ARG_JETPACK_CONNECT_SOURCE";
public static final String MAGIC_LOGIN = "magic-login";
public static final String TOKEN_PARAMETER = "token";
Expand All @@ -86,6 +90,8 @@ public class LoginActivity extends AppCompatActivity implements ConnectionCallba

private static final String FORGOT_PASSWORD_URL_SUFFIX = "wp-login.php?action=lostpassword";

private static final String GOOGLE_ERROR_DIALOG_TAG = "google_error_dialog_tag";

private enum SmartLockHelperState {
NOT_TRIGGERED,
TRIGGER_FILL_IN_ON_CONNECT,
Expand Down Expand Up @@ -158,14 +164,6 @@ protected void onCreate(Bundle savedInstanceState) {

@Override
public void onSaveInstanceState(Bundle outState) {
SignupGoogleFragment signupGoogleFragment;
Copy link
Contributor Author

@malinajirka malinajirka Aug 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@malinajirka malinajirka Aug 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

FragmentManager fragmentManager = getSupportFragmentManager();
signupGoogleFragment = (SignupGoogleFragment) fragmentManager.findFragmentByTag(SignupGoogleFragment.TAG);

if (signupGoogleFragment != null) {
fragmentManager.beginTransaction().remove(signupGoogleFragment).commit();
}

super.onSaveInstanceState(outState);

outState.putBoolean(KEY_SIGNUP_SHEET_DISPLAYED, mSignupSheetDisplayed);
Expand Down Expand Up @@ -256,6 +254,7 @@ private void loggedInAndFinish(ArrayList<Integer> oldSitesIds, boolean doLoginUp

@Override
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
AppLog.d(T.MAIN, "LoginActivity: onActivity Result - requestCode" + requestCode);
super.onActivityResult(requestCode, resultCode, data);

switch (requestCode) {
Expand Down Expand Up @@ -364,19 +363,14 @@ public void onSignupSheetEmailClicked() {

@Override
public void onSignupSheetGoogleClicked() {
dismissSignupSheet();
AnalyticsTracker.track(AnalyticsTracker.Stat.CREATE_ACCOUNT_INITIATED);
AnalyticsTracker.track(AnalyticsTracker.Stat.SIGNUP_GOOGLE_BUTTON_TAPPED);

if (NetworkUtils.checkConnection(this)) {
SignupGoogleFragment signupGoogleFragment;
FragmentManager fragmentManager = getSupportFragmentManager();
FragmentTransaction fragmentTransaction = fragmentManager.beginTransaction();
signupGoogleFragment = (SignupGoogleFragment) fragmentManager.findFragmentByTag(SignupGoogleFragment.TAG);

if (signupGoogleFragment != null) {
fragmentTransaction.remove(signupGoogleFragment);
}

signupGoogleFragment = new SignupGoogleFragment();
signupGoogleFragment.setRetainInstance(true);
fragmentTransaction.add(signupGoogleFragment, SignupGoogleFragment.TAG);
Expand Down Expand Up @@ -592,16 +586,10 @@ public void helpSocialEmailScreen(String email) {
}

@Override
public void addGoogleLoginFragment(@NonNull Fragment parent) {
public void addGoogleLoginFragment() {
LoginGoogleFragment loginGoogleFragment;
FragmentManager fragmentManager = parent.getChildFragmentManager();
FragmentManager fragmentManager = getSupportFragmentManager();
FragmentTransaction fragmentTransaction = fragmentManager.beginTransaction();
loginGoogleFragment = (LoginGoogleFragment) fragmentManager.findFragmentByTag(LoginGoogleFragment.TAG);

if (loginGoogleFragment != null) {
fragmentTransaction.remove(loginGoogleFragment);
}

loginGoogleFragment = new LoginGoogleFragment();
loginGoogleFragment.setRetainInstance(true);
fragmentTransaction.add(loginGoogleFragment, LoginGoogleFragment.TAG);
Expand Down Expand Up @@ -761,6 +749,24 @@ public void onGoogleSignupFinished(String name, String email, String photoUrl, S
finish();
}

@Override public void onGoogleSignupError(String msg) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @Override annotation should be on a separate line from the onGoogleSignupError method for code consistency.

BasicFragmentDialog dialog = new BasicFragmentDialog();
dialog.initialize(GOOGLE_ERROR_DIALOG_TAG, getString(R.string.error),
msg,
getString(org.wordpress.android.login.R.string.login_error_button),
null,
null);
dialog.show(this.getSupportFragmentManager(), GOOGLE_ERROR_DIALOG_TAG);
}

@Override public void onPositiveClicked(@NotNull String instanceTag) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @Override annotation should be on a separate line from the onPositiveClicked method for code consistency.

switch (instanceTag) {
case GOOGLE_ERROR_DIALOG_TAG:
// just dismiss the dialog
break;
}
}

private void dismissSignupSheet() {
if (mSignupSheet != null) {
mSignupSheet.dismiss();
Expand Down
1 change: 1 addition & 0 deletions WordPress/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@malinajirka malinajirka Aug 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<string name="login_error_generic_start">Google login could not be started.</string>
<string name="login_error_suffix">\nMaybe try a different account?</string>
<!-- Screen titles -->
Expand Down
96 changes: 61 additions & 35 deletions libs/login/WordPressLoginFlow/src/main/java/org/wordpress/android/login/GoogleFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@
import android.os.Bundle;
import android.support.annotation.NonNull;
import android.support.v4.app.Fragment;
import android.support.v7.app.AlertDialog;
import android.util.Log;
import android.view.ContextThemeWrapper;

import com.google.android.gms.auth.api.Auth;
import com.google.android.gms.auth.api.signin.GoogleSignInOptions;
Expand All @@ -21,14 +19,22 @@
import org.wordpress.android.fluxc.Dispatcher;
import org.wordpress.android.fluxc.store.SiteStore;
import org.wordpress.android.util.AppLog;
import org.wordpress.android.util.AppLog.T;

import javax.inject.Inject;

import static android.app.Activity.RESULT_OK;

public class GoogleFragment extends Fragment implements ConnectionCallbacks, OnConnectionFailedListener {
private static final String STATE_SHOULD_RESOLVE_ERROR = "STATE_SHOULD_RESOLVE_ERROR";
private static final String STATE_FINISHED = "STATE_FINISHED";
private static final String STATE_DISPLAY_NAME = "STATE_DISPLAY_NAME";
private static final String STATE_GOOGLE_EMAIL = "STATE_GOOGLE_EMAIL";
private static final String STATE_GOOGLE_TOKEN_ID = "STATE_GOOGLE_TOKEN_ID";
private static final String STATE_GOOGLE_PHOTO_URL = "STATE_GOOGLE_PHOTO_URL";
private boolean mIsResolvingError;
private boolean mShouldResolveError;
private boolean mFinished;

private static final String STATE_RESOLVING_ERROR = "STATE_RESOLVING_ERROR";
private static final int REQUEST_CONNECT = 1000;
Expand All @@ -52,14 +58,22 @@ public interface GoogleListener {
void onGoogleEmailSelected(String email);
void onGoogleLoginFinished();
void onGoogleSignupFinished(String name, String email, String photoUrl, String username);
void onGoogleSignupError(String msg);
}

@Override
public void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);

// Restore state of error resolving.
mIsResolvingError = savedInstanceState != null && savedInstanceState.getBoolean(STATE_RESOLVING_ERROR, false);
mDispatcher.register(this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the dispatcher be registered in onAttach rather than onCreate? It's unregistered in onDetach.

Copy link
Contributor Author

@malinajirka malinajirka Aug 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dispatcher is unregistered in onDestroy, therefor it's registered in the onCreate. This is one of the changes I'm not 100% sure about. But we want to receive the events even when we rotate the device -> the fragment is not attached to it's parent during configuration changes.

if (savedInstanceState != null) {
mIsResolvingError = savedInstanceState.getBoolean(STATE_RESOLVING_ERROR, false);
mShouldResolveError = savedInstanceState.getBoolean(STATE_SHOULD_RESOLVE_ERROR, false);
mFinished = savedInstanceState.getBoolean(STATE_FINISHED, false);
mDisplayName = savedInstanceState.getString(STATE_DISPLAY_NAME);
mGoogleEmail = savedInstanceState.getString(STATE_GOOGLE_EMAIL);
mIdToken = savedInstanceState.getString(STATE_GOOGLE_TOKEN_ID);
mPhotoUrl = savedInstanceState.getString(STATE_GOOGLE_PHOTO_URL);
}

// Configure sign-in to request user's ID, basic profile, email address, and ID token.
// ID and basic profile are included in DEFAULT_SIGN_IN.
Expand All @@ -71,7 +85,7 @@ public void onCreate(Bundle savedInstanceState) {
.build();

// Build Google API client with access to sign-in API and options specified above.
mGoogleApiClient = new GoogleApiClient.Builder(getActivity())
mGoogleApiClient = new GoogleApiClient.Builder(getActivity().getApplicationContext())
.addApi(Auth.GOOGLE_SIGN_IN_API, googleSignInOptions)
.addConnectionCallbacks(this)
.addOnConnectionFailedListener(this)
Expand All @@ -83,9 +97,15 @@ public void onCreate(Bundle savedInstanceState) {
}

@Override
public void onSaveInstanceState(Bundle outState) {
public void onSaveInstanceState(@NonNull Bundle outState) {
super.onSaveInstanceState(outState);
outState.putBoolean(STATE_RESOLVING_ERROR, mIsResolvingError);
outState.putBoolean(STATE_SHOULD_RESOLVE_ERROR, mShouldResolveError);
outState.putBoolean(STATE_FINISHED, mFinished);
outState.putString(STATE_DISPLAY_NAME, mDisplayName);
outState.putString(STATE_GOOGLE_EMAIL, mGoogleEmail);
outState.putString(STATE_GOOGLE_TOKEN_ID, mIdToken);
outState.putString(STATE_GOOGLE_PHOTO_URL, mPhotoUrl);
}

@Override
Expand All @@ -98,29 +118,24 @@ public void onAttach(Context context) {
} catch (ClassCastException exception) {
throw new ClassCastException(context.toString() + " must implement GoogleListener");
}

// Show account dialog when Google API onConnected callback returns before fragment is attached.
if (mGoogleApiClient != null && mGoogleApiClient.isConnected() && !mIsResolvingError && !mShouldResolveError) {
showAccountDialog();
if (mFinished) {
finishSignUp();
}
}

@Override
public void onDetach() {
super.onDetach();
@Override public void onDestroy() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @Override annotation should be on a separate line from the onDestroy method for code consistency.

disconnectGoogleClient();
AppLog.d(T.MAIN, "GOOGLE SIGNUP/IN: disconnecting google client");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For better differentiation between the two, we used Google login or signup rather than signin. Let's update the SIGNUP/IN to LOGIN/SIGNUP for consistency.

mDispatcher.unregister(this);
super.onDestroy();
}

@Override
public void onStart() {
super.onStart();
mDispatcher.register(this);
}

@Override
public void onStop() {
super.onStop();
mDispatcher.unregister(this);
@Override public void onResume() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @Override annotation should be on a separate line from the onResume method.

super.onResume();
// Show account dialog when Google API onConnected callback returns before fragment is attached.
if (mGoogleApiClient != null && mGoogleApiClient.isConnected() && !mIsResolvingError && !mShouldResolveError) {
startSignInProcess();
}
}

@Override
Expand All @@ -130,8 +145,11 @@ public void onConnected(Bundle bundle) {
if (mShouldResolveError) {
mShouldResolveError = false;

//noinspection StatementWithEmptyBody
if (isAdded()) {
showAccountDialog();
startSignInProcess();
} else {
// handled in onResume
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the else branch included? A comment above the if branch could explain the logic and a //noinspection suppression wouldn't be needed.

}
}
}
Expand All @@ -154,7 +172,7 @@ public void onConnectionFailed(@NonNull ConnectionResult connectionResult) {
mIsResolvingError = false;
AppLog.e(AppLog.T.NUX, GoogleApiAvailability.getInstance().getErrorString(
connectionResult.getErrorCode()));
showErrorDialog(getString(R.string.login_error_generic));
showError(getString(R.string.login_error_generic));
}
}
}
Expand All @@ -172,7 +190,7 @@ public void connectGoogleClient() {
mShouldResolveError = true;
mGoogleApiClient.connect();
} else {
showAccountDialog();
startSignInProcess();
}
}

Expand All @@ -183,16 +201,24 @@ protected void disconnectGoogleClient() {
}
}

protected void showAccountDialog() {
protected void startSignInProcess() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For better differentiation between the two, we used Google login or signup rather than signin. That's why showAccountDialog was chosen since the method is called for both flows. We should update this method name to maintain that convention.

// Do nothing here. This should be overridden by inheriting class.
}

protected void showErrorDialog(String message) {
AlertDialog dialog = new AlertDialog.Builder(new ContextThemeWrapper(getActivity(), R.style.LoginTheme))
.setMessage(message)
.setPositiveButton(R.string.login_error_button, null)
.create();
dialog.show();
protected void finishSignUp() {
/* This flag might get lost when the finishSignUp is called after the fragment's
onSaveInstanceState was called - however it's a very rare case, since the fragment is retained across
config changes. */
mFinished = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about mFinished. It's only used in onAttach where it calls the finishSignUp method if true. So is it's only purpose to recall the finishSignUp method if the device is rotated between the mFinished = true; and getActivity().getSupportFragmentManager().beginTransaction().remove(this).commitAllowingStateLoss(); statements?

Copy link
Contributor Author

@malinajirka malinajirka Aug 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it handles following situation

  1. Device is rotated
  2. Event is received and as result finishSignUp()/finishFlow() is called
  3. The mFinished is set to true and getActivity returns null or commitAllowingStateLoss is called, but since the configuration change is ongoing, the state is lost
  4. New Activity is created
  5. Retained GoogleFragments is attached
  6. It removes itself, because mFinished is true

It doesn't solve the situation when the retained GoogleFragment is destroyed by the system during configuration change (after onSaveInstanceState) and recreated, but it's such a rare case, that I believe it's ok to keep it as it is.

Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a comment to the flag.

if (getActivity() != null) {
AppLog.d(T.MAIN, "GOOGLE SIGNUP/IN: finishing signup");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For better differentiation between the two, we used Google login or signup rather than signin. Let's update the SIGNUP/IN to LOGIN/SIGNUP for consistency.

getActivity().getSupportFragmentManager().beginTransaction().remove(this).commitAllowingStateLoss();
}
}

protected void showError(String message) {
finishSignUp();
mGoogleListener.onGoogleSignupError(message);
}

@Override
Expand All @@ -208,7 +234,7 @@ public void onActivityResult(int request, int result, Intent data) {
if (!mGoogleApiClient.isConnecting() && !mGoogleApiClient.isConnected()) {
mGoogleApiClient.connect();
} else {
showAccountDialog();
startSignInProcess();
}

mIsResolvingError = false;
Expand Down
2 changes: 1 addition & 1 deletion ...ogin/WordPressLoginFlow/src/main/java/org/wordpress/android/login/LoginEmailFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public void onClick(View view) {
if (isAdded()) {
mOldSitesIDs = SiteUtils.getCurrentSiteIds(mSiteStore, false);
mIsSocialLogin = true;
mLoginListener.addGoogleLoginFragment(LoginEmailFragment.this);
mLoginListener.addGoogleLoginFragment();
} else {
AppLog.e(T.NUX, "Google login could not be started. LoginEmailFragment was not attached.");
showErrorDialog(getString(R.string.login_error_generic_start));
Expand Down
Loading