feat: add DiscountEligibilityCheckRequested filter#335
Conversation
Adds a new DiscountEligibilityCheckRequested filter to the learning subdomain, enabling plugins to mark users as ineligible for LMS- controlled course discounts without coupling the core platform to enterprise-specific logic. ENT-11564 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| user: Any, | ||
| course_key: Any, | ||
| is_eligible: bool, | ||
| ) -> tuple: |
There was a problem hiding this comment.
make type more explicit.
pwnage101
left a comment
There was a problem hiding this comment.
Don't forget to also bump the version.
| org.openedx.learning.discount.eligibility.check.requested.v1 | ||
|
|
||
| Trigger: | ||
| - Repository: openedx/edx-platform |
There was a problem hiding this comment.
Should reference openedx/openedx-platform, not the legacy openedx/edx-platform path.
| - bool: the (possibly overridden) eligibility flag. | ||
| """ | ||
| data = super().run_pipeline(user=user, course_key=course_key, is_eligible=is_eligible) | ||
| return data.get("user"), data.get("course_key"), data.get("is_eligible") |
There was a problem hiding this comment.
Use strict dict access here: data["user"], data["course_key"], data["is_eligible"]. Per the convention established on #333, data.get(...) lets a buggy pipeline step silently drop a key — None would then propagate into the caller and falsy-evaluate as ineligible. We want a KeyError raised inside the pipeline instead.
| user: Any, | ||
| course_key: Any, | ||
| is_eligible: bool, | ||
| ) -> tuple: |
There was a problem hiding this comment.
Tighten the return annotation from bare tuple to tuple[Any, Any, bool] (or, better, concrete User / CourseKey types alongside fixing the input annotations below). Per #333, leaving this as a wide tuple allows a buggy step to return None for required values without the type checker flagging it.
There was a problem hiding this comment.
Done for CourseKey, omitting for User as the type is not referenced anywhere in the file and we would need to do something like User = get_user_model(), which I imagine we shouldn't do just to get a Type annotation.
There was a problem hiding this comment.
You actually can use AbstractBaseUser from django. See my draft PR for a different ticket: https://github.com/openedx/openedx-filters/pull/336/changes#diff-0c79edd111d62405dc0829ac0b0b971c73b410de87c0133a7190b3de08b40ed9
from django.contrib.auth.base_user import AbstractBaseUser
[...]
def run_filter(cls, user: AbstractBaseUser, course_key: CourseKey) -> tuple[AbstractBaseUser, CourseKey]:| user: Any, | ||
| course_key: Any, |
There was a problem hiding this comment.
The docstring below already names User and CourseKey or course object — lift those into the annotations (user: User, course_key: CourseKey) so input and output align and Any doesn't drift in.
| def test_run_filter_returns_false_when_pipeline_sets_ineligible(self): | ||
| user = Mock() | ||
| course_key = Mock() | ||
|
|
||
| with patch( | ||
| "openedx_filters.tooling.OpenEdxPublicFilter.run_pipeline", | ||
| return_value={"user": user, "course_key": course_key, "is_eligible": False}, | ||
| ): | ||
| returned_user, returned_course_key, is_eligible = DiscountEligibilityCheckRequested.run_filter( | ||
| user=user, course_key=course_key, is_eligible=True | ||
| ) | ||
|
|
||
| self.assertFalse(is_eligible) | ||
| self.assertIs(returned_user, user) | ||
| self.assertIs(returned_course_key, course_key) |
There was a problem hiding this comment.
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 test_schedule_requested directly above. The override-to-False case exercises pipeline-step behavior and belongs alongside the pipeline-step implementation in edx-enterprise, not here.
| def run_filter( | ||
| cls, | ||
| user: Any, | ||
| course_key: CourseKey, | ||
| is_eligible: bool, | ||
| ) -> tuple[Any, CourseKey | None, bool | None]: |
There was a problem hiding this comment.
❌ Simplify the output type to remove ambiguity---None values should not be allowed.
| def run_filter( | |
| cls, | |
| user: Any, | |
| course_key: CourseKey, | |
| is_eligible: bool, | |
| ) -> tuple[Any, CourseKey | None, bool | None]: | |
| def run_filter( | |
| cls, | |
| user: AbstractBaseUser, | |
| course_key: CourseKey, | |
| is_eligible: bool, | |
| ) -> tuple[AbstractBaseUser, CourseKey, bool]: |
There was a problem hiding this comment.
See also my comment on an older thread about AbstractBaseUser: #335 (comment)
| cls, | ||
| user: Any, | ||
| course_key: CourseKey, | ||
| is_eligible: bool, |
There was a problem hiding this comment.
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 OpenEdxFilterException exception. There are many examples in this same file, including StudentRegistrationRequested.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/changes
Notably:
class CoursewareAccessChecksRequested(OpenEdxPublicFilter):
[...]
class PreventCoursewareAccess(OpenEdxFilterException):
[...]
def __init__(
self,
error_code: str,
developer_message: str,
user_message: str,
) -> None:
super().__init__()
self.error_code = error_code
self.developer_message = developer_message
self.user_message = user_message
Adds a new DiscountEligibilityCheckRequested filter to the learning subdomain, enabling plugins to mark users as ineligible for LMS-controlled course discounts without coupling the core platform to enterprise-specific logic.
ENT-11564
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Blocks: