feat: Add Content Library Collections signals [FC-0062]

[yusuf-musleh]

Description: This PR adds new signals for Content Library Collections.

The following new signals will be added:

• LIBRARY_COLLECTION_CREATED - emitted when new library collection is created
• LIBRARY_COLLECTION_UPDATED - emitted when a library collection is updated
• LIBRARY_COLLECTION_DELETED - emitted when a library collection is deleted

ISSUE: openedx/modular-learning#226

Dependencies: Needed for openedx/edx-platform#35321

Merge deadline: List merge deadline (if any)

Installation instructions: List any non-trivial installation
instructions.

Testing instructions:

Make sure tests are passing, and follow testing instructions in openedx/edx-platform#35321

Reviewers:

• tag reviewer
• tag reviewer

Merge checklist:

• All reviewers approved
• CI build is green
• Version bumped
• Changelog record added
• Documentation updated (not only docstrings)
• Commits are squashed

Post merge:

• Create a tag
• Check new version is pushed to PyPI after tag-triggered build is
  finished.
• Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

[openedx-webhooks]

Thanks for the pull request, @yusuf-musleh!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If 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 @openedx/hooks-extension-framework. Tag them in a comment and let them know that your changes are ready for review.

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:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

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:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[pomegranited]

@bradenmacdonald Are we allowed to put new signals in this repo? Or are we supposed to put them in https://github.com/openedx/openedx-events ?

[bradenmacdonald]

I'm actually not sure. Maybe @mariajgrimaldi can answer? What's the difference between this signals.py and that signals.py?

[felipemontoya]

I'm not sure I follow. This repo es openedx-events and both signals.py files are the same.

You are definitely allowed to propose new signal definitions in this repository and the location is correct. The idea of having the signal here instead of in the edx-platform code is that plugins that will listen for the signal can include this lightweight library in their dependencies instead of requiring edx-platform all the time.

[bradenmacdonald]

lol whoops, please ignore my confusion. I thought this was edx-platform :/

[felipemontoya]

No worries. Happy to address this in depth. As before, if you are planning to add a signal into edx-platform, then it would be better to write it here so plugins can use that without the large import.
However, other pattern we have seen is when a plugin defines a new signal and uses it in its own code. In that case if reuse of said signal is not a goal from the devs, then the definition can be left in the plugin as you could import that plugin as a library if you are writing a second level library/plugin.

[pomegranited]

My apologies for the noise! I could have sworn there was a signals.py file in openedx-learning, but it's not there now, so not sure what I was on about :)

[pomegranited]

Ah, it was here in this closed PR! signals.py. I'm not going nuts.. 😊

[rpenido]

LGTM! 👍

I would prefer not to add the LIBRARY_COLLECTION_DELETED without implementing it. If we decide not to implement the delete action (or take some time to do it), this code could lead to some confusion.

I put the same comment in the edx-platform PR. Feel free to push back if you think differently!

[bradenmacdonald]

@rpenido We will definitely need to implement deleting collections as part of this MVP so we might as well add the event now to save another PR.

[bradenmacdonald]

@ormsbee Do you think it makes sense to have these hooks in openedx-events called e.g. LIBRARY_COLLECTION_CREATED/UPDATED/DELETED and specific to libraries emitted by the library code, or should we have a general COLLECTION_CREATED/UPDATED/DELETED that is emitted by openedx-learning and which perhaps has some sub-field in the event data specifying it's a library collection? (prob. along with which learning_package the collection is part of)

@yusuf-musleh I'm also wondering if we should have a separate event for when the contents of a collection are updated, i.e. a component is added to the collection or a component is modified within the collection. That case can be handled differently in some cases. Or a field like reason on the _UPDATED event that says why it was updated: component added/removed, component updated, or collection metadata updated

Keep in mind I'm not asking for these changes, just proposing them and wondering if this makes sense.

[ormsbee]

@ormsbee Do you think it makes sense to have these hooks in openedx-events called e.g. LIBRARY_COLLECTION_CREATED/UPDATED/DELETED and specific to libraries emitted by the library code, or should we have a general COLLECTION_CREATED/UPDATED/DELETED that is emitted by openedx-learning and which perhaps has some sub-field in the event data specifying it's a library collection? (prob. along with which learning_package the collection is part of)

