[go: nahoru, domu]

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

Feature/4263 replace optimizely with mixpanel #4264

Merged
merged 14 commits into from
Aug 5, 2016

Conversation

kwonye
Copy link
Contributor
@kwonye kwonye commented Jun 23, 2016

Fixes #4263

Well that was easy...

One thing we do lose is Tracks integration, but since we are using Mixpanel for analytics anyways, it's not a big deal.

To test:

Check out Mixpanel and see that there is a "Magic Link" experiment and that Variation 1 has showMagicLink boolean set to true.

Needs review: @maxme

cc: @aerych

@kwonye kwonye added this to the 5.6 milestone Jun 23, 2016
@maxme
Copy link
Contributor
maxme commented Jun 23, 2016

@kwonye did you try it?

I enabled it in Mixapnel and I tried like 20 times (resetting the emulator between each test), I wasn't able to get the magic link screen once.

From the Mixpanel documentation:

Mixpanel checks for any new experiments asynchronously each time your application is opened or resumed. After the response is received, experiment changes and tweaks are applied or removed where appropriate. To handle network availability, each experiment is cached on the device so they can be re-applied when the API call cannot be successfully made.

If you'd like more control over when this check for new experiments occurs, you can use the addOnMixpanelUpdatesReceivedListener listener and the joinExperimentIfAvailable method to download and apply experiments manually.

Also there is something weird, when I enabled it, I saw usage stats grow (current users already "counted" in "variant #1" even if this patch hasn't shipped yet).

@kwonye
Copy link
Contributor Author
kwonye commented Jun 23, 2016

From the Mixpanel documentation:

Thanks @maxme I overlooked the asyncronous start of it. I'll figure out how we can show the correct login screen on start up.

Also there is something weird, when I enabled it, I saw usage stats grow (current users already "counted" in "variant #1" even if this patch hasn't shipped yet).

Yea, there's no way 400+ people saw that... I went ahead and deleted that test to make sure it wasn't doing anything funny. I'll investigate what was going on there.

@kwonye
Copy link
Contributor Author
kwonye commented Jun 24, 2016

After investigation and conversation into what it will take to have this A/B test work with Mixpanel, we're going to leave Optimizely in for the Magic Link A/B test. Moving this PR to 5.7 and removing the Mixpanel magic link boolean.

@kwonye
Copy link
Contributor Author
kwonye commented Jun 24, 2016

Ready to merge AFTER release/5.6 has been cut from develop

@kwonye kwonye modified the milestones: 5.8, 5.7 Aug 1, 2016
…anel

# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/ActivityLauncher.java
@kwonye
Copy link
Contributor Author
kwonye commented Aug 1, 2016

@maxme ready for another pass as we have solid data from 5.6.x's Optimizely A/B test.

By 5.8 we'll remove the magic link test so it doesn't matter that it won't show this A/B test in the beginning.

@maxme
Copy link
Contributor
maxme commented Aug 2, 2016

I'm seeing this:

screen shot 2016-08-02 at 09 15 51

Can we wait until we have significant numbers?

@kwonye
Copy link
Contributor Author
kwonye commented Aug 3, 2016

@maxme sorry I was unclear, my fault. I meant to say:

I believe we will make a decision about Magic Links by the time we ship 5.8, so we can remove Optimizely right now for develop.

@maxme
Copy link
Contributor
maxme commented Aug 3, 2016

I believe we will make a decision about Magic Links by the time we ship 5.8, so we can remove Optimizely right now for develop.

Ok, but then I'm not sure why you're using this Tweak (Mixpanel A/B test variable) to show the Magic Links or not, should we just wait for the AB test to end and then remove this? (Or eventually use a feature flag if we think we can make that feature better in the future?)

@kwonye
Copy link
Contributor Author
kwonye commented Aug 4, 2016

Again, sorry, my fault. I was already working on having Magic Links be the default SignInActivity in another PR, but that was before you showed me that the significance was actually too low.

I've removed the A/B test from ActivityLauncher and we default to showing the original sign in screen until we make a decision 👍

@@ -259,7 +259,7 @@ public static void newBlogForResult(Activity activity) {
}

public static void showSignInForResult(Activity activity) {
if (WPActivityUtils.isEmailClientAvailable(activity)) {
if (shouldShowMagicLinksLogin(activity);) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The extra ; here breaks the build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... no excuses here, sorry about that.

@kwonye
Copy link
Contributor Author
kwonye commented Aug 5, 2016

I'm sorry @maxme, that was a careless mistake on my part. Fixed and verified working.

@maxme
Copy link
Contributor
maxme commented Aug 5, 2016

Sorry one last pass:

  • can you clean up the unused imports from ActivityLauncher
  • and remove the optimizely token from WordPress/gradle.properties-example

@kwonye
Copy link
Contributor Author
kwonye commented Aug 5, 2016

Wow, great catches. Fixed, thanks.

@maxme
Copy link
Contributor
maxme commented Aug 5, 2016

:shipit: great to have faster launch speed for unsigned users ;)

@maxme maxme merged commit 32683e2 into develop Aug 5, 2016
@maxme maxme deleted the feature/4263-replace-optimizely-with-mixpanel branch August 5, 2016 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants