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).

openedx_learning/apps/authoring/collections/api.py

@@ -30,8 +30,9 @@
 
 def create_collection(
  learning_package_id: int,
- title: str,
  key: str,
+ *,
+ title: str,
  created_by: int | None,
  description: str = "",
  enabled: bool = True,
@@ -41,31 +42,33 @@ def create_collection(
  """
  collection = Collection.objects.create(
  learning_package_id=learning_package_id,
- title=title,
  key=key,
+ title=title,
  created_by_id=created_by,
  description=description,
  enabled=enabled,
  )
  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 +85,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 +101,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 +121,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 +134,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)

tests/openedx_learning/apps/authoring/collections/test_api.py

@@ -96,15 +96,22 @@ 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):
  """
  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"<Collection> ({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",
+ )