Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 128 additions & 2 deletions contentcuration/contentcuration/tests/viewsets/test_contentnode.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ def test_deadlock_move_and_rebuild(self):
for (node_id, target_id) in zip(
root_children_ids, first_child_node_children_ids
)
)
),
)

for result in results:
Expand All @@ -236,7 +236,7 @@ def test_deadlock_create_and_rebuild(self):
*(
(create_contentnode, {"parent_id": node_id})
for node_id in first_child_node_children_ids
)
),
)

for result in results:
Expand Down Expand Up @@ -1903,6 +1903,132 @@ def test_delete_no_permission_prerequisite(self):
self.assertEqual(len(response.data["disallowed"]), 1)
self.assertTrue(contentnode.prerequisite.filter(id=prereq.id).exists())

def test_create_html5_contentnode_with_entry_validation(self):
"""
Regression test for HTML5 nodes validation failure when entry value is set in extra_fields.

This test verifies that newly created HTML5 content nodes with an "entry" value
in extra_fields.options.entry can be successfully validated and created.
"""
contentnode_data = self.contentnode_metadata
contentnode_data["kind"] = content_kinds.HTML5
contentnode_data["extra_fields"] = {"options": {"entry": "index.html"}}

response = self.sync_changes(
[
generate_create_event(
contentnode_data["id"],
CONTENTNODE,
contentnode_data,
channel_id=self.channel.id,
)
],
)
self.assertEqual(response.status_code, 200, response.content)
self.assertEqual(
len(response.data.get("errors", [])),
0,
f"Expected no validation errors, but got: {response.data.get('errors', [])}",
)

try:
new_node = models.ContentNode.objects.get(id=contentnode_data["id"])
except models.ContentNode.DoesNotExist:
self.fail("HTML5 ContentNode with entry value was not created")

self.assertEqual(new_node.parent_id, self.channel.main_tree_id)
self.assertEqual(new_node.kind_id, content_kinds.HTML5)
self.assertEqual(new_node.extra_fields["options"]["entry"], "index.html")

def test_create_exercise_contentnode_requires_randomize(self):
"""
Test that exercise content nodes require the randomize field in extra_fields.
"""
contentnode_data = self.contentnode_metadata
contentnode_data["kind"] = content_kinds.EXERCISE
# Deliberately omit randomize field
contentnode_data["extra_fields"] = {"options": {}}

response = self.sync_changes(
[
generate_create_event(
contentnode_data["id"],
CONTENTNODE,
contentnode_data,
channel_id=self.channel.id,
)
],
)
self.assertEqual(response.status_code, 200, response.content)
self.assertEqual(len(response.data.get("errors", [])), 1)

error = response.data["errors"][0]

self.assertIn("randomize", error["errors"]["extra_fields"])
self.assertEqual(
error["errors"]["extra_fields"]["randomize"][0],
"This field is required for exercise content.",
)

def test_create_exercise_contentnode_with_randomize_succeeds(self):
"""
Test that exercise content nodes with randomize field are created successfully.
"""
contentnode_data = self.contentnode_metadata
contentnode_data["kind"] = content_kinds.EXERCISE
contentnode_data["extra_fields"] = {"randomize": True, "options": {}}

response = self.sync_changes(
[
generate_create_event(
contentnode_data["id"],
CONTENTNODE,
contentnode_data,
channel_id=self.channel.id,
)
],
)
self.assertEqual(response.status_code, 200, response.content)
self.assertEqual(len(response.data.get("errors", [])), 0)

try:
new_node = models.ContentNode.objects.get(id=contentnode_data["id"])
except models.ContentNode.DoesNotExist:
self.fail("Exercise ContentNode with randomize field was not created")

self.assertEqual(new_node.kind_id, content_kinds.EXERCISE)
self.assertTrue(new_node.extra_fields["randomize"])

def test_cannot_update_contentnode_kind(self):
"""
Test that content node kind cannot be changed after creation.
"""
contentnode = models.ContentNode.objects.create(**self.contentnode_db_metadata)
original_kind = contentnode.kind_id

response = self.sync_changes(
[
generate_update_event(
contentnode.id,
CONTENTNODE,
{"kind": content_kinds.HTML5},
channel_id=self.channel.id,
)
],
)
self.assertEqual(response.status_code, 200, response.content)
self.assertEqual(len(response.data.get("errors", [])), 1)

error = response.data["errors"][0]
self.assertIn("kind", error["errors"])
self.assertEqual(
error["errors"]["kind"][0], "Content kind cannot be changed after creation"
)

# Verify kind was not changed
contentnode.refresh_from_db()
self.assertEqual(contentnode.kind_id, original_kind)


class CRUDTestCase(StudioAPITestCase):
def setUp(self):
Expand Down
26 changes: 25 additions & 1 deletion contentcuration/contentcuration/viewsets/contentnode.py
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ class ExtraFieldsOptionsSerializer(JSONFieldDictSerializer):
required=False,
)
completion_criteria = CompletionCriteriaSerializer(required=False)
entry = CharField(required=False, allow_null=True)


class InheritedMetadataSerializer(JSONFieldDictSerializer):
Expand All @@ -307,7 +308,7 @@ class InheritedMetadataSerializer(JSONFieldDictSerializer):


class ExtraFieldsSerializer(JSONFieldDictSerializer):
randomize = BooleanField()
randomize = BooleanField(required=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

options = ExtraFieldsOptionsSerializer(required=False)
suggested_duration_type = ChoiceField(
choices=[completion_criteria.TIME, completion_criteria.APPROX_TIME],
Expand Down Expand Up @@ -428,11 +429,34 @@ def validate(self, data):
raise ValidationError(
{"parent": "This field should only be changed by a move operation"}
)

# Prevent kind from being changed after creation
if self.instance is not None and "kind" in data:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I do prefer the strict sense of rejecting any request that includes kind for an update, it seems less risky to just have this check whether kind included in the request is indeed different. I wouldn't expect the inclusion of kind in the request, considering how frontend syncing functions, but this strictness could cascade into more required changes if there are surprises.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I can loosen it!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

raise ValidationError(
{"kind": "Content kind cannot be changed after creation"}
)

tags = data.get("tags")
if tags is not None:
for tag in tags:
if len(tag) > 30:
raise ValidationError("tag is greater than 30 characters")

# Conditional validation for randomize field on exercise creation
if self.instance is None: # Only validate on creation
kind = data.get("kind")
if kind.kind == content_kinds.EXERCISE:
extra_fields = data.get("extra_fields", {})
if "randomize" not in extra_fields:
raise ValidationError(
{
"extra_fields": {
"randomize": [
"This field is required for exercise content."
]
}
}
)
return data

def _check_completion_criteria(self, kind, complete, validated_data):
Expand Down