-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
@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:
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). |
Thanks @maxme I overlooked the asyncronous start of it. I'll figure out how we can show the correct login screen on start up.
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. |
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. |
…gn in for right now)
Ready to merge AFTER |
…anel # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/ActivityLauncher.java
@maxme ready for another pass as we have solid data from 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 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 |
Ok, but then I'm not sure why you're using this |
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 |
@@ -259,7 +259,7 @@ public static void newBlogForResult(Activity activity) { | |||
} | |||
|
|||
public static void showSignInForResult(Activity activity) { | |||
if (WPActivityUtils.isEmailClientAvailable(activity)) { | |||
if (shouldShowMagicLinksLogin(activity);) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I'm sorry @maxme, that was a careless mistake on my part. Fixed and verified working. |
Sorry one last pass:
|
Wow, great catches. Fixed, thanks. |
great to have faster launch speed for unsigned users ;) |
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