From 06bcb7700939cde17409d904aca036b5353bef15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Bompard?= Date: Fri, 29 Sep 2023 16:38:59 +0200 Subject: [PATCH] Add a footer to email notifications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: #895 Signed-off-by: Aurélien Bompard --- changelog.d/895.added | 1 + fmn/core/config.py | 1 + fmn/database/model/destination.py | 13 +++++++- fmn/rules/notification.py | 13 ++++++++ fmn/sender/email.py | 5 +++- tests/consumer/test_consumer.py | 1 + tests/consumer/test_send_queue.py | 5 +++- tests/database/model/test_destination.py | 38 +++++++++++++++++++++--- tests/database/model/test_rule.py | 24 ++++++++++----- tests/rules/test_notification.py | 11 +++++++ tests/sender/test_email.py | 22 ++++++++++++++ 11 files changed, 119 insertions(+), 15 deletions(-) create mode 100644 changelog.d/895.added create mode 100644 tests/rules/test_notification.py diff --git a/changelog.d/895.added b/changelog.d/895.added new file mode 100644 index 000000000..10dbeb600 --- /dev/null +++ b/changelog.d/895.added @@ -0,0 +1 @@ +Add a footer to email notifications with a link to the rule that generated it diff --git a/fmn/core/config.py b/fmn/core/config.py index 953b5460f..9f28e5129 100644 --- a/fmn/core/config.py +++ b/fmn/core/config.py @@ -60,6 +60,7 @@ class ServicesModel(BaseModel): class Settings(BaseSettings): + public_url: str = "https://notifications.fedoraproject.org" cors_origins: str = "https://notifications.fedoraproject.org" oidc_provider_url: str = "https://id.fedoraproject.org/openidc" oidc_conf_endpoint: str = "/.well-known/openid-configuration" diff --git a/fmn/database/model/destination.py b/fmn/database/model/destination.py index 53406a3f4..6cb8e285d 100644 --- a/fmn/database/model/destination.py +++ b/fmn/database/model/destination.py @@ -6,9 +6,11 @@ from typing import TYPE_CHECKING from httpx import AsyncClient, HTTPStatusError -from sqlalchemy import Column, ForeignKey, Integer, String, UnicodeText +from sqlalchemy import Column, ForeignKey, Integer, String, UnicodeText, select +from sqlalchemy.ext.asyncio import async_object_session from sqlalchemy.orm import relationship +from ...core.config import get_settings from ..main import Base from .generation_rule import GenerationRule @@ -37,17 +39,26 @@ class Destination(Base): async def generate(self, message: "Message") -> "Notification.content": app_name = f"[{message.app_name}] " if message.app_name else "" url = message.url if message.url else "" + settings = get_settings() if self.protocol == "email": body = f"{message!s}\n{url}" extra = await get_extra(message) if extra: body = f"{body}\n{extra}" + + # Find the URL of the Rule that generated this notification + session = async_object_session(self) + result = await session.execute( + select(GenerationRule).where(GenerationRule.id == self.generation_rule_id) + ) + rule_id = result.scalar_one().rule_id return { "headers": { "To": self.address, "Subject": f"{app_name}{message.summary}", }, "body": body, + "footer": f"Sent by Fedora Notifications: {settings.public_url}/rules/{rule_id}", } elif self.protocol == "irc": return {"to": self.address, "message": f"{app_name}{message.summary} {url}"} diff --git a/fmn/rules/notification.py b/fmn/rules/notification.py index 6b22448b0..518f1fbbf 100644 --- a/fmn/rules/notification.py +++ b/fmn/rules/notification.py @@ -19,6 +19,19 @@ class EmailNotificationHeaders(FrozenModel): class EmailNotificationContent(FrozenModel): headers: EmailNotificationHeaders body: str + footer: str | None = None + + def __eq__(self, other): + if isinstance(other, self.__class__): + return self.model_dump(exclude=["footer"]) == other.model_dump(exclude=["footer"]) + return super().__eq__(other) + + def __hash__(self): + # Don't include the footer in the hash to be able to use set() to de-duplicate + # notifications coming from different rules. + internal_dict = self.__dict__.copy() + del internal_dict["footer"] + return hash(self.__class__) + hash(tuple(internal_dict.values())) class EmailNotification(FrozenModel): diff --git a/fmn/sender/email.py b/fmn/sender/email.py index 45ea1d50b..6d434c956 100644 --- a/fmn/sender/email.py +++ b/fmn/sender/email.py @@ -28,7 +28,10 @@ async def handle(self, message): notif["From"] = self._config["from"] for name, value in message["headers"].items(): notif[name] = value - notif.set_content(message["body"]) + body = message["body"] + if message.get("footer") is not None: + body = f"{body}\n\n-- \n{message['footer']}" + notif.set_content(body) log.info("Sending email to %s with subject %s", notif["To"], notif["Subject"]) try: await self._smtp.send_message(notif) diff --git a/tests/consumer/test_consumer.py b/tests/consumer/test_consumer.py index 000011f8f..e5232b0d5 100644 --- a/tests/consumer/test_consumer.py +++ b/tests/consumer/test_consumer.py @@ -149,6 +149,7 @@ async def test_consumer_call_tracked( assert n.content == EmailNotificationContent( body="Body of message on dummy.topic\n", headers={"Subject": "Message on dummy.topic", "To": "dummy@example.com"}, + footer="Sent by Fedora Notifications: https://notifications.fedoraproject.org/rules/1", ) result = await db_async_session.execute(select(model.Generated)) diff --git a/tests/consumer/test_send_queue.py b/tests/consumer/test_send_queue.py index 44a6154e9..84f8d5d24 100644 --- a/tests/consumer/test_send_queue.py +++ b/tests/consumer/test_send_queue.py @@ -49,7 +49,10 @@ async def test_send_queue_send(connection, notif): sq._exchange.publish.assert_called_once() assert sq._exchange.publish.call_args.kwargs.get("routing_key") == "send.email" sent_msg = sq._exchange.publish.call_args.args[0] - assert sent_msg.body == b'{"headers": {"To": "dummy", "Subject": "dummy"}, "body": "dummy"}' + assert ( + sent_msg.body == b'{"headers": {"To": "dummy", "Subject": "dummy"}, ' + b'"body": "dummy", "footer": null}' + ) async def test_send_queue_close(connection): diff --git a/tests/database/model/test_destination.py b/tests/database/model/test_destination.py index 4884f30ef..4269aaf36 100644 --- a/tests/database/model/test_destination.py +++ b/tests/database/model/test_destination.py @@ -7,11 +7,31 @@ import httpx import pytest +from fmn.database import model from fmn.database.model.destination import Destination, get_extra -async def test_email(make_mocked_message): - d = Destination(id=1, protocol="email", address="dummy@example.com") +@pytest.fixture +async def rule_obj(db_async_session): + user = model.User(name="allkneelbeforezod") + tracking_rule = model.TrackingRule(name="datrackingrule") + generation_rules = [model.GenerationRule()] + rule = model.Rule(user=user, tracking_rule=tracking_rule, generation_rules=generation_rules) + db_async_session.add(rule) + await db_async_session.flush() + yield rule + await db_async_session.rollback() + + +async def test_email(make_mocked_message, db_async_session, rule_obj): + d = Destination( + id=1, + protocol="email", + address="dummy@example.com", + generation_rule_id=rule_obj.generation_rules[0].id, + ) + db_async_session.add(d) + message = make_mocked_message( topic="dummy", body={ @@ -25,14 +45,21 @@ async def test_email(make_mocked_message): assert result == { "headers": {"To": "dummy@example.com", "Subject": "[dummy] dummy summary"}, "body": "dummy content\nhttps://dummy.org/dummylink", + "footer": "Sent by Fedora Notifications: https://notifications.fedoraproject.org/rules/1", } -async def test_email_with_extra(make_mocked_message, mocker): +async def test_email_with_extra(make_mocked_message, mocker, db_async_session, rule_obj): mocker.patch( "fmn.database.model.destination.get_extra", mock.AsyncMock(return_value="DUMMY EXTRA") ) - d = Destination(id=1, protocol="email", address="dummy@example.com") + d = Destination( + id=1, + protocol="email", + address="dummy@example.com", + generation_rule_id=rule_obj.generation_rules[0].id, + ) + db_async_session.add(d) message = make_mocked_message( topic="dummy", body={ @@ -42,10 +69,13 @@ async def test_email_with_extra(make_mocked_message, mocker): "url": "https://dummy.org/dummylink", }, ) + result = await d.generate(message) + assert result == { "headers": {"To": "dummy@example.com", "Subject": "[dummy] dummy summary"}, "body": "dummy content\nhttps://dummy.org/dummylink\nDUMMY EXTRA", + "footer": "Sent by Fedora Notifications: https://notifications.fedoraproject.org/rules/1", } diff --git a/tests/database/model/test_rule.py b/tests/database/model/test_rule.py index ba3b0b47b..c87ea9e80 100644 --- a/tests/database/model/test_rule.py +++ b/tests/database/model/test_rule.py @@ -31,18 +31,26 @@ async def test_select_related(self, db_async_session, db_obj): assert len(rule.generation_rules) == 1 assert all(isinstance(gr, model.GenerationRule) for gr in rule.generation_rules) - async def test_handle_match(db_async_session, db_obj, mocker, make_mocked_message): + async def test_handle_match(self, db_async_session, db_obj, mocker, make_mocked_message): message = make_mocked_message(topic="dummy", body={"foo": "bar"}) - tr = model.TrackingRule(name="dummy") + tr = db_obj.tracking_rule + # tr = model.TrackingRule(name="dummy", rule_id=db_obj.id) tr_matches = mocker.patch.object(tr, "matches", return_value=True) - gr = model.GenerationRule() - db_obj.user = model.User(name="dummy") - db_obj.tracking_rule = tr - db_obj.generation_rules = [gr] + # gr = model.GenerationRule() + # db_obj.user = model.User(name="dummy") + # db_obj.tracking_rule = tr + # db_obj.generation_rules = [gr] + gr = db_obj.generation_rules[0] for i in range(1, 4): - gr.destinations.append(model.Destination(protocol="email", address=f"n{i}")) + db_async_session.add( + model.Destination(protocol="email", address=f"n{i}", generation_rule=gr) + ) + await db_async_session.flush() requester = Mock() - result = [n async for n in db_obj.handle(message, requester)] + rule_result = await db_async_session.execute( + db_obj.select_related().filter_by(id=db_obj.id) + ) + result = [n async for n in rule_result.scalar_one().handle(message, requester)] tr_matches.assert_called_once_with(message, requester) assert len(result) == 3 assert [n.content.headers.model_dump()["To"] for n in result] == ["n1", "n2", "n3"] diff --git a/tests/rules/test_notification.py b/tests/rules/test_notification.py new file mode 100644 index 000000000..18f25f2ad --- /dev/null +++ b/tests/rules/test_notification.py @@ -0,0 +1,11 @@ +# SPDX-FileCopyrightText: Contributors to the Fedora Project +# +# SPDX-License-Identifier: MIT + +from fmn.rules.notification import EmailNotificationContent, IRCNotificationContent + + +def test_compare_other(): + content = EmailNotificationContent(headers={"To": "dummy", "Subject": "dummy"}, body="dummy") + other = IRCNotificationContent(to="dummy", message="dummy") + assert content != other diff --git a/tests/sender/test_email.py b/tests/sender/test_email.py index 7aea025f5..4b17a67e6 100644 --- a/tests/sender/test_email.py +++ b/tests/sender/test_email.py @@ -54,3 +54,25 @@ async def test_email_disconnected(): assert smtp.send_message.call_count == 2 smtp.connect.assert_called_once_with() + + +async def test_email_handle_with_footer(): + smtp = MagicMock(spec=SMTP) + handler = EmailHandler({"from": "FMN "}) + handler._smtp = smtp + + await handler.handle( + { + "headers": {"To": "dest@example.com", "Subject": "Testing"}, + "body": "This is a test", + "footer": "This is a footer.", + } + ) + + smtp.send_message.assert_called_once() + + sent = smtp.send_message.call_args[0][0] + assert sent["To"] == "dest@example.com" + assert sent["Subject"] == "Testing" + assert sent.get_body().get_content() == "This is a test\n\n-- \nThis is a footer.\n" + assert sent.get_content_type() == "text/plain"