From bfd0bef0c9935d4f42411ff340d12ac4598c5972 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Thu, 29 Sep 2022 11:10:05 -0700 Subject: [PATCH 1/5] Handle case where multiple content tags exist for tag_name and null channel_id. --- contentcuration/contentcuration/utils/sync.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/contentcuration/contentcuration/utils/sync.py b/contentcuration/contentcuration/utils/sync.py index cba8220da6..c42be1d99d 100644 --- a/contentcuration/contentcuration/utils/sync.py +++ b/contentcuration/contentcuration/utils/sync.py @@ -97,9 +97,11 @@ def sync_node_tags(node, original): for tag in original.tags.exclude( tag_name__in=node.tags.values_list("tag_name", flat=True) ): - new_tag, _ = ContentTag.objects.get_or_create( + new_tag = ContentTag.objects.filter( tag_name=tag.tag_name, channel_id=None, - ) + ).first() + if not new_tag: + new_tag = ContentTag.objects.create(tag_name=tag.tag_name, channel_id=None) node.tags.add(new_tag) node.changed = True From c0eb5dfbddc8180828dd96249b6cca378d65e069 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Thu, 29 Sep 2022 11:10:44 -0700 Subject: [PATCH 2/5] Remove unused decorator from models.py --- contentcuration/contentcuration/models.py | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index cc68b58855..f7bfe2fdc6 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -1,4 +1,3 @@ -import functools import hashlib import json import logging @@ -8,7 +7,6 @@ from datetime import datetime import pytz -from celery import states from django.conf import settings from django.contrib.auth.base_user import AbstractBaseUser from django.contrib.auth.base_user import BaseUserManager @@ -1106,20 +1104,6 @@ class Meta: unique_together = ['tag_name', 'channel'] -def delegate_manager(method): - """ - Delegate method calls to base manager, if exists. - """ - - @functools.wraps(method) - def wrapped(self, *args, **kwargs): - if self._base_manager: - return getattr(self._base_manager, method.__name__)(*args, **kwargs) - return method(self, *args, **kwargs) - - return wrapped - - class License(models.Model): """ Normalize the license of ContentNode model From dd226be71996d9ad970927f4e708506f36b4ff03 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Thu, 29 Sep 2022 12:13:49 -0700 Subject: [PATCH 3/5] Add tests for tag syncing and regression test for fix. --- .../contentcuration/tests/test_sync.py | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/contentcuration/contentcuration/tests/test_sync.py b/contentcuration/contentcuration/tests/test_sync.py index 24b31d6c62..5948ea0d06 100644 --- a/contentcuration/contentcuration/tests/test_sync.py +++ b/contentcuration/contentcuration/tests/test_sync.py @@ -6,6 +6,7 @@ from .testdata import create_temp_file from contentcuration.models import AssessmentItem from contentcuration.models import Channel +from contentcuration.models import ContentTag from contentcuration.utils.publish import mark_all_nodes_as_published from contentcuration.utils.sync import sync_channel @@ -156,3 +157,102 @@ def test_sync_assessment_item_add(self): self.assertEqual(target_ai.files.filter(checksum=db_file.checksum).count(), 1) self.assertTrue(self.derivative_channel.has_changes()) + + def test_sync_tags_add(self): + """ + Test that calling sync_node_tags successfully syncs a tag added to the original node to + the copied node. + """ + + self.assertFalse(self.derivative_channel.has_changes()) + + contentnode = ( + self.channel.main_tree.get_descendants() + .exclude(kind_id=content_kinds.TOPIC) + .first() + ) + + target_child = self.derivative_channel.main_tree.get_descendants().get( + source_node_id=contentnode.node_id + ) + + self.assertIsNotNone(target_child) + self.assertEqual( + target_child.tags.count(), contentnode.tags.count() + ) + + tag = ContentTag.objects.create(tag_name="tagname") + + contentnode.tags.add(tag) + + self.assertNotEqual( + target_child.tags.count(), contentnode.tags.count() + ) + + sync_channel(self.derivative_channel, sync_tags=True) + self.derivative_channel.main_tree.refresh_from_db() + + self.assertEqual( + target_child.tags.count(), contentnode.tags.count() + ) + + self.assertEqual( + target_child.tags.filter( + tag_name=tag.tag_name + ).count(), + 1, + ) + + self.assertTrue(self.derivative_channel.has_changes()) + + def test_sync_tags_add_multiple_tags(self): + """ + Test that calling sync_node_tags does not raise an error when there + are multiple tags with the same tag_name and null channel_id. + """ + + self.assertFalse(self.derivative_channel.has_changes()) + + contentnode = ( + self.channel.main_tree.get_descendants() + .exclude(kind_id=content_kinds.TOPIC) + .first() + ) + + target_child = self.derivative_channel.main_tree.get_descendants().get( + source_node_id=contentnode.node_id + ) + + self.assertIsNotNone(target_child) + self.assertEqual( + target_child.tags.count(), contentnode.tags.count() + ) + + # Create the same tag twice + ContentTag.objects.create(tag_name="tagname") + + tag = ContentTag.objects.create(tag_name="tagname") + + contentnode.tags.add(tag) + + self.assertNotEqual( + target_child.tags.count(), contentnode.tags.count() + ) + try: + sync_channel(self.derivative_channel, sync_tags=True) + except Exception as e: + self.fail("Could not run sync_channel without raising exception: {}".format(e)) + self.derivative_channel.main_tree.refresh_from_db() + + self.assertEqual( + target_child.tags.count(), contentnode.tags.count() + ) + + self.assertEqual( + target_child.tags.filter( + tag_name=tag.tag_name + ).count(), + 1, + ) + + self.assertTrue(self.derivative_channel.has_changes()) From 292e59c02b3672a45db895451888799929112b27 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Thu, 29 Sep 2022 12:14:16 -0700 Subject: [PATCH 4/5] Set original_channel_id on the frontend to prevent errors on first render. --- .../frontend/shared/data/__tests__/ContentNodeResource.spec.js | 1 + .../contentcuration/frontend/shared/data/resources.js | 1 + 2 files changed, 2 insertions(+) diff --git a/contentcuration/contentcuration/frontend/shared/data/__tests__/ContentNodeResource.spec.js b/contentcuration/contentcuration/frontend/shared/data/__tests__/ContentNodeResource.spec.js index de89ccf042..fc0dcb9a83 100644 --- a/contentcuration/contentcuration/frontend/shared/data/__tests__/ContentNodeResource.spec.js +++ b/contentcuration/contentcuration/frontend/shared/data/__tests__/ContentNodeResource.spec.js @@ -664,6 +664,7 @@ describe('ContentNode methods', () => { parent: parent.id, lft: 1, node_id: expect.not.stringMatching(new RegExp(`${node.node_id}|${parent.node_id}`)), + original_channel_id: node.original_channel_id || node.channel_id, original_source_node_id: node.original_source_node_id, source_channel_id: node.channel_id, source_node_id: node.node_id, diff --git a/contentcuration/contentcuration/frontend/shared/data/resources.js b/contentcuration/contentcuration/frontend/shared/data/resources.js index 0895babd25..eb358a3519 100644 --- a/contentcuration/contentcuration/frontend/shared/data/resources.js +++ b/contentcuration/contentcuration/frontend/shared/data/resources.js @@ -1392,6 +1392,7 @@ export const ContentNode = new TreeResource({ // Placeholder node_id, we should receive the new value from backend result node_id: uuid4(), original_source_node_id: node.original_source_node_id || node.node_id, + original_channel_id: node.original_channel_id || node.channel_id, source_channel_id: node.channel_id, source_node_id: node.node_id, channel_id: parent.channel_id, From 10f4e34f7db9c8c98a7ac3a7a26aa2ad06adac99 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Thu, 29 Sep 2022 12:14:34 -0700 Subject: [PATCH 5/5] Only prevent refresh from the backend if changes have not been synced. --- .../contentcuration/frontend/shared/data/resources.js | 1 + 1 file changed, 1 insertion(+) diff --git a/contentcuration/contentcuration/frontend/shared/data/resources.js b/contentcuration/contentcuration/frontend/shared/data/resources.js index eb358a3519..9a38ab37f6 100644 --- a/contentcuration/contentcuration/frontend/shared/data/resources.js +++ b/contentcuration/contentcuration/frontend/shared/data/resources.js @@ -692,6 +692,7 @@ class Resource extends mix(APIResource, IndexedDBResource) { // Only fetch new updates if we've finished syncing the changes table db[CHANGES_TABLE].where('table') .equals(this.tableName) + .filter(c => !c.synced) .limit(1) .toArray() .then(pendingChanges => {