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

feat: add filter for hooking XBlock Render #171

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

nsprenkle
Copy link
Contributor

@nsprenkle nsprenkle commented May 28, 2024

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:

  • All reviewers approved
  • Version bumped
  • Changelog record added
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

@nsprenkle nsprenkle marked this pull request as ready for review May 31, 2024 15:45
@nsprenkle nsprenkle requested a review from a team as a code owner May 31, 2024 15:45
Copy link
Member

@felipemontoya felipemontoya left a comment

Choose a reason for hiding this comment

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

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

@nsprenkle
Copy link
Contributor Author

nsprenkle commented Jun 3, 2024

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
Copy link
Contributor Author

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

Copy link
Member

@felipemontoya felipemontoya left a comment

Choose a reason for hiding this comment

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

The filter looks good, but in order to give users of this filter more range of action I suggest we allow them to render something controllable and not only a 500 error.

openedx_filters/learning/filters.py Show resolved Hide resolved
@nsprenkle
Copy link
Contributor Author

@felipemontoya , ready for re-review

Copy link
Member

@felipemontoya felipemontoya left a comment

Choose a reason for hiding this comment

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

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

@mariajgrimaldi
Copy link
Member

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

@nsprenkle
Copy link
Contributor Author

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!

@nsprenkle nsprenkle force-pushed the nsprenke/render-xblock-filter branch from ffadc2d to a0492a1 Compare June 14, 2024 18:22
@nsprenkle nsprenkle force-pushed the nsprenke/render-xblock-filter branch from a0492a1 to 2b7e510 Compare June 14, 2024 19:04
@felipemontoya felipemontoya merged commit 9168da6 into openedx:main Jun 14, 2024
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants