-
Notifications
You must be signed in to change notification settings - Fork 72
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
PROD-2049: Implement sending emails via property-specific messaging templates #4950
Conversation
…ation from data migration
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4950 +/- ##
==========================================
- Coverage 86.89% 86.81% -0.09%
==========================================
Files 346 346
Lines 21408 21481 +73
Branches 2826 2843 +17
==========================================
+ Hits 18603 18649 +46
- Misses 2308 2329 +21
- Partials 497 503 +6 ☔ View full report in Codecov by Sentry. |
Everything worked with
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good! Just a few things.
- I left a comment on the expected behavior between properties and privacy requests and I'd like to hear your thoughts.
- I got an error when running with
FIDES__NOTIFICATIONS__ENABLE_PROPERTY_SPECIFIC_MESSAGING=true
, I left a root-level comment with the stacktrace - I had to enable the default templates in order to get messaging to work. Is it expected that they're set to disabled by default? I imagine we would want them enabled for OSS users?
...des/api/alembic/migrations/versions/a3c173391603_add_property_id_to_privacy_request_model.py
Outdated
Show resolved
Hide resolved
Thanks so much for the review @galvana !
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes @eastandwestwind!
Fixed the error- turns out I can't pass in DB models to celery workers.
Thanks for the fix here! I re-ran this scenario and it worked as expected
Yes, you'll need to enable the default templates first, I've updated the testing instructions to reflect this. We don't support property-specific messaging in OSS at all. This is only a Plus feature.
I was thinking of it wrong, this makes sense!
Passing run #8293 ↗︎
Details:
Review all test suite changes for PR #4950 ↗︎ |
Closes PROD-2049
Description Of Changes
This change adds the core logic to determine both:
We support property-specific messaging for the following message types
Code Changes
property_id
field toPrivacyRequest
modelSteps to Confirm
FIDES__NOTIFICATIONS__ENABLE_PROPERTY_SPECIFIC_MESSAGING
disabled, confirm that no regressions exist with configuring email templates and sending out emails via Fides (consult the above list for email types to test)FIDES__NOTIFICATIONS__ENABLE_PROPERTY_SPECIFIC_MESSAGING
enabled, confirm that we can send property-specific emails. To do this, to the following:update messaging_template set is_enabled=true;
Pre-Merge Checklist
CHANGELOG.md
main
downgrade()
migration is correct and works