Merge pull request #3803 from vkWeb/fix/content-id-guys

Fixes `content_id` bug whenever underlying content is changed 🎉

contentcuration/contentcuration/models.py

@@ -1778,6 +1778,16 @@ def mark_complete(self):  # noqa C901
         self.complete = not errors
         return errors
 
+    def make_content_id_unique(self):
+        """
+        If self is NOT an original contentnode (in other words, a copied contentnode)
+        and a contentnode with same content_id exists then we update self's content_id.
+        """
+        is_node_original = self.original_source_node_id is None or self.original_source_node_id == self.node_id
+        node_same_content_id = ContentNode.objects.exclude(pk=self.pk).filter(content_id=self.content_id)
+        if (not is_node_original) and node_same_content_id.exists():
+            ContentNode.objects.filter(pk=self.pk).update(content_id=uuid.uuid4().hex)
+
     def on_create(self):
         self.changed = True
         self.recalculate_editors_storage()
@@ -2052,6 +2062,28 @@ def filter_view_queryset(cls, queryset, user):
 
         return queryset.filter(Q(view=True) | Q(edit=True) | Q(public=True))
 
+    def on_create(self):
+        """
+        When an exercise is added to a contentnode, update its content_id
+        if it's a copied contentnode.
+        """
+        self.contentnode.make_content_id_unique()
+
+    def on_update(self):
+        """
+        When an exercise is updated of a contentnode, update its content_id
+        if it's a copied contentnode.
+        """
+        self.contentnode.make_content_id_unique()
+
+    def delete(self, *args, **kwargs):
+        """
+        When an exercise is deleted from a contentnode, update its content_id
+        if it's a copied contentnode.
+        """
+        self.contentnode.make_content_id_unique()
+        return super(AssessmentItem, self).delete(*args, **kwargs)
+
 
 class SlideshowSlide(models.Model):
     contentnode = models.ForeignKey('ContentNode', related_name="slideshow_slides", blank=True, null=True,
@@ -2169,10 +2201,19 @@ def filename(self):
 
         return os.path.basename(self.file_on_disk.name)
 
+    def update_contentnode_content_id(self):
+        """
+        If the file is attached to a contentnode and is not a thumbnail
+        then update that contentnode's content_id if it's a copied contentnode.
+        """
+        if self.contentnode and self.preset.thumbnail is False:
+            self.contentnode.make_content_id_unique()
+
     def on_update(self):
         # since modified was added later as a nullable field to File, we don't use a default but
         # instead we'll just make sure it's always updated through our serializers
         self.modified = timezone.now()
+        self.update_contentnode_content_id()
 
     def save(self, set_by_file_on_disk=True, *args, **kwargs):
         """

contentcuration/contentcuration/tests/test_models.py

@@ -439,6 +439,31 @@ def test_filter_by_pk__tree_id_updated_on_move(self):
             self.assertEqual(after_move_sourcenode.tree_id, testchannel.trash_tree.tree_id)
             self.assertEqual(tree_id_from_cache, testchannel.trash_tree.tree_id)
 
+    def test_make_content_id_unique(self):
+        channel_original = testdata.channel()
+        channel_importer = testdata.channel()
+
+        # Import a node from a channel.
+        original_node = channel_original.main_tree.get_descendants().first()
+        copied_node = original_node.copy_to(target=channel_importer.main_tree)
+
+        original_node.refresh_from_db()
+        copied_node.refresh_from_db()
+
+        original_node_old_content_id = original_node.content_id
+        copied_node_old_content_id = copied_node.content_id
+
+        original_node.make_content_id_unique()
+        copied_node.make_content_id_unique()
+
+        original_node.refresh_from_db()
+        copied_node.refresh_from_db()
+
+        # Assert that original node's content_id doesn't change.
+        self.assertEqual(original_node_old_content_id, original_node.content_id)
+        # Assert copied node's content_id changes.
+        self.assertNotEqual(copied_node_old_content_id, copied_node.content_id)
+
 
 class AssessmentItemTestCase(PermissionQuerysetTestCase):
     @property

contentcuration/contentcuration/tests/test_sync.py

@@ -1,14 +1,27 @@
 from __future__ import absolute_import
 
+import uuid
+
+from django.urls import reverse
 from le_utils.constants import content_kinds
+from le_utils.constants import file_formats
+from le_utils.constants import format_presets
 
 from .base import StudioTestCase
 from .testdata import create_temp_file
 from contentcuration.models import AssessmentItem
 from contentcuration.models import Channel
 from contentcuration.models import ContentTag
+from contentcuration.models import File
+from contentcuration.tests import testdata
+from contentcuration.tests.base import StudioAPITestCase
+from contentcuration.tests.viewsets.base import generate_create_event
+from contentcuration.tests.viewsets.base import generate_update_event
+from contentcuration.tests.viewsets.base import SyncTestMixin
 from contentcuration.utils.publish import mark_all_nodes_as_published
 from contentcuration.utils.sync import sync_channel
+from contentcuration.viewsets.sync.constants import ASSESSMENTITEM
+from contentcuration.viewsets.sync.constants import FILE
 
 
 class SyncTestCase(StudioTestCase):
@@ -256,3 +269,117 @@ def test_sync_tags_add_multiple_tags(self):
         )
 
         self.assertTrue(self.derivative_channel.has_changes())
+
+
+class ContentIDTestCase(SyncTestMixin, StudioAPITestCase):
+    def setUp(self):
+        super(ContentIDTestCase, self).setUp()
+        self.channel = testdata.channel()
+        self.user = testdata.user()
+        self.channel.editors.add(self.user)
+        self.client.force_authenticate(user=self.user)
+
+    def _get_assessmentitem_metadata(self, assessment_id=None, contentnode_id=None):
+        return {
+            "assessment_id": assessment_id or uuid.uuid4().hex,
+            "contentnode_id": contentnode_id or self.channel.main_tree.get_descendants()
+            .filter(kind_id=content_kinds.EXERCISE)
+            .first()
+            .id,
+        }
+
+    def _get_file_metadata(self):
+        return {
+            "size": 2500,
+            "checksum": uuid.uuid4().hex,
+            "name": "le_studio_file",
+            "file_format": file_formats.MP3,
+            "preset": format_presets.AUDIO,
+        }
+
+    def _upload_file_to_contentnode(self, file_metadata=None, contentnode_id=None):
+        """
+        This method mimics the frontend file upload process which is a two-step
+        process for the backend.
+        First, file's upload URL is fetched and then that file's ORM object is updated
+        to point to the contentnode.
+        """
+        file = file_metadata or self._get_file_metadata()
+        self.client.post(reverse("file-upload-url"), file, format="json",)
+        file_from_db = File.objects.get(checksum=file["checksum"])
+        self.sync_changes(
+            [generate_update_event(
+                file_from_db.id,
+                FILE,
+                {
+                    "contentnode": contentnode_id or self.channel.main_tree.get_descendants().first().id
+                },
+                channel_id=self.channel.id)],)
+        file_from_db.refresh_from_db()
+        return file_from_db
+
+    def _create_assessmentitem(self, assessmentitem, channel_id):
+        self.sync_changes(
+            [
+                generate_create_event(
+                    [assessmentitem["contentnode_id"], assessmentitem["assessment_id"]],
+                    ASSESSMENTITEM,
+                    assessmentitem,
+                    channel_id=channel_id,
+                )
+            ],
+        )
+
+    def test_content_id__becomes_equal_on_channel_sync_assessment_item(self):
+        # Make a copy of an existing assessmentitem contentnode.
+        assessmentitem_node = self.channel.main_tree.get_descendants().filter(kind_id=content_kinds.EXERCISE).first()
+        assessmentitem_node_copy = assessmentitem_node.copy_to(target=self.channel.main_tree)
+
+        # Create a new assessmentitem.
+        self._create_assessmentitem(
+            assessmentitem=self._get_assessmentitem_metadata(contentnode_id=assessmentitem_node_copy.id),
+            channel_id=self.channel.id
+        )
+
+        # Assert after creating a new assessmentitem on copied node, it's content_id is changed.
+        assessmentitem_node.refresh_from_db()
+        assessmentitem_node_copy.refresh_from_db()
+        self.assertNotEqual(assessmentitem_node.content_id, assessmentitem_node_copy.content_id)
+
+        # Syncs channel.
+        self.channel.main_tree.refresh_from_db()
+        self.channel.save()
+        sync_channel(
+            self.channel,
+            sync_assessment_items=True,
+        )
+
+        # Now after syncing the original and copied node should have same content_id.
+        assessmentitem_node.refresh_from_db()
+        assessmentitem_node_copy.refresh_from_db()
+        self.assertEqual(assessmentitem_node.content_id, assessmentitem_node_copy.content_id)
+
+    def test_content_id__becomes_equal_on_channel_sync_file(self):
+        file = self._upload_file_to_contentnode()
+        file_contentnode_copy = file.contentnode.copy_to(target=self.channel.main_tree)
+
+        # Upload a new file to the copied contentnode.
+        self._upload_file_to_contentnode(contentnode_id=file_contentnode_copy.id)
+
+        # Assert after new file upload, content_id changes.
+        file.contentnode.refresh_from_db()
+        file_contentnode_copy.refresh_from_db()
+        self.assertNotEqual(file.contentnode.content_id, file_contentnode_copy.content_id)
+
+        # Syncs channel.
+        self.channel.main_tree.refresh_from_db()
+        self.channel.save()
+        sync_channel(
+            self.channel,
+            sync_files=True,
+        )
+
+        # Assert that after channel syncing, content_id becomes equal.
+        file.contentnode.refresh_from_db()
+        file_contentnode_copy.refresh_from_db()
+        self.assertEqual(file.contentnode.content_id, file_contentnode_copy.content_id)

contentcuration/contentcuration/tests/viewsets/base.py

@@ -5,6 +5,9 @@
 from contentcuration.celery import app
 from contentcuration.models import Change
 from contentcuration.tests.helpers import clear_tasks
+from contentcuration.viewsets.sync.constants import CHANNEL
+from contentcuration.viewsets.sync.constants import SYNCED
+from contentcuration.viewsets.sync.utils import _generate_event as base_generate_event
 from contentcuration.viewsets.sync.utils import generate_copy_event as base_generate_copy_event
 from contentcuration.viewsets.sync.utils import generate_create_event as base_generate_create_event
 from contentcuration.viewsets.sync.utils import generate_delete_event as base_generate_delete_event
@@ -35,6 +38,16 @@ def generate_update_event(*args, **kwargs):
     return event
 
 
+def generate_sync_channel_event(channel_id, attributes, tags, files, assessment_items):
+    event = base_generate_event(key=channel_id, table=CHANNEL, event_type=SYNCED, channel_id=channel_id, user_id=None)
+    event["rev"] = random.randint(1, 10000000)
+    event["attributes"] = attributes
+    event["tags"] = tags
+    event["files"] = files
+    event["assessment_items"] = assessment_items
+    return event
+
+
 class SyncTestMixin(object):
     celery_task_always_eager = None