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.

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

Add click tracking to unified login and signup flow #12117

Conversation

planarvoid
Copy link
Contributor

@planarvoid planarvoid commented Jun 5, 2020

Fixes #11797

This PR adds tracking to all the clicks in the login flow. The events tracked are unified_login_interaction (do you think it would be better to use "click" instead?). The event contains the previous source/flow/step to know for sure what happened before in the flow. I tried to cover all the clicks by looking for all the onClickListeners but it's possible I missed some of the cases.

To test:

  • Filter unified_login_interaction in Logcat
  • Try various login flows
  • Check that all the clicks are tracked and that the value tracked makes sense

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@planarvoid planarvoid added [Type] Task Unified Login & Sign-Up Part of a WIP Feature This label is used to disable milestone checks for PRs that are not against `develop` or `release`. labels Jun 5, 2020
@planarvoid planarvoid self-assigned this Jun 5, 2020
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 5, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

Messages
📖

This PR contains changes in the subtree libs/login/. It is your responsibility to ensure these changes are merged back into wordpress-mobile/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!

  1. cd WordPress-Android
  2. git checkout feature/unified-login-interaction-tracking
  3. git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/WordPress-Android/12117
  4. Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/WordPress-Android/12117 and open a new PR.

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 5, 2020

You can test the changes on this Pull Request by downloading the APK here.

@planarvoid planarvoid marked this pull request as ready for review June 23, 2020 12:21
Copy link
Member

@renanferrari renanferrari left a comment

Choose a reason for hiding this comment

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

Overall code looks good, and I couldn't find any missing relevant click event.

I did found a bug though: when we click the Continue with Google button and the Google dialog appears, the flow changes to google_login, but if we dismiss the dialog, the flow stays as google_login. This causes the click to the Continue or help button to be logged as in the google_login flow.

Answering your question:

The events tracked are unified_login_interaction (do you think it would be better to use "click" instead?).

I think this depends on the scope we want to track here. If we want to limit to button taps, then using the term "click" is fine. If we also want to log other interactions, like for example, ViewPager swipes, then we should replace the term "click" to something more generic, and "interaction" seems like a good choice.

@planarvoid
Copy link
Contributor Author

planarvoid commented Jun 24, 2020

thanks for the review @renanferrari

I did found a bug though: when we click the Continue with Google button and the Google dialog appears, the flow changes to google_login, but if we dismiss the dialog, the flow stays as google_login. This causes the click to the Continue or help button to be logged as in the google_login flow.

This is a great catch! I've implemented a solution that sets the flow & step in onResume in the Email and Prologue fragments. It doesn't seem to be issue anywhere else (as far as I can see). I think it's because the other fragments are not kept when you jump around). It seems to work fine now. Let me know what you think!

I think this depends on the scope we want to track here. If we want to limit to button taps, then using the term "click" is fine. If we also want to log other interactions, like for example, ViewPager swipes, then we should replace the term "click" to something more generic, and "interaction" seems like a good choice.

Right now it's only clicks but let's keep the name unified_login_interaction so we can add more later. It's hard to change the event name once it's part of funnels so I'd keep the more abstract name.

Base automatically changed from feature/unified-login-failure-tracking to feature/unified-login-signup June 24, 2020 17:55
Copy link
Member

@renanferrari renanferrari left a comment

Choose a reason for hiding this comment

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

I've implemented a solution that sets the flow & step in onResume in the Email and Prologue fragments. It doesn't seem to be issue anywhere else (as far as I can see). I think it's because the other fragments are not kept when you jump around). It seems to work fine now. Let me know what you think!

I can confirm it's working as expected now. Thanks for the changes!

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Part of a WIP Feature This label is used to disable milestone checks for PRs that are not against `develop` or `release`. [Type] Task Unified Login & Sign-Up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants