Skip to content
2 changes: 1 addition & 1 deletion src/openedx_content/applets/backup_restore/zipper.py
Original file line number Diff line number Diff line change
Expand Up @@ -780,7 +780,7 @@ def _save(
self._save_subsections(learning_package_obj, containers)
self._save_sections(learning_package_obj, containers)
self._save_collections(learning_package_obj, collections)
publishing_api.publish_all_drafts(learning_package_obj.id)
publishing_api.publish_all_drafts(learning_package_obj.id)

with publishing_api.bulk_draft_changes_for(learning_package_obj.id):
self._save_draft_versions(components, containers, component_static_files)
Expand Down
29 changes: 28 additions & 1 deletion src/openedx_content/applets/publishing/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from typing import ContextManager, Optional, cast

from django.contrib.auth import get_user_model
from django.core.exceptions import ObjectDoesNotExist
from django.core.exceptions import ObjectDoesNotExist, ValidationError
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Comment]: I've generally used ValueError instead of ValidationError for invalid arguments, though I realize that doesn't make sense for things like checking that you're not in a bulk change context before publishing. Should I switch over, do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these errors that are more developer-facing (you coded something wrong, not the user gave invalid input), I honestly don't think it makes much difference. I'm on the fence and happy to change to ValueError too if you'd like.

The python docs say

ValueError: Raised when an operation or function receives an argument that has the right type but an inappropriate value, and the situation is not described by a more precise exception

So I guess it depends if you consider ValidationError to be more precise. We could also define a custom exception that's even more specific, but I don't think it's worth it if we expect these to be pretty rare.

from django.db.models import F, OuterRef, Prefetch, Q, QuerySet, Subquery
from django.db.transaction import atomic, on_commit

Expand Down Expand Up @@ -237,6 +237,14 @@ def create_publishable_entity_version(
created_by_id=created_by,
)
if dependencies:
# Validate that dependencies are from the same learning package:
if (
PublishableEntity.objects.filter(id__in=dependencies)
.exclude(learning_package_id=version.entity.learning_package_id)
.exists()
):
raise ValidationError("Dependencies must be from the same learning package")
# Store dependencies:
set_version_dependencies(version.id, dependencies)

set_draft_version(
Expand Down Expand Up @@ -466,6 +474,8 @@ def publish_from_drafts(
By default, this will also publish all dependencies (e.g. unpinned children)
of the Drafts that are passed in.
"""
if DraftChangeLogContext.get_active_draft_change_log(learning_package_id) is not None:
raise ValidationError("Cannot publish while in bulk_draft_changes_for().")
if published_at is None:
published_at = datetime.now(tz=timezone.utc)

Expand Down Expand Up @@ -503,6 +513,12 @@ def publish_from_drafts(
# Skip duplicates that we might get from expanding dependencies.
if draft.pk in published_draft_ids:
continue
# Validate Learning Package here where it won't require any extra queries
if draft.entity.learning_package_id != learning_package_id:
raise ValidationError(
f"Draft entity (id={draft.entity.id}) is from learning package "
f"{draft.entity.learning_package_id}; expected learning package {learning_package_id}."
)

try:
old_version = draft.entity.published.version
Expand Down Expand Up @@ -919,6 +935,17 @@ def set_draft_version(
# block is bookkeeping in our DraftChangeLog.
draft.version_id = publishable_entity_version_pk

# Validate the entity
if publishable_entity_version_pk is not None:
if draft.entity.id != PublishableEntityVersion.objects.only("entity_id").get(
pk=publishable_entity_version_pk
).entity_id:
invalid_pev = PublishableEntityVersion.objects.get(pk=publishable_entity_version_pk)
raise ValidationError(
f"Entity mismatch - the specified PublishableEntityVersion ({repr(invalid_pev)}) does not match "
f"the PublishableEntity ({repr(draft.entity)})."
)

# Check to see if we're inside a context manager for an active
# DraftChangeLog (i.e. what happens if the caller is using the public
# bulk_draft_changes_for() API call), or if we have to make our own.
Expand Down
4 changes: 2 additions & 2 deletions tests/openedx_content/applets/containers/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -373,10 +373,10 @@ def test_create_container_queries(lp: LearningPackage, child_entity1: TestEntity
"container_cls": TestContainer,
}
# The exact numbers here aren't too important - this is just to alert us if anything significant changes.
with django_assert_num_queries(33):
with django_assert_num_queries(34):
containers_api.create_container_and_version(lp.id, container_code="c1", **base_args)
# And try with a a container that has children:
with django_assert_num_queries(34):
with django_assert_num_queries(37):
containers_api.create_container_and_version(lp.id, container_code="c2", **base_args, entities=[child_entity1])


Expand Down
Loading