-
Notifications
You must be signed in to change notification settings - Fork 4.3k
[BD-32] feat: add first batch of Open edX Filters #29449
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| """ | ||
| Test that various filters are fired for models in the student app. | ||
| """ | ||
| from django.test import override_settings | ||
| from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase | ||
| from xmodule.modulestore.tests.factories import CourseFactory | ||
| from openedx_filters.learning.filters import CourseEnrollmentStarted | ||
| from openedx_filters import PipelineStep | ||
|
|
||
| from common.djangoapps.student.models import CourseEnrollment, EnrollmentNotAllowed | ||
| from common.djangoapps.student.tests.factories import UserFactory, UserProfileFactory | ||
| from openedx.core.djangolib.testing.utils import skip_unless_lms | ||
|
|
||
|
|
||
| class TestEnrollmentPipelineStep(PipelineStep): | ||
| """ | ||
| Utility function used when getting steps for pipeline. | ||
| """ | ||
|
|
||
| def run_filter(self, user, course_key, mode): # pylint: disable=arguments-differ | ||
| """Pipeline steps that changes mode to honor.""" | ||
| if mode == "no-id-professional": | ||
| raise CourseEnrollmentStarted.PreventEnrollment() | ||
| return {"mode": "honor"} | ||
|
|
||
|
|
||
| @skip_unless_lms | ||
| class EnrollmentFiltersTest(ModuleStoreTestCase): | ||
| """ | ||
| Tests for the Open edX Filters associated with the enrollment process through the enroll method. | ||
|
|
||
| This class guarantees that the following filters are triggered during the user's enrollment: | ||
|
|
||
| - CourseEnrollmentStarted | ||
| """ | ||
|
|
||
| def setUp(self): # pylint: disable=arguments-differ | ||
| super().setUp() | ||
| self.course = CourseFactory.create() | ||
| self.user = UserFactory.create( | ||
| username="test", | ||
| email="test@example.com", | ||
| password="password", | ||
| ) | ||
| self.user_profile = UserProfileFactory.create(user=self.user, name="Test Example") | ||
|
|
||
| @override_settings( | ||
| OPEN_EDX_FILTERS_CONFIG={ | ||
| "org.openedx.learning.course.enrollment.started.v1": { | ||
| "pipeline": [ | ||
| "common.djangoapps.student.tests.test_filters.TestEnrollmentPipelineStep", | ||
| ], | ||
| "fail_silently": False, | ||
| }, | ||
| }, | ||
| ) | ||
| def test_enrollment_filter_executed(self): | ||
| """ | ||
| Test whether the student enrollment filter is triggered before the user's | ||
| enrollment process. | ||
|
|
||
| Expected result: | ||
| - CourseEnrollmentStarted is triggered and executes TestEnrollmentPipelineStep. | ||
| - The arguments that the receiver gets are the arguments used by the filter | ||
| with the enrollment mode changed. | ||
| """ | ||
| enrollment = CourseEnrollment.enroll(self.user, self.course.id, mode='audit') | ||
|
|
||
| self.assertEqual('honor', enrollment.mode) | ||
|
|
||
| @override_settings( | ||
| OPEN_EDX_FILTERS_CONFIG={ | ||
| "org.openedx.learning.course.enrollment.started.v1": { | ||
| "pipeline": [ | ||
| "common.djangoapps.student.tests.test_filters.TestEnrollmentPipelineStep", | ||
| ], | ||
| "fail_silently": False, | ||
| }, | ||
| }, | ||
| ) | ||
| def test_enrollment_filter_prevent_enroll(self): | ||
| """ | ||
| Test prevent the user's enrollment through a pipeline step. | ||
|
|
||
| Expected result: | ||
| - CourseEnrollmentStarted is triggered and executes TestEnrollmentPipelineStep. | ||
| - The user can't enroll. | ||
| """ | ||
| with self.assertRaises(EnrollmentNotAllowed): | ||
| CourseEnrollment.enroll(self.user, self.course.id, mode='no-id-professional') | ||
|
|
||
| @override_settings(OPEN_EDX_FILTERS_CONFIG={}) | ||
| def test_enrollment_without_filter_configuration(self): | ||
| """ | ||
| Test usual enrollment process, without filter's intervention. | ||
|
|
||
| Expected result: | ||
| - CourseEnrollmentStarted does not have any effect on the enrollment process. | ||
| - The enrollment process ends successfully. | ||
| """ | ||
| enrollment = CourseEnrollment.enroll(self.user, self.course.id, mode='audit') | ||
|
|
||
| self.assertEqual('audit', enrollment.mode) | ||
| self.assertTrue(CourseEnrollment.is_enrolled(self.user, self.course.id)) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ | |
| from edx_toggles.toggles import LegacyWaffleFlag, LegacyWaffleFlagNamespace | ||
| from openedx_events.learning.data import UserData, UserPersonalData | ||
| from openedx_events.learning.signals import STUDENT_REGISTRATION_COMPLETED | ||
| from openedx_filters.learning.filters import StudentRegistrationRequested | ||
| from pytz import UTC | ||
| from ratelimit.decorators import ratelimit | ||
| from requests import HTTPError | ||
|
|
@@ -569,6 +570,14 @@ def post(self, request): | |
| data = request.POST.copy() | ||
| self._handle_terms_of_service(data) | ||
|
|
||
| try: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a little concerned about how we maintain the contract here with
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Formally, we haven't had any discussion about this specific case. But what I think is, why don't we take a screenshot of the form_data contents as they are right now? And use that as v1. Then, if the form's contents change, we update the filter. What do you think? @ormsbee @felipemontoya
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this scenario, i.e. if you bump up the version of the filter from v1 to v2, how do those two interoperate? Do they interoperate? What I mean is, if you bump up a filter to v2, will support be immediately dropped for v1? If not, then how do you work in a mixed environment of some v1 filters and some v2 filters.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following the Open edX filters naming and versioning ADR, they must interoperate:
This is what I imagine it'd be: For each filter, a different configuration: Filters, by definition, must return what they receive (the whole data.keys), but they'll be able to use just the subset that the filter defines. I imagine this evolution happening because we decide that the filter can't modify some fields 🤔 This is just me throwing some ideas. I'm thrilled to hear what you think
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So we make the current fields a part of the filter's contract, and if edx-platform changes the field names for some reason, we do the translation at the filter level so that any pipeline steps that have been written are unaffected? That sounds good to me. The only question I'd have then is how we detect a break in the contract from edx-platform testing–i.e. if I change a field name right now, will it automatically get caught by a filter-related test breaking in CI?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with the sentiment that we should not block the PR on the issue of maintaining the contract that There is always going to be some degree of flexibility due to the optional fields which will make this very difficult if at all possible. From my perspective, the Filter at the student registration is the key piece of extension that allows any sort of registration flow to be handled. A developer could now remove or dramatically alter the form at the registration page and send any sort of info, which the filter will receive and "normalize" into something that the core platform can use to create the final user. E.g:
Filters, specially one like the filter before a student registers, are going to have some risk of breaking since we are passing in memory objects to them. The same would be true for a filter that receives a user object or enrollment object. Eventually it will have to deal with certain methods of the object changing. I think we have to embrace that and learn to write filters that are delicate in the way the handle the live data that is passed to them.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'm on the side of not enforcing the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ormsbee has any of this conversation swayed you to not fixing in place the content of form_data?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @felipemontoya: Yeah, it's not a blocker. |
||
| data = StudentRegistrationRequested.run_filter(form_data=data) | ||
| except StudentRegistrationRequested.PreventRegistration as exc: | ||
| errors = { | ||
| "error_message": [{"user_message": str(exc)}], | ||
| } | ||
| return self._create_response(request, errors, status_code=exc.status_code) | ||
|
|
||
| response = self._handle_duplicate_email_username(request, data) | ||
| if response: | ||
| return response | ||
|
|
||
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.
Does it make sense to log some of these parameters (user/course_key/mode) before running the filter? The reason I ask is that there are places where
CourseEnrollmentExceptionis caught and the message is never used or inspected. From what I can tell, the pipeline will log which step failed and echo out the exception, but that won't necessarily have the inputs that caused it to fail. I guess the main thing I want to understand is: when one of these filters does something wrong with a really unhelpful message likeNoneType is not iterable, will we have enough information in the logs to understand how to reproduce the issue?FWIW, I think that making
EnrollmentNotAlloweda subclass ofCourseEnrollmentException, and then doing this bit of except/raise here is a great idea. It maintains backwards compatibility in a lightweight and very self-contained way. Kudos.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.
You're right. Let's test those scenarios:
Case 1. The developer raises
PreventEnrollmentfrom its filter step:With this config:
When trying to login:
Case 2. The filter step fails and raises NoneType error and
fail_silentlyisFalseWhat we see first

If we scroll up we'll find:

It seems like too little and scarce information, that's why we've been working on a better way for logging this type of errors. For example:
Using this configuration:
Case 1.
INFO log level:

While DEBUG log level:

Case 2.
INFO log level:

While DEBUG log level:

I've been implementing this logging strategy for a later release in this PR: https://github.com/eduNEXT/openedx-filters/pull/17
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.
It's great to see that you folks are already tackling this issue. 😄 Thanks for pointing me to that PR!