-
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
Adding request overrides for consent requests #4920
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Passing run #7961 ↗︎
Details:
Review all test suite changes for PR #4920 ↗︎ |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4920 +/- ##
==========================================
+ Coverage 86.79% 86.91% +0.12%
==========================================
Files 347 346 -1
Lines 21213 21202 -11
Branches 2784 2785 +1
==========================================
+ Hits 18412 18428 +16
+ Misses 2312 2285 -27
Partials 489 489 ☔ View full report in Codecov by Sentry. |
src/fides/api/service/saas_request/override_implementations/mailchimp_request_overrides.py
Outdated
Show resolved
Hide resolved
tests/ops/integration_tests/saas/request_override/test_consent_request_override_task.py
Show resolved
Hide resolved
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.
@galvana this looks solid to me, nice work! i like that this override framework is continuing to expand in what feels like a pretty natural way :)
i left a few nits that you can feel free to take or leave. i assume there's going to be a real use case coming up to leverage these overrides, in which case we'll put it to the test! that's usually the best confirmation that the override has what it needs and there aren't any unexpected issues - but i think it makes sense to put the (proposed) framework updates you have here as their own increment, and then follow up with the first implementation after 👍
src/fides/api/service/saas_request/override_implementations/mailchimp_request_overrides.py
Outdated
Show resolved
Hide resolved
""" | ||
|
||
|
||
@register("mailchimp_messages_access", [SaaSRequestType.READ]) | ||
def mailchimp_messages_access( |
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.
nit: is there any way to split this out into a fixture? or its own file/module? it totally makes sense that you moved this into the tests
package generally (thank you for that!), but it may be nice to keep this separated from the test code itself just so that it can still serve as a standalone reference? not a big deal if it it's a pain!
""" | ||
Invokes the appropriate user-defined SaaS request override for consent requests. | ||
|
||
Includes the necessary data preparations for override input |
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.
nit: this looks like a c+p error, i don't think there's any data prep done here!
Closes #PROD-1526
Description Of Changes
Adding the ability to specify SaaS request overrides of type
OPT_IN
andOPT_OUT
Code Changes
OPT_IN
andOPT_OUT
request override types and related testsSteps to Confirm
Pre-Merge Checklist
CHANGELOG.md