Merge branch '234-sending-emails-to-multiple-recipients' into 'main'

Resolve "Sending Emails to Multiple Recipients"

Closes #234

See merge request inject/backend!249

pyproject.toml

@@ -35,7 +35,8 @@ disable_error_code = ["annotation-unchecked"]
 exclude = [
     "migrations",
     "tests",
-    "dev"
+    "dev",
+    "scripts",
 ]
 
 [tool.black]

running_exercise/lib/email_client.py

@@ -67,47 +67,39 @@ def send_email(
 
 def _validate_participants(
     participant_addresses: List[str], exercise: Exercise
-) -> List[EmailParticipant]:
-    if len(set(participant_addresses)) < 2:
+) -> QuerySet[EmailParticipant]:
+    distinct_addresses = set(participant_addresses)
+    if len(distinct_addresses) < 2:
         raise RunningExerciseOperationException(
             "A thread must have at least 2 participants"
         )
 
-    email_between_teams = exercise.config.has_enabled(
-        Feature.EMAIL_BETWEEN_TEAMS
+    participants = EmailParticipant.objects.filter(
+        exercise_id=exercise.id, address__in=distinct_addresses
     )
-    participants: List[EmailParticipant] = []
-    previous_definition_participant: Optional[EmailParticipant] = None
-    has_team = False
-
-    for address in participant_addresses:
-        # TODO: Handle email address that is not included in the definition,
-        # needs to be logged somewhere, however action logs do not seem as
-        # the best choice, because I am not sure if this should be included
-        # in the trainee visible logs
-        # maybe add a new action log type for errors, which can be used for filtering
-        participant = get_model(
-            EmailParticipant, exercise=exercise, address=address
+
+    if len(participants) != len(distinct_addresses):
+        # how much do we care about the error message?
+        raise RunningExerciseOperationException(
+            f"Some address/es from this list `{distinct_addresses}` don't exist in the exercise"
         )
-        if participant.is_definition():
-            if previous_definition_participant is not None:
-                raise RunningExerciseOperationException(
-                    f"Cannot send email to these addresses at the same time: "
-                    f"'{previous_definition_participant.address}',"
-                    f" '{participant.address}'"
-                )
-            previous_definition_participant = participant
-        else:
-            # this is probably an extremely rare edge case, however there is a possibility
-            # that a team randomly guesses another teams address and without this check
-            # the 'email_between_teams' feature could be bypassed
-            if has_team and not email_between_teams:
-                raise RunningExerciseOperationException(
-                    "Email communication between teams is disabled"
-                )
-            has_team = True
-
-        participants.append(participant)
+
+    team_participants_count = sum(
+        map(lambda participant: participant.team_id is not None, participants)
+    )
+
+    if team_participants_count == 0:
+        raise RunningExerciseOperationException(
+            f"A thread must contain at least 1 team address"
+        )
+
+    if team_participants_count > 1 and not exercise.config.has_enabled(
+        Feature.EMAIL_BETWEEN_TEAMS
+    ):
+        raise RunningExerciseOperationException(
+            f"Email between teams is not enabled"
+        )
+
     return participants
 
 
@@ -118,17 +110,21 @@ def _activate_thread_definition_milestones(
     This method handles proper milestone activation for milestones specified in
     EmailAddress
     """
-    team_email_sent = (
-        thread.emails.filter(sender__definition_address__isnull=True).count()
-        > 0
-    )
+    # Instructor emails should not activate milestones
+    if sender.is_definition():
+        return
 
-    if team_email_sent or sender.is_definition():
+    # If a team already sent an email to this thread, do not activate milestones again
+    team_email_sent = thread.emails.filter(
+        sender__definition_address__isnull=True
+    ).exists()
+
+    if team_email_sent:
         return
 
-    if (
-        definition_participant := thread.get_definition_participant()
-    ) is not None:
+    for definition_participant in thread.participants.filter(
+        definition_address__isnull=False
+    ).select_related("definition_address__control"):
         _update_thread_milestones(
             thread, definition_participant.definition_address.control
         )
@@ -147,9 +143,7 @@ class EmailClient:
         )
         new_thread.participants.add(*participants)
 
-        SubscriptionHandler.broadcast_email_thread(
-            new_thread, participants[0].exercise
-        )
+        SubscriptionHandler.broadcast_email_thread(new_thread, exercise)
         return new_thread
 
     @staticmethod

running_exercise/schema/query.py

@@ -353,17 +353,20 @@ class Query(graphene.ObjectType):
     @protected(User.AuthGroup.INSTRUCTOR)
     def resolve_thread_templates(
         self, info, thread_id: int
-    ) -> List[EmailTemplate]:
+    ) -> QuerySet[EmailTemplate]:
         thread = get_model(EmailThread, id=thread_id)
         exercise_access(info.context, thread.exercise_id)
 
-        definition_participant = thread.get_definition_participant()
-        if definition_participant is None:
-            return []
+        definition_participants = thread.participants.filter(
+            definition_address__isnull=False
+        ).select_related("definition_address")
 
-        return list(
-            definition_participant.get_definition_address().templates.all()
-        )
+        templates = EmailTemplate.objects.none()
+
+        for participant in definition_participants:
+            templates |= participant.definition_address.templates.all()
+
+        return templates
 
     @protected(User.AuthGroup.INSTRUCTOR)
     def resolve_thread_template(self, info, template_id: str) -> EmailTemplate:

running_exercise/tests/email_tests.py

@@ -128,22 +128,15 @@ class EmailTests(TestCase):
                 participants, self.subject, str(self.exercise.id)
             )
 
+        participants.append("team-1@mail.com")
         participants.append("nonexistentaddress@mail.com")
-        with self.assertRaises(ModelNotFoundException) as _:
-            EmailClient.create_thread(
-                participants, self.subject, str(self.exercise.id)
-            )
-
-        # multiple definition addresses
-        participants[1] = "test@mail.ex"
         with self.assertRaises(RunningExerciseOperationException) as _:
             EmailClient.create_thread(
                 participants, self.subject, str(self.exercise.id)
             )
 
         _disable_feature(self.exercise, Feature.EMAIL_BETWEEN_TEAMS)
-        participants[1] = "team-1@mail.com"
-        participants.append("team-2@mail.com")
+        participants[0] = "team-2@mail.com"
         with self.assertRaises(RunningExerciseOperationException) as _:
             EmailClient.create_thread(
                 participants, self.subject, str(self.exercise.id)
@@ -190,6 +183,17 @@ class EmailTests(TestCase):
             self.subject,
         )
 
+        # thread with multiple definition addresses
+        participants = ["team-1@mail.com", "test@mail.ex", "doe@mail.ex"]
+        thread = EmailClient.create_thread(
+            participants, self.subject, str(self.exercise.id)
+        )
+
+        self._compare_expected_participants(
+            participants, thread.participants.all()
+        )
+        self.assertEqual(thread.subject, self.subject)
+
     def test_get_thread(self):
         # each combination consist of a definition and a team address,
         # the team address is first
@@ -355,8 +359,11 @@ class EmailTests(TestCase):
             )
 
     def test_team_email(self):
-        participants = ["doe@mail.ex", "team-1@mail.com"]
-        milestone = self.definition.milestones.get(name="email_doe")
+        participants = ["doe@mail.ex", "test@mail.ex", "team-1@mail.com"]
+        email_doe = self.definition.milestones.get(name="email_doe")
+        email_sent_to_test = self.definition.milestones.get(
+            name="email_sent_to_test"
+        )
         team_sender = self.participants["team-1@mail.com"]
         definition_sender = self.participants["doe@mail.ex"]
         email_channel = self.definition.channels.get(type=InjectTypes.EMAIL)
@@ -365,7 +372,10 @@ class EmailTests(TestCase):
         )
 
         # check that milestones from the definition address were not activated on thread create
-        self.assertFalse(is_milestone_reached(team_sender.team, milestone))
+        self.assertFalse(is_milestone_reached(team_sender.team, email_doe))
+        self.assertFalse(
+            is_milestone_reached(team_sender.team, email_sent_to_test)
+        )
         file_info = self.definition.files.first()
         action_logs = EmailClient.send_email(
             create_input(
@@ -392,7 +402,10 @@ class EmailTests(TestCase):
         )
         # check that after first email was sent the thread milestones were not activated
         # because it was not sent by a team
-        self.assertFalse(is_milestone_reached(team_sender.team, milestone))
+        self.assertFalse(is_milestone_reached(team_sender.team, email_doe))
+        self.assertFalse(
+            is_milestone_reached(team_sender.team, email_sent_to_test)
+        )
 
         action_logs = EmailClient.send_email(
             create_input(
@@ -411,9 +424,12 @@ class EmailTests(TestCase):
             f"'{sent_email.sender=}' and '{team_sender}'",
         )
 
-        self.assertTrue(is_milestone_reached(team_sender.team, milestone))
+        self.assertTrue(is_milestone_reached(team_sender.team, email_doe))
+        self.assertTrue(
+            is_milestone_reached(team_sender.team, email_sent_to_test)
+        )
 
-        deactivate_milestones(team_sender.team, [milestone])
+        deactivate_milestones(team_sender.team, [email_doe, email_sent_to_test])
 
         action_logs = EmailClient.send_email(
             create_input(
@@ -426,4 +442,7 @@ class EmailTests(TestCase):
         self._check_action_logs_channel(action_logs, email_channel)
 
         # check that further emails do not activate the thread milestones again
-        self.assertFalse(is_milestone_reached(team_sender.team, milestone))
+        self.assertFalse(is_milestone_reached(team_sender.team, email_doe))
+        self.assertFalse(
+            is_milestone_reached(team_sender.team, email_sent_to_test)
+        )