Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mailchimp Transactional Email Messaging Service #2742

Merged
merged 27 commits into from
Mar 7, 2023

Conversation

seanpreston
Copy link
Contributor

@seanpreston seanpreston commented Mar 3, 2023

Closes #2737

Description Of Changes

This PR adds bindings for the Mailchimp Transactional mail messaging service.

Code Changes

  • list your code changes here

Steps to Confirm

  • list any manual steps for reviewers to confirm the changes

Pre-Merge Checklist

@cypress
Copy link

cypress bot commented Mar 3, 2023

Passing run #680 ↗︎

0 3 0 0 Flakiness 0

Details:

Merge 5c3039b into ef9f450...
Project: fides Commit: bb36f96124 ℹ️
Status: Passed Duration: 00:40 💡
Started: Mar 7, 2023 4:07 PM Ended: Mar 7, 2023 4:08 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Patch coverage: 42.85% and project coverage change: -0.11 ⚠️

Comparison is base (fe04764) 86.66% compared to head (167828d) 86.55%.

❗ Current head 167828d differs from pull request most recent head 5c3039b. Consider uploading reports for the commit 5c3039b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2742      +/-   ##
==========================================
- Coverage   86.66%   86.55%   -0.11%     
==========================================
  Files         291      291              
  Lines       16458    16488      +30     
  Branches     2114     2117       +3     
==========================================
+ Hits        14264    14272       +8     
- Misses       1795     1817      +22     
  Partials      399      399              
Impacted Files Coverage Δ
src/fides/api/ops/models/messaging.py 90.78% <ø> (ø)
...ides/api/ops/service/connectors/email_connector.py 86.41% <0.00%> (-3.33%) ⬇️
.../ops/service/messaging/message_dispatch_service.py 56.84% <17.39%> (-5.55%) ⬇️
src/fides/api/ops/schemas/messaging/messaging.py 98.91% <100.00%> (+0.06%) ⬆️
...s/schemas/messaging/messaging_secrets_docs_only.py 100.00% <100.00%> (ø)
src/fides/core/config/notification_settings.py 90.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@seanpreston seanpreston marked this pull request as ready for review March 6, 2023 22:36
Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally all looks good to me, i tested locally that i was able to configure successfully, though i did not execute an end to end test sending emails, just because i don't have credentials.

a couple minor nits to address log message typos. then there's code cov, not sure if we want to try and address those - i'm OK with bypassing them since we're trying to squeeze this into the release. i do think we should update the CHANGELOG though since this is a substantial addition that should be noted.

besides that, seems about good to go IMO!

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! let's just make sure to follow up as much as we can on the codecov misses, since we do need to improve some test coverage generally here anyway it seems.

also noting that we haven't completed a full end-to-end functional test due to limitations in our test env at the moment. that's another thing that we'll be looking to address imminently.

@seanpreston seanpreston merged commit dcc7dbf into main Mar 7, 2023
@seanpreston seanpreston deleted the sp/2737/mailchimp-transactional-email branch March 7, 2023 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mailchimp Transactional MessagingConfig support
2 participants