Merge pull request #3410 from rtibbles/node_complete

Set complete marking on nodes created by ricecooker

Author: rtibbles

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()

contentcuration/contentcuration/tests/test_contentnodes.py

@@ -4,28 +4,35 @@
 import random
 import string
 import time
+import uuid
 from builtins import range
 from builtins import str
 from builtins import zip
 
 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
 
 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)

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")

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
     )