Add send email task and testing for the task using mailhog#111
Add send email task and testing for the task using mailhog#111joseph-sentry merged 7 commits intomainfrom
Conversation
joseph-sentry
force-pushed
the
joseph/send-email
branch
from
5b14f26 to
8f4ef07
Compare
Codecov Report
@@ Coverage Diff @@
## main #111 +/- ##
=======================================
Coverage ? 93.41%
=======================================
Files ? 346
Lines ? 26959
Branches ? 0
=======================================
Hits ? 25183
Misses ? 1776
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov Report
@@ Coverage Diff @@
## main #111 +/- ##
========================================
Coverage 98.39% 98.40%
========================================
Files 371 373 +2
Lines 27522 27692 +170
========================================
+ Hits 27081 27251 +170
Misses 441 441
Flags with carried forward coverage won't be shown. Click here to find out more.
This change has been scanned for critical changes. Learn more |
joseph-sentry
requested review from
dana-yaish,
giovanni-guidini,
matt-codecov and
scott-codecov
joseph-sentry
force-pushed
the
joseph/send-email
branch
3 times, most recently
from
a6c9200 to
42874da
Compare
Codecov Report
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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
giovanni-guidini
left a comment
giovanni-guidini
left a comment
There was a problem hiding this comment.
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.
| extra=log_extra_dict, | ||
| ) | ||
| return None | ||
| to_addr = owner.email |
There was a problem hiding this comment.
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
| email_helper = Sendgrid(actual_type) | ||
| return email_helper.send_email(owner) | ||
|
|
||
| owners = db_session.query(Owner).filter_by(ownerid=ownerid) |
There was a problem hiding this comment.
[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
There was a problem hiding this comment.
Ah the original code did this as well, I see.... Still don't see much advantage to that over the one-liner
| return None | ||
| template_service = TemplateService() | ||
|
|
||
| with metrics.timer("worker.tasks.send_email.render_templates"): |
There was a problem hiding this comment.
Nice idea having this metric here, I like that
| 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| username="test_username", | ||
| ) | ||
|
|
||
| assert metrics.data["worker.tasks.send_email.attempt"] == 1 |
There was a problem hiding this comment.
This is nice. But you could have an assert for the result as well
joseph-sentry
force-pushed
the
joseph/send-email
branch
from
bb70d24 to
a44ed80
Compare
* 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]>
Signed-off-by: joseph-sentry <[email protected]>
Signed-off-by: joseph-sentry <[email protected]>
Signed-off-by: joseph-sentry <[email protected]>
joseph-sentry
force-pushed
the
joseph/send-email
branch
from
a44ed80 to
949f06a
Compare
Signed-off-by: joseph-sentry <[email protected]>
Signed-off-by: joseph-sentry <[email protected]>
2 participants
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