From 4b7697dd41bae2c9859a9e8bce1827d9b2326ddd Mon Sep 17 00:00:00 2001 From: ozer550 Date: Wed, 14 Aug 2024 20:57:54 +0530 Subject: [PATCH 1/2] add validation for threshold none invalid cases --- .../tests/viewsets/test_contentnode.py | 43 ++++++++++++++++++- .../contentcuration/utils/nodes.py | 10 +++++ .../contentcuration/viewsets/contentnode.py | 5 +++ 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/contentcuration/contentcuration/tests/viewsets/test_contentnode.py b/contentcuration/contentcuration/tests/viewsets/test_contentnode.py index 48c5ead8ac..99a508e0ca 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_contentnode.py +++ b/contentcuration/contentcuration/tests/viewsets/test_contentnode.py @@ -36,7 +36,6 @@ from contentcuration.viewsets.sync.constants import CONTENTNODE_PREREQUISITE from contentcuration.viewsets.sync.constants import UPDATED - nested_subjects = [subject for subject in SUBJECTSLIST if "." in subject] @@ -816,6 +815,48 @@ def test_update_contentnode_update_options_completion_criteria_threshold_only(se self.assertEqual(c.extra_fields["options"]["completion_criteria"]["model"], completion_criteria.TIME) self.assertEqual(c.extra_fields["options"]["completion_criteria"]["threshold"], 10) + def test_update_completion_criteria_model_to_determined_by_resource_edge_case(self): + metadata = self.contentnode_db_metadata + metadata["kind_id"] = content_kinds.HTML5 + metadata["extra_fields"] = { + "options": { + "completion_criteria": { + "model": completion_criteria.REFERENCE, + "threshold": None, + "learner_managed": False + } + } + } + contentnode = models.ContentNode.objects.create(**metadata) + + response = self.sync_changes( + [ + generate_update_event( + contentnode.id, + CONTENTNODE, + { + "complete": True, + "extra_fields.options.completion_criteria.threshold": 600, + "extra_fields.options.completion_criteria.model": completion_criteria.APPROX_TIME + }, + channel_id=self.channel.id + ), + generate_update_event( + contentnode.id, + CONTENTNODE, + { + "extra_fields.options.completion_criteria.model": completion_criteria.DETERMINED_BY_RESOURCE + }, + channel_id=self.channel.id + ) + ], + ) + + self.assertEqual(len(response.data["errors"]), 0) + updated_contentnode = models.ContentNode.objects.get(id=contentnode.id) + self.assertEqual(updated_contentnode.extra_fields["options"]["completion_criteria"]["model"], completion_criteria.DETERMINED_BY_RESOURCE) + self.assertNotIn("threshold", updated_contentnode.extra_fields["options"]["completion_criteria"]) + def test_update_contentnode_update_options_invalid_completion_criteria(self): metadata = self.contentnode_db_metadata metadata["extra_fields"] = { diff --git a/contentcuration/contentcuration/utils/nodes.py b/contentcuration/contentcuration/utils/nodes.py index 61491305d1..37dce5a60f 100644 --- a/contentcuration/contentcuration/utils/nodes.py +++ b/contentcuration/contentcuration/utils/nodes.py @@ -453,3 +453,13 @@ def migrate_extra_fields(extra_fields): "model": completion_criteria.MASTERY, } return extra_fields + + +def validate_and_conform_to_schema_threshold_none(validated_options): + completion_criteria_validated = validated_options.get("completion_criteria", {}) + model = completion_criteria_validated.get("model", {}) + if model in ["reference", "determined_by_resource"]: + if "threshold" not in completion_criteria_validated or completion_criteria_validated["threshold"] is not None: + completion_criteria_validated["threshold"] = None + validated_options["completion_criteria"] = completion_criteria_validated + return validated_options diff --git a/contentcuration/contentcuration/viewsets/contentnode.py b/contentcuration/contentcuration/viewsets/contentnode.py index b15b717860..fde1296fc5 100644 --- a/contentcuration/contentcuration/viewsets/contentnode.py +++ b/contentcuration/contentcuration/viewsets/contentnode.py @@ -58,6 +58,7 @@ from contentcuration.utils.nodes import calculate_resource_size from contentcuration.utils.nodes import migrate_extra_fields from contentcuration.utils.pagination import ValuesViewsetCursorPagination +from contentcuration.utils.nodes import validate_and_conform_to_schema_threshold_none from contentcuration.viewsets.base import BulkListSerializer from contentcuration.viewsets.base import BulkModelSerializer from contentcuration.viewsets.base import BulkUpdateMixin @@ -286,6 +287,10 @@ class ExtraFieldsOptionsSerializer(JSONFieldDictSerializer): modality = ChoiceField(choices=(("QUIZ", "Quiz"),), allow_null=True, required=False) completion_criteria = CompletionCriteriaSerializer(required=False) + def update(self, instance, validated_data): + validated_data = validate_and_conform_to_schema_threshold_none(validated_data) + return super(ExtraFieldsOptionsSerializer, self).update(instance, validated_data) + class ExtraFieldsSerializer(JSONFieldDictSerializer): randomize = BooleanField() From 230e65e8c3db033d3ecdc586b59b507a1171f35f Mon Sep 17 00:00:00 2001 From: ozer550 Date: Thu, 22 Aug 2024 13:10:02 +0530 Subject: [PATCH 2/2] fix super update error --- .../contentcuration/tests/viewsets/test_contentnode.py | 1 - contentcuration/contentcuration/utils/nodes.py | 6 ++---- contentcuration/contentcuration/viewsets/contentnode.py | 8 ++++---- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/contentcuration/contentcuration/tests/viewsets/test_contentnode.py b/contentcuration/contentcuration/tests/viewsets/test_contentnode.py index 99a508e0ca..4402db855a 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_contentnode.py +++ b/contentcuration/contentcuration/tests/viewsets/test_contentnode.py @@ -851,7 +851,6 @@ def test_update_completion_criteria_model_to_determined_by_resource_edge_case(se ) ], ) - self.assertEqual(len(response.data["errors"]), 0) updated_contentnode = models.ContentNode.objects.get(id=contentnode.id) self.assertEqual(updated_contentnode.extra_fields["options"]["completion_criteria"]["model"], completion_criteria.DETERMINED_BY_RESOURCE) diff --git a/contentcuration/contentcuration/utils/nodes.py b/contentcuration/contentcuration/utils/nodes.py index 37dce5a60f..c84467a5cc 100644 --- a/contentcuration/contentcuration/utils/nodes.py +++ b/contentcuration/contentcuration/utils/nodes.py @@ -455,11 +455,9 @@ def migrate_extra_fields(extra_fields): return extra_fields -def validate_and_conform_to_schema_threshold_none(validated_options): - completion_criteria_validated = validated_options.get("completion_criteria", {}) +def validate_and_conform_to_schema_threshold_none(completion_criteria_validated): model = completion_criteria_validated.get("model", {}) if model in ["reference", "determined_by_resource"]: if "threshold" not in completion_criteria_validated or completion_criteria_validated["threshold"] is not None: completion_criteria_validated["threshold"] = None - validated_options["completion_criteria"] = completion_criteria_validated - return validated_options + return completion_criteria_validated diff --git a/contentcuration/contentcuration/viewsets/contentnode.py b/contentcuration/contentcuration/viewsets/contentnode.py index fde1296fc5..35889bbfcb 100644 --- a/contentcuration/contentcuration/viewsets/contentnode.py +++ b/contentcuration/contentcuration/viewsets/contentnode.py @@ -282,15 +282,15 @@ class CompletionCriteriaSerializer(JSONFieldDictSerializer): model = CharField() learner_managed = BooleanField(required=False, allow_null=True) + def update(self, instance, validated_data): + validated_data = validate_and_conform_to_schema_threshold_none(validated_data) + return super(CompletionCriteriaSerializer, self).update(instance, validated_data) + class ExtraFieldsOptionsSerializer(JSONFieldDictSerializer): modality = ChoiceField(choices=(("QUIZ", "Quiz"),), allow_null=True, required=False) completion_criteria = CompletionCriteriaSerializer(required=False) - def update(self, instance, validated_data): - validated_data = validate_and_conform_to_schema_threshold_none(validated_data) - return super(ExtraFieldsOptionsSerializer, self).update(instance, validated_data) - class ExtraFieldsSerializer(JSONFieldDictSerializer): randomize = BooleanField()