Expose course rosters on the dashboard

Although we don't expose a list of students with metrics for a full course
we do list students of a course on the dropdowns.

This commit uses the roster data for the dropdown when the feature flag is enabled
and the data is aviailable.

lms/services/dashboard.py

@@ -1,11 +1,12 @@
 from datetime import datetime
 
 from pyramid.httpexceptions import HTTPNotFound, HTTPUnauthorized
-from sqlalchemy import Select, select, union
+from sqlalchemy import Select, select, true, union
 
 from lms.models import (
     ApplicationInstance,
     Assignment,
+    Course,
     LMSCourse,
     LMSCourseApplicationInstance,
     LMSCourseMembership,
@@ -65,7 +66,7 @@ def get_request_assignment(self, request, assigment_id: int) -> Assignment:
 
         return assignment
 
-    def get_request_course(self, request, course_id: int):
+    def get_request_course(self, request, course_id: int) -> Course:
         """Get and authorize a course for the given request."""
         course = self._course_service.get_by_id(course_id)
         if not course:
@@ -210,7 +211,40 @@ def get_assignment_roster(
                 assignment_id=assignment.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)
+            ).add_columns(true())
+
+        # Always return the results, no matter the source, sorted
+        return roster_last_updated, query.order_by(LMSUser.display_name, LMSUser.id)
+
+    def get_course_roster(
+        self, lms_course: LMSCourse, h_userids: list[str] | None = None
+    ) -> tuple[datetime | None, Select[tuple[LMSUser, bool]]]:
+        rosters_enabled = lms_course.course.application_instance.settings.get(
+            "dashboard", "rosters"
+        )
+        roster_last_updated = self._roster_service.course_roster_last_updated(
+            lms_course
+        )
+        if rosters_enabled and roster_last_updated:
+            # If rostering is enabled and we do have the data, use it
+            query = self._roster_service.get_course_roster(
+                lms_course,
+                role_scope=RoleScope.COURSE,
+                role_type=RoleType.LEARNER,
+                h_userids=h_userids,
+            )
+
+        else:
+            # If we are not going to return data from the roster, don't return the last updated date
+            roster_last_updated = None
+            # Always fallback to fetch users that have launched the assignment at some point
+            query = self._user_service.get_users_for_course(
+                role_scope=RoleScope.COURSE,
+                role_type=RoleType.LEARNER,
+                course_id=lms_course.course.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 roster_last_updated, query.order_by(LMSUser.display_name, LMSUser.id)

lms/services/roster.py

@@ -46,34 +46,45 @@ def get_course_roster(
         lms_course: LMSCourse,
         role_scope: RoleScope | None = None,
         role_type: RoleType | None = None,
-    ) -> Select[tuple[LMSUser]]:
+        h_userids: list[str] | None = None,
+    ) -> Select[tuple[LMSUser, bool]]:
         """Get the roster information for a course from our DB."""
         roster_query = (
-            select(CourseRoster.lms_user_id)
-            .join(LTIRole)
-            .where(
-                CourseRoster.lms_course_id == lms_course.id,
-                CourseRoster.active.is_(True),
-            )
-        )
+            select(LMSUser, CourseRoster.active)
+            .join(LMSUser, CourseRoster.lms_user_id == LMSUser.id)
+            .join(LTIRole, CourseRoster.lti_role_id == LTIRole.id)
+            .where(CourseRoster.lms_course_id == lms_course.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)
 
-        return select(LMSUser).where(LMSUser.id.in_(roster_query))
+        if h_userids:
+            roster_query = roster_query.where(LMSUser.h_userid.in_(h_userids))
+
+        return roster_query
 
     def assignment_roster_last_updated(self, assignment: Assignment) -> datetime | None:
-        """Return the roster's last updated timestamp for given assignment, or None if we don't have roster data."""
+        """Return the roster's last updated timestamp for a given assignment, or None if we don't have roster data."""
         return self._db.scalar(
             select(AssignmentRoster.updated)
             .where(AssignmentRoster.assignment_id == assignment.id)
             .order_by(AssignmentRoster.updated.desc())
             .limit(1)
         )
 
