-
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
Conversation
Thanks for the pull request, @mariajgrimaldi! This repository is currently maintained by 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.
|
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 ✨
Thanks for addressing my feedback. This looks good to me ✅ |
ad4a794
to
de53680
Compare
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.
Some inline comments. Overall looks good, but I'm starting to see something that we are doing rather unintentionally. By focusing our documentation effort on all the parts of the lifecycle of : creating a new filter that did not previously exist, add said filter to the platform, implement pipeline steps with your feature, we are creating the impression on devs that they all need to take this route. I think we need to make it very well known that there are already quite a few filters and events available and that for every one of those you only need the part where you develop your code. This misconception is pushing people to believe that using filters is too complicated and not worth doing.
docs/how-tos/create-a-new-filter.rst
Outdated
|
||
In our example our use case proposal could be: | ||
|
||
I want to add a filter that will be triggered when a user enrolls in a course from the course about page. This filter will be used to prfilter users from enrolling in a course if they do not meet the eligibility criteria. The filter will be triggered when the user clicks the enroll button on the course about page and will check if the user meets the eligibility criteria. If the user does not meet the criteria, the filter will raise an exception to prfilter the user from enrolling in the course. |
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 want to add a filter that will be triggered when a user enrolls in a course from the course about page. This filter will be used to prfilter users from enrolling in a course if they do not meet the eligibility criteria. The filter will be triggered when the user clicks the enroll button on the course about page and will check if the user meets the eligibility criteria. If the user does not meet the criteria, the filter will raise an exception to prfilter the user from enrolling in the course. | |
I want to add a filter that will be triggered when a user enrolls in a course from the course about page. This filter will be used to prevent users from enrolling in a course if they do not meet the eligibility criteria. The filter will be triggered when the user clicks the enroll button on the course about page and will check if the user meets the eligibility criteria. If the user does not meet the criteria, the filter will raise an exception to prevent the user from enrolling in the course. |
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.
Thank you for noticing!
Step 7: Implement Your Pipeline Steps | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
Implementing pipeline steps allows you to modify the behavior of the application when the filter is triggered. Pipeline steps are a sequence of steps that are executed in a specific order to modify the behavior of the application. You can configure them with the :term:`filter configuration` to define the sequence of steps that are executed when the filter is triggered. Follow the steps in the :doc:`../how-tos/create-a-pipeline-step` guide to implement the pipeline steps 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.
I think we should say here that the pipeline steps are meant to be written in the plugin code and not in the openedx-filters library.
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.
There is a note about it here: https://github.com/openedx/openedx-filters/pull/242/files#diff-f1e167403e1377f568c3a2641fb3a151cd61d26887b3a468f91669d4494aee61R57. But I agree adding it here could make it clearer.
Step 2: Install Open edX Filters | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
First, add the ``openedx-filters`` plugin into your dependencies so the library's environment recognizes the event you want to consume. You can install ``openedx-filters`` by running: |
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.
openedx-filters
plugin -> openedx-filters
library. To not make it more confusing plugin should refer always to the developer's pluggable code and the openedx-filters or openedx-events should be called the library.
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.
"so the library's environment recognizes the event you want to consume" this is also confusing. We are not consuming an event, but creating a filter pipeline in this guide.
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 borrowed this from the consume events docs, that's why it is all mixed up. Thank you for noticing! Although I now have a question, technically openedx-events is a plugin, do you think that would cause misunderstandings? Calling openedx-events a plugin instead?
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.
What threshold do you use to recognize something as a plugin? For me the defining characteristic of a plugin, is that it defines the plugin_app
in its apps.py config. Neither events nor filters do that. That is why I call them libraries.
I could certainly be picking the wrong definition here. The other approach that I use for this, which is what triggered the comment, is that plugin is something that is not part of the core. We try to gently push developers in the direction of using plugins for the custom development instead of forking. By this approach also events and filters would not be considered plugins.
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.
Sorry for the misunderstanding. opened-events is a Django application, which makes it more than a library -- in my humble opinion, at least. I thought it actually defined a plugin_app
, but after reviewing apps.py I realized it didn't.
de53680
to
19d18f6
Compare
Thank you, @felipemontoya, for the thorough review.
I hear what you're saying, but I'm not sure I follow, given that one of the first steps in creating a pipeline specifically mentions reviewing the list of available filters to identify the right one for the use case: https://docsopenedxorg--242.org.readthedocs.build/projects/openedx-filters/en/242/how-tos/create-a-pipeline-step.html#step-1-understand-your-use-case-and-identify-the-filter-to-use. Do you think that part isn't clear enough? I’m open to suggestions to make it more obvious. Maybe a note at the beginning of creating a new filter warning devs that they don't need to create one if there's already one that fits their use case. |
@felipemontoya: I've addressed all your comments in this commit 2ee41e3. Thank you again! |
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 @mariajgrimaldi I think the note makes it clear that this is not the first step, but one that must be taken only when there is no similar available filter.
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.