Merge pull request #23258 from edx/mikix/experiment-flag

Add ExperimentWaffleFlag

common/lib/xmodule/xmodule/modulestore/tests/factories.py

@@ -600,10 +600,10 @@ def check_sum_of_calls(object_, methods, maximum_calls, minimum_calls=1, include
         print("".join(messages))
 
     # verify the counter actually worked by ensuring we have counted greater than (or equal to) the minimum calls
-    assert call_count >= minimum_calls
+    assert call_count >= minimum_calls, call_count
 
     # now verify the number of actual calls is less than (or equal to) the expected maximum
-    assert call_count <= maximum_calls
+    assert call_count <= maximum_calls, call_count
 
 
 def mongo_uses_error_check(store):

lms/djangoapps/course_api/blocks/tests/test_api.py

@@ -243,7 +243,7 @@ class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase):
             self._get_blocks(
                 course,
                 expected_mongo_queries=0,
-                expected_sql_queries=14 if with_storage_backing else 13,
+                expected_sql_queries=13 if with_storage_backing else 12,
             )
 
     @ddt.data(
@@ -260,9 +260,9 @@ class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase):
             clear_course_from_cache(course.id)
 
             if with_storage_backing:
-                num_sql_queries = 24
+                num_sql_queries = 23
             else:
