-
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
initial implementation of privacy notice CRUD endpoints #2928
Conversation
8f3c33a
to
c616a6a
Compare
Passing run #1080 ↗︎
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 #2928 +/- ##
===========================================
+ Coverage 67.24% 86.86% +19.61%
===========================================
Files 300 303 +3
Lines 16922 17177 +255
Branches 2162 2195 +33
===========================================
+ Hits 11380 14921 +3541
+ Misses 5138 1844 -3294
- Partials 404 412 +8
... and 130 files 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. |
c616a6a
to
2cffb7f
Compare
Starting review now - |
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 think this is great work @adamsachs: your code was clear and easy to follow, and you'd taken time to add good code comments where why certain logic was added was necessary. The check_conflicting_data_uses
was the main place that felt a little tricky to trace through but you documented it well. I'd suggest adding some tests on that method specifically and make sure all edge cases are covered.
My comments are minor.
81bc07e
to
139ea34
Compare
@pattisdr i think this is good for another look, hopefully we can get this merged today before the release train leaves the station! let me know if there's anything else you think i need to address. since your last review, i mainly added test coverage and i also adjusted to create a history entry during privacy notice creation/with each update, not "lagging behind" as i'd had it initially. we can easily revert that if you see problems with the rather hasty adjustment there. |
looking now @adamsachs thanks for all these changes |
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 this huge amount of work - this is a great foundation that really sets us up for the rest of the backend consent changes. Really appreciate the extra care that went into the testing on this one. Just a couple of things and we're good to go.
src/fides/api/ctl/migrations/versions/6d6b0b7cbb36_add_privacynotice_tables.py
Show resolved
Hide resolved
c7b01df
to
fb1c419
Compare
006954f
to
a8828f2
Compare
❗ Contains migration; check if downrev needs to be bumped before merging.
Closes #2832
Code Changes
privacynotice
andprivacynoticehistory
DB tables (and corresponding SQL alchemy classes) to store details about privacy notices, and an "audit trail" for each privacy notice record, respectivelyprivacy_notice:[create|update|read\
scopes added to our scope registryowner
andcontributor
roles have all these scopesviewer
(andviewer+approver
) roles haveprivacy_notice:read
scopeGET /api/v1/privacy-notice
endpoint to retrieve a list of privacy notices on the systemshow_disabled
param (optional, default =True
) determines whether disabled privacy notice records should be returnedregion
param (optional, single-value) allows filtering of privacy notices to show only those associated with the given regionsystems_applicable
param (optional, default=True
) determines whether to return only privacy notices that have a data use that is associated with a systemGET /api/v1/privacy-notice/{privacy_notice_id}
endpoint to retrieve details about a specific privacy noticePOST /api/v1/privacy-notice
to create one or more new privacy noticeadvertisting
andadvertising.first_party
are considered overlapping)PATCH /api/v1/privacy-notice
to update details about one or more new privacy noticesid
must be provided on each object in the request body, to indicate which privacy notice the update applies to. theid
must point to a valid, existing privacy noticeid
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md
Description Of Changes
Foundational APIs for creating, reading, updating privacy notices (and their history/audit records) on Fides. I tried to stick as close to the (very thorough and clear!) spec in confluence.