From 3822d2a424dd7875a2ed6008220ad1a4c0e4d517 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Tue, 14 Jan 2025 10:42:55 +0100 Subject: [PATCH] Service method to fetch canvas sections rosters This commit includes a new method to fetch section rosters. It doesn't yet fetch (schedule) rosters. This are different than the other ones we have so far (course, assignments and Canvas groups in a few different ways: - We use the Canvas API instead of the LTI one. This means we have to deal with users tokens, refreshing them etc. For now we'll just attempt a token refresh if necessary. If that, or anything else fails, we'll give up. - The API doesn't provide enough information to create the users we haven't seen. This means that section rosters only help us to organize existing users into existing sections. We fetch sections on every launch so discovering sections shouldn't be a concern. Not being able to discover new users means that we'd need the course/section roster to begin with. This limits canvas sections also to LTI1.3. --- lms/services/canvas_api/client.py | 20 +- lms/services/roster.py | 239 +++++++++++++++++- tests/factories/lms_user.py | 1 + .../lms/services/canvas_api/client_test.py | 22 ++ tests/unit/lms/services/roster_test.py | 207 ++++++++++++++- 5 files changed, 469 insertions(+), 20 deletions(-) diff --git a/lms/services/canvas_api/client.py b/lms/services/canvas_api/client.py index 1b7ce84dad..952406a61c 100644 --- a/lms/services/canvas_api/client.py +++ b/lms/services/canvas_api/client.py @@ -15,6 +15,15 @@ log = logging.getLogger(__name__) +class _SectionStudentSchema(Schema): + """Schema for each of the students that belong to a section.""" + + class Meta: + unknown = EXCLUDE + + id = fields.Int(required=True) + + class _SectionSchema(Schema): """ Schema for an individual course section dict. @@ -30,6 +39,10 @@ class Meta: id = fields.Int(required=True) name = fields.String(required=True) + students = fields.List( + fields.Nested(_SectionStudentSchema), required=False, allow_none=True + ) + class CanvasAPIClient: """ @@ -148,7 +161,7 @@ def post_load(self, data, **_kwargs): # Return the contents of sections without the key return data["sections"] - def course_sections(self, course_id): + def course_sections(self, course_id, with_students=False): """ Return all the sections for the given course_id. @@ -159,10 +172,15 @@ def course_sections(self, course_id): # For documentation of this request see: # https://canvas.instructure.com/doc/api/sections.html#method.sections.index + params = {} + if with_students: + params = {"include[]": "students"} + return self._ensure_sections_unique( self._client.send( "GET", f"courses/{course_id}/sections", + params=params, schema=self._CourseSectionsSchema, ) ) diff --git a/lms/services/roster.py b/lms/services/roster.py index 3d1e568971..92fac9a9a9 100644 --- a/lms/services/roster.py +++ b/lms/services/roster.py @@ -1,7 +1,7 @@ from datetime import datetime from logging import getLogger -from sqlalchemy import Select, func, select, text, update +from sqlalchemy import Select, func, select, text, union, update from lms.models import ( ApplicationInstance, @@ -10,6 +10,7 @@ CourseRoster, LMSCourse, LMSCourseApplicationInstance, + LMSCourseMembership, LMSSegment, LMSSegmentRoster, LMSUser, @@ -20,9 +21,17 @@ ) 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.canvas_api.client import CanvasAPIClient +from lms.services.canvas_api.factory import canvas_api_client_factory +from lms.services.exceptions import ( + CanvasAPIError, + ConcurrentTokenRefreshError, + ExternalRequestError, + OAuth2TokenError, +) from lms.services.lti_names_roles import LTINamesRolesService from lms.services.lti_role_service import LTIRoleService +from lms.services.oauth2_token import OAuth2TokenService from lms.services.upsert import bulk_upsert LOG = getLogger(__name__) @@ -31,12 +40,13 @@ class RosterService: def __init__( self, - db, + request, lti_names_roles_service: LTINamesRolesService, lti_role_service: LTIRoleService, h_authority: str, ): - self._db = db + self._request = request + self._db = request.db self._lti_names_roles_service = lti_names_roles_service self._lti_role_service = lti_role_service self._h_authority = h_authority @@ -245,7 +255,7 @@ def fetch_assignment_roster(self, assignment: Assignment) -> None: ) def fetch_canvas_group_roster(self, canvas_group: LMSSegment) -> None: - """Fetch the roster information for an assignment from the LMS.""" + """Fetch the roster information for a canvas group from the LMS.""" assert canvas_group.type == "canvas_group" lms_course = canvas_group.lms_course @@ -316,6 +326,127 @@ def fetch_canvas_group_roster(self, canvas_group: LMSSegment) -> None: update_columns=["active", "updated"], ) + def fetch_canvas_sections_roster(self, lms_course: LMSCourse) -> None: + """Fetch the roster information for all canvas sections for one particular course. + + Sections are different than other rosters: + - We fetch them via the proprietary Canvas API, not the LTI Names and Roles endpoint. + + - Due to the return value of that API we don't fetch rosters for indivual sections, + but for all sections of one course at once + + - The return value of the API doesn't include enough information to create unseen users + so we'll only match against users we have seen before in the course. + """ + application_instance = self._get_application_instance(lms_course) + + # Last instructor to launch the course, we'll use this user's API token to fetch the sections. + instructor = self._get_course_instructor(lms_course) + if not instructor: + LOG.info( + "Can't fetch roster for sections of course ID:%s. No instructor found.", + lms_course.id, + ) + return + + # Get all the sections for this course that we've seen in the DB. + db_sections = self._db.scalars( + select(LMSSegment).where( + LMSSegment.lms_course_id == lms_course.id, + LMSSegment.type == "canvas_section", + ) + ).all() + + if not db_sections: + LOG.info( + "Can't fetch roster for sections of course ID:%s. No sections found in the DB.", + lms_course.id, + ) + return + + # We'll create a new Canvas API client for the relevant install and instructor to fetch the sections. + canvas_service = canvas_api_client_factory( + None, + self._request, + application_instance=application_instance, + user_id=instructor.lti_user_id, + ) + # We'll take the token service from the client to refresh the token if needed. + # This is already scoped to the user and the install. + oauth2_token_service = canvas_service._client._oauth2_token_service # noqa: SLF001 + + # Fetch the sections and their students from the Canvas API. + api_sections = self._get_canvas_sections( + canvas_service, oauth2_token_service, lms_course, with_refresh_token=True + ) + if not api_sections: + LOG.info( + "Can't fetch roster for sections of course ID:%s. No sections found on the API.", + lms_course.id, + ) + return + api_sections_by_id = {str(section["id"]): section for section in api_sections} + + # The API doesn't send a LTI role, we'll pick a student one from the DB and use that + student_lti_role_id = self._db.scalar( + select(LTIRole.id) + .where(LTIRole.type == RoleType.LEARNER, LTIRole.scope == RoleScope.COURSE) + .order_by(LTIRole.id.asc()) + ) + + roster_upsert_elements = [] + db_course_users_by_lms_api_id = self._get_course_users(lms_course) + for db_section in db_sections: + api_section = api_sections_by_id.get(db_section.lms_id) + if not api_section: + LOG.debug( + "Skiping roster for section ID:%s. Not found on Canvas API", + db_section.lms_id, + ) + continue + + for student in api_section.get("students", []) or []: + db_student = db_course_users_by_lms_api_id.get(str(student["id"])) + if not db_student: + LOG.debug( + "Skiping roster entry for student ID:%s. Not found the DB", + student["id"], + ) + continue + + roster_upsert_elements.append( + { + "lms_segment_id": db_section.id, + "lms_user_id": db_student.id, + "lti_role_id": student_lti_role_id, + "active": True, + } + ) + + if not roster_upsert_elements: + LOG.info( + "No roster entries found for course ID:%s.", + lms_course.id, + ) + return + + # We'll first mark everyone as non-Active. + # We keep a record of who belonged to a section even if they are no longer present. + self._db.execute( + update(LMSSegmentRoster) + .where(LMSSegmentRoster.lms_segment_id.in_([s.id for s in db_sections])) + .values(active=False) + ) + + # Insert and update roster rows. + bulk_upsert( + self._db, + LMSSegmentRoster, + values=roster_upsert_elements, + index_elements=["lms_segment_id", "lms_user_id", "lti_role_id"], + update_columns=["active", "updated"], + ) + def _get_roster_users(self, roster, tool_consumer_instance_guid): values = [] for member in roster: @@ -373,21 +504,101 @@ def _get_roster_roles(self, roster) -> list[LTIRole]: roles = {role for member in roster for role in member["roles"]} return self._lti_role_service.get_roles(list(roles)) - def _get_lti_registration(self, lms_course) -> LTIRegistration: - lti_registration = self._db.scalars( - select(LTIRegistration) - .join(ApplicationInstance) - .where(LMSCourseApplicationInstance.lms_course_id == lms_course.id) + def _get_application_instance(self, lms_course) -> ApplicationInstance: + return self._db.scalars( + select(ApplicationInstance) .join(LMSCourseApplicationInstance) - .order_by(LTIRegistration.updated.desc()) + .where(LMSCourseApplicationInstance.lms_course_id == lms_course.id) + .order_by(LMSCourseApplicationInstance.updated.desc()) ).first() - assert lti_registration, "No LTI registration found for LMSCourse." - return lti_registration + + def _get_lti_registration(self, lms_course) -> LTIRegistration: + ai = self._get_application_instance(lms_course) + assert ai.lti_registration, "No LTI registration found for LMSCourse." + return ai.lti_registration + + def _get_course_instructor(self, lms_course: LMSCourse) -> LMSUser | None: + return self._db.scalars( + select(LMSUser) + .join(LMSCourseMembership) + .join(LTIRole) + .where( + LMSCourseMembership.lms_course_id == lms_course.id, + LTIRole.type == RoleType.INSTRUCTOR, + LTIRole.scope == RoleScope.COURSE, + ) + .order_by(LMSCourseMembership.updated.desc()) + ).first() + + def _get_canvas_sections( + self, + canvas_api_client: CanvasAPIClient, + oauth2_token_service: OAuth2TokenService, + lms_course: LMSCourse, + with_refresh_token=False, + ) -> list[dict]: + try: + return canvas_api_client.course_sections( + lms_course.lms_api_course_id, with_students=True + ) + except OAuth2TokenError as err: + if not with_refresh_token or not err.refreshable: + LOG.info( + "Failed to fetch sections for course %s, invalid API token", + lms_course.id, + ) + return [] + + if not self._refresh_canvas_token(canvas_api_client, oauth2_token_service): + LOG.info( + "Failed to fetch sections for course %s, error refreshing token", + lms_course.id, + ) + return [] + + return self._get_canvas_sections( + canvas_api_client, + oauth2_token_service, + lms_course, + with_refresh_token=False, + ) + + def _refresh_canvas_token( + self, canvas_service: CanvasAPIClient, oauth2_token_service + ) -> bool: + try: + refresh_token = oauth2_token_service.get().refresh_token + canvas_service.get_refreshed_token(refresh_token) + except (ConcurrentTokenRefreshError, CanvasAPIError): + return False + + return True + + def _get_course_users(self, lms_course: LMSCourse) -> dict[str, LMSUser]: + users_from_course_roster = ( + select(LMSUser) + .join(CourseRoster) + .where( + CourseRoster.lms_course_id == lms_course.id, + CourseRoster.active.is_(True), + ) + ) + users_from_launches = ( + select(LMSUser) + .join(LMSCourseMembership) + .where( + LMSCourseMembership.lms_course_id == lms_course.id, + ) + ) + users = self._db.execute( + union(users_from_course_roster, users_from_launches) + ).all() + return {u.lms_api_user_id: u for u in users} def factory(_context, request): return RosterService( - db=request.db, + request=request, lti_names_roles_service=request.find_service(LTINamesRolesService), lti_role_service=request.find_service(LTIRoleService), h_authority=request.registry.settings["h_authority"], diff --git a/tests/factories/lms_user.py b/tests/factories/lms_user.py index e37422c813..1443970ee1 100644 --- a/tests/factories/lms_user.py +++ b/tests/factories/lms_user.py @@ -8,5 +8,6 @@ models.LMSUser, FACTORY_CLASS=SQLAlchemyModelFactory, lti_user_id=USER_ID, + lms_api_user_id=USER_ID, h_userid=H_USERID, ) diff --git a/tests/unit/lms/services/canvas_api/client_test.py b/tests/unit/lms/services/canvas_api/client_test.py index 41c58ccd73..dc62e913a2 100644 --- a/tests/unit/lms/services/canvas_api/client_test.py +++ b/tests/unit/lms/services/canvas_api/client_test.py @@ -104,6 +104,28 @@ def test_course_sections(self, canvas_api_client, http_session): query={"per_page": Any.string()}, ) + def test_course_sections_with_students(self, canvas_api_client, http_session): + sections = [ + {"id": 101, "name": "name_1"}, + {"id": 102, "name": "name_2"}, + ] + sections_with_noise = [ + dict(section, unexpected="ignored") for section in sections + ] + + http_session.send.return_value = factories.requests.Response( + status_code=200, json_data=sections_with_noise + ) + + response = canvas_api_client.course_sections("COURSE_ID", with_students=True) + + assert response == sections + self.assert_session_send( + http_session, + "api/v1/courses/COURSE_ID/sections", + query={"per_page": Any.string(), "include[]": "students"}, + ) + def test_course_sections_deduplicates_sections( self, canvas_api_client, http_session ): diff --git a/tests/unit/lms/services/roster_test.py b/tests/unit/lms/services/roster_test.py index 6cc5e8cce9..6f8e295273 100644 --- a/tests/unit/lms/services/roster_test.py +++ b/tests/unit/lms/services/roster_test.py @@ -1,3 +1,4 @@ +import logging from datetime import datetime from unittest.mock import Mock, sentinel @@ -6,7 +7,14 @@ from sqlalchemy import select from lms.models import AssignmentRoster, CourseRoster, LMSSegmentRoster -from lms.services.exceptions import ExternalRequestError +from lms.models.lms_segment import LMSSegment +from lms.models.lms_user import LMSUser +from lms.models.lti_role import RoleScope, RoleType +from lms.services.exceptions import ( + CanvasAPIError, + ExternalRequestError, + OAuth2TokenError, +) from lms.services.roster import RosterService, factory from tests import factories @@ -369,6 +377,159 @@ def test_fetch_canvas_group_roster( assert roster[3].lms_user.lti_user_id == "USER_ID_INACTIVE" assert not roster[3].active + @pytest.mark.usefixtures("canvas_section") + def test_fetch_canvas_sections_roster_with_no_instructor_token( + self, svc, lms_course, caplog + ): + svc.fetch_canvas_sections_roster(lms_course) + + assert "No instructor found" in caplog.records[0].message + + @pytest.mark.usefixtures("instructor_in_course") + def test_fetch_canvas_sections_roster_with_no_db_sections( + self, svc, lms_course, caplog + ): + svc.fetch_canvas_sections_roster(lms_course) + + assert "No sections found in the DB" in caplog.records[0].message + + @pytest.mark.usefixtures("instructor_in_course", "canvas_section") + def test_fetch_canvas_sections_roster_with_no_api_sections( + self, svc, lms_course, caplog, canvas_api_client + ): + canvas_api_client.course_sections.return_value = [] + + svc.fetch_canvas_sections_roster(lms_course) + + assert "No sections found on the API" in caplog.records[0].message + + @pytest.mark.usefixtures("instructor_in_course", "canvas_section") + def test_fetch_canvas_sections_roster_with_nothing_to_insert( + self, svc, lms_course, caplog, canvas_api_client + ): + canvas_api_client.course_sections.return_value = [ + {"id": 10, "name": "Section 1", "students": []} + ] + + svc.fetch_canvas_sections_roster(lms_course) + + assert "No roster entries" in caplog.records[0].message + + def test_fetch_canvas_sections_roster( + self, + svc, + lms_course, + student_in_course, + canvas_api_client, + canvas_section, + canvas_api_client_factory, + db_session, + pyramid_request, + lti_v13_application_instance, + instructor_in_course, + ): + db_session.flush() + + canvas_api_client.course_sections.return_value = [ + { + "id": int(canvas_section.lms_id), + "name": "Section 1", + "students": [ + # Student that matches student_in_course, the student in the DB + {"id": student_in_course.lms_api_user_id}, + # Student that doesn't match any student in the DB + {"id": "SOME OTHER USER"}, + ], + }, + # Section we haven't seend before in teh DB + {"id": "SOME OTHER SECTION"}, + ] + + svc.fetch_canvas_sections_roster(lms_course) + + canvas_api_client_factory.assert_called_once_with( + None, + pyramid_request, + application_instance=lti_v13_application_instance, + user_id=instructor_in_course.lti_user_id, + ) + canvas_api_client.course_sections.assert_called_once_with( + lms_course.lms_api_course_id, with_students=True + ) + section_roster = db_session.scalars( + select(LMSUser) + .join(LMSSegmentRoster) + .join(LMSSegment) + .where(LMSSegment.id == canvas_section.id) + ).all() + assert section_roster == [student_in_course] + + @pytest.mark.usefixtures("instructor_in_course") + def test_fetch_canvas_sections_roster_needing_refresh( + self, + svc, + lms_course, + student_in_course, + canvas_api_client, + canvas_section, + db_session, + ): + db_session.flush() + + canvas_api_client.course_sections.side_effect = [ + OAuth2TokenError(refreshable=True), + [ + { + "id": int(canvas_section.lms_id), + "name": "Section 1", + "students": [ + # Student that matches student_in_course, the student in the DB + {"id": student_in_course.lms_api_user_id}, + ], + } + ], + ] + + svc.fetch_canvas_sections_roster(lms_course) + + section_roster = db_session.scalars( + select(LMSUser) + .join(LMSSegmentRoster) + .join(LMSSegment) + .where(LMSSegment.id == canvas_section.id) + ).all() + assert section_roster == [student_in_course] + + @pytest.mark.usefixtures("instructor_in_course", "canvas_section") + def test_fetch_canvas_sections_roster_failed_refresh( + self, svc, lms_course, canvas_api_client, db_session, caplog + ): + db_session.flush() + + canvas_api_client.course_sections.side_effect = OAuth2TokenError( + refreshable=True + ) + canvas_api_client.get_refreshed_token.side_effect = CanvasAPIError + + svc.fetch_canvas_sections_roster(lms_course) + + assert "error refreshing token" in caplog.records[0].message + + @pytest.mark.usefixtures("instructor_in_course", "canvas_section") + def test_fetch_canvas_sections_roster_with_invalid_token( + self, svc, lms_course, canvas_api_client, db_session, caplog + ): + db_session.flush() + + canvas_api_client.course_sections.side_effect = OAuth2TokenError( + refreshable=False + ) + canvas_api_client.get_refreshed_token.side_effect = CanvasAPIError + + svc.fetch_canvas_sections_roster(lms_course) + + assert "invalid API token" in caplog.records[0].message + @pytest.fixture def lms_course(self, lti_v13_application_instance): lms_course = factories.LMSCourse(lti_context_memberships_url="SERVICE_URL") @@ -378,6 +539,43 @@ def lms_course(self, lti_v13_application_instance): return lms_course + @pytest.fixture + def instructor_in_course(self, lms_course): + instructor = factories.LMSUser() + role = factories.LTIRole(type=RoleType.INSTRUCTOR, scope=RoleScope.COURSE) + factories.LMSCourseMembership( + lms_course=lms_course, lms_user=instructor, lti_role=role + ) + return instructor + + @pytest.fixture + def student_in_course(self, lms_course): + role = factories.LTIRole(type=RoleType.LEARNER, scope=RoleScope.COURSE) + student = factories.LMSUser() + factories.LMSCourseMembership( + lms_course=lms_course, lms_user=student, lti_role=role + ) + return student + + @pytest.fixture + def canvas_section(self, lms_course): + return factories.LMSSegment( + type="canvas_section", lms_course=lms_course, lms_id="1" + ) + + @pytest.fixture + def caplog(self, caplog): + caplog.set_level(logging.INFO) + return caplog + + @pytest.fixture(autouse=True) + def canvas_api_client_factory(self, patch): + return patch("lms.services.roster.canvas_api_client_factory") + + @pytest.fixture + def canvas_api_client(self, canvas_api_client_factory): + return canvas_api_client_factory.return_value + @pytest.fixture def assignment(self, lms_course): course = factories.Course( @@ -393,9 +591,9 @@ def names_and_roles_roster_response(self): ] @pytest.fixture - def svc(self, lti_names_roles_service, lti_role_service, db_session): + def svc(self, lti_names_roles_service, lti_role_service, pyramid_request): return RosterService( - db_session, + request=pyramid_request, lti_names_roles_service=lti_names_roles_service, lti_role_service=lti_role_service, h_authority="AUTHORITY", @@ -406,7 +604,6 @@ class TestFactory: def test_it( self, pyramid_request, - db_session, RosterService, lti_names_roles_service, lti_role_service, @@ -414,7 +611,7 @@ def test_it( service = factory(sentinel.context, pyramid_request) RosterService.assert_called_once_with( - db=db_session, + request=pyramid_request, lti_names_roles_service=lti_names_roles_service, lti_role_service=lti_role_service, h_authority=pyramid_request.registry.settings["h_authority"],