Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a footer to email notifications #1000

Merged
merged 1 commit into from
Oct 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/895.added
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add a footer to email notifications with a link to the rule that generated it
1 change: 1 addition & 0 deletions fmn/core/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
13 changes: 12 additions & 1 deletion fmn/database/model/destination.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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}"}
Expand Down
13 changes: 13 additions & 0 deletions fmn/rules/notification.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
5 changes: 4 additions & 1 deletion fmn/sender/email.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions tests/consumer/test_consumer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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": "[email protected]"},
footer="Sent by Fedora Notifications: https://notifications.fedoraproject.org/rules/1",
)

result = await db_async_session.execute(select(model.Generated))
Expand Down
5 changes: 4 additions & 1 deletion tests/consumer/test_send_queue.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
38 changes: 34 additions & 4 deletions tests/database/model/test_destination.py
Original file line number Diff line number Diff line change
Expand Up @@ -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="[email protected]")
@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="[email protected]",
generation_rule_id=rule_obj.generation_rules[0].id,
)
db_async_session.add(d)

message = make_mocked_message(
topic="dummy",
body={
Expand All @@ -25,14 +45,21 @@ async def test_email(make_mocked_message):
assert result == {
"headers": {"To": "[email protected]", "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="[email protected]")
d = Destination(
id=1,
protocol="email",
address="[email protected]",
generation_rule_id=rule_obj.generation_rules[0].id,
)
db_async_session.add(d)
message = make_mocked_message(
topic="dummy",
body={
Expand All @@ -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": "[email protected]", "Subject": "[dummy] dummy summary"},
"body": "dummy content\nhttps://dummy.org/dummylink\nDUMMY EXTRA",
"footer": "Sent by Fedora Notifications: https://notifications.fedoraproject.org/rules/1",
}


Expand Down
24 changes: 16 additions & 8 deletions tests/database/model/test_rule.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand Down
11 changes: 11 additions & 0 deletions tests/rules/test_notification.py
Original file line number Diff line number Diff line change
@@ -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
22 changes: 22 additions & 0 deletions tests/sender/test_email.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 <[email protected]>"})
handler._smtp = smtp

await handler.handle(
{
"headers": {"To": "[email protected]", "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"] == "[email protected]"
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"