Add SMTPService and Email helper #110

joseph-sentry · 2023-09-21T16:35:09Z

Email helper class that wraps EmailMessage
SMTPService class that creates connection to SMTP server using config options in services.smtp
send method on SMTPService that takes Email as an arg to send an email using the SMTP connection

codecov-staging · 2023-09-21T16:41:12Z

Codecov Report

Merging #110 (1e9f32a) into main (3428227) will increase coverage by 0.04%.
The diff coverage is 99.52%.

@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
+ Coverage   93.20%   93.25%   +0.04%     
==========================================
  Files         343      346       +3     
  Lines       26679    26889     +210     
==========================================
+ Hits        24866    25075     +209     
- Misses       1813     1814       +1

Flag	Coverage Δ
integration	`93.25% <99.52%> (+0.04%)`	⬆️
latest-uploader-overall	`93.25% <99.52%> (+0.04%)`	⬆️
unit	`93.25% <99.52%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
NonTestCode	`93.96% <98.92%> (+0.04%)`	⬆️
OutsideTasks	`96.72% <99.52%> (+0.03%)`	⬆️

Files	Coverage Δ
conftest.py	`88.09% <ø> (ø)`
helpers/email.py	`100.00% <100.00%> (ø)`
services/tests/test_smtp.py	`100.00% <100.00%> (ø)`
services/smtp.py	`98.79% <98.79%> (ø)`

codecov-public-qa · 2023-09-21T16:41:51Z

Codecov Report

Merging #110 (1e9f32a) into main (3428227) will increase coverage by 0.04%.
The diff coverage is 99.52%.

@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
+ Coverage   93.20%   93.25%   +0.04%     
==========================================
  Files         343      346       +3     
  Lines       26679    26889     +210     
==========================================
+ Hits        24866    25075     +209     
- Misses       1813     1814       +1

Flag	Coverage Δ
integration	`93.25% <99.52%> (+0.04%)`	⬆️
latest-uploader-overall	`93.25% <99.52%> (+0.04%)`	⬆️
unit	`93.25% <99.52%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
NonTestCode	`93.96% <98.92%> (+0.04%)`	⬆️
OutsideTasks	`96.72% <99.52%> (+0.03%)`	⬆️

Files Changed	Coverage Δ
conftest.py	`88.09% <ø> (ø)`
services/smtp.py	`98.79% <98.79%> (ø)`
helpers/email.py	`100.00% <100.00%> (ø)`
services/tests/test_smtp.py	`100.00% <100.00%> (ø)`

codecov · 2023-09-21T16:46:13Z

Codecov Report

Merging #110 (1e9f32a) into main (3428227) will increase coverage by 0.00%.
The diff coverage is 99.52%.

@@           Coverage Diff            @@
##             main     #110    +/-   ##
========================================
  Coverage   98.45%   98.46%            
========================================
  Files         366      369     +3     
  Lines       27023    27233   +210     
========================================
+ Hits        26605    26814   +209     
- Misses        418      419     +1

