diff --git a/AUTHORS b/AUTHORS index 8d566bd0..e08d8e2e 100644 --- a/AUTHORS +++ b/AUTHORS @@ -4,3 +4,4 @@ Jillian Vogel Michael Cornwell Paulo Viadanna Sofiane Bebert +Daniel Valenzuela diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b5ec8ff4..01002aca 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,6 +14,12 @@ Change Log Unreleased ~~~~~~~~~~ +[4.1.0] - 2024-02-17 +~~~~~~~~~~~~~~~~~~~~ + +* Include 'optional' boolean field in responses +* Exclude 'optional' completions from non-optional aggregations + [4.0.3] - 2023-10-24 ~~~~~~~~~~~~~~~~~~~~ diff --git a/completion_aggregator/__init__.py b/completion_aggregator/__init__.py index a9a396aa..a394c7f7 100644 --- a/completion_aggregator/__init__.py +++ b/completion_aggregator/__init__.py @@ -5,4 +5,4 @@ from __future__ import absolute_import, unicode_literals -__version__ = '4.0.3' +__version__ = '4.1.0' diff --git a/completion_aggregator/api/v1/views.py b/completion_aggregator/api/v1/views.py index 274549dd..2814138d 100644 --- a/completion_aggregator/api/v1/views.py +++ b/completion_aggregator/api/v1/views.py @@ -124,6 +124,7 @@ class CompletionListView(CompletionViewMixin, APIView): "earned: 20.0, "possible": 30.0, "ratio": 0.6666666666667 + "optional": false } }, { @@ -133,6 +134,7 @@ class CompletionListView(CompletionViewMixin, APIView): "earned: 22.0, "possible": 24.0, "ratio": 0.9166666666667 + "optional": false } } ] @@ -152,6 +154,7 @@ class CompletionListView(CompletionViewMixin, APIView): "earned: 12.0, "possible": 24.0, "ratio": 0.5 + "optional": false } } ] @@ -251,7 +254,6 @@ class CompletionDetailView(CompletionViewMixin, APIView): in the course. * percent (float in the range [0.0, 1.0]): The percent of possible completions in the course that have been earned by the learner. - Optional fields: * If "requested_fields" is specified, the response will include data @@ -271,6 +273,8 @@ class CompletionDetailView(CompletionViewMixin, APIView): * ratio (float in the range [0.0, 1.0]): The ratio of earned completions to possible completions within the identified block. + * optional (boolean): Whether the block counts towards its parents + completion in case the parent isn't optional. **Parameters** @@ -315,7 +319,8 @@ class CompletionDetailView(CompletionViewMixin, APIView): "completion": { "earned": 12.0, "possible": 24.0, - "ratio": 0.5 + "ratio": 0.5, + "optional": false }, "mean": 0.25, "chapter": [ @@ -325,7 +330,8 @@ class CompletionDetailView(CompletionViewMixin, APIView): "completion": { "earned: 12.0, "possible": 24.0, - "ratio": 0.5 + "ratio": 0.5, + "optional": false } } ], @@ -337,7 +343,8 @@ class CompletionDetailView(CompletionViewMixin, APIView): "completion": { "earned: 12.0, "possible": 12.0, - "ratio": 1.0 + "ratio": 1.0, + "optional": false } }, { @@ -347,7 +354,8 @@ class CompletionDetailView(CompletionViewMixin, APIView): "completion": { "earned: 0.0, "possible": 12.0, - "ratio": 0.0 + "ratio": 0.0, + "optional": false } }, }, diff --git a/completion_aggregator/compat.py b/completion_aggregator/compat.py index 38b3bf31..fd36e345 100644 --- a/completion_aggregator/compat.py +++ b/completion_aggregator/compat.py @@ -136,6 +136,17 @@ def get_block_aggregators(course_blocks, block): ) or [] +def is_block_optional(course_blocks, block): + """ + Return whether a block is optional or not. + """ + return course_blocks.get_transformer_block_field( + block, + AggregatorAnnotationTransformer, + AggregatorAnnotationTransformer.OPTIONAL_COMPLETION + ) + + def get_mobile_only_courses(enrollments): """ Return list of courses with mobile available given a list of enrollments. diff --git a/completion_aggregator/core.py b/completion_aggregator/core.py index 23f4ddf5..689c2481 100644 --- a/completion_aggregator/core.py +++ b/completion_aggregator/core.py @@ -28,7 +28,7 @@ UPDATER_CACHE_TIMEOUT = 600 # 10 minutes CacheEntry = namedtuple('CacheEntry', ['course_blocks', 'root_block']) -CompletionStats = namedtuple('CompletionStats', ['earned', 'possible', 'last_modified']) +CompletionStats = namedtuple('CompletionStats', ['earned', 'possible', 'last_modified', 'optional']) log = logging.getLogger(__name__) @@ -84,7 +84,7 @@ def cache_key(self): ) -CourseBlocksEntry = namedtuple('CourseBlocksEntry', ['children', 'aggregators']) +CourseBlocksEntry = namedtuple('CourseBlocksEntry', ['children', 'aggregators', 'optional']) class AggregationUpdater: @@ -139,7 +139,8 @@ def format_course_blocks(self, course_blocks, root_block): { block_key: CourseBlocksEntry( children=block_key[], - aggregators=block_key[] + aggregators=block_key[], + optional=boolean, ) } @@ -151,6 +152,7 @@ def populate(structure, block): structure[block] = CourseBlocksEntry( children=compat.get_children(course_blocks, block), aggregators=compat.get_block_aggregators(course_blocks, block), + optional=compat.is_block_optional(course_blocks, block), ) for child in structure[block].children: populate(structure, child) @@ -215,7 +217,7 @@ def update(self, changed_blocks=frozenset(), force=False): Aggregator.objects.bulk_create_or_update(updated_aggregators) self.resolve_stale_completions(changed_blocks, start) - def update_for_block(self, block, affected_aggregators, force=False): + def update_for_block(self, block, affected_aggregators, force=False, optional_branch=False): """ Recursive function to perform updates for a given block. @@ -229,13 +231,13 @@ def update_for_block(self, block, affected_aggregators, force=False): if mode == XBlockCompletionMode.EXCLUDED: return self.update_for_excluded() elif mode == XBlockCompletionMode.COMPLETABLE: - return self.update_for_completable(block) + return self.update_for_completable(block, optional_branch) elif mode == XBlockCompletionMode.AGGREGATOR: - return self.update_for_aggregator(block, affected_aggregators, force) + return self.update_for_aggregator(block, affected_aggregators, force, optional_branch) else: raise ValueError(f"Invalid completion mode {mode}") - def update_for_aggregator(self, block, affected_aggregators, force): + def update_for_aggregator(self, block, affected_aggregators, force, optional_branch=False): """ Calculate the new completion values for an aggregator. """ @@ -243,12 +245,19 @@ def update_for_aggregator(self, block, affected_aggregators, force): total_possible = 0.0 last_modified = OLD_DATETIME + optional = self.course_blocks[block].optional or optional_branch if block not in affected_aggregators: obj = self.aggregators.get(block) if obj: - return CompletionStats(earned=obj.earned, possible=obj.possible, last_modified=obj.last_modified) + return CompletionStats( + earned=obj.earned, possible=obj.possible, last_modified=obj.last_modified, optional=optional + ) for child in self.course_blocks[block].children: - (earned, possible, modified) = self.update_for_block(child, affected_aggregators, force) + (earned, possible, modified, is_optional_child) = self.update_for_block( + child, affected_aggregators, force, optional_branch=optional + ) + if is_optional_child and not optional: + continue # Optional children shouldn't count towards non-optional parents aggregations total_earned += earned total_possible += possible if modified is not None: @@ -270,6 +279,7 @@ def update_for_aggregator(self, block, affected_aggregators, force): percent=percent, last_modified=last_modified, modified=timezone.now(), + optional=optional, ) self.aggregators[block] = aggregator else: @@ -279,27 +289,33 @@ def update_for_aggregator(self, block, affected_aggregators, force): aggregator.percent = percent aggregator.last_modified = last_modified aggregator.modified = timezone.now() + aggregator.optional = optional self.updated_aggregators.append(aggregator) - return CompletionStats(earned=total_earned, possible=total_possible, last_modified=last_modified) + return CompletionStats( + earned=total_earned, possible=total_possible, last_modified=last_modified, optional=optional + ) def update_for_excluded(self): """ Return a sentinel empty completion value for excluded blocks. """ - return CompletionStats(earned=0.0, possible=0.0, last_modified=OLD_DATETIME) + return CompletionStats(earned=0.0, possible=0.0, last_modified=OLD_DATETIME, optional=False) - def update_for_completable(self, block): + def update_for_completable(self, block, optional_branch=False): """ Return the block completion value for a given completable block. """ completion = self.block_completions.get(block) + optional = self.course_blocks[block].optional if completion: earned = completion.completion last_modified = completion.modified else: earned = 0.0 last_modified = OLD_DATETIME - return CompletionStats(earned=earned, possible=1.0, last_modified=last_modified) + return CompletionStats( + earned=earned, possible=1.0, last_modified=last_modified, optional=optional or optional_branch + ) def _aggregator_needs_update(self, block, modified, force): """ diff --git a/completion_aggregator/migrations/0006_aggregator_optional.py b/completion_aggregator/migrations/0006_aggregator_optional.py new file mode 100644 index 00000000..459184f5 --- /dev/null +++ b/completion_aggregator/migrations/0006_aggregator_optional.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.24 on 2024-02-16 00:11 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('completion_aggregator', '0005_cachegroupinvalidation'), + ] + + operations = [ + migrations.AddField( + model_name='aggregator', + name='optional', + field=models.BooleanField(default=False), + ), + ] diff --git a/completion_aggregator/models.py b/completion_aggregator/models.py index c1fde462..92c67d8e 100644 --- a/completion_aggregator/models.py +++ b/completion_aggregator/models.py @@ -23,16 +23,17 @@ INSERT_OR_UPDATE_AGGREGATOR_QUERY = """ INSERT INTO completion_aggregator_aggregator - (user_id, course_key, block_key, aggregation_name, earned, possible, percent, last_modified, created, modified) + (user_id, course_key, block_key, aggregation_name, earned, possible, percent, last_modified, created, modified, optional) VALUES (%(user)s, %(course_key)s, %(block_key)s, %(aggregation_name)s, %(earned)s, - %(possible)s, %(percent)s, %(last_modified)s, %(created)s, %(modified)s) + %(possible)s, %(percent)s, %(last_modified)s, %(created)s, %(modified)s, %(optional)s) ON DUPLICATE KEY UPDATE earned=VALUES(earned), possible=VALUES(possible), percent=VALUES(percent), last_modified=VALUES(last_modified), - modified=VALUES(modified); + modified=VALUES(modified), + optional=VALUES(optional); """ @@ -100,7 +101,7 @@ def pre_save(sender, instance, **kwargs): # pylint: disable=unused-argument instance.full_clean() def submit_completion(self, user, course_key, block_key, aggregation_name, earned, possible, - last_modified): + last_modified, optional=False): """ Insert and Update the completion Aggregator for the specified record. @@ -169,6 +170,7 @@ def submit_completion(self, user, course_key, block_key, aggregation_name, earne 'possible': possible, 'earned': earned, 'last_modified': last_modified, + 'optional': optional, }, ) return obj, is_new @@ -190,6 +192,7 @@ def bulk_create_or_update(self, updated_aggregators): possible=aggregator.possible, earned=aggregator.earned, last_modified=aggregator.last_modified, + optional=aggregator.optional, ) else: aggregation_data = [obj.get_values() for obj in updated_aggregators] @@ -211,6 +214,7 @@ class Aggregator(TimeStampedModel): possible = models.FloatField(validators=[validate_positive_float]) percent = models.FloatField(validators=[validate_percent]) last_modified = models.DateTimeField() + optional = models.BooleanField(default=False) objects = AggregatorManager() @@ -239,7 +243,7 @@ def get_values(self): Return a dict object containing fields and their values to be used in bulk create or update query. """ values = {key: getattr(self, key) for key in [ - 'course_key', 'block_key', 'aggregation_name', 'earned', 'possible', + 'course_key', 'block_key', 'aggregation_name', 'earned', 'possible', 'optional' ]} values['user'] = self.user.id values['percent'] = get_percent(values['earned'], values['possible']) diff --git a/completion_aggregator/serializers.py b/completion_aggregator/serializers.py index 0a4b90e0..ff784f09 100644 --- a/completion_aggregator/serializers.py +++ b/completion_aggregator/serializers.py @@ -174,6 +174,7 @@ def course(self): earned=0.0, possible=None, percent=0.0, + optional=False, ) @property @@ -210,6 +211,7 @@ class _CompletionSerializer(serializers.Serializer): earned = serializers.FloatField() possible = serializers.FloatField() percent = serializers.FloatField() + optional = serializers.BooleanField(default=False) class _CompletionSerializerV0(_CompletionSerializer): diff --git a/completion_aggregator/transformers.py b/completion_aggregator/transformers.py index 0ba63667..863896f3 100644 --- a/completion_aggregator/transformers.py +++ b/completion_aggregator/transformers.py @@ -18,6 +18,7 @@ class AggregatorAnnotationTransformer(BlockStructureTransformer): READ_VERSION = 1 WRITE_VERSION = 1 AGGREGATORS = "aggregators" + OPTIONAL_COMPLETION = "optional_completion" @classmethod def name(cls): @@ -48,23 +49,27 @@ def collect(cls, block_structure): """ Collect the data required to perform this calculation. """ - block_structure.request_xblock_fields("completion_mode") + block_structure.request_xblock_fields("completion_mode", "optional_completion") def calculate_aggregators(self, block_structure, block_key): """ Calculate the set of aggregators for the specified block. """ aggregators = set() + optional = False parents = block_structure.get_parents(block_key) for parent in parents: parent_block = block_structure[parent] completion_mode = getattr(parent_block, 'completion_mode', XBlockCompletionMode.COMPLETABLE) + optional_parent = getattr(parent_block, 'optional_completion', False) if completion_mode == XBlockCompletionMode.EXCLUDED: continue if completion_mode == XBlockCompletionMode.AGGREGATOR: aggregators.add(parent) + if optional_parent: + optional = True aggregators.update(self.get_block_aggregators(block_structure, parent)) - return aggregators + return aggregators, optional def transform(self, usage_info, block_structure): # pylint: disable=unused-argument """ @@ -77,5 +82,6 @@ def transform(self, usage_info, block_structure): # pylint: disable=unused-argu XBlockCompletionMode.COMPLETABLE ) if completion_mode != XBlockCompletionMode.EXCLUDED: - aggregators = self.calculate_aggregators(block_structure, block_key) + aggregators, optional = self.calculate_aggregators(block_structure, block_key) block_structure.set_transformer_block_field(block_key, self, self.AGGREGATORS, aggregators) + block_structure.set_transformer_block_field(block_key, self, self.OPTIONAL_COMPLETION, optional) diff --git a/test_utils/compat.py b/test_utils/compat.py index 8144cf2d..c1ac0ae3 100644 --- a/test_utils/compat.py +++ b/test_utils/compat.py @@ -56,6 +56,12 @@ def get_block_aggregators(self, course_blocks, block): return [agg for agg in course_blocks.blocks if block.block_id.startswith(f'{agg.block_id}-')] + def is_block_optional(self, _, block): + """ + Return whether a block is optional or not. + """ + return "optional" in block.block_id or "optional" in block.run + def get_block_completions(self, user, course_key): """ Return all completions for the current course. diff --git a/tests/test_core.py b/tests/test_core.py index 3fead01d..4b288f19 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -33,6 +33,7 @@ class AggregationUpdaterTestCase(TestCase): It should create Aggregator records for new completion objects. """ + def setUp(self): """ For the purpose of the tests, we will use the following course @@ -63,15 +64,16 @@ def setUp(self): course_key.make_usage_key('hidden', 'course-hidden0'), course_key.make_usage_key('html', 'course-other-html4'), course_key.make_usage_key('hidden', 'course-other-hidden1'), + course_key.make_usage_key('html', 'course-other-htmloptional5'), ]) for compat_module in 'completion_aggregator.core.compat', 'completion_aggregator.core.compat': patch = mock.patch(compat_module, stubcompat) patch.start() self.addCleanup(patch.stop) - user = get_user_model().objects.create(username='saskia') + self.user = get_user_model().objects.create(username='saskia') self.course_key = CourseKey.from_string('course-v1:edx+course+test') self.agg, _ = Aggregator.objects.submit_completion( - user=user, + user=self.user, course_key=self.course_key, block_key=self.course_key.make_usage_key('course', 'course'), aggregation_name='course', @@ -80,13 +82,13 @@ def setUp(self): last_modified=self.agg_modified, ) BlockCompletion.objects.create( - user=user, + user=self.user, context_key=self.course_key, block_key=self.course_key.make_usage_key('html', 'course-other-html4'), completion=1.0, modified=now(), ) - self.updater = AggregationUpdater(user, self.course_key, mock.MagicMock()) + self.updater = AggregationUpdater(self.user, self.course_key, mock.MagicMock()) @XBlock.register_temp_plugin(CourseBlock, 'course') @XBlock.register_temp_plugin(HTMLBlock, 'html') @@ -191,6 +193,78 @@ def test_unexpected_updater_errors(self): course_key='course-v1:OpenCraft+Onboarding+2018' ) + @XBlock.register_temp_plugin(CourseBlock, 'course') + @XBlock.register_temp_plugin(HTMLBlock, 'html') + @XBlock.register_temp_plugin(HiddenBlock, 'hidden') + @XBlock.register_temp_plugin(OtherAggBlock, 'other') + def test_optional_child_block_not_counted(self): + optional_block = BlockCompletion.objects.create( + user=self.user, + context_key=self.course_key, + block_key=self.course_key.make_usage_key('html', 'course-optional-html5'), + completion=0.1, + modified=now(), + ) + self.updater.update() + self.agg.refresh_from_db() + assert self.agg.last_modified > self.agg_modified + assert self.agg.earned == 1.0 + assert self.agg.possible == 5.0 + optional_block.delete() + + @XBlock.register_temp_plugin(CourseBlock, 'course') + @XBlock.register_temp_plugin(HTMLBlock, 'html') + @XBlock.register_temp_plugin(HiddenBlock, 'hidden') + @XBlock.register_temp_plugin(OtherAggBlock, 'other') + def test_optional_child_block_counted(self): + agg_modified = now() - timedelta(days=1) + course_key = CourseKey.from_string('course-v1:edx+course-optional+test-optional') + stubcompat = StubCompat([ + course_key.make_usage_key('course', 'course-optional'), + course_key.make_usage_key('html', 'course-html0'), + course_key.make_usage_key('html', 'course-html1'), + course_key.make_usage_key('html', 'course-html2'), + course_key.make_usage_key('html', 'course-html3'), + course_key.make_usage_key('other', 'course-other'), + course_key.make_usage_key('hidden', 'course-hidden0'), + course_key.make_usage_key('html', 'course-other-html4'), + course_key.make_usage_key('html', 'course-other-optional'), + course_key.make_usage_key('hidden', 'course-other-hidden1'), + ]) + for compat_module in 'completion_aggregator.core.compat', 'completion_aggregator.core.compat': + patch = mock.patch(compat_module, stubcompat) + patch.start() + agg, _ = Aggregator.objects.submit_completion( + user=self.user, + course_key=course_key, + block_key=course_key.make_usage_key('course', 'course'), + aggregation_name='course', + earned=0.0, + possible=0.0, + last_modified=agg_modified, + ) + BlockCompletion.objects.create( + user=self.user, + context_key=course_key, + block_key=course_key.make_usage_key('html', 'course-other-html4'), + completion=1.0, + modified=now(), + ) + BlockCompletion.objects.create( + user=self.user, + context_key=course_key, + block_key=course_key.make_usage_key('html', 'course-other-optional'), + completion=0.1, + modified=now(), + ) + updater = AggregationUpdater(self.user, course_key, mock.MagicMock()) + updater.update() + agg.refresh_from_db() + assert agg.last_modified > agg_modified + assert agg.earned == 1.1 + assert agg.possible == 6.0 + patch.stop() + class CalculateUpdatedAggregatorsTestCase(TestCase): """ @@ -450,6 +524,7 @@ class PartialUpdateTest(TestCase): Test that when performing an update for a particular block or subset of blocks, that only part of the course tree gets aggregated. """ + def setUp(self): super().setUp() self.user = get_user_model().objects.create() diff --git a/tests/test_serializers.py b/tests/test_serializers.py index 54e2341a..747f7536 100644 --- a/tests/test_serializers.py +++ b/tests/test_serializers.py @@ -265,6 +265,7 @@ def test_zero_possible(self): 'earned': 0.0, 'possible': 0.0, 'percent': 1.0, + 'optional': False, }, ) @@ -343,6 +344,7 @@ def test_aggregation_without_completions(self): 'earned': 0.0, 'possible': None, 'percent': 0.0, + 'optional': False, }, ) diff --git a/tests/test_views.py b/tests/test_views.py index 0491c7c8..6d6409bd 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -184,6 +184,7 @@ def setUp(self): self.course_key.make_usage_key('html', 'course-sequence2-html6'), self.course_key.make_usage_key('html', 'course-sequence2-html7'), self.course_key.make_usage_key('html', 'course-sequence2-html8'), + self.course_key.make_usage_key('html', 'course-optional-sequence2-html9'), ] compat = StubCompat(self.blocks) for compat_import in ( @@ -210,7 +211,7 @@ def setUp(self): self.client = APIClient() self.client.force_authenticate(user=self.test_user) - def _get_expected_completion(self, version, earned=1.0, possible=8.0, percent=0.125): + def _get_expected_completion(self, version, earned=1.0, possible=8.0, percent=0.125, optional=False): """ Return completion section based on version. """ @@ -218,6 +219,7 @@ def _get_expected_completion(self, version, earned=1.0, possible=8.0, percent=0. 'earned': earned, 'possible': possible, 'percent': percent, + 'optional': optional, } if version == 0: completion['ratio'] = percent @@ -458,6 +460,7 @@ def test_detail_view_root_block(self): 'earned': 0.0, 'possible': None, 'percent': 0.0, + 'optional': False }, 'course_key': six.text_type(self.course_key), 'sequential': [ @@ -468,6 +471,7 @@ def test_detail_view_root_block(self): 'earned': 1.0, 'possible': 5.0, 'percent': 0.2, + 'optional': False, } }, ],