From e1afb6befab5d34ad25edcd8c5d83c913d352929 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 19 Feb 2025 18:03:40 +0100 Subject: [PATCH] refactor: [FC-0074] address docs reviews (#260) Improve how-to docs based on reviews: - Update filter in create-new-filter to match latest standards - Add note about docstring specification - Add sample code for triggering filter - Fix sample code from create-pipeline-step - Format sample codes - Add note about annotating arguments and return --- docs/how-tos/create-a-new-filter.rst | 51 +++++++++++++++++++++---- docs/how-tos/create-a-pipeline-step.rst | 39 ++++++++++++------- openedx_filters/learning/filters.py | 3 +- 3 files changed, 72 insertions(+), 21 deletions(-) diff --git a/docs/how-tos/create-a-new-filter.rst b/docs/how-tos/create-a-new-filter.rst index 86a5f9d..8e30273 100644 --- a/docs/how-tos/create-a-new-filter.rst +++ b/docs/how-tos/create-a-new-filter.rst @@ -114,25 +114,47 @@ In our example, the filter definition could be implemented as follows: class CourseEnrollmentStarted(OpenEdxPublicFilter): """ - Custom class used to create enrollment filters and its custom methods. + Filter used to modify the enrollment process for a given user in a course. + + Purpose: + This filter is triggered when a user initiates the enrollment process, just before the enrollment is completed + allowing the filter to act on the user, course key, and mode. + + Filter Type: + org.openedx.learning.course.enrollment.started.v1 + + Trigger: + - Repository: openedx/edx-platform + - Path: common/djangoapps/student/models/course_enrollment.py + - Function or Method: enroll """ filter_type = "org.openedx.learning.course.enrollment.started.v1" class PreventEnrollment(OpenEdxFilterException): """ - Custom class used to stop the enrollment process. + Raise to prevent the enrollment process to continue. + + This exception is propagated to the course enrollment model and handled by the model to stop the enrollment + process. All components using the enroll method handle this exception in the appropriate way. """ @classmethod - def run_filter(cls, user, course_key, mode): + def run_filter(cls, user: Any, course_key: CourseKey, mode: str) -> tuple[Any, CourseKey | None, str | None]: """ - Execute a filter with the signature specified. + Process the user, course_key, and mode using the configured pipeline steps to modify the enrollment process. Arguments: - user (User): is a Django User object. + user (User): Django User enrolling in the course. course_key (CourseKey): course key associated with the enrollment. - mode (str): is a string specifying what kind of enrollment. + mode (str): specifies what kind of enrollment. The course modes available are: audit, professional, + verified, honor and professional. + + Returns: + tuple[Any, CourseKey, str]: + - User: Django User object. + - CourseKey: course key associated with the enrollment. + - str: mode of the enrollment. """ data = super().run_pipeline( user=user, course_key=course_key, mode=mode, @@ -144,6 +166,8 @@ In our example, the filter definition could be implemented as follows: - The ``PreventEnrollment`` class is a custom exception that is raised when the filter should halt the application behavior. - The ``run_filter`` method is the main method of the filter that is called when the filter is triggered. The method should call the |OpenEdxPublicFilter.run_pipeline| method, passing down the input arguments and returning the final output of the filter. - Use arguments names that are consistent with the triggering logic to avoid confusion and improve readability. +- The docstrings should provide context on the purpose of the filter, the filter type, the triggering logic, and the arguments and return types of the ``run_filter`` method. See :doc:`../reference/documenting-filters-classes` for more details on how to document these definitions. +- Annotate the arguments and return types of the ``run_filter`` method to provide clarity and safety. .. note:: Implement exceptions that are related to the filter behavior and specify how the filter should modify the application behavior with each exception. The caller should handle each exception differently based on the exception's purpose. For example, the caller should halt the application behavior when the ``PreventEnrollment`` exception is raised. @@ -154,7 +178,20 @@ After implementing the filter definition, you should trigger the filter in the a In our example, we identified that the triggering logic is the ``enroll`` method in the enrollment model in the LMS. Therefore, we should trigger the filter in the ``enroll`` method, passing down the user, course key, and mode arguments to the filter. The filter should be placed so that it is triggered before the enrollment process is completed, which can alter the enrollment process if the user does not meet the eligibility criteria. -.. note:: Try placing the filter so it can be triggered before the process is completed, so it can alter the process if needed. In some cases, this would be at the beginning of the process, while in others, it would be elsewhere. +Here is an example of how the filter could be triggered in the ``enroll`` method: + +.. code-block:: python + + try: + # .. filter_implemented_name: CourseEnrollmentStarted + # .. filter_type: org.openedx.learning.course.enrollment.started.v1 + user, course_key, mode = CourseEnrollmentStarted.run_filter( + user=user, course_key=course_key, mode=mode, + ) + except CourseEnrollmentStarted.PreventEnrollment as exc: + raise EnrollmentNotAllowed(str(exc)) from exc + +.. note:: Try placing the filter so it can be triggered before the process is completed, so it can alter the process if needed. In some cases, this would be at the beginning of the process, while in others it would be elsewhere. Step 7: Implement Your Pipeline Steps ======================================== diff --git a/docs/how-tos/create-a-pipeline-step.rst b/docs/how-tos/create-a-pipeline-step.rst index 43edb7a..146592e 100644 --- a/docs/how-tos/create-a-pipeline-step.rst +++ b/docs/how-tos/create-a-pipeline-step.rst @@ -56,17 +56,23 @@ In our example, the pipeline step could look like this: from openedx_filters.filters import PipelineStep - # Location my_plugin/pipeline.py - class CheckValidEmailPipelineStep(PipelineStep): - def run_filter(self, user, course_key, mode): - if self.not is_user_email_allowed(user.email): - log.debug("User %s does not have a valid email address, stopping enrollment", user.email) - raise CourseEnrollmentStarted.PreventEnrollment("User does not have a valid email address") - log.debug("User has a valid email address, allowing enrollment") - return { - "user": user, - "course_key": course_key, - "mode": mode, + # Location my_plugin/pipeline.py + class CheckValidEmailPipelineStep(PipelineStep): + def run_filter(self, user, course_key, mode): + if not is_user_email_allowed(user.email): + log.debug( + "User %s does not have a valid email address, stopping enrollment", + user.email, + ) + raise CourseEnrollmentStarted.PreventEnrollment( + "User does not have a valid email address" + ) + + log.debug("User has a valid email address, allowing enrollment") + return { + "user": user, + "course_key": course_key, + "mode": mode, } - In this example, we create a new class called ``CheckValidEmailPipelineStep`` that inherits from the base class |PipelineStep|. @@ -109,6 +115,11 @@ In our example, you could write a unit test for the pipeline step like this: .. code-block:: python + from django.test import override_settings + from my_plugin.test_utils.factories import UserFactory + + from openedx_filters.filters import CourseEnrollmentStarted + # Location my_plugin/tests/test_pipeline.py @override_settings( OPEN_EDX_FILTERS_CONFIG={ @@ -116,7 +127,7 @@ In our example, you could write a unit test for the pipeline step like this: "fail_silently": False, "pipeline": [ "my_plugin.pipeline.CheckValidEmailPipelineStep", - ] + ], } } ) @@ -124,7 +135,9 @@ In our example, you could write a unit test for the pipeline step like this: user = UserFactory(email="invalid_email") with self.assertRaises(CourseEnrollmentStarted.PreventEnrollment): CourseEnrollmentStarted.run_filter( - user=user, course_key=self.course_key, mode="audit", + user=user, + course_key=self.course_key, + mode="audit", ) Step 6: Debug and Iterate diff --git a/openedx_filters/learning/filters.py b/openedx_filters/learning/filters.py index da4ea34..6db44fd 100644 --- a/openedx_filters/learning/filters.py +++ b/openedx_filters/learning/filters.py @@ -283,7 +283,8 @@ def run_filter(cls, user: Any, course_key: CourseKey, mode: str) -> tuple[Any, C Arguments: user (User): Django User enrolling in the course. course_key (CourseKey): course key associated with the enrollment. - mode (str): specifies what kind of enrollment. + mode (str): specifies what kind of enrollment. The course modes available are: audit, professional, + verified, honor and professional Returns: tuple[Any, CourseKey, str]: