-
Notifications
You must be signed in to change notification settings - Fork 10
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
Conversation
Thanks for the pull request, @pomegranited! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
341efc4
to
56c0a99
Compare
db902ab
to
dfbb40f
Compare
to associate PublishableEntities with Collections. Collection.modified timestamp is updated whenever its contents are changed.
35629c3
to
7ec9ba5
Compare
@ormsbee Can you take a quick look at this when you get some time? In particular, if possible, I want to make sure that this is sufficiently general that we can link units/sections/subsections to collections with the same API, even if they're not implemented as XBlocks/Components. Is |
@bradenmacdonald: I'll do a full review now this afternoon, but yes, |
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.
Some questions and requests. Most of these are minor changes to match openedx_learning
package coding conventions that I have failed to document anywhere, and there is no reasonable way for you to have been aware of them. 😛 Sorry about that, and thank you for your patience.
added = add_to_collections( | ||
Collection.objects.filter(id=collection.id), | ||
contents_qset, | ||
) | ||
if added: | ||
collection.refresh_from_db() # fetch updated modified date |
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.
[nitpick, optional]: It's a little weird to me that this is going to cause multiple writes to the same Collection with just slightly differing datetimes, but in the same transaction. I can't quite place my finger on why this bothers me, except some vague sense that something tracking the Collection lifecycle might get confused.
Instead of invoking add_to_collections
and doing the refresh to get around the timestamp change, can we instead copy some of logic from there that does the adding of the records?
So something like:
records_to_add = [
CollectionPublishableEntity(
collection_id=collection.id,
entity_id=entity_id,
)
for entity_id in entities_qset.values_list("pk", flat=True)
]
CollectionPublishableEntity.bulk_add(records_to_add)
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.
@ormsbee I don't have a use case for creating a collection with components in it, I was just copying that code from your PR. Maybe it's better just to keep them as separate API calls, to avoid this duplication, and the need for a transaction?
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'm fine either way. I think that use case will come up when we do v1 library import functionality, but I'm fine with punting until then.
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 ended up removing the entities_qset
param from create_collection
, we can add it back later if needed.
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.
Hi @pomegranited!
Thank you for your work here! I just left some comments.
If you're trying to do either of these things, you probably want a new model or | ||
app. For more details, read on. |
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.
This adds a lot of context!! Thank you! ❤️
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.
Oh definitely.. @ormsbee 's notes are always worth preserving :)
Addresses PR review. * CollectionObject -> CollectionPublishableEntity, with added created_by and created fields * contents -> entities, contents_qset -> entities_qset * get_object_collections -> get_entity_collections, with added learning_package_id argument
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.
We ended up not needing a "get all collections" function; we always fetch them for a given learning package. So we merged them into a single api.get_collections method that takes a `learning_package_id` + optional `enabled` (defaults to True).
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.
Thank you for the changes, @pomegranited!
LGTM 👍
API: * create_collection -- removed entities_qset param, added enabled param * add/remove entities from a single collection, not a qset * raise ValidationError if adding entities to a collection with mismatching learning packages * add/remove returns updated collection object (not count) Tests: * use authoring API * simplified data used by test classes
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.
Optional nitpick and some speculation, but nothing that would block merging. Thank you!
(Also, this is a three-day weekend in the U.S., so I may be a bit slow on reviews.)
# Disallow adding entities outside the collection's learning package | ||
for entity in entities_qset.all(): | ||
if entity.learning_package_id != learning_package_id: |
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.
[Nit (optional)]: You can also push this to the database layer with something like:
invalid_entity = entities_qset.filter(~Q(learning_package_id=learning_package_id)).first()
if invalid_entity:
# ... validation error with specifics
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.
That's much cleaner, thank you, done with e03daaa
return 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 comment
The 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 entity
, they could do something like entity.collections.active()
.
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.
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 :)
being added to a collection
@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
As get_all_collections api was refactored [1] to only support fetching collections by learning_package, update reindex function to get collections by learning_package. 1. openedx/openedx-learning#216
Description
Adds models and APIs for linking objects (PublishableEntities) to Collections, and for fetching the Collections associated with a given entity.
Supporting information
Part of : openedx/modular-learning#227
Based on : #131
Private-ref: FAL-3784
Testing instructions
See open-craft/edx-platform#674