Merge pull request #4632 from rtibbles/this_is_unpublishable

bjester · web-flow · commit 9db159ada08f · 2024-08-23T11:06:21.000-07:00
Update backend change event generation to mark some changes as unpublishable
diff --git a/contentcuration/contentcuration/migrations/0149_unpublishable_change_field.py b/contentcuration/contentcuration/migrations/0149_unpublishable_change_field.py
@@ -0,0 +1,25 @@
+# Generated by Django 3.2.19 on 2024-08-09 18:28
+from django.db import migrations
+from django.db import models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('contentcuration', '0148_flagfeedbackevent_recommendationsevent_recommendationsinteractionevent'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='change',
+            name='unpublishable',
+            field=models.BooleanField(blank=True, null=True),
+        ),
+        # Add default to False in a separate migration operation
+        # to avoid expensive backfilling of the new column for existing rows
+        migrations.AlterField(
+            model_name='change',
+            name='unpublishable',
+            field=models.BooleanField(blank=True, default=False, null=True),
+        ),
+    ]
diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py
@@ -77,6 +77,8 @@
 from contentcuration.utils.parser import load_json_string
 from contentcuration.viewsets.sync.constants import ALL_CHANGES
 from contentcuration.viewsets.sync.constants import ALL_TABLES
+from contentcuration.viewsets.sync.constants import PUBLISHABLE_CHANGE_TABLES
+from contentcuration.viewsets.sync.constants import PUBLISHED
 
 
 EDIT_ACCESS = "edit"
@@ -2556,14 +2558,36 @@ class Change(models.Model):
     kwargs = JSONField(encoder=JSONEncoder)
     applied = models.BooleanField(default=False)
     errored = models.BooleanField(default=False)
+    # Add an additional flag for change events that are only intended
+    # to transmit a message to the client, and not to actually apply any publishable changes.
+    # Make it nullable, so that we don't have to back fill historic change objects, and we just
+    # exclude true values when we are looking for publishable changes.
+    # This deliberately uses 'unpublishable' so that we can easily filter out by 'not true',
+    # and also that if we are ever interacting with it in Python code, both null and False values
+    # will be falsy.
+    unpublishable = models.BooleanField(null=True, blank=True, default=False)
 
     @classmethod
-    def _create_from_change(cls, created_by_id=None, channel_id=None, user_id=None, session_key=None, applied=False, table=None, rev=None, **data):
+    def _create_from_change(
+        cls,
+        created_by_id=None,
+        channel_id=None,
+        user_id=None,
+        session_key=None,
+        applied=False,
+        table=None,
+        rev=None,
+        unpublishable=False,
+        **data
+    ):
         change_type = data.pop("type")
         if table is None or table not in ALL_TABLES:
             raise TypeError("table is a required argument for creating changes and must be a valid table name")
         if change_type is None or change_type not in ALL_CHANGES:
             raise TypeError("change_type is a required argument for creating changes and must be a valid change type integer")
