Skip to content

Commit 50c6a27

Browse files
fix: apply suggestions from code review
Co-authored-by: Jillian <jill@opencraft.com>
1 parent 66d0d17 commit 50c6a27

1 file changed

Lines changed: 9 additions & 14 deletions

File tree

  • openedx_learning/apps/authoring/collections

openedx_learning/apps/authoring/collections/api.py

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,6 @@ def get_collections(learning_package_id: int, enabled: bool | None = True) -> Qu
208208

209209

210210
def set_collections(
211-
learning_package_id: int,
212211
publishable_entity: PublishableEntity,
213212
collection_qset: QuerySet[Collection],
214213
created_by: int | None = None,
@@ -224,12 +223,9 @@ def set_collections(
224223
Returns the updated collections.
225224
"""
226225
# Disallow adding entities outside the collection's learning package
227-
invalid_collection = collection_qset.exclude(learning_package_id=learning_package_id).first()
228-
if invalid_collection:
226+
if collection_qset.exclude(learning_package_id=publishable_entity.learning_package_id).count():
229227
raise ValidationError(
230-
f"Cannot add collection {invalid_collection.pk} in learning package "
231-
f"{invalid_collection.learning_package_id} to entity {publishable_entity} in "
232-
f"learning package {learning_package_id}."
228+
"Collection entities must be from the same learning package as the collection.",
233229
)
234230
current_relations = CollectionPublishableEntity.objects.filter(
235231
entity=publishable_entity
@@ -238,19 +234,18 @@ def set_collections(
238234
removed_collections = set(
239235
r.collection for r in current_relations.exclude(collection__in=collection_qset)
240236
)
237+
removed_collections = set(
238+
r.collection for r in current_relations.exclude(collection__in=collection_qset)
239+
)
241240
new_collections = set(collection_qset.exclude(
242241
id__in=current_relations.values_list('collection', flat=True)
243242
))
244-
# Use `remove` instead of `CollectionPublishableEntity.delete()` to trigger m2m_changed signal which will handle
245-
# updating entity index.
246-
publishable_entity.collections.remove(*removed_collections)
247-
publishable_entity.collections.add(
248-
*new_collections,
243+
# Triggers a m2m_changed signal
244+
publishable_entity.collections.set(
245+
objs=new_collections,
249246
through_defaults={"created_by_id": created_by},
250247
)
251-
# Update modified date via update to avoid triggering post_save signal for collections
252-
# The signal triggers index update for each collection synchronously which will be very slow in this case.
253-
# Instead trigger the index update in the caller function asynchronously.
248+
# Update modified date via update to avoid triggering post_save signal for all collections, which can be very slow.
254249
affected_collection = removed_collections | new_collections
255250
Collection.objects.filter(
256251
id__in=[collection.id for collection in affected_collection]

0 commit comments

Comments
 (0)