Track signed in event on sign up by planarvoid · Pull Request #10280 · wordpress-mobile/WordPress-Android

planarvoid · 2019-07-25T08:13:02Z

I've tried to identify the places where we're sure we have a token during the signup. I think there are 2 things that could happen. One is a successful Google signup and the other one is going through the magic link and getting the token there. I've covered these two use cases but there might be something wrong because the flow is really hard to follow.

As part of this PR I did a very small refactoring where I moved the AccountStore and SiteStore to the constructor of the LoginAnalyticsModule instead of passing it as parameters. I think that's the correct approach since the module is injected.

Let me know if this approach makes sense!

To test:

Check that the sign-up flow isn't broken.

Update release notes:

If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

peril-wordpress-mobile · 2019-07-25T08:13:07Z

	Messages
📖	This PR contains changes in the subtree `libs/login/`. It is your responsibility to ensure these changes are merged back into `WordPress-Login-Flow-Android`. Follow these handy steps! WARNING: Make sure your git version is 2.19.x or lower - there is currently a bug in later versions that will corrupt the subtree history! `cd WordPress-Android` `git checkout track_signed_in_event_on_sign_up` `git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/WordPress-Android/10280` Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/WordPress-Android/10280 and open a new PR.

Generated by 🚫 dangerJS

aforcier · 2019-07-29T15:50:10Z

WordPress/src/main/java/org/wordpress/android/ui/accounts/LoginActivity.java

    public void onGoogleSignupFinished(String name, String email, String photoUrl, String username) {
        AnalyticsTracker.track(AnalyticsTracker.Stat.SIGNUP_SOCIAL_SUCCESS);
-
+        mLoginAnalyticsListener.trackAnalyticsSignIn(false);


Shouldn't this be trackAnalyticsSignIn(true)? I believe the flag is meant to identify self-hosted vs WP.com logins, and WP.com signups should count as the latter.

(The same would apply to the change in WPMainActivity.)

you're right of course! good catch

aforcier · 2019-07-29T15:55:41Z

@planarvoid aside from my comment this looks good to me. Definitely 👍 on the DI refactor.

It's probably worth having someone who worked on signup take a look as well though as I agree the flow is fairly complex. @theck13 could you give this a once-over?

theck13

I agree with @aforcier that I believe both mLoginAnalyticsListener.trackAnalyticsSignIn(false); statements should be mLoginAnalyticsListener.trackAnalyticsSignIn(true);.

I'm not sure, but we might want to call mLoginAnalyticsListener.trackAnalyticsSignIn(true); immediately after LoginAnalyticsListener.trackCreatedAccount(username, email) instances. @aerych, can you confirm that's what is intended with the issue?

aerych · 2019-07-30T02:25:28Z

can you confirm that's what is intended with the issue?

Basically, once we've obtained the user's bearer token, we want to bump the signed in stat. If that happens immediately after we bump account created, then yes.

planarvoid · 2019-07-30T11:32:05Z

@theck13 You're definitely right with the SignupGoogleFragment, it was already happening at the same moment so I moved the both calls next to each other.
It's hard for me to find out about the WPMainActivity case. I'm basically tracking the moment when we retrieve the token from the magic link and I'm not sure what steps are happening between that and onAccountChanged call which triggers the trackMagicLinkSignupIfNeeded. WDYT?

aforcier · 2019-07-30T17:31:34Z

👍 to the move to the SignupGoogleFragment.

For the WPMainActivity (signup via magic link) case, I'm not completely sure. Here's my take:

Where it currently is, it triggers when we extract the token from the magic link. This matches the criterion of "once we've obtained the user's bearer token" for dispatching the tracks event, but at this point we wouldn't know if the token was invalid for any reason.

If we move it to where trackCreatedAccount is called (inside trackMagicLinkSignupIfNeeded()), we're relying on these things having gone well:

Token is registered with FluxC properly
AccountAction.FETCH_ACCOUNT was dispatched and came back from the server successfully
The account has been updated with a username and e-mail

My instinct would be to leave it where it is, as some of those things can go wrong and succeed later after retrying without needing a new token (so I would count the user as already 'logged in').

Since ultimately we're trying to be consistent with iOS though, @aerych could you tell us at which point this event is fired on that platform? Is it right when the token is obtained from the magic link or sometime later?

aforcier · 2019-07-30T17:32:52Z

Note: I'm taking this PR over for @planarvoid since he's going on vacation.

jkmassel · 2019-07-30T18:23:23Z

I'm bumping this to 13.1 as part of the 13.0 code freeze.

If you'd like it to be released as part of 13.0, please merge it against release/13.0 then DM me, and I'll be happy to issue a new beta release! :)

aerych · 2019-07-30T19:14:59Z

at which point this event is fired on that platform? Is it right when the token is obtained from the magic link or sometime later?