-                num_sql_queries = 14
+                num_sql_queries = 13
 
             self._get_blocks(
                 course,

lms/djangoapps/courseware/tabs.py

@@ -12,20 +12,11 @@ from django.utils.translation import ugettext_noop
 from lms.djangoapps.courseware.access import has_access
 from lms.djangoapps.courseware.entrance_exams import user_can_skip_entrance_exam
 from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
-from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag, WaffleFlagNamespace
 from openedx.core.lib.course_tabs import CourseTabPluginManager
-from openedx.features.course_experience import UNIFIED_COURSE_TAB_FLAG, default_course_url_name
+from openedx.features.course_experience import RELATIVE_DATES_FLAG, UNIFIED_COURSE_TAB_FLAG, default_course_url_name
 from student.models import CourseEnrollment
 from xmodule.tabs import CourseTab, CourseTabList, course_reverse_func_from_name_func, key_checker
 
-COURSEWARE_TABS_NAMESPACE = WaffleFlagNamespace(name=u'courseware_tabs')
-
-ENABLE_DATES_TAB = CourseWaffleFlag(
-    waffle_namespace=COURSEWARE_TABS_NAMESPACE,
-    flag_name="enable_dates_tab",
-    flag_undefined_default=False
-)
-
 
 class EnrolledTab(CourseTab):
     """
@@ -330,10 +321,8 @@ class DatesTab(CourseTab):
     @classmethod
     def is_enabled(cls, course, user=None):
         """Returns true if this tab is enabled."""
-        # We want to only limit this feature to instructor led courses for now
-        if ENABLE_DATES_TAB.is_enabled(course.id):
-            return not CourseOverview.get_from_id(course.id).self_paced
-        return False
+        # We want to only limit this feature to instructor led courses for now (and limit to relative dates experiment)
+        return not CourseOverview.get_from_id(course.id).self_paced and RELATIVE_DATES_FLAG.is_enabled(course.id)
 
 
 def get_course_tab_list(user, course):

lms/djangoapps/courseware/tests/test_course_info.py

@@ -431,8 +431,8 @@ class SelfPacedCourseInfoTestCase(LoginEnrollmentTestCase, SharedModuleStoreTest
 
     def test_num_queries_instructor_paced(self):
         # TODO: decrease query count as part of REVO-28
-        self.fetch_course_info_with_queries(self.instructor_paced_course, 43, 3)
+        self.fetch_course_info_with_queries(self.instructor_paced_course, 55, 7)
 
     def test_num_queries_self_paced(self):
         # TODO: decrease query count as part of REVO-28
-        self.fetch_course_info_with_queries(self.self_paced_course, 43, 3)
+        self.fetch_course_info_with_queries(self.self_paced_course, 55, 7)

lms/djangoapps/experiments/flags.py

@@ -0,0 +1,133 @@
+"""
+Feature flag support for experiments
+"""
+
+import logging
+import pytz
+
+import dateutil
+from crum import get_current_request
+from edx_django_utils.cache import RequestCache
+
+from experiments.stable_bucketing import stable_bucketing_hash_group
+from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag
+from track import segment
+
+log = logging.getLogger(__name__)
+
+
+class ExperimentWaffleFlag(CourseWaffleFlag):
+    """
+    ExperimentWaffleFlag handles logic around experimental bucketing and whitelisting.
+
+    You'll have one main flag that gates the experiment. This allows you to control the scope
+    of your experiment and always provides a quick kill switch.
+
+    But you'll also have smaller related flags that can force bucketing certain users into
+    specific buckets of your experiment. Those can be set using a waffle named like
+    "main_flag.BUCKET_NUM" (e.g. "course_experience.animated_exy.0") to force
+    users that pass the first main waffle check into a specific bucket experience.
+
+    If you pass this flag a course key, tracking calls to segment will be made per-course-run
+    (rather than one call overall) and will include the course key.
+
+    You can also control whether the experiment only affects future enrollments by setting
+    an ExperimentKeyValue model object with a key of 'enrollment_start' to the date of the
+    first enrollments that should be bucketed.
+
+    Bucket 0 is assumed to be the control bucket.
+
+    See a HOWTO here: https://openedx.atlassian.net/wiki/spaces/AC/pages/1250623700/Bucketing+users+for+an+experiment
+    """
+    def __init__(self, waffle_namespace, flag_name, num_buckets=2, experiment_id=None, **kwargs):
+        super().__init__(waffle_namespace, flag_name, **kwargs)
+        self.num_buckets = num_buckets
+        self.experiment_id = experiment_id
+        self.bucket_flags = [
+            CourseWaffleFlag(waffle_namespace, '{}.{}'.format(flag_name, bucket), flag_undefined_default=False)
+            for bucket in range(num_buckets)
+        ]
+
+    def _cache_bucket(self, key, value):
+        request_cache = RequestCache('experiments')
+        request_cache.set(key, value)
+        return value
+
+    def get_bucket(self, course_key=None, track=True):
+        """
+        Return which bucket number the specified user is in.
+
+        Bucket 0 is assumed to be the control bucket and will be returned if the experiment is not enabled for
+        this user and course.
+        """
+        # Keep some imports in here, because this class is commonly used at a module level, and we want to avoid
+        # circular imports for any models.
+        from experiments.models import ExperimentKeyValue
+        from student.models import CourseEnrollment
+
+        request = get_current_request()
+        if not request:
+            return 0
+
+        # Use course key in experiment name to separate caches and segment calls per-course-run
+        experiment_name = self.namespaced_flag_name + ('.{}'.format(course_key) if course_key else '')
+
+        # Check if we have a cache for this request already
+        request_cache = RequestCache('experiments')
+        cache_response = request_cache.get_cached_response(experiment_name)
+        if cache_response.is_found:
+            return cache_response.value
+
+        # Check if the main flag is even enabled for this user and course.
+        if not self._is_enabled(course_key):  # grabs user from the current request, if any
+            return self._cache_bucket(experiment_name, 0)
+
+        # Check if the enrollment should even be considered (if it started before the experiment wants, we ignore)
+        if course_key and self.experiment_id is not None:
+            start_val = ExperimentKeyValue.objects.filter(experiment_id=self.experiment_id, key='enrollment_start')
+            if start_val:
+                try:
+                    start_date = dateutil.parser.parse(start_val.first().value).replace(tzinfo=pytz.UTC)
+                except ValueError:
+                    log.exception('Could not parse enrollment start date for experiment %d', self.experiment_id)
+                    return self._cache_bucket(experiment_name, 0)
+                enrollment = CourseEnrollment.get_enrollment(request.user, course_key)
+                # Only bail if they have an enrollment and it's old -- if they don't have an enrollment, we want to do
+                # normal bucketing -- consider the case where the experiment has bits that show before you enroll. We
+                # want to keep your bucketing stable before and after you do enroll.
+                if enrollment and enrollment.created < start_date:
+                    return self._cache_bucket(experiment_name, 0)
+
+        bucket = stable_bucketing_hash_group(experiment_name, self.num_buckets, request.user.username)
+
+        # Now check if the user is forced into a particular bucket, using our subordinate bucket flags
+        for i, bucket_flag in enumerate(self.bucket_flags):
+            if bucket_flag.is_enabled(course_key):
+                bucket = i
+                break
+
+        session_key = 'tracked.{}'.format(experiment_name)
+        if track and hasattr(request, 'session') and session_key not in request.session:
+            segment.track(
+                user_id=request.user.id,
+                event_name='edx.bi.experiment.user.bucketed',
+                properties={
+                    'site': request.site.domain,
+                    'app_label': self.waffle_namespace.name,
+                    'experiment': self.flag_name,
+                    'course_id': str(course_key) if course_key else None,
+                    'bucket': bucket,
+                    'is_staff': request.user.is_staff,
+                }
+            )
+
+            # Mark that we've recorded this bucketing, so that we don't do it again this session
+            request.session[session_key] = True
+
+        return self._cache_bucket(experiment_name, bucket)
+
+    def is_enabled(self, course_key=None):
+        return self.get_bucket(course_key) != 0
+
+    def is_enabled_without_course_context(self):
+        return self.get_bucket() != 0

lms/djangoapps/experiments/migrations/0004_historicalexperimentkeyvalue.py

@@ -0,0 +1,43 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.28 on 2020-03-03 16:26
+from __future__ import unicode_literals
+
+from django.conf import settings
+from django.db import migrations, models
+import django.db.models.deletion
+import django.utils.timezone
+import model_utils.fields
+import simple_history.models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ('experiments', '0003_auto_20170713_1148'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='HistoricalExperimentKeyValue',
+            fields=[
+                ('id', models.IntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')),
+                ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, editable=False, verbose_name='created')),
+                ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')),
+                ('experiment_id', models.PositiveSmallIntegerField(db_index=True, verbose_name='Experiment ID')),
+                ('key', models.CharField(max_length=255)),
+                ('value', models.TextField()),
+                ('history_id', models.AutoField(primary_key=True, serialize=False)),
+                ('history_date', models.DateTimeField()),
+                ('history_change_reason', models.CharField(max_length=100, null=True)),
+                ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)),
+                ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)),
+            ],
+            options={
+                'get_latest_by': 'history_date',
+                'verbose_name': 'historical Experiment Key-Value Pair',
+                'ordering': ('-history_date', '-history_id'),
+            },
+            bases=(simple_history.models.HistoricalChanges, models.Model),
+        ),
+    ]

lms/djangoapps/experiments/models.py

@@ -6,6 +6,7 @@ Experimentation models
 from django.conf import settings
 from django.db import models
 from model_utils.models import TimeStampedModel
+from simple_history.models import HistoricalRecords
 
 
 class ExperimentData(TimeStampedModel):
@@ -44,6 +45,8 @@ class ExperimentKeyValue(TimeStampedModel):
     key = models.CharField(null=False, blank=False, max_length=255)
     value = models.TextField()
 
+    history = HistoricalRecords()
+
     class Meta(object):
         verbose_name = 'Experiment Key-Value Pair'
         verbose_name_plural = 'Experiment Key-Value Pairs'

lms/djangoapps/experiments/tests/test_flags.py

@@ -0,0 +1,122 @@
+"""
+Tests for experimentation feature flags
+"""
+
+import pytz
+
+import ddt
+from crum import set_current_request
+from dateutil import parser
+from django.test.client import RequestFactory
+from edx_django_utils.cache import RequestCache
+from mock import patch
+from opaque_keys.edx.keys import CourseKey
+
+from experiments.factories import ExperimentKeyValueFactory
+from experiments.flags import ExperimentWaffleFlag
+from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory
+from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag
+from student.tests.factories import CourseEnrollmentFactory, UserFactory
+from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
+
+
+@ddt.ddt
+class ExperimentWaffleFlagTests(SharedModuleStoreTestCase):
+    """ Tests for the ExperimentWaffleFlag class """
+    def setUp(self):
+        super().setUp()
+
+        self.user = UserFactory()
+
+        self.request = RequestFactory().request()
+        self.request.session = {}
+        self.request.site = SiteFactory()
+        self.request.user = self.user
+        self.addCleanup(set_current_request, None)
+        set_current_request(self.request)
+
+        self.flag = ExperimentWaffleFlag('experiments', 'test', num_buckets=2, experiment_id=0)
+        self.key = CourseKey.from_string('a/b/c')
+
+        bucket_patch = patch('experiments.flags.stable_bucketing_hash_group', return_value=1)
+        self.addCleanup(bucket_patch.stop)
+        bucket_patch.start()
+
+        self.addCleanup(RequestCache.clear_all_namespaces)
+
+    def get_bucket(self, track=False, active=True):
+        with self.flag.override(active=active):
+            return self.flag.get_bucket(course_key=self.key, track=track)
+
+    def test_basic_happy_path(self):
+        self.assertEqual(self.get_bucket(), 1)
+
+    def test_no_request(self):
+        set_current_request(None)
+        self.assertEqual(self.get_bucket(), 0)
+
+    def test_not_enabled(self):
+        self.assertEqual(self.get_bucket(active=False), 0)
+
+    @ddt.data(
+        ('2012-01-06', None, 1),  # no enrollment (we allow normal bucketing in this case)
+        ('2012-01-06', '2012-01-05', 0),  # enrolled before experiment
+        ('2012-01-06', '2012-01-07', 1),  # enrolled after experiment
+        (None, '2012-01-07', 1),  # no experiment date
+        ('not-a-date', '2012-01-07', 0),  # bad experiment date
+    )
+    @ddt.unpack
+    def test_enrollment_start(self, experiment_start, enrollment_created, expected_bucket):
+        if enrollment_created:
+            enrollment = CourseEnrollmentFactory(user=self.user, course_id='a/b/c')
+            enrollment.created = parser.parse(enrollment_created).replace(tzinfo=pytz.UTC)
+            enrollment.save()
+        if experiment_start:
+            ExperimentKeyValueFactory(experiment_id=0, key='enrollment_start', value=experiment_start)
+        self.assertEqual(self.get_bucket(), expected_bucket)
+
+    @ddt.data(
+        (True, 0),
+        (False, 1),
+    )
+    @ddt.unpack
+    def test_bucket_override(self, active, expected_bucket):
+        bucket_flag = CourseWaffleFlag('experiments', 'test.0')
+        with bucket_flag.override(active=active):
+            self.assertEqual(self.get_bucket(), expected_bucket)
+
+    def test_tracking(self):
+        # Run twice, with same request
+        with patch('experiments.flags.segment') as segment_mock:
+            self.assertEqual(self.get_bucket(track=True), 1)
+            RequestCache.clear_all_namespaces()  # we want to force get_bucket to check session, not early exit
+            self.assertEqual(self.get_bucket(track=True), 1)
+
+        # Now test that we only sent the signal once, and with the correct properties
+        self.assertEqual(segment_mock.track.call_count, 1)
+        self.assertEqual(segment_mock.track.call_args, ((), {
+            'user_id': self.user.id,
+            'event_name': 'edx.bi.experiment.user.bucketed',
+            'properties': {
+                'site': self.request.site.domain,
+                'app_label': 'experiments',
+                'experiment': 'test',
+                'bucket': 1,
+                'course_id': 'a/b/c',
+                'is_staff': self.user.is_staff,
+            },
+        }))
+
+    def test_caching(self):
+        self.assertEqual(self.get_bucket(active=True), 1)
+        self.assertEqual(self.get_bucket(active=False), 1)  # still returns 1!
+
+    def test_is_enabled(self):
+        with patch('experiments.flags.ExperimentWaffleFlag.get_bucket', return_value=1):
+            self.assertEqual(self.flag.is_enabled_without_course_context(), True)
+            self.assertEqual(self.flag.is_enabled(self.key), True)
+            self.assertEqual(self.flag.is_enabled(), True)
+        with patch('experiments.flags.ExperimentWaffleFlag.get_bucket', return_value=0):
+            self.assertEqual(self.flag.is_enabled_without_course_context(), False)
+            self.assertEqual(self.flag.is_enabled(self.key), False)
+            self.assertEqual(self.flag.is_enabled(), False)

lms/djangoapps/grades/tests/test_tasks.py

@@ -164,10 +164,10 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest
             self.assertEqual(mock_block_structure_create.call_count, 1)
 
     @ddt.data(
-        (ModuleStoreEnum.Type.mongo, 1, 38, True),
-        (ModuleStoreEnum.Type.mongo, 1, 38, False),
-        (ModuleStoreEnum.Type.split, 3, 38, True),
-        (ModuleStoreEnum.Type.split, 3, 38, False),
+        (ModuleStoreEnum.Type.mongo, 1, 37, True),
+        (ModuleStoreEnum.Type.mongo, 1, 37, False),
+        (ModuleStoreEnum.Type.split, 3, 37, True),
+        (ModuleStoreEnum.Type.split, 3, 37, False),
     )
     @ddt.unpack
     def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections):
@@ -179,8 +179,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest
                     self._apply_recalculate_subsection_grade()
 
     @ddt.data(
-        (ModuleStoreEnum.Type.mongo, 1, 38),
-        (ModuleStoreEnum.Type.split, 3, 38),
+        (ModuleStoreEnum.Type.mongo, 1, 37),
+        (ModuleStoreEnum.Type.split, 3, 37),
     )
     @ddt.unpack
     def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls):
@@ -225,8 +225,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest
         )
 
     @ddt.data(
-        (ModuleStoreEnum.Type.mongo, 1, 21),
-        (ModuleStoreEnum.Type.split, 3, 21),
+        (ModuleStoreEnum.Type.mongo, 1, 20),
+        (ModuleStoreEnum.Type.split, 3, 20),
     )
     @ddt.unpack
     def test_persistent_grades_not_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries):
@@ -240,8 +240,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest
             self.assertEqual(len(PersistentSubsectionGrade.bulk_read_grades(self.user.id, self.course.id)), 0)
 
     @ddt.data(
-        (ModuleStoreEnum.Type.mongo, 1, 39),
-        (ModuleStoreEnum.Type.split, 3, 39),
+        (ModuleStoreEnum.Type.mongo, 1, 38),
+        (ModuleStoreEnum.Type.split, 3, 38),
     )
     @ddt.unpack
     def test_persistent_grades_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries):

openedx/features/course_experience/__init__.py

@@ -5,6 +5,7 @@ Unified course experience settings and helper methods.
 
 from django.utils.translation import ugettext as _
 
+from lms.djangoapps.experiments.flags import ExperimentWaffleFlag
 from openedx.core.djangoapps.util.user_messages import UserMessageCollection
 from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag, WaffleFlag, WaffleFlagNamespace
 
@@ -79,7 +80,7 @@ SEO_WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace(name='seo')
 COURSE_ENABLE_UNENROLLED_ACCESS_FLAG = CourseWaffleFlag(SEO_WAFFLE_FLAG_NAMESPACE, 'enable_anonymous_courseware_access')
 
 # Waffle flag to enable relative dates for course content
-RELATIVE_DATES_FLAG = CourseWaffleFlag(WAFFLE_FLAG_NAMESPACE, 'relative_dates')
+RELATIVE_DATES_FLAG = ExperimentWaffleFlag(WAFFLE_FLAG_NAMESPACE, 'relative_dates', experiment_id=17)
 
 # Waffle flag to enable showing FBE messaging, assignment due dates, and modified
 # end date logic (for self-paced courses) in the date widget

openedx/features/course_experience/tests/views/test_course_updates.py

@@ -134,7 +134,7 @@ class TestCourseUpdatesPage(SharedModuleStoreTestCase):
 
         # Fetch the view and verify that the query counts haven't changed
         # TODO: decrease query count as part of REVO-28
-        with self.assertNumQueries(50, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST):
+        with self.assertNumQueries(52, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST):
             with check_mongo_calls(4):
                 url = course_updates_url(self.course)
                 self.client.get(url)