From 51c5c24895d029a646e0ab00cce6f4b6bd942ba3 Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Mon, 26 Feb 2024 23:06:13 -0300 Subject: [PATCH 1/4] feat: handle optional blocks in verticals --- CHANGELOG.rst | 4 ++++ completion/__init__.py | 2 +- completion/services.py | 8 +++++++- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ea60aa9d..bd153061 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,6 +14,10 @@ Change Log Unreleased ~~~~~~~~~~ +[4.5.0] - 2024-02-26 +~~~~~~~~~~~~~~~~~~~~ +* Ignore optional blocks in non-optional verticals + [4.4.1] - 2023-10-27 ~~~~~~~~~~~~~~~~~~~~ * Fix RemovedInDjango41Warning by removing `django_app_config` diff --git a/completion/__init__.py b/completion/__init__.py index 8050a702..142bd91a 100644 --- a/completion/__init__.py +++ b/completion/__init__.py @@ -3,4 +3,4 @@ """ -__version__ = '4.4.1' +__version__ = '4.5.0' diff --git a/completion/services.py b/completion/services.py index 14174e28..b5fbd7d5 100644 --- a/completion/services.py +++ b/completion/services.py @@ -117,8 +117,14 @@ def vertical_is_complete(self, item): if not self.completion_tracking_enabled(): return None + optional_vertical = getattr(item, "optional_content", False) + # this is temporary local logic and will be removed when the whole course tree is included in completion - child_locations = [child.scope_ids.usage_id for child in self.get_completable_children(item)] + child_locations = [ + child.scope_ids.usage_id for child in self.get_completable_children(item) + # for non-optional verticals, only include non-optional children + if optional_vertical or not getattr(child, "optional_content", False) + ] completions = self.get_completions(child_locations) for child_location in child_locations: if completions[child_location] < 1.0: From aae6b92685ca2fdeb2fa99ea7677555372a574eb Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Thu, 29 Feb 2024 03:32:59 -0300 Subject: [PATCH 2/4] refactor: rename to optional completion --- completion/services.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/completion/services.py b/completion/services.py index b5fbd7d5..3effbd74 100644 --- a/completion/services.py +++ b/completion/services.py @@ -117,13 +117,14 @@ def vertical_is_complete(self, item): if not self.completion_tracking_enabled(): return None - optional_vertical = getattr(item, "optional_content", False) + optional_vertical = getattr(item, "optional_completion", False) # this is temporary local logic and will be removed when the whole course tree is included in completion child_locations = [ child.scope_ids.usage_id for child in self.get_completable_children(item) - # for non-optional verticals, only include non-optional children - if optional_vertical or not getattr(child, "optional_content", False) + # if vertical is optional, include all children + # else only include non-optional children + if optional_vertical or not getattr(child, "optional_completion", False) ] completions = self.get_completions(child_locations) for child_location in child_locations: From a1d90c631a84099c5ea663a9ea473d831c4ddf49 Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Thu, 29 Feb 2024 04:14:28 -0300 Subject: [PATCH 3/4] refactor: explain logic --- completion/services.py | 24 +++++++++++++++++++++--- completion/tests/test_services.py | 14 ++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/completion/services.py b/completion/services.py index 3effbd74..6f0ff4bc 100644 --- a/completion/services.py +++ b/completion/services.py @@ -103,6 +103,24 @@ def get_completable_children(self, node): user_children = [node] return user_children + @staticmethod + def matches_vertical_optional_completion(optional_vertical, optional_child): + """ + Checks if child should count towards a vertical's completion. + + There are only four combinations: + 1. Optional Vertical and Optional Child -> Include Child + 2. Optional Vertical and Required Child -> Include Child + 3. Required Vertical and Required Child -> Include Child + 4. Required Vertical and Optional Child -> Exclude Child + (case 2 shouldn't happen but this is how we can handle it gracefully) + + This means that the only case in which we want to exclude the child + is when the parent is required and the child isn't. + """ + required_vertical = not optional_vertical + return not (required_vertical and optional_child) # exclude case 4 from docstring + def vertical_is_complete(self, item): """ Calculates and returns whether a particular vertical is complete. @@ -122,9 +140,9 @@ def vertical_is_complete(self, item): # this is temporary local logic and will be removed when the whole course tree is included in completion child_locations = [ child.scope_ids.usage_id for child in self.get_completable_children(item) - # if vertical is optional, include all children - # else only include non-optional children - if optional_vertical or not getattr(child, "optional_completion", False) + if self.matches_vertical_optional_completion( + optional_vertical, getattr(child, "optional_completion", False) + ) ] completions = self.get_completions(child_locations) for child_location in child_locations: diff --git a/completion/tests/test_services.py b/completion/tests/test_services.py index 8eb5d2cd..893334ca 100644 --- a/completion/tests/test_services.py +++ b/completion/tests/test_services.py @@ -205,6 +205,20 @@ def test_blocks_to_mark_complete_on_view(self): [] ) + def test_matches_vertical_optional_completion(self): + for optional_vertical, optional_child, should_include_child in ( + (True, True, True), + (True, False, True), + (False, False, True), + (False, True, False), # do not count optional children for non-optional verticals + ): + self.assertEqual( + self.completion_service.matches_vertical_optional_completion( + optional_vertical, optional_child + ), + should_include_child, + ) + @ddt.ddt class CompletionDelayTestCase(CompletionSetUpMixin, TestCase): From 15f05cb8ed195a079bb3070baf5fec654260e78f Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Sat, 9 Mar 2024 17:17:23 -0300 Subject: [PATCH 4/4] refactor: use ddt in test and expand explanation --- completion/services.py | 3 +++ completion/tests/test_services.py | 27 ++++++++++++++------------- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/completion/services.py b/completion/services.py index 6f0ff4bc..f3fcc34d 100644 --- a/completion/services.py +++ b/completion/services.py @@ -108,6 +108,9 @@ def matches_vertical_optional_completion(optional_vertical, optional_child): """ Checks if child should count towards a vertical's completion. + This is done by comparing the "optional_completion" values of the vertical + and the child. Here optional completion means that the completion of a child + doesn't count towards the completion of a parent for the purposes of this library. There are only four combinations: 1. Optional Vertical and Optional Child -> Include Child 2. Optional Vertical and Required Child -> Include Child diff --git a/completion/tests/test_services.py b/completion/tests/test_services.py index 893334ca..546fb1d7 100644 --- a/completion/tests/test_services.py +++ b/completion/tests/test_services.py @@ -205,19 +205,20 @@ def test_blocks_to_mark_complete_on_view(self): [] ) - def test_matches_vertical_optional_completion(self): - for optional_vertical, optional_child, should_include_child in ( - (True, True, True), - (True, False, True), - (False, False, True), - (False, True, False), # do not count optional children for non-optional verticals - ): - self.assertEqual( - self.completion_service.matches_vertical_optional_completion( - optional_vertical, optional_child - ), - should_include_child, - ) + @ddt.data( + (True, True, True), + (True, False, True), + (False, False, True), + (False, True, False), # do not count optional children for non-optional verticals + ) + @ddt.unpack + def test_matches_vertical_optional_completion(self, optional_vertical, optional_child, should_include_child): + self.assertEqual( + self.completion_service.matches_vertical_optional_completion( + optional_vertical, optional_child + ), + should_include_child, + ) @ddt.ddt