Skip to content

Add license and title validation to views.internal::create_nodes #2999

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.

Sign up for GitHub

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

Merged
rtibbles merged 3 commits into from
Mar 12, 2021
Merged

Add license and title validation to views.internal::create_nodes #2999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec08ff0
Use consistent whitespace before ‘\’ continuation
jonboiser Feb 22, 2021
ba2889f
Move ‘|’ operator to beginning of line
jonboiser Feb 22, 2021
e34cc2c
Add title and license validation to create_node
jonboiser Feb 23, 2021
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
24 changes: 12 additions & 12 deletions contentcuration/contentcuration/management/commands/mark_incomplete.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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))
Expand All @@ -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)
Expand All @@ -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) \
Expand All @@ -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))

Expand Down
98 changes: 68 additions & 30 deletions contentcuration/contentcuration/tests/views/test_views_internal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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):
"""
Expand Down
22 changes: 17 additions & 5 deletions contentcuration/contentcuration/views/internal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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'],
Expand All @@ -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,
Expand All @@ -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()
Expand Down