-
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
consent/access/erasure query param filters on connection type endpoint #2996
Conversation
Passing run #1209 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2996 +/- ##
==========================================
- Coverage 86.91% 86.90% -0.01%
==========================================
Files 303 303
Lines 17274 17298 +24
Branches 2215 2223 +8
==========================================
+ Hits 15013 15033 +20
- Misses 1847 1849 +2
- Partials 414 416 +2
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
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.
this is going in a good direction Adam! The logic on this endpoint is already a little complicated to follow given all the historical differences between db and saas connectors, and new email connectors, etc. I like your saas_request_type_filter method too, nice that we can tell programmatically what they support. I forgot about data protection requests, that was awhile ago!
c7dbd3e
to
f35abe9
Compare
Whoops wrong PR, starting with the review on this one first |
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.
Great tests here @adamsachs. The logic is a little hard to follow in places but that is the fault of the existing endpoint and historical differences between db/saas/email connectors. I think this looks good as-is, we should just pay attention as new connectors go in how we might need to adjust this.
great making the saas connection logic as programmatic as possible.
src/fides/api/ops/api/v1/endpoints/connection_type_endpoints.py
Outdated
Show resolved
Hide resolved
src/fides/api/ops/api/v1/endpoints/connection_type_endpoints.py
Outdated
Show resolved
Hide resolved
e013fdd
to
ce72b98
Compare
@pattisdr i think this is ready to go now! there's a silly codecov miss, i left a comment around it. curious to get your thoughts just as a matter of curiosity, but it's trivial, don't want to spend more than a few seconds on it :) |
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.
🏆
Partially closes #2834
Code Changes
consent
,access
,erasure
)Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md
Description Of Changes
Write some things here about the changes and any potential caveats