SIENTIAPDE-1172

fix: add conditional login for SMTP server and enhance test coverage

- Updated the Email class to conditionally log in to the SMTP server only if a sender password is provided, improving security.
- Enhanced test cases for the try_send_email method to cover various scenarios, including successful email sending and handling SMTP disconnections.

orchestrator/activities/email.py

@@ -125,8 +125,9 @@ class Email(BaseActivity):
 
             self.server = smtplib.SMTP(
                 self.smtp_server, self.smtp_port, timeout=20)
-            self.server.starttls()
+            if self.sender_password:
-            self.server.login(self.sender_email, self.sender_password)
+                self.server.starttls()
+                self.server.login(self.sender_email, self.sender_password)
             self.server.sendmail(self.sender_email, receivers, msg.as_string())
 
     @activity.defn(name="send_email")

tests/orchestrator/activities/test_email.py

@@ -1,3 +1,4 @@
+from smtplib import SMTPServerDisconnected
 from unittest.mock import MagicMock, call, patch
 from pytest import mark, fixture
 
@@ -37,7 +38,7 @@ def test___init___with_password(smtplib, email_builder):
     assert email.sender_password == "test"
     assert email.smtp_port == 587
 
-    smtplib.SMTP.assert_called_once_with("test", 587)
+    smtplib.SMTP.assert_called_once_with("test", 587, timeout=20)
     smtplib.SMTP.return_value.starttls.assert_called_once()
     smtplib.SMTP.return_value.login.assert_called_once_with(
         "test@test.com", "test")
@@ -61,7 +62,7 @@ def test___init___without_password(smtplib, email_builder):
     assert email.sender_password is None
     assert email.smtp_port == 587
 
-    smtplib.SMTP.assert_called_once_with("test", 587)
+    smtplib.SMTP.assert_called_once_with("test", 587, timeout=20)
     assert email.server == smtplib.SMTP.return_value
 
 
@@ -198,6 +199,83 @@ def test_handle_attachments_failure(mime_base, email):
     assert message.attach.call_count == 0
 
 
+def test_try_send_email_success(email):
+    email.server.sendmail = MagicMock()
+
+    msg = MagicMock()
+
+    email.try_send_email(msg, "test")
+
+    email.server.sendmail.assert_called_once_with(
+        "test@test.com", "test", msg.as_string.return_value)
+
+
+@patch('orchestrator.activities.email.smtplib.SMTP')
+def test_try_send_email_reconnect_quit_success(smtp, email):
+    email.server.sendmail = MagicMock(
+        side_effect=SMTPServerDisconnected("test")
+    )
+    email.server.quit = MagicMock()
+
+    msg = MagicMock()
+
+    email.try_send_email(msg, "test")
+
+    smtp.assert_has_calls([
+        call("test", 587, timeout=20)
+    ])
+
+    smtp.return_value.starttls.assert_called_once()
+    smtp.return_value.login.assert_called_once_with(
+        "test@test.com", "test")
+
+    smtp.return_value.sendmail.assert_called_once_with(
+        "test@test.com", "test", msg.as_string.return_value)
+
+
+@patch('orchestrator.activities.email.smtplib.SMTP')
+def test_try_send_email_reconnect_quit_failure_disconnect(smtp, email):
+    email.server.sendmail = MagicMock(
+        side_effect=SMTPServerDisconnected("test")
+    )
+    email.server.quit = MagicMock(
+        side_effect=SMTPServerDisconnected("test")
+    )
+
+    msg = MagicMock()
+
+    email.try_send_email(msg, "test")
+
+    smtp.assert_has_calls([
+        call("test", 587, timeout=20)
+    ])
+    smtp.return_value.starttls.assert_called_once()
+    smtp.return_value.login.assert_called_once_with(
+        "test@test.com", "test")
+
+    smtp.return_value.sendmail.assert_called_once_with(
+        "test@test.com", "test", msg.as_string.return_value)
+
+
+@patch('orchestrator.activities.email.smtplib.SMTP')
+def test_try_send_email_reconnect_quit_failure(smtp, email):
+    email.server.sendmail = MagicMock(
+        side_effect=SMTPServerDisconnected("test")
+    )
+    email.server.quit = MagicMock(
+        side_effect=Exception("test")
+    )
+
+    msg = MagicMock()
+
+    try:
+        email.try_send_email(msg, "test")
+    except Exception as e:
+        assert str(e) == "test"
+    else:
+        assert False, "Expected exception"
+
+
 @mark.asyncio
 @patch('orchestrator.activities.email.MIMEText')
 @patch('orchestrator.activities.email.MIMEMultipart')
@@ -211,7 +289,7 @@ async def test_send_email(mimemultipart, mimetext, email):
 
     email.email_builder.send_notification = MagicMock()
 
-    email.server.sendmail = MagicMock(
+    email.try_send_email = MagicMock(
         side_effect=[
             None,
             Exception("test")
@@ -271,11 +349,11 @@ async def test_send_email(mimemultipart, mimetext, email):
         ]
     )
 
-    email.server.sendmail.assert_has_calls(
+    email.try_send_email.assert_has_calls(
         [
-            call('test@test.com', 'test@test.com, test2@test.com',
+            call(side_effect_1, 'test@test.com, test2@test.com'),
-                 side_effect_1.as_string.return_value),
+            call(side_effect_2, 'test3@test.com, test4@test.com')
-            call('test@test.com', 'test3@test.com, test4@test.com',
-                 side_effect_2.as_string.return_value)
         ]
     )
+
+    assert email.try_send_email.call_count == 2