From 4d5d4dec33c4cfa16379af8c41f13b3d514c5b05 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Wed, 11 Dec 2024 12:10:57 +0100 Subject: [PATCH 1/2] Return the actual value for Roster.last_updated when available Return the greatest date we have for a given roster or None for cases where we still base the roster data on launches. --- lms/services/dashboard.py | 12 ++++++----- lms/services/roster.py | 20 +++++++++++-------- lms/views/dashboard/api/user.py | 17 ++++++++-------- tests/unit/lms/services/dashboard_test.py | 12 +++++++---- tests/unit/lms/services/roster_test.py | 4 +++- .../unit/lms/views/dashboard/api/user_test.py | 6 ++++-- 6 files changed, 42 insertions(+), 29 deletions(-) diff --git a/lms/services/dashboard.py b/lms/services/dashboard.py index 4e2783598a..6a26cbff96 100644 --- a/lms/services/dashboard.py +++ b/lms/services/dashboard.py @@ -1,3 +1,5 @@ +from datetime import datetime + from pyramid.httpexceptions import HTTPNotFound, HTTPUnauthorized from sqlalchemy import Select, select, union @@ -178,16 +180,16 @@ def get_request_admin_organizations(self, request) -> list[Organization]: def get_assignment_roster( self, assignment: Assignment, h_userids: list[str] | None = None - ) -> Select[tuple[LMSUser, bool]]: + ) -> tuple[datetime | None, Select[tuple[LMSUser, bool]]]: rosters_enabled = ( assignment.course and assignment.course.application_instance.settings.get( "dashboard", "rosters" ) ) - if rosters_enabled and self._roster_service.assignment_roster_exists( - assignment - ): + roster_last_updated = self._roster_service.assignment_roster_exists(assignment) + + if rosters_enabled and roster_last_updated: # If rostering is enabled and we do have the data, use it query = self._roster_service.get_assignment_roster( assignment, @@ -207,7 +209,7 @@ def get_assignment_roster( ).add_columns(True) # Always return the results, no matter the source, sorted - return query.order_by(LMSUser.display_name, LMSUser.id) + return roster_last_updated, query.order_by(LMSUser.display_name, LMSUser.id) def factory(_context, request): diff --git a/lms/services/roster.py b/lms/services/roster.py index be8503298d..e37aca76be 100644 --- a/lms/services/roster.py +++ b/lms/services/roster.py @@ -1,3 +1,4 @@ +from datetime import datetime from logging import getLogger from sqlalchemy import Select, func, select, text, update @@ -64,14 +65,17 @@ def get_course_roster( return select(LMSUser).where(LMSUser.id.in_(roster_query)) - def assignment_roster_exists(self, assignment: Assignment) -> bool: - """Check if we have roster data for the given assignment.""" - return bool( - self._db.scalar( - select(AssignmentRoster) - .where(AssignmentRoster.assignment_id == assignment.id) - .limit(1) - ) + def assignment_roster_exists(self, assignment: Assignment) -> datetime | None: + """ + Check if we have roster data for the given assignment. + + In case we have roster data, return the last updated timestamp, None otherwise. + """ + return self._db.scalar( + select(AssignmentRoster.updated) + .where(AssignmentRoster.assignment_id == assignment.id) + .order_by(AssignmentRoster.updated.desc()) + .limit(1) ) def get_assignment_roster( diff --git a/lms/views/dashboard/api/user.py b/lms/views/dashboard/api/user.py index ebea699f4a..79edbbcd1b 100644 --- a/lms/views/dashboard/api/user.py +++ b/lms/views/dashboard/api/user.py @@ -3,6 +3,7 @@ from marshmallow import fields, validate from pyramid.view import view_config +from sqlalchemy import Select from lms.js_config_types import ( AnnotationMetrics, @@ -84,7 +85,7 @@ def __init__(self, request) -> None: schema=ListUsersSchema, ) def students(self) -> APIStudents: - students_query = self._students_query( + _, students_query = self._students_query( assignment_ids=self.request.parsed_params.get("assignment_ids"), segment_authority_provided_ids=self.request.parsed_params.get( "segment_authority_provided_ids" @@ -141,7 +142,7 @@ def students_metrics(self) -> APIRoster: stats_by_user = {s["userid"]: s for s in stats} students: list[RosterEntry] = [] - users_query = self._students_query( + roster_last_updated, users_query = self._students_query( assignment_ids=[assignment.id], segment_authority_provided_ids=request_segment_authority_provided_ids, h_userids=request_h_userids, @@ -179,9 +180,7 @@ def students_metrics(self) -> APIRoster: if assignment.auto_grading_config: students = self._add_auto_grading_data(assignment, students) - # We are not exposing the roster info here yet, just making the API changes to better coordinate with the frontend - # For now we mark every roster entry as active and we don't include any last_activity. - return APIRoster(students=students, last_updated=None) + return APIRoster(students=students, last_updated=roster_last_updated) def _add_auto_grading_data( self, assignment: Assignment, api_students: list[RosterEntry] @@ -211,7 +210,7 @@ def _students_query( assignment_ids: list[int], segment_authority_provided_ids: list[str], h_userids: list[str] | None = None, - ): + ) -> tuple[datetime | None, Select]: course_ids = self.request.parsed_params.get("course_ids") # Single assigment fetch if ( @@ -234,7 +233,7 @@ def _students_query( self.request, course_id=course_ids[0] ) - return self.user_service.get_users_for_course( + return None, self.user_service.get_users_for_course( role_scope=RoleScope.COURSE, role_type=RoleType.LEARNER, course_id=course.id, @@ -246,7 +245,7 @@ def _students_query( ) # Full organization fetch if not course_ids and not assignment_ids and not segment_authority_provided_ids: - return self.user_service.get_users_for_organization( + return None, self.user_service.get_users_for_organization( role_scope=RoleScope.COURSE, role_type=RoleType.LEARNER, h_userids=h_userids, @@ -257,7 +256,7 @@ def _students_query( admin_organization_ids=[org.id for org in admin_organizations], ) - return self.user_service.get_users( + return None, self.user_service.get_users( role_scope=RoleScope.COURSE, role_type=RoleType.LEARNER, course_ids=self.request.parsed_params.get("course_ids"), diff --git a/tests/unit/lms/services/dashboard_test.py b/tests/unit/lms/services/dashboard_test.py index 102cb934c5..59287c0c6b 100644 --- a/tests/unit/lms/services/dashboard_test.py +++ b/tests/unit/lms/services/dashboard_test.py @@ -218,13 +218,14 @@ def test_get_request_admin_organizations_for_staff( assert svc.get_request_admin_organizations(pyramid_request) == [organization] def test_get_assignment_roster_with_roster_disabled( - self, svc, application_instance, user_service + self, svc, application_instance, user_service, roster_service ): assignment = factories.Assignment( course=factories.Course(application_instance=application_instance) ) + roster_service.assignment_roster_exists.return_value = None - roster = svc.get_assignment_roster(assignment, sentinel.h_userids) + last_updated, roster = svc.get_assignment_roster(assignment, sentinel.h_userids) user_service.get_users_for_assignment.assert_called_once_with( role_scope=RoleScope.COURSE, @@ -232,12 +233,14 @@ def test_get_assignment_roster_with_roster_disabled( assignment_id=assignment.id, h_userids=sentinel.h_userids, ) + assert ( roster == user_service.get_users_for_assignment.return_value.add_columns.return_value.order_by.return_value ) + assert not last_updated - def test_get_assignment_roster_with( + def test_get_assignment_roster_with_roster_enabled( self, svc, application_instance, roster_service ): application_instance.settings.set("dashboard", "rosters", True) @@ -245,7 +248,7 @@ def test_get_assignment_roster_with( course=factories.Course(application_instance=application_instance) ) - roster = svc.get_assignment_roster(assignment, sentinel.h_userids) + last_updated, roster = svc.get_assignment_roster(assignment, sentinel.h_userids) roster_service.get_assignment_roster.assert_called_once_with( assignment, @@ -257,6 +260,7 @@ def test_get_assignment_roster_with( roster == roster_service.get_assignment_roster.return_value.order_by.return_value ) + assert last_updated == roster_service.assignment_roster_exists.return_value @pytest.fixture() def svc( diff --git a/tests/unit/lms/services/roster_test.py b/tests/unit/lms/services/roster_test.py index 508e9f83be..a872cfe79f 100644 --- a/tests/unit/lms/services/roster_test.py +++ b/tests/unit/lms/services/roster_test.py @@ -1,3 +1,4 @@ +from datetime import datetime from unittest.mock import Mock, sentinel import pytest @@ -13,7 +14,7 @@ class TestRosterService: @pytest.mark.parametrize( "create_roster,expected", - [(True, True), (False, False)], + [(True, datetime(2021, 1, 1)), (False, None)], ) def test_assignment_roster_exists( self, svc, assignment, db_session, create_roster, expected @@ -23,6 +24,7 @@ def test_assignment_roster_exists( if create_roster: factories.AssignmentRoster( + updated=datetime(2021, 1, 1), lms_user=lms_user, assignment=assignment, lti_role=lti_role, diff --git a/tests/unit/lms/views/dashboard/api/user_test.py b/tests/unit/lms/views/dashboard/api/user_test.py index db19747d7c..51ae7dcd50 100644 --- a/tests/unit/lms/views/dashboard/api/user_test.py +++ b/tests/unit/lms/views/dashboard/api/user_test.py @@ -111,6 +111,7 @@ def test_students_metrics( else: db_session.flush() dashboard_service.get_assignment_roster.return_value = ( + None, select(User) .where( User.id.in_( @@ -124,7 +125,7 @@ def test_students_metrics( ] ) ) - .add_columns(True) + .add_columns(True), ) db_session.flush() @@ -217,6 +218,7 @@ def test_students_metrics_with_auto_grading( db_session.flush() dashboard_service.get_assignment_roster.return_value = ( + None, select(User) .where( User.id.in_( @@ -226,7 +228,7 @@ def test_students_metrics_with_auto_grading( ] ) ) - .add_columns(True) + .add_columns(True), ) dashboard_service.get_request_assignment.return_value = assignment h_api.get_annotation_counts.return_value = annotation_counts_response From 684450aba6ad72f7ae28653d43e0c7ca646defdc Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Mon, 16 Dec 2024 11:12:48 +0100 Subject: [PATCH 2/2] WIP --- lms/services/dashboard.py | 40 +++++++++++++++++++++++++++++- lms/services/roster.py | 43 ++++++++++++++++++++++++++++++++- lms/services/segment.py | 8 ++++++ lms/views/dashboard/api/user.py | 10 ++++++++ 4 files changed, 99 insertions(+), 2 deletions(-) diff --git a/lms/services/dashboard.py b/lms/services/dashboard.py index 6a26cbff96..b9d4d61e34 100644 --- a/lms/services/dashboard.py +++ b/lms/services/dashboard.py @@ -17,13 +17,14 @@ ) from lms.models.dashboard_admin import DashboardAdmin from lms.security import Permissions -from lms.services import OrganizationService, RosterService, UserService +from lms.services import OrganizationService, RosterService, UserService, SegmentService class DashboardService: def __init__( # noqa: PLR0913, PLR0917 self, request, + segment_service: SegmentService, assignment_service, course_service, roster_service: RosterService, @@ -33,6 +34,7 @@ def __init__( # noqa: PLR0913, PLR0917 ): self._db = request.db + self._segment_service = segment_service self._assignment_service = assignment_service self._course_service = course_service self._roster_service = roster_service @@ -211,10 +213,46 @@ def get_assignment_roster( # Always return the results, no matter the source, sorted return roster_last_updated, query.order_by(LMSUser.display_name, LMSUser.id) + def get_segment_roster( + self, authority_provided_id: str, h_userids: list[str] | None = None + ) -> Select[tuple[LMSUser, bool]]: + """Return a query that fetches the roster for a segment.""" + segment = self._segment_service.get_segment(authority_provided_id) + rosters_enabled = ( + segment + and segment.lms_course + and segment.lms_course.course.application_instance.settings.get( + "dashboard", "rosters" + ) + ) + roster_last_updated = self._roster_service.segment_roster_exists(segment) + if rosters_enabled and roster_last_updated: + # If rostering is enabled and we do have the data, use it + query = self._roster_service.get_segment_roster( + segment, + role_scope=RoleScope.COURSE, + role_type=RoleType.LEARNER, + h_userids=h_userids, + ) + + else: + # Always fallback to fetch users that have launched the assignment at some point + query = self._user_service.get_users_for_segment( + role_scope=RoleScope.COURSE, + role_type=RoleType.LEARNER, + segment_id=segment.id, + h_userids=h_userids, + # For launch data we always add the "active" column as true for compatibility with the roster query. + ).add_columns(True) + + # Always return the results, no matter the source, sorted + return query.order_by(LMSUser.display_name, LMSUser.id) + def factory(_context, request): return DashboardService( request=request, + segment_service=request.find_service(SegmentService), assignment_service=request.find_service(name="assignment"), course_service=request.find_service(name="course"), organization_service=request.find_service(OrganizationService), diff --git a/lms/services/roster.py b/lms/services/roster.py index e37aca76be..fdfa43d292 100644 --- a/lms/services/roster.py +++ b/lms/services/roster.py @@ -78,6 +78,19 @@ def assignment_roster_exists(self, assignment: Assignment) -> datetime | None: .limit(1) ) + def segment_roster_exists(self, segment: LMSSegment) -> datetime | None: + """ + Check if we have roster data for the given segment. + + In case we have roster data, return the last updated timestamp, None otherwise. + """ + return self._db.scalar( + select(LMSSegmentRoster.updated) + .where(LMSSegmentRoster.lms_segment_id == segment.id) + .order_by(LMSSegmentRoster.updated.desc()) + .limit(1) + ) + def get_assignment_roster( self, assignment: Assignment, @@ -85,7 +98,7 @@ def get_assignment_roster( role_type: RoleType | None = None, h_userids: list[str] | None = None, ) -> Select[tuple[LMSUser, bool]]: - """Get the roster information for a course from our DB.""" + """Get the roster information for an assignment from our DB.""" roster_query = ( select(LMSUser, AssignmentRoster.active) .join(LMSUser, AssignmentRoster.lms_user_id == LMSUser.id) @@ -104,6 +117,34 @@ def get_assignment_roster( return roster_query + def get_segment_roster( + self, + segment: LMSSegment, + role_scope: RoleScope | None = None, + role_type: RoleType | None = None, + h_userids: list[str] | None = None, + ) -> Select[tuple[LMSUser, bool]]: + """Get the roster information for a segment from our DB.""" + + roster_query = ( + (select(LMSUser, LMSSegmentRoster.active).join(LMSUser)) + .join(LMSSegmentRoster, LMSSegmentRoster.lms_user_id == LMSUser.id) + .join(LTIRole, LTIRole.id == LMSSegmentRoster.lti_role_id) + .where(LMSSegmentRoster.lms_segment_id == segment.id) + .distinct() + ) + + if role_scope: + roster_query = roster_query.where(LTIRole.scope == role_scope) + + if role_type: + roster_query = roster_query.where(LTIRole.type == role_type) + + if h_userids: + roster_query = roster_query.where(LMSUser.h_userid.in_(h_userids)) + + return roster_query + def fetch_course_roster(self, lms_course: LMSCourse) -> None: """Fetch the roster information for a course from the LMS.""" assert ( diff --git a/lms/services/segment.py b/lms/services/segment.py index 20ac68b5ae..435fe869b0 100644 --- a/lms/services/segment.py +++ b/lms/services/segment.py @@ -17,6 +17,14 @@ def __init__(self, db, group_set_service: GroupSetService): self._db = db self._group_set_service = group_set_service + def get_segment(self, authority_provided_id: str) -> LMSSegment | None: + """Get a segment by its authority_provided_id.""" + return ( + self._db.query(LMSSegment) + .filter_by(h_authority_provided_id=authority_provided_id) + .one_or_none() + ) + def upsert_segments( self, course: LMSCourse, diff --git a/lms/views/dashboard/api/user.py b/lms/views/dashboard/api/user.py index 79edbbcd1b..f9edddea7b 100644 --- a/lms/views/dashboard/api/user.py +++ b/lms/views/dashboard/api/user.py @@ -212,6 +212,16 @@ def _students_query( h_userids: list[str] | None = None, ) -> tuple[datetime | None, Select]: course_ids = self.request.parsed_params.get("course_ids") + + # Single segment fetch + if segment_authority_provided_ids and len(segment_authority_provided_ids) == 1: + # TODO SEGMETN CHECK + + return self.dashboard_service.get_segment_roster( + segment_authority_provided_id=segment_authority_provided_ids, + h_userids=h_userids, + ) + # Single assigment fetch if ( assignment_ids