Skip to content
This repository was archived by the owner on Feb 5, 2025. It is now read-only.

Add signed_in Tracks event whenever we log in socially #156

Merged
merged 3 commits into from
Nov 22, 2019
Merged

Add signed_in Tracks event whenever we log in socially #156

merged 3 commits into from
Nov 22, 2019

Conversation

@frosty
Copy link
Contributor

@frosty frosty commented Nov 20, 2019

Refs wordpress-mobile/WordPress-iOS#12827. This PR adds signedIn tracks events everywhere we're already posting a loginSocialSuccess event. You can test this PR with the associated WPiOS PR here: wordpress-mobile/WordPress-iOS#12981

I updated 4 locations where I could find usages of loginSocialSuccess with no associated signedIn. I'm not 100% sure if these are correct, so please let me know what you think! Here's my thoughts on each one:

  • Login2FAController: I'm a little unsure about this one, as the line above mentions Google login but I'm honestly not quite sure how to trigger this method. It doesn't appear to get called by doing a normal sign in with 2FA.
  • LoginEmailViewController: I updated this one, but as far as I can tell this method is actually not used any more, as we no longer display the Google option on this screen. Since the addition of Sign in with Apple, we moved it as an option at the start of the process.
  • LoginViewController: I also couldn't seem to get this one called. I'm not sure if it's used when connecting an Apple account for the first time? It doesn't seem to be called for my already-connected Google or Apple accounts.
  • SignupGoogleViewController: This one is definitely called when I log in with my Google account :)

To test

  • Review the code and see if you think these placements look okay
  • As far as possible, exercise each event here. As I mentioned, I don't think the email one is used any more, and I could only get the SignupGoogleViewController method called (by signing in with my existing Google account).
  • Any thoughts you have on this would be very welcome!

@frosty frosty requested review from ScoutHarris and aerych November 20, 2019 15:03
@ScoutHarris
Copy link
Contributor

ScoutHarris commented Nov 20, 2019 •
edited
Loading

  • Login2FAViewController - It looks like this particular method is used by social + 2fa accounts. Specifically, I think it's just used by Google.
  • LoginEmailViewController - you are correct.
  • LoginViewController - again correct. linkSocialService gets called when you first use either social account type, but not after.

ScoutHarris
ScoutHarris reviewed Nov 20, 2019
View reviewed changes
WordPressAuthenticator/Signin/Login2FAViewController.swift
// Disconnect now that we're done with Google.
GIDSignIn.sharedInstance().disconnect()

WordPressAuthenticator.track(.signedIn)
Copy link
Contributor

@ScoutHarris ScoutHarris Nov 20, 2019

Choose a reason for hiding this comment

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

I think we can add a ["source": "google"] here since this is only used by Google as far as I can tell. Thoughts?

Copy link
Contributor Author

@frosty frosty Nov 20, 2019

Choose a reason for hiding this comment

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

I'm happy to do that if that's the case, I just couldn't work out if it was used by not-Google logins. There are no other references to Google in here other than line 180, and the loginSocialSuccess call below doesn't mention Google so I wasn't sure!

Copy link
Contributor

@ScoutHarris ScoutHarris Nov 20, 2019

Choose a reason for hiding this comment

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

Well, I just tried with an Apple account and was finally able to repro wordpress-mobile/WordPress-iOS#12477! 🤦‍♂

I'd be ok with leaving it generic for now. Currently, it is only google, but hard-coding that is probably dangerous and might be wrong when 12477 is fixed.

Copy link
Contributor Author

@frosty frosty Nov 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

👍 Thank you for checking!

ScoutHarris
ScoutHarris approved these changes Nov 20, 2019
View reviewed changes
Copy link
Contributor

@ScoutHarris ScoutHarris left a comment

Choose a reason for hiding this comment

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

Hey @frosty . I had one question about 2fa tracking, but otherwise looks good.

:shipit:

aerych
aerych suggested changes Nov 20, 2019
View reviewed changes
Copy link
Contributor

@aerych aerych left a comment

Choose a reason for hiding this comment

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

Heya @frosty!
Thanks for wrangling this. I had one nitpick, and a thought about another place or two we should be bumping the stat. Pending @ScoutHarris's thoughts on it we might nuke what seems like some dead code either in this or another PR. Back to you sir!

WordPressAuthenticator/Signin/LoginViewController.swift Outdated
serviceToken: serviceToken,
connectParameters: appleConnectParameters,
success: {
let source: String = appleConnectParameters != nil ? "apple" : "google"
Copy link
Contributor

@aerych aerych Nov 20, 2019

Choose a reason for hiding this comment

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

We could omit the type specifier here and let it be inferred.

WordPressAuthenticator/Signin/LoginEmailViewController.swift

// Disconnect now that we're done with Google.
GIDSignIn.sharedInstance().disconnect()
WordPressAuthenticator.track(.signedIn, properties: ["source": "google"])
Copy link
Contributor

@aerych aerych Nov 20, 2019

Choose a reason for hiding this comment

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

Wasn't able to reach this code path. I'm guessing its orphaned code now that the social options are available from the prologue vs the LoginEmailViewController. @ScoutHarris, thoughts?

Copy link
Contributor

@ScoutHarris ScoutHarris Nov 20, 2019

Choose a reason for hiding this comment

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

That code is sort of feature flagged via the showNewLoginFlow flag in WordPressAuthenticatorConfiguration. I would suggest removing that flag and associated code in a separate PR. I'm pretty sure there's some storyboard mojo that needs to be removed as well. Yes, it's totally my fault - I haven't cleaned that up post SIWA launch.

Copy link
Contributor Author

@frosty frosty Nov 22, 2019

Choose a reason for hiding this comment

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

After further inspection, I think Woo is still using this code? They still show the Google login option on the email screen. So we can leave this for now I think.

Copy link
Contributor

@aerych aerych Nov 22, 2019

Choose a reason for hiding this comment

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

Good catch!

@frosty
Copy link
Contributor Author

frosty commented Nov 22, 2019

Sorry for the delay - @aerych I pushed up some changes to address your feedback.

@frosty frosty requested a review from aerych November 22, 2019 16:27
aerych
aerych approved these changes Nov 22, 2019
View reviewed changes
Copy link
Contributor

@aerych aerych left a comment

Choose a reason for hiding this comment

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

Thanks @frosty! Looks good to me. :shipit: !

@frosty frosty merged commit 01967f8 into develop Nov 22, 2019
@frosty frosty deleted the fix/social-signed-in-event branch November 22, 2019 21:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@ScoutHarris ScoutHarris ScoutHarris approved these changes

@aerych aerych aerych approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@frosty @ScoutHarris @aerych