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 all 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
54 changes: 31 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,26 @@ public void onGoogleSignupFinished(String name, String email, String photoUrl, S
finish();
}

@Override
public void onGoogleSignupError(String msg) {
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) {
switch (instanceTag) {
case GOOGLE_ERROR_DIALOG_TAG:
// just dismiss the dialog
break;
}
}

private void dismissSignupSheet() {
if (mSignupSheet != null) {
mSignupSheet.dismiss();
Expand Down
3 changes: 2 additions & 1 deletion WordPress/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2064,8 +2064,9 @@
<string name="login_epilogue_mysites_other">My sites</string>
<string name="login_google_button_suffix">Log in with Google.</string>
<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_email_not_found_v2">There\'s no WordPress.com account matching this Google account.</string>
<string name="login_error_generic">There was some trouble connecting with the Google account.</string>
<string name="login_error_sms_throttled">We\'ve made too many attempts to send an SMS verification code — take a break, and request a new one in a minute.</string>
<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: 63 additions & 33 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,26 @@
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;
/**
* This flag is used to store the information the finishFlow was called when the fragment wasn't attached to an
* activity (for example an EventBus event was received during ongoing configuration change).
*/
private boolean mFinished;

private static final String STATE_RESOLVING_ERROR = "STATE_RESOLVING_ERROR";
private static final int REQUEST_CONNECT = 1000;
Expand All @@ -52,14 +62,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 +89,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 +101,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 +122,26 @@ 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) {
finishFlow();
}
}

@Override
public void onDetach() {
super.onDetach();
public void onDestroy() {
disconnectGoogleClient();
AppLog.d(T.MAIN, "GOOGLE SIGNUP/LOGIN: disconnecting google client");
mDispatcher.unregister(this);
super.onDestroy();
}

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

@Override
public void onStop() {
super.onStop();
mDispatcher.unregister(this);
public void onResume() {
super.onResume();
// Show account dialog when Google API onConnected callback returns before fragment is attached.
if (mGoogleApiClient != null && mGoogleApiClient.isConnected() && !mIsResolvingError && !mShouldResolveError) {
startFlow();
}
}

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

// if the fragment is not attached to an activity, the process is started in the onResume
if (isAdded()) {
showAccountDialog();
startFlow();
}
}
}
Expand All @@ -154,7 +176,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 +194,7 @@ public void connectGoogleClient() {
mShouldResolveError = true;
mGoogleApiClient.connect();
} else {
showAccountDialog();
startFlow();
}
}

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

protected void showAccountDialog() {
protected void startFlow() {
// 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 finishFlow() {
/* This flag might get lost when the finishFlow 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;
if (getActivity() != null) {
AppLog.d(T.MAIN, "GOOGLE SIGNUP/LOGIN: finishing signup/login");
getActivity().getSupportFragmentManager().beginTransaction().remove(this).commitAllowingStateLoss();
}
}

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

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

mIsResolvingError = false;
Expand Down
2 changes: 1 addition & 1 deletion .../login/WordPressLoginFlow/src/main/java/org/wordpress/android/login/Login2FaFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ public class Login2FaFragment extends LoginBaseFormFragment<LoginListener> imple

private static final String TWO_FACTOR_TYPE_AUTHENTICATOR = "authenticator";
private static final String TWO_FACTOR_TYPE_BACKUP = "backup";
private static final String TWO_FACTOR_TYPE_SMS = "sms";

public static final String TWO_FACTOR_TYPE_SMS = "sms";
public static final String TAG = "login_2fa_fragment_tag";

private static final Pattern TWO_STEP_AUTH_CODE = Pattern.compile("^[0-9]{6}");
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