Conversation
src/ol_openedx_course_staff_webhook/ol_openedx_course_staff_webhook/tasks.py
Outdated
Show resolved
Hide resolved
src/ol_openedx_course_staff_webhook/ol_openedx_course_staff_webhook/signals.py
Outdated
Show resolved
Hide resolved
257e163 to
fcca452
Compare
There was a problem hiding this comment.
Some comments in general:
It might not be a good idea to specifically create a whole plugin for enrollment-related webhook. If we follow that approach, we might end up creating a huge list of plugins in the future. Instead, I think we should think ahead of the time and decide the architecture of things like these.
My idea is that we create this plugin as a general one for all of our baseline events and filter (maybe?). So this could be a generic plugin that can hold all the filters and events from Open edX taken in for MIT OL.
I was working on something similar #684, A PR that hasn't been merged yet, but in that PR I tried to keep it generic so that this plugin could hold future filters and events as well and is not restricted to a single event/filter.
The name for now could be ol_openedx_events_handler/listener, whatever, but not ol_openedx_course_staff_webhook because that name itself is incorrect because:
- That is not generic
- It gives the impression that this is a webhook itself, but it's not.
| Default settings for the MITx Online integration plugin. | ||
|
|
||
| These are applied to both LMS and CMS configurations. | ||
| """ | ||
| # URL of the MITx Online webhook endpoint for course staff enrollment. | ||
| settings.MITXONLINE_WEBHOOK_URL = None | ||
|
|
||
| # API key / Bearer token for authenticating with the MITx Online webhook. | ||
| settings.MITXONLINE_WEBHOOK_KEY = None | ||
|
|
||
| # Course access roles that should trigger the MITx Online enrollment webhook. | ||
| settings.MITXONLINE_COURSE_STAFF_ROLES = ["instructor", "staff"] |
There was a problem hiding this comment.
Let's make it generic, not MITx Online specific. Remove the MITXONLINE_ parts and anything of the sort in the docstrings. We can call it something like:
| Default settings for the MITx Online integration plugin. | |
| These are applied to both LMS and CMS configurations. | |
| """ | |
| # URL of the MITx Online webhook endpoint for course staff enrollment. | |
| settings.MITXONLINE_WEBHOOK_URL = None | |
| # API key / Bearer token for authenticating with the MITx Online webhook. | |
| settings.MITXONLINE_WEBHOOK_KEY = None | |
| # Course access roles that should trigger the MITx Online enrollment webhook. | |
| settings.MITXONLINE_COURSE_STAFF_ROLES = ["instructor", "staff"] | |
| Configuration settings for the <Name to be updated> | |
| """ | |
| # URL of the webhook endpoint for course staff enrollment. | |
| settings.ENROLLMENT_WEBHOOK_URL = None | |
| # API key / Bearer token for authenticating with the enrollment webhook. | |
| settings.ENROLLMENT_WEBHOOK_KEY = None | |
| # Course access roles that should trigger the enrollment webhook. | |
| settings.ENROLLMENT_COURSE_STAFF_ROLES = ["instructor", "staff"] |
| """Production plugin settings for the course staff webhook plugin.""" | ||
|
|
||
|
|
||
| def plugin_settings(settings): | ||
| """ | ||
| Production settings — reads values from environment/auth tokens. | ||
| """ | ||
| env_tokens = getattr(settings, "ENV_TOKENS", {}) | ||
|
|
||
| settings.MITXONLINE_WEBHOOK_URL = env_tokens.get( | ||
| "MITXONLINE_WEBHOOK_URL", settings.MITXONLINE_WEBHOOK_URL | ||
| ) | ||
| settings.MITXONLINE_WEBHOOK_KEY = env_tokens.get( | ||
| "MITXONLINE_WEBHOOK_KEY", settings.MITXONLINE_WEBHOOK_KEY | ||
| ) | ||
| settings.MITXONLINE_COURSE_STAFF_ROLES = env_tokens.get( | ||
| "MITXONLINE_COURSE_STAFF_ROLES", settings.MITXONLINE_COURSE_STAFF_ROLES | ||
| ) |
There was a problem hiding this comment.
Similar changes as common.py file.
| _RECEIVER_FUNC = "listen_for_course_access_role_added" | ||
| _SIGNAL_PATH = "openedx_events.learning.signals.COURSE_ACCESS_ROLE_ADDED" | ||
| _DISPATCH_UID = ( | ||
| "ol_openedx_course_staff_webhook.signals.listen_for_course_access_role_added" | ||
| ) | ||
|
|
||
| _SIGNAL_RECEIVER = { | ||
| PluginSignals.RECEIVER_FUNC_NAME: _RECEIVER_FUNC, | ||
| PluginSignals.SIGNAL_PATH: _SIGNAL_PATH, | ||
| PluginSignals.DISPATCH_UID: _DISPATCH_UID, | ||
| } | ||
|
|
There was a problem hiding this comment.
Don't really need these; they do not add any additional readability or encapsulation. We can use all these directly in plugin_app.
| """App configuration for the course staff webhook plugin.""" | ||
|
|
||
| name = "ol_openedx_course_staff_webhook" | ||
| verbose_name = "Course Staff Webhook" |
There was a problem hiding this comment.
Reminder: this name will change.
| "lms.djangoapp": { | ||
| PluginSignals.RELATIVE_PATH: "signals", | ||
| PluginSignals.RECEIVERS: [_SIGNAL_RECEIVER], | ||
| }, | ||
| "cms.djangoapp": { | ||
| PluginSignals.RELATIVE_PATH: "signals", | ||
| PluginSignals.RECEIVERS: [_SIGNAL_RECEIVER], | ||
| }, |
There was a problem hiding this comment.
What's the reason for registering this signal in both CMS/LMS? Is it thrown from both places? Are there paths for staff to enroll users from CMS as well?
There was a problem hiding this comment.
Yes, there is one:
- Open a course in Studio
- Settings -> Course Team
- Click the
+ New Team Memberbutton and add an email in the input field. - Click
Add Userand this should add the user in the mitxonline as well
There was a problem hiding this comment.
I think all the tests in this file can be converted into single parametrized test.
| retry_backoff=True, | ||
| retry_backoff_max=120, | ||
| ) | ||
| def notify_course_staff_addition(user_email, course_key, role): |
There was a problem hiding this comment.
| def notify_course_staff_addition(user_email, course_key, role): | |
| def notify_course_role_addition(user_email, course_key, role): |
There was a problem hiding this comment.
All the tests in this file can be converted into a single paramtertize test
| def test_sends_webhook_with_correct_payload(self, mock_post): | ||
| """POST correct payload to the configured webhook URL.""" | ||
| mock_response = mock.MagicMock() | ||
| mock_response.status_code = 200 | ||
| mock_post.return_value = mock_response | ||
|
|
||
| notify_course_staff_addition( | ||
| user_email="instructor@example.com", | ||
| course_key="course-v1:MITx+1.001x+2025_T1", | ||
| role="instructor", | ||
| ) | ||
|
|
||
| mock_post.assert_called_once_with( | ||
| WEBHOOK_URL, | ||
| json={ | ||
| "email": "instructor@example.com", | ||
| "course_id": "course-v1:MITx+1.001x+2025_T1", | ||
| "role": "instructor", | ||
| }, | ||
| headers={ | ||
| "Content-Type": "application/json", | ||
| "Authorization": f"Bearer {WEBHOOK_KEY}", | ||
| }, | ||
| timeout=30, | ||
| ) | ||
| mock_response.raise_for_status.assert_called_once() | ||
|
|
||
| @override_settings( | ||
| MITXONLINE_WEBHOOK_URL=WEBHOOK_URL, | ||
| MITXONLINE_WEBHOOK_KEY=None, | ||
| ) | ||
| @mock.patch("ol_openedx_course_staff_webhook.tasks.requests.post") | ||
| def test_sends_webhook_without_auth_when_key_is_none(self, mock_post): |
There was a problem hiding this comment.
these can be converted into a single parametrized test
|
|
||
| * Initial release. | ||
| * Listen for ``COURSE_ACCESS_ROLE_ADDED`` signal and fire a webhook | ||
| to notify an external system of course staff additions. |
There was a problem hiding this comment.
| to notify an external system of course staff additions. | |
| to notify an external system of course team additions. |
87feab9 to
fb4b11d
Compare
What are the relevant tickets?
https://github.com/mitodl/hq/issues/1209 > https://github.com/mitodl/hq/issues/10518
Description (What does it do?)
Adds a new Open edX plugin
ol_openedx_events_handlerthat fires a webhook when a course access role is added to a user in Open edX.Motivation: When an instructor or staff member is added to a course (via Studio or the LMS Instructor Dashboard), MITx Online needs to be notified so it can enroll them as an auditor in the corresponding course. This plugin automates that by listening for the
COURSE_ACCESS_ROLE_ADDEDsignal and calling a configurable webhook endpoint.How it works:
org.openedx.learning.user.course_access_role.added.v1event in both LMS and CMS viaPluginSignalsinstructorandstaff){ "email": "user@example.com", "course_id": "course-v1:MITx+1.001x+2025_T1", "role": "instructor" }Configuration:
ENROLLMENT_WEBHOOK_URLNone(disabled)ENROLLMENT_WEBHOOK_KEYNoneENROLLMENT_COURSE_ACCESS_ROLES["instructor", "staff"]Plugin structure:
apps.py— Django AppConfig with signal and settings wiringsignals.py— Receiver forCOURSE_ACCESS_ROLE_ADDED, filters by role, dispatches async tasktasks.py— Celery task that POSTs to the webhook with retry logicsettings/— Common and production settingstests/— 9 tests, 96% coverageHow can this be tested?
Start the LMS container:
Access the container and run the test script:
For manual/integration testing:
lms.yml:Additional Context
CELERY_ALWAYS_EAGER=Truecauses the webhook call to run synchronously, which adds ~8-10s to the "Add Instructor" action. In production with a real Celery worker,.delay()returns instantly and the user sees no delay.