Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Associate PublishableEntities with Collections [FC-0062] #216
Changes from 5 commits
534e8d9
1fda3e5
a2cd234
7ec9ba5
0e15dd0
00111bc
b723d79
2fe2e70
e03daaa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
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 fromcreate_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.
[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.
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.
Please change this to be
entities_qset
for better consistency (contents
usually means theContent
model, and may cause confusion).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.
Is this a useful number to return? If it includes the conflicts, doesn't it always return
len(collections_qset) * len(contents_qset)
?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 true.. What should this method return instead?
I had thought that I should filter out any entities that aren't in the same learning package as the collection, so that would change this count. But since there's potentially more than one collection/learning package, I did that filtering in the REST API.
Do you think it makes sense to enforce the "a collection may only contain entities in their learning package" here instead? It'll mean fetching more data in
add_to_collections
-- the Collections + PublishableEntities -- but if it's worth if if we should enforce this rule.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, that's a fantastic callout. We should absolutely enforce it at the Learning Core layer, so people don't shoot themselves in the foot. It's very possible we'll eventually allow folks to add stuff from other LearningPackages into Collections, but we're going to have to be very deliberate when we do and think through the lifecycle issues when those LearningPackages disappear. We definitely don't want to let it happen by accident because other apps used Learning Core directly instead of going through the Libraries REST API.
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 :)