From 4e263b3b8eaf7d64f17eebd3273139362906cc80 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 20 Dec 2024 13:33:48 +0100 Subject: [PATCH 1/8] docs: add suggestion about the design of filters --- .../0007-filter-design-practices.rst | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 docs/decisions/0007-filter-design-practices.rst diff --git a/docs/decisions/0007-filter-design-practices.rst b/docs/decisions/0007-filter-design-practices.rst new file mode 100644 index 00000000..20346cad --- /dev/null +++ b/docs/decisions/0007-filter-design-practices.rst @@ -0,0 +1,52 @@ +7. Filters Design Practices +########################### + +Status +------ + +Draft + +Context +------- + +It is important to follow standards to ensure that the filters are designed in a way that is easy to understand and maintain. This ADR aims to provide guidelines for designing Open edX Filters with long term maintainability in mind. + +Decision +-------- + +These are the practices that we recommend reviewing and following when designing Open edX Filters and contributing them to the library: + +Design Clarity and Understanding +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +- A filter should describe as accurately as possible what behavior intends to modify. +- A filter should have a clear and concise name that describes its purpose. +- Ensure that the triggering logic of the filter is consistent and narrow. It should only trigger when the conditions are met and cover all the cases that the filter should be applied with the minimum number of modifications. + +Contextual Information +~~~~~~~~~~~~~~~~~~~~~~ + +- A filter should provide enough context in its arguments about the process that is modifying to be able to modify the intended behavior. +- A filter should provide enough context in its arguments to avoid runtime dependencies with the application. +- The arguments of the filter should be directly related to the responsibility of the filter. + +Flexibility and Customization +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +- A filter should be flexible enough to allow the developer to customize the behavior of the application. + +Error Handling +~~~~~~~~~~~~~~ + +- Ensure that the exceptions when using the filter are handled properly and that the filter does not break the application. +- The filter exceptions should correspond with the filter behavior. If the filter is not supposed to halt the application behavior in any case, then do not specify exceptions. If the filter is supposed to halt the application behavior in some cases, then specify the exceptions that the filter can raise with enough reason to understand why the filter is halting the application behavior. + +Type Safety +~~~~~~~~~~~ + +- A filter should annotate the type of the arguments that it receives. + +Consequences +------------ + +Following these practices will ensure that the filters are designed in a way that is easy to understand and maintain. Having these standards in place will also make the decision process easier when designing new filters. \ No newline at end of file From e8816cd0fae2e8b0e02ae8ed543eee4c66598570 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 20 Dec 2024 14:21:17 +0100 Subject: [PATCH 2/8] docs: add ADR in the decisions index --- docs/decisions/0007-filter-design-practices.rst | 2 +- docs/decisions/index.rst | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/decisions/0007-filter-design-practices.rst b/docs/decisions/0007-filter-design-practices.rst index 20346cad..8272382a 100644 --- a/docs/decisions/0007-filter-design-practices.rst +++ b/docs/decisions/0007-filter-design-practices.rst @@ -49,4 +49,4 @@ Type Safety Consequences ------------ -Following these practices will ensure that the filters are designed in a way that is easy to understand and maintain. Having these standards in place will also make the decision process easier when designing new filters. \ No newline at end of file +Following these practices will ensure that the filters are designed in a way that is easy to understand and maintain. Having these standards in place will also make the decision process easier when designing new filters. diff --git a/docs/decisions/index.rst b/docs/decisions/index.rst index 2bd7356b..4aa0e37b 100644 --- a/docs/decisions/index.rst +++ b/docs/decisions/index.rst @@ -11,3 +11,4 @@ Decisions 0004-filters-naming-and-versioning 0005-filters-payload 0006-filter-debug-tooling + 0007-filter-design-practices From d535b8fd0a20c02a94c4b2741c9e1475a8e227ff Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 30 Dec 2024 14:08:28 +0100 Subject: [PATCH 3/8] docs: address PR reviews --- .../0007-filter-design-practices.rst | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/docs/decisions/0007-filter-design-practices.rst b/docs/decisions/0007-filter-design-practices.rst index 8272382a..1b597b5d 100644 --- a/docs/decisions/0007-filter-design-practices.rst +++ b/docs/decisions/0007-filter-design-practices.rst @@ -23,6 +23,24 @@ Design Clarity and Understanding - A filter should have a clear and concise name that describes its purpose. - Ensure that the triggering logic of the filter is consistent and narrow. It should only trigger when the conditions are met and cover all the cases that the filter should be applied with the minimum number of modifications. +For instance, consider this filter that is supposed to modify the behavior of the certificate creation process: + +.. code-block:: python + + class CertificateCreationRequested(OpenEdxPublicFilter): + + class PreventCertificateCreation(OpenEdxFilterException): + """ + Custom class used to stop the certificate creation process. + """ + + @classmethod + def run_filter(cls, user, course_key, mode, status, grade, generation_mode): + +Where the filter name indicates that it is a filter that is triggered when the certificate creation is requested, providing a clear understanding of the filter behavior. Avoid using generic names such as ``Filter`` or ``Process`` that do not provide any context about the filter behavior. + +The ``CertificateCreationRequested`` is triggered by `_generate_certificate_task`_ which handles all the cases that the filter should be applied avoiding unnecessary modifications. + Contextual Information ~~~~~~~~~~~~~~~~~~~~~~ @@ -30,6 +48,8 @@ Contextual Information - A filter should provide enough context in its arguments to avoid runtime dependencies with the application. - The arguments of the filter should be directly related to the responsibility of the filter. +Consider the user login filter example above. The filter provides the necessary context to understand the behavior that is being modified such as the user, course key, mode, status, grade, and generation mode. The arguments are directly related to the responsibility of the filter. + Flexibility and Customization ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -41,6 +61,8 @@ Error Handling - Ensure that the exceptions when using the filter are handled properly and that the filter does not break the application. - The filter exceptions should correspond with the filter behavior. If the filter is not supposed to halt the application behavior in any case, then do not specify exceptions. If the filter is supposed to halt the application behavior in some cases, then specify the exceptions that the filter can raise with enough reason to understand why the filter is halting the application behavior. +Consider the certificate creation filter example above. The filter specifies an exception that is raised when the certificate creation process is stopped, providing a clear understanding of the filter behavior when the exception is raised. This exception should be handled properly in the application to avoid runtime errors. + Type Safety ~~~~~~~~~~~ From b5ceb2c77af8df8e300aa5ce39f39c8bd8573f67 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 30 Dec 2024 15:05:43 +0100 Subject: [PATCH 4/8] docs: add reference to _generate_certificate_task --- docs/decisions/0007-filter-design-practices.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/decisions/0007-filter-design-practices.rst b/docs/decisions/0007-filter-design-practices.rst index 1b597b5d..b6a22c83 100644 --- a/docs/decisions/0007-filter-design-practices.rst +++ b/docs/decisions/0007-filter-design-practices.rst @@ -72,3 +72,5 @@ Consequences ------------ Following these practices will ensure that the filters are designed in a way that is easy to understand and maintain. Having these standards in place will also make the decision process easier when designing new filters. + +.. _`_generate_certificate_task`: https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/certificates/generation_handler.py#L116-L128 From 6682914178d044481857d2ec40e98cf1619a2154 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 8 Jan 2025 16:34:21 +0100 Subject: [PATCH 5/8] docs: move example to ADR intro --- .../0007-filter-design-practices.rst | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/docs/decisions/0007-filter-design-practices.rst b/docs/decisions/0007-filter-design-practices.rst index b6a22c83..a01d1acf 100644 --- a/docs/decisions/0007-filter-design-practices.rst +++ b/docs/decisions/0007-filter-design-practices.rst @@ -14,16 +14,7 @@ It is important to follow standards to ensure that the filters are designed in a Decision -------- -These are the practices that we recommend reviewing and following when designing Open edX Filters and contributing them to the library: - -Design Clarity and Understanding -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -- A filter should describe as accurately as possible what behavior intends to modify. -- A filter should have a clear and concise name that describes its purpose. -- Ensure that the triggering logic of the filter is consistent and narrow. It should only trigger when the conditions are met and cover all the cases that the filter should be applied with the minimum number of modifications. - -For instance, consider this filter that is supposed to modify the behavior of the certificate creation process: +These are the practices that we recommend reviewing and following when designing Open edX Filters and contributing them to the library. Consider this example of a filter that is triggered when a user requests a certificate, we will use this example to illustrate the practices: .. code-block:: python @@ -37,7 +28,14 @@ For instance, consider this filter that is supposed to modify the behavior of th @classmethod def run_filter(cls, user, course_key, mode, status, grade, generation_mode): -Where the filter name indicates that it is a filter that is triggered when the certificate creation is requested, providing a clear understanding of the filter behavior. Avoid using generic names such as ``Filter`` or ``Process`` that do not provide any context about the filter behavior. +Design Clarity and Understanding +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +- A filter should describe as accurately as possible what behavior intends to modify. +- A filter should have a clear and concise name that describes its purpose. +- Ensure that the triggering logic of the filter is consistent and narrow. It should only trigger when the conditions are met and cover all the cases that the filter should be applied with the minimum number of modifications. + +Consider the example above, the filter name is ``CertificateCreationRequested`` which indicates that it is a filter that is triggered when the certificate creation is requested, providing a clear understanding of the filter behavior. Avoid using generic names such as ``Filter`` or ``Process`` that do not provide any context about the filter behavior. The ``CertificateCreationRequested`` is triggered by `_generate_certificate_task`_ which handles all the cases that the filter should be applied avoiding unnecessary modifications. @@ -48,7 +46,7 @@ Contextual Information - A filter should provide enough context in its arguments to avoid runtime dependencies with the application. - The arguments of the filter should be directly related to the responsibility of the filter. -Consider the user login filter example above. The filter provides the necessary context to understand the behavior that is being modified such as the user, course key, mode, status, grade, and generation mode. The arguments are directly related to the responsibility of the filter. +Consider the filter example above, the filter provides the necessary context to understand the behavior that is being modified such as the user, course key, mode, status, grade, and generation mode. The arguments are directly related to the responsibility of the filter. Flexibility and Customization ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -61,12 +59,12 @@ Error Handling - Ensure that the exceptions when using the filter are handled properly and that the filter does not break the application. - The filter exceptions should correspond with the filter behavior. If the filter is not supposed to halt the application behavior in any case, then do not specify exceptions. If the filter is supposed to halt the application behavior in some cases, then specify the exceptions that the filter can raise with enough reason to understand why the filter is halting the application behavior. -Consider the certificate creation filter example above. The filter specifies an exception that is raised when the certificate creation process is stopped, providing a clear understanding of the filter behavior when the exception is raised. This exception should be handled properly in the application to avoid runtime errors. +Consider the example above, the filter specifies an exception that is raised when the certificate creation process is stopped, providing a clear understanding of the filter behavior when the exception is raised. This exception should be handled properly in the application to avoid runtime errors. Type Safety ~~~~~~~~~~~ -- A filter should annotate the type of the arguments that it receives. +- A filter should annotate the type of the arguments that it receives, only when it doesn't create unnecessary dependencies with the application. Consequences ------------ From c6659a60f4346f324c27c95af37d8fd508219dd5 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 13 Jan 2025 15:26:27 +0100 Subject: [PATCH 6/8] docs: add note about application behavior when handling filter errors --- docs/decisions/0007-filter-design-practices.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/decisions/0007-filter-design-practices.rst b/docs/decisions/0007-filter-design-practices.rst index a01d1acf..5d86bbfa 100644 --- a/docs/decisions/0007-filter-design-practices.rst +++ b/docs/decisions/0007-filter-design-practices.rst @@ -61,6 +61,8 @@ Error Handling Consider the example above, the filter specifies an exception that is raised when the certificate creation process is stopped, providing a clear understanding of the filter behavior when the exception is raised. This exception should be handled properly in the application to avoid runtime errors. +.. note:: The application should catch the exception and handle it properly by returning an error message to the user through API responses, UI messages, or logs depending on the application context and where the filter is being used. + Type Safety ~~~~~~~~~~~ From 8733c9ad47d4de0e4e9698715cb25aef64f1ac70 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 13 Jan 2025 15:26:54 +0100 Subject: [PATCH 7/8] docs: apply suggestions from code review Co-authored-by: Felipe Montoya --- docs/decisions/0007-filter-design-practices.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/decisions/0007-filter-design-practices.rst b/docs/decisions/0007-filter-design-practices.rst index 5d86bbfa..f711f95f 100644 --- a/docs/decisions/0007-filter-design-practices.rst +++ b/docs/decisions/0007-filter-design-practices.rst @@ -71,6 +71,6 @@ Type Safety Consequences ------------ -Following these practices will ensure that the filters are designed in a way that is easy to understand and maintain. Having these standards in place will also make the decision process easier when designing new filters. +Following these practices will ensure that the filters are designed in a way that is easy to understand and maintain. Having these standards in place will also make the decision process easier when designing new filters. .. _`_generate_certificate_task`: https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/certificates/generation_handler.py#L116-L128 From 4e112eae79d6378ae4eefd428c19c275e3df0d4d Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 13 Jan 2025 16:57:42 +0100 Subject: [PATCH 8/8] docs: reference PR in note --- docs/decisions/0007-filter-design-practices.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/decisions/0007-filter-design-practices.rst b/docs/decisions/0007-filter-design-practices.rst index f711f95f..6f312972 100644 --- a/docs/decisions/0007-filter-design-practices.rst +++ b/docs/decisions/0007-filter-design-practices.rst @@ -61,7 +61,7 @@ Error Handling Consider the example above, the filter specifies an exception that is raised when the certificate creation process is stopped, providing a clear understanding of the filter behavior when the exception is raised. This exception should be handled properly in the application to avoid runtime errors. -.. note:: The application should catch the exception and handle it properly by returning an error message to the user through API responses, UI messages, or logs depending on the application context and where the filter is being used. +.. note:: The application should catch the exception and handle it properly by returning an error message to the user through API responses, UI messages, or logs depending on the application context and where the filter is being used. For reference, see how the exception is handled in the `Pull Request edx-platform#35407`_. Type Safety ~~~~~~~~~~~ @@ -74,3 +74,4 @@ Consequences Following these practices will ensure that the filters are designed in a way that is easy to understand and maintain. Having these standards in place will also make the decision process easier when designing new filters. .. _`_generate_certificate_task`: https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/certificates/generation_handler.py#L116-L128 +.. _`Pull Request edx-platform#35407`: https://github.com/openedx/edx-platform/pull/35407