From 02049930c9679287dd9a934a8da458cde04bf947 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Wed, 25 Apr 2012 17:47:45 -0700 Subject: [PATCH 01/19] Started cleaning up and refactoring the AB testing classes to fit with the new analytics framework --- .../astrid/activity/TaskListActivity.java | 27 --- .../service/AstridDependencyInjector.java | 4 +- .../astrid/service/StartupService.java | 28 ++- .../astrid/service/StatisticsConstants.java | 7 - .../astrid/service/StatisticsService.java | 14 +- .../astrid/service/abtesting/ABChooser.java | 42 ++-- .../astrid/service/abtesting/ABOptions.java | 194 ------------------ .../astrid/service/abtesting/ABTests.java | 147 +++++++++++++ .../service/abtesting/FeatureFlipper.java | 6 +- .../astrid/utility/AstridPreferences.java | 6 +- 10 files changed, 208 insertions(+), 267 deletions(-) delete mode 100644 astrid/src/com/todoroo/astrid/service/abtesting/ABOptions.java create mode 100644 astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java diff --git a/astrid/src/com/todoroo/astrid/activity/TaskListActivity.java b/astrid/src/com/todoroo/astrid/activity/TaskListActivity.java index e80382ccd..10a17361c 100644 --- a/astrid/src/com/todoroo/astrid/activity/TaskListActivity.java +++ b/astrid/src/com/todoroo/astrid/activity/TaskListActivity.java @@ -29,7 +29,6 @@ import com.timsu.astrid.R; import com.todoroo.andlib.sql.Functions; import com.todoroo.andlib.sql.QueryTemplate; import com.todoroo.andlib.utility.AndroidUtilities; -import com.todoroo.andlib.utility.DateUtilities; import com.todoroo.andlib.utility.DialogUtilities; import com.todoroo.andlib.utility.Preferences; import com.todoroo.astrid.actfm.TagSettingsActivity; @@ -52,7 +51,6 @@ import com.todoroo.astrid.ui.FragmentPopover; import com.todoroo.astrid.ui.MainMenuPopover; import com.todoroo.astrid.ui.MainMenuPopover.MainMenuListener; import com.todoroo.astrid.ui.TaskListFragmentPager; -import com.todoroo.astrid.utility.AstridPreferences; import com.todoroo.astrid.utility.Constants; import com.todoroo.astrid.utility.Flags; @@ -200,37 +198,12 @@ public class TaskListActivity extends AstridActivity implements MainMenuListener if (getIntent().hasExtra(TOKEN_SOURCE)) { trackActivitySource(); } - - trackUserRetention(); } private boolean swipeIsEnabled() { return fragmentLayout == LAYOUT_SINGLE && swipeEnabled; } - private void trackUserRetention() { - long firstLaunchTime = Preferences.getLong(AstridPreferences.P_FIRST_LAUNCH, 0); - long now = DateUtilities.now(); - long timeSinceFirst = now - firstLaunchTime; - - if (timeSinceFirst > DateUtilities.ONE_DAY * 3 && !Preferences.getBoolean(StatisticsConstants.APP_OPEN_THREE_DAYS, false)) { - StatisticsService.reportEvent(StatisticsConstants.APP_OPEN_THREE_DAYS); - Preferences.setBoolean(StatisticsConstants.APP_OPEN_THREE_DAYS, true); - } - if (timeSinceFirst > DateUtilities.ONE_WEEK && !Preferences.getBoolean(StatisticsConstants.APP_OPEN_ONE_WEEK, false)) { - StatisticsService.reportEvent(StatisticsConstants.APP_OPEN_ONE_WEEK); - Preferences.setBoolean(StatisticsConstants.APP_OPEN_ONE_WEEK, true); - } - if (timeSinceFirst > 2 * DateUtilities.ONE_WEEK && !Preferences.getBoolean(StatisticsConstants.APP_OPEN_TWO_WEEKS, false)) { - StatisticsService.reportEvent(StatisticsConstants.APP_OPEN_TWO_WEEKS); - Preferences.setBoolean(StatisticsConstants.APP_OPEN_TWO_WEEKS, true); - } - if (timeSinceFirst > 3 * DateUtilities.ONE_WEEK && !Preferences.getBoolean(StatisticsConstants.APP_OPEN_THREE_WEEKS, false)) { - StatisticsService.reportEvent(StatisticsConstants.APP_OPEN_THREE_WEEKS); - Preferences.setBoolean(StatisticsConstants.APP_OPEN_THREE_WEEKS, true); - } - } - @Override public TaskListFragment getTaskListFragment() { if (swipeIsEnabled()) { diff --git a/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java b/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java index 403823427..d39cb1a61 100644 --- a/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java +++ b/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java @@ -25,7 +25,7 @@ import com.todoroo.astrid.gtasks.GtasksPreferenceService; import com.todoroo.astrid.gtasks.GtasksTaskListUpdater; import com.todoroo.astrid.gtasks.sync.GtasksSyncService; import com.todoroo.astrid.service.abtesting.ABChooser; -import com.todoroo.astrid.service.abtesting.ABOptions; +import com.todoroo.astrid.service.abtesting.ABTests; import com.todoroo.astrid.service.abtesting.FeatureFlipper; import com.todoroo.astrid.tags.TagService; import com.todoroo.astrid.utility.Constants; @@ -99,7 +99,7 @@ public class AstridDependencyInjector extends AbstractDependencyInjector { // AB testing injectables.put("abChooser", ABChooser.class); - injectables.put("abOptions", new ABOptions()); + injectables.put("abTests", new ABTests()); injectables.put("featureFlipper", FeatureFlipper.class); // com.todoroo.astrid.tags diff --git a/astrid/src/com/todoroo/astrid/service/StartupService.java b/astrid/src/com/todoroo/astrid/service/StartupService.java index 78c8ada1a..ab5be0db9 100644 --- a/astrid/src/com/todoroo/astrid/service/StartupService.java +++ b/astrid/src/com/todoroo/astrid/service/StartupService.java @@ -42,7 +42,6 @@ import com.todoroo.astrid.opencrx.OpencrxCoreUtils; import com.todoroo.astrid.producteev.ProducteevUtilities; import com.todoroo.astrid.reminders.ReminderStartupReceiver; import com.todoroo.astrid.service.abtesting.ABChooser; -import com.todoroo.astrid.service.abtesting.ABOptions; import com.todoroo.astrid.service.abtesting.FeatureFlipper; import com.todoroo.astrid.utility.AstridPreferences; import com.todoroo.astrid.utility.Constants; @@ -213,10 +212,11 @@ public class StartupService { } }).start(); - abChooser.getChoiceForOption(ABOptions.AB_OPTION_SWIPE_ENABLED_KEY); - abChooser.getChoiceForOption(ABOptions.AB_OPTION_CONTACTS_PICKER_ENABLED); + abChooser.makeChoicesForAllTests(); AstridPreferences.setPreferenceDefaults(); + trackABTestingData(); + // check for task killers if(!Constants.OEM) showTaskKillerHelp(context); @@ -224,6 +224,28 @@ public class StartupService { hasStartedUp = true; } + private void trackABTestingData() { + long firstLaunchTime = Preferences.getLong(AstridPreferences.P_FIRST_LAUNCH, 0); + long now = DateUtilities.now(); + long timeSinceFirst = now - firstLaunchTime; + + if (firstLaunchTime == 0) { + // Event days +0 + } + if (timeSinceFirst > DateUtilities.ONE_DAY * 3 /*&& !some condition*/) { + // Event days +3 + } + if (timeSinceFirst > DateUtilities.ONE_WEEK /*&& !some condition*/) { + // Event days +7 + } + if (timeSinceFirst > 2 * DateUtilities.ONE_WEEK /*&& !some condition*/) { + // Event days +14 + } + if (timeSinceFirst > 3 * DateUtilities.ONE_WEEK /*&& !some condition*/) { + // Event days +21 + } + } + /** * @param context * @param e error that was raised diff --git a/astrid/src/com/todoroo/astrid/service/StatisticsConstants.java b/astrid/src/com/todoroo/astrid/service/StatisticsConstants.java index e442fcc98..e8422c244 100644 --- a/astrid/src/com/todoroo/astrid/service/StatisticsConstants.java +++ b/astrid/src/com/todoroo/astrid/service/StatisticsConstants.java @@ -70,11 +70,4 @@ public class StatisticsConstants { public static final String TASK_ONE_WEEK = "task-created-one-week"; public static final String TASK_TWO_WEEKS = "task-created-two-weeks"; public static final String TASK_THREE_WEEKS = "task-created-three-weeks"; - - public static final String APP_OPEN_THREE_DAYS = "app-open-three-days"; - public static final String APP_OPEN_ONE_WEEK = "app-open-one-week"; - public static final String APP_OPEN_TWO_WEEKS = "app-open-two-weeks"; - public static final String APP_OPEN_THREE_WEEKS = "app-open-three-weeks"; - - } diff --git a/astrid/src/com/todoroo/astrid/service/StatisticsService.java b/astrid/src/com/todoroo/astrid/service/StatisticsService.java index d5db178dd..de519fea6 100644 --- a/astrid/src/com/todoroo/astrid/service/StatisticsService.java +++ b/astrid/src/com/todoroo/astrid/service/StatisticsService.java @@ -11,21 +11,12 @@ import android.content.Context; import com.localytics.android.LocalyticsSession; import com.timsu.astrid.R; -import com.todoroo.andlib.service.Autowired; -import com.todoroo.andlib.service.DependencyInjectionService; import com.todoroo.andlib.utility.Preferences; -import com.todoroo.astrid.service.abtesting.ABOptions; import com.todoroo.astrid.utility.Constants; public class StatisticsService { private static LocalyticsSession localyticsSession; - private static class StatisticsDependencies { - @Autowired ABOptions abOptions; - public StatisticsDependencies() { - DependencyInjectionService.getInstance().inject(this); - } - } /** * Indicate session started @@ -93,15 +84,12 @@ public class StatisticsService { return; if(localyticsSession != null) { - String[] abAttributes = new StatisticsDependencies().abOptions.getLocalyticsAttributeArrayForEvent(event); - if(attributes.length > 0 || abAttributes.length > 0) { + if(attributes.length > 0) { HashMap attrMap = new HashMap(); for(int i = 1; i < attributes.length; i += 2) { if(attributes[i] != null) attrMap.put(attributes[i-1], attributes[i]); } - for (int i = 1; i < abAttributes.length; i += 2) - attrMap.put(abAttributes[i-1], abAttributes[i]); localyticsSession.tagEvent(event, attrMap); } else localyticsSession.tagEvent(event); diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java index 9cdf6edd0..f5e535acb 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java @@ -1,6 +1,7 @@ package com.todoroo.astrid.service.abtesting; import java.util.Random; +import java.util.Set; import com.todoroo.andlib.service.Autowired; import com.todoroo.andlib.service.DependencyInjectionService; @@ -18,7 +19,7 @@ public class ABChooser { public static final int NO_OPTION = -1; @Autowired - private ABOptions abOptions; + private ABTests abTests; private final Random random; @@ -27,6 +28,17 @@ public class ABChooser { random = new Random(); } + /** + * Iterates through the list of all available tests and makes sure that a choice + * is made for each of them + */ + public void makeChoicesForAllTests() { + Set tests = abTests.getAllTestKeys(); + for (String test : tests) { + getChoiceForTest(test); + } + } + /** * Retrieves the choice for the specified feature if already made, * or chooses one randomly from the distribution of feature probabilities @@ -34,42 +46,42 @@ public class ABChooser { * * The statistics session needs to be open here in order to collect statistics * - * @param optionKey - the preference key string of the option (defined in ABOptions) + * @param testKey - the preference key string of the option (defined in ABTests) * @return */ - public int getChoiceForOption(String optionKey) { - int pref = readChoiceForOption(optionKey); + private int getChoiceForTest(String testKey) { + int pref = readChoiceForTest(testKey); if (pref > NO_OPTION) return pref; int chosen = NO_OPTION; - if (abOptions.isValidKey(optionKey)) { - int[] optionProbs = abOptions.getProbsForKey(optionKey); + if (abTests.isValidTestKey(testKey)) { + int[] optionProbs = abTests.getProbsForTestKey(testKey); chosen = chooseOption(optionProbs); - setChoiceForOption(optionKey, chosen); + setChoiceForTest(testKey, chosen); - StatisticsService.reportEvent(abOptions.getDescriptionForOption(optionKey, chosen)); // Session should be open + StatisticsService.reportEvent(abTests.getDescriptionForTestOption(testKey, chosen)); // Session should be open } return chosen; } /** * Returns the chosen option if set or NO_OPTION if unset - * @param optionKey + * @param testKey * @return */ - public static int readChoiceForOption(String optionKey) { - return Preferences.getInt(optionKey, NO_OPTION); + public static int readChoiceForTest(String testKey) { + return Preferences.getInt(testKey, NO_OPTION); } /** * Changes the choice of an A/B feature in the preferences. Useful for * the feature flipper (can manually override previous choice) - * @param optionKey + * @param testKey * @param choiceIndex */ - public void setChoiceForOption(String optionKey, int choiceIndex) { - if (abOptions.isValidKey(optionKey)) - Preferences.setInt(optionKey, choiceIndex); + public void setChoiceForTest(String testKey, int choiceIndex) { + if (abTests.isValidTestKey(testKey)) + Preferences.setInt(testKey, choiceIndex); } /* diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABOptions.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABOptions.java deleted file mode 100644 index 622e6dff9..000000000 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABOptions.java +++ /dev/null @@ -1,194 +0,0 @@ -package com.todoroo.astrid.service.abtesting; - -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; - -import com.todoroo.astrid.service.StatisticsConstants; - -/** - * Helper class to define options with their probabilities and descriptions - * @author Sam Bosley - * - */ -public class ABOptions { - - public ABOptions() { - bundles = new HashMap(); - events = new HashMap>(); - initialize(); - } - - /** - * Gets the integer array of weighted probabilities for an option key - * @param key - * @return - */ - public synchronized int[] getProbsForKey(String key) { - if (bundles.containsKey(key)) { - ABOptionBundle bundle = bundles.get(key); - return bundle.weightedProbs; - } else { - return null; - } - } - - /** - * Updates the weighted probability array for a given key. Returns true - * on success, false if they key doesn't exist or if the array is the wrong - * length. - * @param key - * @param newProbs - * @return - */ - public synchronized boolean setProbsForKey(String key, int[] newProbs) { - if (bundles.containsKey(newProbs)) { - ABOptionBundle bundle = bundles.get(key); - if (bundle.descriptions == null || newProbs.length == bundle.descriptions.length) { - bundle.weightedProbs = newProbs; - return true; - } - } - return false; - } - - /** - * Gets the string array of option descriptions for an option key - * @param key - * @return - */ - public String[] getDescriptionsForKey(String key) { - if (bundles.containsKey(key)) { - ABOptionBundle bundle = bundles.get(key); - return bundle.descriptions; - } else { - return null; - } - } - - /** - * Returns the description for a particular choice of the given option - * @param key - * @param optionIndex - * @return - */ - public String getDescriptionForOption(String key, int optionIndex) { - if (bundles.containsKey(key)) { - ABOptionBundle bundle = bundles.get(key); - if (bundle.descriptions != null && optionIndex < bundle.descriptions.length) { - return bundle.descriptions[optionIndex]; - } - } - return null; - } - - /** - * Maps keys (i.e. preference key identifiers) to feature weights and descriptions - */ - private final HashMap bundles; - private final HashMap> events; // maps events to lists of interested keys - - private static class ABOptionBundle { - public int[] weightedProbs; - public String[] descriptions; - - public ABOptionBundle(int[] weightedProbs, String[] descriptions) { - this.weightedProbs = weightedProbs; - this.descriptions = descriptions; - } - } - - public boolean isValidKey(String key) { - return bundles.containsKey(key); - } - - /** - * Gets a localytics attribute array for the specified event. - * @param event - * @return - */ - public String[] getLocalyticsAttributeArrayForEvent(String event) { - ArrayList attributes = new ArrayList(); - List interestedKeys = events.get(event); - if (interestedKeys != null) - for (String key : interestedKeys) { - // Get choice if exists and add to array - if (isValidKey(key)) { - ABOptionBundle bundle = bundles.get(key); - int choice = ABChooser.readChoiceForOption(key); - if (choice != ABChooser.NO_OPTION && - bundle.descriptions != null && choice < bundle.descriptions.length) { - attributes.add(key); - attributes.add(getDescriptionForOption(key, choice)); - } - } - } - return attributes.toArray(new String[attributes.size()]); - } - - - /** - * A/B testing options are defined below according to the following spec: - * - * @param optionKey = "" - * --This key is used to identify the option in the application and in the preferences - * - * @param probs = { int, int, ... } - * --The different choices in an option correspond to an index in the probability array. - * Probabilities are expressed as integers to easily define relative weights. For example, - * the array { 1, 2 } would mean option 0 would happen one time for every two occurrences of option 1 - * - * (optional) - * @param descriptions = { "...", "...", ... } - * --A string description of each option. Useful for tagging events. The index of - * each description should correspond to the events location in the probability array - * (i.e. the arrays should be the same length if this one exists) - * - * (optional) - * @param relevantEvents = { "...", "...", ... } - * --An arbitrary length list of relevant localytics events. When events are - * tagged from StatisticsService, they will be appended with attributes - * that have that event in this array - */ - public void addOption(String optionKey, int[] probs, String[] descriptions, String[] relevantEvents) { - ABOptionBundle bundle = new ABOptionBundle(probs, descriptions); - bundles.put(optionKey, bundle); - - if (relevantEvents != null) { - for (String event : relevantEvents) { - List interestedKeys = events.get(event); - if (interestedKeys == null) { - interestedKeys = new ArrayList(); - events.put(event, interestedKeys); - } - interestedKeys.add(optionKey); - } - } - } - - private void initialize() { // Set up - //Calls to addOption go here - addOption(AB_OPTION_SWIPE_ENABLED_KEY, AB_OPTION_SWIPE_ENABLED_PROBS, AB_OPTION_SWIPE_ENABLED_DESC, AB_OPTION_SWIPE_ENABLED_EVENTS); - addOption(AB_OPTION_CONTACTS_PICKER_ENABLED, AB_OPTION_CONTACTS_ENABLED_PROBS, AB_OPTION_CONTACTS_ENABLED_DESC, AB_OPTION_CONTACTS_ENABLED_EVENTS); - } - - public static final String AB_OPTION_SWIPE_ENABLED_KEY = "swipeEnabled"; //$NON-NLS-1$ - private static final int[] AB_OPTION_SWIPE_ENABLED_PROBS = { 1, 1 }; - private static final String[] AB_OPTION_SWIPE_ENABLED_DESC = { "swipe-lists-disabled", "swipe-lists-enabled" }; //$NON-NLS-1$//$NON-NLS-2$ - private static final String[] AB_OPTION_SWIPE_ENABLED_EVENTS = { StatisticsConstants.APP_OPEN_THREE_DAYS, - StatisticsConstants.APP_OPEN_ONE_WEEK, - StatisticsConstants.APP_OPEN_TWO_WEEKS, - StatisticsConstants.APP_OPEN_THREE_WEEKS }; - - public static final String AB_OPTION_CONTACTS_PICKER_ENABLED = "contactsEnabled"; //$NON-NLS-1$ - private static final int[] AB_OPTION_CONTACTS_ENABLED_PROBS = { 1, 1 }; - private static final String[] AB_OPTION_CONTACTS_ENABLED_DESC = { "contacts-disabled", "contacts-enabled" }; //$NON-NLS-1$//$NON-NLS-2$ - private static final String[] AB_OPTION_CONTACTS_ENABLED_EVENTS = { StatisticsConstants.APP_OPEN_THREE_DAYS, - StatisticsConstants.APP_OPEN_ONE_WEEK, - StatisticsConstants.APP_OPEN_TWO_WEEKS, - StatisticsConstants.APP_OPEN_THREE_WEEKS, - StatisticsConstants.TASK_ASSIGNED_EMAIL, - StatisticsConstants.TASK_ASSIGNED_PICKER }; - - -} diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java new file mode 100644 index 000000000..292fb0835 --- /dev/null +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java @@ -0,0 +1,147 @@ +package com.todoroo.astrid.service.abtesting; + +import java.util.HashMap; +import java.util.Set; + +/** + * Helper class to define options with their probabilities and descriptions + * @author Sam Bosley + * + */ +public class ABTests { + + public ABTests() { + bundles = new HashMap(); + initialize(); + } + + /** + * Gets the integer array of weighted probabilities for an option key + * @param key + * @return + */ + public synchronized int[] getProbsForTestKey(String key) { + if (bundles.containsKey(key)) { + ABTestBundle bundle = bundles.get(key); + return bundle.weightedProbs; + } else { + return null; + } + } + + /** + * Updates the weighted probability array for a given key. Returns true + * on success, false if they key doesn't exist or if the array is the wrong + * length. + * @param key + * @param newProbs + * @return + */ + public synchronized boolean setProbsForTestKey(String key, int[] newProbs) { + if (bundles.containsKey(newProbs)) { + ABTestBundle bundle = bundles.get(key); + if (bundle.descriptions == null || newProbs.length == bundle.descriptions.length) { + bundle.weightedProbs = newProbs; + return true; + } + } + return false; + } + + /** + * Gets the string array of option descriptions for an option key + * @param key + * @return + */ + public String[] getDescriptionsForTestKey(String key) { + if (bundles.containsKey(key)) { + ABTestBundle bundle = bundles.get(key); + return bundle.descriptions; + } else { + return null; + } + } + + /** + * Returns the description for a particular choice of the given option + * @param testKey + * @param optionIndex + * @return + */ + public String getDescriptionForTestOption(String testKey, int optionIndex) { + if (bundles.containsKey(testKey)) { + ABTestBundle bundle = bundles.get(testKey); + if (bundle.descriptions != null && optionIndex < bundle.descriptions.length) { + return bundle.descriptions[optionIndex]; + } + } + return null; + } + + public Set getAllTestKeys() { + return bundles.keySet(); + } + + /** + * Maps keys (i.e. preference key identifiers) to feature weights and descriptions + */ + private final HashMap bundles; + + private static class ABTestBundle { + public int[] weightedProbs; + public String[] descriptions; + + public ABTestBundle(int[] weightedProbs, String[] descriptions) { + this.weightedProbs = weightedProbs; + this.descriptions = descriptions; + } + } + + public boolean isValidTestKey(String key) { + return bundles.containsKey(key); + } + + /** + * A/B testing options are defined below according to the following spec: + * + * @param testKey = "" + * --This key is used to identify the option in the application and in the preferences + * + * @param probs = { int, int, ... } + * --The different choices in an option correspond to an index in the probability array. + * Probabilities are expressed as integers to easily define relative weights. For example, + * the array { 1, 2 } would mean option 0 would happen one time for every two occurrences of option 1 + * + * (optional) + * @param descriptions = { "...", "...", ... } + * --A string description of each option. Useful for tagging events. The index of + * each description should correspond to the events location in the probability array + * (i.e. the arrays should be the same length if this one exists) + * + * (optional) + * @param relevantEvents = { "...", "...", ... } + * --An arbitrary length list of relevant localytics events. When events are + * tagged from StatisticsService, they will be appended with attributes + * that have that event in this array + */ + public void addTest(String testKey, int[] probs, String[] descriptions) { + ABTestBundle bundle = new ABTestBundle(probs, descriptions); + bundles.put(testKey, bundle); + } + + private void initialize() { // Set up + //Calls to addTest go here + addTest(AB_TEST_SWIPE_ENABLED_KEY, AB_TEST_SWIPE_ENABLED_PROBS, AB_TEST_SWIPE_ENABLED_DESC); + addTest(AB_TEST_CONTACTS_PICKER_ENABLED, AB_TEST_CONTACTS_ENABLED_PROBS, AB_TEST_CONTACTS_ENABLED_DESC); + } + + public static final String AB_TEST_SWIPE_ENABLED_KEY = "swipeEnabled"; //$NON-NLS-1$ + private static final int[] AB_TEST_SWIPE_ENABLED_PROBS = { 1, 1 }; + private static final String[] AB_TEST_SWIPE_ENABLED_DESC = { "swipe-lists-disabled", "swipe-lists-enabled" }; //$NON-NLS-1$//$NON-NLS-2$ + + public static final String AB_TEST_CONTACTS_PICKER_ENABLED = "contactsEnabled"; //$NON-NLS-1$ + private static final int[] AB_TEST_CONTACTS_ENABLED_PROBS = { 1, 1 }; + private static final String[] AB_TEST_CONTACTS_ENABLED_DESC = { "contacts-disabled", "contacts-enabled" }; //$NON-NLS-1$//$NON-NLS-2$ + + +} diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/FeatureFlipper.java b/astrid/src/com/todoroo/astrid/service/abtesting/FeatureFlipper.java index d95457d78..21136dc25 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/FeatureFlipper.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/FeatureFlipper.java @@ -29,7 +29,7 @@ public class FeatureFlipper { @Autowired private RestClient restClient; @Autowired private ABChooser abChooser; - @Autowired private ABOptions abOptions; + @Autowired private ABTests abTests; public FeatureFlipper() { DependencyInjectionService.getInstance().inject(this); @@ -52,7 +52,7 @@ public class FeatureFlipper { if (settings.has(KEY_SET_OPTION)) { int option = settings.getInt(KEY_SET_OPTION); - abChooser.setChoiceForOption(key, option); + abChooser.setChoiceForTest(key, option); } else if (settings.has(KEY_PROBABILITIES)) { JSONArray newProbabilities = settings.getJSONArray(KEY_PROBABILITIES); int[] probs = new int[newProbabilities.length()]; @@ -61,7 +61,7 @@ public class FeatureFlipper { probs[j] = newProbabilities.getInt(j); } - abOptions.setProbsForKey(key, probs); + abTests.setProbsForTestKey(key, probs); } } catch (Exception e) { e.printStackTrace(); diff --git a/astrid/src/com/todoroo/astrid/utility/AstridPreferences.java b/astrid/src/com/todoroo/astrid/utility/AstridPreferences.java index 9592ce3f8..a647ddd73 100644 --- a/astrid/src/com/todoroo/astrid/utility/AstridPreferences.java +++ b/astrid/src/com/todoroo/astrid/utility/AstridPreferences.java @@ -12,7 +12,7 @@ import com.todoroo.astrid.api.AstridApiConstants; import com.todoroo.astrid.data.Task; import com.todoroo.astrid.service.ThemeService; import com.todoroo.astrid.service.abtesting.ABChooser; -import com.todoroo.astrid.service.abtesting.ABOptions; +import com.todoroo.astrid.service.abtesting.ABTests; public class AstridPreferences { @@ -49,10 +49,10 @@ public class AstridPreferences { Preferences.setIfUnset(prefs, editor, r, R.string.p_fontSize, 18); Preferences.setIfUnset(prefs, editor, r, R.string.p_showNotes, false); - boolean swipeEnabled = (ABChooser.readChoiceForOption(ABOptions.AB_OPTION_SWIPE_ENABLED_KEY) == 1); + boolean swipeEnabled = (ABChooser.readChoiceForTest(ABTests.AB_TEST_SWIPE_ENABLED_KEY) == 1); Preferences.setIfUnset(prefs, editor, r, R.string.p_swipe_lists_performance_key, swipeEnabled ? 3 : 0); - boolean contactsPickerEnabled = (ABChooser.readChoiceForOption(ABOptions.AB_OPTION_CONTACTS_PICKER_ENABLED) == 1); + boolean contactsPickerEnabled = (ABChooser.readChoiceForTest(ABTests.AB_TEST_CONTACTS_PICKER_ENABLED) == 1); Preferences.setIfUnset(prefs, editor, r, R.string.p_use_contact_picker, contactsPickerEnabled); if ("white-blue".equals(Preferences.getStringValue(R.string.p_theme))) { //$NON-NLS-1$ migrate from when white-blue wasn't the default From 1f46fa34c64524868f4f0eefaafd75e8836c549e Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Wed, 25 Apr 2012 18:10:11 -0700 Subject: [PATCH 02/19] Added the ABTestEvent model for tracking events in the database --- .../src/com/todoroo/astrid/dao/Database.java | 12 +- .../com/todoroo/astrid/data/ABTestEvent.java | 109 ++++++++++++++++++ 2 files changed, 119 insertions(+), 2 deletions(-) create mode 100644 astrid/src/com/todoroo/astrid/data/ABTestEvent.java diff --git a/astrid/src/com/todoroo/astrid/dao/Database.java b/astrid/src/com/todoroo/astrid/dao/Database.java index 71915080f..f8d4e2836 100644 --- a/astrid/src/com/todoroo/astrid/dao/Database.java +++ b/astrid/src/com/todoroo/astrid/dao/Database.java @@ -13,6 +13,7 @@ import com.todoroo.andlib.data.AbstractModel; import com.todoroo.andlib.data.Property; import com.todoroo.andlib.data.Table; import com.todoroo.andlib.service.ContextManager; +import com.todoroo.astrid.data.ABTestEvent; import com.todoroo.astrid.data.Metadata; import com.todoroo.astrid.data.StoreObject; import com.todoroo.astrid.data.TagData; @@ -38,7 +39,7 @@ public class Database extends AbstractDatabase { * Database version number. This variable must be updated when database * tables are updated, as it determines whether a database needs updating. */ - public static final int VERSION = 23; + public static final int VERSION = 24; /** * Database name (must be unique) @@ -55,7 +56,8 @@ public class Database extends AbstractDatabase { StoreObject.TABLE, TagData.TABLE, Update.TABLE, - User.TABLE + User.TABLE, + ABTestEvent.TABLE, }; // --- listeners @@ -310,6 +312,12 @@ public class Database extends AbstractDatabase { Log.e("astrid", "db-upgrade-" + oldVersion + "-" + newVersion, e); } + case 23: try { + database.execSQL(createTableSql(visitor, ABTestEvent.TABLE.name, ABTestEvent.PROPERTIES)); + } catch (SQLiteException e) { + Log.e("astrid", "db-upgrade-" + oldVersion + "-" + newVersion, e); + } + return true; } diff --git a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java new file mode 100644 index 000000000..3a4c69c50 --- /dev/null +++ b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java @@ -0,0 +1,109 @@ +package com.todoroo.astrid.data; + +import android.content.ContentValues; +import android.net.Uri; + +import com.todoroo.andlib.data.AbstractModel; +import com.todoroo.andlib.data.Property; +import com.todoroo.andlib.data.Property.IntegerProperty; +import com.todoroo.andlib.data.Property.LongProperty; +import com.todoroo.andlib.data.Property.StringProperty; +import com.todoroo.andlib.data.Table; +import com.todoroo.andlib.utility.DateUtilities; +import com.todoroo.astrid.api.AstridApiConstants; + +@SuppressWarnings("nls") +public class ABTestEvent extends AbstractModel { + + public static final long TEST_INTERVAL_0 = 0; + public static final long TEST_INTERVAL_3 = 3 * DateUtilities.ONE_DAY; + public static final long TEST_INTERVAL_7 = DateUtilities.ONE_WEEK; + public static final long TEST_INTERVAL_14 = 2 * DateUtilities.ONE_WEEK; + public static final long TEST_INTERVAL_21 = 3 * DateUtilities.ONE_WEEK; + + + // --- table and uri + + /** table for this model */ + public static final Table TABLE = new Table("abtestevent", ABTestEvent.class); + + /** content uri for this model */ + public static final Uri CONTENT_URI = Uri.parse("content://" + AstridApiConstants.PACKAGE + "/" + + TABLE.name); + + + // --- properties + + /** ID */ + public static final LongProperty ID = new LongProperty( + TABLE, ID_PROPERTY_NAME); + + /** Name of the test -- one of the constant test keys defined in ABOptions */ + public static final StringProperty TEST_NAME = new StringProperty( + TABLE, "testName"); + + /** + * Which variant (option) was chosen for this user -- + * one of the constants in the corresponding descriptions array in ABOptions + */ + public static final StringProperty TEST_VARIANT = new StringProperty( + TABLE, "testVariant"); + + /** + * Indicates if the user should be considered a new user for the purposes + * of this test. + * Should be 0 if no, 1 if yes + */ + public static final IntegerProperty NEW_USER = new IntegerProperty( + TABLE, "newUser"); // 0 if no, 1 if yes + + /** + * Indicates if the user was "activated" at the time of recording this data + * point. + * Should be 0 if no, 1 if yes + * Activated: 3 tasks created, one completed + */ + public static final IntegerProperty ACTIVATED_USER = new IntegerProperty( + TABLE, "activatedUser"); + + /** + * Which time interval event this data point corresponds to. + * Should be one of the time interval constants defined above. + */ + public static final LongProperty TIME_INTERVAL = new LongProperty( + TABLE, "timeInterval"); // one of the constants defined above + + /** The actual date on which this data point was recorded. */ + public static final LongProperty DATE_RECORDED = new LongProperty( + TABLE, "dateRecorded"); + + /** List of all properties for this model */ + public static final Property[] PROPERTIES = generateProperties(Task.class); + + private static final ContentValues defaultValues = new ContentValues(); + + + static { + // initialize with default values + } + + @Override + public ContentValues getDefaultValues() { + return defaultValues; + } + + @Override + public long getId() { + return getIdHelper(ID); + } + + // --- parcelable helpers + + public static final Creator CREATOR = new ModelCreator(ABTestEvent.class); + + @Override + protected Creator getCreator() { + return CREATOR; + } + +} From f08d3c2890e32eabee20e30e26c294b204b70472 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Wed, 25 Apr 2012 18:15:49 -0700 Subject: [PATCH 03/19] Added ABTestEventDao --- .../todoroo/astrid/dao/ABTestEventDao.java | 19 +++++++++++++++++++ .../service/AstridDependencyInjector.java | 2 ++ 2 files changed, 21 insertions(+) create mode 100644 astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java diff --git a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java new file mode 100644 index 000000000..5ccc20d80 --- /dev/null +++ b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java @@ -0,0 +1,19 @@ +package com.todoroo.astrid.dao; + +import com.todoroo.andlib.data.DatabaseDao; +import com.todoroo.andlib.service.Autowired; +import com.todoroo.andlib.service.DependencyInjectionService; +import com.todoroo.astrid.data.ABTestEvent; + +public class ABTestEventDao extends DatabaseDao { + + @Autowired + private Database database; + + public ABTestEventDao() { + super(ABTestEvent.class); + DependencyInjectionService.getInstance().inject(this); + setDatabase(database); + } + +} diff --git a/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java b/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java index d39cb1a61..29ff9241a 100644 --- a/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java +++ b/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java @@ -12,6 +12,7 @@ import com.todoroo.astrid.actfm.sync.ActFmDataService; import com.todoroo.astrid.actfm.sync.ActFmInvoker; import com.todoroo.astrid.actfm.sync.ActFmPreferenceService; import com.todoroo.astrid.actfm.sync.ActFmSyncService; +import com.todoroo.astrid.dao.ABTestEventDao; import com.todoroo.astrid.dao.Database; import com.todoroo.astrid.dao.MetadataDao; import com.todoroo.astrid.dao.StoreObjectDao; @@ -100,6 +101,7 @@ public class AstridDependencyInjector extends AbstractDependencyInjector { // AB testing injectables.put("abChooser", ABChooser.class); injectables.put("abTests", new ABTests()); + injectables.put("abTestEventDao", ABTestEventDao.class); injectables.put("featureFlipper", FeatureFlipper.class); // com.todoroo.astrid.tags From c7e18592a61533879dcfe14bcd0d38285921e997 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 11:10:47 -0700 Subject: [PATCH 04/19] Added methods for creating data points to the abtesteventdao --- .../andlib/utility/AndroidUtilities.java | 13 +++++ .../todoroo/astrid/dao/ABTestEventDao.java | 51 +++++++++++++++++++ .../com/todoroo/astrid/data/ABTestEvent.java | 32 +++++++++--- .../astrid/service/abtesting/ABChooser.java | 20 +++++--- 4 files changed, 101 insertions(+), 15 deletions(-) diff --git a/api/src/com/todoroo/andlib/utility/AndroidUtilities.java b/api/src/com/todoroo/andlib/utility/AndroidUtilities.java index 7a35e8315..4633d459c 100644 --- a/api/src/com/todoroo/andlib/utility/AndroidUtilities.java +++ b/api/src/com/todoroo/andlib/utility/AndroidUtilities.java @@ -200,6 +200,19 @@ public class AndroidUtilities { return -1; } + /** + * Return index of value in integer array + * @param array array to search + * @param value value to look for + * @return + */ + public static int indexOf(int[] array, int value) { + for (int i = 0; i < array.length; i++) + if (array[i] == value) + return i; + return -1; + } + /** * Serializes a content value into a string */ diff --git a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java index 5ccc20d80..0ecb9bfbf 100644 --- a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java +++ b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java @@ -1,8 +1,13 @@ package com.todoroo.astrid.dao; import com.todoroo.andlib.data.DatabaseDao; +import com.todoroo.andlib.data.TodorooCursor; import com.todoroo.andlib.service.Autowired; import com.todoroo.andlib.service.DependencyInjectionService; +import com.todoroo.andlib.sql.Order; +import com.todoroo.andlib.sql.Query; +import com.todoroo.andlib.utility.AndroidUtilities; +import com.todoroo.andlib.utility.DateUtilities; import com.todoroo.astrid.data.ABTestEvent; public class ABTestEventDao extends DatabaseDao { @@ -16,4 +21,50 @@ public class ABTestEventDao extends DatabaseDao { setDatabase(database); } + public void createInitialTestEvent(String testName, String testVariant, + boolean newUser, boolean activeUser) { + ABTestEvent event = new ABTestEvent(); + event.setValue(ABTestEvent.TEST_NAME, testName); + event.setValue(ABTestEvent.TEST_VARIANT, testVariant); + event.setValue(ABTestEvent.NEW_USER, newUser ? 1 : 0); + event.setValue(ABTestEvent.ACTIVATED_USER, activeUser ? 1 : 0); + event.setValue(ABTestEvent.TIME_INTERVAL, ABTestEvent.TIME_INTERVAL_0); + event.setValue(ABTestEvent.DATE_RECORDED, DateUtilities.now()); + + createNew(event); + } + + public boolean createTestEventWithTimeInterval(String testName, int timeInterval) { + TodorooCursor existing = query(Query.select(ABTestEvent.PROPERTIES) + .where(ABTestEvent.TEST_NAME.eq(testName)).orderBy(Order.asc(ABTestEvent.TIME_INTERVAL))); + + try { + if (existing.getCount() == 0) + return false; + + existing.moveToLast(); + ABTestEvent item = new ABTestEvent(existing); + int lastRecordedTimeIntervalIndex = AndroidUtilities.indexOf( + ABTestEvent.TIME_INTERVALS, item.getValue(ABTestEvent.TIME_INTERVAL)); + + int currentTimeIntervalIndex = AndroidUtilities.indexOf( + ABTestEvent.TIME_INTERVALS, timeInterval); + + if (lastRecordedTimeIntervalIndex < 0 || currentTimeIntervalIndex < 0 + || lastRecordedTimeIntervalIndex >= currentTimeIntervalIndex) + return false; + + long now = DateUtilities.now(); + for (int i = lastRecordedTimeIntervalIndex + 1; i <= currentTimeIntervalIndex; i++) { + item.clearValue(ABTestEvent.ID); + item.setValue(ABTestEvent.TIME_INTERVAL, ABTestEvent.TIME_INTERVALS[i]); + item.setValue(ABTestEvent.DATE_RECORDED, now); + createNew(item); + } + } finally { + existing.close(); + } + return true; + } + } diff --git a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java index 3a4c69c50..476484d0b 100644 --- a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java +++ b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java @@ -9,18 +9,25 @@ import com.todoroo.andlib.data.Property.IntegerProperty; import com.todoroo.andlib.data.Property.LongProperty; import com.todoroo.andlib.data.Property.StringProperty; import com.todoroo.andlib.data.Table; -import com.todoroo.andlib.utility.DateUtilities; +import com.todoroo.andlib.data.TodorooCursor; import com.todoroo.astrid.api.AstridApiConstants; @SuppressWarnings("nls") public class ABTestEvent extends AbstractModel { - public static final long TEST_INTERVAL_0 = 0; - public static final long TEST_INTERVAL_3 = 3 * DateUtilities.ONE_DAY; - public static final long TEST_INTERVAL_7 = DateUtilities.ONE_WEEK; - public static final long TEST_INTERVAL_14 = 2 * DateUtilities.ONE_WEEK; - public static final long TEST_INTERVAL_21 = 3 * DateUtilities.ONE_WEEK; + public static final int TIME_INTERVAL_0 = 0; + public static final int TIME_INTERVAL_3 = 3; + public static final int TIME_INTERVAL_7 = 7; + public static final int TIME_INTERVAL_14 = 14; + public static final int TIME_INTERVAL_21 = 21; + public static final int[] TIME_INTERVALS = { + TIME_INTERVAL_0, + TIME_INTERVAL_3, + TIME_INTERVAL_7, + TIME_INTERVAL_14, + TIME_INTERVAL_21 + }; // --- table and uri @@ -70,7 +77,7 @@ public class ABTestEvent extends AbstractModel { * Which time interval event this data point corresponds to. * Should be one of the time interval constants defined above. */ - public static final LongProperty TIME_INTERVAL = new LongProperty( + public static final IntegerProperty TIME_INTERVAL = new IntegerProperty( TABLE, "timeInterval"); // one of the constants defined above /** The actual date on which this data point was recorded. */ @@ -92,6 +99,17 @@ public class ABTestEvent extends AbstractModel { return defaultValues; } + // --- data access boilerplate + + public ABTestEvent() { + super(); + } + + public ABTestEvent(TodorooCursor cursor) { + this(); + readPropertiesFromCursor(cursor); + } + @Override public long getId() { return getIdHelper(ID); diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java index f5e535acb..9c647c4e9 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java @@ -6,7 +6,7 @@ import java.util.Set; import com.todoroo.andlib.service.Autowired; import com.todoroo.andlib.service.DependencyInjectionService; import com.todoroo.andlib.utility.Preferences; -import com.todoroo.astrid.service.StatisticsService; +import com.todoroo.astrid.dao.ABTestEventDao; /** * Helper class to facilitate A/B testing by randomly choosing an option @@ -21,6 +21,9 @@ public class ABChooser { @Autowired private ABTests abTests; + @Autowired + private ABTestEventDao abTestEventDao; + private final Random random; public ABChooser() { @@ -32,10 +35,10 @@ public class ABChooser { * Iterates through the list of all available tests and makes sure that a choice * is made for each of them */ - public void makeChoicesForAllTests() { + public void makeChoicesForAllTests(boolean newUser, boolean activatedUser) { Set tests = abTests.getAllTestKeys(); for (String test : tests) { - getChoiceForTest(test); + makeChoiceForTest(test, newUser, activatedUser); } } @@ -47,21 +50,22 @@ public class ABChooser { * The statistics session needs to be open here in order to collect statistics * * @param testKey - the preference key string of the option (defined in ABTests) - * @return */ - private int getChoiceForTest(String testKey) { + private void makeChoiceForTest(String testKey, boolean newUser, boolean activatedUser) { int pref = readChoiceForTest(testKey); - if (pref > NO_OPTION) return pref; + if (pref > NO_OPTION) return; int chosen = NO_OPTION; if (abTests.isValidTestKey(testKey)) { int[] optionProbs = abTests.getProbsForTestKey(testKey); + String[] optionDescriptions = abTests.getDescriptionsForTestKey(testKey); chosen = chooseOption(optionProbs); setChoiceForTest(testKey, chosen); - StatisticsService.reportEvent(abTests.getDescriptionForTestOption(testKey, chosen)); // Session should be open + String desc = optionDescriptions[chosen]; + abTestEventDao.createInitialTestEvent(testKey, desc, newUser, activatedUser); } - return chosen; + return; } /** From bf15575643bef29e2d7de2bd2cc083cd46db4ac8 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 11:17:18 -0700 Subject: [PATCH 05/19] Check new user and activated status at startup --- .../astrid/service/StartupService.java | 2 +- .../todoroo/astrid/service/TaskService.java | 29 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/astrid/src/com/todoroo/astrid/service/StartupService.java b/astrid/src/com/todoroo/astrid/service/StartupService.java index ab5be0db9..b0d48348b 100644 --- a/astrid/src/com/todoroo/astrid/service/StartupService.java +++ b/astrid/src/com/todoroo/astrid/service/StartupService.java @@ -212,7 +212,7 @@ public class StartupService { } }).start(); - abChooser.makeChoicesForAllTests(); + abChooser.makeChoicesForAllTests(latestSetVersion == 0, taskService.getUserActivationStatus()); AstridPreferences.setPreferenceDefaults(); trackABTestingData(); diff --git a/astrid/src/com/todoroo/astrid/service/TaskService.java b/astrid/src/com/todoroo/astrid/service/TaskService.java index e44a72e37..1e74be88f 100644 --- a/astrid/src/com/todoroo/astrid/service/TaskService.java +++ b/astrid/src/com/todoroo/astrid/service/TaskService.java @@ -17,6 +17,7 @@ import com.todoroo.andlib.sql.Functions; import com.todoroo.andlib.sql.Query; import com.todoroo.andlib.utility.AndroidUtilities; import com.todoroo.andlib.utility.DateUtilities; +import com.todoroo.andlib.utility.Preferences; import com.todoroo.astrid.actfm.sync.ActFmPreferenceService; import com.todoroo.astrid.api.Filter; import com.todoroo.astrid.api.PermaSql; @@ -54,6 +55,10 @@ public class TaskService { public static final String TRANS_EDIT_SAVE = "task-edit-save"; //$NON-NLS-1$ public static final String TRANS_REPEAT_COMPLETE = "repeat-complete"; //$NON-NLS-1$ + + private static final int TOTAL_TASKS_FOR_ACTIVATION = 3; + private static final int COMPLETED_TASKS_FOR_ACTIVATION = 1; + private static final String PREF_USER_ACTVATED = "user-activated"; //$NON-NLS-1$ @Autowired private TaskDao taskDao; @@ -267,6 +272,30 @@ public class TaskService { return taskDao.query(Query.select(properties).withQueryTemplate(sql)); } + public boolean getUserActivationStatus() { + if (Preferences.getBoolean(PREF_USER_ACTVATED, false)) + return true; + + TodorooCursor all = query(Query.select(Task.ID)); + try { + if (all.getCount() < TOTAL_TASKS_FOR_ACTIVATION) + return false; + + TodorooCursor completed = query(Query.select(Task.ID).where(TaskCriteria.completed())); + try { + if (completed.getCount() < COMPLETED_TASKS_FOR_ACTIVATION) + return false; + } finally { + completed.close(); + } + } finally { + all.close(); + } + + Preferences.setBoolean(PREF_USER_ACTVATED, true); + return true; + } + /** * @param query * @return how many tasks are matched by this query From 2a0200a7157f974124745e67ad3057322a507369 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 11:21:34 -0700 Subject: [PATCH 06/19] Added field to track if data point has been reported --- astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java | 1 + astrid/src/com/todoroo/astrid/data/ABTestEvent.java | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java index 0ecb9bfbf..0083ed19c 100644 --- a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java +++ b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java @@ -57,6 +57,7 @@ public class ABTestEventDao extends DatabaseDao { long now = DateUtilities.now(); for (int i = lastRecordedTimeIntervalIndex + 1; i <= currentTimeIntervalIndex; i++) { item.clearValue(ABTestEvent.ID); + item.setValue(ABTestEvent.REPORTED, 0); item.setValue(ABTestEvent.TIME_INTERVAL, ABTestEvent.TIME_INTERVALS[i]); item.setValue(ABTestEvent.DATE_RECORDED, now); createNew(item); diff --git a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java index 476484d0b..06aa942af 100644 --- a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java +++ b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java @@ -84,14 +84,20 @@ public class ABTestEvent extends AbstractModel { public static final LongProperty DATE_RECORDED = new LongProperty( TABLE, "dateRecorded"); + /** Whether or not this data point has been reported to the server */ + public static final IntegerProperty REPORTED = new IntegerProperty( + TABLE, "reported"); // 0 if not yet reported, 1 if reported successfully + /** List of all properties for this model */ public static final Property[] PROPERTIES = generateProperties(Task.class); - private static final ContentValues defaultValues = new ContentValues(); + private static final ContentValues defaultValues = new ContentValues(); + static { // initialize with default values + defaultValues.put(REPORTED.name, 0); } @Override From 7f57679469e81a1021f8f660976bc9b4bd07904f Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 14:04:53 -0700 Subject: [PATCH 07/19] Added service and invoker for reporting analytics data --- .../andlib/utility/AndroidUtilities.java | 2 + .../todoroo/astrid/dao/ABTestEventDao.java | 3 +- .../service/AstridDependencyInjector.java | 10 +- .../ABTestEventReportingService.java | 119 ++++++++++++++++++ .../service/abtesting/ABTestReporter.java | 81 ++++++++++++ 5 files changed, 209 insertions(+), 6 deletions(-) create mode 100644 astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java create mode 100644 astrid/src/com/todoroo/astrid/service/abtesting/ABTestReporter.java diff --git a/api/src/com/todoroo/andlib/utility/AndroidUtilities.java b/api/src/com/todoroo/andlib/utility/AndroidUtilities.java index 4633d459c..d939484d8 100644 --- a/api/src/com/todoroo/andlib/utility/AndroidUtilities.java +++ b/api/src/com/todoroo/andlib/utility/AndroidUtilities.java @@ -14,9 +14,11 @@ import java.lang.reflect.Method; import java.math.BigInteger; import java.net.URL; import java.net.URLConnection; +import java.net.URLEncoder; import java.security.MessageDigest; import java.util.Arrays; import java.util.Comparator; +import java.util.List; import java.util.Map; import java.util.Map.Entry; diff --git a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java index 0083ed19c..7fdd9a63b 100644 --- a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java +++ b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java @@ -50,8 +50,7 @@ public class ABTestEventDao extends DatabaseDao { int currentTimeIntervalIndex = AndroidUtilities.indexOf( ABTestEvent.TIME_INTERVALS, timeInterval); - if (lastRecordedTimeIntervalIndex < 0 || currentTimeIntervalIndex < 0 - || lastRecordedTimeIntervalIndex >= currentTimeIntervalIndex) + if (lastRecordedTimeIntervalIndex < 0 || currentTimeIntervalIndex < 0) return false; long now = DateUtilities.now(); diff --git a/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java b/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java index 29ff9241a..db5b81def 100644 --- a/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java +++ b/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java @@ -26,6 +26,7 @@ import com.todoroo.astrid.gtasks.GtasksPreferenceService; import com.todoroo.astrid.gtasks.GtasksTaskListUpdater; import com.todoroo.astrid.gtasks.sync.GtasksSyncService; import com.todoroo.astrid.service.abtesting.ABChooser; +import com.todoroo.astrid.service.abtesting.ABTestReporter; import com.todoroo.astrid.service.abtesting.ABTests; import com.todoroo.astrid.service.abtesting.FeatureFlipper; import com.todoroo.astrid.tags.TagService; @@ -63,9 +64,9 @@ public class AstridDependencyInjector extends AbstractDependencyInjector { // com.todoroo.astrid.dao injectables.put("database", Database.class); - injectables.put("taskDao", TaskDao.class); - injectables.put("metadataDao", MetadataDao.class); - injectables.put("tagDataDao", TagDataDao.class); + injectables.put("taskDao", new TaskDao()); + injectables.put("metadataDao", new MetadataDao()); + injectables.put("tagDataDao", new TagDataDao()); injectables.put("storeObjectDao", StoreObjectDao.class); injectables.put("updateDao", UpdateDao.class); injectables.put("userDao", UserDao.class); @@ -101,7 +102,8 @@ public class AstridDependencyInjector extends AbstractDependencyInjector { // AB testing injectables.put("abChooser", ABChooser.class); injectables.put("abTests", new ABTests()); - injectables.put("abTestEventDao", ABTestEventDao.class); + injectables.put("abTestEventDao", new ABTestEventDao()); + injectables.put("abTestReporter", ABTestReporter.class); injectables.put("featureFlipper", FeatureFlipper.class); // com.todoroo.astrid.tags diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java new file mode 100644 index 000000000..6e88c5f72 --- /dev/null +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java @@ -0,0 +1,119 @@ +package com.todoroo.astrid.service.abtesting; + +import java.io.IOException; + +import org.json.JSONArray; +import org.json.JSONException; +import org.json.JSONObject; + +import android.util.Log; + +import com.todoroo.andlib.data.DatabaseDao.ModelUpdateListener; +import com.todoroo.andlib.data.TodorooCursor; +import com.todoroo.andlib.service.Autowired; +import com.todoroo.andlib.service.DependencyInjectionService; +import com.todoroo.andlib.sql.Query; +import com.todoroo.astrid.dao.ABTestEventDao; +import com.todoroo.astrid.data.ABTestEvent; + +@SuppressWarnings("nls") +public final class ABTestEventReportingService { + + private static final String KEY_TEST = "test"; + private static final String KEY_VARIANT = "variant"; + private static final String KEY_NEW_USER = "new"; + private static final String KEY_ACTIVE_USER = "activated"; + private static final String KEY_DAYS = "days"; + private static final String KEY_DATE = "date"; + + @Autowired + private ABTestEventDao abTestEventDao; + + @Autowired + private ABTestReporter abTestReporter; + + public ABTestEventReportingService() { + DependencyInjectionService.getInstance().inject(this); + } + + public void initialize() { + abTestEventDao.addListener(new ModelUpdateListener() { + @Override + public void onModelUpdated(ABTestEvent model) { + if (model.getValue(ABTestEvent.REPORTED) == 1) + return; + + pushABTestEvent(model); + } + }); + } + + public void pushABTestEvent(final ABTestEvent model) { + new Thread(new Runnable() { + public void run() { + try { + JSONArray payload = new JSONArray().put(jsonFromABTestEvent(model)); + abTestReporter.post(payload); + model.setValue(ABTestEvent.REPORTED, 1); + abTestEventDao.saveExisting(model); + } catch (JSONException e) { + handleException(e); + } catch (IOException e) { + handleException(e); + } + }; + }).start(); + } + + public void pushAllUnreportedABTestEvents() { + TodorooCursor unreported = abTestEventDao.query(Query.select(ABTestEvent.PROPERTIES) + .where(ABTestEvent.REPORTED.eq(0))); + try { + if (unreported.getCount() > 0) { + JSONArray payload = jsonArrayFromABTestEvents(unreported); + abTestReporter.post(payload); + for (unreported.moveToFirst(); !unreported.isAfterLast(); unreported.moveToNext()) { + ABTestEvent model = new ABTestEvent(unreported); + model.setValue(ABTestEvent.REPORTED, 1); + abTestEventDao.saveExisting(model); + } + } + } catch (JSONException e) { + handleException(e); + } catch (IOException e) { + handleException(e); + } finally { + unreported.close(); + } + } + + private void handleException(Exception e) { + Log.e("analytics", "analytics-error", e); + + // TODO: Schedule retry + } + + private static JSONObject jsonFromABTestEvent(ABTestEvent model) throws JSONException { + JSONObject payload = new JSONObject(); + payload.put(KEY_TEST, model.getValue(ABTestEvent.TEST_NAME)); + payload.put(KEY_VARIANT, model.getValue(ABTestEvent.TEST_VARIANT)); + payload.put(KEY_NEW_USER, model.getValue(ABTestEvent.NEW_USER) > 0 ? true : false); + payload.put(KEY_ACTIVE_USER, model.getValue(ABTestEvent.ACTIVATED_USER) > 0 ? true : false); + payload.put(KEY_DAYS, model.getValue(ABTestEvent.TIME_INTERVAL)); + + long date = model.getValue(ABTestEvent.DATE_RECORDED) / 1000L; + payload.put(KEY_DATE, date); + + return payload; + } + + private static JSONArray jsonArrayFromABTestEvents(TodorooCursor events) throws JSONException { + JSONArray result = new JSONArray(); + for (events.moveToFirst(); !events.isAfterLast(); events.moveToNext()) { + ABTestEvent model = new ABTestEvent(events); + result.put(jsonFromABTestEvent(model)); + } + return result; + } + +} diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestReporter.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestReporter.java new file mode 100644 index 000000000..b4a1a8183 --- /dev/null +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestReporter.java @@ -0,0 +1,81 @@ +package com.todoroo.astrid.service.abtesting; + +import java.io.IOException; +import java.io.UnsupportedEncodingException; +import java.util.ArrayList; +import java.util.List; + +import org.apache.commons.codec.digest.DigestUtils; +import org.apache.http.HttpEntity; +import org.apache.http.NameValuePair; +import org.apache.http.client.entity.UrlEncodedFormEntity; +import org.apache.http.message.BasicNameValuePair; +import org.apache.http.protocol.HTTP; +import org.json.JSONArray; +import org.json.JSONException; +import org.json.JSONObject; + +import com.todoroo.andlib.service.Autowired; +import com.todoroo.andlib.service.DependencyInjectionService; +import com.todoroo.andlib.service.RestClient; + +@SuppressWarnings("nls") +public class ABTestReporter { + + /** NOTE: these values are development values and will not work on production */ + private static final String URL = "http://analytics.astrid.com/api/1/retention"; + private static final String API_KEY = "ryyubd"; + private static final String API_SECRET = "q9ef3i"; + + @Autowired private RestClient restClient; + + public ABTestReporter() { + DependencyInjectionService.getInstance().inject(this); + } + + public JSONObject post(JSONArray payload) throws IOException { + try { + HttpEntity postData = createPostData(payload); + if (postData == null) + throw new IOException("Unsupported URL encoding"); + String response = restClient.post(URL, postData); + JSONObject object = new JSONObject(response); + if (object.getString("status").equals("error")) { + throw new IOException("Error reporting ABTestEvent: " + + object.optString("message")); + } + return object; + } catch (JSONException e) { + throw new IOException(e.getMessage()); + } + + } + + private HttpEntity createPostData(JSONArray payload) { + List params = new ArrayList(); + params.add(new BasicNameValuePair("apikey", API_KEY)); + params.add(new BasicNameValuePair("payload", payload.toString())); + + StringBuilder sigBuilder = new StringBuilder(); + for(NameValuePair entry : params) { + if(entry.getValue() == null) + continue; + + String key = entry.getName(); + String value = entry.getValue(); + + sigBuilder.append(key).append(value); + } + + sigBuilder.append(API_SECRET); + String signature = DigestUtils.md5Hex(sigBuilder.toString()); + params.add(new BasicNameValuePair("sig", signature)); + + try { + return new UrlEncodedFormEntity(params, HTTP.UTF_8); + } catch (UnsupportedEncodingException e) { + return null; + } + } + +} From 1b2d5c25850d82deb755962233fb9a3216b169eb Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 15:06:59 -0700 Subject: [PATCH 08/19] Push unreported test data on startup --- .../service/AstridDependencyInjector.java | 6 ++- .../astrid/service/StartupService.java | 11 ++++- .../ABTestEventReportingService.java | 43 ++++++++++--------- ...ABTestReporter.java => ABTestInvoker.java} | 4 +- 4 files changed, 39 insertions(+), 25 deletions(-) rename astrid/src/com/todoroo/astrid/service/abtesting/{ABTestReporter.java => ABTestInvoker.java} (97%) diff --git a/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java b/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java index db5b81def..5df150afd 100644 --- a/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java +++ b/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java @@ -26,7 +26,8 @@ import com.todoroo.astrid.gtasks.GtasksPreferenceService; import com.todoroo.astrid.gtasks.GtasksTaskListUpdater; import com.todoroo.astrid.gtasks.sync.GtasksSyncService; import com.todoroo.astrid.service.abtesting.ABChooser; -import com.todoroo.astrid.service.abtesting.ABTestReporter; +import com.todoroo.astrid.service.abtesting.ABTestEventReportingService; +import com.todoroo.astrid.service.abtesting.ABTestInvoker; import com.todoroo.astrid.service.abtesting.ABTests; import com.todoroo.astrid.service.abtesting.FeatureFlipper; import com.todoroo.astrid.tags.TagService; @@ -103,7 +104,8 @@ public class AstridDependencyInjector extends AbstractDependencyInjector { injectables.put("abChooser", ABChooser.class); injectables.put("abTests", new ABTests()); injectables.put("abTestEventDao", new ABTestEventDao()); - injectables.put("abTestReporter", ABTestReporter.class); + injectables.put("abTestEventReportingService", ABTestEventReportingService.class); + injectables.put("abTestInvoker", ABTestInvoker.class); injectables.put("featureFlipper", FeatureFlipper.class); // com.todoroo.astrid.tags diff --git a/astrid/src/com/todoroo/astrid/service/StartupService.java b/astrid/src/com/todoroo/astrid/service/StartupService.java index b0d48348b..ca3d00888 100644 --- a/astrid/src/com/todoroo/astrid/service/StartupService.java +++ b/astrid/src/com/todoroo/astrid/service/StartupService.java @@ -42,6 +42,7 @@ import com.todoroo.astrid.opencrx.OpencrxCoreUtils; import com.todoroo.astrid.producteev.ProducteevUtilities; import com.todoroo.astrid.reminders.ReminderStartupReceiver; import com.todoroo.astrid.service.abtesting.ABChooser; +import com.todoroo.astrid.service.abtesting.ABTestEventReportingService; import com.todoroo.astrid.service.abtesting.FeatureFlipper; import com.todoroo.astrid.utility.AstridPreferences; import com.todoroo.astrid.utility.Constants; @@ -88,6 +89,8 @@ public class StartupService { @Autowired ABChooser abChooser; + @Autowired ABTestEventReportingService abTestEventReportingService; + /** * bit to prevent multiple initializations */ @@ -212,7 +215,7 @@ public class StartupService { } }).start(); - abChooser.makeChoicesForAllTests(latestSetVersion == 0, taskService.getUserActivationStatus()); + initializeABTesting(latestSetVersion == 0); AstridPreferences.setPreferenceDefaults(); trackABTestingData(); @@ -224,6 +227,12 @@ public class StartupService { hasStartedUp = true; } + private void initializeABTesting(boolean newUser) { + abTestEventReportingService.initialize(); + abTestEventReportingService.pushAllUnreportedABTestEvents(); + abChooser.makeChoicesForAllTests(newUser, taskService.getUserActivationStatus()); + } + private void trackABTestingData() { long firstLaunchTime = Preferences.getLong(AstridPreferences.P_FIRST_LAUNCH, 0); long now = DateUtilities.now(); diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java index 6e88c5f72..9d14263b7 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java @@ -30,7 +30,7 @@ public final class ABTestEventReportingService { private ABTestEventDao abTestEventDao; @Autowired - private ABTestReporter abTestReporter; + private ABTestInvoker abTestInvoker; public ABTestEventReportingService() { DependencyInjectionService.getInstance().inject(this); @@ -53,7 +53,7 @@ public final class ABTestEventReportingService { public void run() { try { JSONArray payload = new JSONArray().put(jsonFromABTestEvent(model)); - abTestReporter.post(payload); + abTestInvoker.post(payload); model.setValue(ABTestEvent.REPORTED, 1); abTestEventDao.saveExisting(model); } catch (JSONException e) { @@ -66,31 +66,34 @@ public final class ABTestEventReportingService { } public void pushAllUnreportedABTestEvents() { - TodorooCursor unreported = abTestEventDao.query(Query.select(ABTestEvent.PROPERTIES) + final TodorooCursor unreported = abTestEventDao.query(Query.select(ABTestEvent.PROPERTIES) .where(ABTestEvent.REPORTED.eq(0))); - try { - if (unreported.getCount() > 0) { - JSONArray payload = jsonArrayFromABTestEvents(unreported); - abTestReporter.post(payload); - for (unreported.moveToFirst(); !unreported.isAfterLast(); unreported.moveToNext()) { - ABTestEvent model = new ABTestEvent(unreported); - model.setValue(ABTestEvent.REPORTED, 1); - abTestEventDao.saveExisting(model); + if (unreported.getCount() > 0) { + new Thread(new Runnable() { + @Override + public void run() { + try { + JSONArray payload = jsonArrayFromABTestEvents(unreported); + abTestInvoker.post(payload); + for (unreported.moveToFirst(); !unreported.isAfterLast(); unreported.moveToNext()) { + ABTestEvent model = new ABTestEvent(unreported); + model.setValue(ABTestEvent.REPORTED, 1); + abTestEventDao.saveExisting(model); + } + } catch (JSONException e) { + handleException(e); + } catch (IOException e) { + handleException(e); + } finally { + unreported.close(); + } } - } - } catch (JSONException e) { - handleException(e); - } catch (IOException e) { - handleException(e); - } finally { - unreported.close(); + }).start(); } } private void handleException(Exception e) { Log.e("analytics", "analytics-error", e); - - // TODO: Schedule retry } private static JSONObject jsonFromABTestEvent(ABTestEvent model) throws JSONException { diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestReporter.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java similarity index 97% rename from astrid/src/com/todoroo/astrid/service/abtesting/ABTestReporter.java rename to astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java index b4a1a8183..1b56e41b1 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestReporter.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java @@ -20,7 +20,7 @@ import com.todoroo.andlib.service.DependencyInjectionService; import com.todoroo.andlib.service.RestClient; @SuppressWarnings("nls") -public class ABTestReporter { +public class ABTestInvoker { /** NOTE: these values are development values and will not work on production */ private static final String URL = "http://analytics.astrid.com/api/1/retention"; @@ -29,7 +29,7 @@ public class ABTestReporter { @Autowired private RestClient restClient; - public ABTestReporter() { + public ABTestInvoker() { DependencyInjectionService.getInstance().inject(this); } From bf8dc12ff696f66214d0b732f11a27cc763d90bf Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 15:23:11 -0700 Subject: [PATCH 09/19] Create relative date events on startup --- .../todoroo/astrid/dao/ABTestEventDao.java | 30 +++++++++++++++++++ .../astrid/service/StartupService.java | 28 +++-------------- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java index 7fdd9a63b..9cf8e9781 100644 --- a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java +++ b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java @@ -67,4 +67,34 @@ public class ABTestEventDao extends DatabaseDao { return true; } + public void createRelativeDateEvents() { + TodorooCursor allEvents = query(Query.select(ABTestEvent.TEST_NAME, ABTestEvent.DATE_RECORDED) + .where(ABTestEvent.TIME_INTERVAL.eq(ABTestEvent.TIME_INTERVAL_0))); + + try { + long now = DateUtilities.now(); + for (allEvents.moveToFirst(); !allEvents.isAfterLast(); allEvents.moveToNext()) { + ABTestEvent event = new ABTestEvent(allEvents); + long baseTime = event.getValue(ABTestEvent.DATE_RECORDED); + long timeSinceBase = now - baseTime; + + String testName = event.getValue(ABTestEvent.TEST_NAME); + int timeInterval = -1; + if (timeSinceBase > 3 * DateUtilities.ONE_WEEK) + timeInterval = ABTestEvent.TIME_INTERVAL_21; + else if (timeSinceBase > 2 * DateUtilities.ONE_WEEK) + timeInterval = ABTestEvent.TIME_INTERVAL_14; + else if (timeSinceBase > DateUtilities.ONE_WEEK) + timeInterval = ABTestEvent.TIME_INTERVAL_7; + else if (timeSinceBase > 3 * DateUtilities.ONE_DAY) + timeInterval = ABTestEvent.TIME_INTERVAL_3; + + if (timeInterval > 0) + createTestEventWithTimeInterval(testName, timeInterval); + } + } finally { + allEvents.close(); + } + } + } diff --git a/astrid/src/com/todoroo/astrid/service/StartupService.java b/astrid/src/com/todoroo/astrid/service/StartupService.java index ca3d00888..0887193b8 100644 --- a/astrid/src/com/todoroo/astrid/service/StartupService.java +++ b/astrid/src/com/todoroo/astrid/service/StartupService.java @@ -35,6 +35,7 @@ import com.todoroo.astrid.actfm.sync.ActFmSyncService; import com.todoroo.astrid.backup.BackupConstants; import com.todoroo.astrid.backup.BackupService; import com.todoroo.astrid.backup.TasksXmlImporter; +import com.todoroo.astrid.dao.ABTestEventDao; import com.todoroo.astrid.dao.Database; import com.todoroo.astrid.gtasks.GtasksPreferenceService; import com.todoroo.astrid.gtasks.sync.GtasksSyncService; @@ -91,6 +92,8 @@ public class StartupService { @Autowired ABTestEventReportingService abTestEventReportingService; + @Autowired ABTestEventDao abTestEventDao; + /** * bit to prevent multiple initializations */ @@ -218,8 +221,6 @@ public class StartupService { initializeABTesting(latestSetVersion == 0); AstridPreferences.setPreferenceDefaults(); - trackABTestingData(); - // check for task killers if(!Constants.OEM) showTaskKillerHelp(context); @@ -231,28 +232,7 @@ public class StartupService { abTestEventReportingService.initialize(); abTestEventReportingService.pushAllUnreportedABTestEvents(); abChooser.makeChoicesForAllTests(newUser, taskService.getUserActivationStatus()); - } - - private void trackABTestingData() { - long firstLaunchTime = Preferences.getLong(AstridPreferences.P_FIRST_LAUNCH, 0); - long now = DateUtilities.now(); - long timeSinceFirst = now - firstLaunchTime; - - if (firstLaunchTime == 0) { - // Event days +0 - } - if (timeSinceFirst > DateUtilities.ONE_DAY * 3 /*&& !some condition*/) { - // Event days +3 - } - if (timeSinceFirst > DateUtilities.ONE_WEEK /*&& !some condition*/) { - // Event days +7 - } - if (timeSinceFirst > 2 * DateUtilities.ONE_WEEK /*&& !some condition*/) { - // Event days +14 - } - if (timeSinceFirst > 3 * DateUtilities.ONE_WEEK /*&& !some condition*/) { - // Event days +21 - } + abTestEventDao.createRelativeDateEvents(); } /** From 1746ec4e9fe320b7b39109f83b0c076c521ff76b Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 15:25:11 -0700 Subject: [PATCH 10/19] Reorder stuff for better performance --- astrid/src/com/todoroo/astrid/service/StartupService.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/astrid/src/com/todoroo/astrid/service/StartupService.java b/astrid/src/com/todoroo/astrid/service/StartupService.java index 0887193b8..9e83483b1 100644 --- a/astrid/src/com/todoroo/astrid/service/StartupService.java +++ b/astrid/src/com/todoroo/astrid/service/StartupService.java @@ -183,6 +183,9 @@ public class StartupService { // perform startup activities in a background thread final int finalLatestVersion = latestSetVersion; + + initializeABTesting(latestSetVersion == 0); + new Thread(new Runnable() { public void run() { // start widget updating alarm @@ -215,10 +218,11 @@ public class StartupService { // Check for feature flips featureFlipper.updateFeatures(); + + abTestEventDao.createRelativeDateEvents(); } }).start(); - initializeABTesting(latestSetVersion == 0); AstridPreferences.setPreferenceDefaults(); // check for task killers @@ -232,7 +236,6 @@ public class StartupService { abTestEventReportingService.initialize(); abTestEventReportingService.pushAllUnreportedABTestEvents(); abChooser.makeChoicesForAllTests(newUser, taskService.getUserActivationStatus()); - abTestEventDao.createRelativeDateEvents(); } /** From 65d13cbf2299ee7a198fe5624c36d07435e00cb9 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 15:31:36 -0700 Subject: [PATCH 11/19] Hide things that don't need to be visible --- astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java index 9cf8e9781..30e1fa819 100644 --- a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java +++ b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java @@ -34,13 +34,13 @@ public class ABTestEventDao extends DatabaseDao { createNew(event); } - public boolean createTestEventWithTimeInterval(String testName, int timeInterval) { + private void createTestEventWithTimeInterval(String testName, int timeInterval) { TodorooCursor existing = query(Query.select(ABTestEvent.PROPERTIES) .where(ABTestEvent.TEST_NAME.eq(testName)).orderBy(Order.asc(ABTestEvent.TIME_INTERVAL))); try { if (existing.getCount() == 0) - return false; + return; existing.moveToLast(); ABTestEvent item = new ABTestEvent(existing); @@ -51,7 +51,7 @@ public class ABTestEventDao extends DatabaseDao { ABTestEvent.TIME_INTERVALS, timeInterval); if (lastRecordedTimeIntervalIndex < 0 || currentTimeIntervalIndex < 0) - return false; + return; long now = DateUtilities.now(); for (int i = lastRecordedTimeIntervalIndex + 1; i <= currentTimeIntervalIndex; i++) { @@ -64,7 +64,7 @@ public class ABTestEventDao extends DatabaseDao { } finally { existing.close(); } - return true; + return; } public void createRelativeDateEvents() { From 89de6e9fdeaca1eef99f3b256222950503aa0973 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 16:33:38 -0700 Subject: [PATCH 12/19] Optimizations with reporting using arrays instead of separate requests, bugfixes --- .../com/todoroo/astrid/data/ABTestEvent.java | 2 +- .../service/AstridDependencyInjector.java | 8 +-- .../astrid/service/StartupService.java | 9 +-- .../ABTestEventReportingService.java | 57 ++++++++----------- 4 files changed, 31 insertions(+), 45 deletions(-) diff --git a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java index 06aa942af..4644d7c49 100644 --- a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java +++ b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java @@ -89,7 +89,7 @@ public class ABTestEvent extends AbstractModel { TABLE, "reported"); // 0 if not yet reported, 1 if reported successfully /** List of all properties for this model */ - public static final Property[] PROPERTIES = generateProperties(Task.class); + public static final Property[] PROPERTIES = generateProperties(ABTestEvent.class); diff --git a/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java b/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java index 5df150afd..919abb57b 100644 --- a/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java +++ b/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java @@ -65,9 +65,9 @@ public class AstridDependencyInjector extends AbstractDependencyInjector { // com.todoroo.astrid.dao injectables.put("database", Database.class); - injectables.put("taskDao", new TaskDao()); - injectables.put("metadataDao", new MetadataDao()); - injectables.put("tagDataDao", new TagDataDao()); + injectables.put("taskDao", TaskDao.class); + injectables.put("metadataDao", MetadataDao.class); + injectables.put("tagDataDao", TagDataDao.class); injectables.put("storeObjectDao", StoreObjectDao.class); injectables.put("updateDao", UpdateDao.class); injectables.put("userDao", UserDao.class); @@ -103,7 +103,7 @@ public class AstridDependencyInjector extends AbstractDependencyInjector { // AB testing injectables.put("abChooser", ABChooser.class); injectables.put("abTests", new ABTests()); - injectables.put("abTestEventDao", new ABTestEventDao()); + injectables.put("abTestEventDao", ABTestEventDao.class); injectables.put("abTestEventReportingService", ABTestEventReportingService.class); injectables.put("abTestInvoker", ABTestInvoker.class); injectables.put("featureFlipper", FeatureFlipper.class); diff --git a/astrid/src/com/todoroo/astrid/service/StartupService.java b/astrid/src/com/todoroo/astrid/service/StartupService.java index 9e83483b1..8a18995bc 100644 --- a/astrid/src/com/todoroo/astrid/service/StartupService.java +++ b/astrid/src/com/todoroo/astrid/service/StartupService.java @@ -184,7 +184,7 @@ public class StartupService { // perform startup activities in a background thread final int finalLatestVersion = latestSetVersion; - initializeABTesting(latestSetVersion == 0); + abChooser.makeChoicesForAllTests(latestSetVersion == 0, taskService.getUserActivationStatus()); new Thread(new Runnable() { public void run() { @@ -220,6 +220,7 @@ public class StartupService { featureFlipper.updateFeatures(); abTestEventDao.createRelativeDateEvents(); + abTestEventReportingService.pushAllUnreportedABTestEvents(); } }).start(); @@ -232,12 +233,6 @@ public class StartupService { hasStartedUp = true; } - private void initializeABTesting(boolean newUser) { - abTestEventReportingService.initialize(); - abTestEventReportingService.pushAllUnreportedABTestEvents(); - abChooser.makeChoicesForAllTests(newUser, taskService.getUserActivationStatus()); - } - /** * @param context * @param e error that was raised diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java index 9d14263b7..d1793d7b3 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java @@ -8,10 +8,10 @@ import org.json.JSONObject; import android.util.Log; -import com.todoroo.andlib.data.DatabaseDao.ModelUpdateListener; import com.todoroo.andlib.data.TodorooCursor; import com.todoroo.andlib.service.Autowired; import com.todoroo.andlib.service.DependencyInjectionService; +import com.todoroo.andlib.sql.Order; import com.todoroo.andlib.sql.Query; import com.todoroo.astrid.dao.ABTestEventDao; import com.todoroo.astrid.data.ABTestEvent; @@ -36,38 +36,11 @@ public final class ABTestEventReportingService { DependencyInjectionService.getInstance().inject(this); } - public void initialize() { - abTestEventDao.addListener(new ModelUpdateListener() { - @Override - public void onModelUpdated(ABTestEvent model) { - if (model.getValue(ABTestEvent.REPORTED) == 1) - return; - - pushABTestEvent(model); - } - }); - } - - public void pushABTestEvent(final ABTestEvent model) { - new Thread(new Runnable() { - public void run() { - try { - JSONArray payload = new JSONArray().put(jsonFromABTestEvent(model)); - abTestInvoker.post(payload); - model.setValue(ABTestEvent.REPORTED, 1); - abTestEventDao.saveExisting(model); - } catch (JSONException e) { - handleException(e); - } catch (IOException e) { - handleException(e); - } - }; - }).start(); - } - public void pushAllUnreportedABTestEvents() { final TodorooCursor unreported = abTestEventDao.query(Query.select(ABTestEvent.PROPERTIES) - .where(ABTestEvent.REPORTED.eq(0))); + .where(ABTestEvent.REPORTED.eq(0)) + .orderBy(Order.asc(ABTestEvent.TEST_NAME)) + .orderBy(Order.asc(ABTestEvent.TIME_INTERVAL))); if (unreported.getCount() > 0) { new Thread(new Runnable() { @Override @@ -102,7 +75,7 @@ public final class ABTestEventReportingService { payload.put(KEY_VARIANT, model.getValue(ABTestEvent.TEST_VARIANT)); payload.put(KEY_NEW_USER, model.getValue(ABTestEvent.NEW_USER) > 0 ? true : false); payload.put(KEY_ACTIVE_USER, model.getValue(ABTestEvent.ACTIVATED_USER) > 0 ? true : false); - payload.put(KEY_DAYS, model.getValue(ABTestEvent.TIME_INTERVAL)); + payload.put(KEY_DAYS, new JSONArray().put(model.getValue(ABTestEvent.TIME_INTERVAL))); long date = model.getValue(ABTestEvent.DATE_RECORDED) / 1000L; payload.put(KEY_DATE, date); @@ -112,10 +85,28 @@ public final class ABTestEventReportingService { private static JSONArray jsonArrayFromABTestEvents(TodorooCursor events) throws JSONException { JSONArray result = new JSONArray(); + + String lastTestKey = ""; + JSONObject testAcc = null; for (events.moveToFirst(); !events.isAfterLast(); events.moveToNext()) { ABTestEvent model = new ABTestEvent(events); - result.put(jsonFromABTestEvent(model)); + + if (!model.getValue(ABTestEvent.TEST_NAME).equals(lastTestKey)) { + if (testAcc != null) + result.put(testAcc); + testAcc = jsonFromABTestEvent(model); + lastTestKey = model.getValue(ABTestEvent.TEST_NAME); + } else { + int interval = model.getValue(ABTestEvent.TIME_INTERVAL); + if (testAcc != null) { // this should never happen, just stopping the compiler from complaining + JSONArray daysArray = testAcc.getJSONArray(KEY_DAYS); + daysArray.put(interval); + } + } + } + if (testAcc != null) + result.put(testAcc); return result; } From 0a2ff5de4b27283027493dc54fc55031e4aa4884 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 16:42:57 -0700 Subject: [PATCH 13/19] Move retention tracking to TLA onCreate since startup service may not run if app has been living in background --- .../com/todoroo/astrid/activity/TaskListActivity.java | 8 ++++++++ .../src/com/todoroo/astrid/service/StartupService.java | 9 --------- .../service/abtesting/ABTestEventReportingService.java | 10 ++++++++++ 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/astrid/src/com/todoroo/astrid/activity/TaskListActivity.java b/astrid/src/com/todoroo/astrid/activity/TaskListActivity.java index 10a17361c..0e9f4b692 100644 --- a/astrid/src/com/todoroo/astrid/activity/TaskListActivity.java +++ b/astrid/src/com/todoroo/astrid/activity/TaskListActivity.java @@ -26,6 +26,8 @@ import android.widget.PopupWindow.OnDismissListener; import android.widget.TextView; import com.timsu.astrid.R; +import com.todoroo.andlib.service.Autowired; +import com.todoroo.andlib.service.DependencyInjectionService; import com.todoroo.andlib.sql.Functions; import com.todoroo.andlib.sql.QueryTemplate; import com.todoroo.andlib.utility.AndroidUtilities; @@ -45,6 +47,7 @@ import com.todoroo.astrid.data.Task; import com.todoroo.astrid.service.StatisticsConstants; import com.todoroo.astrid.service.StatisticsService; import com.todoroo.astrid.service.ThemeService; +import com.todoroo.astrid.service.abtesting.ABTestEventReportingService; import com.todoroo.astrid.tags.TagService; import com.todoroo.astrid.ui.DateChangedAlerts; import com.todoroo.astrid.ui.FragmentPopover; @@ -61,6 +64,8 @@ public class TaskListActivity extends AstridActivity implements MainMenuListener /** token for indicating source of TLA launch */ public static final String TOKEN_SOURCE = "source"; //$NON-NLS-1$ + @Autowired private ABTestEventReportingService abTestEventReportingService; + private View listsNav; private ImageView listsNavDisclosure; private TextView lists; @@ -136,6 +141,7 @@ public class TaskListActivity extends AstridActivity implements MainMenuListener protected void onCreate(Bundle savedInstanceState) { ThemeService.applyTheme(this); super.onCreate(savedInstanceState); + DependencyInjectionService.getInstance().inject(this); if (AndroidUtilities.isTabletSized(this)) { setContentView(R.layout.task_list_wrapper_activity_3pane); @@ -198,6 +204,8 @@ public class TaskListActivity extends AstridActivity implements MainMenuListener if (getIntent().hasExtra(TOKEN_SOURCE)) { trackActivitySource(); } + + abTestEventReportingService.trackUserRetention(); } private boolean swipeIsEnabled() { diff --git a/astrid/src/com/todoroo/astrid/service/StartupService.java b/astrid/src/com/todoroo/astrid/service/StartupService.java index 8a18995bc..052d1fb6c 100644 --- a/astrid/src/com/todoroo/astrid/service/StartupService.java +++ b/astrid/src/com/todoroo/astrid/service/StartupService.java @@ -35,7 +35,6 @@ import com.todoroo.astrid.actfm.sync.ActFmSyncService; import com.todoroo.astrid.backup.BackupConstants; import com.todoroo.astrid.backup.BackupService; import com.todoroo.astrid.backup.TasksXmlImporter; -import com.todoroo.astrid.dao.ABTestEventDao; import com.todoroo.astrid.dao.Database; import com.todoroo.astrid.gtasks.GtasksPreferenceService; import com.todoroo.astrid.gtasks.sync.GtasksSyncService; @@ -43,7 +42,6 @@ import com.todoroo.astrid.opencrx.OpencrxCoreUtils; import com.todoroo.astrid.producteev.ProducteevUtilities; import com.todoroo.astrid.reminders.ReminderStartupReceiver; import com.todoroo.astrid.service.abtesting.ABChooser; -import com.todoroo.astrid.service.abtesting.ABTestEventReportingService; import com.todoroo.astrid.service.abtesting.FeatureFlipper; import com.todoroo.astrid.utility.AstridPreferences; import com.todoroo.astrid.utility.Constants; @@ -90,10 +88,6 @@ public class StartupService { @Autowired ABChooser abChooser; - @Autowired ABTestEventReportingService abTestEventReportingService; - - @Autowired ABTestEventDao abTestEventDao; - /** * bit to prevent multiple initializations */ @@ -218,9 +212,6 @@ public class StartupService { // Check for feature flips featureFlipper.updateFeatures(); - - abTestEventDao.createRelativeDateEvents(); - abTestEventReportingService.pushAllUnreportedABTestEvents(); } }).start(); diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java index d1793d7b3..a4cff58e1 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java @@ -36,6 +36,16 @@ public final class ABTestEventReportingService { DependencyInjectionService.getInstance().inject(this); } + public void trackUserRetention() { + new Thread(new Runnable() { + @Override + public void run() { + abTestEventDao.createRelativeDateEvents(); + pushAllUnreportedABTestEvents(); + } + }).start(); + } + public void pushAllUnreportedABTestEvents() { final TodorooCursor unreported = abTestEventDao.query(Query.select(ABTestEvent.PROPERTIES) .where(ABTestEvent.REPORTED.eq(0)) From 2acea6f48bc0ee478ade0e6a7976b3fc4387c098 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 16:44:13 -0700 Subject: [PATCH 14/19] Don't report retention if statistics tracking turned off --- .../service/abtesting/ABTestEventReportingService.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java index a4cff58e1..9b82e5cce 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java @@ -15,6 +15,7 @@ import com.todoroo.andlib.sql.Order; import com.todoroo.andlib.sql.Query; import com.todoroo.astrid.dao.ABTestEventDao; import com.todoroo.astrid.data.ABTestEvent; +import com.todoroo.astrid.service.StatisticsService; @SuppressWarnings("nls") public final class ABTestEventReportingService { @@ -46,7 +47,9 @@ public final class ABTestEventReportingService { }).start(); } - public void pushAllUnreportedABTestEvents() { + private void pushAllUnreportedABTestEvents() { + if (StatisticsService.dontCollectStatistics()) + return; final TodorooCursor unreported = abTestEventDao.query(Query.select(ABTestEvent.PROPERTIES) .where(ABTestEvent.REPORTED.eq(0)) .orderBy(Order.asc(ABTestEvent.TEST_NAME)) From c26a1608559ff0cce87555aece32f4b7a5a2cd29 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 16:59:53 -0700 Subject: [PATCH 15/19] Added insertion of analytics api keys to build script --- astrid/build.xml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/astrid/build.xml b/astrid/build.xml index 47da849f1..6364021fd 100644 --- a/astrid/build.xml +++ b/astrid/build.xml @@ -137,6 +137,13 @@ + + + From 69cbd31148e483e8d19cd072201cbdf32d47fe0d Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 17:25:46 -0700 Subject: [PATCH 16/19] Added ability to specifiy different probability distributions for new and existing users --- .../astrid/service/abtesting/ABChooser.java | 2 +- .../astrid/service/abtesting/ABTests.java | 52 ++++++++----------- .../service/abtesting/FeatureFlipper.java | 9 ---- 3 files changed, 22 insertions(+), 41 deletions(-) diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java index 9c647c4e9..08a3c6bfb 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java @@ -57,7 +57,7 @@ public class ABChooser { int chosen = NO_OPTION; if (abTests.isValidTestKey(testKey)) { - int[] optionProbs = abTests.getProbsForTestKey(testKey); + int[] optionProbs = abTests.getProbsForTestKey(testKey, newUser); String[] optionDescriptions = abTests.getDescriptionsForTestKey(testKey); chosen = chooseOption(optionProbs); setChoiceForTest(testKey, chosen); diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java index 292fb0835..17c9f0e86 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java @@ -20,34 +20,18 @@ public class ABTests { * @param key * @return */ - public synchronized int[] getProbsForTestKey(String key) { + public synchronized int[] getProbsForTestKey(String key, boolean newUser) { if (bundles.containsKey(key)) { ABTestBundle bundle = bundles.get(key); - return bundle.weightedProbs; + if (newUser) + return bundle.newUserProbs; + else + return bundle.existingUserProbs; } else { return null; } } - /** - * Updates the weighted probability array for a given key. Returns true - * on success, false if they key doesn't exist or if the array is the wrong - * length. - * @param key - * @param newProbs - * @return - */ - public synchronized boolean setProbsForTestKey(String key, int[] newProbs) { - if (bundles.containsKey(newProbs)) { - ABTestBundle bundle = bundles.get(key); - if (bundle.descriptions == null || newProbs.length == bundle.descriptions.length) { - bundle.weightedProbs = newProbs; - return true; - } - } - return false; - } - /** * Gets the string array of option descriptions for an option key * @param key @@ -88,11 +72,13 @@ public class ABTests { private final HashMap bundles; private static class ABTestBundle { - public int[] weightedProbs; - public String[] descriptions; + protected final int[] newUserProbs; + protected final int[] existingUserProbs; + protected final String[] descriptions; - public ABTestBundle(int[] weightedProbs, String[] descriptions) { - this.weightedProbs = weightedProbs; + protected ABTestBundle(int[] newUserProbs, int[] existingUserProbs, String[] descriptions) { + this.newUserProbs = newUserProbs; + this.existingUserProbs = existingUserProbs; this.descriptions = descriptions; } } @@ -124,23 +110,27 @@ public class ABTests { * tagged from StatisticsService, they will be appended with attributes * that have that event in this array */ - public void addTest(String testKey, int[] probs, String[] descriptions) { - ABTestBundle bundle = new ABTestBundle(probs, descriptions); + public void addTest(String testKey, int[] newUserProbs, int[] existingUserProbs, String[] descriptions) { + ABTestBundle bundle = new ABTestBundle(newUserProbs, existingUserProbs, descriptions); bundles.put(testKey, bundle); } private void initialize() { // Set up //Calls to addTest go here - addTest(AB_TEST_SWIPE_ENABLED_KEY, AB_TEST_SWIPE_ENABLED_PROBS, AB_TEST_SWIPE_ENABLED_DESC); - addTest(AB_TEST_CONTACTS_PICKER_ENABLED, AB_TEST_CONTACTS_ENABLED_PROBS, AB_TEST_CONTACTS_ENABLED_DESC); + addTest(AB_TEST_SWIPE_ENABLED_KEY, AB_TEST_SWIPE_ENABLED_PROBS_NEW_USER, + AB_TEST_SWIPE_ENABLED_PROBS_EXISTING_USER, AB_TEST_SWIPE_ENABLED_DESC); + addTest(AB_TEST_CONTACTS_PICKER_ENABLED, AB_TEST_CONTACTS_ENABLED_PROBS_NEW_USER, + AB_TEST_CONTACTS_ENABLED_PROBS_EXISTING_USER, AB_TEST_CONTACTS_ENABLED_DESC); } public static final String AB_TEST_SWIPE_ENABLED_KEY = "swipeEnabled"; //$NON-NLS-1$ - private static final int[] AB_TEST_SWIPE_ENABLED_PROBS = { 1, 1 }; + private static final int[] AB_TEST_SWIPE_ENABLED_PROBS_NEW_USER = { 1, 1 }; + private static final int[] AB_TEST_SWIPE_ENABLED_PROBS_EXISTING_USER = { 1, 1 }; private static final String[] AB_TEST_SWIPE_ENABLED_DESC = { "swipe-lists-disabled", "swipe-lists-enabled" }; //$NON-NLS-1$//$NON-NLS-2$ public static final String AB_TEST_CONTACTS_PICKER_ENABLED = "contactsEnabled"; //$NON-NLS-1$ - private static final int[] AB_TEST_CONTACTS_ENABLED_PROBS = { 1, 1 }; + private static final int[] AB_TEST_CONTACTS_ENABLED_PROBS_NEW_USER = { 1, 1 }; + private static final int[] AB_TEST_CONTACTS_ENABLED_PROBS_EXISTING_USER = { 1, 1 }; private static final String[] AB_TEST_CONTACTS_ENABLED_DESC = { "contacts-disabled", "contacts-enabled" }; //$NON-NLS-1$//$NON-NLS-2$ diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/FeatureFlipper.java b/astrid/src/com/todoroo/astrid/service/abtesting/FeatureFlipper.java index 21136dc25..2d985e2e6 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/FeatureFlipper.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/FeatureFlipper.java @@ -53,15 +53,6 @@ public class FeatureFlipper { if (settings.has(KEY_SET_OPTION)) { int option = settings.getInt(KEY_SET_OPTION); abChooser.setChoiceForTest(key, option); - } else if (settings.has(KEY_PROBABILITIES)) { - JSONArray newProbabilities = settings.getJSONArray(KEY_PROBABILITIES); - int[] probs = new int[newProbabilities.length()]; - - for (int j = 0; j < newProbabilities.length(); j++) { - probs[j] = newProbabilities.getInt(j); - } - - abTests.setProbsForTestKey(key, probs); } } catch (Exception e) { e.printStackTrace(); From ac4e8b07f50d9635740215627388b4c656bb85b5 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 18:35:05 -0700 Subject: [PATCH 17/19] Added unit tests for ab test retention database --- .../todoroo/astrid/dao/ABTestEventDao.java | 7 +- .../astrid/service/ABTestingServiceTest.java | 95 +++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 tests/src/com/todoroo/astrid/service/ABTestingServiceTest.java diff --git a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java index 30e1fa819..aa860ff13 100644 --- a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java +++ b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java @@ -34,7 +34,12 @@ public class ABTestEventDao extends DatabaseDao { createNew(event); } - private void createTestEventWithTimeInterval(String testName, int timeInterval) { + /** + * Only public for unit testing--don't use unless you really mean it! + * @param testName + * @param timeInterval + */ + public void createTestEventWithTimeInterval(String testName, int timeInterval) { TodorooCursor existing = query(Query.select(ABTestEvent.PROPERTIES) .where(ABTestEvent.TEST_NAME.eq(testName)).orderBy(Order.asc(ABTestEvent.TIME_INTERVAL))); diff --git a/tests/src/com/todoroo/astrid/service/ABTestingServiceTest.java b/tests/src/com/todoroo/astrid/service/ABTestingServiceTest.java new file mode 100644 index 000000000..b7f194969 --- /dev/null +++ b/tests/src/com/todoroo/astrid/service/ABTestingServiceTest.java @@ -0,0 +1,95 @@ +package com.todoroo.astrid.service; + +import com.todoroo.andlib.data.TodorooCursor; +import com.todoroo.andlib.service.Autowired; +import com.todoroo.andlib.sql.Order; +import com.todoroo.andlib.sql.Query; +import com.todoroo.andlib.utility.AndroidUtilities; +import com.todoroo.andlib.utility.Preferences; +import com.todoroo.astrid.dao.ABTestEventDao; +import com.todoroo.astrid.data.ABTestEvent; +import com.todoroo.astrid.service.abtesting.ABChooser; +import com.todoroo.astrid.service.abtesting.ABTests; +import com.todoroo.astrid.test.DatabaseTestCase; + +public class ABTestingServiceTest extends DatabaseTestCase { + + @Autowired ABTestEventDao abTestEventDao; + @Autowired ABChooser abChooser; + @Autowired ABTests abTests; + + public void testReportInitialEventNewUser() { + testInitialEvents(true, false); + } + + public void testReportInitialEventExistingUser() { + testInitialEvents(false, true); + } + + private void testInitialEvents(boolean newUser, boolean activatedUser) { + testInterval(newUser, activatedUser, ABTestEvent.TIME_INTERVAL_0); + } + + public void testIntervalEventWithShortIntervalNewUser() { + testIntervalEventWithShortInterval(true, false); + } + + public void testIntervalEventWithShortIntervalExistingUser() { + testIntervalEventWithShortInterval(false, true); + } + + public void testIntervalEventWithLongIntervalNewUser() { + testIntervalEventWithLongInterval(true, false); + } + + public void testIntervalEventWithLongIntervalExistingUser() { + testIntervalEventWithLongInterval(false, true); + } + + private void testIntervalEventWithShortInterval(boolean newUser, boolean activatedUser) { + testInterval(newUser, activatedUser, ABTestEvent.TIME_INTERVAL_3); + } + + private void testIntervalEventWithLongInterval(boolean newUser, boolean activatedUser) { + testInterval(newUser, activatedUser, ABTestEvent.TIME_INTERVAL_14); + } + + private void testInterval(boolean newUser, boolean activatedUser, int testInterval) { + abChooser.makeChoicesForAllTests(newUser, activatedUser); + abTestEventDao.createTestEventWithTimeInterval(TEST_NAME, testInterval); + + TodorooCursor events = abTestEventDao.query( + Query.select(ABTestEvent.PROPERTIES) + .where(ABTestEvent.TEST_NAME.eq(TEST_NAME)) + .orderBy(Order.asc(ABTestEvent.TIME_INTERVAL))); + try { + int maxIntervalIndex = AndroidUtilities.indexOf(ABTestEvent.TIME_INTERVALS, testInterval); + assertEquals(maxIntervalIndex + 1, events.getCount()); + + for (int i = 0; i < events.getCount(); i++) { + events.moveToNext(); + ABTestEvent event = new ABTestEvent(events); + assertExpectedValues(event, newUser, activatedUser, ABTestEvent.TIME_INTERVALS[i]); + } + } finally { + events.close(); + } + } + + private void assertExpectedValues(ABTestEvent event, boolean newUser, boolean activatedUser, int timeInterval) { + assertEquals(TEST_NAME, event.getValue(ABTestEvent.TEST_NAME)); + assertEquals(newUser ? 1 : 0, event.getValue(ABTestEvent.NEW_USER).intValue()); + assertEquals(activatedUser ? 1 : 0, event.getValue(ABTestEvent.ACTIVATED_USER).intValue()); + assertEquals(timeInterval, event.getValue(ABTestEvent.TIME_INTERVAL).intValue()); + } + + @Override + protected void setUp() throws Exception { + super.setUp(); + abTests.addTest(TEST_NAME, new int[] { 9, 1 } , new int[] { 1, 9 }, TEST_OPTIONS); + Preferences.clear(TEST_NAME); + } + + private static final String TEST_NAME = "test_experiment"; + private static final String[] TEST_OPTIONS = new String[] { "opt-1", "opt-2" }; +} From fe47ed4eddbdc0cb2600dfcb41c868ae3e70a178 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Fri, 27 Apr 2012 13:08:15 -0700 Subject: [PATCH 18/19] Added documentation and comments --- .../astrid/activity/TaskListActivity.java | 2 ++ .../com/todoroo/astrid/dao/ABTestEventDao.java | 16 ++++++++++++++++ .../com/todoroo/astrid/data/ABTestEvent.java | 5 +++++ .../todoroo/astrid/service/StartupService.java | 3 ++- .../astrid/service/abtesting/ABChooser.java | 7 ++----- .../abtesting/ABTestEventReportingService.java | 13 +++++++++++++ .../service/abtesting/ABTestInvoker.java | 18 ++++++++++++++++++ .../astrid/service/abtesting/ABTests.java | 11 +++++------ 8 files changed, 63 insertions(+), 12 deletions(-) diff --git a/astrid/src/com/todoroo/astrid/activity/TaskListActivity.java b/astrid/src/com/todoroo/astrid/activity/TaskListActivity.java index 0e9f4b692..7b7c4be03 100644 --- a/astrid/src/com/todoroo/astrid/activity/TaskListActivity.java +++ b/astrid/src/com/todoroo/astrid/activity/TaskListActivity.java @@ -205,6 +205,8 @@ public class TaskListActivity extends AstridActivity implements MainMenuListener trackActivitySource(); } + // Have to call this here because sometimes StartupService + // isn't called (i.e. if the app was silently alive in the background) abTestEventReportingService.trackUserRetention(); } diff --git a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java index aa860ff13..7eeaaff31 100644 --- a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java +++ b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java @@ -21,6 +21,14 @@ public class ABTestEventDao extends DatabaseDao { setDatabase(database); } + /** + * Creates the baseline +0 days event (i.e. the first time the user + * launches the app containing this test) + * @param testName - the name of the test + * @param testVariant - which option was chosen for this user + * @param newUser - are they a new user? + * @param activeUser - are they an activated user? + */ public void createInitialTestEvent(String testName, String testVariant, boolean newUser, boolean activeUser) { ABTestEvent event = new ABTestEvent(); @@ -36,6 +44,9 @@ public class ABTestEventDao extends DatabaseDao { /** * Only public for unit testing--don't use unless you really mean it! + * + * Creates data points for the specified test name, creating one data point + * for each time interval that hasn't yet been recorded up to the specified one * @param testName * @param timeInterval */ @@ -72,6 +83,11 @@ public class ABTestEventDao extends DatabaseDao { return; } + /** + * For each baseline data point that exists in the database, check the current + * time against the time that baseline was recorded and report the appropriate + * +n days events. Called on startup. + */ public void createRelativeDateEvents() { TodorooCursor allEvents = query(Query.select(ABTestEvent.TEST_NAME, ABTestEvent.DATE_RECORDED) .where(ABTestEvent.TIME_INTERVAL.eq(ABTestEvent.TIME_INTERVAL_0))); diff --git a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java index 4644d7c49..6cd0c6d05 100644 --- a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java +++ b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java @@ -12,6 +12,11 @@ import com.todoroo.andlib.data.Table; import com.todoroo.andlib.data.TodorooCursor; import com.todoroo.astrid.api.AstridApiConstants; +/** + * Model for data points used for tracking user retention in AB tests + * @author Sam + * + */ @SuppressWarnings("nls") public class ABTestEvent extends AbstractModel { diff --git a/astrid/src/com/todoroo/astrid/service/StartupService.java b/astrid/src/com/todoroo/astrid/service/StartupService.java index 052d1fb6c..77c1eae61 100644 --- a/astrid/src/com/todoroo/astrid/service/StartupService.java +++ b/astrid/src/com/todoroo/astrid/service/StartupService.java @@ -175,11 +175,12 @@ public class StartupService { upgradeService.performSecondaryUpgrade(context); - // perform startup activities in a background thread final int finalLatestVersion = latestSetVersion; + // For any uninitialized ab test, make sure an option is chosen abChooser.makeChoicesForAllTests(latestSetVersion == 0, taskService.getUserActivationStatus()); + // perform startup activities in a background thread new Thread(new Runnable() { public void run() { // start widget updating alarm diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java index 08a3c6bfb..00ec3350a 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABChooser.java @@ -43,11 +43,8 @@ public class ABChooser { } /** - * Retrieves the choice for the specified feature if already made, - * or chooses one randomly from the distribution of feature probabilities - * if not. - * - * The statistics session needs to be open here in order to collect statistics + * If a choice/variant has not yet been selected for the specified test, + * make one and create the initial +0 analytics data point * * @param testKey - the preference key string of the option (defined in ABTests) */ diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java index 9b82e5cce..426ff19eb 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java @@ -17,6 +17,14 @@ import com.todoroo.astrid.dao.ABTestEventDao; import com.todoroo.astrid.data.ABTestEvent; import com.todoroo.astrid.service.StatisticsService; +/** + * Service to manage the reporting of launch events for AB testing. + * On startup, queries the ABTestEvent database for unreported data + * points, merges them into the expected JSONArray format, and + * pushes them to the server. + * @author Sam + * + */ @SuppressWarnings("nls") public final class ABTestEventReportingService { @@ -37,6 +45,11 @@ public final class ABTestEventReportingService { DependencyInjectionService.getInstance().inject(this); } + /** + * Called on startup from TaskListActivity. Creates any +n days + * launch events that need to be recorded, and pushes all unreported + * data to the server. + */ public void trackUserRetention() { new Thread(new Runnable() { @Override diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java index 1b56e41b1..b7a048e52 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java @@ -19,6 +19,11 @@ import com.todoroo.andlib.service.Autowired; import com.todoroo.andlib.service.DependencyInjectionService; import com.todoroo.andlib.service.RestClient; +/** + * Invoker for communicating with the Astrid Analytics server + * @author Sam + * + */ @SuppressWarnings("nls") public class ABTestInvoker { @@ -33,6 +38,13 @@ public class ABTestInvoker { DependencyInjectionService.getInstance().inject(this); } + /** + * Posts the payload to the analytics server + * @param payload - JSONArray of data points. Created by the + * helper method in ABTestReportingService + * @return + * @throws IOException + */ public JSONObject post(JSONArray payload) throws IOException { try { HttpEntity postData = createPostData(payload); @@ -51,6 +63,12 @@ public class ABTestInvoker { } + /** + * Converts the JSONArray payload into an HTTPEntity suitable for + * POSTing. + * @param payload + * @return + */ private HttpEntity createPostData(JSONArray payload) { List params = new ArrayList(); params.add(new BasicNameValuePair("apikey", API_KEY)); diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java index 17c9f0e86..8d2648721 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java @@ -93,22 +93,21 @@ public class ABTests { * @param testKey = "" * --This key is used to identify the option in the application and in the preferences * - * @param probs = { int, int, ... } + * @param newUserProbs = { int, int, ... } + * @param existingUserProbs = { int, int, ... } * --The different choices in an option correspond to an index in the probability array. * Probabilities are expressed as integers to easily define relative weights. For example, * the array { 1, 2 } would mean option 0 would happen one time for every two occurrences of option 1 * + * The first array is used for new users and the second is used for existing/upgrading users, + * allowing us to specify different distributions for each group. + * * (optional) * @param descriptions = { "...", "...", ... } * --A string description of each option. Useful for tagging events. The index of * each description should correspond to the events location in the probability array * (i.e. the arrays should be the same length if this one exists) * - * (optional) - * @param relevantEvents = { "...", "...", ... } - * --An arbitrary length list of relevant localytics events. When events are - * tagged from StatisticsService, they will be appended with attributes - * that have that event in this array */ public void addTest(String testKey, int[] newUserProbs, int[] existingUserProbs, String[] descriptions) { ABTestBundle bundle = new ABTestBundle(newUserProbs, existingUserProbs, descriptions); From 476b3af20661d9e6d52e2046abff34e1534c889d Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Fri, 27 Apr 2012 14:36:54 -0700 Subject: [PATCH 19/19] Refactor and cleanup based on Tim's suggestions --- .../todoroo/astrid/dao/ABTestEventDao.java | 19 +++++++---------- .../com/todoroo/astrid/data/ABTestEvent.java | 21 +++++++------------ .../ABTestEventReportingService.java | 7 ++++--- .../service/abtesting/ABTestInvoker.java | 6 ++---- .../astrid/service/abtesting/ABTests.java | 16 ++++---------- .../astrid/service/ABTestingServiceTest.java | 6 +++--- 6 files changed, 28 insertions(+), 47 deletions(-) diff --git a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java index 7eeaaff31..c070f9b41 100644 --- a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java +++ b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java @@ -36,7 +36,7 @@ public class ABTestEventDao extends DatabaseDao { event.setValue(ABTestEvent.TEST_VARIANT, testVariant); event.setValue(ABTestEvent.NEW_USER, newUser ? 1 : 0); event.setValue(ABTestEvent.ACTIVATED_USER, activeUser ? 1 : 0); - event.setValue(ABTestEvent.TIME_INTERVAL, ABTestEvent.TIME_INTERVAL_0); + event.setValue(ABTestEvent.TIME_INTERVAL, 0); event.setValue(ABTestEvent.DATE_RECORDED, DateUtilities.now()); createNew(event); @@ -90,25 +90,22 @@ public class ABTestEventDao extends DatabaseDao { */ public void createRelativeDateEvents() { TodorooCursor allEvents = query(Query.select(ABTestEvent.TEST_NAME, ABTestEvent.DATE_RECORDED) - .where(ABTestEvent.TIME_INTERVAL.eq(ABTestEvent.TIME_INTERVAL_0))); + .where(ABTestEvent.TIME_INTERVAL.eq(0))); try { long now = DateUtilities.now(); + ABTestEvent event = new ABTestEvent(); for (allEvents.moveToFirst(); !allEvents.isAfterLast(); allEvents.moveToNext()) { - ABTestEvent event = new ABTestEvent(allEvents); + event.readFromCursor(allEvents); long baseTime = event.getValue(ABTestEvent.DATE_RECORDED); long timeSinceBase = now - baseTime; String testName = event.getValue(ABTestEvent.TEST_NAME); int timeInterval = -1; - if (timeSinceBase > 3 * DateUtilities.ONE_WEEK) - timeInterval = ABTestEvent.TIME_INTERVAL_21; - else if (timeSinceBase > 2 * DateUtilities.ONE_WEEK) - timeInterval = ABTestEvent.TIME_INTERVAL_14; - else if (timeSinceBase > DateUtilities.ONE_WEEK) - timeInterval = ABTestEvent.TIME_INTERVAL_7; - else if (timeSinceBase > 3 * DateUtilities.ONE_DAY) - timeInterval = ABTestEvent.TIME_INTERVAL_3; + long days = timeSinceBase / DateUtilities.ONE_DAY; + for(int i = 0; i < ABTestEvent.TIME_INTERVALS.length; i++) + if(days >= ABTestEvent.TIME_INTERVALS[i]) + timeInterval = ABTestEvent.TIME_INTERVALS[i]; if (timeInterval > 0) createTestEventWithTimeInterval(testName, timeInterval); diff --git a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java index 6cd0c6d05..145fcc511 100644 --- a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java +++ b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java @@ -20,19 +20,7 @@ import com.todoroo.astrid.api.AstridApiConstants; @SuppressWarnings("nls") public class ABTestEvent extends AbstractModel { - public static final int TIME_INTERVAL_0 = 0; - public static final int TIME_INTERVAL_3 = 3; - public static final int TIME_INTERVAL_7 = 7; - public static final int TIME_INTERVAL_14 = 14; - public static final int TIME_INTERVAL_21 = 21; - - public static final int[] TIME_INTERVALS = { - TIME_INTERVAL_0, - TIME_INTERVAL_3, - TIME_INTERVAL_7, - TIME_INTERVAL_14, - TIME_INTERVAL_21 - }; + public static final int[] TIME_INTERVALS = { 0, 3, 7, 14, 21 }; // --- table and uri @@ -80,7 +68,8 @@ public class ABTestEvent extends AbstractModel { /** * Which time interval event this data point corresponds to. - * Should be one of the time interval constants defined above. + * Should be one of the time interval constants defined int the + * above array. */ public static final IntegerProperty TIME_INTERVAL = new IntegerProperty( TABLE, "timeInterval"); // one of the constants defined above @@ -121,6 +110,10 @@ public class ABTestEvent extends AbstractModel { readPropertiesFromCursor(cursor); } + public void readFromCursor(TodorooCursor cursor) { + super.readPropertiesFromCursor(cursor); + } + @Override public long getId() { return getIdHelper(ID); diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java index 426ff19eb..bf4b91508 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java @@ -74,8 +74,9 @@ public final class ABTestEventReportingService { try { JSONArray payload = jsonArrayFromABTestEvents(unreported); abTestInvoker.post(payload); + ABTestEvent model = new ABTestEvent(); for (unreported.moveToFirst(); !unreported.isAfterLast(); unreported.moveToNext()) { - ABTestEvent model = new ABTestEvent(unreported); + model.readFromCursor(unreported); model.setValue(ABTestEvent.REPORTED, 1); abTestEventDao.saveExisting(model); } @@ -114,9 +115,9 @@ public final class ABTestEventReportingService { String lastTestKey = ""; JSONObject testAcc = null; + ABTestEvent model = new ABTestEvent(); for (events.moveToFirst(); !events.isAfterLast(); events.moveToNext()) { - ABTestEvent model = new ABTestEvent(events); - + model.readFromCursor(events); if (!model.getValue(ABTestEvent.TEST_NAME).equals(lastTestKey)) { if (testAcc != null) result.put(testAcc); diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java index b7a048e52..2334dd5a6 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java @@ -48,8 +48,6 @@ public class ABTestInvoker { public JSONObject post(JSONArray payload) throws IOException { try { HttpEntity postData = createPostData(payload); - if (postData == null) - throw new IOException("Unsupported URL encoding"); String response = restClient.post(URL, postData); JSONObject object = new JSONObject(response); if (object.getString("status").equals("error")) { @@ -69,7 +67,7 @@ public class ABTestInvoker { * @param payload * @return */ - private HttpEntity createPostData(JSONArray payload) { + private HttpEntity createPostData(JSONArray payload) throws IOException { List params = new ArrayList(); params.add(new BasicNameValuePair("apikey", API_KEY)); params.add(new BasicNameValuePair("payload", payload.toString())); @@ -92,7 +90,7 @@ public class ABTestInvoker { try { return new UrlEncodedFormEntity(params, HTTP.UTF_8); } catch (UnsupportedEncodingException e) { - return null; + throw new IOException("Unsupported URL encoding"); } } diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java index 8d2648721..66b797819 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java @@ -116,21 +116,13 @@ public class ABTests { private void initialize() { // Set up //Calls to addTest go here - addTest(AB_TEST_SWIPE_ENABLED_KEY, AB_TEST_SWIPE_ENABLED_PROBS_NEW_USER, - AB_TEST_SWIPE_ENABLED_PROBS_EXISTING_USER, AB_TEST_SWIPE_ENABLED_DESC); - addTest(AB_TEST_CONTACTS_PICKER_ENABLED, AB_TEST_CONTACTS_ENABLED_PROBS_NEW_USER, - AB_TEST_CONTACTS_ENABLED_PROBS_EXISTING_USER, AB_TEST_CONTACTS_ENABLED_DESC); + addTest(AB_TEST_SWIPE_ENABLED_KEY, new int[] { 1, 1 }, + new int[] { 1, 1 }, new String[] { "swipe-lists-disabled", "swipe-lists-enabled" }); //$NON-NLS-1$//$NON-NLS-2$ + addTest(AB_TEST_CONTACTS_PICKER_ENABLED, new int[] { 1, 1 }, + new int[] { 1, 1 }, new String[] { "contacts-disabled", "contacts-enabled" }); //$NON-NLS-1$//$NON-NLS-2$ } public static final String AB_TEST_SWIPE_ENABLED_KEY = "swipeEnabled"; //$NON-NLS-1$ - private static final int[] AB_TEST_SWIPE_ENABLED_PROBS_NEW_USER = { 1, 1 }; - private static final int[] AB_TEST_SWIPE_ENABLED_PROBS_EXISTING_USER = { 1, 1 }; - private static final String[] AB_TEST_SWIPE_ENABLED_DESC = { "swipe-lists-disabled", "swipe-lists-enabled" }; //$NON-NLS-1$//$NON-NLS-2$ - public static final String AB_TEST_CONTACTS_PICKER_ENABLED = "contactsEnabled"; //$NON-NLS-1$ - private static final int[] AB_TEST_CONTACTS_ENABLED_PROBS_NEW_USER = { 1, 1 }; - private static final int[] AB_TEST_CONTACTS_ENABLED_PROBS_EXISTING_USER = { 1, 1 }; - private static final String[] AB_TEST_CONTACTS_ENABLED_DESC = { "contacts-disabled", "contacts-enabled" }; //$NON-NLS-1$//$NON-NLS-2$ - } diff --git a/tests/src/com/todoroo/astrid/service/ABTestingServiceTest.java b/tests/src/com/todoroo/astrid/service/ABTestingServiceTest.java index b7f194969..c1802b08f 100644 --- a/tests/src/com/todoroo/astrid/service/ABTestingServiceTest.java +++ b/tests/src/com/todoroo/astrid/service/ABTestingServiceTest.java @@ -27,7 +27,7 @@ public class ABTestingServiceTest extends DatabaseTestCase { } private void testInitialEvents(boolean newUser, boolean activatedUser) { - testInterval(newUser, activatedUser, ABTestEvent.TIME_INTERVAL_0); + testInterval(newUser, activatedUser, 0); } public void testIntervalEventWithShortIntervalNewUser() { @@ -47,11 +47,11 @@ public class ABTestingServiceTest extends DatabaseTestCase { } private void testIntervalEventWithShortInterval(boolean newUser, boolean activatedUser) { - testInterval(newUser, activatedUser, ABTestEvent.TIME_INTERVAL_3); + testInterval(newUser, activatedUser, 3); } private void testIntervalEventWithLongInterval(boolean newUser, boolean activatedUser) { - testInterval(newUser, activatedUser, ABTestEvent.TIME_INTERVAL_14); + testInterval(newUser, activatedUser, 14); } private void testInterval(boolean newUser, boolean activatedUser, int testInterval) {