ref: Tweaks to send email task (#833)

codecov · Oct 29, 2024 · 9c74b4e · 9c74b4e
1 parent bcb07ef
commit 9c74b4e
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 77 deletions.
diff --git a/tasks/send_email.py b/tasks/send_email.py
@@ -1,10 +1,10 @@
 import logging
 
 from shared.celery_config import send_email_task_name
+from shared.config import get_config
 
 import services.smtp
 from app import celery_app
-from database.models import Owner
 from helpers.email import Email
 from helpers.metrics import metrics
 from services.template import TemplateService
@@ -15,37 +15,26 @@
 
 class SendEmailTask(BaseCodecovTask, name=send_email_task_name):
     def run_impl(
-        self, db_session, ownerid, template_name, from_addr, subject, **kwargs
+        self, db_session, to_addr, subject, template_name, from_addr=None, **kwargs
     ):
         with metrics.timer("worker.tasks.send_email"):
+            if from_addr is None:
+                from_addr = get_config(
+                    "services", "smtp", "from_address", default="[email protected]"
+                )
+
             log_extra_dict = {
+                "to_addr": to_addr,
                 "from_addr": from_addr,
                 "template_name": template_name,
                 "template_kwargs": kwargs,
-                "ownerid": ownerid,
             }
 
             log.info(
                 "Received send email task",
                 extra=log_extra_dict,
             )
 
-            owner = db_session.query(Owner).filter_by(ownerid=ownerid).first()
-            if not owner:
-                log.warning(
-                    "Unable to find owner",
-                    extra=log_extra_dict,
-                )
-                return {"email_successful": False, "err_msg": "Unable to find owner"}
-
-            to_addr = owner.email
-            if not owner.email:
-                log.warning("Owner does not have email", extra=log_extra_dict)
-                return {
-                    "email_successful": False,
-                    "err_msg": "Owner does not have email",
-                }
-
             smtp_service = services.smtp.SMTPService()
 
             if not smtp_service.active():

diff --git a/tasks/tests/integration/test_send_email_task.py b/tasks/tests/integration/test_send_email_task.py
@@ -39,10 +39,10 @@ def test_send_email_integration(
 
         result = task.run_impl(
             dbsession,
-            owner.ownerid,
+            owner.email,
+            "TestSubject",
             "test",
             "[email protected]",
-            "TestSubject",
             username=owner.username,
         )
 

diff --git a/tasks/tests/unit/test_send_email_task.py b/tasks/tests/unit/test_send_email_task.py
@@ -58,51 +58,49 @@ def test_send_email(
         dbsession.flush()
         result = SendEmailTask().run_impl(
             db_session=dbsession,
-            ownerid=owner.ownerid,
-            from_addr="[email protected]",
-            template_name="test",
+            to_addr=owner.email,
             subject="Test",
+            template_name="test",
+            from_addr="[email protected]",
             username="test_username",
         )
 
         assert result == {"email_successful": True, "err_msg": None}
 
-    def test_send_email_non_existent_template(
+    def test_send_email_uses_default_from_addr(
         self, mocker, mock_configuration, dbsession, mock_smtp
     ):
+        mock_smtp.configure_mock(**{"send.return_value": None})
         owner = OwnerFactory.create(email=to_addr)
         dbsession.add(owner)
         dbsession.flush()
-        with pytest.raises(TemplateNotFound):
-            result = SendEmailTask().run_impl(
-                db_session=dbsession,
-                ownerid=owner.ownerid,
-                from_addr="[email protected]",
-                template_name="non_existent",
-                subject="Test",
-                username="test_username",
-            )
-
-    def test_send_email_no_owner(
-        self, mocker, mock_configuration, dbsession, mock_smtp
-    ):
-        owner = OwnerFactory.create(email=None)
-        dbsession.add(owner)
-        dbsession.flush()
         result = SendEmailTask().run_impl(
             db_session=dbsession,
-            ownerid=owner.ownerid,
-            from_addr="[email protected]",
-            template_name="test",
+            to_addr=owner.email,
             subject="Test",
+            template_name="test",
             username="test_username",
         )
-
         assert result == {
-            "email_successful": False,
-            "err_msg": "Owner does not have email",
+            "email_successful": True,
+            "err_msg": None,
         }
 
+    def test_send_email_non_existent_template(
+        self, mocker, mock_configuration, dbsession, mock_smtp
+    ):
+        owner = OwnerFactory.create(email=to_addr)
+        dbsession.add(owner)
+        dbsession.flush()
+        with pytest.raises(TemplateNotFound):
+            _ = SendEmailTask().run_impl(
+                db_session=dbsession,
+                to_addr=owner.email,
+                subject="Test",
+                template_name="non_existent",
+                username="test_username",
+            )
+
     def test_send_email_missing_kwargs(
         self, mocker, mock_configuration, dbsession, mock_smtp
     ):
@@ -112,25 +110,11 @@ def test_send_email_missing_kwargs(
         with pytest.raises(UndefinedError):
             result = SendEmailTask().run_impl(
                 db_session=dbsession,
-                ownerid=owner.ownerid,
-                from_addr="[email protected]",
+                to_addr=owner.email,
                 subject="Test",
                 template_name="test",
             )
 
-    def test_send_email_invalid_owner_no_list_type(
-        self, mocker, mock_configuration, dbsession, mock_smtp
-    ):
-        result = SendEmailTask().run_impl(
-            db_session=dbsession,
-            ownerid=99999999,
-            from_addr="[email protected]",
-            template_name="test",
-            subject="Test",
-            username="test_username",
-        )
-        assert result == {"email_successful": False, "err_msg": "Unable to find owner"}
-
     def test_send_email_recipients_refused(
         self, mocker, mock_configuration, dbsession, mock_smtp
     ):
@@ -143,10 +127,9 @@ def test_send_email_recipients_refused(
         dbsession.flush()
         result = SendEmailTask().run_impl(
             db_session=dbsession,
-            ownerid=owner.ownerid,
-            from_addr="[email protected]",
-            template_name="test",
+            to_addr=owner.email,
             subject="Test",
+            template_name="test",
             username="test_username",
         )
 
@@ -164,10 +147,9 @@ def test_send_email_sender_refused(
         dbsession.flush()
         result = SendEmailTask().run_impl(
             db_session=dbsession,
-            ownerid=owner.ownerid,
-            from_addr="[email protected]",
-            template_name="test",
+            to_addr=owner.email,
             subject="Test",
+            template_name="test",
             username="test_username",
         )
         assert result["email_successful"] == False
@@ -188,10 +170,9 @@ def test_send_email_data_error(
         dbsession.flush()
         result = SendEmailTask().run_impl(
             db_session=dbsession,
-            ownerid=owner.ownerid,
-            from_addr="[email protected]",
-            template_name="test",
+            to_addr=owner.email,
             subject="Test",
+            template_name="test",
             username="test_username",
         )
         assert result["email_successful"] == False
@@ -208,10 +189,9 @@ def test_send_email_sends_errs(
         dbsession.flush()
         result = SendEmailTask().run_impl(
             db_session=dbsession,
-            ownerid=owner.ownerid,
-            from_addr="[email protected]",
-            template_name="test",
+            to_addr=owner.email,
             subject="Test",
+            template_name="test",
             username="test_username",
         )
         assert result["email_successful"] == False
@@ -226,10 +206,9 @@ def test_send_email_no_smtp_config(
         dbsession.flush()
         result = SendEmailTask().run_impl(
             db_session=dbsession,
-            ownerid=owner.ownerid,
-            from_addr="[email protected]",
-            template_name="test",
+            to_addr=owner.email,
             subject="Test",
+            template_name="test",
             username="test_username",
         )
         assert result == {