-
Notifications
You must be signed in to change notification settings - Fork 16
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
Conversation
openedx_filters/learning/filters.py
Outdated
filter_type = "org.openedx.learning.idv.page.url.requested.v1" | ||
|
||
@classmethod | ||
def run_filter(cls, url): |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this 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): |
There was a problem hiding this comment.
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:
def test_course_idv_page_url_requested(self): | |
def test_idv_page_url_requested(self): |
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
@ilee2u: please, add an entry to the changelog file so I can proceed with the merge. Thanks! |
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:
Merge checklist:
Post merge:
finished.