Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

ARCHBOM-1305: remove deprecated flag_undefined_default (#24426)

Browse Source 
This is the final step in removing the deprecated
flag_undefined_default as explained by the following ADR:
https://github.com/edx/edx-platform/blob/master/openedx/core/djangoapps/waffle_utils/docs/decisions/0001-refactor-waffle-flag-default.rst

Notes:

* All uses of flag_undefined_default=False were always
  supposed to have been no-ops.
* All uses of flag_undefined_default=True that are removed
  in this PR have been replaced by migrations in past PRs.
* The temporary metric temp_flag_default_used id no longer
  reporting any data.

ARCHBOM-1305
This commit is contained in:
Robert Raposa
2020-07-09 09:31:31 -04:00
committed by GitHub
parent 36e07f6be8
commit 77e490f057
19 changed files with 89 additions and 120 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
cms/djangoapps/contentstore/config/waffle.py
View File

@@ -30,18 +30,15 @@ def waffle_flags():
ENABLE_PROCTORING_PROVIDER_OVERRIDES = CourseWaffleFlag(
waffle_namespace=waffle_flags(),
flag_name=u'enable_proctoring_provider_overrides',
flag_undefined_default=False
)
# TODO: After removing this flag, add a migration to remove waffle flag in a follow-up deployment.
ENABLE_CHECKLISTS_QUALITY = CourseWaffleFlag(
waffle_namespace=waffle_flags(),
flag_name=u'enable_checklists_quality',
flag_undefined_default=True
)
SHOW_REVIEW_RULES_FLAG = CourseWaffleFlag(
waffle_namespace=waffle_flags(),
flag_name=u'show_review_rules',
flag_undefined_default=False
)

1
cms/djangoapps/contentstore/views/videos.py
View File

@@ -81,7 +81,6 @@ WAFFLE_STUDIO_FLAG_NAMESPACE = WaffleFlagNamespace(name=u'studio')
ENABLE_VIDEO_UPLOAD_PAGINATION = CourseWaffleFlag(
waffle_namespace=WAFFLE_STUDIO_FLAG_NAMESPACE,
flag_name=u'enable_video_upload_pagination',
flag_undefined_default=False
)
# Default expiration, in seconds, of one-time URLs used for uploading videos.
KEY_EXPIRATION_IN_SECONDS = 86400

1
lms/djangoapps/course_api/blocks/toggles.py
View File

@@ -23,5 +23,4 @@ COURSE_BLOCKS_API_NAMESPACE = WaffleFlagNamespace(name=u'course_blocks_api')
HIDE_ACCESS_DENIALS_FLAG = WaffleFlag(
waffle_namespace=COURSE_BLOCKS_API_NAMESPACE,
flag_name=u'hide_access_denials',
flag_undefined_default=False
)

2
lms/djangoapps/experiments/flags.py
View File

@@ -61,7 +61,7 @@ class ExperimentWaffleFlag(CourseWaffleFlag):
self.num_buckets = num_buckets
self.experiment_id = experiment_id
self.bucket_flags = [
CourseWaffleFlag(waffle_namespace, '{}.{}'.format(flag_name, bucket), flag_undefined_default=False)
CourseWaffleFlag(waffle_namespace, '{}.{}'.format(flag_name, bucket))
for bucket in range(num_buckets)
]

1
lms/djangoapps/experiments/views_custom.py
View File

@@ -45,7 +45,6 @@ from track import segment
MOBILE_UPSELL_FLAG = WaffleFlag(
waffle_namespace=WaffleFlagNamespace(name=u'experiments'),
flag_name=u'mobile_upsell_rev934',
flag_undefined_default=False
)
MOBILE_UPSELL_EXPERIMENT = 'mobile_upsell_experiment'

4
lms/djangoapps/grades/config/waffle.py
View File

@@ -38,25 +38,21 @@ def waffle_flags():
REJECTED_EXAM_OVERRIDES_GRADE: CourseWaffleFlag(
namespace,
REJECTED_EXAM_OVERRIDES_GRADE,
flag_undefined_default=True,
),
# TODO: After removing this flag, add a migration to remove waffle flag in a follow-up deployment.
ENFORCE_FREEZE_GRADE_AFTER_COURSE_END: CourseWaffleFlag(
namespace,
ENFORCE_FREEZE_GRADE_AFTER_COURSE_END,
flag_undefined_default=True,
),
# Have this course override flag so we can selectively turn off the gradebook for courses.
# TODO: After removing this flag, add a migration to remove waffle flag in a follow-up deployment.
WRITABLE_GRADEBOOK: CourseWaffleFlag(
namespace,
WRITABLE_GRADEBOOK,
flag_undefined_default=True,
),
BULK_MANAGEMENT: CourseWaffleFlag(
namespace,
BULK_MANAGEMENT,
flag_undefined_default=False,
),
}

