Replace set() by dict comprehension in recipients for new comments

CHANGELOG.md

@@ -10,6 +10,7 @@
 - Return 403 when posting comment on discussion closed [#2881](https://github.com/opendatateam/udata/pull/2881)
 - Ensure rdf parsed frequency is lowercase [#2883](https://github.com/opendatateam/udata/pull/2883)
 - Add a dict of URIs to replace in a RDF graph at harvest time [#2884](https://github.com/opendatateam/udata/pull/2884)
+- Fix duplicate recipients in new comments mail [#2886](https://github.com/opendatateam/udata/pull/2886)
 
 ## 6.1.6 (2023-07-19)
 

udata/core/discussions/tasks.py

@@ -41,7 +41,7 @@ def notify_new_discussion_comment(discussion_id, message=None):
     if isinstance(discussion.subject, (Dataset, Reuse, Post)):
         recipients = owner_recipients(discussion) + [
             m.posted_by for m in discussion.discussion]
-        recipients = [u for u in set(recipients) if u != message.posted_by]
+        recipients = list({u.id: u for u in recipients if u != message.posted_by}.values())
         subject = _('%(user)s commented your discussion',
                     user=message.posted_by.fullname)
 
@@ -59,7 +59,7 @@ def notify_discussion_closed(discussion_id, message=None):
     if isinstance(discussion.subject, (Dataset, Reuse, Post)):
         recipients = owner_recipients(discussion) + [
             m.posted_by for m in discussion.discussion]
-        recipients = [u for u in set(recipients) if u != message.posted_by]
+        recipients = list({u.id: u for u in recipients if u != message.posted_by}.values())
         subject = _('A discussion has been closed')
         mail.send(subject, recipients, 'discussion_closed',
                   discussion=discussion, message=message)

udata/templates/mail/badge_added_certified.html

@@ -6,7 +6,7 @@
     _('%(user)s has certified your organization "%(name)s"',
     user=(
         '<a href="'|safe
-        + url_for('users.show', user=badge.created_by, _external=True)
+        + url_for('api.user', user=badge.created_by, _external=True)
         + '">'|safe
         + badge.created_by.fullname
         + '</a>'|safe

udata/templates/mail/badge_added_public_service.html

@@ -6,7 +6,7 @@
     _('%(user)s has identified your organization "%(name)s" as public service',
     user=(
         '<a href="'|safe
-        + url_for('users.show', user=badge.created_by, _external=True)
+        + url_for('api.user', user=badge.created_by, _external=True)
         + '">'|safe
         + badge.created_by.fullname
         + '</a>'|safe

udata/templates/mail/discussion_closed.html

@@ -7,7 +7,7 @@
         type=discussion.subject.verbose_name,
         user=(
             '<a href="'|safe
-            + url_for('users.show', user=message.posted_by, _external=True)
+            + url_for('api.user', user=message.posted_by, _external=True)
             + '">'|safe
             + message.posted_by.fullname
             + '</a>'|safe

udata/templates/mail/membership_request.html

@@ -8,7 +8,7 @@
 {{ _('As an administrator of "%(org)s" you are being informed than a new membership request from %(user)s is pending for validation',
     user=(
         '<a href="'|safe
-        + url_for('users.show', user=request.user, _external=True)
+        + url_for('api.user', user=request.user, _external=True)
         + '">'|safe
         + request.user.fullname
         + '</a>'|safe

udata/templates/mail/new_discussion.html

@@ -7,7 +7,7 @@
     type=discussion.subject.verbose_name,
     user=(
         '<a href="'|safe
-        + url_for('users.show', user=discussion.user, _external=True)
+        + url_for('api.user', user=discussion.user, _external=True)
         + '">'|safe
         + discussion.user.fullname
         + '</a>'|safe

udata/templates/mail/new_discussion_comment.html

@@ -7,7 +7,7 @@
         type=discussion.subject.verbose_name,
         user=(
             '<a href="'|safe
-            + url_for('users.show', user=message.posted_by, _external=True)
+            + url_for('api.user', user=message.posted_by, _external=True)
             + '">'|safe
             + message.posted_by.fullname
             + '</a>'|safe

udata/templates/mail/user_mail_card.html

@@ -1,5 +1,5 @@
 {% macro user_mail_card(user, msg=None) %}
-<a href="{{ url_for('users.show', user=user.id, _external=True) }}"
+<a href="{{ url_for('api.user', user=user.id, _external=True) }}"
      style="margin: 0;padding: 0;width:100%;">
     <table height="60" cellpadding="0" cellspacing="0" style="margin: 0;padding: 0;border:1px solid #eee;width:100%;">
         <tr style="margin: 0;padding: 0;">

udata/tests/test_discussions.py

@@ -4,7 +4,7 @@
 
 from udata.models import Dataset, Member
 from udata.core.discussions.models import Message, Discussion
-from udata.core.discussions.metrics import update_discussions_metric
+from udata.core.discussions.metrics import update_discussions_metric  # noqa
 from udata.core.discussions.notifications import discussions_notifications
 from udata.core.discussions.signals import (
     on_new_discussion, on_new_discussion_comment,
@@ -17,6 +17,7 @@
 from udata.core.dataset.factories import DatasetFactory
 from udata.core.organization.factories import OrganizationFactory
 from udata.core.user.factories import UserFactory, AdminFactory
+from udata.tests.helpers import capture_mails
 from udata.utils import faker
 
 from . import TestCase, DBTestMixin
@@ -550,3 +551,76 @@ def test_notify_org_discussions(self):
             self.assertEqual(details['title'], discussion.title)
             self.assertEqual(details['subject']['id'], discussion.subject.id)
             self.assertEqual(details['subject']['type'], 'dataset')
+
+
+class DiscussionsMailsTest(APITestCase):
+    modules = []
+
+    def test_new_discussion_mail(self):
+        user = UserFactory()
+        owner = UserFactory()
+        message = Message(content=faker.sentence(), posted_by=user)
+        discussion = Discussion.objects.create(
+            subject=DatasetFactory(owner=owner),
+            user=user,
+            title=faker.sentence(),
+            discussion=[message]
+        )
+
+        with capture_mails() as mails:
+            notify_new_discussion(discussion.id)
+
+        # Should have sent one mail to the owner
+        self.assertEqual(len(mails), 1)
+        self.assertEqual(mails[0].recipients[0], owner.email)
+
+    def test_new_discussion_comment_mail(self):
+        owner = UserFactory()
+        poster = UserFactory()
+        commenter = UserFactory()
+        message = Message(content=faker.sentence(), posted_by=poster)
+        second_message = Message(content=faker.sentence(), posted_by=owner)
+        new_message = Message(content=faker.sentence(), posted_by=commenter)
+        discussion = Discussion.objects.create(
+            subject=DatasetFactory(owner=owner),
+            user=poster,
+            title=faker.sentence(),
+            discussion=[message, second_message, new_message]
+        )
+
+        with capture_mails() as mails:
+            notify_new_discussion_comment(discussion.id, message=len(discussion.discussion) - 1)
+
+        # Should have sent one mail to the owner and the other participants
+        # and no mail to the commenter. The owner should appear only once in the recipients
+        # even if he is in both the discussion and the owner of the dataset.
+        expected_recipients = (owner.email, poster.email)
+        self.assertEqual(len(mails), len(expected_recipients))
+        for mail in mails:
+            self.assertIn(mail.recipients[0], expected_recipients)
+            self.assertNotIn(commenter.email, mail.recipients)
+
+    def test_closed_discussion_mail(self):
+        owner = UserFactory()
+        poster = UserFactory()
+        commenter = UserFactory()
+        message = Message(content=faker.sentence(), posted_by=poster)
+        second_message = Message(content=faker.sentence(), posted_by=commenter)
+        closing_message = Message(content=faker.sentence(), posted_by=owner)
+        discussion = Discussion.objects.create(
+            subject=DatasetFactory(owner=owner),
+            user=poster,
+            title=faker.sentence(),
+            discussion=[message, second_message, closing_message]
+        )
+
+        with capture_mails() as mails:
+            notify_discussion_closed(discussion.id, message=len(discussion.discussion) - 1)
+
+        # Should have sent one mail to each participant
+        # and no mail to the closer
+        expected_recipients = (poster.email, commenter.email)
+        self.assertEqual(len(mails), len(expected_recipients))
+        for mail in mails:
+            self.assertIn(mail.recipients[0], expected_recipients)
+            self.assertNotIn(owner.email, mail.recipients)