From 503fa0ca35dfade055332aa2c7da509ad8e184c6 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Wed, 11 Dec 2024 12:10:57 +0100 Subject: [PATCH] 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