Thanks a lot for the thoughtful answer. About locating the configuration in the django settings we were also leaning towards it, because of the lack of control of the ordering we would have using the Plugin App Configs.
About the other comments you brought up, we are very happy to take your feedback into account even if you might not be the target audience for this. You have extensive experience in a larger scale of operation and might be seeing things in advance that we can not even imagine.
I’d like to follow up on those.
Ergonomics of Writing the Hooks
Agreed. It is super important to make clear how big of a public promise this is. So far we thought we would try to maintain this via unit-tests. Basically testing that the triggers are being called with the correct input and the output of the filters is used as the parameter for calling the functions they are designed to transform.
Now, for the tests, we had in mind using the definition of the test to hightlight the individual documentation of each trigger. E.g: edx-platform/pre_enrollment_trigger.rst at fmo/initial_hook_framework_docs · eduNEXT/edx-platform · GitHub.
The same could be done at the place in the code where the trigger is called.
from edx_django_utils.hooks.triggers import trigger_filter
# Public hook. See: https://github.com/eduNEXT/edx-platform/blob/fmo/initial_hook_framework_docs/docs/guides/hooks/pre_enrollment_trigger.rst
out = trigger_filter('openedx.lms.enrollment.pre_enrollment.filter.v1', user=user, course_key=course_key, mode=mode)
This comes as the example of our current implementation of the trigger_action and trigger_filter functions. We have it as an internal PR to own fork for now (feat: Add Hooks Extension Framework tooling by mariajgrimaldi · Pull Request #6 · eduNEXT/edx-django-utils · GitHub).
Do you think that is enough to prevent anyone from casually changing the public signature of the functions?
I’m interested and curious as how the decorators could work for this.
Filter-style hooks sound like middleware
Do filters have the capability of altering parameters to the function (as well as altering the results coming back)?
If so, they sound a lot like middleware.
I tend to think in the middleware that goes before and after the django views and this do differ a lot from that. But in the broader sense of middleware, yes it does. Now, the django middleware list does also run in a a pipeline since the request that you pass to the first middleware is returned and pass to the second middleware and so. However we are drawing inspiration in the social-core pipelines for the mechanics of what the pipeline should return and how to stop it or raise exception from it.
I think that’s a good thing actually, but it makes me wonder if these filter functions should be classes instead, so that you could get called with some sort of initialization. Maybe that’s overkill, but it might also be a straightforward way to provide the request object context to something that wouldn’t normally use it, in case your filter needed to check some querystring params or some such.
I think of filters as “the agreed upon places” where developers working outside the platform core get access to data that determines business rules of the application. I like the idea of the classes and this ties nicely with the next point but for now it does seem like overkill. However doing it now, even with very simple classes, can make the public promise more stable since we already have the base class and there is no need to ask developers in the future to go from function based hooks to class based ones.
I was unaware of the async views+middleware thing coming for django 3.2. I have made it a task for our team to review it and make plans accordingly. I agree that we might not need to do anything yet but we need to leave enough space in the design to accommodate when the time comes. Using classes instead of functions to implement the actions/
triggersfilters could very well be the way to go.
So far we have been writing this code in the edx-django-utils library. If we want to add support for celery there we would need to include celery as a dependency. I don’t know if this is a big thing and we should try to avoid it or just go ahead and add it.
If we wanted to avoid it the only thing I can come up with is to put this in a different (new) library that depends on edx-django-utils and includes celery. Thoughts on this?
Django Signals for broadcasts
I left this for last because it the one where I am having a stronger opinion and I wanted the previous context before answering.
I see a lot of value in using django signal and I was initially the proponent of using signals everywhere. However after the firsts rounds of feedback, you helped me understand that we where not actually gaining much from constraining the use of actions to the django signals pattern.
Since we are introducing a new way of extending the platform, we are asking developers and operators across the community to learn yet another pattern to get Open edX to do what they need. I’m giving this a lot of weight and I’m trying to make it just one pattern that lets you alter the core in 44 ways. This might force a bit of a compromise in the way we write the framework, but I feel strongly against adding 2 patterns which let you alter ~20 places each.
Keeping the two mechanisms similar would facilitate developers to write an Action that can be used at any trigger in the platform and can foster a community supported list of integrations to third party services (CRMs for example). It would still be possible to do it if we separate the mechanism, but it would raise the entry barrier.
As long as the configuration and calling mechanism is the same, I’m all for using the django signals pattern internally. But I don’t think this was your idea. Now, if we do it, I still have the question whether this means creating new signals for the triggers or re-using the existing signals for the Django models that we already have (for example, CourseEnrollment.post_save)? I am inclined here to use new signals, because a big part of this proposal is the general and individual documentation that would let you find the available triggers. Connecting to the post_save of an existing model is already permitted in the current form of the plugins and I have not seen much use of that pattern anywhere.
Am I missing the point of why you support separating the mechanisms? Perhaps I’m putting to much weight on the whole keeping the two things similar. Does anyone else have an opinion here?
Thanks a lot for all the amazing contributions to this conversation.