From ab978f749b770368352b87f0dcec081df4312646 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Mon, 30 Dec 2024 09:37:50 +0100 Subject: [PATCH] Refactor the canvas API factory to take parameters 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. --- lms/services/__init__.py | 4 +- lms/services/canvas_api/factory.py | 29 +++++++++-- lms/services/file.py | 9 ++-- .../lms/services/canvas_api/factory_test.py | 52 ++++++++++++++++--- tests/unit/lms/services/file_test.py | 29 +++++++++-- 5 files changed, 103 insertions(+), 20 deletions(-) diff --git a/lms/services/__init__.py b/lms/services/__init__.py index 98e9b9a47c..41813936ec 100644 --- a/lms/services/__init__.py +++ b/lms/services/__init__.py @@ -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( + "lms.services.file.file_service_factory", name="file" + ) config.register_service_factory( "lms.services.jstor.service_factory", iface=JSTORService ) diff --git a/lms/services/canvas_api/factory.py b/lms/services/canvas_api/factory.py index 129d7eaa9d..97ede57a82 100644 --- a/lms/services/canvas_api/factory.py +++ b/lms/services/canvas_api/factory.py @@ -3,16 +3,37 @@ from lms.services.canvas_api._basic import BasicClient from lms.services.canvas_api._pages import CanvasPagesClient from lms.services.canvas_api.client import CanvasAPIClient +from lms.services.file import file_service_factory +from lms.services.oauth2_token import oauth2_token_service_factory -def canvas_api_client_factory(_context, request): +def canvas_api_client_factory( + _context, request, application_instance=None, user_id=None +): """ 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( + _context, + request, + application_instance=application_instance, + user_id=user_id, + ) + file_service = file_service_factory(_context, request, application_instance) + + else: + oauth2_token_service = request.find_service(name="oauth2_token") + file_service = request.find_service(name="file") + + if not application_instance: + application_instance = request.lti_user.application_instance + + if not user_id: + user_id = request.lti_user.user_id developer_secret = application_instance.decrypted_developer_secret( request.find_service(AESService) @@ -22,13 +43,11 @@ def canvas_api_client_factory(_context, request): authenticated_api = AuthenticatedClient( basic_client=basic_client, - oauth2_token_service=request.find_service(name="oauth2_token"), + oauth2_token_service=oauth2_token_service, client_id=application_instance.developer_key, client_secret=developer_secret, redirect_uri=request.route_url("canvas_api.oauth.callback"), ) - file_service = request.find_service(name="file") - return CanvasAPIClient( authenticated_api, file_service=file_service, diff --git a/lms/services/file.py b/lms/services/file.py index dad6a8663a..fc89de2025 100644 --- a/lms/services/file.py +++ b/lms/services/file.py @@ -90,7 +90,8 @@ def _file_search_query( # noqa: PLR0913 return query -def factory(_context, request): - return FileService( - application_instance=request.lti_user.application_instance, db=request.db - ) +def file_service_factory(_context, request, application_instance=None): + if application_instance is None: + application_instance = request.lti_user.application_instance + + return FileService(application_instance=application_instance, db=request.db) diff --git a/tests/unit/lms/services/canvas_api/factory_test.py b/tests/unit/lms/services/canvas_api/factory_test.py index cbaffb55f8..daadb82de1 100644 --- a/tests/unit/lms/services/canvas_api/factory_test.py +++ b/tests/unit/lms/services/canvas_api/factory_test.py @@ -3,6 +3,7 @@ import pytest from lms.services.canvas_api.factory import canvas_api_client_factory +from tests import factories pytestmark = pytest.mark.usefixtures( "application_instance_service", "oauth2_token_service", "file_service" @@ -12,16 +13,18 @@ class TestCanvasAPIClientFactory: @pytest.mark.usefixtures("aes_service") @pytest.mark.parametrize("folders_enabled", [True, False]) - def test_building_the_CanvasAPIClient( + def test_it( self, pyramid_request, CanvasAPIClient, AuthenticatedClient, BasicClient, CanvasPagesClient, + oauth2_token_service, file_service, folders_enabled, application_instance, + aes_service, ): application_instance.settings.set("canvas", "folders_enabled", folders_enabled) @@ -37,27 +40,56 @@ def test_building_the_CanvasAPIClient( pages_client=CanvasPagesClient.return_value, folders_enabled=folders_enabled, ) + AuthenticatedClient.assert_called_once_with( + basic_client=BasicClient.return_value, + oauth2_token_service=oauth2_token_service, + client_id=application_instance.developer_key, + client_secret=application_instance.decrypted_developer_secret(aes_service), + redirect_uri=pyramid_request.route_url("canvas_api.oauth.callback"), + ) + assert canvas_api == CanvasAPIClient.return_value - def test_building_the_AuthenticatedClient( + def test_it_with_application_instance_and_user_id( self, pyramid_request, - application_instance, + CanvasAPIClient, AuthenticatedClient, BasicClient, - oauth2_token_service, + CanvasPagesClient, aes_service, + file_service_factory, + oauth2_token_service_factory, ): - canvas_api_client_factory(sentinel.context, pyramid_request) + application_instance = factories.ApplicationInstance() + + canvas_api = canvas_api_client_factory( + sentinel.context, + pyramid_request, + application_instance=application_instance, + user_id=sentinel.user_id, + ) + BasicClient.assert_called_once_with(application_instance.lms_host()) + CanvasPagesClient.assert_called_once_with( + AuthenticatedClient.return_value, file_service_factory.return_value + ) + CanvasAPIClient.assert_called_once_with( + AuthenticatedClient.return_value, + file_service=file_service_factory.return_value, + pages_client=CanvasPagesClient.return_value, + folders_enabled=False, + ) AuthenticatedClient.assert_called_once_with( basic_client=BasicClient.return_value, - oauth2_token_service=oauth2_token_service, + oauth2_token_service=oauth2_token_service_factory.return_value, client_id=application_instance.developer_key, client_secret=application_instance.decrypted_developer_secret(aes_service), redirect_uri=pyramid_request.route_url("canvas_api.oauth.callback"), ) + assert canvas_api == CanvasAPIClient.return_value + @pytest.fixture(autouse=True) def BasicClient(self, patch): return patch("lms.services.canvas_api.factory.BasicClient") @@ -73,3 +105,11 @@ def CanvasPagesClient(self, patch): @pytest.fixture(autouse=True) def CanvasAPIClient(self, patch): return patch("lms.services.canvas_api.factory.CanvasAPIClient") + + @pytest.fixture(autouse=True) + def file_service_factory(self, patch): + return patch("lms.services.canvas_api.factory.file_service_factory") + + @pytest.fixture(autouse=True) + def oauth2_token_service_factory(self, patch): + return patch("lms.services.canvas_api.factory.oauth2_token_service_factory") diff --git a/tests/unit/lms/services/file_test.py b/tests/unit/lms/services/file_test.py index 463891fd0e..14592e1a57 100644 --- a/tests/unit/lms/services/file_test.py +++ b/tests/unit/lms/services/file_test.py @@ -3,7 +3,7 @@ import pytest from lms.models import File -from lms.services.file import FileService, factory +from lms.services.file import FileService, file_service_factory from tests import factories @@ -140,7 +140,28 @@ def file(self, application_instance): @pytest.mark.usefixtures("application_instance_service") class TestFactory: - def test_it(self, pyramid_request): - file_service = factory(sentinel.context, pyramid_request) + def test_it(self, pyramid_request, FileService, application_instance, db_session): + file_service = file_service_factory(sentinel.context, pyramid_request) - assert isinstance(file_service, FileService) + FileService.assert_called_once_with( + application_instance=application_instance, db=db_session + ) + assert file_service == FileService.return_value + + def test_it_with_application_instance_parameter( + self, pyramid_request, FileService, db_session + ): + file_service = file_service_factory( + sentinel.context, + pyramid_request, + application_instance=sentinel.application_instance, + ) + + FileService.assert_called_once_with( + application_instance=sentinel.application_instance, db=db_session + ) + assert file_service == FileService.return_value + + @pytest.fixture + def FileService(self, patch): + return patch("lms.services.file.FileService")