How to implement event receivers could be clarified and possible DEPR

[robrap]

The how-to for receiving events documents two options (see https://github.com/openedx/openedx-events/blame/main/docs/how-tos/using-events.rst#L10-L53).

1. Connecting signals using regular django syntax, and
2. Connecting signals using apps.py plugin configuration.

Please help me check my understanding. Is the following accurate?

I believe that the second option, using apps.py plugin configuration, pre-dates the use of openedx-events. In the earliest version of plugins, they needed to reach into a signal definition that was defined in edx-platform (as an example), and was not exposed to the plugin. Now that openedx-events signals exist, the signal can instead be imported from openedx-events, and regular django syntax can be used. In fact, the earlier mechanism of using apps.py should probably be deprecated, in favor of transitioning any necessary signals to an openedx-event.

Inside edx-platform, there are many uses of apps.py to listen to signals. See https://github.com/search?q=repo%3Aopenedx%2Fedx-platform+plugin_app&type=code. I find this very confusing, because it is using "plugin" syntax for non-plugin apps that live inside the repo. Is there any good reason to do this, or is this just a lot of copy/paste of something that doesn't make as much sense?

One proposal is to deprecate the apps.py plugin way of listening to signals. If the signal is an OpenedxSignal, then the warning could simply ask to refactor to use simple django syntax. Otherwise, the deprecation warning could note that the simple django syntax should be accessible for any signal inside the same repo, or if this is truly for a plugin, then a new signal should be added to openedx-events to expose the event.

Thoughts?

[kdmccormick]

@robrap I have not written a plugin with openedx-events, but I agree that if we have two equally-powerful ways of implementing receivers, one which uses idiomatic Python/Django and the other of which has a bespoke stringly-typed config syntax, then we should keep the former and depr the latter.

Inside edx-platform, there are many uses of apps.py to listen to signals. See https://github.com/search?q=repo%3Aopenedx%2Fedx-platform+plugin_app&type=code. I find this very confusing, because it is using "plugin" syntax for non-plugin apps that live inside the repo. Is there any good reason to do this, or is this just a lot of copy/paste of something that doesn't make as much sense?

Yes, I find that confusing too. This all predate ADRs, but I believe that the reasoning at the time was something like:

• If we turn built-in edx-platform djangoapps into plugins, then it'll make them easier to extract.
• If edx-platform repo developers dogfooded the django plugin system, they would learn it and contribute to it.
• Built-in XBlocks are still XBlocks, so built-in Django apps should still be plugins.

I could write more about why I disagree with each of those, but to be brief, I think that building internal apps as django plugins has just led to a stranger, more-indirect codebase with a higher barrier to entry for new maintainers.

[robrap]

If we turn built-in edx-platform djangoapps into plugins, then it'll make them easier to extract.

• That (and the rest of your comment) is great context. I turned this into a new edx-platform issue: Create ADR for avoiding use of plugin patterns where they are not needed edx-platform#35898.
• We'll need to decide if a DEPR makes sense, and if so, I guess that would belong in edx-django-utils with the plugin code. Does that also make sense to ticket now or at some point?
• That said, improving the documentation in this repo may be a less expensive step in the right direction if we all agree. I think we simply want to drop the string-based plugin pattern from these docs altogether, right?
  • There still would be references in edx-django-utils for legacy usage. I don't know if those docs need to be improved as well, but maybe that could all be related to this issue.

[mariajgrimaldi]

I believe that the second option, using apps.py plugin configuration, pre-dates the use of openedx-events. In the earliest version of plugins, they needed to reach into a signal definition that was defined in edx-platform (as an example), and was not exposed to the plugin. Now that openedx-events signals exist, the signal can instead be imported from openedx-events, and regular django syntax can be used. In fact, the earlier mechanism of using apps.py should probably be deprecated, in favor of transitioning any necessary signals to an openedx-event.

I don't have a very good reason to explain why we went with the suggested way of listening to signals in plugins back then for the docs - this is the method I remember when I started experimenting with Django plugins. Even though the advantages of having a separate library to implement signals, we didn't question whether this should still be the recommended way after the library's implementation and consequent adoption. So, thank you for bringing this up!

Although I have a few concerns about deprecating this approach, which is mainly related to the fact that opened-events are not a drop-in replacement to Django signals in the edx-platform - so we still have a few of them to support. So, before deprecating it, we should offer a migration path for users of those signals.

As for the docs, a few months back I created this list of docs improvements to gradually implement this year. I added this one to it, so I plan to work on it in December or January at the latest.

[robrap]

1. Here is a proposed doc change for this repo: docs: update signal receiver how-to #422
2. I found https://github.com/openedx/edx-platform/blob/73ba58ae11a9b1a896cdeeb515d1818c207f6b21/docs/hooks/events.rst, which is a slightly redundant doc. Do we want DRY docs in this case, or not?

Although I have a few concerns about deprecating this approach, which is mainly related to the fact that opened-events are not a drop-in replacement to Django signals in the edx-platform - so we still have a few of them to support. So, before deprecating it, we should offer a migration path for users of those signals.

@mariajgrimaldi: I believe this can be deprecated sooner-rather-than-later to avoid the contagion factor. That doesn't mean the feature will be removed until it is no longer needed. I think the "replacement" path for any legacy signal that is already in use in this way would be to introduce an openedx-event, which is a process that would take time, and could be tracked separately. Does this address your concern, or do you have other concerns? Thank you.

UPDATE:

... that opened-events are not a drop-in replacement to Django signals in the edx-platform

Note that we don't need a replacement of all signals, but just signals that are exposed publicly for consumption in a plugin. As far as I am aware, that is exactly what openedx-events is for, and these are the only type of events that should ultimately be consumed by plugins. I may be missing something though.

[mariajgrimaldi]

@robrap:

1. Thank you! As I mentioned, I'll also use the recommended method when updating the docs.
2. Yes! This PR with a centralized approach was recently merged, so I'll drop outdated (duplicated) docs.
3. You're absolutely right. However, developers also consume signals implemented in edx-platform as well. I believe the replacement path should be enough to address this. Thank you.

[robrap]

@mariajgrimaldi: Regarding the first point, its unclear whether you plan on reviewing my proposal? If we like it, we can land this change now, and not have to wait for your larger doc effort.

• See docs: update signal receiver how-to #422

Or @kdmccormick, if you end up reviewing it.

[mariajgrimaldi]

@robrap: I'm sorry for not clarifying. I reviewed and approved the PR, so it's ready to merge. Thank you.

[robrap]

Proposed action items left for this ticket:

• Clean up https://github.com/openedx/edx-platform/blob/73ba58ae11a9b1a896cdeeb515d1818c207f6b21/docs/hooks/events.rst, or confirm that this is tasked with the upcoming docs work. @mariajgrimaldi: Can you confirm this is tasked?
• Create a DEPR ticket in edx-django-utils. Although it may not be announced soon, it is a better place to capture some of these details and allow this ticket to be closed. I’ll take care of this.

[mariajgrimaldi]

@robrap: confirmed! I'll take care of it on Monday. Thank you!

EDIT: PR openedx/edx-platform#35921

[mariajgrimaldi]

Clean up PR merged: openedx/edx-platform#35921