Its actually a bit later. (My earlier comment wasn't terribly accurate.) The tl;dr is we bump the signed in stat once account information has been synced. This is true for both sign up and log in flows.

Here are the relevant code points:

The magic links is clicked, the app is launched, and the token extracted. The user is presented with an instance of the NUXLinkAuthViewController to look at while account information is synced. The account created stat is bumped from here. Note that the stat is bumped while the api call(s) to sync account information are in flight.
To sync account info,NUXLinkAuthViewController calls a method declared in LoginViewController called syncWPComAndPresentEpilogue #. This in turn calls syncWPCom #, which calls a method on the authentication delegate (the actual WordPress app) to perform the sync. When syncing account and blog information is complete the completion callback block is executed, and we see that here is where we finally bump the signed in stat.

aforcier · 2019-07-30T21:34:33Z

@aerych thanks for the walkthrough on that!

Then, given that:
a. iOS is bumping the stat after account info is fetched and we want to be as consistent as possible, and
b. We're storing whether signup tracks need to be sent or not in a preference (AppPrefs.getShouldTrackMagicLinkSignup()), which will persist and should cause the event to be tracked on the first successful account fetch even if the app is restarted,

I propose we move the call to trackAnalyticsSignIn() to the same place we call trackCreatedAccount() for the e-mail sign up flow, as @theck13 had suggested:

WordPress-Android/WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java

Lines 969 to 976 in 1d03591

    
           private void trackMagicLinkSignupIfNeeded() { 
        
               AccountModel account = mAccountStore.getAccount(); 
        
               if (!TextUtils.isEmpty(account.getUserName()) && !TextUtils.isEmpty(account.getEmail())) { 
        
                   mLoginAnalyticsListener.trackCreatedAccount(account.getUserName(), account.getEmail()); 
        
                   mLoginAnalyticsListener.trackSignupMagicLinkSucceeded(); 
        
                   AppPrefs.removeShouldTrackMagicLinkSignup(); 
        
               } 
        
           }

WDYT @theck13 ?

theck13 · 2019-07-30T21:45:50Z

Just for clarification, we would only move the trackAnalyticsSignIn() instance in WPMainActivity to the location in your code snippet above and leave the other trackAnalyticsSignIn() instances as is, right? If so, that makes sense to me.

aforcier · 2019-07-30T21:47:08Z

@theck13 yep! Just this one would be moved.

This is more consistent with when iOS dispatches a signed in event during e-mail sign up.

aerych · 2019-07-31T14:46:35Z

Thank you, everyone, for wrangling this!

planarvoid added 2 commits July 25, 2019 09:20

Track signed in event on sign up

a2ddffc

Fix dagger injection

775a893

planarvoid added the Signup label Jul 25, 2019

planarvoid added this to the 13.0 milestone Jul 25, 2019

planarvoid requested a review from aforcier July 25, 2019 08:13

planarvoid self-assigned this Jul 25, 2019

aforcier assigned aforcier and unassigned planarvoid Jul 29, 2019

aforcier reviewed Jul 29, 2019

View reviewed changes

theck13 reviewed Jul 30, 2019

View reviewed changes

Change isWpCom flag to true

16db036

Move signedin tracking to the google fragment

1d03591

aforcier assigned theck13 Jul 30, 2019

jkmassel modified the milestones: 13.0, 13.1 Jul 30, 2019

Pair signed in tracking event with account created event

8ba1951

This is more consistent with when iOS dispatches a signed in event during e-mail sign up.

theck13 approved these changes Jul 30, 2019

View reviewed changes

theck13 merged commit f76d724 into develop Jul 30, 2019

theck13 deleted the track_signed_in_event_on_sign_up branch July 30, 2019 21:57

aforcier mentioned this pull request Jul 30, 2019

Merge from WPAndroid: Sign up logged-in analytics wordpress-mobile/WordPress-Login-Flow-Android#25

Merged

aforcier mentioned this pull request Jul 31, 2019

Merge in latest changes from login library woocommerce/woocommerce-android#1312

Merged

1 task

Conversation

planarvoid commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Jul 25, 2019

Uh oh!

aforcier Jul 29, 2019

Choose a reason for hiding this comment

Uh oh!

aforcier Jul 29, 2019

Choose a reason for hiding this comment

Uh oh!

planarvoid Jul 30, 2019

Choose a reason for hiding this comment

Uh oh!

aforcier commented Jul 29, 2019

Uh oh!

theck13 left a comment

Choose a reason for hiding this comment

Uh oh!

aerych commented Jul 30, 2019

Uh oh!

planarvoid commented Jul 30, 2019

Uh oh!

aforcier commented Jul 30, 2019

Uh oh!

aforcier commented Jul 30, 2019

Uh oh!

jkmassel commented Jul 30, 2019

Uh oh!

aerych commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aforcier commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theck13 commented Jul 30, 2019

Uh oh!

aforcier commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aerych commented Jul 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

planarvoid commented Jul 25, 2019 •

edited

Loading

aerych commented Jul 30, 2019 •

edited

Loading

aforcier commented Jul 30, 2019 •

edited

Loading

aforcier commented Jul 30, 2019 •

edited

Loading