Skip to content

Feature/social login analytics - #6822

Merged
theck13 merged 12 commits into
developfrom
feature/social-login-analytics
Nov 3, 2017
Merged

Feature/social login analytics#6822
theck13 merged 12 commits into
developfrom
feature/social-login-analytics

Conversation

@aerych

@aerych aerych commented Nov 2, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Refs #6505
Adds social login analytics. These are the same as Calypso except for the following:

"login_social_2fa_needed"
"login_social_accounts_need_connecting"
"login_social_error_unknown_user"

To test:
Test social login.
Ensure that each analytic is being bumped correctly.

(Companion iOS version wordpress-mobile/WordPress-iOS#8028)

@aerych aerych added the Login label Nov 2, 2017
@aerych aerych added this to the 8.7 milestone Nov 2, 2017
@theck13
theck13 self-requested a review November 2, 2017 17:13
theck13
theck13 suggested changes Nov 2, 2017
View reviewed changes

@theck13 theck13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Adding analytics was a lot more complicated than I expected. I left a few questions and suggestions. Everything else looks good!

Comment thread WordPress/src/main/java/org/wordpress/android/ui/accounts/LoginActivity.java
@Override
public void loggedInViaSocialAccount(ArrayList<Integer> oldSitesIds) {
AnalyticsTracker.track(AnalyticsTracker.Stat.LOGIN_GOOGLE_SUCCEEDED);
AnalyticsTracker.track(AnalyticsTracker.Stat.LOGIN_SOCIAL_SUCCESS);

@theck13 theck13 Nov 2, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Having both LOGIN_GOOGLE_SUCCEEDED and LOGIN_SOCIAL_SUCCESS seems unnecessary if Google is the only social login. Should we remove LOGIN_GOOGLE_SUCCEEDED until other social login types are added since both of these stats are triggered at the same time and LOGIN_SOCIAL_SUCCESS corresponds to Calypso?

@aerych aerych Nov 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I waffled a bit on removing it, but I agree. I'll remove it.

Comment thread WordPress/src/main/java/org/wordpress/android/ui/accounts/login/Login2FaFragment.java Outdated
AnalyticsUtils.trackAnalyticsSignIn(mAccountStore, mSiteStore, true);

if (isSocialLogin) {
AnalyticsTracker.track(AnalyticsTracker.Stat.LOGIN_SOCIAL_SUCCESS);

@theck13 theck13 Nov 2, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The LOGIN_SOCIAL_SUCCESS stat is being tracked in LoginActivity.loggedInViaSocialAccount, which is being called just below this line. That will cause double-counting LOGIN_SOCIAL_SUCCESS. I think we can remove this line.

Comment thread WordPress/src/main/java/org/wordpress/android/ui/accounts/login/LoginEmailPasswordFragment.java Outdated
AnalyticsUtils.trackAnalyticsSignIn(mAccountStore, mSiteStore, true);

if (isSocialLogin) {
AnalyticsTracker.track(AnalyticsTracker.Stat.LOGIN_SOCIAL_SUCCESS);

@theck13 theck13 Nov 2, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This LOGIN_SOCIAL_SUCCESS stat is being double-counted here too since LoginActivity.loggedInViaSocialAccount is called just below. I think we can remove this line.

Comment thread WordPress/src/main/java/org/wordpress/android/ui/accounts/login/Login2FaFragment.java Outdated
AnalyticsTracker.track(AnalyticsTracker.Stat.LOGIN_TWO_FACTOR_FORM_VIEWED);
if (mService != null) {
AnalyticsTracker.track(AnalyticsTracker.Stat.LOGIN_SOCIAL_2FA_NEEDED);
}

@theck13 theck13 Nov 2, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the LOGIN_SOCIAL_2FA_NEEDED stat meant to be tracked when the two-factor authentication screen is shown during a social login? If so, the LoginActivity.needs2faSocial and LoginActivity.needs2faSocialConnect methods would be better locations for that. Those methods are called when the Login2FaFragment is shown from LoginGoogleFragment or LoginEmailPasswordFragment (i.e. the only places two-factor authentication can be shown from social login).

@aerych aerych Nov 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah! Def a much better location. Will do!

Comment thread WordPress/src/main/java/org/wordpress/android/ui/accounts/login/LoginEmailFragment.java Outdated
@Override
public void onClick(View view) {
AnalyticsTracker.track(AnalyticsTracker.Stat.LOGIN_SOCIAL_BUTTON_CLICK);
AnalyticsTracker.track(AnalyticsTracker.Stat.LOGIN_GOOGLE_TAPPED);

@theck13 theck13 Nov 2, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems like another redundant stat. Should we remove LOGIN_GOOGLE_TAPPED in favor of LOGIN_SOCIAL_BUTTON_CLICK?

@aerych aerych Nov 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I'll remove this one too.

Comment thread WordPress/src/main/java/org/wordpress/android/ui/accounts/login/LoginGoogleFragment.java
AnalyticsTracker.track(AnalyticsTracker.Stat.LOGIN_FAILED, event.getClass().getSimpleName(),
event.error.type.toString(), event.error.message);

if (event.error.type != AccountStore.AccountSocialErrorType.USER_EXISTS) {

@theck13 theck13 Nov 2, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The import org.wordpress.android.fluxc.store.AccountStore; statement needs to be added to be able to build with this conditional.

@aerych aerych Nov 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How this could have been omitted baffles me. Adding back.

@aerych

aerych commented Nov 3, 2017

Copy link
Copy Markdown
Contributor Author

Thanks @theck13 !! Changes made. Ready for another peek.

@theck13

theck13 commented Nov 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

It looks like some references to LOGIN_GOOGLE_SUCCEEDED and LOGIN_GOOGLE_TAPPED slipped by. Once those are removed, the build should pass for Travis.

@aerych

aerych commented Nov 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

It looks like some references to LOGIN_GOOGLE_SUCCEEDED and LOGIN_GOOGLE_TAPPED slipped by

theck13
theck13 approved these changes Nov 3, 2017
View reviewed changes

@theck13 theck13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

@theck13
theck13 merged commit 17a5ee4 into develop Nov 3, 2017
@theck13
theck13 deleted the feature/social-login-analytics branch November 3, 2017 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@theck13 theck13 theck13 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Login

Projects

None yet

Milestone

8.7

Development

Successfully merging this pull request may close these issues.

2 participants

@aerych @theck13