-
Notifications
You must be signed in to change notification settings - Fork 85
Eng 2185 finalization and email for consent tasks #7102
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7102 +/- ##
==========================================
- Coverage 87.30% 87.29% -0.02%
==========================================
Files 533 533
Lines 35090 35121 +31
Branches 4067 4073 +6
==========================================
+ Hits 30635 30658 +23
- Misses 3570 3576 +6
- Partials 885 887 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile OverviewGreptile SummaryThis PR adds consent request finalization and email notification capabilities, mirroring the existing erasure finalization workflow. Key Changes
Implementation NotesThe logic correctly distinguishes between consent-only requests (which now get consent completion emails) and mixed requests with access/erasure rules (which continue to get access/erasure emails). The finalization logic uses Confidence Score: 5/5
Important Files ChangedFile Analysis
|
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.
18 files reviewed, 1 comment
src/fides/api/service/privacy_request/request_runner_service.py
Outdated
Show resolved
Hide resolved
gilluminate
left a comment
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.
LGTM
|
@greptile please review |
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.
18 files reviewed, no comments
galvana
left a comment
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.
I called out one issue but we should also check with @mfbrown to make sure we're not pausing system-generated consent requests.
| and config_proxy.execution.consent_request_finalization_required | ||
| ) | ||
| ) | ||
| if requires_finalization: |
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.
There are some cases where we wouldn't want to pause the request for finalization even if the setting is enabled. I know this is at least the case for requests with a source of consent_webhook. These requests are system-generated and should just continue processing. You might want to check with @mfbrown if there are other sources that we want to exclude from this finalization (like Fides.js or Janus SDK).
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.
What would the impact of manualizing those be? I can imagine that if a notice was used on a website it might be too many DSRs to manually approve. But is there a technical reason beyond human workload?
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.
Do we want to maybe add a specific type etc to the env var that way a user would turn on the finalization for the types they wanted explicitly?
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.
If we have to add this, that would be better yeah. My concern being that source should be more expansive than it currently is and I'd like some future wiggle room to define more of them.
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.
I do think we should add a config to specify which sources require finalization. To the consent point, it would add a lot of manual work to a process that that could process many thousands of webhook responses, so I don't think we can put that work suddenly on customers for all requests.
I agree with @tvandort that we should add the ability to define more sources, more easily.
Ticket ENG-2185
Description Of Changes
🎯 Request for Consent finalization allowing for the option for manual sign off. This setting will default to not being turned on, but can be enabled in the config.
In this PR added a consent finalize option to config. This config option is not enabled by default and must be turned on. When a consent DSR is finalizing it checks for the setting and will either complete or wait for finalization. I also added a consent finalization email template which is not enabled by default and must be turned on by the user.
Code Changes
clients/admin-ui/src/features/messaging-templates/CustomizableMessagingTemplatesEnum.tsclients/admin-ui/src/features/messaging-templates/CustomizableMessagingTemplatesLabelEnum.tsclients/admin-ui/src/features/messaging-templates/CustomizableMessagingTemplatesLabelEnum.tsclients/admin-ui/src/types/api/models/MessagingActionType.tsclients/privacy-center/types/api/models/MessagingActionType.tssrc/fides/api/models/messaging_template.pysrc/fides/api/schemas/messaging/messaging.pysrc/fides/api/service/privacy_request/request_runner_service.pywith both new finalize check and new messaging callsrc/fides/config/config_proxy.pysrc/fides/config/execution_settings.pySteps to Confirm
consent_request_finalization_requiredexecution setting can be updated using config_proxy.PATCH /api/v1/configVerify that in the returned response the value is true
Note: if it shows completed you might need to restart your worker (especially if it was running before you pulled this in for testing.
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works