-
Notifications
You must be signed in to change notification settings - Fork 26
feat: add DiscountEligibilityCheckRequested filter #335
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
base: main
Are you sure you want to change the base?
Changes from all commits
e5a649e
72cbf06
68489dd
17f623e
434bd19
017e33c
6af767f
0b26a0a
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 |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
| CourseRunAPIRenderStarted, | ||
| CourseUnenrollmentStarted, | ||
| DashboardRenderStarted, | ||
| DiscountEligibilityCheckRequested, | ||
| GradeEventContextRequested, | ||
| IDVPageURLRequested, | ||
| InstructorDashboardRenderStarted, | ||
|
|
@@ -962,3 +963,41 @@ def test_prevent_tabs_generation_exception(self, exception_class, attributes): | |
| exception = exception_class(**attributes) | ||
|
|
||
| self.assertLessEqual(attributes.items(), exception.__dict__.items()) | ||
|
|
||
|
|
||
| class TestDiscountEligibilityCheckRequestedFilter(TestCase): | ||
| """ | ||
| Tests for the DiscountEligibilityCheckRequested filter. | ||
| """ | ||
|
|
||
| def test_filter_type(self): | ||
| self.assertEqual( | ||
| DiscountEligibilityCheckRequested.filter_type, | ||
| "org.openedx.learning.discount.eligibility.check.requested.v1", | ||
| ) | ||
|
|
||
| def test_run_filter_returns_false_when_pipeline_sets_ineligible(self): | ||
| user = Mock() | ||
| course_key = Mock() | ||
|
|
||
| returned_user, returned_course_key, is_eligible = ( | ||
| DiscountEligibilityCheckRequested.run_filter(user, course_key, True) | ||
| ) | ||
|
|
||
| self.assertTrue(is_eligible) | ||
| self.assertIs(returned_user, user) | ||
| self.assertIs(returned_course_key, course_key) | ||
|
|
||
| def test_run_filter_skips_pipeline_when_already_ineligible(self): | ||
| user = Mock() | ||
| course_key = Mock() | ||
|
|
||
| with patch("openedx_filters.tooling.OpenEdxPublicFilter.run_pipeline") as mock_run_pipeline: | ||
| returned_user, returned_course_key, is_eligible = ( | ||
| DiscountEligibilityCheckRequested.run_filter(user, course_key, False) | ||
| ) | ||
|
|
||
| mock_run_pipeline.assert_not_called() | ||
| self.assertFalse(is_eligible) | ||
| self.assertIs(returned_user, user) | ||
| self.assertIs(returned_course_key, course_key) | ||
|
Comment on lines
+979
to
+989
Contributor
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. Per the minimal-test convention from #334, the canonical test for a new filter in this repo verifies inputs pass through unchanged when no pipeline is configured — mirror |
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Nit: since it only takes one pipeline step to deem the entire discount ineligible, every pipeline step implementation must take care to skip itself when
is_eligible=False to prevent clobbering a prior step which set it to False. This pushes complexity into 3rd party code when there's really a much simpler solution:We could make pipeline steps simply raise an
OpenEdxFilterExceptionexception. There are many examples in this same file, includingStudentRegistrationRequested.PreventRegistration:https://github.com/edx/openedx-filters/blob/main/openedx_filters/learning/filters.py#L161
This has the side-benefit of providing a clean way to pass error messages from the caller without polluting the
run_filter()signature. See my in-progress work for a different ticket: https://github.com/openedx/openedx-filters/pull/336/changesNotably: