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)

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

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

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).

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

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.

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

@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.)

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

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

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?)

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

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

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

Signals renamed in 26683ff:

• ENTITIES_DRAFT_CHANGED
• ENTITIES_PUBLISHED
• COLLECTION_CHANGED

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

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:

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?

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.

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.

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