From 322475e4c1d519198cf83cbda837768f509541cf Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Mon, 2 Sep 2024 18:20:10 +0200 Subject: [PATCH] Fetch assignment rosters Same process as for course rosters, schedule fetch rosters for assignments with recent launches and no recent roster in the DB. --- lms/models/grouping.py | 1 + lms/services/roster.py | 94 ++++++++++++-- lms/tasks/roster.py | 100 +++++++++++++-- tests/factories/__init__.py | 2 +- .../factories/{course_roster.py => roster.py} | 3 + tests/unit/lms/services/roster_test.py | 115 +++++++++++++---- tests/unit/lms/tasks/roster_test.py | 116 ++++++++++++++++-- 7 files changed, 375 insertions(+), 56 deletions(-) rename tests/factories/{course_roster.py => roster.py} (65%) diff --git a/lms/models/grouping.py b/lms/models/grouping.py index f5527acd76..820ad8c826 100644 --- a/lms/models/grouping.py +++ b/lms/models/grouping.py @@ -196,6 +196,7 @@ class Course(Grouping): lms_course: Mapped[LMSCourse] = sa.orm.relationship( LMSCourse, primaryjoin="Grouping.authority_provided_id == foreign(LMSCourse.h_authority_provided_id)", + backref=sa.orm.backref("course"), ) def set_group_sets(self, group_sets: list[dict]): diff --git a/lms/services/roster.py b/lms/services/roster.py index 7216437c5e..72d06b1589 100644 --- a/lms/services/roster.py +++ b/lms/services/roster.py @@ -4,6 +4,8 @@ from lms.models import ( ApplicationInstance, + Assignment, + AssignmentRoster, CourseRoster, LMSCourse, LMSCourseApplicationInstance, @@ -33,19 +35,11 @@ def __init__( self._lti_role_service = lti_role_service self._h_authority = h_authority - def fetch_roster(self, lms_course: LMSCourse) -> None: + def fetch_course_roster(self, lms_course: LMSCourse) -> None: assert ( lms_course.lti_context_memberships_url ), "Trying fetch roster for course without service URL." - - lti_registration = self._db.scalars( - select(LTIRegistration) - .join(ApplicationInstance) - .where(LMSCourseApplicationInstance.lms_course_id == lms_course.id) - .join(LMSCourseApplicationInstance) - .order_by(LTIRegistration.updated.desc()) - ).first() - assert lti_registration, "No LTI registration found for LMSCourse." + lti_registration = self._get_lti_registration(lms_course) roster = self._lti_names_roles_service.get_context_memberships( lti_registration, lms_course.lti_context_memberships_url @@ -97,6 +91,75 @@ def fetch_roster(self, lms_course: LMSCourse) -> None: update_columns=["active", "updated"], ) + def fetch_assignment_roster(self, assignment: Assignment) -> None: + assert ( + assignment.lti_v13_resource_link_id + ), "Trying fetch roster for an assignment without LTI1.3 ID." + + lms_course = self._db.scalars( + select(LMSCourse).where( + LMSCourse.h_authority_provided_id + == assignment.course.authority_provided_id + ) + ).one() + + assert ( + lms_course.lti_context_memberships_url + ), "Trying fetch roster for course without service URL." + lti_registration = self._get_lti_registration(lms_course) + + roster = self._lti_names_roles_service.get_context_memberships( + lti_registration, + lms_course.lti_context_memberships_url, + resource_link_id=assignment.lti_v13_resource_link_id, + ) + + # Insert any users we might be missing in the DB + lms_users_by_lti_user_id = { + u.lti_user_id: u + for u in self._get_roster_users( + roster, lms_course.tool_consumer_instance_guid + ) + } + # Also insert any roles we might be missing + lti_roles_by_value: dict[str, LTIRole] = { + r.value: r for r in self._get_roster_roles(roster) + } + + # Make sure any new rows have IDs + self._db.flush() + + roster_upsert_elements = [] + + for member in roster: + lti_user_id = member.get("lti11_legacy_user_id") or member["user_id"] + # Now, for every user + role, insert a row in the roster table + for role in member["roles"]: + roster_upsert_elements.append( + { + "assignment_id": assignment.id, + "lms_user_id": lms_users_by_lti_user_id[lti_user_id].id, + "lti_role_id": lti_roles_by_value[role].id, + "active": member["status"] == "Active", + } + ) + # We'll first mark everyone as non-Active. + # We keep a record of who belonged to a course even if they are no longer present. + self._db.execute( + update(AssignmentRoster) + .where(AssignmentRoster.assignment_id == assignment.id) + .values(active=False) + ) + + # Insert and update roster rows. + bulk_upsert( + self._db, + AssignmentRoster, + values=roster_upsert_elements, + index_elements=["assignment_id", "lms_user_id", "lti_role_id"], + update_columns=["active", "updated"], + ) + def _get_roster_users(self, roster, tool_consumer_instance_guid): values = [] for member in roster: @@ -134,6 +197,17 @@ def _get_roster_roles(self, roster) -> list[LTIRole]: roles = {role for member in roster for role in member["roles"]} return self._lti_role_service.get_roles(list(roles)) + def _get_lti_registration(self, lms_course) -> LTIRegistration: + lti_registration = self._db.scalars( + select(LTIRegistration) + .join(ApplicationInstance) + .where(LMSCourseApplicationInstance.lms_course_id == lms_course.id) + .join(LMSCourseApplicationInstance) + .order_by(LTIRegistration.updated.desc()) + ).first() + assert lti_registration, "No LTI registration found for LMSCourse." + return lti_registration + def factory(_context, request): return RosterService( diff --git a/lms/tasks/roster.py b/lms/tasks/roster.py index 7d324d3b66..134c17c3a7 100644 --- a/lms/tasks/roster.py +++ b/lms/tasks/roster.py @@ -4,22 +4,37 @@ from sqlalchemy import exists, select -from lms.models import Course, CourseRoster, Event, LMSCourse +from lms.models import ( + Assignment, + AssignmentRoster, + Course, + CourseRoster, + Event, + LMSCourse, +) from lms.services.roster import RosterService from lms.tasks.celery import app -COURSE_LAUNCHED_WINDOW = timedelta(hours=24) -"""How recent we need to have seen a launch from a course before we stop fetching rosters for it.""" +LAUNCHED_WINDOW = timedelta(hours=24) +"""How recent we need to have seen a launch from a course/assignment before we stop fetching rosters for it.""" ROSTER_REFRESH_WINDOW = timedelta(hours=24 * 7) -"""How frequenly should we fetch roster for the same course""" +"""How frequenly should we fetch roster for the same course/assignment""" -ROSTER_COURSE_LIMIT = 5 +ROSTER_LIMIT = 5 """How many roster should we fetch per executing of the schedule task.""" @app.task() def schedule_fetching_rosters() -> None: + # Schedule fetching course rosters + schedule_fetching_course_rosters() + + # Schedule fetching assignment rosters + schedule_fetching_assignment_rosters() + + +def schedule_fetching_course_rosters() -> None: """Schedule fetching course rosters based on their last lunches and the most recent roster fetch.""" # We use the python version (and not func.now()) for easier mocking during tests @@ -38,7 +53,7 @@ def schedule_fetching_rosters() -> None: select(Event) .join(Course, Event.course_id == Course.id) .where( - Event.timestamp >= now - COURSE_LAUNCHED_WINDOW, + Event.timestamp >= now - LAUNCHED_WINDOW, Course.authority_provided_id == LMSCourse.h_authority_provided_id, ) ) @@ -53,11 +68,58 @@ def schedule_fetching_rosters() -> None: no_recent_roster_clause, recent_launches_cluase, ) - # Schedule only a few roster per call to this method - .limit(ROSTER_COURSE_LIMIT) + # Schedule only a few rosters per call to this method + .limit(ROSTER_LIMIT) ) for lms_course_id in request.db.scalars(query).all(): - fetch_roster.delay(lms_course_id=lms_course_id) + fetch_course_roster.delay(lms_course_id=lms_course_id) + + +def schedule_fetching_assignment_rosters() -> None: + """Schedule fetching assignment rosters based on their last lunches and the most recent roster fetch.""" + + # We use the python version (and not func.now()) for easier mocking during tests + now = datetime.now() + + # Only fetch roster for assignments that don't have recent roster information + no_recent_roster_clause = ~exists( + select(AssignmentRoster).where( + AssignmentRoster.assignment_id == Assignment.id, + AssignmentRoster.updated >= now - ROSTER_REFRESH_WINDOW, + ) + ) + + # Only fetch rosters for assignments that have been recently launched + recent_launches_cluase = exists( + select(Event) + .join(Assignment, Event.assignment_id == Assignment.id) + .where( + Event.timestamp >= now - LAUNCHED_WINDOW, + ) + ) + + with app.request_context() as request: + with request.tm: + query = ( + select(Assignment.id) + .join(Course) + .join( + LMSCourse, + LMSCourse.h_authority_provided_id == Course.authority_provided_id, + ) + .where( + # Assignments that belongs to courses for which we have a LTIA membership service URL + LMSCourse.lti_context_memberships_url.is_not(None), + # Assignments for which we have stored the LTI1.3 ID + Assignment.lti_v13_resource_link_id.is_not(None), + no_recent_roster_clause, + recent_launches_cluase, + ) + # Schedule only a few roster per call to this method + .limit(ROSTER_LIMIT) + ) + for assignment_id in request.db.scalars(query).all(): + fetch_assignment_roster.delay(assignment_id=assignment_id) @app.task( @@ -67,10 +129,26 @@ def schedule_fetching_rosters() -> None: retry_backoff=3600, retry_backoff_max=7200, ) -def fetch_roster(*, lms_course_id) -> None: +def fetch_course_roster(*, lms_course_id) -> None: """Fetch the roster for one course.""" with app.request_context() as request: roster_service = request.find_service(RosterService) with request.tm: lms_course = request.db.get(LMSCourse, lms_course_id) - roster_service.fetch_roster(lms_course) + roster_service.fetch_course_roster(lms_course) + + +@app.task( + acks_late=True, + autoretry_for=(Exception,), + max_retries=2, + retry_backoff=3600, + retry_backoff_max=7200, +) +def fetch_assignment_roster(*, assignment_id) -> None: + """Fetch the roster for one course.""" + with app.request_context() as request: + roster_service = request.find_service(RosterService) + with request.tm: + assignment = request.db.get(Assignment, assignment_id) + roster_service.fetch_assignment_roster(assignment) diff --git a/tests/factories/__init__.py b/tests/factories/__init__.py index aaaa9528d7..b9174bff64 100644 --- a/tests/factories/__init__.py +++ b/tests/factories/__init__.py @@ -18,7 +18,6 @@ TOOL_CONSUMER_INSTANCE_GUID, USER_ID, ) -from tests.factories.course_roster import CourseRoster from tests.factories.dashboard_admin import DashboardAdmin from tests.factories.event import Event from tests.factories.file import File @@ -37,6 +36,7 @@ from tests.factories.oauth2_token import OAuth2Token from tests.factories.organization import Organization from tests.factories.organization_usage import OrganizationUsageReport +from tests.factories.roster import AssignmentRoster, CourseRoster from tests.factories.rsa_key import RSAKey from tests.factories.task_done import TaskDone from tests.factories.user import User diff --git a/tests/factories/course_roster.py b/tests/factories/roster.py similarity index 65% rename from tests/factories/course_roster.py rename to tests/factories/roster.py index 9097905a4f..7e9d414a60 100644 --- a/tests/factories/course_roster.py +++ b/tests/factories/roster.py @@ -4,3 +4,6 @@ from lms import models CourseRoster = make_factory(models.CourseRoster, FACTORY_CLASS=SQLAlchemyModelFactory) +AssignmentRoster = make_factory( + models.AssignmentRoster, FACTORY_CLASS=SQLAlchemyModelFactory +) diff --git a/tests/unit/lms/services/roster_test.py b/tests/unit/lms/services/roster_test.py index ed0d571176..99bc7acc30 100644 --- a/tests/unit/lms/services/roster_test.py +++ b/tests/unit/lms/services/roster_test.py @@ -4,13 +4,13 @@ from h_matchers import Any from sqlalchemy import select -from lms.models import CourseRoster +from lms.models import AssignmentRoster, CourseRoster from lms.services.roster import RosterService, factory from tests import factories class TestLTINameRolesServices: - def test_fetch_roster( + def test_fetch_course_roster( self, svc, lti_names_roles_service, @@ -18,12 +18,9 @@ def test_fetch_roster( db_session, names_and_roles_roster_response, lti_role_service, + lms_course, ): - lms_course = factories.LMSCourse(lti_context_memberships_url="SERVICE_URL") - factories.LMSCourseApplicationInstance( - lms_course=lms_course, application_instance=lti_v13_application_instance - ) - # Active user not returned by the roster, should be marked inactive + # Active user not returned by the roster, should be marked inactive after fetch the roster factories.CourseRoster( lms_course=lms_course, lms_user=factories.LMSUser(lti_user_id="EXISTING USER"), @@ -39,7 +36,7 @@ def test_fetch_roster( factories.LTIRole(value="ROLE2"), ] - svc.fetch_roster(lms_course) + svc.fetch_course_roster(lms_course) lti_names_roles_service.get_context_memberships.assert_called_once_with( lti_v13_application_instance.lti_registration, "SERVICE_URL" @@ -48,28 +45,102 @@ def test_fetch_roster( Any.list.containing(["ROLE2", "ROLE1"]) ) - course_roster = db_session.scalars( + roster = db_session.scalars( select(CourseRoster) .where(CourseRoster.lms_course_id == lms_course.id) .order_by(CourseRoster.lms_user_id) ).all() - assert len(course_roster) == 4 - assert course_roster[0].lms_course_id == lms_course.id - assert course_roster[0].lms_user.lti_user_id == "EXISTING USER" - assert not course_roster[0].active + assert len(roster) == 4 + assert roster[0].lms_course_id == lms_course.id + assert roster[0].lms_user.lti_user_id == "EXISTING USER" + assert not roster[0].active + + assert roster[1].lms_course_id == lms_course.id + assert roster[1].lms_user.lti_user_id == "USER_ID" + assert roster[1].active + + assert roster[2].lms_course_id == lms_course.id + assert roster[2].lms_user.lti_user_id == "USER_ID" + assert roster[2].active + + assert roster[3].lms_course_id == lms_course.id + assert roster[3].lms_user.lti_user_id == "USER_ID_INACTIVE" + assert not roster[3].active + + def test_fetch_assignment_roster( + self, + svc, + lti_names_roles_service, + lti_v13_application_instance, + db_session, + names_and_roles_roster_response, + lti_role_service, + assignment, + ): + # Active user not returned by the roster, should be marked inactive after fetch the roster + factories.AssignmentRoster( + assignment=assignment, + lms_user=factories.LMSUser(lti_user_id="EXISTING USER"), + lti_role=factories.LTIRole(), + active=True, + ) + db_session.flush() + lti_names_roles_service.get_context_memberships.return_value = ( + names_and_roles_roster_response + ) + lti_role_service.get_roles.return_value = [ + factories.LTIRole(value="ROLE1"), + factories.LTIRole(value="ROLE2"), + ] + + svc.fetch_assignment_roster(assignment) + + lti_names_roles_service.get_context_memberships.assert_called_once_with( + lti_v13_application_instance.lti_registration, "SERVICE_URL", "LTI1.3_ID" + ) + lti_role_service.get_roles.assert_called_once_with( + Any.list.containing(["ROLE2", "ROLE1"]) + ) + + roster = db_session.scalars( + select(AssignmentRoster) + .order_by(AssignmentRoster.lms_user_id) + .where(AssignmentRoster.assignment_id == assignment.id) + ).all() + + assert len(roster) == 4 + assert roster[0].assignment_id == assignment.id + assert roster[0].lms_user.lti_user_id == "EXISTING USER" + assert not roster[0].active + + assert roster[1].assignment_id == assignment.id + assert roster[1].lms_user.lti_user_id == "USER_ID" + assert roster[1].active - assert course_roster[1].lms_course_id == lms_course.id - assert course_roster[1].lms_user.lti_user_id == "USER_ID" - assert course_roster[1].active + assert roster[2].assignment_id == assignment.id + assert roster[2].lms_user.lti_user_id == "USER_ID" + assert roster[2].active + + assert roster[3].assignment_id == assignment.id + assert roster[3].lms_user.lti_user_id == "USER_ID_INACTIVE" + assert not roster[3].active + + @pytest.fixture + def lms_course(self, lti_v13_application_instance): + lms_course = factories.LMSCourse(lti_context_memberships_url="SERVICE_URL") + factories.LMSCourseApplicationInstance( + lms_course=lms_course, application_instance=lti_v13_application_instance + ) - assert course_roster[2].lms_course_id == lms_course.id - assert course_roster[2].lms_user.lti_user_id == "USER_ID" - assert course_roster[2].active + return lms_course - assert course_roster[3].lms_course_id == lms_course.id - assert course_roster[3].lms_user.lti_user_id == "USER_ID_INACTIVE" - assert not course_roster[3].active + @pytest.fixture + def assignment(self, lms_course): + course = factories.Course( + authority_provided_id=lms_course.h_authority_provided_id + ) + return factories.Assignment(lti_v13_resource_link_id="LTI1.3_ID", course=course) @pytest.fixture def names_and_roles_roster_response(self): diff --git a/tests/unit/lms/tasks/roster_test.py b/tests/unit/lms/tasks/roster_test.py index 85868614bb..c79f46508e 100644 --- a/tests/unit/lms/tasks/roster_test.py +++ b/tests/unit/lms/tasks/roster_test.py @@ -4,44 +4,93 @@ import pytest from freezegun import freeze_time -from lms.tasks.roster import fetch_roster, schedule_fetching_rosters +from lms.tasks.roster import ( + fetch_assignment_roster, + fetch_course_roster, + schedule_fetching_assignment_rosters, + schedule_fetching_course_rosters, + schedule_fetching_rosters, +) from tests import factories -class TestFetchRoster: - def test_it(self, roster_service, db_session): +class TestRosterTasks: + def test_fetch_course_roster(self, roster_service, db_session): lms_course = factories.LMSCourse() db_session.flush() - fetch_roster(lms_course_id=lms_course.id) + fetch_course_roster(lms_course_id=lms_course.id) - roster_service.fetch_roster.assert_called_once_with(lms_course) + roster_service.fetch_course_roster.assert_called_once_with(lms_course) + def test_fetch_assignment_roster(self, roster_service, db_session): + assignment = factories.Assignment() + db_session.flush() + + fetch_assignment_roster(assignment_id=assignment.id) + + roster_service.fetch_assignment_roster.assert_called_once_with(assignment) + + def test_schedule_fetching_rosters( + self, schedule_fetching_assignment_rosters, schedule_fetching_course_rosters + ): + schedule_fetching_rosters() + + schedule_fetching_course_rosters.assert_called_once_with() + schedule_fetching_assignment_rosters.assert_called_once_with() -@freeze_time("2024-08-28") -class TestScheduleFetchingRosters: + @freeze_time("2024-08-28") @pytest.mark.usefixtures( "lms_course_with_no_launch", "lms_course_with_no_service_url", "lms_course_with_launch_and_recent_roster", ) - def test_it(self, lms_course_with_recent_launch, db_session, fetch_roster): + def test_schedule_fetching_course_rosters( + self, lms_course_with_recent_launch, db_session, fetch_course_roster + ): db_session.flush() - schedule_fetching_rosters() + schedule_fetching_course_rosters() - fetch_roster.delay.assert_called_once_with( + fetch_course_roster.delay.assert_called_once_with( lms_course_id=lms_course_with_recent_launch.id ) + @freeze_time("2024-08-28") + @pytest.mark.usefixtures( + "assignment_with_no_launch", + "assignment_with_no_lti_v13_id", + "assignment_with_launch_and_recent_roster", + ) + def test_schedule_fetching_assignment_rosters( + self, assignment_with_recent_launch, db_session, fetch_assignment_roster + ): + db_session.flush() + + schedule_fetching_assignment_rosters() + + fetch_assignment_roster.delay.assert_called_once_with( + assignment_id=assignment_with_recent_launch.id + ) + @pytest.fixture def lms_course_with_no_service_url(self): return factories.LMSCourse() + @pytest.fixture + def assignment_with_no_lti_v13_id(self): + return factories.LMSCourse() + @pytest.fixture def lms_course_with_no_launch(self): return factories.LMSCourse(lti_context_memberships_url="URL") + @pytest.fixture + def assignment_with_no_launch(self): + return factories.Assignment( + lti_v13_resource_link_id="ID", course=factories.Course() + ) + @pytest.fixture def lms_course_with_recent_launch(self): course = factories.Course() @@ -53,8 +102,20 @@ def lms_course_with_recent_launch(self): return factories.LMSCourse( lti_context_memberships_url="URL", h_authority_provided_id=course.authority_provided_id, + course=course, ) + @pytest.fixture + def assignment_with_recent_launch(self, lms_course_with_recent_launch): + assignment = factories.Assignment( + lti_v13_resource_link_id="ID", course=lms_course_with_recent_launch.course + ) + factories.Event( + assignment=assignment, + timestamp=datetime(2024, 8, 28), + ) + return assignment + @pytest.fixture def lms_course_with_launch_and_recent_roster(self): course = factories.Course() @@ -74,8 +135,39 @@ def lms_course_with_launch_and_recent_roster(self): return lms_course @pytest.fixture - def fetch_roster(self, patch): - return patch("lms.tasks.roster.fetch_roster") + def assignment_with_launch_and_recent_roster(self, lms_course_with_recent_launch): + assignment = factories.Assignment( + lti_v13_resource_link_id="ID", course=lms_course_with_recent_launch.course + ) + factories.Event( + assignment=assignment, + timestamp=datetime(2024, 8, 28), + ) + factories.AssignmentRoster( + assignment=assignment, + lms_user=factories.LMSUser(), + lti_role=factories.LTIRole(), + active=True, + updated=datetime(2024, 8, 25), + ) + + return assignment + + @pytest.fixture + def fetch_course_roster(self, patch): + return patch("lms.tasks.roster.fetch_course_roster") + + @pytest.fixture + def fetch_assignment_roster(self, patch): + return patch("lms.tasks.roster.fetch_assignment_roster") + + @pytest.fixture + def schedule_fetching_assignment_rosters(self, patch): + return patch("lms.tasks.roster.schedule_fetching_assignment_rosters") + + @pytest.fixture + def schedule_fetching_course_rosters(self, patch): + return patch("lms.tasks.roster.schedule_fetching_course_rosters") @pytest.fixture(autouse=True)