Skip to content
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: add newest docs for openedx-filters #64

Merged
merged 8 commits into from
Aug 22, 2023
Merged

Conversation

mariajgrimaldi
Copy link
Member

@mariajgrimaldi mariajgrimaldi commented Jan 26, 2023

Description

This PR adds the latest documentation for Open edX Filters usage.

@openedx-webhooks openedx-webhooks added core committer open-source-contribution PR author is not from Axim or 2U labels Jan 26, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Jan 26, 2023

Thanks for the pull request, @mariajgrimaldi!

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

@mphilbrick211
Copy link

Hey @mariajgrimaldi :) Just checking to see if you plan to pursue this one?

@feanil
Copy link

feanil commented Jun 2, 2023

@mariajgrimaldi I looked through the content you already put in here and it was very valuable, I'd love for you or other filters users to flesh out the rest of this tree that you built. I went ahead and wired everything together so the docs build is green. Are there other things we can do to help with this.

@openedx/arch-bom do you have any knowledge in this area of extensibility that you could use to help build out some of these docs?

@mariajgrimaldi
Copy link
Member Author

@feanil thank you so much! I will write at least the minimum info needed for a 1st version so we can deploy it and iterate over it! I already blocked a slot in my calendar to do it. I'll let you know once I'm done so you folks can review it!

@mariajgrimaldi mariajgrimaldi changed the title feat: fill in outline for new docs docs: add newest docs for openedx-filters Jun 28, 2023
@mariajgrimaldi mariajgrimaldi marked this pull request as ready for review June 28, 2023 21:32
@mariajgrimaldi mariajgrimaldi requested a review from a team as a code owner June 28, 2023 21:32
@mariajgrimaldi
Copy link
Member Author

note: we can add specifications about filters associated with the template rendering process, which are a bit different from the usual filters. We can also add a list of filters, like the index from edx-platform, but with descriptions and possible use cases.

Copy link

@feanil feanil left a comment

Choose a reason for hiding this comment

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

@mariajgrimaldi changes look great! Lots of awesome details, I have a few suggestions to add clarity but overall this looks great!

docs/concepts/hooks-extension-framework.rst Outdated Show resolved Hide resolved
docs/concepts/hooks-extension-framework.rst Outdated Show resolved Hide resolved
docs/concepts/hooks-extension-framework.rst Outdated Show resolved Hide resolved
docs/concepts/hooks-extension-framework.rst Outdated Show resolved Hide resolved
docs/concepts/hooks-extension-framework.rst Outdated Show resolved Hide resolved
Comment on lines 132 to 140
- The ``run_filter`` class method calls the ``run_pipeline`` method, which is the method that executes the filter's logic. This method is defined in the ``OpenEdxPublicFilter`` class, which is the base class for all the filters in the library. This method returns a dictionary with the following structure:

.. code-block:: python

{
"form_data": form_data,
}

Where ``form_data`` is the same set of parameters that the filter receives, which is the accumulated output for the filter's pipeline. This is how ``run_filter`` should always look like.
Copy link

Choose a reason for hiding this comment

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

This bullet point is a bit confusing. I think what you're saying is that for this specific filter, the run_pipeline method will return the dictionary with form_data in it. But in general what is return is up to the specific filter type? Is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! That's right. I'm adding a clarifying example.

Copy link
Member Author

Choose a reason for hiding this comment

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

1. The filter step must be a subclass of ``PipelineStep``.

2. The ``run_filter`` signature must match the filters definition, eg.,
the previous step matches the method's definition in CertificateCreationRequested.
Copy link

Choose a reason for hiding this comment

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

suggestion: It might be useful to put a copy of the filter definition here so that readers can see how the signature in the PipelineStep maps to the signature of the run_filter in the filter definition. Because when you say the filters definition it's not clear if you mean the signature of run_pipeline or of run_filter in the filter definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

docs/how-tos/using-filters.rst Outdated Show resolved Hide resolved
docs/how-tos/using-filters.rst Outdated Show resolved Hide resolved
docs/reference/django-plugins-and-filters.rst Outdated Show resolved Hide resolved
@mphilbrick211
Copy link

Hi @mariajgrimaldi - just following up on this!

Copy link

@feanil feanil left a comment

Choose a reason for hiding this comment

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

@mariajgrimaldi this is looking great, I had a few small suggestions but then the how-tos for creating and using filters seem good enough to land. I see you have a couple of quickstarts that are still in progress, do you want to do those in a follow-up PR so we can land the pieces that already seem like they're nearly done.

Filter signature
----------------

It's the signature of the `run_filter` method. It defines the input and output of the filter. The input is a dictionary with the data to be processed and the output is a dictionary with the processed data.
Copy link

Choose a reason for hiding this comment

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

Since both the PipelineStep and Open edX Filter class implement a run_filter function which do you mean here? Do both of them have the same signature, it would be good to clarify that in this glossary item.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm I'll better rephrase this then! Thanks

@@ -0,0 +1,11 @@
Using Open edX Filters in the LMS service
=========================================
Copy link

Choose a reason for hiding this comment

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

This document should follow standard heading order as defined here: https://docs.openedx.org/en/latest/documentors/references/quick_reference_rst.html#headings

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I'll change it

mariajgrimaldi and others added 8 commits August 14, 2023 14:51
Also drop the changelog from the long description.  The way it is right
now, that will never succeed the strict checks becaause of the way
setuptools currently parses the RST.  It generates implicit labels for
all headings by default and then it complains when there are duplicates.

This will always happen with changelogs because the sub-headings under
a changelog entry will always be one of a small set of values (added,
fixed, etc.)
@openedx-webhooks
Copy link

Thanks for the pull request, @mariajgrimaldi! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@mariajgrimaldi
Copy link
Member Author

@feanil: last changes! Let me know what you think

@feanil
Copy link

feanil commented Aug 14, 2023

Looks great! Thanks for all the hard work on this @mariajgrimaldi !

@mariajgrimaldi
Copy link
Member Author

I'll be merging this now! Thank you folks!

@mariajgrimaldi mariajgrimaldi merged commit 70d3fb1 into main Aug 22, 2023
6 checks passed
@mariajgrimaldi mariajgrimaldi deleted the MJG/filers-docs branch August 22, 2023 12:47
@openedx-webhooks
Copy link

@mariajgrimaldi 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core committer open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants