From 2a5bfac7091cc01b36f0ae6b5e740f0ae259f065 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 3 Sep 2024 17:11:01 +0930 Subject: [PATCH] refactor: updates collections api to use learning_package_id + key to identify Collections. We do this because the `key` will be used in the Collection's opaque key (not the ID). --- .../apps/authoring/collections/api.py | 25 +++++----- .../apps/authoring/collections/test_api.py | 50 +++++++++++++------ 2 files changed, 49 insertions(+), 26 deletions(-) diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py index 88a6da8c..0b9d17da 100644 --- a/openedx_learning/apps/authoring/collections/api.py +++ b/openedx_learning/apps/authoring/collections/api.py @@ -50,22 +50,23 @@ def create_collection( return collection -def get_collection(collection_id: int) -> Collection: +def get_collection(learning_package_id: int, collection_key: str) -> Collection: """ Get a Collection by ID """ - return Collection.objects.get(id=collection_id) + return Collection.objects.get_by_key(learning_package_id, collection_key) def update_collection( - collection_id: int, + learning_package_id: int, + key: str, title: str | None = None, description: str | None = None, ) -> Collection: """ - Update a Collection + Update a Collection identified by the learning_package_id + key. """ - collection = Collection.objects.get(id=collection_id) + collection = get_collection(learning_package_id, key) # If no changes were requested, there's nothing to update, so just return # the Collection as-is @@ -82,7 +83,8 @@ def update_collection( def add_to_collection( - collection_id: int, + learning_package_id: int, + key: str, entities_qset: QuerySet[PublishableEntity], created_by: int | None = None, ) -> Collection: @@ -97,17 +99,15 @@ def add_to_collection( Returns the updated Collection object. """ - collection = get_collection(collection_id) - learning_package_id = collection.learning_package_id - # Disallow adding entities outside the collection's learning package invalid_entity = entities_qset.exclude(learning_package_id=learning_package_id).first() if invalid_entity: raise ValidationError( f"Cannot add entity {invalid_entity.pk} in learning package {invalid_entity.learning_package_id} " - f"to collection {collection_id} in learning package {learning_package_id}." + f"to collection {key} in learning package {learning_package_id}." ) + collection = get_collection(learning_package_id, key) collection.entities.add( *entities_qset.all(), through_defaults={"created_by_id": created_by}, @@ -119,7 +119,8 @@ def add_to_collection( def remove_from_collection( - collection_id: int, + learning_package_id: int, + key: str, entities_qset: QuerySet[PublishableEntity], ) -> Collection: """ @@ -131,7 +132,7 @@ def remove_from_collection( Returns the updated Collection. """ - collection = get_collection(collection_id) + collection = get_collection(learning_package_id, key) collection.entities.remove(*entities_qset.all()) collection.modified = datetime.now(tz=timezone.utc) diff --git a/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py index 78930daa..f0e53477 100644 --- a/tests/openedx_learning/apps/authoring/collections/test_api.py +++ b/tests/openedx_learning/apps/authoring/collections/test_api.py @@ -96,7 +96,7 @@ def test_get_collection(self): """ Test getting a single collection. """ - collection = api.get_collection(self.collection1.pk) + collection = api.get_collection(self.learning_package.pk, 'COL1') assert collection == self.collection1 def test_get_collection_not_found(self): @@ -104,7 +104,14 @@ def test_get_collection_not_found(self): Test getting a collection that doesn't exist. """ with self.assertRaises(ObjectDoesNotExist): - api.get_collection(12345) + api.get_collection(self.learning_package.pk, '12345') + + def test_get_collection_wrong_learning_package(self): + """ + Test getting a collection that doesn't exist in the requested learning package. + """ + with self.assertRaises(ObjectDoesNotExist): + api.get_collection(self.learning_package.pk, self.collection3.key) def test_get_collections(self): """ @@ -258,21 +265,24 @@ def setUpTestData(cls) -> None: # Add some shared entities to the collections cls.collection1 = api.add_to_collection( - cls.collection1.id, + cls.learning_package.id, + key=cls.collection1.key, entities_qset=PublishableEntity.objects.filter(id__in=[ cls.published_entity.id, ]), created_by=cls.user.id, ) cls.collection2 = api.add_to_collection( - cls.collection2.id, + cls.learning_package.id, + key=cls.collection2.key, entities_qset=PublishableEntity.objects.filter(id__in=[ cls.published_entity.id, cls.draft_entity.id, ]), ) cls.disabled_collection = api.add_to_collection( - cls.disabled_collection.id, + cls.learning_package.id, + key=cls.disabled_collection.key, entities_qset=PublishableEntity.objects.filter(id__in=[ cls.published_entity.id, ]), @@ -298,7 +308,8 @@ def test_add_to_collection(self): modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): self.collection1 = api.add_to_collection( - self.collection1.id, + self.learning_package.id, + self.collection1.key, PublishableEntity.objects.filter(id__in=[ self.draft_entity.id, ]), @@ -320,7 +331,8 @@ def test_add_to_collection_again(self): modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): self.collection2 = api.add_to_collection( - self.collection2.id, + self.learning_package.id, + self.collection2.key, PublishableEntity.objects.filter(id__in=[ self.published_entity.id, ]), @@ -338,7 +350,8 @@ def test_add_to_collection_wrong_learning_package(self): """ with self.assertRaises(ValidationError): api.add_to_collection( - self.collection3.id, + self.learning_package_2.id, + self.collection3.key, PublishableEntity.objects.filter(id__in=[ self.published_entity.id, ]), @@ -353,7 +366,8 @@ def test_remove_from_collection(self): modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): self.collection2 = api.remove_from_collection( - self.collection2.id, + self.learning_package.id, + self.collection2.key, PublishableEntity.objects.filter(id__in=[ self.published_entity.id, ]), @@ -405,7 +419,8 @@ def test_update_collection(self): modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): collection = api.update_collection( - self.collection.pk, + self.learning_package.id, + key=self.collection.key, title="New Title", description="", ) @@ -420,7 +435,8 @@ def test_update_collection_partial(self): Test updating a collection's title. """ collection = api.update_collection( - self.collection.pk, + self.learning_package.id, + key=self.collection.key, title="New Title", ) @@ -429,7 +445,8 @@ def test_update_collection_partial(self): assert f"{collection}" == f" ({self.collection.key}:New Title)" collection = api.update_collection( - self.collection.pk, + self.learning_package.id, + key=self.collection.key, description="New description", ) @@ -443,7 +460,8 @@ def test_update_collection_empty(self): modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): collection = api.update_collection( - self.collection.pk, + self.learning_package.id, + key=self.collection.key, ) assert collection.title == self.collection.title # unchanged @@ -455,4 +473,8 @@ def test_update_collection_not_found(self): Test updating a collection that doesn't exist. """ with self.assertRaises(ObjectDoesNotExist): - api.update_collection(12345, title="New Title") + api.update_collection( + self.learning_package.id, + key="12345", + title="New Title", + )