+        # Don't let someone mark a change as unpublishable if it's not in the list of tables that make changes that we can publish
+        # also, by definition, publishing is not a publishable change - this probably doesn't matter, but making sense is nice.
+        unpublishable = unpublishable or table not in PUBLISHABLE_CHANGE_TABLES or change_type == PUBLISHED
         return cls(
             session_id=session_key,
             created_by_id=created_by_id,
@@ -2573,21 +2597,30 @@ def _create_from_change(cls, created_by_id=None, channel_id=None, user_id=None,
             table=table,
             change_type=change_type,
             kwargs=data,
-            applied=applied
+            applied=applied,
+            unpublishable=unpublishable,
         )
 
     @classmethod
-    def create_changes(cls, changes, created_by_id=None, session_key=None, applied=False):
+    def create_changes(cls, changes, created_by_id=None, session_key=None, applied=False, unpublishable=False):
         change_models = []
         for change in changes:
-            change_models.append(cls._create_from_change(created_by_id=created_by_id, session_key=session_key, applied=applied, **change))
+            change_models.append(
+                cls._create_from_change(
+                    created_by_id=created_by_id,
+                    session_key=session_key,
+                    applied=applied,
+                    unpublishable=unpublishable,
+                    **change
+                )
+            )
 
         cls.objects.bulk_create(change_models)
         return change_models
 
     @classmethod
-    def create_change(cls, change, created_by_id=None, session_key=None, applied=False):
-        obj = cls._create_from_change(created_by_id=created_by_id, session_key=session_key, applied=applied, **change)
+    def create_change(cls, change, created_by_id=None, session_key=None, applied=False, unpublishable=False):
+        obj = cls._create_from_change(created_by_id=created_by_id, session_key=session_key, applied=applied, unpublishable=unpublishable, **change)
         obj.save()
         return obj
 
diff --git a/contentcuration/contentcuration/tests/viewsets/base.py b/contentcuration/contentcuration/tests/viewsets/base.py
@@ -12,6 +12,7 @@
 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
 from contentcuration.viewsets.sync.utils import generate_deploy_event as base_generate_deploy_event
+from contentcuration.viewsets.sync.utils import generate_publish_event as base_generate_publish_event
 from contentcuration.viewsets.sync.utils import generate_update_event as base_generate_update_event
 from contentcuration.viewsets.sync.utils import generate_update_descendants_event as base_generate_update_descendants_event
 
@@ -60,6 +61,12 @@ def generate_update_descendants_event(*args, **kwargs):
     event["rev"] = random.randint(1, 10000000)
     return event
 
+def generate_publish_channel_event(channel_id):
+    event = base_generate_publish_event(channel_id)
+    event["rev"] = random.randint(1, 10000000)
+    return event
+
+
 class SyncTestMixin(object):
     celery_task_always_eager = None
 
diff --git a/contentcuration/contentcuration/tests/viewsets/test_channel.py b/contentcuration/contentcuration/tests/viewsets/test_channel.py
@@ -3,6 +3,8 @@
 import uuid
 
 import mock
+from django.db.models import Exists
+from django.db.models import OuterRef
 from django.urls import reverse
 from le_utils.constants import content_kinds
 
@@ -14,9 +16,11 @@
 from contentcuration.tests.viewsets.base import generate_create_event
 from contentcuration.tests.viewsets.base import generate_delete_event
 from contentcuration.tests.viewsets.base import generate_deploy_channel_event
+from contentcuration.tests.viewsets.base import generate_publish_channel_event
 from contentcuration.tests.viewsets.base import generate_sync_channel_event
 from contentcuration.tests.viewsets.base import generate_update_event
 from contentcuration.tests.viewsets.base import SyncTestMixin
+from contentcuration.viewsets.channel import _unpublished_changes_query
 from contentcuration.viewsets.sync.constants import CHANNEL
 
 
@@ -374,6 +378,19 @@ def test_deploy_with_staging_tree_None(self):
         self.assertNotEqual(modified_channel.main_tree, channel.staging_tree)
         self.assertNotEqual(modified_channel.previous_tree, channel.main_tree)
 
+    def test_publish_does_not_make_publishable(self):
+        user = testdata.user()
+        channel = models.Channel.objects.create(actor_id=user.id, **self.channel_metadata)
+        channel.editors.add(user)
+
+        self.sync_changes(
+            [
+                generate_publish_channel_event(channel.id)
+            ]
+        )
+
+        self.assertEqual(_unpublished_changes_query(channel).count(), 0)
+
 
 class CRUDTestCase(StudioAPITestCase):
     @property
@@ -466,3 +483,134 @@ def test_admin_restore_channel(self):
         channel = models.Channel.objects.get(id=channel.id)
         self.assertFalse(channel.deleted)
         self.assertEqual(1, channel.history.filter(actor=user, action=channel_history.RECOVERY).count())
+
+
+class UnpublishedChangesQueryTestCase(StudioAPITestCase):
+    def test_unpublished_changes_query_with_channel_object(self):
+        channel = testdata.channel()
+        user = testdata.user()
+        models.Change.create_change(generate_update_event(channel.id, CHANNEL, {"name": "new name"}, channel_id=channel.id), created_by_id=user.id)
+        models.Change.create_change(generate_publish_channel_event(channel.id), created_by_id=user.id)
+        models.Change.create_change(generate_update_event(channel.id, CHANNEL, {"name": "new name 2"}, channel_id=channel.id), created_by_id=user.id)
+
+        queryset = _unpublished_changes_query(channel)
+        self.assertEqual(queryset.count(), 1)
+        self.assertEqual(queryset[0].kwargs["mods"]["name"], "new name 2")
+
+    def test_unpublished_changes_query_with_channel_object_none_since_publish(self):
+        channel = testdata.channel()
+        user = testdata.user()
+        models.Change.create_change(generate_update_event(channel.id, CHANNEL, {"name": "new name"}, channel_id=channel.id), created_by_id=user.id)
+        models.Change.create_change(generate_update_event(channel.id, CHANNEL, {"name": "new name 2"}, channel_id=channel.id), created_by_id=user.id)
+        models.Change.create_change(generate_publish_channel_event(channel.id), created_by_id=user.id)
+
+        queryset = _unpublished_changes_query(channel)
+        self.assertEqual(queryset.count(), 0)
+
+    def test_unpublished_changes_query_with_channel_object_no_publishable_since_publish(self):
+        channel = testdata.channel()
+        user = testdata.user()
+        models.Change.create_change(generate_update_event(channel.id, CHANNEL, {"name": "new name"}, channel_id=channel.id), created_by_id=user.id)
+        models.Change.create_change(generate_publish_channel_event(channel.id), created_by_id=user.id)
+        models.Change.create_change(
+            generate_update_event(
+                channel.id,
+                CHANNEL,
+                {"name": "new name 2"},
+                channel_id=channel.id
+            ),
+            created_by_id=user.id,
+            unpublishable=True,
+        )
+
+        queryset = _unpublished_changes_query(channel)
+        self.assertEqual(queryset.count(), 0)
+
+    def test_unpublished_changes_query_with_channel_object_no_publishable_since_publish_if_publish_fails_through_error(self):
+        channel = testdata.channel()
+        user = testdata.user()
+        channel.main_tree = None
+        channel.save()
+        models.Change.create_change(generate_publish_channel_event(channel.id), created_by_id=user.id)
+
+        queryset = _unpublished_changes_query(channel)
+        self.assertEqual(queryset.count(), 0)
+
+    def test_unpublished_changes_query_with_channel_object_no_publishable_since_publish_if_publish_fails_because_incomplete(self):
+        channel = testdata.channel()
+        user = testdata.user()
+        channel.main_tree.complete = False
+        channel.save()
+        models.Change.create_change(generate_publish_channel_event(channel.id), created_by_id=user.id)
+
+        queryset = _unpublished_changes_query(channel)
+        self.assertEqual(queryset.count(), 0)
+
+    def test_unpublished_changes_query_with_outerref(self):
+        channel = testdata.channel()
+        user = testdata.user()
+        models.Change.create_change(generate_update_event(channel.id, CHANNEL, {"name": "new name"}, channel_id=channel.id), created_by_id=user.id)
+        models.Change.create_change(generate_publish_channel_event(channel.id), created_by_id=user.id)
+        models.Change.create_change(generate_update_event(channel.id, CHANNEL, {"name": "new name 2"}, channel_id=channel.id), created_by_id=user.id)
+
+        outer_ref = OuterRef("id")
+        unpublished_changes = _unpublished_changes_query(outer_ref)
+        channels = models.Channel.objects.filter(pk=channel.pk).annotate(unpublished_changes=Exists(unpublished_changes))
+        self.assertTrue(channels[0].unpublished_changes)
+
+    def test_unpublished_changes_query_with_outerref_none_since_publish(self):
+        channel = testdata.channel()
+        user = testdata.user()
+        models.Change.create_change(generate_update_event(channel.id, CHANNEL, {"name": "new name"}, channel_id=channel.id), created_by_id=user.id)
+        models.Change.create_change(generate_update_event(channel.id, CHANNEL, {"name": "new name 2"}, channel_id=channel.id), created_by_id=user.id)
+        models.Change.create_change(generate_publish_channel_event(channel.id), created_by_id=user.id)
+
+        outer_ref = OuterRef("id")
+        unpublished_changes = _unpublished_changes_query(outer_ref)
+        channels = models.Channel.objects.filter(pk=channel.pk).annotate(unpublished_changes=Exists(unpublished_changes))
+        self.assertFalse(channels[0].unpublished_changes)
+
+    def test_unpublished_changes_query_with_outerref_no_publishable_since_publish(self):
+        channel = testdata.channel()
+        user = testdata.user()
+        models.Change.create_change(generate_update_event(channel.id, CHANNEL, {"name": "new name"}, channel_id=channel.id), created_by_id=user.id)
+        models.Change.create_change(generate_publish_channel_event(channel.id), created_by_id=user.id)
+        models.Change.create_change(
+            generate_update_event(
+                channel.id,
+                CHANNEL,
+                {"name": "new name 2"},
+                channel_id=channel.id
+            ),
+            created_by_id=user.id,
+            unpublishable=True
+        )
+
+        outer_ref = OuterRef("id")
+        unpublished_changes = _unpublished_changes_query(outer_ref)
+        channels = models.Channel.objects.filter(pk=channel.pk).annotate(unpublished_changes=Exists(unpublished_changes))
+        self.assertFalse(channels[0].unpublished_changes)
+
+    def test_unpublished_changes_query_no_publishable_since_publish_if_publish_fails_through_error(self):
+        channel = testdata.channel()
+        user = testdata.user()
+        channel.main_tree = None
+        channel.save()
+        models.Change.create_change(generate_publish_channel_event(channel.id), created_by_id=user.id)
+
+        outer_ref = OuterRef("id")
+        unpublished_changes = _unpublished_changes_query(outer_ref)
+        channels = models.Channel.objects.filter(pk=channel.pk).annotate(unpublished_changes=Exists(unpublished_changes))
+        self.assertFalse(channels[0].unpublished_changes)
+
+    def test_unpublished_changes_query_no_publishable_since_publish_if_publish_fails_because_incomplete(self):
+        channel = testdata.channel()
+        user = testdata.user()
+        channel.main_tree.complete = False
+        channel.save()
+        models.Change.create_change(generate_publish_channel_event(channel.id), created_by_id=user.id)
+
+        outer_ref = OuterRef("id")
+        unpublished_changes = _unpublished_changes_query(outer_ref)
+        channels = models.Channel.objects.filter(pk=channel.pk).annotate(unpublished_changes=Exists(unpublished_changes))
+        self.assertFalse(channels[0].unpublished_changes)
diff --git a/contentcuration/contentcuration/tests/viewsets/test_contentnode.py b/contentcuration/contentcuration/tests/viewsets/test_contentnode.py
@@ -27,10 +27,12 @@
 from contentcuration.tests.viewsets.base import generate_copy_event
 from contentcuration.tests.viewsets.base import generate_create_event
 from contentcuration.tests.viewsets.base import generate_delete_event
+from contentcuration.tests.viewsets.base import generate_publish_channel_event
 from contentcuration.tests.viewsets.base import generate_update_event
 from contentcuration.tests.viewsets.base import generate_update_descendants_event
 from contentcuration.tests.viewsets.base import SyncTestMixin
 from contentcuration.utils.db_tools import TreeBuilder
+from contentcuration.viewsets.channel import _unpublished_changes_query
 from contentcuration.viewsets.contentnode import ContentNodeFilter
 from contentcuration.viewsets.sync.constants import CONTENTNODE
 from contentcuration.viewsets.sync.constants import CONTENTNODE_PREREQUISITE
@@ -1248,6 +1250,23 @@ def test_copy_contentnode(self):
 
         self.assertEqual(new_node.parent_id, self.channel.main_tree_id)
 
+    def test_copy_contentnode_finalization_does_not_make_publishable(self):
+        self.channel.editors.add(self.user)
+        contentnode = models.ContentNode.objects.create(**self.contentnode_db_metadata)
+        new_node_id = uuid.uuid4().hex
+        response = self.sync_changes(
+            [
+                generate_copy_event(
+                    new_node_id, CONTENTNODE, contentnode.id, self.channel.main_tree_id, channel_id=self.channel.id
+                ),
+                # Save a published change for the channel, so that the finalization change will be generated
+                # after the publish change, and we can check that it is properly not making the channel appear publishable.
+                generate_publish_channel_event(self.channel.id),
+            ],
+        )
+        self.assertEqual(response.status_code, 200, response.content)
+        self.assertEqual(_unpublished_changes_query(self.channel).count(), 0)
+
     def test_cannot_copy_contentnode__source_permission(self):
         source_channel = testdata.channel()
         contentnode = create_and_get_contentnode(source_channel.main_tree_id)
diff --git a/contentcuration/contentcuration/views/internal.py b/contentcuration/contentcuration/views/internal.py
@@ -236,7 +236,8 @@ def api_commit_channel(request):
             channel_id=channel_id,
         )
         # Send event (new staging tree or new main tree) for the channel
