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

Adds config for notification service type, genericize concept of email to messaging #1519

Merged
merged 28 commits into from
Nov 7, 2022

Conversation

eastandwestwind
Copy link
Contributor

@eastandwestwind eastandwestwind commented Oct 21, 2022

Closes 1466

Code Changes

  • Adds new config option notification_service_type for customer to specify notification service type (now includes Twilio SMS and email types)
  • Adds helper method to help determine SMS vs Email based on service type- https://github.com/ethyca/fides/pull/1519/files#diff-c241db7ea3fb18f8bd7a0a70ff921c6c2802f515357a4c0c0f4d6b99554f4dfeR37
  • Refactor to genericize email dispatcher to handle all messaging types, including SMS
  • Renaming email to messaging for endpoint paths, docs, API scopes, and celery queue
  • Updates postman collection such that we have separate requests to add Twilio service type
  • Adds twilio dependency to requirements.txt
  • Removes API restriction on messaging config that only one config is allowed. We now support configuring 3 different messaging configs: twilio_text, twilio_email, and mailgun.
  • Updates tests

Follow-ups needed

  • Docs to add a Twilio service

Steps to Confirm

  • list any manual steps taken to confirm the changes

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated:
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

Write some things here about the changes and any potential caveats

@eastandwestwind eastandwestwind marked this pull request as draft October 21, 2022 19:55
@eastandwestwind eastandwestwind force-pushed the 1466-sms-config branch 2 times, most recently from 8b45aec to e194266 Compare October 31, 2022 13:44
@eastandwestwind eastandwestwind changed the title Adds config for notification service type, starts work to genericize email dispatcher Adds config for notification service type, genericize email dispatcher Oct 31, 2022
@eastandwestwind eastandwestwind changed the title Adds config for notification service type, genericize email dispatcher Adds config for notification service type, genericize concept of email to messaging Oct 31, 2022
docs/fides/docs/guides/messaging.md Show resolved Hide resolved
src/fides/api/ops/api/v1/endpoints/messaging_endpoints.py Outdated Show resolved Hide resolved
src/fides/api/ops/schemas/messaging/messaging.py Outdated Show resolved Hide resolved
src/fides/api/ops/service/connectors/email_connector.py Outdated Show resolved Hide resolved
src/fides/ctl/core/config/notification_settings.py Outdated Show resolved Hide resolved
@eastandwestwind eastandwestwind marked this pull request as ready for review November 3, 2022 19:30
@eastandwestwind eastandwestwind requested a review from a team November 3, 2022 19:30
scripts/setup/email.py Show resolved Hide resolved
src/fides/api/ops/api/v1/endpoints/messaging_endpoints.py Outdated Show resolved Hide resolved
src/fides/api/ops/models/messaging.py Show resolved Hide resolved
src/fides/api/ops/schemas/messaging/messaging.py Outdated Show resolved Hide resolved
src/fides/api/ops/schemas/messaging/messaging.py Outdated Show resolved Hide resolved
tests/ops/api/v1/endpoints/test_messaging_endpoints.py Outdated Show resolved Hide resolved
tests/ops/api/v1/endpoints/test_messaging_endpoints.py Outdated Show resolved Hide resolved
Copy link
Contributor

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

lgtm

@sanders41 sanders41 merged commit 9a3a6d3 into main Nov 7, 2022
@sanders41 sanders41 deleted the 1466-sms-config branch November 7, 2022 20:48
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.

Add Twilio Messaging provider, add config for messaging service
2 participants