diff --git a/contentcuration/contentcuration/management/commands/mark_incomplete.py b/contentcuration/contentcuration/management/commands/mark_incomplete.py index 8df69f3691..f0b2962f86 100644 --- a/contentcuration/contentcuration/management/commands/mark_incomplete.py +++ b/contentcuration/contentcuration/management/commands/mark_incomplete.py @@ -55,7 +55,7 @@ def handle(self, *args, **options): count = ContentNode.objects.exclude(kind_id=content_kinds.TOPIC) \ .exclude(complete=False) \ .filter(license_id__in=copyright_licenses) \ - .filter(Q(copyright_holder__isnull=True) | Q(copyright_holder=''))\ + .filter(Q(copyright_holder__isnull=True) | Q(copyright_holder='')) \ .order_by() \ .update(complete=False) logging.info('Marked {} invalid copyright holders (finished in {})'.format(count, time.time() - licensestart)) @@ -65,13 +65,13 @@ def handle(self, *args, **options): logging.info('Marking file resources...') file_check_query = With(File.objects.filter(preset__supplementary=False).values("contentnode_id").order_by(), name="t_file") - query = file_check_query.join(ContentNode, id=file_check_query.col.contentnode_id, _join_type=LOUTER)\ + query = file_check_query.join(ContentNode, id=file_check_query.col.contentnode_id, _join_type=LOUTER) \ .with_cte(file_check_query) \ .annotate(t_contentnode_id=file_check_query.col.contentnode_id) \ .exclude(kind_id=content_kinds.TOPIC) \ .exclude(kind_id=content_kinds.EXERCISE) \ .exclude(complete=False) \ - .filter(t_contentnode_id__isnull=True)\ + .filter(t_contentnode_id__isnull=True) \ .order_by() count = ContentNode.objects.filter(id__in=query.order_by().values_list('id', flat=True)).update(complete=False) logging.info('Marked {} invalid file resources (finished in {})'.format(count, time.time() - resourcestart)) @@ -82,12 +82,12 @@ def handle(self, *args, **options): has_questions_query = With(AssessmentItem.objects.all().values("contentnode_id").order_by(), name="t_assessmentitem") - query = has_questions_query.join(ContentNode, id=has_questions_query.col.contentnode_id, _join_type=LOUTER)\ + query = has_questions_query.join(ContentNode, id=has_questions_query.col.contentnode_id, _join_type=LOUTER) \ .with_cte(has_questions_query) \ .annotate(t_contentnode_id=has_questions_query.col.contentnode_id) \ .filter(kind_id=content_kinds.EXERCISE) \ .exclude(complete=False) \ - .filter(t_contentnode_id__isnull=True)\ + .filter(t_contentnode_id__isnull=True) \ .order_by() exercisestart = time.time() count = ContentNode.objects.filter(id__in=query.order_by().values_list('id', flat=True)).update(complete=False) @@ -98,12 +98,12 @@ def handle(self, *args, **options): exercise_check_query = With(AssessmentItem.objects.exclude(type=exercises.PERSEUS_QUESTION) .filter( - Q(question='') | - Q(answers='[]') | + Q(question='') + | Q(answers='[]') # hack to check if no correct answers - (~Q(type=exercises.INPUT_QUESTION) & ~Q(answers__iregex=r'"correct":\s*true'))).order_by(), name="t_assessmentitem") + | (~Q(type=exercises.INPUT_QUESTION) & ~Q(answers__iregex=r'"correct":\s*true'))).order_by(), name="t_assessmentitem") - query = exercise_check_query.join(ContentNode, id=has_questions_query.col.contentnode_id)\ + query = exercise_check_query.join(ContentNode, id=has_questions_query.col.contentnode_id) \ .with_cte(exercise_check_query) \ .annotate(t_contentnode_id=exercise_check_query.col.contentnode_id) \ .filter(kind_id=content_kinds.EXERCISE) \ @@ -116,14 +116,14 @@ def handle(self, *args, **options): exercisestart = time.time() logging.info('Marking mastery_model less exercises...') - count = ContentNode.objects.exclude(complete=False).filter(kind_id=content_kinds.EXERCISE).filter(~Q(extra_fields__has_key='mastery_model'))\ + count = ContentNode.objects.exclude(complete=False).filter(kind_id=content_kinds.EXERCISE).filter(~Q(extra_fields__has_key='mastery_model')) \ .order_by().update(complete=False) logging.info('Marked {} mastery_model less exercises(finished in {})'.format(count, time.time() - exercisestart)) exercisestart = time.time() logging.info('Marking bad mastery model exercises...') - count = ContentNode.objects.exclude(complete=False).filter(kind_id=content_kinds.EXERCISE)\ - .filter(Q(extra_fields__mastery_model=exercises.M_OF_N) & (~Q(extra_fields__has_key='m') | ~Q(extra_fields__has_key='n')))\ + count = ContentNode.objects.exclude(complete=False).filter(kind_id=content_kinds.EXERCISE) \ + .filter(Q(extra_fields__mastery_model=exercises.M_OF_N) & (~Q(extra_fields__has_key='m') | ~Q(extra_fields__has_key='n'))) \ .order_by().update(complete=False) logging.info('Marked {} bad mastery model exercises (finished in {})'.format(count, time.time() - exercisestart)) diff --git a/contentcuration/contentcuration/tests/views/test_views_internal.py b/contentcuration/contentcuration/tests/views/test_views_internal.py index 6d551aedda..709b25068c 100644 --- a/contentcuration/contentcuration/tests/views/test_views_internal.py +++ b/contentcuration/contentcuration/tests/views/test_views_internal.py @@ -48,46 +48,73 @@ class ApiAddNodesToTreeTestCase(StudioTestCase): Tests for contentcuration.views.internal.api_add_nodes_to_tree function. """ + def _make_node_data(self): + random_data = mixer.blend(SampleContentNodeDataSchema) + fileobj = self.fileobj + return { + "title": random_data.title, + "language": "en", + "description": random_data.description, + "node_id": random_data.node_id, + "content_id": random_data.content_id, + "source_domain": random_data.source_domain, + "source_id": random_data.source_id, + "author": random_data.author, + "files": [ + { + "size": fileobj.file_size, + "preset": "video", + "filename": fileobj.filename(), + "original_filename": fileobj.original_filename, + "language": fileobj.language, + "source_url": fileobj.source_url, + } + ], + "kind": "document", + "license": "CC BY", + "license_description": None, + "copyright_holder": random_data.copyright_holder, + "questions": [], + "extra_fields": "{}", + "role": "learner", + } + def setUp(self): super(ApiAddNodesToTreeTestCase, self).setUp() # first setup a test channel... self.channel = channel() self.root_node = self.channel.main_tree - # get our random data from mixer - random_data = mixer.blend(SampleContentNodeDataSchema) + # File used for every node self.fileobj = fileobj_video() - self.title = random_data.title + + # Valid node + valid_node = self._make_node_data() + self.title = valid_node["title"] + + # Node with invalid title + invalid_title_node = self._make_node_data() + invalid_title_node["title"] = "" + invalid_title_node["description"] = "invalid_title_node" + + # Node with "Special Permissions" license, but no license description + invalid_license_description = self._make_node_data() + invalid_license_description["title"] = "invalid_license_description" + invalid_license_description["license"] = "Special Permissions" + invalid_license_description["license_description"] = "" + + # Node with "CC By" license, but no copyright holder + invalid_copyright_holder = self._make_node_data() + invalid_copyright_holder["title"] = "invalid_copyright_holder" + invalid_copyright_holder["copyright_holder"] = "" + self.sample_data = { "root_id": self.root_node.id, "content_data": [ - { - "title": self.title, - "language": "en", - "description": random_data.description, - "node_id": random_data.node_id, - "content_id": random_data.content_id, - "source_domain": random_data.source_domain, - "source_id": random_data.source_id, - "author": random_data.author, - "files": [ - { - "size": self.fileobj.file_size, - "preset": "video", - "filename": self.fileobj.filename(), - "original_filename": self.fileobj.original_filename, - "language": self.fileobj.language, - "source_url": self.fileobj.source_url, - } - ], - "kind": "document", - "license": "CC BY", - "license_description": None, - "copyright_holder": random_data.copyright_holder, - "questions": [], - "extra_fields": "{}", - "role": "learner", - } + valid_node, + invalid_title_node, + invalid_license_description, + invalid_copyright_holder, ], } self.resp = self.admin_client().post( @@ -137,6 +164,17 @@ def test_associates_file_with_created_node(self): # our original file object assert f.file_on_disk.read() == self.fileobj.file_on_disk.read() + def test_invalid_nodes_are_not_complete(self): + node_0 = ContentNode.objects.get(title=self.title) + node_1 = ContentNode.objects.get(description="invalid_title_node") + node_2 = ContentNode.objects.get(title="invalid_license_description") + node_3 = ContentNode.objects.get(title="invalid_copyright_holder") + + self.assertTrue(node_0.complete) + self.assertFalse(node_1.complete) + self.assertFalse(node_2.complete) + self.assertFalse(node_3.complete) + class ApiAddExerciseNodesToTreeTestCase(StudioTestCase): """ diff --git a/contentcuration/contentcuration/views/internal.py b/contentcuration/contentcuration/views/internal.py index bf8fe7947b..ca7693b85e 100644 --- a/contentcuration/contentcuration/views/internal.py +++ b/contentcuration/contentcuration/views/internal.py @@ -559,7 +559,7 @@ def convert_data_to_nodes(user, content_data, parent_node): raise ObjectDoesNotExist("Error creating node: {0}".format(e)) -def create_node(node_data, parent_node, sort_order): +def create_node(node_data, parent_node, sort_order): # noqa: C901 """ Generate node based on node dict """ # Make sure license is valid license = None @@ -574,8 +574,20 @@ def create_node(node_data, parent_node, sort_order): if isinstance(extra_fields, basestring): extra_fields = json.loads(extra_fields) + # Validate title and license fields + is_complete = True + title = node_data.get('title', "") + license_description = node_data.get('license_description', "") + copyright_holder = node_data.get('copyright_holder', "") + is_complete &= title != "" + if node_data['kind'] != content_kinds.TOPIC: + if license.is_custom: + is_complete &= license_description != "" + if license.copyright_holder_required: + is_complete &= copyright_holder != "" + node = ContentNode.objects.create( - title=node_data['title'], + title=title, tree_id=parent_node.tree_id, kind_id=node_data['kind'], node_id=node_data['node_id'], @@ -585,8 +597,8 @@ def create_node(node_data, parent_node, sort_order): aggregator=node_data.get('aggregator') or "", provider=node_data.get('provider') or "", license=license, - license_description=node_data.get('license_description'), - copyright_holder=node_data.get('copyright_holder') or "", + license_description=license_description, + copyright_holder=copyright_holder, parent=parent_node, extra_fields=extra_fields, sort_order=sort_order, @@ -595,7 +607,7 @@ def create_node(node_data, parent_node, sort_order): language_id=node_data.get('language'), freeze_authoring_data=True, role_visibility=node_data.get('role') or roles.LEARNER, - complete=True, + complete=is_complete, ) tags = [] channel = node.get_channel()