openedx / modular-learning

3 stars 1 forks source link

[Libraries] [Collections] [Backend] API to associate library component(s) with collection(s) #227

Open bradenmacdonald opened 1 month ago

bradenmacdonald commented 1 month ago

This step involves implementing a way to associate library component(s) with one or more collection(s).

  1. Define new CollectionObject Model with the following fields in openedx_learning:
    • collection_id - ForeignKey to Collection instance
    • object_id - ForeignKey to PublishableEntity instance (e.g. library component). This would allow for adding other publishable items (like units + sections) to collections in the future.
  2. Create a python API in openedx-learning that can associate a [set of] publishable entities with a [set of] collections, or remove an entity/entities from the given collection(s)
  3. Create a REST API in content_libraries django app that can do the same as 2 for library components, with permissions checks.
  4. Collections can not be associated with other collections.
  5. Library components can be associated with multiple collections.
  6. Emit an event(s) when the collections associated with a component have changed. This can just be the existing CONTENT_OBJECT_TAGS_CHANGED event, if we think of collections as a sort of special tag.
  7. Modifying a collection's components should update the collection's modified timestamp.
pomegranited commented 1 month ago

@ormsbee I'm planning on implementing this task next sprint (20 Aug - 1 Sep). Do you have time to CC review the openedx-learning changes and maybe edx-platform too?

ormsbee commented 1 month ago

@pomegranited: Sure!

ormsbee commented 1 month ago

@pomegranited: I have an abandoned PR from back when we thought we'd do a tag-based approach to this, in case it's helpful: https://github.com/openedx/openedx-learning/pull/131/files

pomegranited commented 3 weeks ago

@ormsbee

I have an abandoned PR from back when we thought we'd do a tag-based approach to this, in case it's helpful: https://github.com/openedx/openedx-learning/pull/131/files

Thanks for this! There's a lot more in there than what is specified in this ticket description.

How much do you want me to do here? Specifically:

Thanks for your input!

ormsbee commented 3 weeks ago

Do Collections need to be PublishableEntities?

No. In fact, the way they're currently envisioned, they explicitly shouldn't be. They aren't in the PR I linked.

Should Collections link to a Component or a ComponentVersion? My understanding is they should link to Component, and so don't need to be updated if the version changes.

Yes. They should be linked to the PublishableEntity, not the PublishableEntityVersion.

I think there's an edge case here around soft-deletion: Does soft-deleting (i.e. setting the draft version to null) a PublishableEntity mean that it is removed from all Collections? If I soft-delete something today and make a new entity with the same key, it will treat it as a new version of the same PublishableEntity. So if I soft-delete something that was in a Collection, and then create a new thing that exactly matches the key, should it go back into all the Collections it was a part of originally?

Do changes to Collections need to be recorded?

It's out of scope for MVP, but may be something we'll look into later.

pomegranited commented 3 weeks ago

@ormsbee

No. In fact, the way they're currently envisioned, they explicitly shouldn't be. They aren't in the PR I linked.

Ach sorry.. I skimmed CollectionPublishableEntity too quickly, I get it now.

Yes. They should be linked to the PublishableEntity, not the PublishableEntityVersion.

👍

I think there's an edge case here around soft-deletion: Does soft-deleting (i.e. setting the draft version to null) a PublishableEntity mean that it is removed from all Collections? If I soft-delete something today and make a new entity with the same key, it will treat it as a new version of the same PublishableEntity. So if I soft-delete something that was in a Collection, and then create a new thing that exactly matches the key, should it go back into all the Collections it was a part of originally?

Creating a new entity with the same key is one use case, but your code comments mention a more common use case where a soft-deleted PublishableEntity is "un-deleted" and restored back to the most recent PublishableEntityVersion.

So I think that when a PublishableEntity is soft deleted, we have to preserve the link between the Collection and the PublishableEntity just in case it gets un-soft-deleted. But that means that we can't really get around the other use case, where a re-created PublishableEntity re-appears in all the Collections it was previously linked to. But a content author could always explicitly remove a re-created PublishableEntity from their Collection if they don't want it there anymore.

@ormsbee Am I reading this right?

ormsbee commented 3 weeks ago

Creating a new entity with the same key is one use case, but your code comments mention a more common use case where a soft-deleted PublishableEntity is "un-deleted" and restored back to the most recent PublishableEntityVersion.

Oh right, good point.

So I think that when a PublishableEntity is soft deleted, we have to preserve the link between the Collection and the PublishableEntity just in case it gets un-soft-deleted. But that means that we can't really get around the other use case, where a re-created PublishableEntity re-appears in all the Collections it was previously linked to. But a content author could always explicitly remove a re-created PublishableEntity from their Collection if they don't want it there anymore.

@ormsbee Am I reading this right?

Yeah, I think that's fine. I mean, I guess we could do the actual "remove from Collection" after they publish a delete (because they wouldn't be able to revert at that point). But it's probably not worthwhile to do that yet–I'm honestly not even sure what the desirable behavior would be in my obscure "re-create the same component" use case. And as you point out, there's a really simple workaround to that default behavior if anyone runs across it.