diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 2d5233770..801f95057 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -9,7 +9,7 @@ from datetime import datetime, timezone from django.core.exceptions import ObjectDoesNotExist -from django.db.models import F, QuerySet +from django.db.models import F, Q, QuerySet from django.db.transaction import atomic from .model_mixins import PublishableContentModelRegistry, PublishableEntityMixin, PublishableEntityVersionMixin @@ -245,10 +245,23 @@ def publish_all_drafts( """ Publish everything that is a Draft and is not already published. """ - draft_qset = Draft.objects \ - .select_related("entity__published") \ - .filter(entity__learning_package_id=learning_package_id) \ - .exclude(entity__published__version_id=F("version_id")) + draft_qset = ( + Draft.objects.select_related("entity__published") + .filter(entity__learning_package_id=learning_package_id) + + # Exclude entities where the Published version already matches the + # Draft version. + .exclude(entity__published__version_id=F("version_id")) + + # Account for soft-deletes: + # NULL != NULL in SQL, so simply excluding entities where the Draft + # and Published versions match will not catch the case where a + # soft-delete has been published (i.e. both the Draft and Published + # versions are NULL). We need to explicitly check for that case + # instead, or else we will re-publish the same soft-deletes over + # and over again. + .exclude(Q(version__isnull=True) & Q(entity__published__version__isnull=True)) + ) return publish_from_drafts( learning_package_id, draft_qset, message, published_at, published_by ) diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index 03d9ebf06..a9e7bf855 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -152,6 +152,49 @@ def test_draft_lifecycle(self) -> None: deleted_entity_version = publishing_api.get_draft_version(entity.id) assert deleted_entity_version is None + def test_soft_deletes(self) -> None: + """Test the publishing behavior of soft deletes.""" + entity = publishing_api.create_publishable_entity( + self.learning_package.id, + "my_entity", + created=self.now, + created_by=None, + ) + entity_version = publishing_api.create_publishable_entity_version( + entity.id, + version_num=1, + title="An Entity 🌴", + created=self.now, + created_by=None, + ) + + # Initial publish + publish_log = publishing_api.publish_all_drafts(self.learning_package.id) + log_records = list(publish_log.records.all()) + assert len(log_records) == 1 + record = log_records[0] + assert record.entity_id == entity.id + assert record.old_version is None + assert record.new_version_id == entity_version.id + + # Publishing the soft-delete + publishing_api.soft_delete_draft(entity.id) + publish_log = publishing_api.publish_all_drafts(self.learning_package.id) + log_records = list(publish_log.records.all()) + assert len(log_records) == 1 + record = log_records[0] + assert record.entity_id == entity.id + assert record.old_version_id == entity_version.id + assert record.new_version is None + + # Verify that we do not re-publish soft-deleted records. We initially + # had a bug here because NULL != NULL in SQL, so the check to "publish + # all the Drafts that have different versions than their Published + # counterparts" would mistakenly pull in records that were NULL in both + # places. + publish_log = publishing_api.publish_all_drafts(self.learning_package.id) + assert publish_log.records.count() == 0 + def test_reset_drafts_to_published(self) -> None: """ Test throwing out Draft data and resetting to the Published versions.