From 6c249b7ccdbfefb9d7a778d1b9177403c8cf2261 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Wed, 4 Sep 2024 14:07:56 +0200 Subject: [PATCH] Use the LTI1.1 assignment ID after a failure to fetch a roster We have seen some issues in production where the assignment roster fetch fails with a cryptic error from Canvas. We have not been able to reproduce this situation locally. As we have two IDs for the same assignment lets try the LTI1.1 one. We have tested the other following scenarios: - LTI1.3 assignment. Use the LTI1.3 id works. - Upgrade from LTI1.1 LTI1.3 assignment. Using the LTI1.1 ID fails while the 1.3 one works. --- lms/services/roster.py | 33 +++++++++++++--- lms/tasks/roster.py | 2 +- tests/unit/lms/services/roster_test.py | 53 +++++++++++++++++++++++++- 3 files changed, 81 insertions(+), 7 deletions(-) diff --git a/lms/services/roster.py b/lms/services/roster.py index b3b03270de..b9f35ea902 100644 --- a/lms/services/roster.py +++ b/lms/services/roster.py @@ -15,6 +15,7 @@ ) from lms.models.h_user import get_h_userid, get_h_username from lms.models.lti_user import display_name +from lms.services.exceptions import ExternalRequestError from lms.services.lti_names_roles import LTINamesRolesService from lms.services.lti_role_service import LTIRoleService from lms.services.upsert import bulk_upsert @@ -111,11 +112,33 @@ def fetch_assignment_roster(self, assignment: Assignment) -> None: ), "Trying fetch roster for course without service URL." lti_registration = self._get_lti_registration(lms_course) - roster = self._lti_names_roles_service.get_context_memberships( - lti_registration, - lms_course.lti_context_memberships_url, - resource_link_id=assignment.lti_v13_resource_link_id, - ) + try: + roster = self._lti_names_roles_service.get_context_memberships( + lti_registration, + lms_course.lti_context_memberships_url, + resource_link_id=assignment.lti_v13_resource_link_id, + ) + except ExternalRequestError as err: + if ( + err.response_body + and ( + # Error message from Canvas + "Requested ResourceLink bound to unexpected external tool" + in err.response_body + ) + # In cases for which we have different assignment IDs + and assignment.resource_link_id != assignment.lti_v13_resource_link_id + ): + # The LMS (Canvas) doesn't seem to be able to link our tool to this assignment + # Try the same call but with the LTI1.1 assignment identifier. + LOG.info("Try to fetch roster with the LTI1.1 identifier") + roster = self._lti_names_roles_service.get_context_memberships( + lti_registration, + lms_course.lti_context_memberships_url, + resource_link_id=assignment.resource_link_id, + ) + else: + raise # Insert any users we might be missing in the DB lms_users_by_lti_user_id = { diff --git a/lms/tasks/roster.py b/lms/tasks/roster.py index 670b4c4986..8fc86a1c32 100644 --- a/lms/tasks/roster.py +++ b/lms/tasks/roster.py @@ -145,7 +145,7 @@ def fetch_course_roster(*, lms_course_id) -> None: def fetch_assignment_roster(*, assignment_id) -> None: """Fetch the roster for one course.""" with app.request_context() as request: - roster_service = request.find_service(RosterService) + roster_service: RosterService = request.find_service(RosterService) with request.tm: assignment = request.db.get(Assignment, assignment_id) roster_service.fetch_assignment_roster(assignment) diff --git a/tests/unit/lms/services/roster_test.py b/tests/unit/lms/services/roster_test.py index 99bc7acc30..f03b2b8f1a 100644 --- a/tests/unit/lms/services/roster_test.py +++ b/tests/unit/lms/services/roster_test.py @@ -1,10 +1,11 @@ -from unittest.mock import sentinel +from unittest.mock import Mock, call, sentinel import pytest from h_matchers import Any from sqlalchemy import select from lms.models import AssignmentRoster, CourseRoster +from lms.services.exceptions import ExternalRequestError from lms.services.roster import RosterService, factory from tests import factories @@ -126,6 +127,56 @@ def test_fetch_assignment_roster( assert roster[3].lms_user.lti_user_id == "USER_ID_INACTIVE" assert not roster[3].active + def test_fetch_assignment_roster_retries_with_lti_v11_id( + self, + svc, + lti_names_roles_service, + lti_v13_application_instance, + assignment, + names_and_roles_roster_response, + lti_role_service, + ): + lti_role_service.get_roles.return_value = [ + factories.LTIRole(value="ROLE1"), + factories.LTIRole(value="ROLE2"), + ] + + lti_names_roles_service.get_context_memberships.side_effect = [ + ExternalRequestError( + response=Mock( + text="Requested ResourceLink bound to unexpected external tool" + ) + ), + names_and_roles_roster_response, + ] + + svc.fetch_assignment_roster(assignment) + + lti_names_roles_service.get_context_memberships.assert_has_calls( + [ + call( + lti_v13_application_instance.lti_registration, + "SERVICE_URL", + assignment.lti_v13_resource_link_id, + ), + call( + lti_v13_application_instance.lti_registration, + "SERVICE_URL", + assignment.resource_link_id, + ), + ] + ) + + def test_fetch_assignment_roster_raises_external_request_error( + self, svc, lti_names_roles_service, assignment + ): + lti_names_roles_service.get_context_memberships.side_effect = ( + ExternalRequestError() + ) + + with pytest.raises(ExternalRequestError): + svc.fetch_assignment_roster(assignment) + @pytest.fixture def lms_course(self, lti_v13_application_instance): lms_course = factories.LMSCourse(lti_context_memberships_url="SERVICE_URL")