Skip to content

feat: adds the VerticalBlockChildRenderStarted filter#38

Merged
mariajgrimaldi merged 1 commit intoopenedx:mainfrom
open-craft:tecoholic/add-vertical-block-child-render-started-filter
Aug 18, 2022
Merged

feat: adds the VerticalBlockChildRenderStarted filter#38
mariajgrimaldi merged 1 commit intoopenedx:mainfrom
open-craft:tecoholic/add-vertical-block-child-render-started-filter

Conversation

@tecoholic
Copy link
Contributor

@tecoholic tecoholic commented Jul 23, 2022

Description:

This adds the VerticalBlockChildRenderStarted filter which passes the child XBlock and it's rendering context dictionary to the filter allowing the filter pipeline to react to the content of the XBlock based on the context.

This is a follow up of the PoC implemented in PR #35. The filter attempted in the previous PR was poorly placed and was limiting. The filter introduced in this PR provides a better opportunity for plugin's to enhance the experience based on the XBlock being rendered.

The Edit Links PR open-craft/openedx-edit-links#1 shows one such use-case of adding extra HTML to the content of HTML Block before rendering.

JIRA: Link to JIRA ticket

Dependencies:

Merge deadline: NA

Installation instructions:

Ensure that edx-platform is checked out to branch of openedx/openedx-platform#30773

Testing instructions:

Unit tests are added for the filter. For full feature test, follow testing instructions from open-craft/openedx-edit-links#1

Reviewers:

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • 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)

Author concerns:
NA

@navinkarkera
Copy link

👍

  • I tested this: (describe what you tested)
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

@mariajgrimaldi mariajgrimaldi self-requested a review August 2, 2022 16:31
@mariajgrimaldi mariajgrimaldi self-assigned this Aug 2, 2022
Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

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

This definition looks good! In the next few days, I'll test the full feature with openedx-edit-links & the Open edX PR. Sorry for the delay!

@mariajgrimaldi
Copy link
Member

This was quite fun to test! I'll attach a screenshot:
pr-34-test

Thanks for your contribution again. I'm just a bit hesitant on the filter type: "org.openedx.learning.verticalblockchild.render.started.v1" it could be "org.openedx.learning.vertical_block_child.render.started.v1" what do you think?

FYI @felipemontoya

@felipemontoya
Copy link
Member

vertical_block_child -> this would be any xblock that can be added vía the studio interface, correct?
Any leave from the tree of xblocks

@tecoholic
Copy link
Contributor Author

@mariajgrimaldi vertical_block_child sound right. We can change it to that.

@felipemontoya Yes. This is any block added to a unit.

@tecoholic
Copy link
Contributor Author

@mariajgrimaldi I have updated the filter name to vertical_block_child.

Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

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

This looks great! Let me know when it's ready for merging

This adds the VerticalBlockChildRenderStarted filter which passes the
child XBlock and it's rendering context dictionary to the filter
allowing the filter pipeline to alter the content of the XBlock based on
the context.

Private-ref: https://gitlab.com/mooc-floss/mooc-floss/-/issues/112
@tecoholic tecoholic force-pushed the tecoholic/add-vertical-block-child-render-started-filter branch from 1989c46 to 62663a7 Compare August 18, 2022 05:09
@tecoholic
Copy link
Contributor Author

@mariajgrimaldi Thank you. I have made the following updates to the PR.

  • Added the right spacing in tests for conforming to the AAA
  • Bumped the version to 0.8.0
  • Added change log for the version 0.8.0
  • Squashed the commits into a single commit

With the above changes, I think this is ready for merging.

The only item missing in the checklist is documentation. I checked the docs folder to see if there is any relevant documentation that I could update, didn't find any filter specific changes that are required. Let me know if I have overlooked something here.

@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Aug 18, 2022

Thanks! @tecoholic

We have a hooks guide listing the current filters here. We're currently figuring out how to better document filters and events, but that's the primary reference that should be updated. Feel free to edit it & let me know for a review.

@mariajgrimaldi mariajgrimaldi merged commit 0318401 into openedx:main Aug 18, 2022
@mariajgrimaldi
Copy link
Member

I opened this issue regarding docs improvement. As a filters adopter, do you think there's something else meaningful to add?

@tecoholic
Copy link
Contributor Author

@mariajgrimaldi Thank you merging this :) And also the link to the place where the hooks are documented, I will add the documentation and make it a part of the PR openedx/openedx-platform#30773.

#41 looks like a good improvement for the library. I will be able to contribute some way as well. It would be of huge help for the community in general. Eg., Discussion on implementing tag management

@tecoholic tecoholic deleted the tecoholic/add-vertical-block-child-render-started-filter branch August 20, 2022 07:17
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.

4 participants