From 8f2afd1c28cbcc8692f8bcd4feca3c3c3fab8b4a Mon Sep 17 00:00:00 2001 From: Jacob Pierce Date: Thu, 29 Aug 2024 19:09:16 -0700 Subject: [PATCH 1/4] regression test for content node remaining complete after update change on file --- .../tests/viewsets/test_file.py | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/contentcuration/contentcuration/tests/viewsets/test_file.py b/contentcuration/contentcuration/tests/viewsets/test_file.py index dd5e010552..99b27410a2 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_file.py +++ b/contentcuration/contentcuration/tests/viewsets/test_file.py @@ -108,6 +108,34 @@ def test_update_file_no_channel(self): models.File.objects.get(id=file.id).contentnode_id, contentnode_id, ) + def test_update_file_with_complete_contentnode(self): + file_data = self.file_db_metadata + del file_data["contentnode_id"] + file = models.File.objects.create(**file_data) + + complete_except_no_file = models.ContentNode( + title="yes", + kind_id=file.preset, + parent=self.channel.main_tree, + license_id=models.License.objects.first().id, + license_description="don't do this!", + copyright_holder="Some person" + ) + errors = complete_except_no_file.mark_complete() + complete_except_no_file.save() + + self.assertTrue(len(errors) > 0) + + self.assertEqual(complete_except_no_file.complete, False) + + self.sync_changes( + [generate_update_event(file.id, FILE, {"contentnode": complete_except_no_file.id}, channel_id=self.channel.id)], + ) + + complete_except_no_file.refresh_from_db() + + self.assertTrue(complete_except_no_file.complete) + def test_update_file_no_channel_permission(self): file = models.File.objects.create(**self.file_db_metadata) new_preset = format_presets.VIDEO_HIGH_RES From 5736b63537f8177f932e0a66e57ed0f69d0a8624 Mon Sep 17 00:00:00 2001 From: Jacob Pierce Date: Fri, 30 Aug 2024 15:00:52 -0700 Subject: [PATCH 2/4] contentnode.mark_complete in FileSerializer#update FileViewset mixes in UpdateModelMixin in place of BulkUpdateMixin to resolve primary issue, but this results in needing to call on_update directly to avoid duplicate content_ids --- contentcuration/contentcuration/viewsets/file.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/contentcuration/contentcuration/viewsets/file.py b/contentcuration/contentcuration/viewsets/file.py index a4131dce0e..77e4129bf8 100644 --- a/contentcuration/contentcuration/viewsets/file.py +++ b/contentcuration/contentcuration/viewsets/file.py @@ -20,9 +20,9 @@ from contentcuration.viewsets.base import BulkDeleteMixin from contentcuration.viewsets.base import BulkListSerializer from contentcuration.viewsets.base import BulkModelSerializer -from contentcuration.viewsets.base import BulkUpdateMixin from contentcuration.viewsets.base import ReadOnlyValuesViewset from contentcuration.viewsets.base import RequiredFilterSet +from contentcuration.viewsets.base import UpdateModelMixin from contentcuration.viewsets.common import UserFilteredPrimaryKeyRelatedField from contentcuration.viewsets.common import UUIDInFilter @@ -53,6 +53,7 @@ class FileSerializer(BulkModelSerializer): ) def update(self, instance, validated_data): + update_node = None if "contentnode" in validated_data: # if we're updating the file's related node, we'll trigger a reset for the # old channel's cache modified date @@ -61,6 +62,12 @@ def update(self, instance, validated_data): ResourceSizeCache.reset_modified_for_file(instance) results = super(FileSerializer, self).update(instance, validated_data) + results.on_update() # Make sure contentnode.content_id is unique + + if results.contentnode: + results.contentnode.refresh_from_db() + if not len(results.contentnode.mark_complete()): + results.contentnode.save() if instance.uploaded_by_id: calculate_user_storage(instance.uploaded_by_id) return results @@ -83,7 +90,7 @@ def retrieve_storage_url(item): return generate_storage_url("{}.{}".format(item["checksum"], item["file_format"])) -class FileViewSet(BulkDeleteMixin, BulkUpdateMixin, ReadOnlyValuesViewset): +class FileViewSet(BulkDeleteMixin, UpdateModelMixin, ReadOnlyValuesViewset): queryset = File.objects.all() serializer_class = FileSerializer permission_classes = [IsAuthenticated] From fad1c7461c4d6003e1ed913add49554939a610ee Mon Sep 17 00:00:00 2001 From: Jacob Pierce Date: Thu, 5 Sep 2024 10:32:16 -0700 Subject: [PATCH 3/4] create Change event on file-upload when contentnode is complete; cover in test --- .../tests/viewsets/test_file.py | 5 +++ .../contentcuration/viewsets/file.py | 38 +++++++++++++++---- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/contentcuration/contentcuration/tests/viewsets/test_file.py b/contentcuration/contentcuration/tests/viewsets/test_file.py index 99b27410a2..9c7c78a1a5 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_file.py +++ b/contentcuration/contentcuration/tests/viewsets/test_file.py @@ -6,6 +6,7 @@ from le_utils.constants import content_kinds from le_utils.constants import file_formats from le_utils.constants import format_presets +from mock import MagicMock from contentcuration import models from contentcuration.tests import testdata @@ -128,10 +129,14 @@ def test_update_file_with_complete_contentnode(self): self.assertEqual(complete_except_no_file.complete, False) + models.Change.create_change = MagicMock() + self.sync_changes( [generate_update_event(file.id, FILE, {"contentnode": complete_except_no_file.id}, channel_id=self.channel.id)], ) + self.assertTrue(models.Change.create_change.called) + complete_except_no_file.refresh_from_db() self.assertTrue(complete_except_no_file.complete) diff --git a/contentcuration/contentcuration/viewsets/file.py b/contentcuration/contentcuration/viewsets/file.py index 77e4129bf8..932620835e 100644 --- a/contentcuration/contentcuration/viewsets/file.py +++ b/contentcuration/contentcuration/viewsets/file.py @@ -9,6 +9,7 @@ from rest_framework.response import Response from contentcuration.models import AssessmentItem +from contentcuration.models import Change from contentcuration.models import ContentNode from contentcuration.models import File from contentcuration.models import generate_object_storage_name @@ -25,6 +26,8 @@ from contentcuration.viewsets.base import UpdateModelMixin from contentcuration.viewsets.common import UserFilteredPrimaryKeyRelatedField from contentcuration.viewsets.common import UUIDInFilter +from contentcuration.viewsets.sync.constants import CONTENTNODE +from contentcuration.viewsets.sync.utils import generate_update_event class FileFilter(RequiredFilterSet): @@ -68,8 +71,20 @@ def update(self, instance, validated_data): results.contentnode.refresh_from_db() if not len(results.contentnode.mark_complete()): results.contentnode.save() + Change.create_change( + generate_update_event( + results.contentnode.id, + CONTENTNODE, + {"complete": True}, + channel_id=results.contentnode.get_channel_id(), + ), + created_by_id=instance.uploaded_by_id, + applied=True, + ) + if instance.uploaded_by_id: calculate_user_storage(instance.uploaded_by_id) + return results class Meta: @@ -80,13 +95,13 @@ class Meta: "contentnode", "assessment_item", "preset", - "duration" + "duration", ) list_serializer_class = BulkListSerializer def retrieve_storage_url(item): - """ Get the file_on_disk url """ + """Get the file_on_disk url""" return generate_storage_url("{}.{}".format(item["checksum"], item["file_format"])) @@ -108,7 +123,7 @@ class FileViewSet(BulkDeleteMixin, UpdateModelMixin, ReadOnlyValuesViewset): "language_id", "original_filename", "uploaded_by", - "duration" + "duration", ) field_map = { @@ -129,7 +144,9 @@ def delete_from_changes(self, changes): # Find all root nodes for files, and reset the cache modified date. root_nodes = ContentNode.objects.filter( parent__isnull=True, - tree_id__in=files_qs.values_list('contentnode__tree_id', flat=True).distinct(), + tree_id__in=files_qs.values_list( + "contentnode__tree_id", flat=True + ).distinct(), ) for root_node in root_nodes: ResourceSizeCache(root_node).reset_modified(None) @@ -162,16 +179,23 @@ def upload_url(self, request): return HttpResponseBadRequest(reason="File duration must be a number") duration = math.floor(duration) if duration <= 0: - return HttpResponseBadRequest(reason="File duration is equal to or less than 0") + return HttpResponseBadRequest( + reason="File duration is equal to or less than 0" + ) try: request.user.check_space(float(size), checksum) except PermissionDenied: - return HttpResponseBadRequest(reason="Not enough space. Check your storage under Settings page.", status=412) + return HttpResponseBadRequest( + reason="Not enough space. Check your storage under Settings page.", + status=412, + ) might_skip = File.objects.filter(checksum=checksum).exists() - filepath = generate_object_storage_name(checksum, filename, default_ext=file_format) + filepath = generate_object_storage_name( + checksum, filename, default_ext=file_format + ) checksum_base64 = codecs.encode( codecs.decode(checksum, "hex"), "base64" ).decode() From 84520f08795e92f04fb8dc2389dd268773648fac Mon Sep 17 00:00:00 2001 From: Jacob Pierce Date: Mon, 16 Sep 2024 13:08:38 -0700 Subject: [PATCH 4/4] test_file: query DB rather than use mocks --- .../contentcuration/tests/viewsets/test_file.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/contentcuration/contentcuration/tests/viewsets/test_file.py b/contentcuration/contentcuration/tests/viewsets/test_file.py index 9c7c78a1a5..2a4e4e2376 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_file.py +++ b/contentcuration/contentcuration/tests/viewsets/test_file.py @@ -6,7 +6,6 @@ from le_utils.constants import content_kinds from le_utils.constants import file_formats from le_utils.constants import format_presets -from mock import MagicMock from contentcuration import models from contentcuration.tests import testdata @@ -15,6 +14,7 @@ from contentcuration.tests.viewsets.base import generate_delete_event from contentcuration.tests.viewsets.base import generate_update_event from contentcuration.tests.viewsets.base import SyncTestMixin +from contentcuration.viewsets.sync.constants import CONTENTNODE from contentcuration.viewsets.sync.constants import FILE @@ -129,13 +129,16 @@ def test_update_file_with_complete_contentnode(self): self.assertEqual(complete_except_no_file.complete, False) - models.Change.create_change = MagicMock() - self.sync_changes( [generate_update_event(file.id, FILE, {"contentnode": complete_except_no_file.id}, channel_id=self.channel.id)], ) - self.assertTrue(models.Change.create_change.called) + # We should see two Changes, one of them should be for the CONTENTNODE table + self.assertEqual(models.Change.objects.count(), 2) + self.assertEqual( + models.Change.objects.filter(table=CONTENTNODE).count(), + 1 + ) complete_except_no_file.refresh_from_db()