From 721282233e1509e3f7b11d81bdcae07147b618a5 Mon Sep 17 00:00:00 2001 From: Michael Terry Date: Tue, 27 Jul 2021 10:58:10 -0400 Subject: [PATCH] feat: enable effort estimation feature by default Previously, effort estimation was disabled by default and gated by an experiment flag (effort_estimation.location). Now, it is enabled by default and courses can opt-out by adding course waffle override flags (effort_estimation.disabled). --- .../effort_estimation/block_transformers.py | 28 +++--------- .../tests/test_block_transformers.py | 45 +++++-------------- openedx/features/effort_estimation/toggles.py | 18 ++++---- 3 files changed, 27 insertions(+), 64 deletions(-) diff --git a/openedx/features/effort_estimation/block_transformers.py b/openedx/features/effort_estimation/block_transformers.py index b734214dfb..11efd6fba7 100644 --- a/openedx/features/effort_estimation/block_transformers.py +++ b/openedx/features/effort_estimation/block_transformers.py @@ -13,7 +13,7 @@ from edxval.api import get_videos_for_course from openedx.core.djangoapps.content.block_structure.transformer import BlockStructureTransformer from openedx.core.lib.mobile_utils import is_request_from_mobile_app -from .toggles import EFFORT_ESTIMATION_LOCATION_FLAG +from .toggles import EFFORT_ESTIMATION_DISABLED_FLAG class EffortEstimationTransformer(BlockStructureTransformer): @@ -121,6 +121,10 @@ class EffortEstimationTransformer(BlockStructureTransformer): block_structure.set_transformer_block_field(block_key, cls, cls.VIDEO_CLIP_DURATION, clip_duration) def transform(self, usage_info, block_structure): + # Early exit if our per-course opt-out flag is enabled + if EFFORT_ESTIMATION_DISABLED_FLAG.is_enabled(block_structure.root_block_usage_key.course_key): + return + # Skip any transformation if our collection phase said to cls = EffortEstimationTransformer if block_structure.get_transformer_data(cls, cls.DISABLE_ESTIMATION, default=False): @@ -154,28 +158,6 @@ class EffortEstimationTransformer(BlockStructureTransformer): if activities is not None: block_structure.override_xblock_field(block_key, self.EFFORT_ACTIVITIES, activities) - # Get bucket for this experiment. 0 is no estimate. 1 is only on sections. 2 is only on subsections. - # For cleanup ticket AA-659: remove everything below. - - # We only want to get the bucket if there is data available - i.e. there is something to actually experiment - # on. This helps avoid rollout issues where we don't want to claim a user is in bucket 1 if we haven't even - # re-published the course so that it has any estimation data available. - root_key = block_structure.root_block_usage_key - total_activities = block_structure.get_xblock_field(root_key, self.EFFORT_ACTIVITIES) - total_time = block_structure.get_xblock_field(root_key, self.EFFORT_TIME) - if not total_activities and not total_time: - return - - # Second pass to clear out collected estimate on levels we don't want to share. Just an easy way to test - # estimates at different levels, per the experiment. - bucket = EFFORT_ESTIMATION_LOCATION_FLAG.get_bucket(course_key=block_structure.root_block_usage_key.course_key) - for block_key in block_structure.post_order_traversal(): - category = block_structure.get_xblock_field(block_key, 'category') - allowed = (bucket == 1 and category == 'chapter') or (bucket == 2 and category == 'sequential') - if not allowed: - block_structure.override_xblock_field(block_key, self.EFFORT_TIME, None) - block_structure.override_xblock_field(block_key, self.EFFORT_ACTIVITIES, None) - @cached_property def _is_on_mobile(self): """Returns whether the current request is from our mobile app.""" diff --git a/openedx/features/effort_estimation/tests/test_block_transformers.py b/openedx/features/effort_estimation/tests/test_block_transformers.py index 42dd51f25a..27f7d6565b 100644 --- a/openedx/features/effort_estimation/tests/test_block_transformers.py +++ b/openedx/features/effort_estimation/tests/test_block_transformers.py @@ -1,20 +1,19 @@ """Tests for effort_estimation transformers.""" from datetime import timedelta -from unittest.mock import patch from crum import set_current_request from django.test.client import RequestFactory +from edx_toggles.toggles.testutils import override_waffle_flag from edxval.api import create_video, remove_video_for_course -from lms.djangoapps.experiments.testutils import override_experiment_waffle_flag from openedx.core.djangoapps.content.block_structure.factory import BlockStructureFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import SampleCourseFactory from xmodule.modulestore.tests.sample_courses import BlockInfo from ..block_transformers import EffortEstimationTransformer -from ..toggles import EFFORT_ESTIMATION_LOCATION_FLAG +from ..toggles import EFFORT_ESTIMATION_DISABLED_FLAG # Copied here, rather than used directly from class, just to catch any accidental changes @@ -108,9 +107,8 @@ class TestEffortEstimationTransformer(ModuleStoreTestCase): def get_collection_field(self, key, name): return self.block_structure.get_transformer_block_field(key, EffortEstimationTransformer, name) - def test_collection(self): - self.collect() - + def assert_collected(self): + """Confirm we at least collected the data (but not necessarily that we injected that data into block tree)""" assert self.get_collection_field(self.video_clip_key, VIDEO_DURATION) == 200 assert self.get_collection_field(self.video_clip_key, VIDEO_CLIP_DURATION) == 40 assert self.get_collection_field(self.video_normal_key, VIDEO_DURATION) == 30 @@ -121,7 +119,10 @@ class TestEffortEstimationTransformer(ModuleStoreTestCase): assert self.block_structure.get_transformer_data(EffortEstimationTransformer, DISABLE_ESTIMATION) is None - @override_experiment_waffle_flag(EFFORT_ESTIMATION_LOCATION_FLAG, bucket=1) + def test_collection(self): + self.collect() + self.assert_collected() + def test_incomplete_data_collection(self): """Ensure that missing video data prevents any estimates from being generated""" remove_video_for_course(str(self.course_key), 'edxval3') @@ -134,44 +135,22 @@ class TestEffortEstimationTransformer(ModuleStoreTestCase): assert self.block_structure.get_xblock_field(self.subsection_key, EFFORT_ACTIVITIES) is None assert self.block_structure.get_xblock_field(self.subsection_key, EFFORT_TIME) is None - @override_experiment_waffle_flag(EFFORT_ESTIMATION_LOCATION_FLAG, bucket=0) - def test_control_bucket(self): + @override_waffle_flag(EFFORT_ESTIMATION_DISABLED_FLAG, True) + def test_disabled(self): self.collect_and_transform() + self.assert_collected() assert self.block_structure.get_xblock_field(self.section_key, EFFORT_ACTIVITIES) is None assert self.block_structure.get_xblock_field(self.section_key, EFFORT_TIME) is None assert self.block_structure.get_xblock_field(self.subsection_key, EFFORT_ACTIVITIES) is None assert self.block_structure.get_xblock_field(self.subsection_key, EFFORT_TIME) is None - @override_experiment_waffle_flag(EFFORT_ESTIMATION_LOCATION_FLAG, bucket=1) - def test_section_bucket(self): + def test_enabled(self): self.collect_and_transform() assert self.block_structure.get_xblock_field(self.section_key, EFFORT_ACTIVITIES) == 1 assert self.block_structure.get_xblock_field(self.section_key, EFFORT_TIME) == 121 - assert self.block_structure.get_xblock_field(self.subsection_key, EFFORT_ACTIVITIES) is None - assert self.block_structure.get_xblock_field(self.subsection_key, EFFORT_TIME) is None - - @override_experiment_waffle_flag(EFFORT_ESTIMATION_LOCATION_FLAG, bucket=2) - def test_subsection_bucket(self): - self.collect_and_transform() - assert self.block_structure.get_xblock_field(self.section_key, EFFORT_ACTIVITIES) is None - assert self.block_structure.get_xblock_field(self.section_key, EFFORT_TIME) is None assert self.block_structure.get_xblock_field(self.subsection_key, EFFORT_ACTIVITIES) == 1 assert self.block_structure.get_xblock_field(self.subsection_key, EFFORT_TIME) == 121 - def test_no_collection_no_bucket(self): - """ - Test that if we don't have any collection data, we don't bucket at all. - - Useful to make sure that during rollout before we re-publish a course, we aren't trying to bucket anyone. - """ - where = 'openedx.features.effort_estimation.toggles.EFFORT_ESTIMATION_LOCATION_FLAG.get_bucket' - with patch(where, return_value=1) as mock_get_bucket: - self.transform() # no collection - assert self.block_structure.get_xblock_field(self.section_key, EFFORT_ACTIVITIES) is None - assert self.block_structure.get_xblock_field(self.section_key, EFFORT_TIME) is None - assert mock_get_bucket.call_count == 0 - - @override_experiment_waffle_flag(EFFORT_ESTIMATION_LOCATION_FLAG, bucket=1) def test_mobile_video_support(self): """Clips values are ignored and web only videos should be excluded""" self.set_mobile_request() diff --git a/openedx/features/effort_estimation/toggles.py b/openedx/features/effort_estimation/toggles.py index 0dbbb16076..be1d67acd5 100644 --- a/openedx/features/effort_estimation/toggles.py +++ b/openedx/features/effort_estimation/toggles.py @@ -1,17 +1,19 @@ """ -Feature/experiment toggles used for effort estimation. +Feature toggles used for effort estimation. """ from edx_toggles.toggles import LegacyWaffleFlagNamespace -from lms.djangoapps.experiments.flags import ExperimentWaffleFlag +from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag WAFFLE_FLAG_NAMESPACE = LegacyWaffleFlagNamespace(name='effort_estimation') -# Temporary flag while we test which location works best: -# - Bucket 0: off -# - Bucket 1: section (chapter) estimations -# - Bucket 2: subsection (sequential) estimations -EFFORT_ESTIMATION_LOCATION_FLAG = ExperimentWaffleFlag(WAFFLE_FLAG_NAMESPACE, 'location', __name__, num_buckets=3, # lint-amnesty, pylint: disable=toggle-missing-annotation - use_course_aware_bucketing=False) +# .. toggle_name: effort_estimation.disabled +# .. toggle_implementation: CourseWaffleFlag +# .. toggle_default: False +# .. toggle_description: If effort estimations are confusing for a given course (e.g. the course team has added manual +# estimates), you can turn them off case by case here. +# .. toggle_use_cases: opt_out +# .. toggle_creation_date: 2021-07-27 +EFFORT_ESTIMATION_DISABLED_FLAG = CourseWaffleFlag(WAFFLE_FLAG_NAMESPACE, 'disabled', __name__)