Skip to content

Commit

Permalink
Use the generic UserService.get_users instead of Assignment.get_members
Browse files Browse the repository at this point in the history
get_users already has most of the filters we need, added h_userids
filter to support the new view query parameter.
  • Loading branch information
marcospri committed Jul 3, 2024
1 parent 5e678ba commit 5a9b5fa
Show file tree
Hide file tree
Showing 7 changed files with 77 additions and 90 deletions.
29 changes: 1 addition & 28 deletions lms/services/assignment.py
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
import logging

from sqlalchemy import Select, select
from sqlalchemy.orm import Session, joinedload
from sqlalchemy.orm import Session

from lms.models import (
Assignment,
AssignmentGrouping,
AssignmentMembership,
Grouping,
LTIRole,
RoleScope,
RoleType,
User,
)
from lms.services.upsert import bulk_upsert
Expand Down Expand Up @@ -211,31 +209,6 @@ def is_member(self, assignment: Assignment, h_userid: str) -> bool:
assignment.membership.join(User).filter(User.h_userid == h_userid).first()
)

def get_members(
self,
assignment,
role_type: RoleType,
role_scope: RoleScope,
h_userids: list[str] | None = None,
) -> list[User]:
"""Get a list of users that are member of assignment.
:params: assignment to get members of.
:params: role_scope: only return members with this role scope.
:params: role_type: only return members with this role type.
:params: h_userids only return users whose h_userid is in this list.
"""
query = (
assignment.membership.options(joinedload(AssignmentMembership.user))
.join(LTIRole)
.where(LTIRole.scope == role_scope, LTIRole.type == role_type)
)

if h_userids:
query = query.join(User).where(User.h_userid._in(h_userids))

return [member.user for member in query]

def get_assignments(
self, h_userid: str | None = None, course_id: int | None = None
) -> Select[tuple[Assignment]]:
Expand Down
7 changes: 6 additions & 1 deletion lms/services/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,12 +102,13 @@ def _user_search_query(self, application_instance_id, user_id) -> Select:

return query

def get_users( # noqa: PLR0913
def get_users( # noqa: PLR0913, PLR0917
self,
role_scope: RoleScope,
role_type: RoleType,
instructor_h_userid: str | None = None,
course_id: str | None = None,
h_userids: list[str] | None = None,
assignment_id: str | None = None,
) -> Select[tuple[User]]:
"""
Expand All @@ -116,6 +117,7 @@ def get_users( # noqa: PLR0913
:param role_scope: return only users with this LTI role scope.
:param role_type: return only users with this LTI role type.
:param instructor_h_userid: return only users that belongs to courses/assignments where the user instructor_h_userid is an instructor.
:param h_userids: return only users with a h_userid in this list.
:param course_id: return only users that belong to course_id.
:param assignment_id: return only users that belong to assignment_id.
"""
Expand All @@ -140,6 +142,9 @@ def get_users( # noqa: PLR0913
)
)

if h_userids:
query = query.where(User.h_userid.in_(h_userids))