I'm inclined to start with signals in openedx-learning, and add them to openedx-events where there's a clear use case for it. The signals in openedx-events constrain themselves to be very portable, JSON-serializable, etc. What I found when I was initially creating my first stab at Collections was that we also wanted some really low-level signals where passing the model instance itself was very useful (see PUBLISHED_PRE_COMMIT).

[pomegranited]

@ormsbee @bradenmacdonald

What I found when I was initially creating my first stab at Collections was that we also wanted some really low-level signals where passing the model instance itself was very useful (see PUBLISHED_PRE_COMMIT).

I disagree.. I don't think events should be used for synchronous actions like this -- they're for notifying other areas of the code about something that happened, possibly via the event bus. I think if we want a mechanism for rolling back commits, it should be some kind of hook.

But I'm less sure about where these events should live.. The argument for openedx-events being separate from edx-platform is obvious, because no one wants to import edx-platform just to receive some events. But since you'll need the openedx-learning APIs to access any objects indicated by the events, you have to import it anyway.

Does having all the events in one place make it easier to document them? If so, they should continue to live in openedx-events.

@yusuf-musleh

I'm also wondering if we should have a separate event for when the contents of a collection are updated, i.e. a component is added to the collection or a component is modified within the collection. That case can be handled differently in some cases. Or a field like reason on the _UPDATED event that says why it was updated: component added/removed, component updated, or collection metadata updated

I'm doing three things for openedx/modular-learning#227 when adding/removing components:

