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: Implement IDV URL Filters #213

Merged
merged 4 commits into from
Sep 23, 2024

Conversation

ilee2u
Copy link
Contributor

@ilee2u ilee2u commented Sep 10, 2024

Description: The purpose of this PR is to add an Open edX filter hook for getting the URL to the IDV page to the GitHub - openedx/openedx-filters: Open edX filters from the Hooks Extensions Framework repository.

For the sake of justifying this filter, here is the relevant Open edX Product Proposal and Issue to Add Extensibility Mechanisms to IDV to Enable Integration of New IDV Vendor Persona.

JIRA: Link to JIRA ticket

Dependencies: None

Merge deadline: N/A

Reviewers:

  • tag reviewer
  • tag reviewer

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)

filter_type = "org.openedx.learning.idv.page.url.requested.v1"

@classmethod
def run_filter(cls, url):

Choose a reason for hiding this comment

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

Could you add type annotations to the method definition, since that seems to be the pattern in this repository, please?

- The filter must have the signature specified.
- The filter should return the url.
"""
url = Mock(), Mock()

Choose a reason for hiding this comment

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

I'm curious why you need a tuple here. url should be a single value. Does url = Mock() not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to remove this when I copy/pasted an earlier test

Copy link
Member

@varshamenon4 varshamenon4 left a comment

Choose a reason for hiding this comment

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

Looks good!

- IDVPageURLRequested
"""

def test_course_idv_page_url_requested(self):
Copy link
Member

Choose a reason for hiding this comment

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

nit: is there a reason that the test case includes course in it? Wondering if it should be:

Suggested change
def test_course_idv_page_url_requested(self):
def test_idv_page_url_requested(self):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this is another artifact from me copy/pasting tests.

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.

I left a minor comment in the edx-platform PR for you to review, the rest looks good!

@mariajgrimaldi
Copy link
Member

@ilee2u: please, add an entry to the changelog file so I can proceed with the merge. Thanks!

@mariajgrimaldi mariajgrimaldi merged commit 809bc38 into openedx:main Sep 23, 2024
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.

4 participants