From a8288e2df33742ebc3939d0897335102aed4ec0f Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Mon, 6 May 2024 15:50:31 +0200 Subject: [PATCH] Change approach to display acquired license for VS Instead of trying to guess the launch from the VitalSource course materials via LTI parameters that will be different from each LMS show the message in all cases for: - Non DL launches - No configured assignment In the VS integration we don't use our non-DL filepicker because it will required teachers selecting the H sku in *every* launch. We'll always use DL to configure assignments. That means that every non configured launch must be from the course materials section. --- lms/services/vitalsource/service.py | 21 ++++++++++----- lms/views/lti/basic_launch.py | 8 +++--- .../lms/services/vitalsource/service_test.py | 26 ++++++++++++++----- tests/unit/lms/views/lti/basic_launch_test.py | 6 +++-- 4 files changed, 42 insertions(+), 19 deletions(-) diff --git a/lms/services/vitalsource/service.py b/lms/services/vitalsource/service.py index 9866b1da68..4a985e10bc 100644 --- a/lms/services/vitalsource/service.py +++ b/lms/services/vitalsource/service.py @@ -2,7 +2,7 @@ from urllib.parse import parse_qs, urlencode, urlparse, urlunparse from lms.error_code import ErrorCode -from lms.models import LTIParams, LTIUser +from lms.models import Assignment, LTIParams, LTIUser from lms.services.vitalsource._client import VitalSourceClient from lms.services.vitalsource.exceptions import VitalSourceMalformedRegex from lms.services.vitalsource.model import VSBookLocation @@ -185,17 +185,26 @@ def get_user_reference(self, lti_params: LTIParams) -> str | None: return value def check_h_license( - self, lti_user: LTIUser, lti_params: LTIParams + self, lti_user: LTIUser, lti_params: LTIParams, assignment: Assignment | None ) -> ErrorCode | None: """Check if the user of the current launch has a license for the H LTI app.""" if not self._student_pay_enabled: # Not a school using student pay return None - if lti_params["resource_link_id"] == lti_params["context_id"]: - # This looks like a "course launch" - # This means that the user launched our tool from the course - # "course materials" placement to acquire a license for our SKU. + if not assignment: + # This looks like a launch meant to acquire a license for our SKU + # While in Canvas that type of launch is always made from a "Course placement" + # the best method to detect these across all LMSes is: + # - A non Deep linking launch. + # Students never do DL launches + # We won't check here this is not a DL launch. We'll rely on the caller for that + # As we don't ever check licenses for instructors there's shudn't be a need to check license in DL. + # + # - No assignment in the DB + # We'd expect no assignment in DL launches but for a regular launch that means the instructor is accessing + # us via the Launch Courseware VS button + # # We can't do anything else from here other than display a message return ( ErrorCode.VITALSOURCE_STUDENT_PAY_LICENSE_LAUNCH_INSTRUCTOR diff --git a/lms/views/lti/basic_launch.py b/lms/views/lti/basic_launch.py index 3f6a1edfbe..8178f66bcb 100644 --- a/lms/views/lti/basic_launch.py +++ b/lms/views/lti/basic_launch.py @@ -58,16 +58,16 @@ def __init__(self, context, request) -> None: def lti_launch(self): """Handle regular LTI launches.""" + assignment = self.assignment_service.get_assignment_for_launch(self.request) + if error_code := self.request.find_service(VitalSourceService).check_h_license( - self.request.lti_user, self.request.lti_params + self.request.lti_user, self.request.lti_params, assignment ): self.request.override_renderer = "lms:templates/error_dialog.html.jinja2" self.context.js_config.enable_error_dialog_mode(error_code) return {} - if assignment := self.assignment_service.get_assignment_for_launch( - self.request - ): + if assignment: self.request.override_renderer = ( "lms:templates/lti/basic_launch/basic_launch.html.jinja2" ) diff --git a/tests/unit/lms/services/vitalsource/service_test.py b/tests/unit/lms/services/vitalsource/service_test.py index da4b1a8602..b05c31616b 100644 --- a/tests/unit/lms/services/vitalsource/service_test.py +++ b/tests/unit/lms/services/vitalsource/service_test.py @@ -104,7 +104,7 @@ def test_check_h_license_disabled(self, svc, pyramid_request): svc._student_pay_enabled = False # pylint:disable=protected-access assert not svc.check_h_license( - pyramid_request.lti_user, pyramid_request.lti_params + pyramid_request.lti_user, pyramid_request.lti_params, sentinel.assignment ) @pytest.mark.parametrize( @@ -121,12 +121,12 @@ def test_check_h_license_course_launch( self, request, svc, pyramid_request, user_fixture, code ): svc._student_pay_enabled = True # pylint:disable=protected-access - pyramid_request.lti_params["context_id"] = "COURSE_ID" - pyramid_request.lti_params["resource_link_id"] = "COURSE_ID" _ = request.getfixturevalue(user_fixture) assert ( - svc.check_h_license(pyramid_request.lti_user, pyramid_request.lti_params) + svc.check_h_license( + pyramid_request.lti_user, pyramid_request.lti_params, None + ) == code ) @@ -136,7 +136,11 @@ def test_check_h_license_failure(self, svc, pyramid_request, customer_client): customer_client.get_user_book_license.return_value = None assert ( - svc.check_h_license(pyramid_request.lti_user, pyramid_request.lti_params) + svc.check_h_license( + pyramid_request.lti_user, + pyramid_request.lti_params, + sentinel.assignment, + ) == ErrorCode.VITALSOURCE_STUDENT_PAY_NO_LICENSE ) @@ -150,7 +154,11 @@ def test_check_h_license_success(self, svc, pyramid_request, customer_client): customer_client.get_user_book_license.return_value = sentinel.license assert not ( - svc.check_h_license(pyramid_request.lti_user, pyramid_request.lti_params) + svc.check_h_license( + pyramid_request.lti_user, + pyramid_request.lti_params, + sentinel.assignment, + ) ) @pytest.mark.usefixtures("user_is_instructor") @@ -160,7 +168,11 @@ def test_check_h_license_no_license_check_for_instructors( svc._student_pay_enabled = True # pylint:disable=protected-access assert not ( - svc.check_h_license(pyramid_request.lti_user, pyramid_request.lti_params) + svc.check_h_license( + pyramid_request.lti_user, + pyramid_request.lti_params, + sentinel.assignment, + ) ) customer_client.get_user_book_license.assert_not_called() diff --git a/tests/unit/lms/views/lti/basic_launch_test.py b/tests/unit/lms/views/lti/basic_launch_test.py index 52f3d3b7e9..134429f656 100644 --- a/tests/unit/lms/views/lti/basic_launch_test.py +++ b/tests/unit/lms/views/lti/basic_launch_test.py @@ -163,14 +163,16 @@ def test_lti_launch_unconfigured_launch_not_authorized( assert not response def test_lti_launch_check_h_license_fails( - self, context, pyramid_request, vitalsource_service + self, context, pyramid_request, vitalsource_service, assignment_service ): vitalsource_service.check_h_license.return_value = sentinel.error_code response = BasicLaunchViews(context, pyramid_request).lti_launch() vitalsource_service.check_h_license.assert_called_once_with( - pyramid_request.lti_user, pyramid_request.lti_params + pyramid_request.lti_user, + pyramid_request.lti_params, + assignment_service.get_assignment_for_launch.return_value, ) assert (