feat: add filters to support logistration integration - #337
Conversation
0fa2f77 to
8651fc4
Compare
5ebe0d6 to
c0c46d4
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the openedx_filters.learning public filter surface to support “logistration” (combined login/registration) and related auth flows, aligning the filters library with new integration points in edx-platform/enterprise.
Changes:
- Added six new learning filters for logistration context/response customization, MFE redirect control, post-login redirect override, and TPA form overrides.
- Added two structural
Protocoltypes (FormDescriptionProtocol,ProviderConfigProtocol) to document the minimal interface passed to pipeline steps. - Added/updated test coverage for the new filters, and bumped package version + changelog entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| openedx_filters/learning/filters.py | Adds new logistration/auth-related filters and protocol structural types. |
| openedx_filters/learning/tests/test_filters.py | Adds unit tests validating new filters’ filter_type values and passthrough/exception behavior. |
| openedx_filters/init.py | Bumps package version to 3.9.0. |
| CHANGELOG.rst | Adds a 3.9.0 changelog entry describing the new filters and protocols. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0c4ee36 to
2f95918
Compare
57a747a to
6d579f6
Compare
iloveagent57
left a comment
There was a problem hiding this comment.
Looks good, one small suggestion to DRY things up a tiny bit.
* Added new ``LogistrationContextRequested`` filter * Added new ``PostLoginRedirectURLRequested`` filter * Added new ``LoginFormTPAOverridesRequested`` filter * Added new ``RegistrationFormTPAOverridesRequested`` filter * Added new ``LogistrationResponseRendered`` filter * Added ``FormDescriptionProtocol`` and ``ProviderConfigProtocol`` structural types declaring the minimal surface the TPA-override filters pass to pipeline steps ENT-11568
6d579f6 to
5602d20
Compare
|
@felipemontoya or @feanil could use one of your eyes on this big one. The full ADR may be the best way to catch up, and there's a diagram in there too: https://github.com/openedx/edx-enterprise/blob/abd13d1be582e5c51f57373608d67032c96d4056/docs/decisions/0016-logistration-filters.md |
| return data["user"], data["course_mode_data"], data["price"] | ||
|
|
||
|
|
||
| class LogistrationContextRequested(OpenEdxPublicFilter): |
There was a problem hiding this comment.
Note to self: this should probably have "Legacy" in the class name to emphasize that it does not apply to the AuthN MFE logistration page.
There was a problem hiding this comment.
One thing that I always thought filters could help us solve is the difference between legacy and new mechanisms. Not only scope to MFE or auth, but in general.
Would it be possible to reuse this filter in the same place where the logistration MFE context is being calculated?
There was a problem hiding this comment.
Researched a bit and it looks possible:
MFEContextView.get (user_authn/api/views.py:50) builds a context dict via get_mfe_context(request, redirect_to, third_party_auth_hint) at :73 — structurally the same shape as login_and_registration_form's context. So it's one filter with two trigger points, and documenting-filters-classes.rst explicitly supports listing multiple triggers in the docstring. That also answers the author's own note-to-self at filters.py:1941 ("should probably have Legacy in the class name") — if the filter covers both, it shouldn't.
There was a problem hiding this comment.
Interesting, sounds promising.
felipemontoya
left a comment
There was a problem hiding this comment.
I think this work is good and overall very needed in the login/registration extensibility story. Thanks for putting this together @pwnage101. Now that I understand the context, new review should be quicker.
| return data["user"], data["course_mode_data"], data["price"] | ||
|
|
||
|
|
||
| class LogistrationContextRequested(OpenEdxPublicFilter): |
There was a problem hiding this comment.
One thing that I always thought filters could help us solve is the difference between legacy and new mechanisms. Not only scope to MFE or auth, but in general.
Would it be possible to reuse this filter in the same place where the logistration MFE context is being calculated?
| - Function or Method: login_user | ||
| """ | ||
|
|
||
| filter_type = "org.openedx.learning.auth.post_login.redirect_url.requested.v1" |
There was a problem hiding this comment.
This will be the only filter type that has auth in it.
To keep it more in line with the existing filter I would call it something like this:
"org.openedx.learning.student.login.alt_redirect_url.requested.v1"
And updating the class to match LoginAltRedirectURLRequested. I'm calling it Alt for Alternative since any step can do it, it might not be necessarily be at the end or post-login.
| return data["redirect_url"], data["user"], data["next_url"] | ||
|
|
||
|
|
||
| class FormDescriptionProtocol(Protocol): |
There was a problem hiding this comment.
This is solid work, but does not belong to the filters.py file.
| ... # pylint: disable=unnecessary-ellipsis | ||
|
|
||
|
|
||
| class ProviderConfigProtocol(Protocol): |
There was a problem hiding this comment.
also does not belong in this file. We could move them both into a protocols.py right next to filters.py.
| - Function or Method: login_and_registration_form | ||
| """ | ||
|
|
||
| filter_type = "org.openedx.learning.logistration.response.rendered.v1" |
There was a problem hiding this comment.
To keep things consistent with other filters and because the action there is not rendered yet, we have used completed in the past.
how about:
"org.openedx.learning.logistration.render.completed.v1" with "LogistrationRenderCompleted"
| Filter used to apply third-party-auth overrides to the login form description. | ||
|
|
||
| Purpose: | ||
| This filter is only triggered when third-party auth is enabled and an auth pipeline |
There was a problem hiding this comment.
Is it necessary that we only trigger this for third party auth and never for non third party auth? I think we could make the case here and already make this override capability functional for the general case. Enterprise/TPA don't lose anything, but any other extension cases gain a lot.
I researched a bit with claude to see how much of a detour that would be.
Result:
Each form description has exactly one construction point:
- get_login_session_form(request) — login_form.py:77
- RegistrationFormFactory.get_registration_form(request) — registration_form.py:567
and each is called from exactly two places: the public API endpoint (login.py:756, register.py:557) and the logistration page (login_form.py:302-303). One line in each function covers 100% of form builds, including every case the TPA-scoped version covers, for the same one-line modification count.
We know the need is real: PROFILE_EXTENSION_FORM (registration_form.py:337, with REGISTRATION_EXTENSION_FORM deprecated at :342) exists precisely so operators can extend the registration form. It's settings-based, requires shipping a Django Form class, and has no login-side equivalent. A general filter is the natural modern successor to that mechanism. A TPA-scoped misses the opportunity, which means a second, overlapping filter at the same code point later.
| return data["user"], data["course_mode_data"], data["price"] | ||
|
|
||
|
|
||
| class LogistrationContextRequested(OpenEdxPublicFilter): |
There was a problem hiding this comment.
Researched a bit and it looks possible:
MFEContextView.get (user_authn/api/views.py:50) builds a context dict via get_mfe_context(request, redirect_to, third_party_auth_hint) at :73 — structurally the same shape as login_and_registration_form's context. So it's one filter with two trigger points, and documenting-filters-classes.rst explicitly supports listing multiple triggers in the docstring. That also answers the author's own note-to-self at filters.py:1941 ("should probably have Legacy in the class name") — if the filter covers both, it shouldn't.
LogistrationContextRequestedfilterPostLoginRedirectURLRequestedfilterLoginFormTPAOverridesRequestedfilterRegistrationFormTPAOverridesRequestedfilterLogistrationResponseRenderedfilterFormDescriptionProtocolandProviderConfigProtocolstructural types declaring the minimal surface the TPA-override filters pass to pipeline stepsENT-11568
Integration Testing
See openedx/edx-enterprise#2551 (comment)
Related: