From 7e8566b4d81366d1b249d413b928ed7c02ca9636 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Fri, 14 Jul 2017 16:09:55 -0400 Subject: [PATCH] Fix course override caching bug. LEARNER-1896 --- .../core/djangoapps/waffle_utils/__init__.py | 35 +++++++++++++------ .../waffle_utils/tests/test_init.py | 11 ++++++ 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/waffle_utils/__init__.py b/openedx/core/djangoapps/waffle_utils/__init__.py index bcae90833c..23c4e74088 100644 --- a/openedx/core/djangoapps/waffle_utils/__init__.py +++ b/openedx/core/djangoapps/waffle_utils/__init__.py @@ -193,24 +193,29 @@ class WaffleFlagNamespace(WaffleNamespace): If check_before_waffle_callback returns None, or if it is not supplied, then waffle is used to check the flag. + Important: Caching for the check_before_waffle_callback must be handled + by the callback itself. + Arguments: flag_name (String): The name of the flag to check. check_before_waffle_callback (function): (Optional) A function that will be checked before continuing on to waffle. If check_before_waffle_callback(namespaced_flag_name) returns True - or False, it is cached and returned. If it returns None, then - waffle is used. + or False, it is returned. If it returns None, then waffle is + used. flag_undefined_default (Boolean): A default value to be returned if the waffle flag is to be checked, but doesn't exist. """ # validate arguments namespaced_flag_name = self._namespaced_name(flag_name) - value = self._cached_flags.get(namespaced_flag_name) + + if check_before_waffle_callback: + value = check_before_waffle_callback(namespaced_flag_name) if value is None: - if check_before_waffle_callback: - value = check_before_waffle_callback(namespaced_flag_name) - + # Do not get cached value for the callback, because the key might be different. + # The callback needs to handle its own caching if it wants it. + value = self._cached_flags.get(namespaced_flag_name) if value is None: if flag_undefined_default is not None: @@ -223,7 +228,7 @@ class WaffleFlagNamespace(WaffleNamespace): if value is None: value = flag_is_active(get_request(), namespaced_flag_name) - self._cached_flags[namespaced_flag_name] = value + self._cached_flags[namespaced_flag_name] = value return value @@ -264,12 +269,12 @@ class CourseWaffleFlag(WaffleFlag): Uses a cached waffle namespace. """ - def _get_course_override_callback(self, course_id): + def _get_course_override_callback(self, course_key): """ Returns a function to use as the check_before_waffle_callback. Arguments: - course_id (CourseKey): The course to check for override before + course_key (CourseKey): The course to check for override before checking waffle. """ def course_override_callback(namespaced_flag_name): @@ -277,17 +282,25 @@ class CourseWaffleFlag(WaffleFlag): Returns True/False if the flag was forced on or off for the provided course. Returns None if the flag was not overridden. + Note: Has side effect of caching the override value. + Arguments: namespaced_flag_name (String): A namespaced version of the flag to check. """ - force_override = WaffleFlagCourseOverrideModel.override_value(namespaced_flag_name, course_id) + cache_key = u'{}.{}'.format(namespaced_flag_name, unicode(course_key)) + force_override = self.waffle_namespace._cached_flags.get(cache_key) + + if force_override is None: + force_override = WaffleFlagCourseOverrideModel.override_value(namespaced_flag_name, course_key) + self.waffle_namespace._cached_flags[cache_key] = force_override if force_override == WaffleFlagCourseOverrideModel.ALL_CHOICES.on: return True if force_override == WaffleFlagCourseOverrideModel.ALL_CHOICES.off: return False return None + return course_override_callback def is_enabled(self, course_key=None): @@ -299,7 +312,7 @@ class CourseWaffleFlag(WaffleFlag): checking waffle. """ # validate arguments - assert issubclass(type(course_key), CourseKey), "The course_id '{}' must be a CourseKey.".format( + assert issubclass(type(course_key), CourseKey), "The course_key '{}' must be a CourseKey.".format( str(course_key) ) diff --git a/openedx/core/djangoapps/waffle_utils/tests/test_init.py b/openedx/core/djangoapps/waffle_utils/tests/test_init.py index 2d83eaed6e..2732b83611 100644 --- a/openedx/core/djangoapps/waffle_utils/tests/test_init.py +++ b/openedx/core/djangoapps/waffle_utils/tests/test_init.py @@ -23,6 +23,7 @@ class TestCourseWaffleFlag(TestCase): NAMESPACED_FLAG_NAME = NAMESPACE_NAME + "." + FLAG_NAME TEST_COURSE_KEY = CourseKey.from_string("edX/DemoX/Demo_Course") + TEST_COURSE_2_KEY = CourseKey.from_string("edX/DemoX/Demo_Course_2") TEST_NAMESPACE = WaffleFlagNamespace(NAMESPACE_NAME) TEST_COURSE_FLAG = CourseWaffleFlag(TEST_NAMESPACE, FLAG_NAME) @@ -50,6 +51,16 @@ class TestCourseWaffleFlag(TestCase): self.TEST_COURSE_KEY ) + # check flag for a second course + if data['course_override'] == WaffleFlagCourseOverrideModel.ALL_CHOICES.unset: + # When course override wasn't set for the first course, the second course will get the same + # cached value from waffle. + self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_2_KEY), data['waffle_enabled']) + else: + # When course override was set for the first course, it should not apply to the second + # course which should get the default value of False. + self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_2_KEY), False) + @ddt.data( {'flag_undefined_default': None, 'result': False}, {'flag_undefined_default': False, 'result': False},