From d265985416aeaf75f7aee10a29378c0f78298a49 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Mon, 18 Nov 2024 16:22:44 +0100 Subject: [PATCH] Use custom queries for getting users for course, assignments and organizations This queries are different from the existing `get_users` one in a few different ways: - They use LMSUser / LMSCourse instead of grouping and user. This avoid the de duplication step needed on the older tables. - Use one query per case instead of one generic one. While this leads to more code duplication the generic query has some inefficiencies trying to cater to all cases. We still don't handle filter by segments and cases where we filter by multiple courses or assignments. Those will more complex and we'll tackle them after including the roster concept on the existing one. --- lms/services/dashboard.py | 29 ++++- lms/services/user.py | 103 ++++++++++++++++ lms/views/dashboard/api/user.py | 42 +++++++ tests/unit/lms/services/dashboard_test.py | 18 +++ tests/unit/lms/services/user_test.py | 110 +++++++++++++++--- .../unit/lms/views/dashboard/api/user_test.py | 70 ++++++++--- 6 files changed, 335 insertions(+), 37 deletions(-) diff --git a/lms/services/dashboard.py b/lms/services/dashboard.py index 9e65dd99b7..05b97d8ff8 100644 --- a/lms/services/dashboard.py +++ b/lms/services/dashboard.py @@ -36,9 +36,20 @@ def __init__( # noqa: PLR0913 def get_request_assignment(self, request) -> Assignment: """Get and authorize an assignment for the given request.""" - assigment_id = request.matchdict.get( - "assignment_id" - ) or request.parsed_params.get("assignment_id") + # Requests that are scoped to one assignment on the URL parameter + assigment_id = request.matchdict.get("assignment_id") + if not assigment_id: + # Request that are scoped to a single assignment but as a query parameter + assigment_id = request.parsed_params.get("assignment_id") + + if ( + not assigment_id + and request.parsed_params.get("assignment_ids") + and len(request.parsed_params["assignment_ids"]) == 1 + ): + # Request that take a list of assignments, but we only recieved one, the requests is scoped to that one assignment + assigment_id = request.parsed_params["assignment_ids"][0] + assignment = self._assignment_service.get_by_id(assigment_id) if not assignment: raise HTTPNotFound() @@ -64,7 +75,17 @@ def get_request_assignment(self, request) -> Assignment: def get_request_course(self, request): """Get and authorize a course for the given request.""" - course = self._course_service.get_by_id(request.matchdict["course_id"]) + # Requests that are scoped to one course on the URL parameter + course_id = request.matchdict.get("course_id") + if ( + not course_id + and request.parsed_params.get("course_ids") + and len(request.parsed_params["course_ids"]) == 1 + ): + # Request that take a list of courses, but we only recieved one, the requests is scoped to that one course + course_id = request.parsed_params["course_ids"][0] + + course = self._course_service.get_by_id(course_id) if not course: raise HTTPNotFound() diff --git a/lms/services/user.py b/lms/services/user.py index 4ef89a09b5..8054a4cc94 100644 --- a/lms/services/user.py +++ b/lms/services/user.py @@ -9,8 +9,11 @@ AssignmentMembership, Grouping, GroupingMembership, + LMSCourse, + LMSCourseMembership, LMSUser, LMSUserApplicationInstance, + LMSUserAssignmentMembership, LTIParams, LTIRole, LTIUser, @@ -139,6 +142,106 @@ def _user_search_query(self, application_instance_id, user_id) -> Select: return query + def get_users_for_assignment( + self, + role_scope: RoleScope, + role_type: RoleType, + assignment_id: int, + h_userids: list[str] | None = None, + ): + """Get the users that belong to one assignment.""" + query = ( + select(LMSUser) + .distinct() + .join( + LMSUserAssignmentMembership, + LMSUserAssignmentMembership.lms_user_id == LMSUser.id, + ) + .where( + LMSUserAssignmentMembership.assignment_id == assignment_id, + LMSUserAssignmentMembership.lti_role_id.in_( + select(LTIRole.id).where( + LTIRole.scope == role_scope, LTIRole.type == role_type + ) + ), + ) + ) + if h_userids: + query = query.where(LMSUser.h_userid.in_(h_userids)) + + return query.order_by(LMSUser.display_name, LMSUser.id) + + def get_users_for_course( + self, + role_scope: RoleScope, + role_type: RoleType, + course_id: int, + h_userids: list[str] | None = None, + ): + """Get the users that belong to one course.""" + query = ( + select(LMSUser) + .distinct() + .join( + LMSCourseMembership, + LMSCourseMembership.lms_user_id == LMSUser.id, + ) + .join(LMSCourse, LMSCourse.id == LMSCourseMembership.lms_course_id) + # course_id is the PK on Grouping, we need to join with LMSCourse by authority_provided_id + .join( + Grouping, + Grouping.authority_provided_id == LMSCourse.h_authority_provided_id, + ) + .where( + Grouping.id == course_id, + LMSCourseMembership.lti_role_id.in_( + select(LTIRole.id).where( + LTIRole.scope == role_scope, LTIRole.type == role_type + ) + ), + ) + ) + if h_userids: + query = query.where(LMSUser.h_userid.in_(h_userids)) + + return query.order_by(LMSUser.display_name, LMSUser.id) + + def get_users_for_organization( # noqa: PLR0913 + self, + role_scope: RoleScope, + role_type: RoleType, + instructor_h_userid: str | None = None, + admin_organization_ids: list[int] | None = None, + h_userids: list[str] | None = None, + ) -> Select[tuple[LMSUser]]: + candidate_courses = CourseService.courses_permission_check_query( + instructor_h_userid, admin_organization_ids, course_ids=None + ).cte("candidate_courses") + + query = ( + select(LMSUser) + .distinct() + .join(LMSCourseMembership, LMSCourseMembership.lms_user_id == LMSUser.id) + .join(LMSCourse, LMSCourseMembership.lms_course_id == LMSCourse.id) + .join( + Grouping, + Grouping.authority_provided_id == LMSCourse.h_authority_provided_id, + ) + .join(candidate_courses, candidate_courses.c[0] == Grouping.id) + .where( + LMSCourseMembership.lti_role_id.in_( + select(LTIRole.id).where( + LTIRole.scope == role_scope, LTIRole.type == role_type + ) + ) + ) + ) + + if h_userids: + query = query.where(LMSUser.h_userid.in_(h_userids)) + + return query.order_by(LMSUser.display_name, LMSUser.id) + def get_users( # noqa: PLR0913, PLR0917 self, role_scope: RoleScope, diff --git a/lms/views/dashboard/api/user.py b/lms/views/dashboard/api/user.py index 73fb30115e..01afa18fb7 100644 --- a/lms/views/dashboard/api/user.py +++ b/lms/views/dashboard/api/user.py @@ -203,9 +203,51 @@ def _students_query( segment_authority_provided_ids: list[str], h_userids: list[str] | None = None, ): + course_ids = self.request.parsed_params.get("course_ids") + # Single assigment fetch + if ( + assignment_ids + and len(assignment_ids) == 1 + and not segment_authority_provided_ids + ): + # Fetch the assignment to be sure the current user has access to it. + assignment = self.dashboard_service.get_request_assignment(self.request) + + return self.user_service.get_users_for_assignment( + role_scope=RoleScope.COURSE, + role_type=RoleType.LEARNER, + assignment_id=assignment.id, + h_userids=h_userids, + ) + + # Single course fetch + if course_ids and len(course_ids) == 1 and not segment_authority_provided_ids: + # Fetch the course to be sure the current user has access to it. + course = self.dashboard_service.get_request_course(self.request) + + return self.user_service.get_users_for_course( + role_scope=RoleScope.COURSE, + role_type=RoleType.LEARNER, + course_id=course.id, + h_userids=h_userids, + ) + admin_organizations = self.dashboard_service.get_request_admin_organizations( self.request ) + # 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( + role_scope=RoleScope.COURSE, + role_type=RoleType.LEARNER, + h_userids=h_userids, + # Users the current user has access to see + instructor_h_userid=self.request.user.h_userid + if self.request.user + else None, + admin_organization_ids=[org.id for org in admin_organizations], + ) + return self.user_service.get_users( role_scope=RoleScope.COURSE, role_type=RoleType.LEARNER, diff --git a/tests/unit/lms/services/dashboard_test.py b/tests/unit/lms/services/dashboard_test.py index 8c16077554..b33acf5cf7 100644 --- a/tests/unit/lms/services/dashboard_test.py +++ b/tests/unit/lms/services/dashboard_test.py @@ -66,6 +66,15 @@ def test_get_request_assignment_for_admin( assert svc.get_request_assignment(pyramid_request) + def test_get_request_assignment_for_parsed_params_assignment_id( + self, pyramid_request, assignment_service, svc + ): + pyramid_request.parsed_params = {"assignment_ids": [sentinel.parsed_params_id]} + + svc.get_request_assignment(pyramid_request) + + assignment_service.get_by_id.assert_called_once_with(sentinel.parsed_params_id) + def test_get_request_course_404( self, pyramid_request, @@ -94,6 +103,15 @@ def test_get_request_course_for_staff( assert svc.get_request_course(pyramid_request) + def test_get_request_for_parsed_params_course_ids( + self, pyramid_request, course_service, svc + ): + pyramid_request.parsed_params = {"course_ids": [sentinel.parsed_params_id]} + + svc.get_request_course(pyramid_request) + + course_service.get_by_id.assert_called_once_with(sentinel.parsed_params_id) + def test_get_request_course_for_admin( self, pyramid_request, diff --git a/tests/unit/lms/services/user_test.py b/tests/unit/lms/services/user_test.py index 95fe48651b..990946cefe 100644 --- a/tests/unit/lms/services/user_test.py +++ b/tests/unit/lms/services/user_test.py @@ -90,9 +90,9 @@ def test_get_users( service, db_session, organization, - student_in_assigment, + student_in_assignment, ): - factories.User(h_userid=student_in_assigment.h_userid) # Duplicated student + factories.User(h_userid=student_in_assignment.h_userid) # Duplicated student query = service.get_users( role_scope=RoleScope.COURSE, @@ -100,10 +100,10 @@ def test_get_users( admin_organization_ids=[organization.id], ) - assert db_session.scalars(query).all() == [student_in_assigment] + assert db_session.scalars(query).all() == [student_in_assignment] def test_get_users_by_h_userids( - self, service, db_session, student_in_assigment, assignment, organization + self, service, db_session, student_in_assignment, assignment, organization ): other_student = factories.User( application_instance=organization.application_instances[0] @@ -121,7 +121,7 @@ def test_get_users_by_h_userids( role_type=RoleType.LEARNER, admin_organization_ids=[organization.id], ) - ).all() == [student_in_assigment, other_student] + ).all() == [student_in_assignment, other_student] query = service.get_users( role_scope=RoleScope.COURSE, @@ -134,7 +134,7 @@ def test_get_users_by_h_userids( @pytest.mark.usefixtures("assignment") def test_get_users_by_course_id( - self, service, db_session, student_in_assigment, course, organization + self, service, db_session, student_in_assignment, course, organization ): query = service.get_users( role_scope=RoleScope.COURSE, @@ -143,13 +143,29 @@ def test_get_users_by_course_id( admin_organization_ids=[organization.id], ) - assert db_session.scalars(query).all() == [student_in_assigment] + assert db_session.scalars(query).all() == [student_in_assignment] + + @pytest.mark.usefixtures("assignment") + @pytest.mark.parametrize("with_h_userids", [True, False]) + def test_get_users_for_course( + self, service, db_session, student_in_assignment, course, with_h_userids + ): + query = service.get_users_for_course( + role_scope=RoleScope.COURSE, + role_type=RoleType.LEARNER, + course_id=course.id, + h_userids=[student_in_assignment.h_userid] if with_h_userids else None, + ) + + assert [s.h_userid for s in db_session.scalars(query).all()] == [ + student_in_assignment.h_userid + ] @pytest.mark.usefixtures("teacher_in_assigment") def test_get_users_by_assigment_id( - self, service, db_session, student_in_assigment, assignment, organization + self, service, db_session, student_in_assignment, assignment, organization ): - factories.User(h_userid=student_in_assigment.h_userid) # Duplicated student + factories.User(h_userid=student_in_assignment.h_userid) # Duplicated student db_session.flush() query = service.get_users( @@ -159,11 +175,26 @@ def test_get_users_by_assigment_id( admin_organization_ids=[organization.id], ) - assert db_session.scalars(query).all() == [student_in_assigment] + assert db_session.scalars(query).all() == [student_in_assignment] + + @pytest.mark.parametrize("with_h_userids", [True, False]) + def test_get_users_for_assignment( + self, service, db_session, student_in_assignment, assignment, with_h_userids + ): + query = service.get_users_for_assignment( + role_scope=RoleScope.COURSE, + role_type=RoleType.LEARNER, + assignment_id=assignment.id, + h_userids=[student_in_assignment.h_userid] if with_h_userids else None, + ) + + assert [s.h_userid for s in db_session.scalars(query).all()] == [ + student_in_assignment.h_userid + ] @pytest.mark.usefixtures("assignment", "course") def test_get_users_by_instructor_h_userid( - self, service, db_session, student_in_assigment, teacher_in_assigment + self, service, db_session, student_in_assignment, teacher_in_assigment ): # Assignment in another course other_assignment = factories.Assignment(course=factories.Course()) @@ -181,15 +212,36 @@ def test_get_users_by_instructor_h_userid( instructor_h_userid=teacher_in_assigment.h_userid, ) - assert db_session.scalars(query).all() == [student_in_assigment] + assert db_session.scalars(query).all() == [student_in_assignment] + + @pytest.mark.usefixtures("assignment", "course") + @pytest.mark.parametrize("with_h_userids", [True, False]) + def test_get_users_for_organization( + self, + service, + db_session, + student_in_assignment, + teacher_in_assigment, + with_h_userids, + ): + query = service.get_users_for_organization( + role_scope=RoleScope.COURSE, + role_type=RoleType.LEARNER, + instructor_h_userid=teacher_in_assigment.h_userid, + h_userids=[student_in_assignment.h_userid] if with_h_userids else None, + ) + + assert [s.h_userid for s in db_session.scalars(query).all()] == [ + student_in_assignment.h_userid + ] @pytest.mark.usefixtures("teacher_in_assigment") def test_get_users_by_segment_authority_provided_id( - self, service, db_session, student_in_assigment, assignment, organization + self, service, db_session, student_in_assignment, assignment, organization ): - factories.User(h_userid=student_in_assigment.h_userid) # Duplicated student + factories.User(h_userid=student_in_assignment.h_userid) # Duplicated student segment = factories.CanvasSection() - factories.GroupingMembership(user=student_in_assigment, grouping=segment) + factories.GroupingMembership(user=student_in_assignment, grouping=segment) db_session.flush() @@ -201,11 +253,15 @@ def test_get_users_by_segment_authority_provided_id( segment_authority_provided_ids=[segment.authority_provided_id], ) - assert db_session.scalars(query).all() == [student_in_assigment] + assert db_session.scalars(query).all() == [student_in_assignment] @pytest.fixture def course(self, application_instance, db_session): course = factories.Course(application_instance=application_instance) + lms_course = factories.LMSCourse( + h_authority_provided_id=course.authority_provided_id + ) + course.lms_course = lms_course db_session.flush() return course @@ -218,8 +274,11 @@ def assignment(self, course): return assignment @pytest.fixture - def student_in_assigment(self, assignment, application_instance): + def student_in_assignment(self, assignment, application_instance, db_session): student = factories.User(application_instance=application_instance) + lms_user = factories.LMSUser( + lti_user_id=student.user_id, h_userid=student.h_userid + ) factories.AssignmentMembership.create( assignment=assignment, user=student, @@ -229,11 +288,24 @@ def student_in_assigment(self, assignment, application_instance): grouping=assignment.course, user=student, ) + factories.LMSCourseMembership.create( + lms_course=assignment.course.lms_course, + lms_user=lms_user, + lti_role=factories.LTIRole(scope=RoleScope.COURSE, type=RoleType.LEARNER), + ) + factories.LMSUserAssignmentMembership.create( + assignment=assignment, + lms_user=lms_user, + lti_role=factories.LTIRole(scope=RoleScope.COURSE, type=RoleType.LEARNER), + ) + db_session.flush() + return student @pytest.fixture def teacher_in_assigment(self, assignment): teacher = factories.User() + factories.LMSUser(lti_user_id=teacher.user_id, h_userid=teacher.h_userid) factories.AssignmentMembership.create( assignment=assignment, user=teacher, @@ -245,12 +317,14 @@ def teacher_in_assigment(self, assignment): @pytest.fixture def user(self, lti_user, application_instance): - return factories.User( + user = factories.User( application_instance=application_instance, user_id=lti_user.user_id, h_userid=lti_user.h_user.userid("authority.example.com"), roles="old_roles", ) + factories.LMSUser(lti_user_id=user.user_id) + return user @pytest.fixture def service(self, db_session): diff --git a/tests/unit/lms/views/dashboard/api/user_test.py b/tests/unit/lms/views/dashboard/api/user_test.py index 64b5f94a8c..bba115f5fc 100644 --- a/tests/unit/lms/views/dashboard/api/user_test.py +++ b/tests/unit/lms/views/dashboard/api/user_test.py @@ -21,8 +21,8 @@ class TestUserViews: def test_get_students(self, user_service, pyramid_request, views, get_page): pyramid_request.parsed_params = { - "course_ids": sentinel.course_ids, - "assignment_ids": sentinel.assignment_ids, + "course_ids": [sentinel.course_id_1, sentinel.course_id_2], + "assignment_ids": [sentinel.assignment_id_1, sentinel.assignment_id_2], "segment_authority_provided_ids": sentinel.segment_authority_provided_ids, } students = factories.User.create_batch(5) @@ -35,8 +35,8 @@ def test_get_students(self, user_service, pyramid_request, views, get_page): role_type=RoleType.LEARNER, instructor_h_userid=pyramid_request.user.h_userid, admin_organization_ids=[], - course_ids=sentinel.course_ids, - assignment_ids=sentinel.assignment_ids, + course_ids=[sentinel.course_id_1, sentinel.course_id_2], + assignment_ids=[sentinel.assignment_id_1, sentinel.assignment_id_2], segment_authority_provided_ids=sentinel.segment_authority_provided_ids, h_userids=None, ) @@ -92,20 +92,34 @@ def test_students_metrics( pyramid_request.parsed_params["segment_authority_provided_ids"] = [ g.authority_provided_id for g in segments ] + user_service.get_users.return_value = select(User).where( + User.id.in_( + [ + u.id + for u in [student, student_no_annos, student_no_annos_no_name] + ] + ) + ) + else: + db_session.flush() + user_service.get_users_for_assignment.return_value = select(User).where( + User.id.in_( + [ + u.id + for u in [student, student_no_annos, student_no_annos_no_name] + ] + ) + ) db_session.flush() - user_service.get_users.return_value = select(User).where( - User.id.in_( - [u.id for u in [student, student_no_annos, student_no_annos_no_name]] - ) - ) + dashboard_service.get_request_assignment.return_value = assignment h_api.get_annotation_counts.return_value = annotation_counts_response response = views.students_metrics() - dashboard_service.get_request_assignment.assert_called_once_with( - pyramid_request + dashboard_service.get_request_assignment.assert_has_calls( + [call(pyramid_request)] ) h_api.get_annotation_counts.assert_called_once_with( [g.authority_provided_id for g in assignment.groupings], @@ -169,7 +183,6 @@ def test_students_metrics_with_auto_grading( student_no_annos_no_name = factories.User(display_name=None) pyramid_request.parsed_params = { - "assignment_id": sentinel.id, "h_userids": sentinel.h_userids, } assignment = factories.Assignment(course=factories.Course()) @@ -183,7 +196,7 @@ def test_students_metrics_with_auto_grading( auto_grading_service.get_last_grades.return_value = {} db_session.flush() - user_service.get_users.return_value = select(User).where( + user_service.get_users_for_assignment.return_value = select(User).where( User.id.in_( [u.id for u in [student, student_no_annos, student_no_annos_no_name]] ) @@ -193,8 +206,8 @@ def test_students_metrics_with_auto_grading( response = views.students_metrics() - dashboard_service.get_request_assignment.assert_called_once_with( - pyramid_request + dashboard_service.get_request_assignment.assert_has_calls( + [call(pyramid_request), call(pyramid_request)] ) h_api.get_annotation_counts.assert_called_once_with( [g.authority_provided_id for g in assignment.groupings], @@ -257,8 +270,35 @@ def test_students_metrics_with_auto_grading( assert response == expected + def test__students_query_single_course( + self, views, pyramid_request, dashboard_service, user_service + ): + pyramid_request.parsed_params = {"course_ids": [sentinel.course_id]} + + views._students_query(assignment_ids=None, segment_authority_provided_ids=None) # noqa: SLF001 + + dashboard_service.get_request_course.assert_called_once_with(pyramid_request) + 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, + h_userids=None, + ) + + def test__students_query_organization(self, views, user_service, pyramid_request): + views._students_query(assignment_ids=None, segment_authority_provided_ids=None) # noqa: SLF001 + + user_service.get_users_for_organization.assert_called_once_with( + role_scope=RoleScope.COURSE, + role_type=RoleType.LEARNER, + h_userids=None, + instructor_h_userid=pyramid_request.user.h_userid, + admin_organization_ids=[], + ) + @pytest.fixture def views(self, pyramid_request): + pyramid_request.parsed_params = {} return UserViews(pyramid_request) @pytest.fixture