From 8a9293eb6b30e92bd329ceb9ce4ccc9d535ae7f1 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Fri, 9 Aug 2024 12:52:33 -0700 Subject: [PATCH] Update backend change event generation to mark some changes as unpublishable. Mark all changes from tables that are not important for channel publishing as unpublishable. Add test coverage for new behaviour. --- .../0149_unpublishable_change_field.py | 25 +++ contentcuration/contentcuration/models.py | 45 +++++- .../contentcuration/tests/viewsets/base.py | 7 + .../tests/viewsets/test_channel.py | 148 ++++++++++++++++++ .../tests/viewsets/test_contentnode.py | 19 +++ .../contentcuration/views/internal.py | 3 +- .../contentcuration/viewsets/base.py | 6 +- .../contentcuration/viewsets/channel.py | 14 +- .../contentcuration/viewsets/contentnode.py | 4 +- .../viewsets/sync/constants.py | 12 ++ 10 files changed, 267 insertions(+), 16 deletions(-) create mode 100644 contentcuration/contentcuration/migrations/0149_unpublishable_change_field.py diff --git a/contentcuration/contentcuration/migrations/0149_unpublishable_change_field.py b/contentcuration/contentcuration/migrations/0149_unpublishable_change_field.py new file mode 100644 index 0000000000..a1ebff4d29 --- /dev/null +++ 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 index b9e7864619..09d481c749 100644 --- 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" @@ -2545,14 +2547,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, @@ -2562,21 +2586,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 index f02ff3c4d1..0dcaf6eff8 100644 --- 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 @@ -55,6 +56,12 @@ def generate_deploy_channel_event(channel_id, user_id): 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 index 505fbd836b..271367e58f 100644 --- 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 index 11d4dd6147..3e0b83953e 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_contentnode.py +++ b/contentcuration/contentcuration/tests/viewsets/test_contentnode.py @@ -27,9 +27,11 @@ 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 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 @@ -1154,6 +1156,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 index 32733d7d4e..13eba6376f 100644 --- 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 index 38eb177064..161588a675 100644 --- 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 index fdd9c99cd8..ee06244836 100644 --- a/contentcuration/contentcuration/viewsets/channel.py +++ b/contentcuration/contentcuration/viewsets/channel.py @@ -397,10 +397,12 @@ def _unpublished_changes_query(channel): change_type=PUBLISHED, errored=False ).values("server_rev").order_by("-server_rev")[:1], Value(0)), - created_by__isnull=False, channel=channel, - user__isnull=True - ) + # Going forwards, these changes will be marked as unpublishable, + # but leave these filters here for now for backwards compatibility + created_by__isnull=False, + user__isnull=True, + ).exclude(unpublishable=True) class ChannelViewSet(ValuesViewset): @@ -528,20 +530,20 @@ def publish(self, pk, version_notes="", language=None): "unpublished_changes": _unpublished_changes_query(channel).exists() }, channel_id=channel.id ), - ], applied=True) + ], applied=True, unpublishable=True) except ChannelIncompleteError: Change.create_changes([ generate_update_event( channel.id, CHANNEL, {"publishing": False}, channel_id=channel.id ), - ], applied=True) + ], applied=True, unpublishable=True) raise ValidationError("Channel is not ready to be published") except Exception: Change.create_changes([ generate_update_event( channel.id, CHANNEL, {"publishing": False, "unpublished_changes": True}, channel_id=channel.id ), - ], applied=True) + ], applied=True, unpublishable=True) raise def sync_from_changes(self, changes): diff --git a/contentcuration/contentcuration/viewsets/contentnode.py b/contentcuration/contentcuration/viewsets/contentnode.py index 229d4f81ad..16d7804270 100644 --- a/contentcuration/contentcuration/viewsets/contentnode.py +++ b/contentcuration/contentcuration/viewsets/contentnode.py @@ -16,9 +16,9 @@ from django.http import Http404 from django.utils.timezone import now from django_cte import CTEQuerySet +from django_filters.rest_framework import BooleanFilter from django_filters.rest_framework import CharFilter from django_filters.rest_framework import UUIDFilter -from django_filters.rest_framework import BooleanFilter from le_utils.constants import completion_criteria from le_utils.constants import content_kinds from le_utils.constants import roles @@ -969,6 +969,8 @@ def copy( ), applied=True, created_by_id=self.request.user.id, + # This is not a publishable change, as it is just updating ephemeral status updates. + unpublishable=True, ) def perform_create(self, serializer, change=None): diff --git a/contentcuration/contentcuration/viewsets/sync/constants.py b/contentcuration/contentcuration/viewsets/sync/constants.py index ace4ebaf7b..df6d1afea0 100644 --- a/contentcuration/contentcuration/viewsets/sync/constants.py +++ b/contentcuration/contentcuration/viewsets/sync/constants.py @@ -54,6 +54,18 @@ ] ) +# Some edits have implications for channels, +# but do not affect whether a channel is publishable or not +# only edits to these tables are considered publishable changes +# although individual changes can still be marked as unpublishable. +PUBLISHABLE_CHANGE_TABLES = set([ + CHANNEL, + CONTENTNODE, + CONTENTNODE_PREREQUISITE, + ASSESSMENTITEM, + FILE, +]) + # Enum for copying states class COPYING_STATUS_VALUES: