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

joseph-sentry · 2023-09-21T17:37:09Z

Changes send_email task to send a single email to the email of a specific owner using the SMTP service. The contents of the email are generated using the template service. The send_email task takes in an ownerid, from addr, subject, email and kwargs to be substituted in the template as its args.

Fixes: codecov/engineering-team#157
Depends on: #110 #108 #107

codecov-staging · 2023-09-21T17:43:40Z

Codecov Report

❗ No coverage uploaded for pull request base (main@aba7972). Click here to learn what that means.
The diff coverage is 87.79%.

❗ Current head bb70d24 differs from pull request most recent head 1088ac0. Consider uploading reports for the commit 1088ac0 to get more accurate results

@@           Coverage Diff           @@
##             main     #111   +/-   ##
=======================================
  Coverage        ?   93.41%           
=======================================
  Files           ?      346           
  Lines           ?    26959           
  Branches        ?        0           
=======================================
  Hits            ?    25183           
  Misses          ?     1776           
  Partials        ?        0

Flag	Coverage Δ
integration	`93.41% <87.79%> (?)`
latest-uploader-overall	`93.41% <87.79%> (?)`
unit	`93.41% <87.79%> (?)`

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

Components	Coverage Δ
NonTestCode	`94.23% <100.00%> (?)`
OutsideTasks	`96.81% <100.00%> (?)`

Files	Coverage Δ
conftest.py	`88.63% <100.00%> (ø)`
services/tests/test_smtp.py	`100.00% <100.00%> (ø)`
tasks/send_email.py	`100.00% <100.00%> (ø)`
tasks/tests/unit/test_send_email_task.py	`100.00% <100.00%> (ø)`
tasks/tests/integration/test_send_email_task.py	`36.36% <36.36%> (ø)`

codecov-public-qa · 2023-09-21T17:44:14Z

Codecov Report

Merging #111 (1088ac0) into main (aba7972) will increase coverage by 5.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #111      +/-   ##
==========================================
+ Coverage   93.30%   98.44%   +5.14%     
==========================================
  Files         347      347              
  Lines       27113    27196      +83     
==========================================
+ Hits        25298    26774    +1476     
+ Misses       1815      422    -1393

Flag	Coverage Δ
integration	`98.44% <100.00%> (+5.14%)`	⬆️
latest-uploader-overall	`98.44% <100.00%> (+5.14%)`	⬆️
unit	`98.44% <100.00%> (+5.14%)`	⬆️

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

Components	Coverage Δ
NonTestCode	`97.02% <100.00%> (+3.00%)`	⬆️
OutsideTasks	`98.23% <100.00%> (+1.49%)`	⬆️

Files	Coverage Δ
conftest.py	`94.69% <100.00%> (+6.60%)`	⬆️
services/tests/test_smtp.py	`100.00% <100.00%> (ø)`
tasks/send_email.py	`100.00% <100.00%> (+52.17%)`	⬆️
tasks/tests/integration/test_send_email_task.py	`100.00% <100.00%> (ø)`
tasks/tests/unit/test_send_email_task.py	`100.00% <100.00%> (ø)`

... and 43 files with indirect coverage changes

codecov · 2023-09-21T17:44:31Z

Codecov Report

Merging #111 (1088ac0) into main (aba7972) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##             main     #111    +/-   ##
========================================
  Coverage   98.39%   98.40%            
========================================
  Files         371      373     +2     
  Lines       27522    27692   +170     
========================================
+ Hits        27081    27251   +170     
  Misses        441      441

Flag	Coverage Δ
integration	`98.44% <100.00%> (+5.14%)`	⬆️
latest-uploader-overall	`98.44% <100.00%> (+5.14%)`	⬆️
unit	`98.44% <100.00%> (+5.14%)`	⬆️

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

Components	Coverage Δ
NonTestCode	`96.91% <100.00%> (+<0.01%)`	⬆️
OutsideTasks	`98.23% <100.00%> (+1.49%)`	⬆️

Files	Coverage Δ
conftest.py	`94.69% <100.00%> (+0.25%)`	⬆️
services/tests/test_smtp.py	`100.00% <100.00%> (ø)`
tasks/send_email.py	`100.00% <100.00%> (ø)`
tasks/tests/integration/test_send_email_task.py	`100.00% <100.00%> (ø)`
tasks/tests/unit/test_send_email_task.py	`100.00% <100.00%> (ø)`

This change has been scanned for critical changes. Learn more

codecov-qa · 2023-10-04T16:35:15Z

Codecov Report

Merging #111 (1088ac0) into main (aba7972) will increase coverage by 5.14%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #111      +/-   ##
==========================================
+ Coverage   93.30%   98.44%   +5.14%     
==========================================
  Files         347      347              
  Lines       27113    27196      +83     
==========================================
+ Hits        25298    26774    +1476     
+ Misses       1815      422    -1393

Flag	Coverage Δ
integration	`98.44% <100.00%> (+5.14%)`	⬆️
latest-uploader-overall	`98.44% <100.00%> (+5.14%)`	⬆️
unit	`98.44% <100.00%> (+5.14%)`	⬆️

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

Components	Coverage Δ
NonTestCode	`97.02% <100.00%> (+3.00%)`	⬆️
OutsideTasks	`98.23% <100.00%> (+1.49%)`	⬆️

Files	Coverage Δ
conftest.py	`94.69% <100.00%> (+6.60%)`	⬆️
services/tests/test_smtp.py	`100.00% <100.00%> (ø)`
tasks/send_email.py	`100.00% <100.00%> (+52.17%)`	⬆️
tasks/tests/integration/test_send_email_task.py	`100.00% <100.00%> (ø)`
tasks/tests/unit/test_send_email_task.py	`100.00% <100.00%> (ø)`

