From 9f14c52d346e6492fa29cfb93ccdc370c654b1a9 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 3 Oct 2024 20:44:36 +0530 Subject: [PATCH 1/6] feat: add & remove collections to components --- .../apps/authoring/components/api.py | 58 ++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/openedx_learning/apps/authoring/components/api.py b/openedx_learning/apps/authoring/components/api.py index 3be562a85..aad28cf76 100644 --- a/openedx_learning/apps/authoring/components/api.py +++ b/openedx_learning/apps/authoring/components/api.py @@ -12,12 +12,13 @@ """ from __future__ import annotations -from datetime import datetime +from datetime import datetime, timezone from enum import StrEnum, auto from logging import getLogger from pathlib import Path from uuid import UUID +from django.core.exceptions import ValidationError from django.db.models import Q, QuerySet from django.db.transaction import atomic from django.http.response import HttpResponse, HttpResponseNotFound @@ -25,6 +26,7 @@ from ..contents import api as contents_api from ..publishing import api as publishing_api from .models import Component, ComponentType, ComponentVersion, ComponentVersionContent +from ..collections.models import Collection # The public API that will be re-exported by openedx_learning.apps.authoring.api # is listed in the __all__ entries below. Internal helper functions that are @@ -48,6 +50,8 @@ "look_up_component_version_content", "AssetError", "get_redirect_response_for_component_asset", + "add_collections", + "remove_collections", ] @@ -603,3 +607,55 @@ def _error_header(error: AssetError) -> dict[str, str]: ) return HttpResponse(headers={**info_headers, **redirect_headers}) + + +def add_collections( + learning_package_id: int, + component: Component, + collection_qset: QuerySet[Collection], + created_by: int | None = None, +) -> Component: + """ + Adds a QuerySet of Collection to Component. + + These Collections must belong to the same LearningPackage as the Component, or a ValidationError will be raised. + + Collections already in the Component.PublishableEntities are silently ignored. + + The all collection object's modified date is updated. + + Returns the updated component object. + """ + # Disallow adding entities outside the collection's learning package + invalid_collection = collection_qset.exclude(learning_package_id=learning_package_id).first() + if invalid_collection: + raise ValidationError( + f"Cannot add collection {invalid_collection.pk} in learning package " + f"{invalid_collection.learning_package_id} to component {component} in " + f"learning package {learning_package_id}." + ) + component.publishable_entity.collections.add( + *collection_qset.all(), + through_defaults={"created_by_id": created_by}, + ) + collection_qset.update(modified=datetime.now(tz=timezone.utc)) + + return component + + +def remove_collections( + learning_package_id: int, + component: Component, + collection_qset: QuerySet[Collection], +) -> Component: + """ + Removes a QuerySet of Collections from a Component. + + The Collections modified date is updated. + + Returns the updated component. + """ + component.publishable_entity.collections.remove(*collection_qset.all()) + collection_qset.update(modified=datetime.now(tz=timezone.utc)) + + return component From 5161de2094d8bf01c29fc456bb42c51fed19039c Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 4 Oct 2024 11:43:19 +0530 Subject: [PATCH 2/6] refactor: set collections in a component --- .../apps/authoring/components/api.py | 41 +++++++------------ 1 file changed, 14 insertions(+), 27 deletions(-) diff --git a/openedx_learning/apps/authoring/components/api.py b/openedx_learning/apps/authoring/components/api.py index aad28cf76..f750e59c5 100644 --- a/openedx_learning/apps/authoring/components/api.py +++ b/openedx_learning/apps/authoring/components/api.py @@ -26,7 +26,7 @@ from ..contents import api as contents_api from ..publishing import api as publishing_api from .models import Component, ComponentType, ComponentVersion, ComponentVersionContent -from ..collections.models import Collection +from ..collections.models import Collection, CollectionPublishableEntity # The public API that will be re-exported by openedx_learning.apps.authoring.api # is listed in the __all__ entries below. Internal helper functions that are @@ -50,8 +50,7 @@ "look_up_component_version_content", "AssetError", "get_redirect_response_for_component_asset", - "add_collections", - "remove_collections", + "set_collections", ] @@ -609,20 +608,18 @@ def _error_header(error: AssetError) -> dict[str, str]: return HttpResponse(headers={**info_headers, **redirect_headers}) -def add_collections( +def set_collections( learning_package_id: int, component: Component, collection_qset: QuerySet[Collection], created_by: int | None = None, ) -> Component: """ - Adds a QuerySet of Collection to Component. + Set collections for a given component. These Collections must belong to the same LearningPackage as the Component, or a ValidationError will be raised. - Collections already in the Component.PublishableEntities are silently ignored. - - The all collection object's modified date is updated. + The collection_qset object's modified date is updated. Returns the updated component object. """ @@ -634,28 +631,18 @@ def add_collections( f"{invalid_collection.learning_package_id} to component {component} in " f"learning package {learning_package_id}." ) + # Clear other collections for given component and add/keep only the ones in collection_qset + relations = CollectionPublishableEntity.objects.filter( + entity=component.publishable_entity + ).exclude(collection__in=collection_qset) + removed_collections = [relation.collection for relation in relations] + relations.delete() component.publishable_entity.collections.add( *collection_qset.all(), through_defaults={"created_by_id": created_by}, ) - collection_qset.update(modified=datetime.now(tz=timezone.utc)) - - return component - - -def remove_collections( - learning_package_id: int, - component: Component, - collection_qset: QuerySet[Collection], -) -> Component: - """ - Removes a QuerySet of Collections from a Component. - - The Collections modified date is updated. - - Returns the updated component. - """ - component.publishable_entity.collections.remove(*collection_qset.all()) - collection_qset.update(modified=datetime.now(tz=timezone.utc)) + for collection in list(collection_qset.all()) + removed_collections: + collection.modified = datetime.now(tz=timezone.utc) + collection.save() return component From b041649ece84235b10717eb85be84b25fcbd4dc5 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 4 Oct 2024 16:28:27 +0530 Subject: [PATCH 3/6] refactor: improve performance --- .../apps/authoring/components/api.py | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/openedx_learning/apps/authoring/components/api.py b/openedx_learning/apps/authoring/components/api.py index f750e59c5..82961fc65 100644 --- a/openedx_learning/apps/authoring/components/api.py +++ b/openedx_learning/apps/authoring/components/api.py @@ -613,15 +613,15 @@ def set_collections( component: Component, collection_qset: QuerySet[Collection], created_by: int | None = None, -) -> Component: +) -> set[Collection]: """ Set collections for a given component. These Collections must belong to the same LearningPackage as the Component, or a ValidationError will be raised. - The collection_qset object's modified date is updated. + Modified date of all collections related to component is updated. - Returns the updated component object. + Returns the updated collections. """ # Disallow adding entities outside the collection's learning package invalid_collection = collection_qset.exclude(learning_package_id=learning_package_id).first() @@ -635,14 +635,18 @@ def set_collections( relations = CollectionPublishableEntity.objects.filter( entity=component.publishable_entity ).exclude(collection__in=collection_qset) - removed_collections = [relation.collection for relation in relations] + removed_collections = set(relation.collection for relation in relations) relations.delete() component.publishable_entity.collections.add( *collection_qset.all(), through_defaults={"created_by_id": created_by}, ) - for collection in list(collection_qset.all()) + removed_collections: - collection.modified = datetime.now(tz=timezone.utc) - collection.save() - - return component + # Update modified date via update to avoid triggering post_save signal for collections + # The signal triggers index update for each collection synchronously which will be very slow in this case. + # Instead trigger the index update in the caller function asynchronously. + affected_collection = removed_collections | set(collection_qset.all()) + Collection.objects.filter( + id__in=[collection.id for collection in affected_collection] + ).update(modified=datetime.now(tz=timezone.utc)) + + return affected_collection From 33e267c7f3cd7c04b64c86ddd86fda29a4ab098d Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 4 Oct 2024 20:04:18 +0530 Subject: [PATCH 4/6] test: set collections api --- .../apps/authoring/components/test_api.py | 134 +++++++++++++++++- 1 file changed, 133 insertions(+), 1 deletion(-) diff --git a/tests/openedx_learning/apps/authoring/components/test_api.py b/tests/openedx_learning/apps/authoring/components/test_api.py index 1fd8f4941..4d61f3138 100644 --- a/tests/openedx_learning/apps/authoring/components/test_api.py +++ b/tests/openedx_learning/apps/authoring/components/test_api.py @@ -3,8 +3,12 @@ """ from datetime import datetime, timezone -from django.core.exceptions import ObjectDoesNotExist +from django.contrib.auth import get_user_model +from django.core.exceptions import ObjectDoesNotExist, ValidationError +from freezegun import freeze_time +from openedx_learning.apps.authoring.collections import api as collection_api +from openedx_learning.apps.authoring.collections.models import Collection, CollectionPublishableEntity from openedx_learning.apps.authoring.components import api as components_api from openedx_learning.apps.authoring.components.models import Component, ComponentType from openedx_learning.apps.authoring.contents import api as contents_api @@ -14,6 +18,9 @@ from openedx_learning.lib.test_utils import TestCase +User = get_user_model() + + class ComponentTestCase(TestCase): """ Base-class for setting up commonly used test data. @@ -503,3 +510,128 @@ def test_multiple_versions(self): version_3.contents .get(componentversioncontent__key="hello.txt") ) + + +class SetCollectionsTestCase(ComponentTestCase): + """ + Test setting collections for a component. + """ + collection1: Collection + collection2: Collection + collection3: Collection + published_problem: Component + user: User # type: ignore [valid-type] + + @classmethod + def setUpTestData(cls) -> None: + """ + Initialize some collections + """ + super().setUpTestData() + v2_problem_type = components_api.get_or_create_component_type("xblock.v2", "problem") + cls.published_problem, _ = components_api.create_component_and_version( + cls.learning_package.id, + component_type=v2_problem_type, + local_key="pp_lk", + title="Published Problem", + created=cls.now, + created_by=None, + ) + cls.collection1 = collection_api.create_collection( + cls.learning_package.id, + key="MYCOL1", + title="Collection1", + created_by=None, + description="Description of Collection 1", + ) + cls.collection2 = collection_api.create_collection( + cls.learning_package.id, + key="MYCOL2", + title="Collection2", + created_by=None, + description="Description of Collection 2", + ) + cls.collection3 = collection_api.create_collection( + cls.learning_package.id, + key="MYCOL3", + title="Collection3", + created_by=None, + description="Description of Collection 3", + ) + cls.user = User.objects.create( + username="user", + email="user@example.com", + ) + + def test_set_collections(self): + """ + Test setting collections in a component + """ + modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) + with freeze_time(modified_time): + components_api.set_collections( + self.learning_package.id, + self.published_problem, + collection_qset=Collection.objects.filter(id__in=[ + self.collection1.pk, + self.collection2.pk, + ]), + created_by=self.user.id, + ) + assert list(self.collection1.entities.all()) == [ + self.published_problem.publishable_entity, + ] + assert list(self.collection2.entities.all()) == [ + self.published_problem.publishable_entity, + ] + for collection_entity in CollectionPublishableEntity.objects.filter( + entity=self.published_problem.publishable_entity + ): + assert collection_entity.created_by == self.user + assert Collection.objects.get(id=self.collection1.pk).modified == modified_time + assert Collection.objects.get(id=self.collection2.pk).modified == modified_time + + # Set collections again, but this time remove collection1 and add collection3 + # Expected result: collection2 & collection3 associated to component and collection1 is excluded. + new_modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) + with freeze_time(new_modified_time): + components_api.set_collections( + self.learning_package.id, + self.published_problem, + collection_qset=Collection.objects.filter(id__in=[ + self.collection3.pk, + self.collection2.pk, + ]), + created_by=self.user.id, + ) + assert not list(self.collection1.entities.all()) + assert list(self.collection2.entities.all()) == [ + self.published_problem.publishable_entity, + ] + assert list(self.collection3.entities.all()) == [ + self.published_problem.publishable_entity, + ] + # update modified time of all three collections as they were all updated + assert Collection.objects.get(id=self.collection1.pk).modified == new_modified_time + assert Collection.objects.get(id=self.collection2.pk).modified == new_modified_time + assert Collection.objects.get(id=self.collection3.pk).modified == new_modified_time + + def test_set_collection_wrong_learning_package(self): + """ + We cannot set collections with a different learning package than the component. + """ + learning_package_2 = publishing_api.create_learning_package( + key="ComponentTestCase-test-key-2", + title="Components Test Case Learning Package-2", + ) + with self.assertRaises(ValidationError): + components_api.set_collections( + learning_package_2.id, + self.published_problem, + collection_qset=Collection.objects.filter(id__in=[ + self.collection1.pk, + ]), + created_by=self.user.id, + ) + + assert not list(self.collection1.entities.all()) From 0287f2e0723a3a563d71f03a10e9547ddcbf0080 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 4 Oct 2024 20:22:17 +0530 Subject: [PATCH 5/6] fix: lint issues --- openedx_learning/apps/authoring/components/api.py | 2 +- tests/openedx_learning/apps/authoring/components/test_api.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/openedx_learning/apps/authoring/components/api.py b/openedx_learning/apps/authoring/components/api.py index 82961fc65..44a96c9ea 100644 --- a/openedx_learning/apps/authoring/components/api.py +++ b/openedx_learning/apps/authoring/components/api.py @@ -23,10 +23,10 @@ from django.db.transaction import atomic from django.http.response import HttpResponse, HttpResponseNotFound +from ..collections.models import Collection, CollectionPublishableEntity from ..contents import api as contents_api from ..publishing import api as publishing_api from .models import Component, ComponentType, ComponentVersion, ComponentVersionContent -from ..collections.models import Collection, CollectionPublishableEntity # The public API that will be re-exported by openedx_learning.apps.authoring.api # is listed in the __all__ entries below. Internal helper functions that are diff --git a/tests/openedx_learning/apps/authoring/components/test_api.py b/tests/openedx_learning/apps/authoring/components/test_api.py index 4d61f3138..9cb761daf 100644 --- a/tests/openedx_learning/apps/authoring/components/test_api.py +++ b/tests/openedx_learning/apps/authoring/components/test_api.py @@ -17,7 +17,6 @@ from openedx_learning.apps.authoring.publishing.models import LearningPackage from openedx_learning.lib.test_utils import TestCase - User = get_user_model() From 83aec7f07203d52818e1eed0c513dc244129580c Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 10 Oct 2024 15:04:15 +0530 Subject: [PATCH 6/6] refactor: improve set collections performance --- .../apps/authoring/components/api.py | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/openedx_learning/apps/authoring/components/api.py b/openedx_learning/apps/authoring/components/api.py index 44a96c9ea..22750d9dd 100644 --- a/openedx_learning/apps/authoring/components/api.py +++ b/openedx_learning/apps/authoring/components/api.py @@ -631,20 +631,27 @@ def set_collections( f"{invalid_collection.learning_package_id} to component {component} in " f"learning package {learning_package_id}." ) - # Clear other collections for given component and add/keep only the ones in collection_qset - relations = CollectionPublishableEntity.objects.filter( + current_relations = CollectionPublishableEntity.objects.filter( entity=component.publishable_entity - ).exclude(collection__in=collection_qset) - removed_collections = set(relation.collection for relation in relations) - relations.delete() + ).select_related('collection') + # Clear other collections for given component and add only new collections from collection_qset + removed_collections = set( + r.collection for r in current_relations.exclude(collection__in=collection_qset) + ) + new_collections = set(collection_qset.exclude( + id__in=current_relations.values_list('collection', flat=True) + )) + # Use `remove` instead of `CollectionPublishableEntity.delete()` to trigger m2m_changed signal which will handle + # updating component index. + component.publishable_entity.collections.remove(*removed_collections) component.publishable_entity.collections.add( - *collection_qset.all(), + *new_collections, through_defaults={"created_by_id": created_by}, ) # Update modified date via update to avoid triggering post_save signal for collections # The signal triggers index update for each collection synchronously which will be very slow in this case. # Instead trigger the index update in the caller function asynchronously. - affected_collection = removed_collections | set(collection_qset.all()) + affected_collection = removed_collections | new_collections Collection.objects.filter( id__in=[collection.id for collection in affected_collection] ).update(modified=datetime.now(tz=timezone.utc))