Refactor the canvas API factory to take parameters #6927
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
marcospri
force-pushed
the
canvas-factory
branch
from
67d4fec to
263e0d3
Compare
marcospri
changed the title
marcospri
commented
Dec 30, 2024
| @@ -106,7 +106,9 @@ def includeme(config): # noqa: PLR0915 | |||
| config.register_service_factory( | |||
| "lms.services.grouping.service_factory", name="grouping" | |||
| ) | |||
| config.register_service_factory("lms.services.file.factory", name="file") | |||
| config.register_service_factory( | |||
There was a problem hiding this comment.
Use the new name on register_service_factory
marcospri
commented
Dec 30, 2024
|
|
||
|
|
||
| def canvas_api_client_factory(_context, request): | ||
| def canvas_api_client_factory( |
There was a problem hiding this comment.
Take both AI and user_id as new optional parameters in the factory.
| """ | ||
| Get a CanvasAPIClient from a pyramid request. | ||
|
|
||
| :param request: Pyramid request object | ||
| :return: An instance of CanvasAPIClient | ||
| """ | ||
| application_instance = request.lti_user.application_instance | ||
| if application_instance and user_id: | ||
| oauth2_token_service = oauth2_token_service_factory( |
There was a problem hiding this comment.
Note that oauth2_token_service_factory already uses this pattern, takes AI and user_id.
| file_service = file_service_factory(_context, request, application_instance) | ||
|
|
||
| else: | ||
| oauth2_token_service = request.find_service(name="oauth2_token") |
There was a problem hiding this comment.
By default, use the services from the factories without parameters.
| return FileService( | ||
| application_instance=request.lti_user.application_instance, db=request.db | ||
| ) | ||
| def file_service_factory(_context, request, application_instance=None): |
There was a problem hiding this comment.
Give this a better name to be imported in other modules.
marcospri
force-pushed
the
canvas-factory
branch
from
263e0d3 to
ab978f7
Compare
Take application_instance and user_id on the Canvas API factory to allow creating a service for arbitrary users instead of just the one of the current request. The same modification is also made to the file service as it is a dependency of the Canvas service.
marcospri
force-pushed
the
canvas-factory
branch
from
6a7bf82 to
00ee5b6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Take application_instance and user_id on the Canvas API factory
to allow creating a service for arbitrary users instead of just the one
of the current request.
The same modification is also made to the file service
as it is a dependency of the Canvas service.
Motivation
This will allow us to use the Canvas API to fetch sections rosters outside the request cycle, using any application instance we need.
Testing
https://hypothesis.instructure.com/courses/125/assignments/875