-
Notifications
You must be signed in to change notification settings - Fork 17
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
docs: [FC-0074] add docs about creating new filters with pipeline steps #242
base: main
Are you sure you want to change the base?
Conversation
Thanks for the pull request, @mariajgrimaldi! This repository is currently maintained by @openedx/hooks-extension-framework. Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.
|
|
||
This will mainly make the filters available for your CI/CD pipeline and local development environment. If you are using the Open edX platform, the library should be already be installed in the environment so no need to install it. | ||
|
||
Step 4: Create a Pipeline Step |
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.
This goes from step 2 to step 4!
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.
I totally missed this. Thank you!
- Limit each step to a single responsibility to make the code easier to maintain and test. | ||
- Keep the pipeline step logic simple and focused on the specific task it needs to perform. | ||
- Consider the performance implications of the pipeline step and avoid adding unnecessary complexity or overhead, considering the pipeline will be executed each time the filter is triggered. | ||
- Implement error handling and logging in the pipeline step to handle exceptions and provide useful information for debugging, considering both development and production environments. |
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.
Does it make sense to include an example of logging or verbose error messaging in your simple example above?
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.
I added some. This suggestion mainly focuses on helping developers understand when the filter is doing what it's supposed to do based on the given conditions. Do you think the example illustrates that?
+-------------------+----------------------------------------------------------------------------------------------------+ | ||
| Learning | Allows learners to consume content and perform actions in a learning activity on the platform. | | ||
+-------------------+----------------------------------------------------------------------------------------------------+ | ||
| Analytics | Provides insights into learner behavior and course performance. | |
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.
| Analytics | Provides insights into learner behavior and course performance. | | |
| Analytics | A look into learner behavior and course performance. | |
I don't love this but I think you should avoid using "insights" to not confuse with Insights, the deprecated analytics product.
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.
I used "visbility" instead of insights, what do you think?
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.
I have one suggestion, but the rest looks good to me.
Thanks @mariajgrimaldi ✨
- You understand the concept of filters or have reviewed the relevant :doc:`/concepts/index` docs. | ||
- You are familiar with the terminology used in the project, such as the terms :term:`Filter Type`. If not, you can review the :doc:`../reference/glossary` docs. | ||
- You have reviewed the :doc:`../decisions/0007-filter-design-practices` ADR. | ||
- You have identified that you need to create a new filter and have a use case for the filter. |
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.
My only comment is that we may want to drop the last assumption because, in this doc, we are talking about pipelines, and you cover the understanding part in Step 1.
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.
Thanks for the suggestion! I replaced this line with understanding the pipeline step logic to implement instead: 667abe5
|
||
Before creating a pipeline step, you should understand your use case for the filter and the specific logic you want to implement in the pipeline step. In our example, we want to prevent users from enrolling in a course if they do not have a valid email address. We will create a pipeline step that checks if the user's email address is valid and raise an exception if it is not. | ||
|
||
You should review the list of filters available in the Open edX platform and identify the filter that best fits your use case. In our example, we will use the `CourseEnrollmentStarted filter`_ to implement the logic for our use case. You should review the filter's arguments to understand the data that will be passed to the pipeline step and the expected output. This will help you define the pipeline step's logic and signature. |
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.
You should review the list of filters available in the Open edX platform and identify the filter that best fits your use case. In our example, we will use the `CourseEnrollmentStarted filter`_ to implement the logic for our use case. You should review the filter's arguments to understand the data that will be passed to the pipeline step and the expected output. This will help you define the pipeline step's logic and signature. | |
You should review the `list of filters`_ available in the Open edX platform and identify the filter that best fits your use case. In our example, we will use the `CourseEnrollmentStarted filter`_ to implement the logic for our use case. You should review the filter's arguments to understand the data that will be passed to the pipeline step and the expected output. This will help you define the pipeline step's logic and signature. |
I added this suggestion because I saw something similar in the openedx-events PR, and it took me to Reference/Events. I didn't know that page existed. I think it should be helpful to be able to redirect the people to the list of filters here: https://docs.openedx.org/projects/openedx-filters/en/latest/reference/filters.html#
Note: In the suggestion, we need to reference the link because I didn't put it.
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.
This is pretty useful, thank you! ad4a794
Thanks for addressing my feedback. This looks good to me ✅ |
ad4a794
to
de53680
Compare
Description
Update how-to documents to create and implement pipeline steps considering design suggestions introduced in #240.
Here are the main documents to review:
Testing instructions
You can review the docs here: https://docsopenedxorg--242.org.readthedocs.build/projects/openedx-filters/en/242/how-tos/index.html
Deadline
None
Checklists
Check off if complete or not applicable:
Merge Checklist:
Post Merge:
finished.