Skip to content

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.

Sign up for GitHub

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

Draft
pwnage101 wants to merge 8 commits into
base: main
Choose a base branch
from
+96 −1
Draft

feat: add DiscountEligibilityCheckRequested filter #335

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e5a649e
feat: add DiscountEligibilityCheckRequested filter
pwnage101 Mar 3, 2026
72cbf06
Merge branch 'main' into pwnage101/ENT-11564
marlonkeating May 22, 2026
68489dd
docs: correct comments and specify typing
marlonkeating May 22, 2026
17f623e
fix: use strict typing to throw key errors early
marlonkeating May 22, 2026
434bd19
test: passthrough behavior for DiscountEligibilityCheckRequested
marlonkeating May 22, 2026
017e33c
chore: version bump
marlonkeating May 22, 2026
6af767f
docs: use AbstractBaseUser type
marlonkeating May 22, 2026
0b26a0a
fix: skip eligibility check when already ineligible
marlonkeating May 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions openedx_filters/learning/filters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1445,3 +1445,48 @@ def run_filter(cls, schedules: QuerySet) -> QuerySet | None:
"""
data = super().run_pipeline(schedules=schedules)
return data.get("schedules")


class DiscountEligibilityCheckRequested(OpenEdxPublicFilter):
"""
Filter used to allow plugins to mark a user as ineligible for a course discount.

Purpose:
This filter is triggered during discount applicability checks, just before the
final eligibility decision is returned to the caller. Pipeline steps may set
``is_eligible`` to ``False`` to exclude a user from receiving a discount.

Filter Type:
org.openedx.learning.discount.eligibility.check.requested.v1

Trigger:
- Repository: openedx/edx-platform
Copy link
Copy Markdown
Contributor Author

@pwnage101 pwnage101 May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should reference openedx/openedx-platform, not the legacy openedx/edx-platform path.

- Path: openedx/features/discounts/applicability.py
- Function or Method: can_receive_discount, can_show_streak_discount_coupon
"""

filter_type = "org.openedx.learning.discount.eligibility.check.requested.v1"

@classmethod
def run_filter(
cls,
user: Any,
course_key: Any,
Copy link
Copy Markdown
Contributor Author

@pwnage101 pwnage101 May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

is_eligible: bool,
Copy link
Copy Markdown
Contributor Author

@pwnage101 pwnage101 May 22, 2026 •
edited
Loading

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 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

) -> tuple:
Copy link
Copy Markdown
Contributor Author

@pwnage101 pwnage101 Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make type more explicit.

Copy link
Copy Markdown
Contributor Author

@pwnage101 pwnage101 May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@marlonkeating marlonkeating May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@pwnage101 pwnage101 May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]:

"""
Process the inputs using the configured pipeline steps.

Arguments:
user (User): the Django User being checked for discount eligibility.
course_key (CourseKey or course object): identifies the course.
is_eligible (bool): the current eligibility status before plugin evaluation.

Returns:
tuple[User, CourseKey, bool]:
- User: the Django User object (unchanged).
- CourseKey: the course key (unchanged).
- 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")
Copy link
Copy Markdown
Contributor Author

@pwnage101 pwnage101 May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

29 changes: 29 additions & 0 deletions openedx_filters/learning/tests/test_filters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
CourseRunAPIRenderStarted,
CourseUnenrollmentStarted,
DashboardRenderStarted,
DiscountEligibilityCheckRequested,
IDVPageURLRequested,
InstructorDashboardRenderStarted,
ORASubmissionViewRenderStarted,
Expand Down Expand Up @@ -801,3 +802,31 @@ def test_schedule_requested(self):
result = ScheduleQuerySetRequested.run_filter(schedules)

self.assertEqual(schedules, result)


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()

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)
Comment on lines +979 to +989
Copy link
Copy Markdown
Contributor Author

@pwnage101 pwnage101 May 19, 2026

Choose a reason for hiding this comment

The 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 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.