Flag	Coverage Δ
integration	`98.43% <99.52%> (+5.22%)`	⬆️
latest-uploader-overall	`98.43% <99.52%> (+5.22%)`	⬆️
unit	`98.43% <99.52%> (+5.22%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
NonTestCode	`97.05% <98.92%> (+0.01%)`	⬆️
OutsideTasks	`98.22% <99.52%> (+1.53%)`	⬆️

Files	Coverage Δ
conftest.py	`94.44% <ø> (ø)`
helpers/email.py	`100.00% <100.00%> (ø)`
services/tests/test_smtp.py	`100.00% <100.00%> (ø)`
services/smtp.py	`98.79% <98.79%> (ø)`

This change has been scanned for critical changes. Learn more

giovanni-guidini

I think it's a good start.
We might add error handling / retries / metrics later on

Just the Test class name that I'd like to double check. The name is not very descriptive of what it does.

giovanni-guidini · 2023-09-25T13:46:41Z

services/tests/test_smtp.py

+from services.smtp import get_smtp_service
+
+
+class TestStorage(object):


The class name 👀

services/smtp.py

services/tests/test_smtp.py

giovanni-guidini · 2023-09-28T12:38:09Z

services/smtp.py

+_smtp_service = None
+
+
+def get_smtp_service():


I understand why you are doing this. It might be that the service is not configured, and it makes it possible to re-use connections. I think those are good ideas.

However the pattern we use to instantiate services is to do that directly. It may cause confusion to other devs that try to instantiate the service directly cause they don't know they have to use this function.

My recommendation that you leave the connection aspect in this global / cached situation, and when instantiating the SMTPService() you use the cached connection, if available.

Also that you add

@classmethod def active(self): return self._conn is not None

for us to know if we can use the SMTP service or not.
(and an extra check in send to see if the connection is not None either

i am less concerned with the connection being set to None and more concerned with the server disconnecting after a long period of inactivity. if joseph takes my suggestion of wrapping the raw connection in something that will automatically reconnect, i don't think we need this active() method anymore or a check in send()

services/smtp.py

matt-codecov

despite how long my comments are i think you're very close to merging this. i just talk a lot, the PR is solid lol

services/smtp.py

matt-codecov · 2023-09-29T01:15:02Z

services/smtp.py

+        except smtplib.SMTPDataError:
+            err_msg = "The SMTP server did not accept the data"
+
+        return err_msg


should we just log this error message now instead of making the caller do it for us?

i think i'd prefer you create a new SMTPServiceSendFailure exception and raise it on error. it's more clear/aggressive than returning None on success and a value on failure, which is the opposite of how functions usually behave. but i think python's type system might be too weak to make me totally happy

(nothing actionable is below this point, just explaining why i don't really like any options here)

when i write error-handling code i keep two questions in mind:

should the caller care about the outcome of this function?

if so, how can i force the caller to deal with both success and failure?

on the first point, SMTPService doesn't have its own retry logic, so retrying is something the caller is responsible for. the caller should care if this function passes or fails. i think this makes sense; the caller is probably a celery task which has a ready-made retry() function but inside this function we'd need custom logic

but for the second point, python leaves me sad. compare to other languages:

java makes you declare every type of exception your function can throw. so you either must place your call to foo() in a try/catch block that handles every exception foo() can throw, or you must declare your function to throw all the exceptions that foo() can throw (and punt the problem up a stack frame). either way, the compiler forces the caller to make a decision about how to handle the error

C++ and Rust provide ways to require that you handle the return value after you call foo(). you can return Result<Val, Err> types annotated in this way and the compiler will make the caller explicitly decide how to handle them:

fn foo() -> Result<T, E>; fn bar1() { foo(); // fails to build because return value is not assigned to anything } fn bar2() { let result = foo(); // fails because the result variable is never actually used } fn bar3() { let result = foo(); if result.is_err() { // passes because we're actually using the result variable schedule_retry(); } } fn bar4() { let _ = foo(); // passes because _ is a magic "i am intentionally ignoring this" identifier }

python can make you handle neither return values nor exceptions. an uncaught exception will make the caller handle the error after they ship a bug into production, but python can't save them from the bug in the first place like other languages can

matt-codecov

see if you can add a test case for the "reconnect" logic:

mocker.patch.object(SMTPService.connection, 'noop', side_effect=SMTPServerDisconnected("foo"))
mock_fn = mocker.patch.object(smtp, 'make_connection')

smtp.send(...)
assert mock_fn.has_calls(...) # or whatever

approving with nits, great work seeing this through!

matt-codecov · 2023-09-29T17:24:18Z

services/smtp.py

+            log.warning(
+                "The SMTP server did not accept the data", extra=self.extra_dict
+            )
+            raise SMTPServiceError("The SMTP server did not accept the data") from exc


i think you could save some lines with the err_msg approach you used earlier

if not SMTPService.connection: // this one is returning right now which is inconsistent with raising the exception in the other error cases err_msg = "Connection was not initialized" try: .... except smtplib.SMTPReceipientsRefused: err_msg = "..." except ...: err_msg = "..." if err_msg: log.warning(err_msg) raise SMTPServiceError(err_msg)

services/smtp.py

matt-codecov · 2023-09-29T20:42:08Z

services/smtp.py

+            self.try_starttls()
+
+            if self.username and self.password:
+                self.try_login()


would be nice to get rid of this duplicated code. it's already in make_connection(), could we just use that here too? maybe add the if SMTPService.connection is None stuff inside make_connection() too

not a blocking comment, and sorry if it was like this before and i missed commenting on it!

it was not there, I added these methods in one of my recent commits, the reason I'm not using make_connection here is that here we are creating the SMTP object, but in make_connection we're assuming that this step has already been done and we're just running connect again on that existing object

matt-codecov · 2023-09-29T20:45:08Z

services/tests/test_smtp.py

+        service.connection.starttls.assert_called_with(context=m.return_value)
+        service.connection.login.assert_called_with("test_username", "test_password")
+
+    def test_idempotentconnectionection(self, mocker, mock_configuration):


test_idempotent_connection

* Email helper class that wraps EmailMessage * SMTPService class that creates connection to SMTP server using config options in services.smtp * send method on SMTPService that takes Email as an arg to send an email using the SMTP connection Signed-off-by: joseph-sentry <[email protected]>

Signed-off-by: joseph-sentry <[email protected]>

joseph-sentry force-pushed the joseph/smtp-service-email branch from e83af8e to 2ef32d9 Compare September 21, 2023 16:35

joseph-sentry mentioned this pull request Sep 21, 2023

Add send email task and testing for the task using mailhog #111

Merged

joseph-sentry marked this pull request as ready for review September 21, 2023 18:47

joseph-sentry requested a review from giovanni-guidini September 21, 2023 18:47

giovanni-guidini reviewed Sep 25, 2023

View reviewed changes

joseph-sentry force-pushed the joseph/smtp-service-email branch 2 times, most recently from b5cb008 to 76f2b2c Compare September 27, 2023 14:48

joseph-sentry requested a review from giovanni-guidini September 27, 2023 14:48

joseph-sentry force-pushed the joseph/smtp-service-email branch from 76f2b2c to 8c80f88 Compare September 27, 2023 15:05

joseph-sentry requested review from dana-yaish, scott-codecov and matt-codecov September 27, 2023 15:48

joseph-sentry force-pushed the joseph/smtp-service-email branch 2 times, most recently from 62d15b6 to ed6fc14 Compare September 27, 2023 19:20

giovanni-guidini reviewed Sep 28, 2023

View reviewed changes

joseph-sentry requested a review from giovanni-guidini September 28, 2023 18:04

matt-codecov reviewed Sep 29, 2023

View reviewed changes

matt-codecov approved these changes Sep 29, 2023

View reviewed changes

joseph-sentry force-pushed the joseph/smtp-service-email branch from b16d903 to b932b3f Compare September 29, 2023 20:14

matt-codecov reviewed Sep 29, 2023

View reviewed changes

joseph-sentry added 7 commits September 29, 2023 17:05

Fix typo in test_smtp

1b47309

Signed-off-by: joseph-sentry <[email protected]>

Test more in test_smtp.py

298e198

Signed-off-by: joseph-sentry <[email protected]>

test get_smtp_service with empty config

e143224

Signed-off-by: joseph-sentry <[email protected]>

address feedback

dbb41d9

Signed-off-by: joseph-sentry <[email protected]>

make lint

3876cb9

Signed-off-by: joseph-sentry <[email protected]>

remove some print statements

3ff8e99

Signed-off-by: joseph-sentry <[email protected]>

joseph-sentry added 9 commits September 29, 2023 17:05

fixing SMTP connection class scope var

7aa38be

Signed-off-by: joseph-sentry <[email protected]>

Make SMTPService a singleton and better error handling and logging

b8da3ef

Signed-off-by: joseph-sentry <[email protected]>

Fix tests and logs

a2f767a

Signed-off-by: joseph-sentry <[email protected]>

run make lint

bb87337

Signed-off-by: joseph-sentry <[email protected]>

write some more tests

23a0676

Signed-off-by: joseph-sentry <[email protected]>

run make lint

07b895c

Signed-off-by: joseph-sentry <[email protected]>

Improving tests

d14a018

Signed-off-by: joseph-sentry <[email protected]>

one more cleanup

1803145

Signed-off-by: joseph-sentry <[email protected]>

more tests

105ed31

Signed-off-by: joseph-sentry <[email protected]>

joseph-sentry force-pushed the joseph/smtp-service-email branch from b932b3f to 105ed31 Compare September 29, 2023 21:06

another lint

1e9f32a

Signed-off-by: joseph-sentry <[email protected]>

joseph-sentry merged commit d5d1155 into main Sep 29, 2023
30 of 32 checks passed

joseph-sentry deleted the joseph/smtp-service-email branch September 29, 2023 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SMTPService and Email helper #110

Add SMTPService and Email helper #110

joseph-sentry commented Sep 21, 2023

codecov-staging bot commented Sep 21, 2023 •

edited

Loading

codecov-public-qa bot commented Sep 21, 2023 •

edited

Loading

codecov bot commented Sep 21, 2023 •

edited

Loading

giovanni-guidini left a comment

giovanni-guidini Sep 25, 2023

giovanni-guidini Sep 28, 2023

matt-codecov Sep 29, 2023

matt-codecov left a comment

matt-codecov Sep 29, 2023

matt-codecov left a comment

matt-codecov Sep 29, 2023

matt-codecov Sep 29, 2023

joseph-sentry Sep 29, 2023

matt-codecov Sep 29, 2023

		from services.smtp import get_smtp_service


		class TestStorage(object):

Add SMTPService and Email helper #110

Add SMTPService and Email helper #110

Conversation

joseph-sentry commented Sep 21, 2023

codecov-staging bot commented Sep 21, 2023 • edited Loading

Codecov Report

codecov-public-qa bot commented Sep 21, 2023 • edited Loading

Codecov Report

codecov bot commented Sep 21, 2023 • edited Loading

Codecov Report

giovanni-guidini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt-codecov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt-codecov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-staging bot commented Sep 21, 2023 •

edited

Loading

codecov-public-qa bot commented Sep 21, 2023 •

edited

Loading

codecov bot commented Sep 21, 2023 •

edited

Loading