20
lms/djangoapps/grades/tests/test_tasks.py
View File

@@ -164,10 +164,10 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest
self.assertEqual(mock_block_structure_create.call_count, 1)
@ddt.data(
(ModuleStoreEnum.Type.mongo, 1, 37, True),
(ModuleStoreEnum.Type.mongo, 1, 37, False),
(ModuleStoreEnum.Type.split, 3, 37, True),
(ModuleStoreEnum.Type.split, 3, 37, False),
(ModuleStoreEnum.Type.mongo, 1, 36, True),
(ModuleStoreEnum.Type.mongo, 1, 36, False),
(ModuleStoreEnum.Type.split, 3, 36, True),
(ModuleStoreEnum.Type.split, 3, 36, False),
)
@ddt.unpack
def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections):
@@ -179,8 +179,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest
self._apply_recalculate_subsection_grade()
@ddt.data(
(ModuleStoreEnum.Type.mongo, 1, 37),
(ModuleStoreEnum.Type.split, 3, 37),
(ModuleStoreEnum.Type.mongo, 1, 36),
(ModuleStoreEnum.Type.split, 3, 36),
)
@ddt.unpack
def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls):
@@ -225,8 +225,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest
)
@ddt.data(
(ModuleStoreEnum.Type.mongo, 1, 20),
(ModuleStoreEnum.Type.split, 3, 20),
(ModuleStoreEnum.Type.mongo, 1, 19),
(ModuleStoreEnum.Type.split, 3, 19),
)
@ddt.unpack
def test_persistent_grades_not_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries):
@@ -240,8 +240,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest
self.assertEqual(len(PersistentSubsectionGrade.bulk_read_grades(self.user.id, self.course.id)), 0)
@ddt.data(
(ModuleStoreEnum.Type.mongo, 1, 38),
(ModuleStoreEnum.Type.split, 3, 38),
(ModuleStoreEnum.Type.mongo, 1, 37),
(ModuleStoreEnum.Type.split, 3, 37),
)
@ddt.unpack
def test_persistent_grades_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries):

1
lms/djangoapps/instructor/toggles.py
View File

@@ -22,7 +22,6 @@ WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace(name='instructor')
OPTIMISED_IS_SMALL_COURSE = WaffleFlag(
waffle_namespace=WAFFLE_FLAG_NAMESPACE,
flag_name='optimised_is_small_course',
flag_undefined_default=False
)

2
lms/djangoapps/instructor_task/config/waffle.py
View File

@@ -25,12 +25,10 @@ def waffle_flags():
GENERATE_PROBLEM_GRADE_REPORT_VERIFIED_ONLY: CourseWaffleFlag(
waffle_namespace=INSTRUCTOR_TASK_WAFFLE_FLAG_NAMESPACE,
flag_name=GENERATE_PROBLEM_GRADE_REPORT_VERIFIED_ONLY,
flag_undefined_default=False,
),
GENERATE_COURSE_GRADE_REPORT_VERIFIED_ONLY: CourseWaffleFlag(
waffle_namespace=INSTRUCTOR_TASK_WAFFLE_FLAG_NAMESPACE,
flag_name=GENERATE_COURSE_GRADE_REPORT_VERIFIED_ONLY,
flag_undefined_default=False,
),
}

1
lms/djangoapps/verify_student/toggles.py
View File

