Merge pull request #15577 from edx/robrap/LEARNER-1896-fix-waffle-flag-cache

Fix course override caching bug.

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 value is None:
-            if check_before_waffle_callback:
-                value = check_before_waffle_callback(namespaced_flag_name)
+        if check_before_waffle_callback:
+            value = check_before_waffle_callback(namespaced_flag_name)
 
+        if value is None:
+            # 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
 
 
@@ -271,12 +276,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):
@@ -284,17 +289,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):
@@ -306,7 +319,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)
         )
 

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},