Feedback

ocadotechnology · Jul 31, 2024 · eae8603 · eae8603
1 parent fe7861f
commit eae8603
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 45 deletions.
diff --git a/portal/tests/test_views.py b/portal/tests/test_views.py
@@ -17,6 +17,7 @@
     UserProfile,
     UserSession,
 )
+from common.mail import campaign_ids
 from common.tests.utils.classes import create_class_directly
 from common.tests.utils.organisation import (
     create_organisation_directly,
@@ -1225,6 +1226,7 @@ def send_inactivity_reminder(
         days: int,
         view_name: str,
         assert_called: bool,
+        campaign_name: str,
         mock_send_dotdigital_email: Mock,
     ):
         self.teacher_user.date_joined = timezone.now() - timedelta(
@@ -1244,11 +1246,11 @@ def send_inactivity_reminder(
 
         if assert_called:
             mock_send_dotdigital_email.assert_any_call(
-                ANY, [self.teacher_user.email]
+                campaign_ids[campaign_name], [self.teacher_user.email]
             )
 
             mock_send_dotdigital_email.assert_any_call(
-                ANY, [self.indy_user.email]
+                campaign_ids[campaign_name], [self.indy_user.email]
             )
 
             # Check only two emails are sent - the student should never be included.
@@ -1259,19 +1261,61 @@ def send_inactivity_reminder(
         mock_send_dotdigital_email.reset_mock()
 
     def test_first_inactivity_reminder_view(self):
-        self.send_inactivity_reminder(729, "first-inactivity-reminder", False)
-        self.send_inactivity_reminder(730, "first-inactivity-reminder", True)
-        self.send_inactivity_reminder(731, "first-inactivity-reminder", False)
-        self.send_inactivity_reminder(732, "first-inactivity-reminder", False)
+        self.send_inactivity_reminder(
+            729,
+            "first-inactivity-reminder",
+            False,
+            "inactive_users_on_website_first_reminder",
+        )
+        self.send_inactivity_reminder(
+            730,
+            "first-inactivity-reminder",
+            True,
+            "inactive_users_on_website_first_reminder",
+        )
+        self.send_inactivity_reminder(
+            731,
+            "first-inactivity-reminder",
+            False,
+            "inactive_users_on_website_first_reminder",
+        )
 
     def test_second_inactivity_reminder_view(self):
-        self.send_inactivity_reminder(972, "second-inactivity-reminder", False)
-        self.send_inactivity_reminder(973, "second-inactivity-reminder", True)
-        self.send_inactivity_reminder(974, "second-inactivity-reminder", False)
-        self.send_inactivity_reminder(975, "second-inactivity-reminder", False)
+        self.send_inactivity_reminder(
+            972,
+            "second-inactivity-reminder",
+            False,
+            "inactive_users_on_website_second_reminder",
+        )
+        self.send_inactivity_reminder(
+            973,
+            "second-inactivity-reminder",
+            True,
+            "inactive_users_on_website_second_reminder",
+        )
+        self.send_inactivity_reminder(
+            974,
+            "second-inactivity-reminder",
+            False,
+            "inactive_users_on_website_second_reminder",
+        )
 
     def test_third_inactivity_reminder_view(self):
-        self.send_inactivity_reminder(1064, "third-inactivity-reminder", False)
-        self.send_inactivity_reminder(1065, "third-inactivity-reminder", True)
-        self.send_inactivity_reminder(1066, "third-inactivity-reminder", False)
-        self.send_inactivity_reminder(1067, "third-inactivity-reminder", False)
+        self.send_inactivity_reminder(
+            1064,
+            "third-inactivity-reminder",
+            False,
+            "inactive_users_on_website_final_reminder",
+        )
+        self.send_inactivity_reminder(
+            1065,
+            "third-inactivity-reminder",
+            True,
+            "inactive_users_on_website_final_reminder",
+        )
+        self.send_inactivity_reminder(
+            1066,
+            "third-inactivity-reminder",
+            False,
+            "inactive_users_on_website_final_reminder",
+        )
diff --git a/portal/views/cron/user.py b/portal/views/cron/user.py
@@ -51,9 +51,7 @@ def get_unverified_users(
     return teacher_queryset, independent_student_queryset
 
 
-def get_inactive_users(
-    days: int, same_day: bool
-) -> (QuerySet[User], QuerySet[User]):
+def get_inactive_users(days: int) -> QuerySet[User]:
     now = timezone.now()
 
     # All users who haven't logged in in X days OR who've never logged in and
@@ -62,25 +60,15 @@ def get_inactive_users(
         Q(
             last_login__isnull=False,
             last_login__lte=now - timedelta(days=days),
+            last_login__gt=now - timedelta(days=days + 1),
         )
         | Q(
             last_login__isnull=True,
             date_joined__lte=now - timedelta(days=days),
+            date_joined__gt=now - timedelta(days=days + 1),
         )
     )
 
-    if same_day:
-        user_queryset = user_queryset.filter(
-            Q(
-                last_login__isnull=False,
-                last_login__gt=now - timedelta(days=days + 1),
-            )
-            | Q(
-                last_login__isnull=True,
-                date_joined__gt=now - timedelta(days=days + 1),
-            )
-        )
-
     teacher_queryset = user_queryset.filter(
         new_teacher__isnull=False,
         new_student__isnull=True,
@@ -90,7 +78,7 @@ def get_inactive_users(
         new_student__class_field__isnull=True,
     )
 
-    return teacher_queryset, independent_student_queryset
+    return teacher_queryset.union(independent_student_queryset)
 
 
 def build_absolute_google_uri(request, location: str) -> str:
@@ -234,11 +222,7 @@ def get(self, request):
 
 class FirstInactivityReminderView(CronMixin, APIView):
     def get(self, request):
-        teacher_queryset, independent_student_queryset = get_inactive_users(
-            USER_1ST_INACTIVE_REMINDER_DAYS,
-            same_day=True,
-        )
-        user_queryset = teacher_queryset.union(independent_student_queryset)
+        user_queryset = get_inactive_users(USER_1ST_INACTIVE_REMINDER_DAYS)
         user_count = user_queryset.count()
 
         logging.info(
@@ -272,11 +256,7 @@ def get(self, request):
 
 class SecondInactivityReminderView(CronMixin, APIView):
     def get(self, request):
-        teacher_queryset, independent_student_queryset = get_inactive_users(
-            USER_2ND_INACTIVE_REMINDER_DAYS,
-            same_day=True,
-        )
-        user_queryset = teacher_queryset.union(independent_student_queryset)
+        user_queryset = get_inactive_users(USER_2ND_INACTIVE_REMINDER_DAYS)
         user_count = user_queryset.count()
 
         logging.info(
@@ -310,11 +290,7 @@ def get(self, request):
 
 class ThirdInactivityReminderView(CronMixin, APIView):
     def get(self, request):
-        teacher_queryset, independent_student_queryset = get_inactive_users(
-            USER_3RD_INACTIVE_REMINDER_DAYS,
-            same_day=True,
-        )
-        user_queryset = teacher_queryset.union(independent_student_queryset)
+        user_queryset = get_inactive_users(USER_3RD_INACTIVE_REMINDER_DAYS)
         user_count = user_queryset.count()
 
         logging.info(