Emit events when a learning package is modified [FC-0117]

[bradenmacdonald]

Implements #462 .

This updates openedx-code to emit the following events:

1. LEARNING_PACKAGE_ENTITIES_CHANGED

openedx-core/src/openedx_content/applets/publishing/signals.py

Lines 90 to 95 in 7ad1e79

	The draft version of one or more entities in a `LearningPackage` has changed.
	This is emitted when the first version of an entity is **created**, when a new
	version of an entity is created (i.e. an entity is **modified**), when an entity
	is **reverted** to an old version, when **a dependency is modified**, or when an
	entity is **deleted**. (All referring to the draft version of the entity.)

2. LEARNING_PACKAGE_ENTITIES_PUBLISHED

openedx-core/src/openedx_content/applets/publishing/signals.py

Lines 125 to 129 in 7ad1e79

	This is emitted when **a newly-created entity is first published**, when
	**changes to an existing entity** are published, when **changes to a
	dependency** (or a dependency's dependencies...) are published, when a published
	entity is **reverted** to a previous version, or when **a "delete" is
	published**.

3. LEARNING_PACKAGE_COLLECTION_CHANGED

openedx-core/src/openedx_content/applets/collections/signals.py

Lines 48 to 49 in 7ad1e79

	A ``Collection`` has been created, modified, or deleted, or its entities have
	changed.

4. LEARNING_PACKAGE_CREATED / LEARNING_PACKAGE_UPDATED / LEARNING_PACKAGE_DELETED. Not sure we have much use case for these yet, but they seem like important low-level hooks.

TODOs

Test that the performance of event handlers responding to set_collections is OK in practice and change it to async if needed.

Notes

Tagging events are much more complicated so will be in their own PR. The proposed spec is at #557

While working on this, I also opened openedx/openedx-events#570 which would make working with these kind of events easier in the future, and let us slightly simplify this code in a few places.

These events are defined locally in openedx_content for now but we could move them to openedx-events in the future?

Platform PR

openedx/openedx-platform#38437

The platform PR is approx -500 lines of non-test code, and +265 lines of test code, and I think it's much cleaner, so I'm really happy with the downstream effects of this PR. Part of this is being able to take advantage of the "dependencies" / "side effect" tracking properly, which the original library events weren't able to do at the time.

[openedx-webhooks]

Thanks for the pull request, @bradenmacdonald!

This repository is currently maintained by @axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are 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.

Details

---

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.

[bradenmacdonald]

Working on this PR has made me want to add fully-typed IDs for PublishableEntityVersion and some kind of VersionNum type, because we do mix and match those a bit in the code, but they're very different things.

(I made PublishableEntity.id fully-typed but hadn't yet done ___Version)

[bradenmacdonald]

Unfortunately, openedx-events doesn't yet support typing so event.kwargs is just a generic dict.

[bradenmacdonald]

This helper would not be necessary if openedx/openedx-events#570 is accepted, and then we could just use that.

[bradenmacdonald]

I am using multiple x: XData values in the data dict per event. I think this is a much cleaner and DRYer pattern than forcing all events to have a single key-value pair in their data dictionaries, but most (all?) event definitions in openedx-events have only a single key-value pair in the dict even when it doesn't make sense, and I don't know why.

Edit: based on the additional info I posted there, it should be completely fine. But any automatic parsing of the signal documentation may have incomplete type info (or rather, slightly more incomplete than it already is, since it already lacks the dict key info for all events).

[ormsbee]

Trying to do this in pieces to give a quicker feedback cycle. This is for Collections.

[ormsbee]

Minor comments on the rest of the app code. I don't consider any of these blocking. I'm going to look through the rest of the tests now.

[ormsbee]

Nit: So technically, this means that you can get a deletion event for a collection that never emitted a create event, right?

[bradenmacdonald]

Hmm, I should add a test case for that but technically no, having enabled=False should be the same as never being created at all as far as events are concerned, and then actually deleting it should not emit any further events. But enabling it will emit a CREATED event and then re-disabling it ("soft deleting") should emit a deleted event. Let me know if you disagree.

[bradenmacdonald]

Added a fix+test for that edge case: 5ff0496

[ormsbee]

@bmtcril, @mariajgrimaldi, @robrap: We're declaring an OpenEdxPublicSignal outside of openedx-events here. A couple of questions:

1. Is this going to have any weird consequences w.r.t. the message bus? Should we be making it a simple Django signal for now?
2. @bradenmacdonald is introducing a Sphinx doc convention of doing docstring-style comments for module variables. Will this cause any problems? Should we also be putting the normal annotations above the signal? (I'm not even clear if the normal annotations for signals are picked up if they're defined outside of openedx-events.)

[ormsbee]

I'm guessing that we'll eventually want a pre-delete hook as well, but that's definitely puntable.

[ormsbee]

Nit: Is it worth calling out in the name that it's more specifically when the Entities' draft state changes?

[bradenmacdonald]

Yeah, you're right - the more explicit the better. Some of the other openedx-events have quite ambiguous names and documentation and that's been a pain.

Do you like LEARNING_PACKAGE_ENTITIES_DRAFTS_CHANGED? Or since that's getting rather long we could consider ENTITIES_DRAFTS_CHANGED. (Or _DRAFT_CHANGED singular?)

[ormsbee]

I have a mild preference for ENTITIES_DRAFT_CHANGED with a corresponding ENTITIES_PUBLISHED.

[bradenmacdonald]

Or maybe DRAFT_ENTITIES_CHANGED and ENTITIES_PUBLISHED ? I think that reads better.

[ormsbee]

I'd prefer ENTITIES_DRAFT_CHANGED to keep it closer to DraftChangeLog and PublishLog.

[bradenmacdonald]

Signals renamed in 26683ff:

• ENTITIES_DRAFT_CHANGED
• ENTITIES_PUBLISHED
• COLLECTION_CHANGED

[ormsbee]

Nit: Why do this import? The signal receiver import happens in apps.py anyway, no?

[bradenmacdonald]

This is for the public API, e.g. in my platform PR:

from openedx_content.api import signals as content_signals
...
@receiver(content_signals.LEARNING_PACKAGE_ENTITIES_CHANGED)
def entities_updated(...):
    ...

or even in this PR:

openedx-core/tests/openedx_content/applets/publishing/test_signals.py

Line 46 in a53166d

with capture_events(signals=[api.signals.LEARNING_PACKAGE_CREATED], expected_count=1) as captured:

[ormsbee]

Oh, I see. I thought it would be expected that clients import api_signals. But if they're expected to import api.signals, it seems like the aggregating thing should be just signals.py to match what we do with models.py and admin.py, and then api.py should re-export that?

[bradenmacdonald]

I already find it annoying to almost always need a second import of api_models in addition to api everywhere we use these APIs, so I do prefer including signals in the api instead of a separate api_signals. But if you think there's a good argument for not always loading them and having them in a separate import, let me know.

I definitely don't mind renaming api_signals to signals; I just wanted to signal that things in it were public, but maybe that's obvious if you look at it.

[ormsbee]

Yeah, I think it's fine to:

1. Do the aggregation in signals.py and then;
2. Import signals.py into api.py with the expectation that all external clients will import from api.signals

I guess that if we ever do have internal signals for some reason, we can tweak things with an intermediate module later before we re-export.

[bradenmacdonald]

Updated api_signals -> signals with a lot more explanation in e42144b

[ormsbee]

No blockers. I would prefer the signal name be changed and the signal aggregation be done in signals.py instead of api_signals.py before being re-exported, but I have no additional concerns. Thank you.

[kdmccormick]

@bradenmacdonald I won't review this unless you'd like me to.