diff --git a/config.py.example b/config.py.example index a6589f3e..862e9054 100644 --- a/config.py.example +++ b/config.py.example @@ -97,6 +97,7 @@ ADMINS = [ SMTP_SERVER = "metabrainz-mail" SMTP_PORT = 25 MAIL_FROM_DOMAIN = "metabrainz.org" +MB_MAIL_SERVER_URI = "http://localhost:3000" # OTHER STUFF diff --git a/metabrainz/db/notification.py b/metabrainz/db/notification.py index 7590c902..9a69ce6c 100644 --- a/metabrainz/db/notification.py +++ b/metabrainz/db/notification.py @@ -4,7 +4,7 @@ from datetime import datetime from typing import List, Tuple, Optional from flask import current_app -from psycopg2.extras import execute_values +from psycopg2.extras import execute_values, RealDictCursor from metabrainz import db @@ -125,14 +125,14 @@ def delete_notifications(user_id: int, delete_ids: Tuple[int, ...]): connection.execute(delete_query, {'user_id': user_id, 'delete_ids': delete_ids}) -def insert_notifications(notifications: List[dict]) -> List[tuple[int]]: +def insert_notifications(notifications: List[dict]) -> List[dict]: """ Inserts a batch of notifications into the table. Args: notifications (List[dict]): List of notifications to be inserted. Returns: - List[tuple[int]]: List of tuples with each tuple contaning ID of an inserted notification. + List[dict]: List of inserted notifications. """ params = _prepare_notifications(notifications) insert_query = """ @@ -147,7 +147,7 @@ def insert_notifications(notifications: List[dict]) -> List[tuple[int]]: VALUES %s RETURNING - id + * """ template = """ ( @@ -160,14 +160,14 @@ def insert_notifications(notifications: List[dict]) -> List[tuple[int]]: conn = db.engine.raw_connection() try: - with conn.cursor() as cur: + with conn.cursor(cursor_factory=RealDictCursor) as cur: execute_values(cur, insert_query, params, template) - notification_ids = cur.fetchall() + inserted_notifications = cur.fetchall() conn.commit() finally: conn.close() - return notification_ids + return inserted_notifications def _prepare_notifications(notifications: List[dict]) -> List[dict]: diff --git a/metabrainz/mail.py b/metabrainz/mail.py index 2bb2cb64..1831e74f 100644 --- a/metabrainz/mail.py +++ b/metabrainz/mail.py @@ -1,9 +1,10 @@ import uuid import orjson +import requests from flask import current_app -from typing import List from brainzutils import cache from brainzutils.mail import send_mail + from metabrainz.db.notification import ( filter_non_digest_notifications, get_digest_notifications, @@ -17,27 +18,35 @@ class NotificationSender: def __init__(self, notifications): self.notifications = notifications - def _send_notifications(self, notifications: List[dict]): + def _send_notifications(self, notifications: list[dict]): """Helper function to send immediate notifications through mail. If sending fails, the notification is stored in cache for later retry. """ for notification in notifications: try: - send_mail( - subject=notification["subject"], - text=notification["body"], - recipients=[notification["to"]], - from_addr=notification["sent_from"], - ) + if notification.get("subject") and notification.get("body"): + send_mail( + subject=notification["subject"], + text=notification["body"], + recipients=[notification["recipient"]], + from_addr=notification["sent_from"], + ) + elif notification.get("template_id") and notification.get("template_params"): + self.send_html_mail( + from_addr=notification["sent_from"], + template_id=notification["template_id"], + template_params=notification["template_params"], + recipient=notification["recipient"], + message_id=notification["email_id"], + ) + except Exception as err: current_app.logger.error( "Failed to send mail to recipient %s, %s", - notification["to"], + notification["recipient"], str(err), ) - cache.hset( - FAILED_MAIL_HASH_KEY, str(uuid.uuid4()), orjson.dumps(notification) - ) + cache.hset(FAILED_MAIL_HASH_KEY, str(uuid.uuid4()), orjson.dumps(notification)) def send_immediate_notifications(self): """Sends notifications that are marked as important or are for users with notifications enabled and digest disabled.""" @@ -47,10 +56,7 @@ def send_immediate_notifications(self): for notification in self.notifications: if notification["important"] and notification["send_email"]: - if notification.get("subject") and notification.get("body"): - immediate_notifications.append(notification) - else: - pass # TODO: integrate mb-mail + immediate_notifications.append(notification) elif not notification["important"] and notification["send_email"]: unimportant_notifications.append(notification) @@ -69,3 +75,62 @@ def send_digest_notifications(self): notifications = get_digest_notifications() self._send_notifications(notifications) mark_notifications_sent(notifications) + + def send_html_mail( + self, + from_addr: str, + recipient: str, + template_id: str, + template_params: dict, + language: str | None = None, + in_reply_to: list[str] | None = [], + sender: str | None = None, + references: list[str] | None = [], + reply_to: str | None = None, + message_id: str | None = None, + ): + """ + Send HTML email through MB mail service. + + Args: + from_addr (str): + The email address of the sender. + recipient (str): + The email address of the recipient. + template_id (str): + MB mail service template id for the required template. + template_params (dict): + Dictionary containing the data to be passed into the template. + language (str) : + Optional. The language preference of the recipient. + in_reply_to (list[str]): + Optional. list of Message ID of the emails thats being replied to. + sender (str): + Optional. The address ultimately sending the email Should not be set if same as from address. + references (list[str]): + Optional. list of Message ID of the emails that this email references. + reply_to (str): + Optional. Reply-To email header. + message_id (str): + Optional. UUID for the email. + + Raises: + HTTPError: If sending email fails. + """ + + single_mail_endpoint = current_app.config["MB_MAIL_SERVER_URI"] + "/send_single" + data = { + "from": from_addr, + "to": recipient, + "template_id": template_id, + "params": template_params, + "language": language, + "in_reply_to": in_reply_to, + "sender": sender, + "references": references, + "reply_to": reply_to, + "message_id": message_id, + } + + response = requests.post(url=single_mail_endpoint, json=data) + response.raise_for_status() diff --git a/metabrainz/mail_test.py b/metabrainz/mail_test.py new file mode 100644 index 00000000..68a269b5 --- /dev/null +++ b/metabrainz/mail_test.py @@ -0,0 +1,154 @@ +from unittest import mock +import requests +import requests_mock +import orjson + +from flask import current_app +from metabrainz.testing import FlaskTestCase +from metabrainz.mail import NotificationSender, FAILED_MAIL_HASH_KEY + + +class NotificationSenderTest(FlaskTestCase): + def setUp(self): + super(NotificationSenderTest, self).setUp() + self.notifications = [ + { + "musicbrainz_row_id": 3, + "project": "metabrainz", + "recipient": "user@example.com", + "sent_from": "noreply@project.org", + "reply_to": "noreply@project.org", + "send_email": True, + "subject": "test", + "body": "test-123", + "important": False, + "expire_age": 30, + "email_id": "c7d7b431-7130-4a3c-ad8a-17cdd5ebdf2d", + "id": 6, + }, + { + "musicbrainz_row_id": 1, + "project": "musicbrainz", + "recipient": "user@example.com", + "sent_from": "noreply@project.org", + "reply_to": "noreply@project.org", + "send_email": True, + "template_id": "verify-email", + "template_params": {"reason": "verify"}, + "important": True, + "expire_age": 30, + "email_id": "c512e637-21ff-45b1-a58a-03412224b48b", + "id": 4, + }, + ] + self.service = NotificationSender(self.notifications) + + @requests_mock.Mocker() + def test_send_html_mail(self, mock_request): + single_mail_endpoint = current_app.config["MB_MAIL_SERVER_URI"] + "/send_single" + mock_request.post(single_mail_endpoint, status_code=200) + expected_payload = { + "from": self.notifications[1]["sent_from"], + "to": self.notifications[1]["recipient"], + "template_id": self.notifications[1]["template_id"], + "params": self.notifications[1]["template_params"], + "language": None, + "in_reply_to": [], + "sender": None, + "references": [], + "reply_to": None, + "message_id": self.notifications[1]["email_id"], + } + + self.service.send_html_mail( + from_addr=self.notifications[1]["sent_from"], + recipient=self.notifications[1]["recipient"], + template_id=self.notifications[1]["template_id"], + template_params=self.notifications[1]["template_params"], + message_id=self.notifications[1]["email_id"] + ) + + self.assertEqual(mock_request.last_request.json(), expected_payload) + + # Error on the mb-mail server side. + mock_request.post(single_mail_endpoint, status_code=500) + + with self.assertRaises(requests.exceptions.HTTPError) as err: + self.service.send_html_mail( + from_addr=self.notifications[1]["sent_from"], + recipient=self.notifications[1]["recipient"], + template_id=self.notifications[1]["template_id"], + template_params=self.notifications[1]["template_params"], + message_id=self.notifications[1]["email_id"] + ) + + self.assertEqual(err.exception.response.status_code, 500) + + @mock.patch("metabrainz.mail.cache") + @mock.patch("metabrainz.mail.NotificationSender.send_html_mail") + @mock.patch("metabrainz.mail.send_mail") + def test_send_notifications(self, mock_send_mail, mock_send_html_mail, mock_cache): + mock_send_mail.return_value = None + mock_send_html_mail.return_value = None + + self.service._send_notifications(self.notifications) + + # Plain text mail notification. + mock_send_mail.assert_called_once_with( + subject=self.notifications[0]["subject"], + text=self.notifications[0]["body"], + recipients=[self.notifications[0]["recipient"]], + from_addr=self.notifications[0]["sent_from"], + ) + # HTML mail notification. + mock_send_html_mail.assert_called_once_with( + from_addr=self.notifications[1]["sent_from"], + recipient=self.notifications[1]["recipient"], + template_id=self.notifications[1]["template_id"], + template_params=self.notifications[1]["template_params"], + message_id=self.notifications[1]["email_id"] + ) + + # MB-mail failure + mock_send_html_mail.side_effect = Exception() + mock_cache.return_value = 1 + + self.service._send_notifications(self.notifications) + + args, _ = mock_cache.hset.call_args + self.assertEqual(args[0], FAILED_MAIL_HASH_KEY) + self.assertIsInstance(args[1], str) + self.assertEqual(args[2], orjson.dumps(self.notifications[1])) + + + + @mock.patch("metabrainz.mail.NotificationSender._send_notifications") + @mock.patch("metabrainz.mail.mark_notifications_sent") + @mock.patch("metabrainz.mail.filter_non_digest_notifications") + def test_send_immediate_notifications(self, mock_filter, mock_mark_sent, mock_send): + mock_filter.return_value = [] + mock_mark_sent.return_value = None + mock_send.return_value = None + + self.service.send_immediate_notifications() + + # Non-important notification + mock_filter.assert_called_once_with([self.notifications[0]]) + # Immediate notification + mock_send.assert_called_once_with([self.notifications[1]]) + mock_mark_sent.has_called_with([self.notifications[1]]) + + @mock.patch("metabrainz.mail.mark_notifications_sent") + @mock.patch("metabrainz.mail.get_digest_notifications") + @mock.patch("metabrainz.mail.NotificationSender._send_notifications") + def test_send_digest_notifications(self, mock_send, mock_get, mock_mark): + mock_send.return_value = None + mock_get.return_value = [self.notifications[0]] + mock_mark.return_value = None + + self.service.send_digest_notifications() + + mock_get.assert_called_once() + mock_send.assert_called_once_with([self.notifications[0]]) + mock_mark.assert_called_once_with([self.notifications[0]]) + diff --git a/metabrainz/notifications/views.py b/metabrainz/notifications/views.py index 785244d2..2fa8bc83 100644 --- a/metabrainz/notifications/views.py +++ b/metabrainz/notifications/views.py @@ -327,12 +327,9 @@ def send_notifications(): raise APIBadRequest(f'Notification {idx} should include either subject and body or template_id and template_params.') try: - notification_ids = insert_notifications(data) + notifications = insert_notifications(data) - for index, id_tuple in enumerate(notification_ids): - data[index]["id"] = id_tuple[0] - - sender = NotificationSender(data) + sender = NotificationSender(notifications) sender.send_immediate_notifications() except Exception as err: diff --git a/metabrainz/notifications/views_test.py b/metabrainz/notifications/views_test.py index a0e2dcf3..c1cfeaee 100644 --- a/metabrainz/notifications/views_test.py +++ b/metabrainz/notifications/views_test.py @@ -166,16 +166,11 @@ def test_remove_notifications(self, mock_requests, mock_delete): self.assertEqual(res.json['error'], 'Cannot delete notifications right now.') @requests_mock.Mocker() + @mock.patch('metabrainz.mail.NotificationSender.send_immediate_notifications') @mock.patch('metabrainz.notifications.views.insert_notifications') - def test_send_notifications(self, mock_requests, mock_insert): - mock_insert.return_value= [(1, ), (3, )] - mock_requests.post(self.introspect_url, json={ - "active": True, - "client_id": "abc", - "scope": ["notification"], - }) + def test_send_notifications(self, mock_requests, mock_insert, mock_mail): test_data = [ - { + { "id": 102, "user_id": 1, "project": "listenbrainz", "to": "user1@example.com", @@ -188,7 +183,7 @@ def test_send_notifications(self, mock_requests, mock_insert): "email_id": "scam-email-3421435", "send_email": True }, - { + { "id": 103, "user_id": 3, "project": "musicbrainz", "to": "user3@example.com", @@ -202,6 +197,13 @@ def test_send_notifications(self, mock_requests, mock_insert): "send_email": True } ] + mock_mail.return_value = None + mock_insert.return_value= test_data + mock_requests.post(self.introspect_url, json={ + "active": True, + "client_id": "abc", + "scope": ["notification"], + }) url = "notification/send" headers = {"Authorization": "Bearer good_token"}