From 6d1005ac47cafc1a517d861d5578045310b018bd Mon Sep 17 00:00:00 2001
From: Marcos Prieto <marcospri@gmail.com>
Date: Mon, 25 Nov 2024 16:51:51 +0100
Subject: [PATCH] Fix query to find assignment rosters that need fetching

The existing query used a subquery that joined itself on assignments.
That meant that that condition will return true if any event can be
joined with an assignments, that's always true.

Avoid the join and use the parent query Assignment, making the subquery
a correlated one to get the correct result.
---
 lms/tasks/roster.py                 |  5 ++---
 tests/unit/lms/tasks/roster_test.py | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/lms/tasks/roster.py b/lms/tasks/roster.py
index 172bf9e22e..e012b20191 100644
--- a/lms/tasks/roster.py
+++ b/lms/tasks/roster.py
@@ -114,9 +114,8 @@ def schedule_fetching_assignment_rosters() -> None:
 
     # Only fetch rosters for assignments that have been recently launched
     recent_launches_clause = exists(
-        select(Event)
-        .join(Assignment, Event.assignment_id == Assignment.id)
-        .where(
+        select(Event).where(
+            Event.assignment_id == Assignment.id,
             Event.timestamp >= now - LAUNCHED_WINDOW,
         )
     )
diff --git a/tests/unit/lms/tasks/roster_test.py b/tests/unit/lms/tasks/roster_test.py
index 2beec85f41..7774f39f83 100644
--- a/tests/unit/lms/tasks/roster_test.py
+++ b/tests/unit/lms/tasks/roster_test.py
@@ -42,6 +42,7 @@ def test_schedule_fetching_rosters(
     @freeze_time("2024-08-28")
     @pytest.mark.usefixtures(
         "lms_course_with_no_launch",
+        "lms_course_with_no_recent_launch",
         "lms_course_with_no_service_url",
         "lms_course_with_launch_and_recent_roster",
         "lms_course_with_recent_launch_and_task_done_row",
@@ -60,6 +61,7 @@ def test_schedule_fetching_course_rosters(
     @freeze_time("2024-08-28")
     @pytest.mark.usefixtures(
         "assignment_with_no_launch",
+        "assignment_with_no_recent_launch",
         "assignment_with_no_lti_v13_id",
         "assignment_with_recent_launch_and_task_done_row",
         "assignment_with_launch_and_recent_roster",
@@ -93,6 +95,17 @@ def assignment_with_no_launch(self):
             lti_v13_resource_link_id="ID", course=factories.Course()
         )
 
+    @pytest.fixture
+    def assignment_with_no_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, 1, 1),
+        )
+        return assignment
+
     @pytest.fixture
     def lms_course_with_recent_launch(self):
         course = factories.Course()
@@ -107,6 +120,20 @@ def lms_course_with_recent_launch(self):
             course=course,
         )
 
+    @pytest.fixture
+    def lms_course_with_no_recent_launch(self):
+        course = factories.Course()
+        factories.Event(
+            course=course,
+            timestamp=datetime(2024, 1, 1),
+        )
+
+        return factories.LMSCourse(
+            lti_context_memberships_url="URL",
+            h_authority_provided_id=course.authority_provided_id,
+            course=course,
+        )
+
     @pytest.fixture
     def lms_course_with_recent_launch_and_task_done_row(self, db_session):
         course = factories.Course()