search

openedx / openedx-filters

Open edX filters from the Hooks Extensions Framework
GNU Affero General Public License v3.0
7 stars 17 forks source link

feat: add filter for hooking XBlock Render #171

Closed nsprenkle closed 3 months ago

nsprenkle commented 4 months ago

Description: Add a filter just before rendering of an XBlock scope. See discussion forum.

Justification: Some of our current work requires an ability to read and modify data in the XBlock render pipeline. While there already exist filters in the Vertical rendering pipeline, these are limited compared to the XBlock rendering pipeline in 2 important ways.

  1. The vertical render does not have access to the original request object, meaning if we are otherwise unable to read/modify data on or in response to that request.
  2. The vertical render pipeline does not apply to XBlock scopes outside of vertical/unit. If, for example, we ask to render an individual block, these filters will not get activated.

JIRA: AU-2039 and 2037

Merge deadline: List merge deadline (if any)

Installation instructions: n/a

Testing instructions:

  1. Check out related edx-platform branch: nsprenkle/xblock-render-filter
  2. Install local copy of openedx-filters.
  3. Attempt to render an xblobk: http://{lms}/xblock/block-v1:{block-id}?view=student_view
  4. Verify filter step is run.

Reviewers:

Merge checklist:

Post merge:

nsprenkle commented 4 months ago

The filter definition seems reasonable to me. Given the location you are planning to use the filter, it looks to me more similar to a RenderXBlockCompleted. I left this comment at the commit you shared. openedx/edx-platform@28e1601#commitcomment-142615584

The filter definition seems reasonable to me. Given the location you are planning to use the filter, it looks to me more similar to a RenderXBlockCompleted. I left this comment at the commit you shared. openedx/edx-platform@28e1601#commitcomment-142615584

Have updated filter names per your suggestion.

Scratch that, after thinking more about it, have moved filter location per your suggestion.

nsprenkle commented 3 months ago

@felipemontoya / @ormsbee , looking for re-review when you have time :)

nsprenkle commented 3 months ago

@felipemontoya , ready for re-review

mariajgrimaldi commented 3 months ago

@nsprenkle @felipemontoya: yes, that's all we need! Thank you both.

nsprenkle commented 3 months ago

Brilliant. Thanks a lot @nsprenkle. I think the filter is ready.

@mariajgrimaldi correct me please if I'm wrong. To merge we need to do a minor bump and add the changelog, right?

such as with https://github.com/openedx/openedx-filters/pull/158/files

Will add, thank you!