@@ -22,7 +22,6 @@ WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace(name='verify_student')
USE_NEW_EMAIL_TEMPLATES = WaffleFlag(
waffle_namespace=WAFFLE_FLAG_NAMESPACE,
flag_name='use_new_email_templates',
flag_undefined_default=False
)

1
openedx/core/djangoapps/models/config/waffle.py
View File

@@ -26,7 +26,6 @@ def waffle_flags():
COURSE_DETAIL_UPDATE_CERTIFICATE_DATE: CourseWaffleFlag(
waffle_namespace=COURSE_DETAIL_WAFFLE_NAMESPACE,
flag_name=COURSE_DETAIL_UPDATE_CERTIFICATE_DATE,
flag_undefined_default=False,
)
}

2
openedx/core/djangoapps/schedules/config.py
View File

@@ -14,13 +14,11 @@ WAFFLE_SWITCH_NAMESPACE = WaffleSwitchNamespace(name=u'schedules')
CREATE_SCHEDULE_WAFFLE_FLAG = CourseWaffleFlag(
waffle_namespace=WAFFLE_FLAG_NAMESPACE,
flag_name=u'create_schedules_for_course',
flag_undefined_default=False
)
COURSE_UPDATE_WAFFLE_FLAG = CourseWaffleFlag(
waffle_namespace=WAFFLE_FLAG_NAMESPACE,
flag_name=u'send_updates_for_course',
flag_undefined_default=False
)
DEBUG_MESSAGE_WAFFLE_FLAG = WaffleFlag(WAFFLE_FLAG_NAMESPACE, u'enable_debugging')

1
openedx/core/djangoapps/user_authn/views/register.py
View File

@@ -109,7 +109,6 @@ REGISTER_USER = Signal(providing_args=["user", "registration"])
REGISTRATION_FAILURE_LOGGING_FLAG = WaffleFlag(
waffle_namespace=WaffleFlagNamespace(name=u'registration'),
flag_name=u'enable_failure_logging',
flag_undefined_default=False
)

2
openedx/core/djangoapps/video_pipeline/config/waffle.py
View File

@@ -27,11 +27,9 @@ def waffle_flags():
ENABLE_DEVSTACK_VIDEO_UPLOADS: WaffleFlag(
waffle_namespace=namespace,
flag_name=ENABLE_DEVSTACK_VIDEO_UPLOADS,
flag_undefined_default=False
),
ENABLE_VEM_PIPELINE: CourseWaffleFlag(
waffle_namespace=namespace,
flag_name=ENABLE_VEM_PIPELINE,
flag_undefined_default=False
)
}

116
openedx/core/djangoapps/waffle_utils/__init__.py
View File

@@ -226,7 +226,7 @@ class WaffleFlagNamespace(six.with_metaclass(ABCMeta, WaffleNamespace)):
"""
return self._get_request_cache().setdefault('flags', {})
def is_flag_active(self, flag_name, check_before_waffle_callback=None, flag_undefined_default=None):
def is_flag_active(self, flag_name, check_before_waffle_callback=None):
"""
Returns and caches whether the provided flag is active.
@@ -239,6 +239,10 @@ class WaffleFlagNamespace(six.with_metaclass(ABCMeta, WaffleNamespace)):
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
alternatives.
Arguments:
flag_name (String): The name of the flag to check.
check_before_waffle_callback (function): (Optional) A function that
@@ -246,62 +250,55 @@ class WaffleFlagNamespace(six.with_metaclass(ABCMeta, WaffleNamespace)):
check_before_waffle_callback(namespaced_flag_name) returns True
or False, it is returned. If it returns None, then waffle is
used.
DEPRECATED flag_undefined_default (Boolean): A default value to be
returned if the waffle flag is to be checked, but doesn't exist.
See module docstring for alternative.
"""
# 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_metric(namespaced_flag_name, value)
return value
value = self._cached_flags.get(namespaced_flag_name)
if value is not None:
self._set_waffle_flag_metric(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_metric(namespaced_flag_name, value)
set_custom_metric('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_metric(namespaced_flag_name, value)
return value
def _is_flag_active_for_everyone(self, 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
if flag_undefined_default:
warnings.warn(
# NOTE: This will be removed once ARCHBOM-132, currently in-progress, is complete.
'flag_undefined_default has been deprecated. For existing uses this is already actively being fixed.',
DeprecationWarning
)
# validate arguments
namespaced_flag_name = self._namespaced_name(flag_name)
value = None
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:
is_flag_active_for_everyone = False
if flag_undefined_default is not None:
# determine if the flag is undefined in waffle
try:
waffle_flag = Flag.objects.get(name=namespaced_flag_name)
is_flag_active_for_everyone = (waffle_flag.everyone is True)
except Flag.DoesNotExist:
if flag_undefined_default:
# This metric will go away once this has been fully retired with ARCHBOM-132.
# Also, even though the value will only track the last flag, that should be enough.
set_custom_metric('temp_flag_default_used', namespaced_flag_name)
value = flag_undefined_default
if value is None:
request = crum.get_current_request()
if request:
value = flag_is_active(request, namespaced_flag_name)
else:
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 in Celery workers.
self._set_waffle_flag_metric(namespaced_flag_name, is_flag_active_for_everyone)
set_custom_metric('warn_flag_no_request_return_value', is_flag_active_for_everyone)
return is_flag_active_for_everyone
self._cached_flags[namespaced_flag_name] = value
self._set_waffle_flag_metric(namespaced_flag_name, value)
return value
try:
waffle_flag = Flag.objects.get(name=namespaced_flag_name)
return (waffle_flag.everyone is True)
except Flag.DoesNotExist:
return False
def _set_waffle_flag_metric(self, name, value):
"""
@@ -378,16 +375,14 @@ class WaffleFlag(object):
Represents a single waffle flag, using a cached waffle namespace.
"""
def __init__(self, waffle_namespace, flag_name, flag_undefined_default=None):
def __init__(self, waffle_namespace, flag_name):
"""
Initializes the waffle flag instance.
Arguments:
waffle_namespace (WaffleFlagNamespace | String): Namespace for this flag.
flag_name (String): The name of the flag (without namespacing).
DEPRECATED flag_undefined_default (Boolean): A default value to be returned
if the waffle flag is to be checked, but doesn't exist. See module
docstring for alternative.
"""
if isinstance(waffle_namespace, six.string_types):
waffle_namespace = WaffleFlagNamespace(name=waffle_namespace)
@@ -395,7 +390,6 @@ class WaffleFlag(object):
self.waffle_namespace = waffle_namespace
self.waffle_namespace = waffle_namespace
self.flag_name = flag_name
self.flag_undefined_default = flag_undefined_default
@property
def namespaced_flag_name(self):
@@ -408,10 +402,7 @@ class WaffleFlag(object):
"""
Returns whether or not the flag is enabled.
"""
return self.waffle_namespace.is_flag_active(
self.flag_name,
flag_undefined_default=self.flag_undefined_default
)
return self.waffle_namespace.is_flag_active(self.flag_name)
@contextmanager
def override(self, active=True):
@@ -476,7 +467,6 @@ class CourseWaffleFlag(WaffleFlag):
return self.waffle_namespace.is_flag_active(
self.flag_name,
check_before_waffle_callback=self._get_course_override_callback(course_key),
flag_undefined_default=self.flag_undefined_default
)
def is_enabled_without_course_context(self):

2
openedx/core/djangoapps/waffle_utils/docs/decisions/0001-refactor-waffle-flag-default.rst
View File

@@ -30,6 +30,8 @@ Consequences
* We will need to implement one of the alternate solutions for each flag currently using ``flag_undefined_default=True``, and then finally remove the deprecated ``flag_undefined_default`` argument.
UPDATE: This work has been completed and ``flag_undefined_default`` has been removed.
Rejected Alternatives
=====================

46
openedx/core/djangoapps/waffle_utils/tests/test_init.py
View File

@@ -94,19 +94,13 @@ class TestCourseWaffleFlag(TestCase):
@override_settings(WAFFLE_FLAG_CUSTOM_METRICS=[NAMESPACED_FLAG_NAME])
@patch('openedx.core.djangoapps.waffle_utils.set_custom_metric')
@ddt.data(
{'flag_undefined_default': None, 'result': False},
{'flag_undefined_default': False, 'result': False},
{'flag_undefined_default': True, 'result': True},
)
def test_undefined_waffle_flag(self, data, mock_set_custom_metric):
def test_undefined_waffle_flag(self, mock_set_custom_metric):
"""
Test flag with various defaults provided for undefined waffle flags.
Test flag with undefined waffle flag.
"""
test_course_flag = CourseWaffleFlag(
self.TEST_NAMESPACE,
self.FLAG_NAME,
flag_undefined_default=data['flag_undefined_default']
)
with patch(
@@ -119,8 +113,8 @@ class TestCourseWaffleFlag(TestCase):
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), data['result'])
self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), data['result'])
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,
@@ -129,26 +123,31 @@ class TestCourseWaffleFlag(TestCase):
self._assert_waffle_flag_metric(
mock_set_custom_metric,
expected_flag_value=str(data['result']),
flag_undefined_default=data['flag_undefined_default'],
expected_flag_value=str(False),
)
@ddt.data(
{'flag_undefined_default': None, 'result': False},
{'flag_undefined_default': False, 'result': False},
{'flag_undefined_default': True, 'result': True},
)
def test_without_request(self, data):
def test_without_request_and_undefined_waffle(self):
"""
Test the flag behavior when outside a request context.
Test the flag behavior when outside a request context and waffle data undefined.
"""
crum.set_current_request(None)
test_course_flag = CourseWaffleFlag(
self.TEST_NAMESPACE,
self.FLAG_NAME,
flag_undefined_default=data['flag_undefined_default']
)
self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), data['result'])
self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), False)
def test_without_request_and_everyone_active_waffle(self):
"""
Test the flag behavior when outside a request context and waffle active for everyone.
"""
crum.set_current_request(None)
test_course_flag = CourseWaffleFlag(
self.TEST_NAMESPACE,
self.FLAG_NAME,
)
with override_flag(self.NAMESPACED_FLAG_NAME, active=True):
self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), True)
@ddt.data(
{'expected_count': 0, 'waffle_flag_metric_setting': None},
@@ -169,13 +168,12 @@ class TestCourseWaffleFlag(TestCase):
self.assertEqual(mock_set_custom_metric.call_count, data['expected_count'])
def _assert_waffle_flag_metric(self, mock_set_custom_metric, expected_flag_value=None, flag_undefined_default=None):
def _assert_waffle_flag_metric(self, mock_set_custom_metric, expected_flag_value=None):
if expected_flag_value:
expected_flag_name = 'flag_{}'.format(self.NAMESPACED_FLAG_NAME)
expected_calls = [call(expected_flag_name, expected_flag_value)]
mock_set_custom_metric.assert_has_calls(expected_calls)
expected_call_count = 2 if flag_undefined_default else 1
self.assertEqual(mock_set_custom_metric.call_count, expected_call_count)
self.assertEqual(mock_set_custom_metric.call_count, 1)
else:
self.assertEqual(mock_set_custom_metric.call_count, 0)

2
openedx/features/course_experience/__init__.py
View File

@@ -22,7 +22,7 @@ class DefaultTrueWaffleFlagNamespace(WaffleFlagNamespace):
and refactor/fix any tests that shouldn't be removed.
"""
def is_flag_active(self, flag_name, check_before_waffle_callback=None, flag_undefined_default=None):
def is_flag_active(self, flag_name, check_before_waffle_callback=None):
"""
Overrides is_flag_active, and returns and caches whether the provided flag is active.

1
openedx/features/discounts/applicability.py
View File

@@ -40,7 +40,6 @@ from track import segment
DISCOUNT_APPLICABILITY_FLAG = WaffleFlag(
waffle_namespace=WaffleFlagNamespace(name=u'discounts'),
flag_name=u'enable_discounting',
flag_undefined_default=False
)
DISCOUNT_APPLICABILITY_HOLDBACK = 'first_purchase_discount_holdback'