Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Creates a regression test for and fixes duplicate node assessments returning a 500. #4614

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
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
23 changes: 20 additions & 3 deletions contentcuration/contentcuration/tests/views/test_views_internal.py
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,9 @@ def setUp(self):
}
],
}
self.resp = self.admin_client().post(

def _make_request(self):
return self.admin_client().post(
reverse_lazy("api_add_nodes_to_tree"), data=self.sample_data, format="json"
)

Expand All @@ -378,15 +380,17 @@ def test_returns_200_status_code(self):
"""
Check that we return 200 if passed in a valid JSON.
"""
response = self._make_request()
# check that we returned 200 with that POST request
assert self.resp.status_code == 200, "Got a request error: {}".format(
self.resp.content
assert response.status_code == 200, "Got a request error: {}".format(
response.content
)

def test_creates_nodes(self):
"""
Test that it creates a node with the given title and parent.
"""
self._make_request()
# make sure a node with our given self.title exists, with the given parent.
assert ContentNode.get_nodes_with_title(
title=self.title, limit_to_children_of=self.root_node.id
Expand All @@ -397,6 +401,7 @@ def test_associated_assesment_items_with_created_node(self):
Check that the file we created beforehand is now associated
with the node we just created through add_nodes.
"""
self._make_request()
c = ContentNode.objects.get(title=self.title)

# there shold be no files associated with the condent node
Expand All @@ -421,6 +426,7 @@ def test_exercise_image_files_associated_with_assesment_items(self):
Check that the files we created beforehand are now associated with the
correct assesment items.
"""
self._make_request()
c = ContentNode.objects.get(title=self.title)

question1 = c.assessment_items.get(
Expand Down Expand Up @@ -452,6 +458,17 @@ def test_exercise_image_files_associated_with_assesment_items(self):
file2.original_filename == self.exercise_graphie.original_filename
), "wrong original_filename"

def test_duplicate_assessment_item_returns_400_status_code(self):
"""
Check that we return 400 if passed in duplicate assessment items.
"""
self.sample_data["content_data"][0]["questions"][1]["assessment_id"] = self.sample_data["content_data"][0]["questions"][0]["assessment_id"]
response = self._make_request()
# check that we returned 400 with that POST request
assert response.status_code == 400, "Got a non-400 request error: {}".format(
response.status_code
)


class PublishEndpointTestCase(BaseAPITestCase):
@classmethod
Expand Down
7 changes: 6 additions & 1 deletion contentcuration/contentcuration/views/internal.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
import logging
from builtins import str
from collections import namedtuple
from distutils.version import LooseVersion

from distutils.version import LooseVersion
from django.core.exceptions import ObjectDoesNotExist
from django.core.exceptions import PermissionDenied
from django.core.exceptions import SuspiciousOperation
Expand Down Expand Up @@ -777,6 +777,11 @@ def create_node(node_data, parent_node, sort_order): # noqa: C901

def create_exercises(user, node, data):
""" Generate exercise from data """
# First check that all assessment_ids are unique within the node
assessment_ids = [question.get("assessment_id") for question in data]
if len(assessment_ids) != len(set(assessment_ids)):
raise NodeValidationError("Duplicate assessment_ids found in node {}".format(node.node_id))

with transaction.atomic():
order = 0

Expand Down