From 14cb7b605642e6618ddde01c8dcb4ed23de90030 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Mon, 5 Oct 2020 09:57:25 +0200 Subject: [PATCH 01/10] Add a make target to compile requirements (without upgrade) I grew tired of modifying the Makefile to compile requirements without upgrading them. Also, installing pip-tools should not be part of the compile-requirements target, so a separate target was created. --- Makefile | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index 49e63dca32..32b078d402 100644 --- a/Makefile +++ b/Makefile @@ -63,8 +63,10 @@ detect_changed_source_translations: ## check if translation files are up-to-date pull: ## update the Docker image used by "make shell" docker pull edxops/edxapp:latest -requirements: ## install development environment requirements +pre-requirements: ## install Python requirements for running pip-tools pip install -qr requirements/edx/pip-tools.txt + +requirements: pre-requirements ## install development environment requirements pip-sync -q requirements/edx/development.txt requirements/edx/private.* shell: ## launch a bash shell in a Docker container with all edx-platform dependencies installed @@ -87,15 +89,14 @@ REQ_FILES = \ requirements/edx/development \ scripts/xblock/requirements -upgrade: export CUSTOM_COMPILE_COMMAND=make upgrade -upgrade: ## update the pip requirements files to use the latest releases satisfying our constraints - pip install -qr requirements/edx/pip-tools.txt +compile-requirements: export CUSTOM_COMPILE_COMMAND=make upgrade +compile-requirements: ## Re-compile *.in requirements to *.txt @ export REBUILD='--rebuild'; \ for f in $(REQ_FILES); do \ echo ; \ echo "== $$f ===============================" ; \ - echo "pip-compile -v --no-emit-trusted-host --no-index $$REBUILD --upgrade -o $$f.txt $$f.in"; \ - pip-compile -v --no-emit-trusted-host --no-index $$REBUILD --upgrade -o $$f.txt $$f.in || exit 1; \ + echo "pip-compile -v --no-emit-trusted-host --no-index $$REBUILD ${COMPILE_OPTS} -o $$f.txt $$f.in"; \ + pip-compile -v --no-emit-trusted-host --no-index $$REBUILD ${COMPILE_OPTS} -o $$f.txt $$f.in || exit 1; \ export REBUILD=''; \ done # Post process all of the files generated above to work around open pip-tools issues @@ -105,6 +106,9 @@ upgrade: ## update the pip requirements files to use the latest releases satisfy sed '/^[dD]jango==/d' requirements/edx/testing.txt > requirements/edx/testing.tmp mv requirements/edx/testing.tmp requirements/edx/testing.txt +upgrade: pre-requirements ## update the pip requirements files to use the latest releases satisfying our constraints + $(MAKE) compile-requirements COMPILE_OPTS="--upgrade" + # These make targets currently only build LMS images. docker_build: docker build . -f Dockerfile --target lms -t openedx/edx-platform From 24cf0543f3ce78122e5c7ccddffa0d4a58efc9d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Mon, 5 Oct 2020 13:08:45 +0200 Subject: [PATCH 02/10] [BD-21] Migrate WaffleSwitch to edx-toggles This makes this class reusable by other IDAs. --- .../core/djangoapps/waffle_utils/__init__.py | 205 ++++-------------- .../waffle_utils/tests/test_init.py | 21 -- 2 files changed, 42 insertions(+), 184 deletions(-) diff --git a/openedx/core/djangoapps/waffle_utils/__init__.py b/openedx/core/djangoapps/waffle_utils/__init__.py index 8bbcbb263a..140fa8a074 100644 --- a/openedx/core/djangoapps/waffle_utils/__init__.py +++ b/openedx/core/djangoapps/waffle_utils/__init__.py @@ -69,174 +69,36 @@ from waffle import flag_is_active, switch_is_active from openedx.core.lib.cache_utils import get_cache as get_request_cache +from edx_toggles.toggles import BaseNamespace +from edx_toggles.toggles import WaffleSwitch as BaseWaffleSwitch +from edx_toggles.toggles import WaffleSwitchNamespace as BaseWaffleSwitchNamespace + log = logging.getLogger(__name__) -class WaffleNamespace(six.with_metaclass(ABCMeta, object)): - """ - A base class for a request cached namespace for waffle flags/switches. - - An instance of this class represents a single namespace - (e.g. "course_experience"), and can be used to work with a set of - flags or switches that will all share this namespace. - """ - - def __init__(self, name, log_prefix=None): - """ - Initializes the waffle namespace instance. - - Arguments: - name (String): Namespace string appended to start of all waffle - flags and switches (e.g. "grades") - log_prefix (String): Optional string to be appended to log messages - (e.g. "Grades: "). Defaults to ''. - - """ - assert name, "The name is required." - self.name = name - self.log_prefix = log_prefix if log_prefix else '' - - def _namespaced_name(self, setting_name): - """ - Returns the namespaced name of the waffle switch/flag. - - For example, the namespaced name of a waffle switch/flag would be: - my_namespace.my_setting_name - - Arguments: - setting_name (String): The name of the flag or switch. - """ - return u'{}.{}'.format(self.name, setting_name) - +class RequestCacheMixin: @staticmethod def _get_request_cache(): """ Returns a request cache shared by all instances of this class. """ - return get_request_cache('WaffleNamespace') + return get_request_cache("WaffleNamespace") -class WaffleSwitchNamespace(WaffleNamespace): - """ - Provides a single namespace for a set of waffle switches. - - All namespaced switch values are stored in a single request cache containing - all switches for all namespaces. - """ - def is_enabled(self, switch_name): - """ - Returns and caches whether the given waffle switch is enabled. - """ - namespaced_switch_name = self._namespaced_name(switch_name) - value = self._cached_switches.get(namespaced_switch_name) - if value is None: - value = switch_is_active(namespaced_switch_name) - self._cached_switches[namespaced_switch_name] = value - return value - - @contextmanager - def override(self, switch_name, active=True): - """ - Overrides the active value for the given switch for the duration of this - contextmanager. - Note: The value is overridden in the request cache AND in the model. - """ - previous_active = self.is_enabled(switch_name) - try: - self.override_for_request(switch_name, active) - with self.override_in_model(switch_name, active): - yield - finally: - self.override_for_request(switch_name, previous_active) - - def override_for_request(self, switch_name, active=True): - """ - Overrides the active value for the given switch for the remainder of - this request (as this is not a context manager). - Note: The value is overridden in the request cache, not in the model. - """ - namespaced_switch_name = self._namespaced_name(switch_name) - self._cached_switches[namespaced_switch_name] = active - log.info(u"%sSwitch '%s' set to %s for request.", self.log_prefix, namespaced_switch_name, active) - - @contextmanager - def override_in_model(self, switch_name, active=True): - """ - Overrides the active value for the given switch for the duration of this - contextmanager. - Note: The value is overridden in the model, not the request cache. - """ - # Import is placed here to avoid model import at project startup. - from waffle.testutils import override_switch as waffle_override_switch - namespaced_switch_name = self._namespaced_name(switch_name) - with waffle_override_switch(namespaced_switch_name, active): - log.info(u"%sSwitch '%s' set to %s in model.", self.log_prefix, namespaced_switch_name, active) - yield - +class WaffleSwitchNamespace(BaseWaffleSwitchNamespace, RequestCacheMixin): @property def _cached_switches(self): """ Returns a dictionary of all namespaced switches in the request cache. """ - return self._get_request_cache().setdefault('switches', {}) + return self._get_request_cache().setdefault("switches", {}) -class WaffleSwitch(object): - """ - Represents a single waffle switch, using a cached namespace. - """ - # use a WeakSet so these instances can be garbage collected if need be - _class_instances = WeakSet() - - def __init__(self, waffle_namespace, switch_name, module_name=None): - """ - Arguments: - waffle_namespace (WaffleSwitchNamespace | String): Namespace for this switch. - switch_name (String): The name of the switch (without namespacing). - module_name (String): The name of the module where the flag is created. This should be ``__name__`` in most - cases. - """ - if isinstance(waffle_namespace, six.string_types): - waffle_namespace = WaffleSwitchNamespace(name=waffle_namespace) - - self.waffle_namespace = waffle_namespace - self.switch_name = switch_name - self._module_name = module_name or "" - self._class_instances.add(self) - - @classmethod - def get_instances(cls): - """ Returns a WeakSet of the instantiated instances of WaffleFlag. """ - return cls._class_instances - - @property - def module_name(self): - """ - Returns the module name. This is cached to work with the WeakSet instances. - """ - return self._module_name - - @module_name.setter - def module_name(self, value): - self._module_name = value - - @property - def namespaced_switch_name(self): - """ - Returns the fully namespaced switch name. - """ - return self.waffle_namespace._namespaced_name(self.switch_name) # pylint: disable=protected-access - - def is_enabled(self): - return self.waffle_namespace.is_enabled(self.switch_name) - - @contextmanager - def override(self, active=True): - with self.waffle_namespace.override(self.switch_name, active): - yield +class WaffleSwitch(BaseWaffleSwitch): + NAMESPACE_CLASS = WaffleSwitchNamespace -class WaffleFlagNamespace(six.with_metaclass(ABCMeta, WaffleNamespace)): +class WaffleFlagNamespace(BaseNamespace, RequestCacheMixin): """ Provides a single namespace for a set of waffle flags. @@ -249,7 +111,7 @@ class WaffleFlagNamespace(six.with_metaclass(ABCMeta, WaffleNamespace)): """ Returns a dictionary of all namespaced flags in the request cache. """ - return self._get_request_cache().setdefault('flags', {}) + return self._get_request_cache().setdefault("flags", {}) def is_flag_active(self, flag_name, check_before_waffle_callback=None): """ @@ -295,14 +157,18 @@ class WaffleFlagNamespace(six.with_metaclass(ABCMeta, WaffleNamespace)): request = crum.get_current_request() if not request: - log.warning(u"%sFlag '%s' accessed without a request", self.log_prefix, namespaced_flag_name) + log.warning( + u"%sFlag '%s' accessed without a request", + self.log_prefix, + namespaced_flag_name, + ) # Return the Flag's Everyone value if not in a request context. # Note: this skips the cache as the value might be different # in a normal request context. This case seems to occur when # a page redirects to a 404, or for celery workers. value = self._is_flag_active_for_everyone(namespaced_flag_name) self._set_waffle_flag_attribute(namespaced_flag_name, value) - set_custom_attribute('warn_flag_no_request_return_value', value) + set_custom_attribute("warn_flag_no_request_return_value", value) return value value = flag_is_active(request, namespaced_flag_name) @@ -321,7 +187,7 @@ class WaffleFlagNamespace(six.with_metaclass(ABCMeta, WaffleNamespace)): try: waffle_flag = Flag.objects.get(name=namespaced_flag_name) - return (waffle_flag.everyone is True) + return waffle_flag.everyone is True except Flag.DoesNotExist: return False @@ -355,18 +221,18 @@ class WaffleFlagNamespace(six.with_metaclass(ABCMeta, WaffleNamespace)): if name not in _WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET: return - flag_attribute_data = self._get_request_cache().setdefault('flag_attribute', {}) + flag_attribute_data = self._get_request_cache().setdefault("flag_attribute", {}) is_value_change = False if name not in flag_attribute_data: flag_attribute_data[name] = str(value) is_value_change = True else: if flag_attribute_data[name] != str(value): - flag_attribute_data[name] = 'Both' + flag_attribute_data[name] = "Both" is_value_change = True if is_value_change: - attribute_name = 'flag_{}'.format(name) + attribute_name = "flag_{}".format(name) set_custom_attribute(attribute_name, flag_attribute_data[name]) @@ -374,10 +240,15 @@ def _get_waffle_flag_custom_attributes_set(): """ Returns a set based on the Django setting WAFFLE_FLAG_CUSTOM_ATTRIBUTES (list). """ - waffle_flag_custom_attributes = getattr(settings, _WAFFLE_FLAG_CUSTOM_ATTRIBUTES, None) - waffle_flag_custom_attributes = waffle_flag_custom_attributes if waffle_flag_custom_attributes else [] + waffle_flag_custom_attributes = getattr( + settings, _WAFFLE_FLAG_CUSTOM_ATTRIBUTES, None + ) + waffle_flag_custom_attributes = ( + waffle_flag_custom_attributes if waffle_flag_custom_attributes else [] + ) return set(waffle_flag_custom_attributes) + # .. toggle_name: WAFFLE_FLAG_CUSTOM_ATTRIBUTES # .. toggle_implementation: DjangoSetting # .. toggle_default: False @@ -386,7 +257,7 @@ def _get_waffle_flag_custom_attributes_set(): # .. toggle_use_cases: opt_in # .. toggle_creation_date: 2020-06-17 # .. toggle_warnings: Intent is for temporary research (1 day - several weeks) of a flag's usage. -_WAFFLE_FLAG_CUSTOM_ATTRIBUTES = 'WAFFLE_FLAG_CUSTOM_ATTRIBUTES' +_WAFFLE_FLAG_CUSTOM_ATTRIBUTES = "WAFFLE_FLAG_CUSTOM_ATTRIBUTES" # set of waffle flags that should be instrumented with custom attributes _WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET = _get_waffle_flag_custom_attributes_set() @@ -396,6 +267,7 @@ class WaffleFlag(object): """ Represents a single waffle flag, using a cached waffle namespace. """ + # use a WeakSet so these instances can be garbage collected if need be _class_instances = WeakSet() @@ -452,6 +324,7 @@ class WaffleFlag(object): # TODO We can move this import to the top of the file once this code is # not all contained within the __init__ module. from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag + with override_waffle_flag(self, active): yield @@ -471,6 +344,7 @@ class CourseWaffleFlag(WaffleFlag): course_key (CourseKey): The course to check for override before checking waffle. """ + def course_override_callback(namespaced_flag_name): """ Returns True/False if the flag was forced on or off for the provided @@ -484,11 +358,14 @@ class CourseWaffleFlag(WaffleFlag): """ # Import is placed here to avoid model import at project startup. from .models import WaffleFlagCourseOverrideModel - cache_key = u'{}.{}'.format(namespaced_flag_name, six.text_type(course_key)) + + cache_key = u"{}.{}".format(namespaced_flag_name, six.text_type(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) + 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: @@ -509,8 +386,10 @@ class CourseWaffleFlag(WaffleFlag): outside the context of any course. """ if course_key: - assert isinstance(course_key, CourseKey), ( - "Provided course_key '{}' is not instance of CourseKey.".format(course_key) + assert isinstance( + course_key, CourseKey + ), "Provided course_key '{}' is not instance of CourseKey.".format( + course_key ) return self.waffle_namespace.is_flag_active( self.flag_name, diff --git a/openedx/core/djangoapps/waffle_utils/tests/test_init.py b/openedx/core/djangoapps/waffle_utils/tests/test_init.py index 3135e0d864..2d69635a81 100644 --- a/openedx/core/djangoapps/waffle_utils/tests/test_init.py +++ b/openedx/core/djangoapps/waffle_utils/tests/test_init.py @@ -16,8 +16,6 @@ from .. import ( _get_waffle_flag_custom_attributes_set, CourseWaffleFlag, WaffleFlagNamespace, - WaffleSwitchNamespace, - WaffleSwitch, ) from ..models import WaffleFlagCourseOverrideModel @@ -185,22 +183,3 @@ class TestCourseWaffleFlag(TestCase): self.assertEqual(mock_set_custom_attribute.call_count, 1) else: self.assertEqual(mock_set_custom_attribute.call_count, 0) - - -class TestWaffleSwitch(TestCase): - """ - Tests the WaffleSwitch. - """ - - NAMESPACE_NAME = "test_namespace" - WAFFLE_SWITCH_NAME = "test_switch_name" - TEST_NAMESPACE = WaffleSwitchNamespace(NAMESPACE_NAME) - WAFFLE_SWITCH = WaffleSwitch(TEST_NAMESPACE, WAFFLE_SWITCH_NAME, __name__) - - def test_namespaced_switch_name(self): - """ - Verify namespaced_switch_name returns the correct namespace switch name - """ - expected = self.NAMESPACE_NAME + "." + self.WAFFLE_SWITCH_NAME - actual = self.WAFFLE_SWITCH.namespaced_switch_name - self.assertEqual(actual, expected) From 474da0c5a5b52915bd2f5f1fe0d3f3737c6664e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Wed, 7 Oct 2020 19:12:24 +0200 Subject: [PATCH 03/10] Refactor WaffleFlag and WaffleFlagNamespace internal API This simplifies the internals of the waffle flag classes in order to better move them to edx-toggles later. --- .../core/djangoapps/waffle_utils/__init__.py | 343 +++++++++--------- .../waffle_utils/tests/test_init.py | 95 +++-- .../features/course_experience/__init__.py | 3 +- 3 files changed, 217 insertions(+), 224 deletions(-) diff --git a/openedx/core/djangoapps/waffle_utils/__init__.py b/openedx/core/djangoapps/waffle_utils/__init__.py index 140fa8a074..36a741a949 100644 --- a/openedx/core/djangoapps/waffle_utils/__init__.py +++ b/openedx/core/djangoapps/waffle_utils/__init__.py @@ -52,7 +52,7 @@ Also see ``WAFFLE_FLAG_CUSTOM_ATTRIBUTES`` and docstring for _set_waffle_flag_at for temporarily instrumenting/monitoring waffle flag usage. """ - +from functools import lru_cache import logging import re import traceback @@ -61,7 +61,6 @@ from contextlib import contextmanager from weakref import WeakSet import crum -import six from django.conf import settings from edx_django_utils.monitoring import set_custom_attribute from opaque_keys.edx.keys import CourseKey @@ -76,29 +75,31 @@ from edx_toggles.toggles import WaffleSwitchNamespace as BaseWaffleSwitchNamespa log = logging.getLogger(__name__) -class RequestCacheMixin: - @staticmethod - def _get_request_cache(): - """ - Returns a request cache shared by all instances of this class. - """ - return get_request_cache("WaffleNamespace") +class WaffleSwitchNamespace(BaseWaffleSwitchNamespace): + """ + A waffle switch namespace class that implements request-based caching. + """ - -class WaffleSwitchNamespace(BaseWaffleSwitchNamespace, RequestCacheMixin): @property def _cached_switches(self): """ Returns a dictionary of all namespaced switches in the request cache. """ - return self._get_request_cache().setdefault("switches", {}) + return _get_waffle_namespace_request_cache().setdefault("switches", {}) + + +def _get_waffle_namespace_request_cache(): + """ + Returns a request cache shared by all Waffle namespace objects. + """ + return get_request_cache("WaffleNamespace") class WaffleSwitch(BaseWaffleSwitch): NAMESPACE_CLASS = WaffleSwitchNamespace -class WaffleFlagNamespace(BaseNamespace, RequestCacheMixin): +class WaffleFlagNamespace(BaseNamespace): """ Provides a single namespace for a set of waffle flags. @@ -111,20 +112,13 @@ class WaffleFlagNamespace(BaseNamespace, RequestCacheMixin): """ Returns a dictionary of all namespaced flags in the request cache. """ - return self._get_request_cache().setdefault("flags", {}) + return _get_waffle_namespace_request_cache().setdefault("flags", {}) - def is_flag_active(self, flag_name, check_before_waffle_callback=None): + def is_flag_active(self, flag_name): """ Returns and caches whether the provided flag is active. If the flag value is already cached in the request, it is returned. - If check_before_waffle_callback is supplied, it is called before - checking waffle. - 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. Note: A waffle flag's default is False if not defined. If you think you need the default to be True, see the module docstring for @@ -132,121 +126,50 @@ class WaffleFlagNamespace(BaseNamespace, RequestCacheMixin): 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 returned. If it returns None, then waffle is - used. - """ - # validate arguments namespaced_flag_name = self._namespaced_name(flag_name) - - if check_before_waffle_callback: - value = check_before_waffle_callback(namespaced_flag_name) - if value is not None: - # Do not cache value for the callback, because the key might be different. - # The callback needs to handle its own caching if it wants it. - self._set_waffle_flag_attribute(namespaced_flag_name, value) - return value - - value = self._cached_flags.get(namespaced_flag_name) - if value is not None: - self._set_waffle_flag_attribute(namespaced_flag_name, value) - return value - - request = crum.get_current_request() - if not request: - log.warning( - u"%sFlag '%s' accessed without a request", - self.log_prefix, - namespaced_flag_name, - ) - # Return the Flag's Everyone value if not in a request context. - # Note: this skips the cache as the value might be different - # in a normal request context. This case seems to occur when - # a page redirects to a 404, or for celery workers. - value = self._is_flag_active_for_everyone(namespaced_flag_name) - self._set_waffle_flag_attribute(namespaced_flag_name, value) - set_custom_attribute("warn_flag_no_request_return_value", value) - return value - - value = flag_is_active(request, namespaced_flag_name) - self._cached_flags[namespaced_flag_name] = value - - self._set_waffle_flag_attribute(namespaced_flag_name, value) + value = self._get_flag_active(namespaced_flag_name) + _set_waffle_flag_attribute(namespaced_flag_name, value) return value - def _is_flag_active_for_everyone(self, namespaced_flag_name): + def _get_flag_active(self, namespaced_flag_name): """ - Returns True if the waffle flag is configured as active for Everyone, - False otherwise. + Return the value of the flag activation without touching the _waffle_flag_attribute. """ - # Import is placed here to avoid model import at project startup. - from waffle.models import Flag + # Check namespace cache + value = self._cached_flags.get(namespaced_flag_name) + if value is not None: + return value - try: - waffle_flag = Flag.objects.get(name=namespaced_flag_name) - return waffle_flag.everyone is True - except Flag.DoesNotExist: - return False + # Check in context of request + request = crum.get_current_request() + value = self._get_flag_active_request(namespaced_flag_name, request) + if value is not None: + return value - def _set_waffle_flag_attribute(self, name, value): + # Check value in the absence of any context + log.warning( + u"%sFlag '%s' accessed without a request", + self.log_prefix, + namespaced_flag_name, + ) + # Return the Flag's Everyone value if not in a request context. + # Note: this skips the cache as the value might be different + # in a normal request context. This case seems to occur when + # a page redirects to a 404, or for celery workers. + value = is_flag_active_for_everyone(namespaced_flag_name) + set_custom_attribute("warn_flag_no_request_return_value", value) + return value + + def _get_flag_active_request(self, namespaced_flag_name, request): """ - For any flag name in _WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET, add name/value - to cached values and set custom attribute if the value changed. - - The name of the custom attribute will have the prefix ``flag_`` and the - suffix will match the name of the flag. - The value of the custom attribute could be False, True, or Both. - - The value Both would mean that the flag had both a True and False - value at different times during the transaction. This is most - likely due to having a check_before_waffle_callback, as is the - case with CourseWaffleFlag. - - An example NewRelic query to see the values of a flag in different - environments, if your waffle flag was named ``my.waffle.flag`` might - look like:: - - SELECT count(*) FROM Transaction - WHERE flag_my.waffle.flag IS NOT NULL - FACET appName, flag_my.waffle.flag - - Important: Remember to configure ``WAFFLE_FLAG_CUSTOM_ATTRIBUTES`` for - LMS, Studio and Workers in order to see waffle flag usage in all - edx-platform environments. - + Get flag value in the context of the current request. """ - if name not in _WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET: - return - - flag_attribute_data = self._get_request_cache().setdefault("flag_attribute", {}) - is_value_change = False - if name not in flag_attribute_data: - flag_attribute_data[name] = str(value) - is_value_change = True - else: - if flag_attribute_data[name] != str(value): - flag_attribute_data[name] = "Both" - is_value_change = True - - if is_value_change: - attribute_name = "flag_{}".format(name) - set_custom_attribute(attribute_name, flag_attribute_data[name]) - - -def _get_waffle_flag_custom_attributes_set(): - """ - Returns a set based on the Django setting WAFFLE_FLAG_CUSTOM_ATTRIBUTES (list). - """ - waffle_flag_custom_attributes = getattr( - settings, _WAFFLE_FLAG_CUSTOM_ATTRIBUTES, None - ) - waffle_flag_custom_attributes = ( - waffle_flag_custom_attributes if waffle_flag_custom_attributes else [] - ) - return set(waffle_flag_custom_attributes) + if request: + value = flag_is_active(request, namespaced_flag_name) + self._cached_flags[namespaced_flag_name] = value + return value + return None # .. toggle_name: WAFFLE_FLAG_CUSTOM_ATTRIBUTES @@ -257,16 +180,86 @@ def _get_waffle_flag_custom_attributes_set(): # .. toggle_use_cases: opt_in # .. toggle_creation_date: 2020-06-17 # .. toggle_warnings: Intent is for temporary research (1 day - several weeks) of a flag's usage. -_WAFFLE_FLAG_CUSTOM_ATTRIBUTES = "WAFFLE_FLAG_CUSTOM_ATTRIBUTES" - -# set of waffle flags that should be instrumented with custom attributes -_WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET = _get_waffle_flag_custom_attributes_set() +@lru_cache(maxsize=None) +def _get_waffle_flag_custom_attributes_set(): + attributes = getattr(settings, "WAFFLE_FLAG_CUSTOM_ATTRIBUTES", None) or [] + return set(attributes) -class WaffleFlag(object): +def _set_waffle_flag_attribute(name, value): """ - Represents a single waffle flag, using a cached waffle namespace. + For any flag name in settings.WAFFLE_FLAG_CUSTOM_ATTRIBUTES, add name/value + to cached values and set custom attribute if the value changed. + + The name of the custom attribute will have the prefix ``flag_`` and the + suffix will match the name of the flag. + The value of the custom attribute could be False, True, or Both. + + The value Both would mean that the flag had both a True and False + value at different times during the transaction. This is most + likely due to having a course override, as is the case with + CourseWaffleFlag. + + An example NewRelic query to see the values of a flag in different + environments, if your waffle flag was named ``my.waffle.flag`` might + look like:: + + SELECT count(*) FROM Transaction + WHERE flag_my.waffle.flag IS NOT NULL + FACET appName, flag_my.waffle.flag + + Important: Remember to configure ``WAFFLE_FLAG_CUSTOM_ATTRIBUTES`` for + LMS, Studio and Workers in order to see waffle flag usage in all + edx-platform environments. """ + if name not in _get_waffle_flag_custom_attributes_set(): + return + + flag_attribute_data = _get_waffle_namespace_request_cache().setdefault( + "flag_attribute", {} + ) + is_value_changed = True + if name not in flag_attribute_data: + # New flag + flag_attribute_data[name] = str(value) + else: + # Existing flag + if flag_attribute_data[name] == str(value): + # Same value + is_value_changed = False + else: + # New value + flag_attribute_data[name] = "Both" + + if is_value_changed: + attribute_name = "flag_{}".format(name) + set_custom_attribute(attribute_name, flag_attribute_data[name]) + + +def is_flag_active_for_everyone(namespaced_flag_name): + """ + Returns True if the waffle flag is configured as active for Everyone, + False otherwise. + """ + # Import is placed here to avoid model import at project startup. + from waffle.models import Flag + + try: + waffle_flag = Flag.objects.get(name=namespaced_flag_name) + return waffle_flag.everyone is True + except Flag.DoesNotExist: + return False + + +class WaffleFlag: + """ + Waffle flag class that implements custom override method. + + This class should be removed in favour of edx_toggles.toggles.WaffleFlag once we get rid of the WaffleFlagNamespace + class and the `override` method. + """ + + NAMESPACE_CLASS = WaffleFlagNamespace # use a WeakSet so these instances can be garbage collected if need be _class_instances = WeakSet() @@ -281,10 +274,9 @@ class WaffleFlag(object): module_name (String): The name of the module where the flag is created. This should be ``__name__`` in most cases. """ - if isinstance(waffle_namespace, six.string_types): - waffle_namespace = WaffleFlagNamespace(name=waffle_namespace) + if isinstance(waffle_namespace, str): + waffle_namespace = self.NAMESPACE_CLASS(name=waffle_namespace) - self.waffle_namespace = waffle_namespace self.waffle_namespace = waffle_namespace self.flag_name = flag_name self._module_name = module_name or "" @@ -311,6 +303,7 @@ class WaffleFlag(object): """ Returns the fully namespaced flag name. """ + # pylint: disable=protected-access return self.waffle_namespace._namespaced_name(self.flag_name) def is_enabled(self): @@ -321,6 +314,9 @@ class WaffleFlag(object): @contextmanager def override(self, active=True): + """ + Shortcut method for `override_waffle_flag`. + """ # TODO We can move this import to the top of the file once this code is # not all contained within the __init__ module. from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag @@ -334,47 +330,49 @@ class CourseWaffleFlag(WaffleFlag): Represents a single waffle flag that can be forced on/off for a course. Uses a cached waffle namespace. + + Usage: + + WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace(name='my_namespace') + SOME_COURSE_FLAG = CourseWaffleFlag(WAFFLE_FLAG_NAMESPACE, 'some_course_feature', __name__) + + And then we can check this flag in code with:: + + SOME_COURSE_FLAG.is_enabled(course_key) + + The Django Admin "waffle_utils" section can be used to configure a course override for this same flag (e.g. + my_namespace.some_course_feature). """ - def _get_course_override_callback(self, course_key): + def _get_course_override_value(self, course_key): """ - Returns a function to use as the check_before_waffle_callback. + 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: - course_key (CourseKey): The course to check for override before - checking waffle. + course_key (CourseKey): The course to check for override before checking waffle. """ + # Import is placed here to avoid model import at project startup. + from .models import WaffleFlagCourseOverrideModel - def course_override_callback(namespaced_flag_name): - """ - Returns True/False if the flag was forced on or off for the provided - course. Returns None if the flag was not overridden. + cache_key = "{}.{}".format(self.namespaced_flag_name, str(course_key)) + # pylint: disable=protected-access + course_override = self.waffle_namespace._cached_flags.get(cache_key) - Note: Has side effect of caching the override value. + if course_override is None: + course_override = WaffleFlagCourseOverrideModel.override_value( + self.namespaced_flag_name, course_key + ) + # pylint: disable=protected-access + self.waffle_namespace._cached_flags[cache_key] = course_override - Arguments: - namespaced_flag_name (String): A namespaced version of the flag - to check. - """ - # Import is placed here to avoid model import at project startup. - from .models import WaffleFlagCourseOverrideModel - - cache_key = u"{}.{}".format(namespaced_flag_name, six.text_type(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 + if course_override == WaffleFlagCourseOverrideModel.ALL_CHOICES.on: + return True + if course_override == WaffleFlagCourseOverrideModel.ALL_CHOICES.off: + return False + return None def is_enabled(self, course_key=None): # pylint: disable=arguments-differ """ @@ -391,7 +389,8 @@ class CourseWaffleFlag(WaffleFlag): ), "Provided course_key '{}' is not instance of CourseKey.".format( course_key ) - return self.waffle_namespace.is_flag_active( - self.flag_name, - check_before_waffle_callback=self._get_course_override_callback(course_key), - ) + is_enabled_for_course = self._get_course_override_value(course_key) + if is_enabled_for_course is not None: + _set_waffle_flag_attribute(self.namespaced_flag_name, is_enabled_for_course) + return is_enabled_for_course + return super().is_enabled() diff --git a/openedx/core/djangoapps/waffle_utils/tests/test_init.py b/openedx/core/djangoapps/waffle_utils/tests/test_init.py index 2d69635a81..fa32c647b0 100644 --- a/openedx/core/djangoapps/waffle_utils/tests/test_init.py +++ b/openedx/core/djangoapps/waffle_utils/tests/test_init.py @@ -57,35 +57,33 @@ class TestCourseWaffleFlag(TestCase): Tests various combinations of a flag being set in waffle and overridden for a course. """ - with patch( - 'openedx.core.djangoapps.waffle_utils._WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET', - _get_waffle_flag_custom_attributes_set(), - ): - with patch.object(WaffleFlagCourseOverrideModel, 'override_value', return_value=data['course_override']): - with override_flag(self.NAMESPACED_FLAG_NAME, active=data['waffle_enabled']): - # check twice to test that the result is properly cached - self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY), data['result']) - self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY), data['result']) - # result is cached, so override check should happen once - WaffleFlagCourseOverrideModel.override_value.assert_called_once_with( - self.NAMESPACED_FLAG_NAME, - self.TEST_COURSE_KEY - ) + _get_waffle_flag_custom_attributes_set.cache_clear() + with patch.object(WaffleFlagCourseOverrideModel, 'override_value', return_value=data['course_override']): + with override_flag(self.NAMESPACED_FLAG_NAME, active=data['waffle_enabled']): + # check twice to test that the result is properly cached + self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY), data['result']) + self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY), data['result']) + # result is cached, so override check should happen once + # pylint: disable=no-member + WaffleFlagCourseOverrideModel.override_value.assert_called_once_with( + self.NAMESPACED_FLAG_NAME, + self.TEST_COURSE_KEY + ) - self._assert_waffle_flag_attribute(mock_set_custom_attribute, expected_flag_value=str(data['result'])) - mock_set_custom_attribute.reset_mock() + self._assert_waffle_flag_attribute(mock_set_custom_attribute, expected_flag_value=str(data['result'])) + mock_set_custom_attribute.reset_mock() - # 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. - second_value = data['waffle_enabled'] - self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_2_KEY), second_value) - 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. - second_value = False - self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_2_KEY), second_value) + # 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. + second_value = data['waffle_enabled'] + self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_2_KEY), second_value) + 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. + second_value = False + self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_2_KEY), second_value) expected_flag_value = None if second_value == data['result'] else 'Both' self._assert_waffle_flag_attribute(mock_set_custom_attribute, expected_flag_value=expected_flag_value) @@ -102,23 +100,21 @@ class TestCourseWaffleFlag(TestCase): __name__, ) - with patch( - 'openedx.core.djangoapps.waffle_utils._WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET', - _get_waffle_flag_custom_attributes_set(), + _get_waffle_flag_custom_attributes_set.cache_clear() + with patch.object( + WaffleFlagCourseOverrideModel, + 'override_value', + return_value=WaffleFlagCourseOverrideModel.ALL_CHOICES.unset ): - with patch.object( - WaffleFlagCourseOverrideModel, - 'override_value', - return_value=WaffleFlagCourseOverrideModel.ALL_CHOICES.unset - ): - # check twice to test that the result is properly cached - self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), False) - self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), False) - # result is cached, so override check should happen once - WaffleFlagCourseOverrideModel.override_value.assert_called_once_with( - self.NAMESPACED_FLAG_NAME, - self.TEST_COURSE_KEY - ) + # check twice to test that the result is properly cached + self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), False) + self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), False) + # result is cached, so override check should happen once + # pylint: disable=no-member + WaffleFlagCourseOverrideModel.override_value.assert_called_once_with( + self.NAMESPACED_FLAG_NAME, + self.TEST_COURSE_KEY + ) self._assert_waffle_flag_attribute( mock_set_custom_attribute, @@ -161,14 +157,11 @@ class TestCourseWaffleFlag(TestCase): Test that custom attributes are recorded when waffle flag accessed. """ with override_settings(WAFFLE_FLAG_CUSTOM_ATTRIBUTES=data['waffle_flag_attribute_setting']): - with patch( - 'openedx.core.djangoapps.waffle_utils._WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET', - _get_waffle_flag_custom_attributes_set(), - ): - test_course_flag = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_NAME, __name__) - test_course_flag.is_enabled(self.TEST_COURSE_KEY) - test_course_flag_2 = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_2_NAME, __name__) - test_course_flag_2.is_enabled(self.TEST_COURSE_KEY) + _get_waffle_flag_custom_attributes_set.cache_clear() + test_course_flag = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_NAME, __name__) + test_course_flag.is_enabled(self.TEST_COURSE_KEY) + test_course_flag_2 = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_2_NAME, __name__) + test_course_flag_2.is_enabled(self.TEST_COURSE_KEY) self.assertEqual(mock_set_custom_attribute.call_count, data['expected_count']) diff --git a/openedx/features/course_experience/__init__.py b/openedx/features/course_experience/__init__.py index 5ae92ba88c..40be566219 100644 --- a/openedx/features/course_experience/__init__.py +++ b/openedx/features/course_experience/__init__.py @@ -8,7 +8,8 @@ from waffle import flag_is_active 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 +from openedx.core.djangoapps.waffle_utils import WaffleFlag, WaffleFlagNamespace +from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag # Namespace for course experience waffle flags. WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace(name='course_experience') From 58043727d536117cbac75daa1c6c9c59e92e761b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Mon, 12 Oct 2020 17:52:58 +0200 Subject: [PATCH 04/10] [BD-21] Migrate waffle flag classes to edx-toggles The same API is preserved, internally, to avoid many changes across the edx-platform codebase (for now). --- .../core/djangoapps/waffle_utils/__init__.py | 148 ++---------------- 1 file changed, 14 insertions(+), 134 deletions(-) diff --git a/openedx/core/djangoapps/waffle_utils/__init__.py b/openedx/core/djangoapps/waffle_utils/__init__.py index 36a741a949..f9736b8e01 100644 --- a/openedx/core/djangoapps/waffle_utils/__init__.py +++ b/openedx/core/djangoapps/waffle_utils/__init__.py @@ -53,26 +53,19 @@ for temporarily instrumenting/monitoring waffle flag usage. """ from functools import lru_cache -import logging -import re -import traceback -from abc import ABCMeta from contextlib import contextmanager from weakref import WeakSet -import crum from django.conf import settings from edx_django_utils.monitoring import set_custom_attribute from opaque_keys.edx.keys import CourseKey -from waffle import flag_is_active, switch_is_active -from openedx.core.lib.cache_utils import get_cache as get_request_cache - -from edx_toggles.toggles import BaseNamespace +from edx_toggles.toggles import WaffleFlag as BaseWaffleFlag +from edx_toggles.toggles import WaffleFlagNamespace as BaseWaffleFlagNamespace from edx_toggles.toggles import WaffleSwitch as BaseWaffleSwitch from edx_toggles.toggles import WaffleSwitchNamespace as BaseWaffleSwitchNamespace -log = logging.getLogger(__name__) +from openedx.core.lib.cache_utils import get_cache as get_request_cache class WaffleSwitchNamespace(BaseWaffleSwitchNamespace): @@ -96,15 +89,16 @@ def _get_waffle_namespace_request_cache(): class WaffleSwitch(BaseWaffleSwitch): + """ + Waffle switch class that benefits from the additional features of the WaffleSwitchNamespace. + """ + NAMESPACE_CLASS = WaffleSwitchNamespace -class WaffleFlagNamespace(BaseNamespace): +class WaffleFlagNamespace(BaseWaffleFlagNamespace): """ - Provides a single namespace for a set of waffle flags. - - All namespaced flag values are stored in a single request cache containing - all flags for all namespaces. + Namespace class that implements request-based caching. Also, setup some custom value-checking and processing. """ @property @@ -114,63 +108,16 @@ class WaffleFlagNamespace(BaseNamespace): """ return _get_waffle_namespace_request_cache().setdefault("flags", {}) - def is_flag_active(self, flag_name): - """ - Returns and caches whether the provided flag is active. - - If the flag value is already cached in the request, it is returned. - - Note: A waffle flag's default is False if not defined. If you think you - need the default to be True, see the module docstring for - alternatives. - - Arguments: - flag_name (String): The name of the flag to check. - """ - namespaced_flag_name = self._namespaced_name(flag_name) - value = self._get_flag_active(namespaced_flag_name) + def _get_flag_active(self, namespaced_flag_name): + value = super()._get_flag_active(namespaced_flag_name) _set_waffle_flag_attribute(namespaced_flag_name, value) return value - def _get_flag_active(self, namespaced_flag_name): - """ - Return the value of the flag activation without touching the _waffle_flag_attribute. - """ - # Check namespace cache - value = self._cached_flags.get(namespaced_flag_name) - if value is not None: - return value - - # Check in context of request - request = crum.get_current_request() - value = self._get_flag_active_request(namespaced_flag_name, request) - if value is not None: - return value - - # Check value in the absence of any context - log.warning( - u"%sFlag '%s' accessed without a request", - self.log_prefix, - namespaced_flag_name, - ) - # Return the Flag's Everyone value if not in a request context. - # Note: this skips the cache as the value might be different - # in a normal request context. This case seems to occur when - # a page redirects to a 404, or for celery workers. - value = is_flag_active_for_everyone(namespaced_flag_name) + def _get_flag_active_default(self, namespaced_flag_name): + value = super()._get_flag_active_default(namespaced_flag_name) set_custom_attribute("warn_flag_no_request_return_value", value) return value - def _get_flag_active_request(self, namespaced_flag_name, request): - """ - Get flag value in the context of the current request. - """ - if request: - value = flag_is_active(request, namespaced_flag_name) - self._cached_flags[namespaced_flag_name] = value - return value - return None - # .. toggle_name: WAFFLE_FLAG_CUSTOM_ATTRIBUTES # .. toggle_implementation: DjangoSetting @@ -236,82 +183,15 @@ def _set_waffle_flag_attribute(name, value): set_custom_attribute(attribute_name, flag_attribute_data[name]) -def is_flag_active_for_everyone(namespaced_flag_name): - """ - Returns True if the waffle flag is configured as active for Everyone, - False otherwise. - """ - # Import is placed here to avoid model import at project startup. - from waffle.models import Flag - - try: - waffle_flag = Flag.objects.get(name=namespaced_flag_name) - return waffle_flag.everyone is True - except Flag.DoesNotExist: - return False - - -class WaffleFlag: +class WaffleFlag(BaseWaffleFlag): """ Waffle flag class that implements custom override method. This class should be removed in favour of edx_toggles.toggles.WaffleFlag once we get rid of the WaffleFlagNamespace class and the `override` method. """ - NAMESPACE_CLASS = WaffleFlagNamespace - # use a WeakSet so these instances can be garbage collected if need be - _class_instances = WeakSet() - - def __init__(self, waffle_namespace, flag_name, module_name=None): - """ - Initializes the waffle flag instance. - - Arguments: - waffle_namespace (WaffleFlagNamespace | String): Namespace for this flag. - flag_name (String): The name of the flag (without namespacing). - module_name (String): The name of the module where the flag is created. This should be ``__name__`` in most - cases. - """ - if isinstance(waffle_namespace, str): - waffle_namespace = self.NAMESPACE_CLASS(name=waffle_namespace) - - self.waffle_namespace = waffle_namespace - self.flag_name = flag_name - self._module_name = module_name or "" - self._class_instances.add(self) - - @classmethod - def get_instances(cls): - """ Returns a WeakSet of the instantiated instances of WaffleFlag. """ - return cls._class_instances - - @property - def module_name(self): - """ - Returns the module name. This is cached to work with the WeakSet instances. - """ - return self._module_name - - @module_name.setter - def module_name(self, value): - self._module_name = value - - @property - def namespaced_flag_name(self): - """ - Returns the fully namespaced flag name. - """ - # pylint: disable=protected-access - return self.waffle_namespace._namespaced_name(self.flag_name) - - def is_enabled(self): - """ - Returns whether or not the flag is enabled. - """ - return self.waffle_namespace.is_flag_active(self.flag_name) - @contextmanager def override(self, active=True): """ From db5feec4cfa4d309f735a106d6f22c43d7aabdb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Tue, 13 Oct 2020 12:13:01 +0200 Subject: [PATCH 05/10] Move waffle_utils/testutils.py to edx-toggles --- .../waffle_utils/tests/test_testutils.py | 68 ------------------- .../core/djangoapps/waffle_utils/testutils.py | 67 +++--------------- 2 files changed, 9 insertions(+), 126 deletions(-) delete mode 100644 openedx/core/djangoapps/waffle_utils/tests/test_testutils.py diff --git a/openedx/core/djangoapps/waffle_utils/tests/test_testutils.py b/openedx/core/djangoapps/waffle_utils/tests/test_testutils.py deleted file mode 100644 index a6bd73b4f6..0000000000 --- a/openedx/core/djangoapps/waffle_utils/tests/test_testutils.py +++ /dev/null @@ -1,68 +0,0 @@ -""" -Tests for waffle utils test utilities. -""" - - -import crum -from django.test import TestCase -from django.test.client import RequestFactory -from edx_django_utils.cache import RequestCache -from opaque_keys.edx.keys import CourseKey - -from .. import CourseWaffleFlag, WaffleFlagNamespace -from ..testutils import override_waffle_flag - - -class OverrideWaffleFlagTests(TestCase): - """ - Tests for the override_waffle_flag decorator/context manager. - """ - - NAMESPACE_NAME = "test_namespace" - FLAG_NAME = "test_flag" - NAMESPACED_FLAG_NAME = NAMESPACE_NAME + "." + FLAG_NAME - - TEST_COURSE_KEY = CourseKey.from_string("edX/DemoX/Demo_Course") - TEST_NAMESPACE = WaffleFlagNamespace(NAMESPACE_NAME) - TEST_COURSE_FLAG = CourseWaffleFlag(TEST_NAMESPACE, FLAG_NAME, __name__) - - def setUp(self): - super(OverrideWaffleFlagTests, self).setUp() - request = RequestFactory().request() - self.addCleanup(crum.set_current_request, None) - crum.set_current_request(request) - RequestCache.clear_all_namespaces() - - @override_waffle_flag(TEST_COURSE_FLAG, True) - def assert_decorator_activates_flag(self): - assert self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY) - - def test_override_waffle_flag_pre_cached(self): - # checks and caches the is_enabled value - assert not self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY) - flag_cache = self.TEST_COURSE_FLAG.waffle_namespace._cached_flags - assert self.NAMESPACED_FLAG_NAME in flag_cache - - self.assert_decorator_activates_flag() - - # test cached flag is restored - assert self.NAMESPACED_FLAG_NAME in flag_cache - assert not self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY) - - def test_override_waffle_flag_not_pre_cached(self): - # check that the flag is not yet cached - flag_cache = self.TEST_COURSE_FLAG.waffle_namespace._cached_flags - assert self.NAMESPACED_FLAG_NAME not in flag_cache - - self.assert_decorator_activates_flag() - - # test cache is removed when no longer using decorator/context manager - assert self.NAMESPACED_FLAG_NAME not in flag_cache - - def test_override_waffle_flag_as_context_manager(self): - assert not self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY) - - with override_waffle_flag(self.TEST_COURSE_FLAG, True): - assert self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY) - - assert not self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY) diff --git a/openedx/core/djangoapps/waffle_utils/testutils.py b/openedx/core/djangoapps/waffle_utils/testutils.py index af7b6a614b..6823287047 100644 --- a/openedx/core/djangoapps/waffle_utils/testutils.py +++ b/openedx/core/djangoapps/waffle_utils/testutils.py @@ -2,66 +2,17 @@ Test utilities for waffle utilities. """ - -from waffle.testutils import override_flag +# Import from edx-toggles to preserve import paths +# pylint: disable=unused-import +from edx_toggles.toggles.testutils import override_waffle_flag # Can be used with FilteredQueryCountMixin.assertNumQueries() to blacklist # waffle tables. For example: # QUERY_COUNT_TABLE_BLACKLIST = WAFFLE_TABLES # with self.assertNumQueries(6, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): -WAFFLE_TABLES = ['waffle_utils_waffleflagcourseoverridemodel', 'waffle_flag', 'waffle_switch', 'waffle_sample'] - - -class override_waffle_flag(override_flag): - """ - override_waffle_flag is a contextmanager for easier testing of flags. - - It accepts two parameters, the flag itself and its intended state. Example - usage:: - - with override_waffle_flag(SOME_COURSE_FLAG, active=True): - ... - - If the flag already exists, its value will be changed inside the context - block, then restored to the original value. If the flag does not exist - before entering the context, it is created, then removed at the end of the - block. - - It can also act as a decorator:: - - @override_waffle_flag(SOME_COURSE_FLAG, active=True) - def test_happy_mode_enabled(): - ... - """ - _cached_value = None - - def __init__(self, flag, active): - """ - - Args: - flag (WaffleFlag): The namespaced cached waffle flag. - active (Boolean): The value to which the flag will be set. - """ - self.flag = flag - waffle_namespace = flag.waffle_namespace - name = waffle_namespace._namespaced_name(flag.flag_name) # pylint: disable=protected-access - super(override_waffle_flag, self).__init__(name, active) - - def __enter__(self): - super(override_waffle_flag, self).__enter__() - - # pylint: disable=protected-access - # Store values that have been cached on the flag - self._cached_value = self.flag.waffle_namespace._cached_flags.get(self.name) - self.flag.waffle_namespace._cached_flags[self.name] = self.active - - def __exit__(self, exc_type, exc_val, exc_tb): - super(override_waffle_flag, self).__exit__(exc_type, exc_val, exc_tb) - - # pylint: disable=protected-access - # Restore the cached values - waffle_namespace = self.flag.waffle_namespace - waffle_namespace._cached_flags.pop(self.name, None) - - if self._cached_value is not None: - waffle_namespace._cached_flags[self.name] = self._cached_value +WAFFLE_TABLES = [ + "waffle_utils_waffleflagcourseoverridemodel", + "waffle_flag", + "waffle_switch", + "waffle_sample", +] From e5500b34a04d0c77ba7c11aee2b2561c3d66aecd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Tue, 13 Oct 2020 19:57:13 +0200 Subject: [PATCH 06/10] Backport override features from edx-toggles to waffle_utils Note that those features are destined to be deprecated, eventually. --- .../core/djangoapps/waffle_utils/__init__.py | 202 ++++++------------ 1 file changed, 64 insertions(+), 138 deletions(-) diff --git a/openedx/core/djangoapps/waffle_utils/__init__.py b/openedx/core/djangoapps/waffle_utils/__init__.py index f9736b8e01..66750c49a0 100644 --- a/openedx/core/djangoapps/waffle_utils/__init__.py +++ b/openedx/core/djangoapps/waffle_utils/__init__.py @@ -1,186 +1,110 @@ """ -Utilities for waffle. +Extra utilities for waffle: most classes are defined in edx_toggles.toggles, but we keep here some extra classes for +usage within edx-platform. These classes cover course override use cases. -Includes namespacing, caching, and course overrides for waffle flags. Usage: -For Waffle Flags, first set up the namespace, and then create flags using the -namespace. For example:: - WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace(name='my_namespace') - # Use CourseWaffleFlag when you are in the context of a course. SOME_COURSE_FLAG = CourseWaffleFlag(WAFFLE_FLAG_NAMESPACE, 'some_course_feature', __name__) - # Use WaffleFlag when outside the context of a course. - SOME_FLAG = WaffleFlag(WAFFLE_FLAG_NAMESPACE, 'some_feature', __name__) -You can check these flags in code using the following:: +You can check this flag in code using the following:: - SOME_FLAG.is_enabled() SOME_COURSE_FLAG.is_enabled(course_key) -To test these WaffleFlags, see testutils.py. - -In the above examples, you will use Django Admin "waffle" section to configure -for a flag named: my_namespace.some_course_feature - -You could also use the Django Admin "waffle_utils" section to configure a course -override for this same flag (e.g. my_namespace.some_course_feature). - -For Waffle Switches, first set up the namespace, and then create the flag name. -For example:: - - WAFFLE_SWITCHES = WaffleSwitchNamespace(name=WAFFLE_NAMESPACE) - - ESTIMATE_FIRST_ATTEMPTED = 'estimate_first_attempted' - -You can then use the switch as follows:: - - WAFFLE_SWITCHES.is_enabled(waffle.ESTIMATE_FIRST_ATTEMPTED) - To test WaffleSwitchNamespace, use the provided context managers. For example: with WAFFLE_SWITCHES.override(waffle.ESTIMATE_FIRST_ATTEMPTED, active=True): ... -For long-lived flags, you may want to change the default for devstack, sandboxes, -or new Open edX releases. For help with this, see: -openedx/core/djangoapps/waffle_utils/docs/decisions/0001-refactor-waffle-flag-default.rst - Also see ``WAFFLE_FLAG_CUSTOM_ATTRIBUTES`` and docstring for _set_waffle_flag_attribute for temporarily instrumenting/monitoring waffle flag usage. """ -from functools import lru_cache +import logging from contextlib import contextmanager -from weakref import WeakSet -from django.conf import settings -from edx_django_utils.monitoring import set_custom_attribute from opaque_keys.edx.keys import CourseKey from edx_toggles.toggles import WaffleFlag as BaseWaffleFlag -from edx_toggles.toggles import WaffleFlagNamespace as BaseWaffleFlagNamespace +from edx_toggles.toggles import WaffleFlagNamespace from edx_toggles.toggles import WaffleSwitch as BaseWaffleSwitch from edx_toggles.toggles import WaffleSwitchNamespace as BaseWaffleSwitchNamespace -from openedx.core.lib.cache_utils import get_cache as get_request_cache +log = logging.getLogger(__name__) class WaffleSwitchNamespace(BaseWaffleSwitchNamespace): """ - A waffle switch namespace class that implements request-based caching. + Waffle switch namespace that implements custom overriding methods. We should eventually get rid of this class. """ - @property - def _cached_switches(self): + @contextmanager + def override(self, switch_name, active=True): """ - Returns a dictionary of all namespaced switches in the request cache. + Overrides the active value for the given switch for the duration of this + contextmanager. + Note: The value is overridden in the request cache AND in the model. """ - return _get_waffle_namespace_request_cache().setdefault("switches", {}) + previous_active = self.is_enabled(switch_name) + try: + self.override_for_request(switch_name, active) + with self.override_in_model(switch_name, active): + yield + finally: + self.override_for_request(switch_name, previous_active) + def override_for_request(self, switch_name, active=True): + """ + Overrides the active value for the given switch for the remainder of + this request (as this is not a context manager). + Note: The value is overridden in the request cache, not in the model. + """ + namespaced_switch_name = self._namespaced_name(switch_name) + self._cached_switches[namespaced_switch_name] = active + log.info( + "%sSwitch '%s' set to %s for request.", + self.log_prefix, + namespaced_switch_name, + active, + ) -def _get_waffle_namespace_request_cache(): - """ - Returns a request cache shared by all Waffle namespace objects. - """ - return get_request_cache("WaffleNamespace") + @contextmanager + def override_in_model(self, switch_name, active=True): + """ + Overrides the active value for the given switch for the duration of this + contextmanager. + Note: The value is overridden in the model, not the request cache. + Note: This should probably be moved to a test class. + """ + # Import is placed here to avoid model import at project startup. + # pylint: disable=import-outside-toplevel + from waffle.testutils import override_switch as waffle_override_switch + + namespaced_switch_name = self._namespaced_name(switch_name) + with waffle_override_switch(namespaced_switch_name, active): + log.info( + "%sSwitch '%s' set to %s in model.", + self.log_prefix, + namespaced_switch_name, + active, + ) + yield class WaffleSwitch(BaseWaffleSwitch): """ - Waffle switch class that benefits from the additional features of the WaffleSwitchNamespace. + This class should be removed in favour of edx_toggles.toggles.WaffleSwitch once we get rid of the + WaffleSwitchNamespace class. """ NAMESPACE_CLASS = WaffleSwitchNamespace - -class WaffleFlagNamespace(BaseWaffleFlagNamespace): - """ - Namespace class that implements request-based caching. Also, setup some custom value-checking and processing. - """ - - @property - def _cached_flags(self): - """ - Returns a dictionary of all namespaced flags in the request cache. - """ - return _get_waffle_namespace_request_cache().setdefault("flags", {}) - - def _get_flag_active(self, namespaced_flag_name): - value = super()._get_flag_active(namespaced_flag_name) - _set_waffle_flag_attribute(namespaced_flag_name, value) - return value - - def _get_flag_active_default(self, namespaced_flag_name): - value = super()._get_flag_active_default(namespaced_flag_name) - set_custom_attribute("warn_flag_no_request_return_value", value) - return value - - -# .. toggle_name: WAFFLE_FLAG_CUSTOM_ATTRIBUTES -# .. toggle_implementation: DjangoSetting -# .. toggle_default: False -# .. toggle_description: A list of waffle flags to track with custom attributes having -# values of (True, False, or Both). -# .. toggle_use_cases: opt_in -# .. toggle_creation_date: 2020-06-17 -# .. toggle_warnings: Intent is for temporary research (1 day - several weeks) of a flag's usage. -@lru_cache(maxsize=None) -def _get_waffle_flag_custom_attributes_set(): - attributes = getattr(settings, "WAFFLE_FLAG_CUSTOM_ATTRIBUTES", None) or [] - return set(attributes) - - -def _set_waffle_flag_attribute(name, value): - """ - For any flag name in settings.WAFFLE_FLAG_CUSTOM_ATTRIBUTES, add name/value - to cached values and set custom attribute if the value changed. - - The name of the custom attribute will have the prefix ``flag_`` and the - suffix will match the name of the flag. - The value of the custom attribute could be False, True, or Both. - - The value Both would mean that the flag had both a True and False - value at different times during the transaction. This is most - likely due to having a course override, as is the case with - CourseWaffleFlag. - - An example NewRelic query to see the values of a flag in different - environments, if your waffle flag was named ``my.waffle.flag`` might - look like:: - - SELECT count(*) FROM Transaction - WHERE flag_my.waffle.flag IS NOT NULL - FACET appName, flag_my.waffle.flag - - Important: Remember to configure ``WAFFLE_FLAG_CUSTOM_ATTRIBUTES`` for - LMS, Studio and Workers in order to see waffle flag usage in all - edx-platform environments. - """ - if name not in _get_waffle_flag_custom_attributes_set(): - return - - flag_attribute_data = _get_waffle_namespace_request_cache().setdefault( - "flag_attribute", {} - ) - is_value_changed = True - if name not in flag_attribute_data: - # New flag - flag_attribute_data[name] = str(value) - else: - # Existing flag - if flag_attribute_data[name] == str(value): - # Same value - is_value_changed = False - else: - # New value - flag_attribute_data[name] = "Both" - - if is_value_changed: - attribute_name = "flag_{}".format(name) - set_custom_attribute(attribute_name, flag_attribute_data[name]) + @contextmanager + def override(self, active=True): + with self.waffle_namespace.override(self.switch_name, active): + yield class WaffleFlag(BaseWaffleFlag): @@ -190,7 +114,6 @@ class WaffleFlag(BaseWaffleFlag): This class should be removed in favour of edx_toggles.toggles.WaffleFlag once we get rid of the WaffleFlagNamespace class and the `override` method. """ - NAMESPACE_CLASS = WaffleFlagNamespace @contextmanager def override(self, active=True): @@ -271,6 +194,9 @@ class CourseWaffleFlag(WaffleFlag): ) is_enabled_for_course = self._get_course_override_value(course_key) if is_enabled_for_course is not None: - _set_waffle_flag_attribute(self.namespaced_flag_name, is_enabled_for_course) + # pylint: disable=protected-access + self.namespace._monitor_value( + self.namespaced_flag_name, is_enabled_for_course + ) return is_enabled_for_course return super().is_enabled() From 98ffa347d5eb250f87dec21deff90f6c0de3528e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Tue, 13 Oct 2020 21:06:09 +0200 Subject: [PATCH 07/10] Fix waffle_utils unit tests --- openedx/core/djangoapps/waffle_utils/__init__.py | 2 +- .../core/djangoapps/waffle_utils/tests/test_init.py | 12 +++++------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/openedx/core/djangoapps/waffle_utils/__init__.py b/openedx/core/djangoapps/waffle_utils/__init__.py index 66750c49a0..010f5bb14f 100644 --- a/openedx/core/djangoapps/waffle_utils/__init__.py +++ b/openedx/core/djangoapps/waffle_utils/__init__.py @@ -195,7 +195,7 @@ class CourseWaffleFlag(WaffleFlag): is_enabled_for_course = self._get_course_override_value(course_key) if is_enabled_for_course is not None: # pylint: disable=protected-access - self.namespace._monitor_value( + self.NAMESPACE_CLASS._monitor_value( self.namespaced_flag_name, is_enabled_for_course ) return is_enabled_for_course diff --git a/openedx/core/djangoapps/waffle_utils/tests/test_init.py b/openedx/core/djangoapps/waffle_utils/tests/test_init.py index fa32c647b0..e39359dc4f 100644 --- a/openedx/core/djangoapps/waffle_utils/tests/test_init.py +++ b/openedx/core/djangoapps/waffle_utils/tests/test_init.py @@ -7,13 +7,14 @@ import ddt from django.test import TestCase from django.test.client import RequestFactory from django.test.utils import override_settings +# Note that we really shouldn't import from edx_toggles' internal API +import edx_toggles.toggles.internal.waffle from edx_django_utils.cache import RequestCache from mock import call, patch from opaque_keys.edx.keys import CourseKey from waffle.testutils import override_flag from .. import ( - _get_waffle_flag_custom_attributes_set, CourseWaffleFlag, WaffleFlagNamespace, ) @@ -45,7 +46,7 @@ class TestCourseWaffleFlag(TestCase): RequestCache.clear_all_namespaces() @override_settings(WAFFLE_FLAG_CUSTOM_ATTRIBUTES=[NAMESPACED_FLAG_NAME]) - @patch('openedx.core.djangoapps.waffle_utils.set_custom_attribute') + @patch.object(edx_toggles.toggles.internal.waffle, 'set_custom_attribute') @ddt.data( {'course_override': WaffleFlagCourseOverrideModel.ALL_CHOICES.on, 'waffle_enabled': False, 'result': True}, {'course_override': WaffleFlagCourseOverrideModel.ALL_CHOICES.off, 'waffle_enabled': True, 'result': False}, @@ -57,7 +58,6 @@ class TestCourseWaffleFlag(TestCase): Tests various combinations of a flag being set in waffle and overridden for a course. """ - _get_waffle_flag_custom_attributes_set.cache_clear() with patch.object(WaffleFlagCourseOverrideModel, 'override_value', return_value=data['course_override']): with override_flag(self.NAMESPACED_FLAG_NAME, active=data['waffle_enabled']): # check twice to test that the result is properly cached @@ -89,7 +89,7 @@ class TestCourseWaffleFlag(TestCase): self._assert_waffle_flag_attribute(mock_set_custom_attribute, expected_flag_value=expected_flag_value) @override_settings(WAFFLE_FLAG_CUSTOM_ATTRIBUTES=[NAMESPACED_FLAG_NAME]) - @patch('openedx.core.djangoapps.waffle_utils.set_custom_attribute') + @patch.object(edx_toggles.toggles.internal.waffle, 'set_custom_attribute') def test_undefined_waffle_flag(self, mock_set_custom_attribute): """ Test flag with undefined waffle flag. @@ -100,7 +100,6 @@ class TestCourseWaffleFlag(TestCase): __name__, ) - _get_waffle_flag_custom_attributes_set.cache_clear() with patch.object( WaffleFlagCourseOverrideModel, 'override_value', @@ -151,13 +150,12 @@ class TestCourseWaffleFlag(TestCase): {'expected_count': 1, 'waffle_flag_attribute_setting': [NAMESPACED_FLAG_NAME]}, {'expected_count': 2, 'waffle_flag_attribute_setting': [NAMESPACED_FLAG_NAME, NAMESPACED_FLAG_2_NAME]}, ) - @patch('openedx.core.djangoapps.waffle_utils.set_custom_attribute') + @patch.object(edx_toggles.toggles.internal.waffle, 'set_custom_attribute') def test_waffle_flag_attribute_for_various_settings(self, data, mock_set_custom_attribute): """ Test that custom attributes are recorded when waffle flag accessed. """ with override_settings(WAFFLE_FLAG_CUSTOM_ATTRIBUTES=data['waffle_flag_attribute_setting']): - _get_waffle_flag_custom_attributes_set.cache_clear() test_course_flag = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_NAME, __name__) test_course_flag.is_enabled(self.TEST_COURSE_KEY) test_course_flag_2 = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_2_NAME, __name__) From a27499830c4ed87a1f534a9ce5226e1bb6a2eedf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Wed, 14 Oct 2020 11:12:40 +0200 Subject: [PATCH 08/10] Add deprecation warning comments to waffle_utils code --- .../core/djangoapps/waffle_utils/__init__.py | 34 ++++++------------- .../core/djangoapps/waffle_utils/testutils.py | 1 + 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/openedx/core/djangoapps/waffle_utils/__init__.py b/openedx/core/djangoapps/waffle_utils/__init__.py index 010f5bb14f..086d97e287 100644 --- a/openedx/core/djangoapps/waffle_utils/__init__.py +++ b/openedx/core/djangoapps/waffle_utils/__init__.py @@ -1,26 +1,6 @@ """ -Extra utilities for waffle: most classes are defined in edx_toggles.toggles, but we keep here some extra classes for -usage within edx-platform. These classes cover course override use cases. - - -Usage: - - WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace(name='my_namespace') - # Use CourseWaffleFlag when you are in the context of a course. - SOME_COURSE_FLAG = CourseWaffleFlag(WAFFLE_FLAG_NAMESPACE, 'some_course_feature', __name__) - -You can check this flag in code using the following:: - - SOME_COURSE_FLAG.is_enabled(course_key) - -To test WaffleSwitchNamespace, use the provided context managers. For example: - - with WAFFLE_SWITCHES.override(waffle.ESTIMATE_FIRST_ATTEMPTED, active=True): - ... - -Also see ``WAFFLE_FLAG_CUSTOM_ATTRIBUTES`` and docstring for _set_waffle_flag_attribute -for temporarily instrumenting/monitoring waffle flag usage. - +Extra utilities for waffle: most classes are defined in edx_toggles.toggles (https://edx-toggles.readthedocs.io/), but +we keep here some extra classes for usage within edx-platform. These classes cover course override use cases. """ import logging from contextlib import contextmanager @@ -38,6 +18,13 @@ log = logging.getLogger(__name__) class WaffleSwitchNamespace(BaseWaffleSwitchNamespace): """ Waffle switch namespace that implements custom overriding methods. We should eventually get rid of this class. + + To test WaffleSwitchNamespace, use the provided context managers. For example: + + with WAFFLE_SWITCHES.override(waffle.ESTIMATE_FIRST_ATTEMPTED, active=True): + ... + + Note: this should eventually be deprecated in favour of a dedicated `override_waffle_switch` context manager. """ @contextmanager @@ -130,7 +117,8 @@ class WaffleFlag(BaseWaffleFlag): class CourseWaffleFlag(WaffleFlag): """ - Represents a single waffle flag that can be forced on/off for a course. + Represents a single waffle flag that can be forced on/off for a course. This class should be used instead of + WaffleFlag when in the context of a course. Uses a cached waffle namespace. diff --git a/openedx/core/djangoapps/waffle_utils/testutils.py b/openedx/core/djangoapps/waffle_utils/testutils.py index 6823287047..03c44ce3a5 100644 --- a/openedx/core/djangoapps/waffle_utils/testutils.py +++ b/openedx/core/djangoapps/waffle_utils/testutils.py @@ -3,6 +3,7 @@ Test utilities for waffle utilities. """ # Import from edx-toggles to preserve import paths +# TODO: Deprecate and remove # pylint: disable=unused-import from edx_toggles.toggles.testutils import override_waffle_flag From 373ee5f3209401cba77da5c5e0bea0b74d0fc0a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Fri, 23 Oct 2020 08:54:54 +0200 Subject: [PATCH 09/10] Ensure compatibility with edx-toggles despite upcoming breaking changes edx-toggles==2.0.0 is likely to suppress namespace objects for the management of toggle objefcts. We explicitely prevent this by adding a requirement constraint. --- requirements/constraints.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 98e41abf49..2390752fb1 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -40,6 +40,9 @@ edx-enterprise==3.9.11 # v2 requires the ES7 upgrade work to be complete edx-search<2.0.0 +# We expect v2.0.0 to introduce large breaking changes in the feature toggle API +edx-toggles<2.0.0 + # Upgrading to 2.12.0 breaks several test classes due to API changes, need to update our code accordingly factory-boy==2.8.1 From 312f0cd749027da31b6987fc188b2f28248be47d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Thu, 29 Oct 2020 20:08:10 +0100 Subject: [PATCH 10/10] Fix error in toggle state endpoint in the absence of module_name When module_name is None, the call to edx-django-utils' get_code_owner_from_module crashes. So we avoid making that call when the module_name is None, which sometimes happens (for good reasons or not, but it's valid behaviour). --- .../djangoapps/waffle_utils/tests/test_views.py | 15 ++++++++++++++- openedx/core/djangoapps/waffle_utils/views.py | 8 ++++---- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/waffle_utils/tests/test_views.py b/openedx/core/djangoapps/waffle_utils/tests/test_views.py index f6396a95d2..de47e17d02 100644 --- a/openedx/core/djangoapps/waffle_utils/tests/test_views.py +++ b/openedx/core/djangoapps/waffle_utils/tests/test_views.py @@ -2,14 +2,16 @@ Tests for waffle utils views. """ from django.test import TestCase +from edx_django_utils.monitoring.code_owner import utils as code_owner_utils +from mock import patch from rest_framework.test import APIRequestFactory from waffle.testutils import override_switch from student.tests.factories import UserFactory from .. import WaffleFlag, WaffleFlagNamespace, WaffleSwitch, WaffleSwitchNamespace -from ..views import ToggleStateView from ..testutils import override_waffle_flag +from ..views import ToggleStateView TEST_WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace('test') TEST_WAFFLE_FLAG = WaffleFlag(TEST_WAFFLE_FLAG_NAMESPACE, 'flag', __name__) @@ -45,6 +47,17 @@ class ToggleStateViewTests(TestCase): # This is no longer the first switch #self.assertEqual(response.data['waffle_switches'][0]['name'], 'test.switch') + def test_code_owners_without_module_information(self): + # Create a waffle flag without any associated module_name + waffle_flag = WaffleFlag(TEST_WAFFLE_FLAG_NAMESPACE, "flag2", module_name=None) + with patch.object(code_owner_utils, "get_code_owner_mappings", return_value={}): + response = self._get_toggle_state_response(is_staff=True) + + result = [ + flag for flag in response.data["waffle_flags"] if flag["name"] == waffle_flag.name + ][0] + self.assertNotIn("code_owner", result) + def _get_toggle_state_response(self, is_staff=True): request = APIRequestFactory().get('/api/toggles/state/') user = UserFactory() diff --git a/openedx/core/djangoapps/waffle_utils/views.py b/openedx/core/djangoapps/waffle_utils/views.py index 64ea31fcb4..2ce629a62e 100644 --- a/openedx/core/djangoapps/waffle_utils/views.py +++ b/openedx/core/djangoapps/waffle_utils/views.py @@ -2,13 +2,13 @@ Views that we will use to view toggle state in edx-platform. """ from collections import OrderedDict -from django.conf import settings -from edx_django_utils.monitoring.code_owner.utils import get_code_owner_from_module, is_code_owner_mappings_configured +from django.conf import settings +from edx_django_utils.monitoring import get_code_owner_from_module from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.permissions import IsStaff -from rest_framework.authentication import SessionAuthentication from rest_framework import permissions, views +from rest_framework.authentication import SessionAuthentication from rest_framework.response import Response from waffle.models import Flag, Switch @@ -107,7 +107,7 @@ class ToggleStateView(views.APIView): """ toggle['class'] = toggle_instance.__class__.__name__ toggle['module'] = toggle_instance.module_name - if is_code_owner_mappings_configured(): + if toggle_instance.module_name: code_owner = get_code_owner_from_module(toggle_instance.module_name) if code_owner: toggle['code_owner'] = code_owner