diff --git a/CHANGELOG.md b/CHANGELOG.md index f5b555bcda..95fca3fad3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ - Add api endpoint /me/org_topics/ [#3070](https://github.com/opendatateam/udata/pull/3070) - Expose dataservices in RDF catalog [#3058](https://github.com/opendatateam/udata/pull/3058) - CORS: always returns 204 on OPTIONS request [#3046](https://github.com/opendatateam/udata/pull/3046) +- Allow discussion notification customization [#3072](https://github.com/opendatateam/udata/pull/3072) ## 9.0.0 (2024-06-07) diff --git a/udata/core/discussions/tasks.py b/udata/core/discussions/tasks.py index 449b2c41d4..0e7a2e7cf1 100644 --- a/udata/core/discussions/tasks.py +++ b/udata/core/discussions/tasks.py @@ -1,11 +1,18 @@ +import fnmatch + +from urllib.parse import urlparse + +from flask import current_app + from udata import mail from udata.i18n import lazy_gettext as _ from udata.core.dataset.models import Dataset -from udata.core.reuse.models import Reuse from udata.core.post.models import Post +from udata.core.reuse.models import Reuse +from udata.core.topic.models import Topic from udata.tasks import connect, get_logger -from .models import Discussion, Message +from .models import Discussion from .signals import ( on_new_discussion, on_new_discussion_comment, on_discussion_closed ) @@ -22,15 +29,48 @@ def owner_recipients(discussion): return [] +def get_external_url(discussion): + url = None + if (meta_url := discussion.extras.get('notification', {}).get('external_url')): + meta_url_parsed = urlparse(meta_url) + if any( + fnmatch.fnmatch(meta_url_parsed.netloc, pattern) + for pattern in current_app.config['DISCUSSION_ALLOWED_EXTERNAL_DOMAINS'] + ): + url = f'{meta_url}#discussion-{discussion.id}' + if not url: + url = getattr(discussion, 'external_url', None) + return url + + +def get_subject_type(discussion): + if (meta_name := discussion.extras.get('notification', {}).get('model_name')): + if meta_name in current_app.config['DISCUSSION_ALTERNATE_MODEL_NAMES']: + return meta_name + return discussion.subject.verbose_name + + @connect(on_new_discussion, by_id=True) def notify_new_discussion(discussion_id): discussion = Discussion.objects.get(pk=discussion_id) - if isinstance(discussion.subject, (Dataset, Reuse, Post)): + if isinstance(discussion.subject, (Dataset, Reuse, Post, Topic)): recipients = owner_recipients(discussion) + subject_type = get_subject_type(discussion) subject = _('Your %(type)s have a new discussion', - type=discussion.subject.verbose_name) - mail.send(subject, recipients, 'new_discussion', - discussion=discussion, message=discussion.discussion[0]) + type=subject_type) + external_url = get_external_url(discussion) + if external_url: + mail.send( + subject, + recipients, + 'new_discussion', + discussion=discussion, + message=discussion.discussion[0], + external_url=external_url, + subject_type=subject_type + ) + else: + log.warning(f'No external url could be computed for discussion {discussion.id}') else: log.warning('Unrecognized discussion subject type %s', type(discussion.subject)) @@ -40,15 +80,25 @@ def notify_new_discussion(discussion_id): def notify_new_discussion_comment(discussion_id, message=None): discussion = Discussion.objects.get(pk=discussion_id) message = discussion.discussion[message] - if isinstance(discussion.subject, (Dataset, Reuse, Post)): + if isinstance(discussion.subject, (Dataset, Reuse, Post, Topic)): recipients = owner_recipients(discussion) + [ m.posted_by for m in discussion.discussion] 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) - - mail.send(subject, recipients, 'new_discussion_comment', - discussion=discussion, message=message) + external_url = get_external_url(discussion) + if external_url: + mail.send( + subject, + recipients, + 'new_discussion_comment', + discussion=discussion, + message=message, + external_url=external_url, + subject_type=get_subject_type(discussion) + ) + else: + log.warning(f'No external url could be computed for discussion {discussion.id}') else: log.warning('Unrecognized discussion subject type %s', type(discussion.subject)) @@ -58,13 +108,24 @@ def notify_new_discussion_comment(discussion_id, message=None): def notify_discussion_closed(discussion_id, message=None): discussion = Discussion.objects.get(pk=discussion_id) message = discussion.discussion[message] - if isinstance(discussion.subject, (Dataset, Reuse, Post)): + if isinstance(discussion.subject, (Dataset, Reuse, Post, Topic)): recipients = owner_recipients(discussion) + [ m.posted_by for m in discussion.discussion] 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) + external_url = get_external_url(discussion) + if external_url: + mail.send( + subject, + recipients, + 'discussion_closed', + discussion=discussion, + message=message, + external_url=external_url, + subject_type=get_subject_type(discussion), + ) + else: + log.warning(f'No external url could be computed for discussion {discussion.id}') else: log.warning('Unrecognized discussion subject type %s', type(discussion.subject)) diff --git a/udata/core/topic/models.py b/udata/core/topic/models.py index d2dfceaf12..bd2cccd41b 100644 --- a/udata/core/topic/models.py +++ b/udata/core/topic/models.py @@ -1,5 +1,6 @@ from flask import url_for from mongoengine.signals import pre_save +from udata.i18n import lazy_gettext as _ from udata.models import db, SpatialCoverage from udata.search import reindex from udata.tasks import as_task_param @@ -40,6 +41,8 @@ class Topic(db.Document, Owned, db.Datetimed): 'queryset_class': OwnedQuerySet, } + verbose_name = _('topic') + def __str__(self): return self.name diff --git a/udata/settings.py b/udata/settings.py index f82a27c659..19edae9c7c 100644 --- a/udata/settings.py +++ b/udata/settings.py @@ -232,7 +232,7 @@ class Defaults(object): # A list of tuples, each tuple describing a group with its title and # a list of licenses associated. Translations are not supported. # Example: - # LICENSE_GROUPS = [ + # LICENSE_GROUPS = [ # ("Autorités administratives", [ # {"value": "lov2", "recommended": True, "description": "Recommandée", "code": "etalab-2.0"}, # {"value": "notspecified", "description": "Le Code des relations entre le public et l’administration ne s’applique pas"}]), @@ -264,7 +264,7 @@ class Defaults(object): HARVEST_VALIDATION_CONTACT_FORM = None - HARVEST_MAX_CATALOG_SIZE_IN_MONGO = None # Defaults to the size of a MongoDB document + HARVEST_MAX_CATALOG_SIZE_IN_MONGO = None # Defaults to the size of a MongoDB document HARVEST_GRAPHS_S3_BUCKET = None # If the catalog is bigger than `HARVEST_MAX_CATALOG_SIZE_IN_MONGO` store the graph inside S3 instead of MongoDB HARVEST_GRAPHS_S3_FILENAME_PREFIX = '' # Useful to store the graphs inside a subfolder of the bucket. For example by setting `HARVEST_GRAPHS_S3_FILENAME_PREFIX = 'graphs/'` @@ -473,6 +473,11 @@ class Defaults(object): ########################################################################### MATTERMOST_WEBHOOK = None + # Discussion settings + ########################################################################### + DISCUSSION_ALLOWED_EXTERNAL_DOMAINS = ['*.data.gouv.fr'] + DISCUSSION_ALTERNATE_MODEL_NAMES = [] + class Testing(object): '''Sane values for testing. Should be applied as override''' diff --git a/udata/templates/mail/discussion_closed.html b/udata/templates/mail/discussion_closed.html index c2143f0dc1..b9d74514e1 100644 --- a/udata/templates/mail/discussion_closed.html +++ b/udata/templates/mail/discussion_closed.html @@ -4,7 +4,7 @@ {% block body %}

{{ _('%(user)s closed an discussion on your %(type)s %(subject)s', - type=discussion.subject.verbose_name, + type=subject_type, user=( ''|safe + discussion.subject|string + ''|safe @@ -38,7 +38,7 @@ {{ mail_button( _('See the discussion'), - discussion.external_url + external_url ) }} diff --git a/udata/templates/mail/discussion_closed.txt b/udata/templates/mail/discussion_closed.txt index 8a07a43e71..00a33ef391 100644 --- a/udata/templates/mail/discussion_closed.txt +++ b/udata/templates/mail/discussion_closed.txt @@ -2,7 +2,7 @@ {% block body %} {{ _('%(user)s closed an discussion on your %(type)s %(subject)s', - type=discussion.subject.verbose_name, + type=subject_type, user=message.posted_by.fullname, subject=discussion.subject|string ) }}. @@ -12,5 +12,5 @@ {{ _('You can see the discussion on this page:') }} -{{ discussion.external_url }} +{{ external_url }} {% endblock %} diff --git a/udata/templates/mail/new_discussion.html b/udata/templates/mail/new_discussion.html index e165812f2f..64a1c4e334 100644 --- a/udata/templates/mail/new_discussion.html +++ b/udata/templates/mail/new_discussion.html @@ -4,7 +4,7 @@ {% block body %}

{{ _('%(user)s submitted a new discussion on your %(type)s %(subject)s', - type=discussion.subject.verbose_name, + type=subject_type, user=( ''|safe + discussion.subject|string + ''|safe @@ -36,7 +36,7 @@ {{ mail_button( _('See the discussion'), - discussion.external_url + external_url ) }} diff --git a/udata/templates/mail/new_discussion.txt b/udata/templates/mail/new_discussion.txt index 41bd6b9f30..ac26cca698 100644 --- a/udata/templates/mail/new_discussion.txt +++ b/udata/templates/mail/new_discussion.txt @@ -2,7 +2,7 @@ {% block body %} {{ _('%(user)s submitted a new discussion on your %(type)s %(subject)s', - type=discussion.subject.verbose_name, + type=subject_type, user=discussion.user.fullname, subject=discussion.subject|string ) }}. @@ -11,5 +11,5 @@ {{ _('Title') }}: {{ discussion.title }} {{ _('You can see the discussion on this page:') }} -{{ discussion.subject.external_url }} +{{ external_url }} {% endblock %} diff --git a/udata/templates/mail/new_discussion_comment.html b/udata/templates/mail/new_discussion_comment.html index 07f43711f7..4c3e935d1c 100644 --- a/udata/templates/mail/new_discussion_comment.html +++ b/udata/templates/mail/new_discussion_comment.html @@ -4,7 +4,7 @@ {% block body %}

{{ _('%(user)s commented an discussion on your %(type)s %(subject)s', - type=discussion.subject.verbose_name, + type=subject_type, user=( ''|safe + discussion.subject|string + ''|safe @@ -37,7 +37,7 @@ {{ mail_button( _('See the discussion'), - discussion.external_url + external_url ) }} diff --git a/udata/templates/mail/new_discussion_comment.txt b/udata/templates/mail/new_discussion_comment.txt index f200d11678..73c1706ca1 100644 --- a/udata/templates/mail/new_discussion_comment.txt +++ b/udata/templates/mail/new_discussion_comment.txt @@ -2,7 +2,7 @@ {% block body %} {{ _('%(user)s commented an discussion on your %(type)s %(subject)s', - type=discussion.subject.verbose_name, + type=subject_type, user=message.posted_by.fullname, subject=discussion.subject|string ) }}. @@ -12,5 +12,5 @@ {{ _('You can see the discussion on this page:') }} -{{ discussion.external_url }} +{{ external_url }} {% endblock %} diff --git a/udata/tests/test_discussions.py b/udata/tests/test_discussions.py index c7347c8142..b013eb4d42 100644 --- a/udata/tests/test_discussions.py +++ b/udata/tests/test_discussions.py @@ -19,6 +19,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.core.topic.factories import TopicFactory from udata.tests.helpers import capture_mails from udata.utils import faker @@ -138,7 +139,7 @@ def test_spam_by_owner(self): } }) self.assertStatus(response, 201) - + with assert_not_emit(on_new_potential_spam): response = self.post(url_for('api.discussion', id=response.json['id']), { 'comment': 'A comment with spam by owner' @@ -735,6 +736,130 @@ def test_new_discussion_mail(self): self.assertEqual(len(mails), 1) self.assertEqual(mails[0].recipients[0], owner.email) + @pytest.mark.options( + DISCUSSION_ALLOWED_EXTERNAL_DOMAINS=['*.example.com'], + DISCUSSION_ALTERNATE_MODEL_NAMES=['CUSTOM_MODEL'] + ) + def test_new_discussion_mail_with_extras(self): + user = UserFactory() + owner = UserFactory() + message = Message(content=faker.sentence(), posted_by=user) + discussion = Discussion.objects.create( + # Topic will be able to nofify if an external url is provided + subject=TopicFactory(owner=owner), + user=user, + title=faker.sentence(), + discussion=[message], + extras={ + 'notification': { + 'model_name': 'CUSTOM_MODEL', + 'external_url': 'https://sub.example.com/custom/' + } + }, + ) + + with capture_mails() as mails: + notify_new_discussion(discussion.id) + + mail = mails[0] + assert 'CUSTOM_MODEL' in mail.subject + assert 'CUSTOM_MODEL' in mail.body + assert f'https://sub.example.com/custom/#discussion-{discussion.id}' in mail.body + + @pytest.mark.options( + DISCUSSION_ALLOWED_EXTERNAL_DOMAINS=['*.example.com'], + ) + def test_new_discussion_mail_with_extras_no_model_name(self): + user = UserFactory() + owner = UserFactory() + message = Message(content=faker.sentence(), posted_by=user) + discussion = Discussion.objects.create( + # Topic will be able to nofify if an external url is provided + subject=TopicFactory(owner=owner), + user=user, + title=faker.sentence(), + discussion=[message], + extras={ + 'notification': { + 'external_url': 'https://sub.example.com/custom/' + } + }, + ) + + with capture_mails() as mails: + notify_new_discussion(discussion.id) + + mail = mails[0] + assert 'topic' in mail.subject + assert f'https://sub.example.com/custom/#discussion-{discussion.id}' in mail.body + + def test_new_discussion_mail_with_extras_not_allowed(self): + user = UserFactory() + owner = UserFactory() + message = Message(content=faker.sentence(), posted_by=user) + discussion = Discussion.objects.create( + # Topic has no external_url, so we're testing this case with Dataset + subject=DatasetFactory(owner=owner), + user=user, + title=faker.sentence(), + discussion=[message], + extras={ + 'notification': { + 'model_name': 'CUSTOM_MODEL', + 'external_url': 'https://sub.example.com/custom/' + } + }, + ) + + with capture_mails() as mails: + notify_new_discussion(discussion.id) + + mail = mails[0] + assert 'CUSTOM_MODEL' not in mail.subject + assert 'https://sub.example.com/custom/' not in mail.body + + def test_new_discussion_mail_with_extras_not_an_url(self): + user = UserFactory() + owner = UserFactory() + message = Message(content=faker.sentence(), posted_by=user) + discussion = Discussion.objects.create( + # Topic has no external_url, so we're testing this case with Dataset + subject=DatasetFactory(owner=owner), + user=user, + title=faker.sentence(), + discussion=[message], + extras={ + 'notification': { + 'external_url': 'xxx' + } + }, + ) + + with capture_mails() as mails: + notify_new_discussion(discussion.id) + + mail = mails[0] + assert 'xxx' not in mail.body + assert f"/{discussion.subject.slug}/#" in mail.body + + def test_new_discussion_mail_topic_no_extras(self): + user = UserFactory() + owner = UserFactory() + message = Message(content=faker.sentence(), posted_by=user) + discussion = Discussion.objects.create( + subject=TopicFactory(owner=owner), + user=user, + title=faker.sentence(), + discussion=[message], + ) + + with capture_mails() as mails: + notify_new_discussion(discussion.id) + + # Topic has no external_url, no email should be sent and no error thrown + assert len(mails) == 0 + + def test_new_discussion_comment_mail(self): owner = UserFactory() poster = UserFactory() @@ -761,6 +886,80 @@ def test_new_discussion_comment_mail(self): self.assertIn(mail.recipients[0], expected_recipients) self.assertNotIn(commenter.email, mail.recipients) + @pytest.mark.options( + DISCUSSION_ALLOWED_EXTERNAL_DOMAINS=['*.example.com'], + DISCUSSION_ALTERNATE_MODEL_NAMES=['CUSTOM_MODEL'] + ) + def test_new_discussion_comment_mail_with_extras(self): + owner = UserFactory() + poster = UserFactory() + commenter = UserFactory() + new_message = Message(content=faker.sentence(), posted_by=commenter) + discussion = Discussion.objects.create( + # Topic will be able to nofify if an external url is provided + subject=TopicFactory(owner=owner), + user=poster, + title=faker.sentence(), + discussion=[new_message], + extras={ + 'notification': { + 'model_name': 'CUSTOM_MODEL', + 'external_url': 'https://sub.example.com/custom/' + } + }, + ) + + with capture_mails() as mails: + notify_new_discussion_comment(discussion.id, message=0) + + mail = mails[0] + assert 'CUSTOM_MODEL' in mail.body + assert f'https://sub.example.com/custom/#discussion-{discussion.id}' in mail.body + + def test_new_discussion_comment_mail_with_extras_not_allowed(self): + owner = UserFactory() + poster = UserFactory() + commenter = UserFactory() + new_message = Message(content=faker.sentence(), posted_by=commenter) + discussion = Discussion.objects.create( + # Topic has no external_url, so we're testing this case with Dataset + subject=DatasetFactory(owner=owner), + user=poster, + title=faker.sentence(), + discussion=[new_message], + extras={ + 'notification': { + 'model_name': 'CUSTOM_MODEL', + 'external_url': 'https://sub.example.com/custom/' + } + }, + ) + + with capture_mails() as mails: + notify_new_discussion_comment(discussion.id, message=0) + + mail = mails[0] + assert 'CUSTOM_MODEL' not in mail.body + assert 'https://sub.example.com/custom/' not in mail.body + + def test_new_discussion_comment_mail_topic_no_extras(self): + owner = UserFactory() + poster = UserFactory() + commenter = UserFactory() + new_message = Message(content=faker.sentence(), posted_by=commenter) + discussion = Discussion.objects.create( + subject=TopicFactory(owner=owner), + user=poster, + title=faker.sentence(), + discussion=[new_message], + ) + + with capture_mails() as mails: + notify_new_discussion_comment(discussion.id, message=0) + + # Topic has no external_url, no email should be sent and no error thrown + assert len(mails) == 0 + def test_closed_discussion_mail(self): owner = UserFactory() poster = UserFactory() @@ -785,3 +984,79 @@ def test_closed_discussion_mail(self): for mail in mails: self.assertIn(mail.recipients[0], expected_recipients) self.assertNotIn(owner.email, mail.recipients) + + @pytest.mark.options( + DISCUSSION_ALLOWED_EXTERNAL_DOMAINS=['*.example.com'], + DISCUSSION_ALTERNATE_MODEL_NAMES=['CUSTOM_MODEL'] + ) + def test_closed_discussion_mail_with_extras(self): + owner = UserFactory() + poster = UserFactory() + commenter = UserFactory() + message = Message(content=faker.sentence(), posted_by=commenter) + closing_message = Message(content=faker.sentence(), posted_by=owner) + discussion = Discussion.objects.create( + subject=TopicFactory(owner=owner), + user=poster, + title=faker.sentence(), + discussion=[message, closing_message], + extras={ + 'notification': { + 'model_name': 'CUSTOM_MODEL', + 'external_url': 'https://sub.example.com/custom/' + } + }, + ) + + with capture_mails() as mails: + notify_discussion_closed(discussion.id, message=0) + + mail = mails[0] + assert 'CUSTOM_MODEL' in mail.body + assert f'https://sub.example.com/custom/#discussion-{discussion.id}' in mail.body + + def test_closed_discussion_mail_with_extras_not_allowed(self): + owner = UserFactory() + poster = UserFactory() + commenter = UserFactory() + message = Message(content=faker.sentence(), posted_by=commenter) + closing_message = Message(content=faker.sentence(), posted_by=owner) + discussion = Discussion.objects.create( + # Topic has no external_url, so we're testing this case with Dataset + subject=DatasetFactory(owner=owner), + user=poster, + title=faker.sentence(), + discussion=[message, closing_message], + extras={ + 'notification': { + 'model_name': 'CUSTOM_MODEL', + 'external_url': 'https://sub.example.com/custom/' + } + }, + ) + + with capture_mails() as mails: + notify_discussion_closed(discussion.id, message=0) + + mail = mails[0] + assert 'CUSTOM_MODEL' not in mail.body + assert 'https://sub.example.com/custom/' not in mail.body + + def test_closed_discussion_mail_topic_no_extras(self): + owner = UserFactory() + poster = UserFactory() + commenter = UserFactory() + message = Message(content=faker.sentence(), posted_by=commenter) + closing_message = Message(content=faker.sentence(), posted_by=owner) + discussion = Discussion.objects.create( + subject=TopicFactory(owner=owner), + user=poster, + title=faker.sentence(), + discussion=[message, closing_message], + ) + + with capture_mails() as mails: + notify_discussion_closed(discussion.id, message=0) + + # Topic has no external_url, no email should be sent and no error thrown + assert len(mails) == 0