+    def course_roster_last_updated(self, course: LMSCourse) -> datetime | None:
+        """Return the roster's last updated timestamp for a given course, or None if we don't have roster data."""
+        return self._db.scalar(
+            select(CourseRoster.updated)
+            .where(CourseRoster.lms_course_id == course.id)
+            .order_by(CourseRoster.updated.desc())
+            .limit(1)
+        )
+
     def get_assignment_roster(
         self,
         assignment: Assignment,

lms/services/user.py

@@ -164,7 +164,7 @@ def get_users_for_assignment(
         role_type: RoleType,
         assignment_id: int,
         h_userids: list[str] | None = None,
-    ):
+    ) -> Select[tuple[LMSUser]]:
         """Get the users that belong to one assignment."""
         query = (
             select(LMSUser)
@@ -193,7 +193,7 @@ def get_users_for_course(
         role_type: RoleType,
         course_id: int,
         h_userids: list[str] | None = None,
-    ):
+    ) -> Select[tuple[LMSUser]]:
         """Get the users that belong to one course."""
         query = (
             select(LMSUser)

lms/views/dashboard/api/user.py

@@ -1,5 +1,6 @@
 import logging
 from datetime import datetime
+from typing import TYPE_CHECKING
 
 from marshmallow import fields, validate
 from pyramid.view import view_config
@@ -23,6 +24,9 @@
 
 LOG = logging.getLogger(__name__)
 
+if TYPE_CHECKING:
+    from lms.services.dashboard import DashboardService
+
 
 class ListUsersSchema(PaginationParametersMixin):
     """Query parameters to fetch a list of users."""
@@ -70,7 +74,9 @@ class UserViews:
     def __init__(self, request) -> None:
         self.request = request
         self.assignment_service = request.find_service(name="assignment")
-        self.dashboard_service = request.find_service(name="dashboard")
+        self.dashboard_service: DashboardService = request.find_service(
+            name="dashboard"
+        )
         self.h_api: HAPI = request.find_service(HAPI)
         self.user_service: UserService = request.find_service(UserService)
         self.auto_grading_service: AutoGradingService = request.find_service(
@@ -232,12 +238,8 @@ def _students_query(
             course = self.dashboard_service.get_request_course(
                 self.request, course_id=course_ids[0]
             )
-
-            return None, self.user_service.get_users_for_course(
-                role_scope=RoleScope.COURSE,
-                role_type=RoleType.LEARNER,
-                course_id=course.id,
-                h_userids=h_userids,
+            return self.dashboard_service.get_course_roster(
+                lms_course=course.lms_course, h_userids=h_userids
             )
 
         admin_organizations = self.dashboard_service.get_request_admin_organizations(

tests/unit/lms/services/dashboard_test.py

@@ -219,7 +219,7 @@ def test_get_request_admin_organizations_for_staff(
 
     @pytest.mark.parametrize("rosters_enabled", [True, False])
     @pytest.mark.parametrize("roster_available", [True, False])
-    def test_get_assignment_roster_with_roster_disabled(
+    def test_get_assignment_roster(
         self,
         svc,
         application_instance,
@@ -265,6 +265,53 @@ def test_get_assignment_roster_with_roster_disabled(
                 == roster_service.get_assignment_roster.return_value.order_by.return_value
             )
 
+    @pytest.mark.parametrize("rosters_enabled", [True, False])
+    @pytest.mark.parametrize("roster_available", [True, False])
+    def test_get_course_roster(
+        self,
+        svc,
+        application_instance,
+        user_service,
+        roster_service,
+        rosters_enabled,
+        roster_available,
+    ):
+        application_instance.settings.set("dashboard", "rosters", rosters_enabled)
+        lms_course = factories.LMSCourse(
+            course=factories.Course(application_instance=application_instance)
+        )
+        if not roster_available:
+            roster_service.course_roster_last_updated.return_value = None
+
+        last_updated, roster = svc.get_course_roster(lms_course, sentinel.h_userids)
+
+        if not roster_available or not rosters_enabled:
+            user_service.get_users_for_course.assert_called_once_with(
+                role_scope=RoleScope.COURSE,
+                role_type=RoleType.LEARNER,
+                course_id=lms_course.course.id,
+                h_userids=sentinel.h_userids,
+            )
+            assert not last_updated
+            assert (
+                roster
+                == user_service.get_users_for_course.return_value.add_columns.return_value.order_by.return_value
+            )
+        else:
+            roster_service.get_course_roster.assert_called_once_with(
+                lms_course,
+                role_scope=RoleScope.COURSE,
+                role_type=RoleType.LEARNER,
+                h_userids=sentinel.h_userids,
+            )
+            assert (
+                last_updated == roster_service.course_roster_last_updated.return_value
+            )
+            assert (
+                roster
+                == roster_service.get_course_roster.return_value.order_by.return_value
+            )
+
     @pytest.fixture()
     def svc(
         self,

tests/unit/lms/services/roster_test.py

@@ -34,10 +34,39 @@ def test_assignment_roster_last_updated(
 
         assert svc.assignment_roster_last_updated(assignment) == expected
 
+    @pytest.mark.parametrize(
+        "create_roster,expected",
+        [(True, datetime(2021, 1, 1)), (False, None)],
+    )
+    def test_course_roster_last_updated(
+        self, svc, lms_course, db_session, create_roster, expected
+    ):
+        lms_user = factories.LMSUser()
+        lti_role = factories.LTIRole()
+
+        if create_roster:
+            factories.CourseRoster(
+                updated=datetime(2021, 1, 1),
+                lms_user=lms_user,
+                lms_course=lms_course,
+                lti_role=lti_role,
+                active=True,
+            )
+        db_session.flush()
+
+        assert svc.course_roster_last_updated(lms_course) == expected
+
     @pytest.mark.parametrize("with_role_scope", [True, False])
     @pytest.mark.parametrize("with_role_type", [True, False])
+    @pytest.mark.parametrize("with_h_userids", [True, False])
     def test_get_course_roster(
-        self, svc, lms_course, db_session, with_role_scope, with_role_type
+        self,
+        svc,
+        lms_course,
+        db_session,
+        with_role_scope,
+        with_role_type,
+        with_h_userids,
     ):
         lms_user = factories.LMSUser()
         inactive_lms_user = factories.LMSUser()
@@ -57,13 +86,18 @@ def test_get_course_roster(
         )
         db_session.flush()
 
-        assert db_session.scalars(
+        result = db_session.execute(
             svc.get_course_roster(
                 lms_course,
                 role_scope=lti_role.scope if with_role_scope else None,
                 role_type=lti_role.type if with_role_type else None,
+                h_userids=[lms_user.h_userid, inactive_lms_user.h_userid]
+                if with_h_userids
+                else None,
             )
-        ).all() == [lms_user]
+        ).all()
+
+        assert [(lms_user, True), (inactive_lms_user, False)] == result
 
     @pytest.mark.parametrize("with_role_scope", [True, False])
     @pytest.mark.parametrize("with_role_type", [True, False])

tests/unit/lms/views/dashboard/api/user_test.py

@@ -319,10 +319,8 @@ def test__students_query_single_course(
         dashboard_service.get_request_course.assert_called_once_with(
             pyramid_request, sentinel.course_id
         )
-        user_service.get_users_for_course.assert_called_once_with(
-            role_scope=RoleScope.COURSE,
-            role_type=RoleType.LEARNER,
-            course_id=dashboard_service.get_request_course.return_value.id,
+        dashboard_service.get_course_roster.assert_called_once_with(
+            lms_course=dashboard_service.get_request_course.return_value.lms_course,
             h_userids=None,
         )