-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: pluggable url for idv location #35494
base: master
Are you sure you want to change the base?
Conversation
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 so far! A question/suggestion!
url = IDVerificationService.get_verify_location() | ||
assert url == TestIdvPageUrlRequestedPipelineStep.TEST_URL | ||
|
||
url = IDVerificationService.get_verify_location('course-v1:edX+DemoX+Demo_Course') |
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.
Suggestion: is there a reason that the urls tested are both the same here? I'm not sure if this would make a difference, but would it be better for the urls to be different. Or is the point that they should in fact be the same? Maybe I'm misunderstanding.
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 want the output the same because I'm validating the url is overridden by the filter hook in both cases.
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.
We should also test the case when the filter is not configured so we know it's not getting overridden.
@@ -244,7 +245,8 @@ def get_verify_location(cls, course_id=None): | |||
location = f'{settings.ACCOUNT_MICROFRONTEND_URL}/id-verification' | |||
if course_id: | |||
location += f'?course_id={quote(str(course_id))}' | |||
return location | |||
|
|||
return IDVPageURLRequested.run_filter(location) |
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.
Don't forget to add:
# .. filter_implemented_name: IDVPageURLRequested
# .. filter_type: org.openedx.learning.idv.page.url.requested.v1
So it's easier to identify later on. Thanks!
Note this is blocked from merging until openedx/openedx-filters#213 is available in edx-platform
Description
Adds an extension point when generating the url for id verification. This makes use of a new filter introduced here (openedx/openedx-filters#213) to allow plugin code to override where a user would be directed to complete the verification process.
Supporting information
This is part of the overall IDV plugability approach defined here: openedx/platform-roadmap#367
2U Internal Ticket: COSMO-429