-        Change.create_change(event, created_by_id=request.user.id)
+        # mark is as unpublishable, as by itself a new staging tree is not a publishable change
+        Change.create_change(event, created_by_id=request.user.id, unpublishable=True)
 
         # Mark old staging tree for garbage collection
         if old_staging and old_staging != obj.main_tree:
diff --git a/contentcuration/contentcuration/viewsets/base.py b/contentcuration/contentcuration/viewsets/base.py
@@ -967,7 +967,8 @@ def update_progress(progress=None):
             custom_task_metadata_object.save()
 
     Change.create_change(
-        generate_update_event(pk, table, {TASK_ID: task_object.task_id}, channel_id=channel_id), applied=True
+        # These changes are purely for ephemeral progress updating, and do not constitute a publishable change.
+        generate_update_event(pk, table, {TASK_ID: task_object.task_id}, channel_id=channel_id), applied=True, unpublishable=True
     )
 
     tracker = ProgressTracker(task_id, update_progress)
@@ -982,8 +983,9 @@ def update_progress(progress=None):
     finally:
         if task_object.status == states.STARTED:
             # No error reported, cleanup.
+            # Mark as unpublishable, as this is a continuation of the progress updating, and not a publishable change.
             Change.create_change(
-                generate_update_event(pk, table, {TASK_ID: None}, channel_id=channel_id), applied=True
+                generate_update_event(pk, table, {TASK_ID: None}, channel_id=channel_id), applied=True, unpublishable=True
             )
             task_object.delete()
             custom_task_metadata_object.delete()
diff --git a/contentcuration/contentcuration/viewsets/channel.py b/contentcuration/contentcuration/viewsets/channel.py
diff --git a/contentcuration/contentcuration/viewsets/contentnode.py b/contentcuration/contentcuration/viewsets/contentnode.py
diff --git a/contentcuration/contentcuration/viewsets/sync/constants.py b/contentcuration/contentcuration/viewsets/sync/constants.py

Original file line number	Diff line number	Diff line change
`@@ -236,7 +236,8 @@ def api_commit_channel(request):`
`236`	`236`	`channel_id=channel_id,`
`237`	`237`	`)`
`238`	`238`	`# Send event (new staging tree or new main tree) for the channel`
`239`		`- Change.create_change(event, created_by_id=request.user.id)`
	`239`	`+ # mark is as unpublishable, as by itself a new staging tree is not a publishable change`
	`240`	`+ Change.create_change(event, created_by_id=request.user.id, unpublishable=True)`
`240`	`241`
`241`	`242`	`# Mark old staging tree for garbage collection`
`242`	`243`	`if old_staging and old_staging != obj.main_tree:`