diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index 33cf1f5309..9e12e02d29 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/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): diff --git a/lms/djangoapps/course_api/blocks/tests/test_api.py b/lms/djangoapps/course_api/blocks/tests/test_api.py index 46ccca1506..b7694edfc7 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_api.py +++ b/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, diff --git a/lms/djangoapps/courseware/tabs.py b/lms/djangoapps/courseware/tabs.py index 2fc36c3345..e3ef9eebeb 100644 --- a/lms/djangoapps/courseware/tabs.py +++ b/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): diff --git a/lms/djangoapps/courseware/tests/test_course_info.py b/lms/djangoapps/courseware/tests/test_course_info.py index 578cd8b7dc..e19a422cf8 100644 --- a/lms/djangoapps/courseware/tests/test_course_info.py +++ b/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) diff --git a/lms/djangoapps/experiments/flags.py b/lms/djangoapps/experiments/flags.py new file mode 100644 index 0000000000..3d26e5cafa --- /dev/null +++ b/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 diff --git a/lms/djangoapps/experiments/migrations/0004_historicalexperimentkeyvalue.py b/lms/djangoapps/experiments/migrations/0004_historicalexperimentkeyvalue.py new file mode 100644 index 0000000000..bc7f59b782 --- /dev/null +++ b/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), + ), + ] diff --git a/lms/djangoapps/experiments/models.py b/lms/djangoapps/experiments/models.py index 6228362f14..503d6f8e71 100644 --- a/lms/djangoapps/experiments/models.py +++ b/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' diff --git a/lms/djangoapps/experiments/tests/test_flags.py b/lms/djangoapps/experiments/tests/test_flags.py new file mode 100644 index 0000000000..a45d9c6877 --- /dev/null +++ b/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) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index f088a19d75..316be35a76 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/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): diff --git a/openedx/features/course_experience/__init__.py b/openedx/features/course_experience/__init__.py index 034541e53e..4e48a961ff 100644 --- a/openedx/features/course_experience/__init__.py +++ b/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 diff --git a/openedx/features/course_experience/tests/views/test_course_updates.py b/openedx/features/course_experience/tests/views/test_course_updates.py index 69ad5b78d1..bc29fc687d 100644 --- a/openedx/features/course_experience/tests/views/test_course_updates.py +++ b/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)