if course_id:
query = query.join(
AssignmentGrouping,
Expand Down
14 changes: 10 additions & 4 deletions lms/views/dashboard/api/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,19 @@ def students_metrics(self) -> APIStudents:
stats_by_user = {s["userid"]: s for s in stats}
students: list[APIStudent] = []

# Iterate over all the students we have in the DB
for user in self.assignment_service.get_members(
assignment,
users_query = self.user_service.get_users(
role_scope=RoleScope.COURSE,
role_type=RoleType.LEARNER,
assignment_id=assignment.id,
# Users the current user has access to see
instructor_h_userid=self.request.user.h_userid
if self.request.user
else None,
# Users the current user requested
h_userids=request_h_userids,
):
)
# Iterate over all the students we have in the DB
for user in self.request.db.scalars(users_query).all():
if s := stats_by_user.get(user.h_userid):
# We seen this student in H, get all the data from there
students.append(
Expand Down
21 changes: 0 additions & 21 deletions tests/unit/lms/services/assignment_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -239,27 +239,6 @@ def test_is_member(self, svc, db_session):
assert svc.is_member(assignment, user.h_userid)
assert not svc.is_member(assignment, other_user.h_userid)

def test_get_members(self, svc, db_session):
factories.User() # User not in assignment
assignment = factories.Assignment()
user = factories.User()
lti_role = factories.LTIRole(scope=RoleScope.COURSE)
factories.AssignmentMembership.create(
assignment=assignment, user=user, lti_role=lti_role
)
# User in assignment with other role
factories.AssignmentMembership.create(
assignment=assignment,
user=factories.User(),
lti_role=factories.LTIRole(scope=RoleScope.SYSTEM),
)

db_session.flush()

assert svc.get_members(
assignment, role_scope=lti_role.scope, role_type=lti_role.type
) == [user]

def test_get_assignments(self, svc, db_session):
assert db_session.scalars(svc.get_assignments()).all()

Expand Down
90 changes: 57 additions & 33 deletions tests/unit/lms/services/user_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,32 +95,40 @@ def test_get_users(self, service, db_session):

assert db_session.scalars(query).all() == [student]

def test_get_users_by_course_id(self, service, db_session):
assignment = factories.Assignment()
course = factories.Course()
student = factories.User()
factories.User(h_userid=student.h_userid) # Duplicated student
teacher = factories.User()
def test_get_users_by_h_userids(
self, service, db_session, student_in_assigment, assignment
):
other_student = factories.User()
factories.AssignmentMembership.create(
assignment=assignment,
user=student,
user=other_student,
lti_role=factories.LTIRole(scope=RoleScope.COURSE, type=RoleType.LEARNER),
)
factories.AssignmentMembership.create(
assignment=assignment,
user=teacher,
lti_role=factories.LTIRole(
scope=RoleScope.COURSE, type=RoleType.INSTRUCTOR
),
# Make sure we have in fact two users
assert db_session.scalars(
service.get_users(role_scope=RoleScope.COURSE, role_type=RoleType.LEARNER)
).all() == [student_in_assigment, other_student]

query = service.get_users(
role_scope=RoleScope.COURSE,
role_type=RoleType.LEARNER,
h_userids=[other_student.h_userid],
)

assert db_session.scalars(query).all() == [other_student]

def test_get_users_by_course_id(
self, service, db_session, student_in_assigment, assignment
):
course = factories.Course()
factories.AssignmentGrouping(assignment=assignment, grouping=course)
db_session.flush()

query = service.get_users(
role_scope=RoleScope.COURSE, role_type=RoleType.LEARNER, course_id=course.id
)

assert db_session.scalars(query).all() == [student]
assert db_session.scalars(query).all() == [student_in_assigment]

def test_get_users_by_assigment_id(self, service, db_session):
assignment = factories.Assignment()
Expand Down Expand Up @@ -149,23 +157,13 @@ def test_get_users_by_assigment_id(self, service, db_session):

assert db_session.scalars(query).all() == [student]

def test_get_users_by_h_userid(self, service, db_session):
# Assignment the h_userid belongs to as a teacher
assignment = factories.Assignment()
student = factories.User()
teacher = factories.User()
factories.AssignmentMembership.create(
assignment=assignment,
user=student,
lti_role=factories.LTIRole(scope=RoleScope.COURSE, type=RoleType.LEARNER),
)
factories.AssignmentMembership.create(
assignment=assignment,
user=teacher,
lti_role=factories.LTIRole(
scope=RoleScope.COURSE, type=RoleType.INSTRUCTOR
),
)
def test_get_users_by_instructor_h_userid(
self,
service,
db_session,
student_in_assigment,
teacher_in_assigment,
):
# Assignment the h_userid does not belong to
other_assignment = factories.Assignment()
other_student = factories.User()
Expand All @@ -178,10 +176,36 @@ def test_get_users_by_h_userid(self, service, db_session):
query = service.get_users(
role_scope=RoleScope.COURSE,
role_type=RoleType.LEARNER,
instructor_h_userid=teacher.h_userid,
instructor_h_userid=teacher_in_assigment.h_userid,
)

assert db_session.scalars(query).all() == [student]
assert db_session.scalars(query).all() == [student_in_assigment]

@pytest.fixture
def assignment(self):
return factories.Assignment()

@pytest.fixture
def student_in_assigment(self, assignment):
student = factories.User()
factories.AssignmentMembership.create(
assignment=assignment,
user=student,
lti_role=factories.LTIRole(scope=RoleScope.COURSE, type=RoleType.LEARNER),
)
return student

@pytest.fixture
def teacher_in_assigment(self, assignment):
teacher = factories.User()
factories.AssignmentMembership.create(
assignment=assignment,
user=teacher,
lti_role=factories.LTIRole(
scope=RoleScope.COURSE, type=RoleType.INSTRUCTOR
),
)
return teacher

@pytest.fixture
def user(self, lti_user, application_instance):
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/lms/views/dashboard/api/course_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def test_get_organization_courses(
org = factories.Organization()
courses = factories.Course.create_batch(5)
dashboard_service.get_request_organization.return_value = org
course_service.get_courses.return_value = select(Course)
course_service.get_courses.return_value = select(Course).order_by(Course.id)
pyramid_request.matchdict["organization_public_id"] = sentinel.public_id
db_session.flush()

Expand Down
4 changes: 2 additions & 2 deletions tests/unit/lms/views/dashboard/api/user_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def test_get_students(self, user_service, pyramid_request, views, get_page):
}

def test_students_metrics(
self, views, pyramid_request, assignment_service, h_api, dashboard_service
self, views, pyramid_request, user_service, h_api, dashboard_service
):
# User returned by the stats endpoint
student = factories.User(display_name="Bart")
Expand All @@ -64,7 +64,7 @@ def test_students_metrics(
"h_userids": sentinel.h_userids,
}
assignment = factories.Assignment()
assignment_service.get_members.return_value = [
user_service.get_users.return_value = [
student,
student_no_annos,
student_no_annos_no_name,
Expand Down

0 comments on commit 5a9b5fa

Please sign in to comment.