... and 43 files with indirect coverage changes

giovanni-guidini

Looks good. Good job testing the many different error cases in the unit tests.

There seems to be something wrong with the integration tests tho. The annotations would indicate that it didn't run. Maybe it's a CI issue (that we are not running the integration tests in CI), but I think it is worth investigating.

giovanni-guidini · 2023-10-06T13:42:19Z

tasks/send_email.py

+                    extra=log_extra_dict,
+                )
+                return None
+            to_addr = owner.email


I think some Owners might not have an email address... Maybe I'm confusing with orgs (which are also owners)

But I think it'd be inexpensive to check for that here, before trying to send the email

giovanni-guidini · 2023-10-06T13:43:29Z

tasks/send_email.py

-        email_helper = Sendgrid(actual_type)
-        return email_helper.send_email(owner)
+
+            owners = db_session.query(Owner).filter_by(ownerid=ownerid)


[nit] I think it's fine to use the one-liner owners = db_session.query(Owner).filter_by(ownerid=ownerid).first() . It's all through the code.

There should ever only be 1 owner with any given ownerid after all

Ah the original code did this as well, I see.... Still don't see much advantage to that over the one-liner

giovanni-guidini · 2023-10-06T13:45:07Z

tasks/send_email.py

+                return None
+            template_service = TemplateService()
+
+            with metrics.timer("worker.tasks.send_email.render_templates"):


Nice idea having this metric here, I like that

giovanni-guidini · 2023-10-06T13:46:05Z

tasks/send_email.py

+                html_template = template_service.get_template(f"{template_name}.html")
+                html = html_template.render(**kwargs)
+
+            email_wrapper = Email(to_addr, from_addr, subject, text, html)


Just so I understand this better, the txt template is a fallback to the html template in case the owner's email service can't handle html emails?

sort of, I guess if the email client of the person viewing the email does not support displaying HTML, then it will show the text

giovanni-guidini · 2023-10-06T13:55:07Z

tasks/tests/unit/test_send_email_task.py

+            username="test_username",
+        )
+
+        assert metrics.data["worker.tasks.send_email.attempt"] == 1


This is nice. But you could have an assert for the result as well

* Changes send_email task to send a single email to the email of a specific owner using the SMTP service. The contents of the email are generated using the template service. The send_email task takes in an ownerid, from addr, subject, email and kwargs to be substituted in the template as its args. * Adds testing for the send_email task using mailhog. Signed-off-by: joseph-sentry <[email protected]>

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

joseph-sentry force-pushed the joseph/send-email branch from 5b14f26 to 8f4ef07 Compare September 21, 2023 17:38

joseph-sentry marked this pull request as ready for review September 27, 2023 14:22

joseph-sentry requested review from giovanni-guidini, dana-yaish, scott-codecov and matt-codecov September 27, 2023 14:22

joseph-sentry force-pushed the joseph/send-email branch 3 times, most recently from a6c9200 to 42874da Compare October 4, 2023 16:27

giovanni-guidini reviewed Oct 6, 2023

View reviewed changes

joseph-sentry force-pushed the joseph/send-email branch from bb70d24 to a44ed80 Compare October 6, 2023 17:09

joseph-sentry added 5 commits October 6, 2023 13:40

adapt send email task to changes in smtp and template services

56c70f5

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

quick fix to tests

5fbd00c

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

update gha workflow version

f47fa50

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

Address feedback

949f06a

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

joseph-sentry force-pushed the joseph/send-email branch from a44ed80 to 949f06a Compare October 6, 2023 17:41

joseph-sentry requested a review from giovanni-guidini October 6, 2023 18:00

Improve tests

44a0efc

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

giovanni-guidini approved these changes Oct 9, 2023

View reviewed changes

one more fix to the test

1088ac0

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

joseph-sentry merged commit 41cf406 into main Oct 9, 2023
25 of 26 checks passed

joseph-sentry deleted the joseph/send-email branch October 9, 2023 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

joseph-sentry commented Sep 21, 2023 •

edited

Loading

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

codecov-qa bot commented Oct 4, 2023 •

edited

Loading

giovanni-guidini left a comment

giovanni-guidini Oct 6, 2023

giovanni-guidini Oct 6, 2023

giovanni-guidini Oct 6, 2023

giovanni-guidini Oct 6, 2023

giovanni-guidini Oct 6, 2023

joseph-sentry Oct 6, 2023

giovanni-guidini Oct 6, 2023

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

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

Conversation

joseph-sentry commented Sep 21, 2023 • edited Loading

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

codecov-qa bot commented Oct 4, 2023 • edited Loading

Codecov Report

giovanni-guidini left a comment

Choose a reason for hiding this comment

giovanni-guidini Oct 6, 2023

Choose a reason for hiding this comment

giovanni-guidini Oct 6, 2023

Choose a reason for hiding this comment

giovanni-guidini Oct 6, 2023

Choose a reason for hiding this comment

giovanni-guidini Oct 6, 2023

Choose a reason for hiding this comment

giovanni-guidini Oct 6, 2023

Choose a reason for hiding this comment

joseph-sentry Oct 6, 2023

Choose a reason for hiding this comment

giovanni-guidini Oct 6, 2023

Choose a reason for hiding this comment

joseph-sentry commented Sep 21, 2023 •

edited

Loading

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

codecov-qa bot commented Oct 4, 2023 •

edited

Loading