-
Notifications
You must be signed in to change notification settings - Fork 20
feat: add 1st batch of filters #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
openedx_filters/learning/auth.py
Outdated
| @@ -0,0 +1,54 @@ | |||
| """ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we move all filters to learning/filters.py file? @felipemontoya to be compliant with openedx-events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have indeed most signal definitions for events at learning/signals.py, which suggests that we should also move to learning/filters.py.
If we don't, then we will have a filter:
"org.openedx.learning.student.registration.requested.v1"
defined at openedx_filters.learning.auth.PreRegisterFilter.
If we do, then we are looking at:
"org.openedx.learning.student.registration.requested.v1"
defined at openedx_filters.learning.filters.PreRegisterFilter.
which is a little more consistent with from openedx_events.learning.signals import SESSION_LOGIN_COMPLETED which is a live example from the events implementation.
0c76fc0 to
1a2df09
Compare
openedx_filters/learning/auth.py
Outdated
| @@ -0,0 +1,54 @@ | |||
| """ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have indeed most signal definitions for events at learning/signals.py, which suggests that we should also move to learning/filters.py.
If we don't, then we will have a filter:
"org.openedx.learning.student.registration.requested.v1"
defined at openedx_filters.learning.auth.PreRegisterFilter.
If we do, then we are looking at:
"org.openedx.learning.student.registration.requested.v1"
defined at openedx_filters.learning.filters.PreRegisterFilter.
which is a little more consistent with from openedx_events.learning.signals import SESSION_LOGIN_COMPLETED which is a live example from the events implementation.
openedx_filters/learning/auth.py
Outdated
| from openedx_filters.tooling import OpenEdxPublicFilter | ||
|
|
||
|
|
||
| class PreRegisterFilter(OpenEdxPublicFilter): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I find the names "PreRegisterFilter" or "PreLoginFilter" very easy to understand, I think that we set an important precedent at the events library. There we have:
COURSE_ENROLLMENT_CREATED:
event_type="org.openedx.learning.course.enrollment.created.v1"
STUDENT_REGISTRATION_COMPLETED:
event_type="org.openedx.learning.student.registration.completed.v1"
COURSE_UNENROLLMENT_COMPLETED
event_type="org.openedx.learning.course.unenrollment.completed.v1"While in this PR we are proposing:
PreRegisterFilter:
filter_type = "org.openedx.learning.student.registration.requested.v1"We could change this so that the name matches a little better the filter_type.
For example we could use any of the following as the class names:
- StudentRegistrationRequested
- StudentRegistrationStarted
- StudentRegistrationFilter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably also affects the naming of the PreEnrollmentFilter and the implementing PR as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion. I'll go with the StudentRegistrationRequested because it's the closest to the events naming convention.
openedx_filters/learning/auth.py
Outdated
|
|
||
| filter_type = "org.openedx.learning.student.registration.requested.v1" | ||
| sensitive_form_data = [ | ||
| "password", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is password enough? should we be censoring all the fields at https://github.com/edx/edx-platform/blob/master/common/djangoapps/track/middleware.py#L64
felipemontoya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking almost ready to me now. I left a comment about the name of the exception but once that is in, I''m good with merging.
bb002c4 to
8fdf3c2
Compare
* Add PreRegisterFilter * Add PreLoginFilter
8fdf3c2 to
4470dc9
Compare
Description:
This PR adds:
Used by edx-platform.
Testing instructions:
These sample filter steps are implemented in this plugin
With this configuration, you won't be able to register, login, or enroll in a course. Another configuration you could try is, instead of
Stop<process>useNoopFilterReviewers:
Merge checklist:
Post merge:
finished.