diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index 4b38a63f92..7fb62dc89c 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -64,6 +64,7 @@ from rest_framework.utils.encoders import JSONEncoder from contentcuration.constants import channel_history +from contentcuration.constants import completion_criteria from contentcuration.constants.contentnode import kind_activity_map from contentcuration.db.models.expressions import Array from contentcuration.db.models.functions import ArrayRemove @@ -1722,6 +1723,45 @@ def recalculate_editors_storage(self): for editor in self.files.values_list('uploaded_by_id', flat=True).distinct(): calculate_user_storage(editor) + def mark_complete(self): # noqa C901 + errors = [] + # Is complete if title is falsy but only if not a root node. + if not (bool(self.title) or self.parent_id is None): + errors.append("Empty title") + if self.kind_id != content_kinds.TOPIC: + if not self.license: + errors.append("Missing license") + if self.license and self.license.is_custom and not self.license_description: + errors.append("Missing license description for custom license") + if self.license and self.license.copyright_holder_required and not self.copyright_holder: + errors.append("Missing required copyright holder") + if self.kind_id != content_kinds.EXERCISE and not self.files.filter(preset__supplementary=False).exists(): + errors.append("Missing default file") + if self.kind_id == content_kinds.EXERCISE: + # Check to see if the exercise has at least one assessment item that has: + if not self.assessment_items.filter( + # A non-blank question + ~Q(question='') + # Non-blank answers + & ~Q(answers='[]') + # With either an input question or one answer marked as correct + & (Q(type=exercises.INPUT_QUESTION) | Q(answers__iregex=r'"correct":\s*true')) + ).exists(): + errors.append("No questions with question text and complete answers") + # Check that it has a mastery model set + # Either check for the previous location for the mastery model, or rely on our completion criteria validation + # that if it has been set, then it has been set correctly. + criterion = self.extra_fields.get("options", {}).get("completion_criteria") + if not (self.extra_fields.get("mastery_model") or criterion): + errors.append("Missing mastery criterion") + if criterion: + try: + completion_criteria.validate(criterion, kind=content_kinds.EXERCISE) + except completion_criteria.ValidationError: + errors.append("Mastery criterion is defined but is invalid") + self.complete = not errors + return errors + def on_create(self): self.changed = True self.recalculate_editors_storage() diff --git a/contentcuration/contentcuration/tests/test_contentnodes.py b/contentcuration/contentcuration/tests/test_contentnodes.py index 6fa1235fb8..aaa49e7553 100644 --- a/contentcuration/contentcuration/tests/test_contentnodes.py +++ b/contentcuration/contentcuration/tests/test_contentnodes.py @@ -4,6 +4,7 @@ import random import string import time +import uuid from builtins import range from builtins import str from builtins import zip @@ -11,7 +12,10 @@ import pytest from django.db import IntegrityError from django.db.utils import DataError +from le_utils.constants import completion_criteria from le_utils.constants import content_kinds +from le_utils.constants import exercises +from le_utils.constants import format_presets from mixer.backend.django import mixer from mock import patch from past.utils import old_div @@ -19,13 +23,16 @@ from . import testdata from .base import StudioTestCase from .testdata import create_studio_file +from contentcuration.models import AssessmentItem from contentcuration.models import Channel from contentcuration.models import ContentKind from contentcuration.models import ContentNode from contentcuration.models import ContentTag +from contentcuration.models import File from contentcuration.models import FormatPreset from contentcuration.models import generate_storage_url from contentcuration.models import Language +from contentcuration.models import License from contentcuration.utils.db_tools import TreeBuilder from contentcuration.utils.files import create_thumbnail_from_base64 from contentcuration.utils.sync import sync_node @@ -869,3 +876,214 @@ def test_create_node_null_complete(self): new_obj.save() except IntegrityError: self.fail("Caused an IntegrityError") + + +class NodeCompletionTestCase(StudioTestCase): + + old_extra_fields = { + "mastery_model": exercises.M_OF_N, + "randomize": False, + "m": 3, + "n": 5, + } + + new_extra_fields = { + "randomize": False, + "options": { + "completion_criteria": { + "threshold": { + "mastery_model": exercises.M_OF_N, + "m": 4, + "n": 5, + }, + "model": completion_criteria.MASTERY, + } + } + } + + def setUp(self): + return super(NodeCompletionTestCase, self).setUpBase() + + def test_create_topic_set_complete_no_parent(self): + new_obj = ContentNode(kind_id=content_kinds.TOPIC) + new_obj.save() + new_obj.mark_complete() + self.assertTrue(new_obj.complete) + + def test_create_topic_set_complete_parent_no_title(self): + channel = testdata.channel() + new_obj = ContentNode(kind_id=content_kinds.TOPIC, parent=channel.main_tree) + new_obj.save() + new_obj.mark_complete() + self.assertFalse(new_obj.complete) + + def test_create_topic_set_complete_parent_title(self): + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.TOPIC, parent=channel.main_tree) + new_obj.save() + new_obj.mark_complete() + self.assertTrue(new_obj.complete) + + def test_create_video_set_complete_no_license(self): + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.VIDEO, parent=channel.main_tree) + new_obj.save() + File.objects.create(contentnode=new_obj, preset_id=format_presets.VIDEO_HIGH_RES, checksum=uuid.uuid4().hex) + new_obj.mark_complete() + self.assertFalse(new_obj.complete) + + def test_create_video_set_complete_custom_license_no_description(self): + custom_licenses = list(License.objects.filter(is_custom=True).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.VIDEO, parent=channel.main_tree, license_id=custom_licenses[0], copyright_holder="Some person") + new_obj.save() + File.objects.create(contentnode=new_obj, preset_id=format_presets.VIDEO_HIGH_RES, checksum=uuid.uuid4().hex) + new_obj.mark_complete() + self.assertFalse(new_obj.complete) + + def test_create_video_set_complete_custom_license_with_description(self): + custom_licenses = list(License.objects.filter(is_custom=True).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode( + title="yes", + kind_id=content_kinds.VIDEO, + parent=channel.main_tree, + license_id=custom_licenses[0], + license_description="don't do this!", + copyright_holder="Some person" + ) + new_obj.save() + File.objects.create(contentnode=new_obj, preset_id=format_presets.VIDEO_HIGH_RES, checksum=uuid.uuid4().hex) + new_obj.mark_complete() + self.assertTrue(new_obj.complete) + + def test_create_video_set_complete_copyright_holder_required_no_copyright_holder(self): + required_holder = list(License.objects.filter(copyright_holder_required=True, is_custom=False).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.VIDEO, parent=channel.main_tree, license_id=required_holder[0]) + new_obj.save() + File.objects.create(contentnode=new_obj, preset_id=format_presets.VIDEO_HIGH_RES, checksum=uuid.uuid4().hex) + new_obj.mark_complete() + self.assertFalse(new_obj.complete) + + def test_create_video_set_complete_copyright_holder_required_copyright_holder(self): + required_holder = list(License.objects.filter(copyright_holder_required=True, is_custom=False).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.VIDEO, parent=channel.main_tree, license_id=required_holder[0], copyright_holder="Some person") + new_obj.save() + File.objects.create(contentnode=new_obj, preset_id=format_presets.VIDEO_HIGH_RES, checksum=uuid.uuid4().hex) + new_obj.mark_complete() + self.assertTrue(new_obj.complete) + + def test_create_video_no_files(self): + licenses = list(License.objects.filter(copyright_holder_required=False, is_custom=False).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.VIDEO, parent=channel.main_tree, license_id=licenses[0]) + new_obj.save() + new_obj.mark_complete() + self.assertFalse(new_obj.complete) + + def test_create_video_thumbnail_only(self): + licenses = list(License.objects.filter(copyright_holder_required=False, is_custom=False).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.VIDEO, parent=channel.main_tree, license_id=licenses[0]) + new_obj.save() + File.objects.create(contentnode=new_obj, preset_id=format_presets.VIDEO_THUMBNAIL, checksum=uuid.uuid4().hex) + new_obj.mark_complete() + self.assertFalse(new_obj.complete) + + def test_create_exercise_no_assessment_items(self): + licenses = list(License.objects.filter(copyright_holder_required=False, is_custom=False).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.EXERCISE, parent=channel.main_tree, license_id=licenses[0], extra_fields=self.new_extra_fields) + new_obj.save() + new_obj.mark_complete() + self.assertFalse(new_obj.complete) + + def test_create_exercise_invalid_assessment_item_no_question(self): + licenses = list(License.objects.filter(copyright_holder_required=False, is_custom=False).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.EXERCISE, parent=channel.main_tree, license_id=licenses[0], extra_fields=self.new_extra_fields) + new_obj.save() + AssessmentItem.objects.create(contentnode=new_obj, answers="[{\"correct\": true, \"text\": \"answer\"}]") + new_obj.mark_complete() + self.assertFalse(new_obj.complete) + + def test_create_exercise_invalid_assessment_item_no_answers(self): + licenses = list(License.objects.filter(copyright_holder_required=False, is_custom=False).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.EXERCISE, parent=channel.main_tree, license_id=licenses[0], extra_fields=self.new_extra_fields) + new_obj.save() + AssessmentItem.objects.create(contentnode=new_obj, question="This is a question") + new_obj.mark_complete() + self.assertFalse(new_obj.complete) + + def test_create_exercise_invalid_assessment_item_no_correct_answers(self): + licenses = list(License.objects.filter(copyright_holder_required=False, is_custom=False).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.EXERCISE, parent=channel.main_tree, license_id=licenses[0], extra_fields=self.new_extra_fields) + new_obj.save() + AssessmentItem.objects.create(contentnode=new_obj, question="This is a question", answers="[{\"correct\": false, \"text\": \"answer\"}]") + new_obj.mark_complete() + self.assertFalse(new_obj.complete) + + def test_create_exercise_valid_assessment_item_no_correct_answers_input(self): + licenses = list(License.objects.filter(copyright_holder_required=False, is_custom=False).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.EXERCISE, parent=channel.main_tree, license_id=licenses[0], extra_fields=self.new_extra_fields) + new_obj.save() + AssessmentItem.objects.create( + contentnode=new_obj, + question="This is a question", + answers="[{\"correct\": false, \"text\": \"answer\"}]", + type=exercises.INPUT_QUESTION + ) + new_obj.mark_complete() + self.assertTrue(new_obj.complete) + + def test_create_exercise_valid_assessment_items(self): + licenses = list(License.objects.filter(copyright_holder_required=False, is_custom=False).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.EXERCISE, parent=channel.main_tree, license_id=licenses[0], extra_fields=self.new_extra_fields) + new_obj.save() + AssessmentItem.objects.create(contentnode=new_obj, question="This is a question", answers="[{\"correct\": true, \"text\": \"answer\"}]") + new_obj.mark_complete() + self.assertTrue(new_obj.complete) + + def test_create_exercise_no_extra_fields(self): + licenses = list(License.objects.filter(copyright_holder_required=False, is_custom=False).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.EXERCISE, parent=channel.main_tree, license_id=licenses[0]) + new_obj.save() + AssessmentItem.objects.create(contentnode=new_obj, question="This is a question", answers="[{\"correct\": true, \"text\": \"answer\"}]") + new_obj.mark_complete() + self.assertFalse(new_obj.complete) + + def test_create_exercise_old_extra_fields(self): + licenses = list(License.objects.filter(copyright_holder_required=False, is_custom=False).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.EXERCISE, parent=channel.main_tree, license_id=licenses[0], extra_fields=self.old_extra_fields) + new_obj.save() + AssessmentItem.objects.create(contentnode=new_obj, question="This is a question", answers="[{\"correct\": true, \"text\": \"answer\"}]") + new_obj.mark_complete() + self.assertTrue(new_obj.complete) + + def test_create_exercise_bad_new_extra_fields(self): + licenses = list(License.objects.filter(copyright_holder_required=False, is_custom=False).values_list("pk", flat=True)) + channel = testdata.channel() + new_obj = ContentNode(title="yes", kind_id=content_kinds.EXERCISE, parent=channel.main_tree, license_id=licenses[0], extra_fields={ + "randomize": False, + "options": { + "completion_criteria": { + "threshold": { + "mastery_model": exercises.M_OF_N, + "n": 5, + }, + "model": completion_criteria.MASTERY, + } + } + }) + new_obj.save() + AssessmentItem.objects.create(contentnode=new_obj, question="This is a question", answers="[{\"correct\": true, \"text\": \"answer\"}]") + new_obj.mark_complete() + self.assertFalse(new_obj.complete) diff --git a/contentcuration/contentcuration/tests/views/test_views_internal.py b/contentcuration/contentcuration/tests/views/test_views_internal.py index 27ff4d026d..999cc9a7b6 100644 --- a/contentcuration/contentcuration/tests/views/test_views_internal.py +++ b/contentcuration/contentcuration/tests/views/test_views_internal.py @@ -4,6 +4,7 @@ """ import json import uuid +from unittest import skipIf from django.db import connections from django.urls import reverse_lazy @@ -195,6 +196,7 @@ def test_metadata_properly_created(self): values[0]: True }) + @skipIf(True, "Disable until we mark nodes as incomplete rather than just warn") 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") diff --git a/contentcuration/contentcuration/views/internal.py b/contentcuration/contentcuration/views/internal.py index 4a57e88a03..65a954d4da 100644 --- a/contentcuration/contentcuration/views/internal.py +++ b/contentcuration/contentcuration/views/internal.py @@ -53,6 +53,7 @@ from contentcuration.utils.nodes import map_files_to_assessment_item from contentcuration.utils.nodes import map_files_to_node from contentcuration.utils.nodes import map_files_to_slideshow_slide_item +from contentcuration.utils.sentry import report_exception from contentcuration.utils.tracing import trace from contentcuration.viewsets.sync.constants import CHANNEL from contentcuration.viewsets.sync.utils import generate_publish_event @@ -555,6 +556,20 @@ def create_channel(channel_data, user): return channel # Return new channel +class IncompleteNodeError(Exception): + """ + Used to track incomplete nodes from ricecooker. We don't raise this error, + just feed it to Sentry for reporting. + """ + + def __init__(self, node, errors): + self.message = ( + "Node {} had the following errors: {}".format(node, ",".join(errors)) + ) + + super(IncompleteNodeError, self).__init__(self.message) + + @trace def convert_data_to_nodes(user, content_data, parent_node): """ Parse dict and create nodes accordingly """ @@ -596,6 +611,17 @@ def convert_data_to_nodes(user, content_data, parent_node): user, new_node, slides, node_data["files"] ) + # Wait until after files have been set on the node to check for node completeness + # as some node kinds are counted as incomplete if they lack a default file. + completion_errors = new_node.mark_complete() + + if completion_errors: + try: + # we need to raise it to get Python to fill out the stack trace. + raise IncompleteNodeError(new_node, completion_errors) + except IncompleteNodeError as e: + report_exception(e) + # Track mapping between newly created node and node id root_mapping.update({node_data["node_id"]: new_node.pk}) return root_mapping @@ -637,16 +663,9 @@ def create_node(node_data, parent_node, sort_order): # noqa: C901 raise NodeValidationError("Node {} has invalid completion criteria".format(node_data["node_id"])) # 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 != "" metadata_labels = {} @@ -681,7 +700,10 @@ def create_node(node_data, parent_node, sort_order): # noqa: C901 language_id=node_data.get("language"), freeze_authoring_data=True, role_visibility=node_data.get('role') or roles.LEARNER, - complete=is_complete, + # Assume it is complete to start with, we will do validation + # later when we have all data available to determine if it is + # complete or not. + complete=True, **metadata_labels )