diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 3eab8bdc42..27c1452a66 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -3268,7 +3268,7 @@ class TestShowCoursewareMFE(TestCase): ) for user, course_key, is_course_staff, preview_active, redirect_active in combos: with override_waffle_flag(COURSEWARE_MICROFRONTEND_COURSE_TEAM_PREVIEW, preview_active): - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, redirect_active): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=redirect_active): assert show_courseware_mfe_link(user, is_course_staff, course_key) is False @patch.dict(settings.FEATURES, {'ENABLE_COURSEWARE_MICROFRONTEND': True}) @@ -3288,14 +3288,14 @@ class TestShowCoursewareMFE(TestCase): ) for user, is_course_staff, preview_active, redirect_active in old_mongo_combos: with override_waffle_flag(COURSEWARE_MICROFRONTEND_COURSE_TEAM_PREVIEW, preview_active): - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, redirect_active): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=redirect_active): assert show_courseware_mfe_link(user, is_course_staff, old_course_key) is False # We've checked all old-style course keys now, so we can test only the # new ones going forward. Now we check combinations of waffle flags and # user permissions... with override_waffle_flag(COURSEWARE_MICROFRONTEND_COURSE_TEAM_PREVIEW, True): - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, True): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=True): # (preview=on, redirect=on) # Global and Course Staff can see the link. self.assertTrue(show_courseware_mfe_link(global_staff_user, True, new_course_key)) @@ -3304,7 +3304,7 @@ class TestShowCoursewareMFE(TestCase): # Regular users don't see the link. self.assertFalse(show_courseware_mfe_link(regular_user, False, new_course_key)) - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, False): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=False): # (preview=on, redirect=off) # Global and Course Staff can see the link. self.assertTrue(show_courseware_mfe_link(global_staff_user, True, new_course_key)) @@ -3315,7 +3315,7 @@ class TestShowCoursewareMFE(TestCase): self.assertFalse(show_courseware_mfe_link(regular_user, False, new_course_key)) with override_waffle_flag(COURSEWARE_MICROFRONTEND_COURSE_TEAM_PREVIEW, False): - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, True): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=True): # (preview=off, redirect=on) # Global staff see the link anyway self.assertTrue(show_courseware_mfe_link(global_staff_user, True, new_course_key)) @@ -3327,7 +3327,7 @@ class TestShowCoursewareMFE(TestCase): # Regular users don't see the link. self.assertFalse(show_courseware_mfe_link(regular_user, False, new_course_key)) - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, False): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=False): # (preview=off, redirect=off) # Global staff see the link anyway self.assertTrue(show_courseware_mfe_link(global_staff_user, True, new_course_key)) @@ -3360,8 +3360,7 @@ class TestShowCoursewareMFE(TestCase): '/block-v1:OpenEdX+MFE+2020+type@vertical+block@Getting_To_Know_You' ) -# TODO: TNL-7157 Re-enable these tests before Courseware MFE Canary -@unittest.skip + @patch.dict('django.conf.settings.FEATURES', {'ENABLE_COURSEWARE_MICROFRONTEND': True}) @ddt.ddt class MFERedirectTests(BaseViewsTestCase): @@ -3387,7 +3386,7 @@ class MFERedirectTests(BaseViewsTestCase): # learners will be redirected when the waffle flag is set lms_url, mfe_url = self._get_urls() - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, True): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=True): assert self.client.get(lms_url).url == mfe_url def test_staff_no_redirect(self): @@ -3399,14 +3398,14 @@ class MFERedirectTests(BaseViewsTestCase): self.client.login(username=course_staff.username, password='test') assert self.client.get(lms_url).status_code == 200 - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, True): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=True): assert self.client.get(lms_url).status_code == 200 # global staff will never be redirected self._create_global_staff_user() assert self.client.get(lms_url).status_code == 200 - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, True): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=True): assert self.client.get(lms_url).status_code == 200 def test_exam_no_redirect(self): @@ -3416,5 +3415,5 @@ class MFERedirectTests(BaseViewsTestCase): lms_url, mfe_url = self._get_urls() - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, True): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=True): assert self.client.get(lms_url).status_code == 200 diff --git a/lms/djangoapps/experiments/flags.py b/lms/djangoapps/experiments/flags.py index 3c274e3566..254cb81bef 100644 --- a/lms/djangoapps/experiments/flags.py +++ b/lms/djangoapps/experiments/flags.py @@ -40,6 +40,21 @@ class ExperimentWaffleFlag(CourseWaffleFlag): 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 + + When writing tests involving an ExperimentWaffleFlag you must not use the + override_waffle_flag utility. That will only turn the experiment on or off and won't + override bucketing. Instead use ExperimentWaffleFlag's override method which + will do both. Example: + + with MY_EXPERIMENT_WAFFLE_FLAG.override(active=True, bucket=1): + ... + + or as a decorator: + + @MY_EXPERIMENT_WAFFLE_FLAG.override(active=True, bucket=1) + def test_my_experiment(self): + ... + """ def __init__(self, waffle_namespace, flag_name, num_buckets=2, experiment_id=None, **kwargs): super().__init__(waffle_namespace, flag_name, **kwargs) @@ -123,7 +138,7 @@ class ExperimentWaffleFlag(CourseWaffleFlag): 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 + if not self.is_experiment_on(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) @@ -170,12 +185,19 @@ class ExperimentWaffleFlag(CourseWaffleFlag): return self.is_enabled() def is_experiment_on(self, course_key=None): + # If no course_key is supplied check the global flag irrespective of courses + if course_key is None: + return super().is_enabled_without_course_context() + return super().is_enabled(course_key) @contextmanager def override(self, active=True, bucket=1): # pylint: disable=arguments-differ - from mock import patch - if not active: - bucket = 0 - with patch.object(self, 'get_bucket', return_value=bucket): - yield + # Let CourseWaffleFlag override the base waffle flag value + with super().override(active=active): + # Now override the experiment bucket value + from mock import patch + if not active: + bucket = 0 + with patch.object(self, 'get_bucket', return_value=bucket): + yield diff --git a/lms/djangoapps/experiments/tests/test_flags.py b/lms/djangoapps/experiments/tests/test_flags.py index c4fa568200..3f1328df87 100644 --- a/lms/djangoapps/experiments/tests/test_flags.py +++ b/lms/djangoapps/experiments/tests/test_flags.py @@ -102,7 +102,7 @@ class ExperimentWaffleFlagTests(SharedModuleStoreTestCase): (False, 1), ) @ddt.unpack - def test_bucket_override(self, active, expected_bucket): + def test_forcing_bucket(self, active, expected_bucket): bucket_flag = CourseWaffleFlag('experiments', 'test.0') with bucket_flag.override(active=active): self.assertEqual(self.get_bucket(), expected_bucket) @@ -143,3 +143,16 @@ class ExperimentWaffleFlagTests(SharedModuleStoreTestCase): 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) + + @ddt.data( + (True, 1, 1), + (True, 0, 0), + (False, 1, 0), # bucket is always 0 if the experiment is off + (False, 0, 0), + ) + @ddt.unpack + # Test the override method + def test_override_method(self, active, bucket_override, expected_bucket): + with self.flag.override(active=active, bucket=bucket_override): + self.assertEqual(self.flag.get_bucket(), expected_bucket) + self.assertEqual(self.flag.is_experiment_on(), active)