-
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
Add Collections of PublishableEntities #131
Conversation
|
||
This API sacrifices some power in order to try to keep things simpler. For | ||
instance, ``add_to_collection`` and ``remove_from_collection`` each create a | ||
new CollectionChangeSet when they're invoked. The data model supports having |
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.
Looks like you were in the middle of a sentences here.
.order_by('-version_num') \ | ||
.first() | ||
if last_change_set: | ||
next_version_num = last_change_set.version_num + 1 |
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 there a race condition that can occur here if multiple calls to add to collection happen at the same time?
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.
Yes, but CollectionChangeSet
has a unique constraint on (collection_id, version_num)
, so in the event of a race condition, only one of those new version writes will succeed, and the other will fail and need to be retried.
models.UniqueConstraint( | ||
fields=[ | ||
"learning_package", | ||
"key", |
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 key
the version number here?
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.
key
is a string that identifies the Collection, so if we wanted to, we could make this a v1 LibraryKey.
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.
Gotcha, I was confused by the comment above that mentioned version_num
Here's my take on this:
So: Alice Author makes an update to the library, such as adding a new block with a tag "hyperspace". Learning Core sends out an async update to any LCBs subscribed to that library's topic (or perhaps even just subscribed to that tag, for higher efficiency). Each LCB checks its "Collection" of components to see if there is a change (new block, deleted block, edited block). If there is at least one change, the changes are consolidated into a list of changes, and a notification is queued for the user. If a notification was previously sent, the notification is updated. When Alice clicks on the notification "Your course LCB has updates..." she'll see a list of the changes, kind of like this that we're building for tagging: But it would say something like:
This scenario does not require any new models on the Learning Core side, but does require pub-sub (which we have available already via the message bus, right?). The only model required is for each LCB to have some table wherein it can track the set of matching components and the version of each. And even though I talk about push notifications, it could be built in a similar way with a simple page that lists all the changes, until a notification framework is available. I believe that a notification framework is being / was already (?) implemented in the LMS and can be extended to Studio too, but that isn't necessary to implement this approach - just to make the UX better. Thoughts? EDIT: This was discussed further on Slack and it turns out that what I said is only relevant under some assumptions which may or may not be true... |
@kdmccormick and @bradenmacdonald have convinced me that Collections are not currently necessary. Particularly, @kdmccormick points out that we can push more of the logic of deciding what versions to use into the
It also occurred to me that pushing more of the logic into the I still think that the idea of Collections will have some usefulness when we get to doing things like course-runs. That being said, it's going to undoubtedly come with additional requirements at that point, and there's no point in implementing this without the concrete use cases that require it, especially as a core app. I'm going to close this PR for now and park the idea of Collections as a whole. I'll also extract out the small handful of publishing model changes (changing the primary key size and some reverse relation naming). |
* docs: adds guidelines for the Collection models copied and modified slightly from @ormsbee's #131 * feat: adds CollectionPublishableEntity model and APIs to retrieve and associate PublishableEntities with Collections. * feat: Collection.modified timestamp is updated whenever its entities are changed. * refactor: merge get_collections + get_learning_package_collections We ended up not needing a "get all collections" function; we always fetch them for a given learning package. * chore: bumps version to 0.11.3
To support having an independently versioned set of things within a LearningPackage.
FYI @kdmccormick, @bradenmacdonald, @feanil: Putting up a draft. I think the data models are roughly where I want them (though the names are definitely up for change). Just started writing tests, and the API layer is still incomplete.
As is typical with my PRs, the models are the most fleshed out things at the moment. 😛