-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Associate PublishableEntities with Collections [FC-0062] #216
Changes from 2 commits
534e8d9
1fda3e5
a2cd234
7ec9ba5
0e15dd0
00111bc
b723d79
2fe2e70
e03daaa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,8 +7,9 @@ | |
from django.db.transaction import atomic | ||
from django.utils import timezone | ||
|
||
from ..publishing import api as publishing_api | ||
from ..publishing.models import PublishableEntity | ||
from .models import Collection, CollectionObject | ||
from .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 | ||
|
@@ -20,8 +21,7 @@ | |
"create_collection", | ||
"get_collection", | ||
"get_collections", | ||
"get_learning_package_collections", | ||
"get_object_collections", | ||
"get_entity_collections", | ||
"remove_from_collections", | ||
"update_collection", | ||
] | ||
|
@@ -32,7 +32,7 @@ def create_collection( | |
title: str, | ||
created_by: int | None, | ||
description: str = "", | ||
contents_qset: QuerySet[PublishableEntity] = PublishableEntity.objects.none(), # default to empty set, | ||
entities_qset: QuerySet[PublishableEntity] = PublishableEntity.objects.none(), # default to empty set, | ||
) -> Collection: | ||
""" | ||
Create a new Collection | ||
|
@@ -48,7 +48,8 @@ def create_collection( | |
|
||
added = add_to_collections( | ||
Collection.objects.filter(id=collection.id), | ||
contents_qset, | ||
entities_qset, | ||
created_by, | ||
) | ||
if added: | ||
collection.refresh_from_db() # fetch updated modified date | ||
|
@@ -89,33 +90,35 @@ def update_collection( | |
|
||
def add_to_collections( | ||
collections_qset: QuerySet[Collection], | ||
contents_qset: QuerySet[PublishableEntity], | ||
entities_qset: QuerySet[PublishableEntity], | ||
created_by: int | None = None, | ||
) -> int: | ||
""" | ||
Adds a QuerySet of PublishableEntities to a QuerySet of Collections. | ||
|
||
Records are created in bulk, and so integrity errors are deliberately ignored: they indicate that the content(s) | ||
Records are created in bulk, and so integrity errors are deliberately ignored: they indicate that the entity(s) | ||
have already been added to the collection(s). | ||
|
||
Returns the number of entities added (including any that already exist). | ||
""" | ||
collection_objects = [] | ||
object_ids = contents_qset.values_list("pk", flat=True) | ||
collection_entities = [] | ||
entity_ids = entities_qset.values_list("pk", flat=True) | ||
collection_ids = collections_qset.values_list("pk", flat=True) | ||
|
||
for collection_id in collection_ids: | ||
for object_id in object_ids: | ||
collection_objects.append( | ||
CollectionObject( | ||
for entity_id in entity_ids: | ||
collection_entities.append( | ||
CollectionPublishableEntity( | ||
collection_id=collection_id, | ||
object_id=object_id, | ||
entity_id=entity_id, | ||
created_by_id=created_by, | ||
) | ||
) | ||
|
||
created = [] | ||
if collection_objects: | ||
created = CollectionObject.objects.bulk_create( | ||
collection_objects, | ||
if collection_entities: | ||
created = CollectionPublishableEntity.objects.bulk_create( | ||
collection_entities, | ||
ignore_conflicts=True, | ||
) | ||
|
||
|
@@ -127,27 +130,27 @@ def add_to_collections( | |
|
||
|
||
def remove_from_collections( | ||
collections_qset: QuerySet, | ||
contents_qset: QuerySet, | ||
collections_qset: QuerySet[Collection], | ||
entities_qset: QuerySet[PublishableEntity], | ||
pomegranited marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) -> int: | ||
""" | ||
Removes a QuerySet of PublishableEntities from a QuerySet of Collections. | ||
|
||
PublishableEntities are deleted from each Collection, in bulk. | ||
|
||
Collections which had objects removed are marked with modified=now(). | ||
Collections which had entities removed are marked with modified=now(). | ||
|
||
Returns the total number of entities deleted. | ||
""" | ||
total_deleted = 0 | ||
object_ids = contents_qset.values_list("pk", flat=True) | ||
entity_ids = entities_qset.values_list("pk", flat=True) | ||
collection_ids = collections_qset.values_list("pk", flat=True) | ||
modified_collection_ids = [] | ||
|
||
for collection_id in collection_ids: | ||
num_deleted, _ = CollectionObject.objects.filter( | ||
num_deleted, _ = CollectionPublishableEntity.objects.filter( | ||
collection_id=collection_id, | ||
object_id__in=object_ids, | ||
entity__in=entity_ids, | ||
).delete() | ||
|
||
if num_deleted: | ||
|
@@ -161,33 +164,26 @@ def remove_from_collections( | |
return total_deleted | ||
|
||
|
||
def get_object_collections(object_key: str) -> QuerySet[Collection]: | ||
def get_entity_collections(learning_package_id: int, entity_key: str) -> QuerySet[Collection]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [speculation (please do not block PR for this)]: It might be nice if we could a custom Manager on the through-model so that if someone has an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh absolutely -- I am interested to see the other use cases for Collections besides what we're supporting here. But will leave that for a day when it's needed, thank you :) |
||
""" | ||
Get all collections associated with a given PublishableEntity.key. | ||
Get all collections in the given learning package which contain this entity. | ||
|
||
Only enabled collections are returned. | ||
""" | ||
entity = PublishableEntity.objects.get(key=object_key) | ||
entity = publishing_api.get_publishable_entity_by_key( | ||
learning_package_id, | ||
key=entity_key, | ||
) | ||
return entity.collections.filter(enabled=True).order_by("pk") | ||
|
||
|
||
def get_learning_package_collections(learning_package_id: int) -> QuerySet[Collection]: | ||
def get_collections(learning_package_id: int, enabled: bool | None = True) -> QuerySet[Collection]: | ||
""" | ||
Get all collections for a given learning package | ||
|
||
Only enabled collections are returned | ||
pomegranited marked this conversation as resolved.
Show resolved
Hide resolved
|
||
""" | ||
return Collection.objects \ | ||
.filter(learning_package_id=learning_package_id, enabled=True) \ | ||
.select_related("learning_package") \ | ||
.order_by('pk') | ||
|
||
|
||
def get_collections(enabled: bool | None = None) -> QuerySet[Collection]: | ||
""" | ||
Get all collections, optionally caller can filter by enabled flag | ||
""" | ||
qs = Collection.objects.all() | ||
qs = Collection.objects.filter(learning_package_id=learning_package_id) | ||
if enabled is not None: | ||
qs = qs.filter(enabled=enabled) | ||
return qs.select_related("learning_package").order_by('pk') |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,32 +1,38 @@ | ||
# Generated by Django 4.2.14 on 2024-08-21 07:15 | ||
# Generated by Django 4.2.15 on 2024-08-29 00:05 | ||
|
||
import django.db.models.deletion | ||
from django.conf import settings | ||
from django.db import migrations, models | ||
|
||
import openedx_learning.lib.validators | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
migrations.swappable_dependency(settings.AUTH_USER_MODEL), | ||
('oel_publishing', '0002_alter_learningpackage_key_and_more'), | ||
('oel_collections', '0002_remove_collection_name_collection_created_by_and_more'), | ||
] | ||
|
||
operations = [ | ||
migrations.CreateModel( | ||
name='CollectionObject', | ||
name='CollectionPublishableEntity', | ||
fields=[ | ||
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
('created', models.DateTimeField(auto_now_add=True, validators=[openedx_learning.lib.validators.validate_utc_datetime])), | ||
('collection', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_collections.collection')), | ||
('object', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.publishableentity')), | ||
('created_by', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL)), | ||
('entity', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.publishableentity')), | ||
], | ||
), | ||
migrations.AddField( | ||
model_name='collection', | ||
name='contents', | ||
field=models.ManyToManyField(related_name='collections', through='oel_collections.CollectionObject', to='oel_publishing.publishableentity'), | ||
name='entities', | ||
field=models.ManyToManyField(related_name='collections', through='oel_collections.CollectionPublishableEntity', to='oel_publishing.publishableentity'), | ||
), | ||
migrations.AddConstraint( | ||
model_name='collectionobject', | ||
constraint=models.UniqueConstraint(fields=('collection', 'object'), name='oel_collections_cpe_uniq_col_obj'), | ||
model_name='collectionpublishableentity', | ||
constraint=models.UniqueConstraint(fields=('collection', 'entity'), name='oel_collections_cpe_uniq_col_ent'), | ||
), | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] Is it a common use case that we'll want to add to multiple Collections simultaneously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't think so, but that's what was in the ticket: openedx/modular-learning#227
@bradenmacdonald can you comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was just trying to indicate that the API needs to support having an item linked to multiple collections, but it doesn't necessarily have to support linking multiple collections to an item in a single API call. If it simplifies the implementation to only operate by adding/removing a single collection at a time, that's fine with me.
There is only one use case I can think of where we might want to associate/dissociate from multiple collections simultaneously and that's import/export. But we haven't planned that part out yet at all.