• Raising a CONTENT_OBJECT_TAGS_CHANGED event for each affected content library block, to trigger reindexing the content object (see [Libraries] [Collections] [Backend] store component-collection association in Studio Library Index modular-learning#230, WIP branch).
  This event must occur in content_libraries because we need the Library Block's usage key (not the Component key) to trigger the search index update.
• Updating the collection.modified dates when contents of the collection change.
  We could also trigger COLLECTION_UPDATED events here for each affected collection. Note that there would be 2 reasons in this case, since we've updated the components + modified metadata.
• Creating a collection can also involve adding components.
  If we start raising events here, we'd have COLLECTION_CREATED + COLLECTION_UPDATED[components, metadata], as noted above.

[rpenido]

Raising a CONTENT_OBJECT_TAGS_CHANGED event for each affected content library block

I'm not thrilled about using the TAG event here. Today, tagging and collections are completely decoupled. We could deactivate tags and still have collections. This will not break the collections as we implement it, but I still find it odd to use the tags events, functions and properties in the index with the tagging feature disabled.

If we start raising events here, we'd have COLLECTION_CREATED + COLLECTION_UPDATED[components, metadata], as noted above.

If we pass the changed component list in the COLLECTION_UPDATED[components] event (we need to evaluate the key size to make it doable), we could use this event to update the tag_collection (or just collection) in the component document in the index.

As a bonus, we can do a single index update (instead of one call for each component in the CONTENT_OBJECT_TAGS_CHANGED case).

Does this make sense?

[yusuf-musleh]

I'm also wondering if we should have a separate event for when the contents of a collection are updated, i.e. a component is added to the collection or a component is modified within the collection. That case can be handled differently in some cases. Or a field like reason on the _UPDATED event that says why it was updated: component added/removed, component updated, or collection metadata updated

@bradenmacdonald @pomegranited I like the idea of adding a reason field to the LIBRARY_COLLECTION_UPDATED event more, as opposed to creating more events that describe what is being updated.

Depending on how the collection was updated, whether its the collection itself or its contents, there will likely be more than one reason, eg: when a component is added to a collection (reason 1) the modified date for the collection is updated (reason 2), as @pomegranited mentioned. To avoid emitting multiple events, I think we can have the reason be a list of objects that describe what has been updated and hence trigged the event. For the example above it could looks something like:

[
  {
    "type": "metadata",
    "change": "modified updated",
    "value": "..."
  },
  {
    "type": "content",
    "change": "components changed"
    "value": ["cmp1", "cmp2", "..."],
  },
]

This is also great because it gives us more flexibility on what type of reasons we can provide. In the future, if we'd like to add more granularity to the "components changed" reason for example, we can just pass in more reasons like "components added" and "components removed", each listing which components were added/removed, without needing to make changes to the events in this repo. The changes would be done in the emitting/handling code logic. So extending it to meet changing requirements would be easier.

@rpenido

I'm not thrilled about using the TAG event here. Today, tagging and collections are completely decoupled.

That is still the case, that they are decoupled. There was a discussion in the discovery about reusing this event (and it's logic) to update meillisearch index for the content object when either the content objects tags or it's collections changed. The name of the event now would be a bit confusing, since technically now it's not just the content object TAGS are being changed, but more general than that.

I can see how this can get confusing though. It might be interesting to revisit the discussion of possibly renaming or creating a new more general event, eg: CONTENT_OBJECT_ASSOCIATIONS_CHANGED @bradenmacdonald ?

[pomegranited]

Re event reasons: I don't think there's a use case for this level of granularity in the event, right now anyway. If that changes, we can add this detail to the events, and we're still backward compatible.

If we pass the changed component list in the COLLECTION_UPDATED[components] event (we need to evaluate the key size to make it doable), we could use this event to update the tag_collection (or just collection) in the component document in the index.

As a bonus, we can do a single index update (instead of one call for each component in the CONTENT_OBJECT_TAGS_CHANGED case).

Actually, emitting a single COLLECTION_UPDATED[components] event doesn't help us with reducing the index updates, because we still need to update each component's index. It's the components' indexes that we'll use to filter/identify components in a collection, not the collection's index -- just like we do with libraries.

I get that using the CONTENT_OBJECT_TAGS_CHANGED named event for component updates is confusing, but I like the idea of storing them in the index alongside tags. Because that's really all they are, just a way to group components. If we had free-text tags, I'd have used them instead of a model :)

But I'm ok with whatever @bradenmacdonald decides.

[pomegranited]

👍 I'm good with these events -- both where they live, and the data they contain. We can add more detail later if the use case arises.

• I tested this as part of feat: Add Library Collections REST endpoints [FC-0062] edx-platform#35321
• I read through the code
• I checked for accessibility issues N/A
• Includes documentation
• User-facing strings are extracted for translation

[rpenido]

Actually, emitting a single COLLECTION_UPDATED[components] event doesn't help us with reducing the index updates, because we still need to update each component's index. It's the components' indexes that we'll use to filter/identify components in a collection, not the collection's index -- just like we do with libraries.

Just to clarify, it would certainly be a batch of changes, but they can be done in only one meilisearch API call, with less overhead.

[bradenmacdonald]

I do still like the idea of adding reason fields where we can - it means there's a single event, so consumers won't miss out on new "reasons" we add in the future, but can still have granular/smart behavior by ignoring reasons that aren't relevant to them.

Renaming the TAGS event to CONTENT_OBJECT_ASSOCIATIONS_CHANGED makes sense to me, especially if we add a field to indicate if it's tags or collections that changed.

I don't have a strong opinion on these events though.

[pomegranited]

@bradenmacdonald Should the COLLECTION_CREATED / UPDATED events be sent from openedx-learning or edx-platform?

[bradenmacdonald]

@pomegranited I would prefer to send them from openedx-learning so we only implement it once, and they're consistent - but that assumes we have an easy way to indicate what type of collection it is in the event (i.e. that it's a library collection). And I'm not sure if Learning Core "knows" about that. So if that's not easy to do, then I guess we'll emit them from platform.

[rpenido]

LGTM 👍
Thank you for your work, @yusuf-musleh !

• I tested this using the instructions from feat: Add Library Collections REST endpoints [FC-0062] edx-platform#35321
• I read through the code
• I checked for accessibility issues
• Includes documentation

[pomegranited]

Hello @openedx/hooks-extension-framework people 😃 Can anyone help us with reviewing & merging this change?

Sorry, a I'm a bit premature here, we have one more issue to sort out first.

[pomegranited]

Ok! @openedx/hooks-extension-framework this is now ready for your eyes, thank you!

FYI I'm unable to update the PR description here, but this PR also includes Collection-related changes from open-craft#66.

[navinkarkera]

@yusuf-musleh @pomegranited LGTM, left some suggestions which you can ignore if you disagree.

[navinkarkera]

Suggested change

	event_type="org.openedx.content_authoring.content.object.associations.changed.v1",
	event_type="org.openedx.content_authoring.content_object.associations.changed.v1",

[pomegranited]

Thank you for your review @navinkarkera ! I believe I've addressed them, and so if you're happy, feel free to merge and create a new release :)