ENG-3301: Refactor messaging dispatchers into provider classes

[JadeCara]

Ticket ENG-3301

PR Series

This is PR 1 of 3 in the messaging provider refactor:

PR	Branch	Description	Status
This PR (PR1)	messaging-provider-refactor-pr1	Extract 4 dispatchers into provider classes	✅ Review-ready
#8120 (PR2)	messaging-provider-refactor-pr2-ses	Migrate AWS SES to provider class	✅ Review-ready
#8138 (PR2.5)	messaging-service-alignment	Wire route endpoints through MessagingService	✅ In review

Base branch: main

Description Of Changes

Refactors 4 of 5 messaging dispatchers into provider classes behind a two-level ABC hierarchy. This is a pure structural refactor — no behavioral changes. AWS SES remains on the legacy dispatcher and will be migrated in a follow-up PR (#8120).

Key changes:

• Introduces BaseMessageProviderService / BaseEmailProviderService / BaseSMSProviderService ABCs
• Creates MailgunService, TwilioEmailService, MailchimpTransactionalService, TwilioSmsService provider classes
• Refactors dispatch_message() to use a provider map instead of _get_dispatcher_from_config_type()
• AWS SES falls back to the legacy _aws_ses_dispatcher (temporary, removed in ENG-3301: Migrate AWS SES dispatcher to provider class #8120)

Code Changes

• New src/fides/api/service/messaging/messaging_providers/ package with ABCs and 4 provider classes
• Rewired dispatch_message() to use _PROVIDER_MAP constant for provider lookup
• Added legacy _aws_ses_dispatcher fallback for SES (removed in follow-up PR)
• Deleted old _mailgun_dispatcher, _twilio_email_dispatcher, _twilio_sms_dispatcher, _mailchimp_transactional_dispatcher, _get_dispatcher_from_config_type, validate_config, _compose_twilio_mail, _get_template_id_if_exists functions
• Updated mock targets in 8 test files

Steps to Confirm

1. Server starts cleanly

docker logs fides 2>&1 | grep -i traceback

Confirm no import errors from the new messaging provider modules.

2. Test Mailgun dispatch

Create config and send a test message:

PUT /api/v1/messaging/default/mailgun
{"domain": "<your-domain>", "api_version": "v3"}

PUT /api/v1/messaging/default/mailgun/secret
{"mailgun_api_key": "<your-key>"}

POST /api/v1/messaging/test/mailgun
{"email": "<your-email>"}

Verify 200 response and email received.

3. Test Twilio Email dispatch

PUT /api/v1/messaging/default/twilio_email
{"twilio_email_from": "<from-address>"}

PUT /api/v1/messaging/default/twilio_email/secret
{"twilio_api_key": "<your-key>"}

POST /api/v1/messaging/test/twilio_email
{"email": "<your-email>"}

Verify 200 response and email received.

4. Test Twilio SMS dispatch

PUT /api/v1/messaging/default/twilio_text
{}

PUT /api/v1/messaging/default/twilio_text/secret
{"twilio_account_sid": "<sid>", "twilio_auth_token": "<token>", "twilio_messaging_service_sid": "<msid>"}

POST /api/v1/messaging/test/twilio_text
{"phone_number": "<your-phone>"}

Verify 200 response and SMS received.

5. Error path — missing config

Delete any existing messaging config, then:

POST /api/v1/messaging/test/mailgun
{"email": "test@example.com"}

Verify a clear 400 error (not a 500/traceback).

Pre-Merge Checklist

• Issue requirements met
• All CI pipelines succeeded
• CHANGELOG.md updated
  • Updates unreleased work already in Changelog, no new entry necessary
• UX feedback:
  • No UX review needed
• Followup issues:
  • Followup issues created
• Database migrations:
  • No migrations
• Documentation:
  • No documentation updates required

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
fides-plus-nightly	Ignored	Preview	May 8, 2026 8:10pm
fides-privacy-center	Ignored		May 8, 2026 8:10pm

[codecov]

Codecov Report

❌ Patch coverage is 93.54839% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.34%. Comparing base (6d220c9) to head (0cea832).

Files with missing lines	Patch %	Lines
.../api/service/messaging/message_dispatch_service.py	84.21%	4 Missing and 2 partials ⚠️
...e/messaging/messaging_providers/mailgun_service.py	88.23%	3 Missing and 1 partial ⚠️
.../api/service/messaging/messaging_providers/base.py	94.28%	1 Missing and 1 partial ⚠️
...essaging/messaging_providers/twilio_sms_service.py	93.75%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8118      +/-   ##
==========================================
+ Coverage   85.23%   85.34%   +0.11%     
==========================================
  Files         638      643       +5     
  Lines       42011    42110      +99     
  Branches     4937     4947      +10     
==========================================
+ Hits        35808    35940     +132     
+ Misses       5096     5063      -33     
  Partials     1107     1107              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JadeCara]

/code-review

[claude]

Code Review: Messaging Provider Refactor (PR #8118)

This is a clean, well-scoped refactor. The two-level ABC hierarchy (BaseMessageProviderService → BaseEmailProviderService / BaseSMSProviderService) is a logical structure, the test updates are thorough, and the decision to leave AWS SES as a legacy dispatcher with a clear follow-up note is reasonable.

Issues found

Bug (pre-existing, preserved): Two test functions in test_integration_dynamic_erasure_email.py (test_erasure_email_multiple_requests and test_erasure_email_multiple_requests_same_email_different_vendor) contain bare equality expressions instead of assert statements — they are silent no-ops and never actually validate mock call args. See inline comment.

Design concern: _resolve_provider_map() is rebuilt on every dispatch call specifically to accommodate test mocking. This is production code structured around test infrastructure. A module-level constant + standard mock.patch at the import path achieves the same test isolation without this coupling. See inline comment.

Type safety / ClassVar: provider_name: str on the base class is a bare annotation that type checkers and the runtime won't enforce. Should be ClassVar[str], and an __init__-time assertion or default would make missing definitions fail loudly at construction rather than silently in the error message. See inline comment.

KeyError vs MessageDispatchException: Across all four new provider __init__ methods, validation confirms details/secrets is truthy, then immediately subscripts the dict by key. A truthy-but-key-missing config escapes validation and raises a bare KeyError. This is pre-existing behavior from the old dispatchers but is worth fixing now that the structure makes it easy. See inline comment on MailchimpTransactionalService.

Minor test assertion inconsistency: "No identity" test cases assert mock_cls.return_value.send_email.assert_not_called() — this checks the instance method but not whether the constructor itself was called. Since these failures occur before instantiation, mock_cls.assert_not_called() is the more accurate assertion.

What's working well

• The ABC hierarchy cleanly separates email and SMS concerns.
• autospec=True on all the mocks is a nice improvement over the raw mock.patch in the original tests.
• Renaming private helpers to static methods (_get_template_id_if_exists, _compose_mail) on TwilioEmailService is a better fit for their actual usage.
• MailgunService.send_email correctly re-raises MessageDispatchException before the outer except Exception swallows it — a subtle correctness fix over the original.
• The _aws_ses_dispatcher is kept as a named legacy function with a clear comment, making the follow-up PR's task obvious.

---

🔬 Codegraph: connected (49390 nodes)

---

💡 Write /code-review in a comment to re-run this review.

[JadeCara]

/code-review

[claude]

Code Review: Messaging Provider Refactor (#8118)

This is a solid structural improvement. Moving the dispatcher functions into a two-level ABC hierarchy (BaseMessageProviderService → BaseEmailProviderService / BaseSMSProviderService) makes the extension point much clearer than the previous function-map pattern, and the _PROVIDER_MAP dict is a clean replacement for _get_dispatcher_from_config_type. The test migration from @mock.patch on the internal dispatcher to requests_mock is a genuine improvement — tests now exercise the real HTTP layer rather than stopping at the boundary.

Issues Found

Test coverage regressions (most important)

Two unit tests in message_dispatch_service_test.py — test_email_dispatch_property_specific_templates_enabled_no_template and test_email_dispatch_property_specific_templates_enabled_with_template_no_property_default_request — now have zero assertions. They previously verified mock_mailgun_dispatcher.assert_not_called() to confirm that no email is dispatched when property-specific messaging is enabled but no matching template is found. After the refactor these tests pass trivially and no longer verify the intended behaviour. They need mock_mailgun_http added and assert not mock_mailgun_http.called.

Two integration tests in test_integration_dynamic_erasure_email.py (test_erasure_email_multiple_requests, test_erasure_email_multiple_requests_same_email_different_vendor) replace detailed call-argument assertions with only a POST-count check, leaving recipient and content correctness unverified. (The original assertions were also broken — missing assert — so the original tests were already not catching regressions, but the replacement is an opportunity to fix that properly.)

type: ignore on the AWS SES legacy path

The call to _aws_ses_dispatcher carries a # type: ignore[arg-type] because message is Union[EmailForActionType, str] at that point. A simple isinstance guard before the call would remove the suppression and make the code safer (see inline comment).

_get_detail / _get_secret return type annotation

Both helpers are annotated -> str but actually return whatever type is stored in the config dict. IS_EU_DOMAIN is stored as a boolean, which works at runtime with if is_eu, but the annotation is misleading and will confuse the next person adding a provider who expects a string. Consider -> Any or documenting the convention.

mailchimp_transactional_service.py unguarded response.json()[0]

Pre-existing, but now that the logic lives in an isolated class it's a good time to add a bounds check before indexing into the response list. An empty or malformed response currently raises an unhandled IndexError.

Nits

• provider_name = "" would bypass the __init_subclass__ enforcement (falsy empty string); a minor hardening concern.
• The TwilioSmsService.send_sms else branch raises MessageDispatchException inside a try/except TwilioRestException block — it propagates correctly, but a brief comment would help future maintainers avoid accidentally wrapping it.

Summary

The architecture is right and the approach is sound. The main ask before merging is to restore the two unit tests that lost their "not called" assertions, so the property-specific messaging guard is actually tested. The other items are lower priority.

🔬 Codegraph: connected (49390 nodes)

---

💡 Write /code-review in a comment to re-run this review.

[erosselli]

instead of overriding __init_subclass__ and having to check that the class isn't abstract etc, can't we just add an assertion in __init__ ? Something like:

Suggested change

	def __init_subclass__(cls, **kwargs: object) -> None:
	super().__init_subclass__(**kwargs)
	# Skip intermediate ABCs that define their own abstract methods
	has_abstract = any(
	getattr(v, "__isabstractmethod__", False) for v in cls.__dict__.values()
	)
	if not has_abstract and not getattr(cls, "provider_name", None):
	raise TypeError(f"{cls.__name__} must define 'provider_name'")
	def __init__(self, messaging_config: MessagingConfig):
	self.messaging_config = messaging_config
	self.validate_config()
	def __init__(self, messaging_config: MessagingConfig):
	assert self.provider_name, "BaseMessageProviderService must define provider_name"
	self.messaging_config = messaging_config
	self.validate_config()

[erosselli]

there's also a problem with the existing code that because it only looks at the class' __dict__, an intermediate ABC that inherits abstract methods without declaring new ones would incorrectly trigger the provider_name guard.

[JadeCara]

Done — removed __init_subclass__ and moved the guard to __init__. Used if not ... raise TypeError instead of assert since asserts are stripped by python -O. Good catch on the intermediate ABC issue with __dict__.

[erosselli]

can we not use your mock_mailgun_http fixture for the tests on this file?

[JadeCara]

Done — switched to mock_mailgun_http for all standard Mailgun tests in this file. The one exception is test_email_dispatch_mailgun_failed_email which needs a custom 403 response, so it keeps inline mocking. Also updated TestInitSubclassGuard → TestInitGuard to reflect the guard moving to __init__.

[erosselli]

both mailgun and twilio email define this, maybe we should have this be a constant in the base file and import it from both ?

[JadeCara]

Done — moved EMAIL_TEMPLATE_NAME to base.py and imported from there in both providers.

[erosselli]

this may be overkill, but str | bool is a bit of a weird type, given that there's only a single detail key that returns bool.... I wonder if we could use function signature overloads like

 @overload       
  def _get_detail(self, key: Literal[MessagingServiceDetails.IS_EU_DOMAIN]) -> bool: ...
                                                                                                                                                                                                                                                    
  @overload
  def _get_detail(self, key: MessagingServiceDetails) -> str: ...         

or if this is too complicated, then maybe we could inline the bool look up in the mailgun class and then keep _get_detail as always returning str ?

[JadeCara]

Done — inlined the IS_EU_DOMAIN lookup in MailgunService and kept _get_detail as -> str. Simpler than overloads for a single bool key. mypy passes clean on all changed files.

[erosselli]

thanks for making the changes! I tested with mailgun and got emails as normal