-
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
fides: add System
model support for new tcf fields
#4228
fides: add System
model support for new tcf fields
#4228
Conversation
433d4b7
to
2f2633c
Compare
Passing run #4580 ↗︎
Details:
Review all test suite changes for PR #4228 ↗︎ |
2f2633c
to
36ea973
Compare
36ea973
to
ce5c491
Compare
src/fides/api/alembic/migrations/versions/81886da90395_add_legal_basis_dimension.py
Show resolved
Hide resolved
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## integration/compass-1.0.0 #4228 +/- ##
============================================================
Coverage ? 87.74%
============================================================
Files ? 331
Lines ? 20895
Branches ? 2710
============================================================
Hits ? 18334
Misses ? 2094
Partials ? 467 ☔ View full report in Codecov by Sentry. |
i've got a changelog commit queued up locally, just didn't want to push to re-trigger CI - writing this so i don't forget to push it up! |
starting 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.
Looks great @adamsachs. I know we're not ready to bump a fideslang requirement here, but did you test locally with a fideslang alpha release here to make sure this will work as expected?
src/fides/api/alembic/migrations/versions/81886da90395_add_legal_basis_dimension.py
Show resolved
Hide resolved
EmbeddedVendor(id="sendgrid", name="TCF System Test") | ||
] | ||
|
||
assert tcf_contents.tcf_vendor_consents[0].id == "sendgrid" |
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 something that will be affected with my PR: #4256, I stop making the vendor_id: "sendgrid". Just something to sort out on merge, for whoever merges second 😄
yup! the base branch is pointed at a fideslang alpha tag, so this branch is too, it's just not showing up in the diff 👍 |
Ah! 😅 missed that thank you |
this also ensures that VendorRelationship records are created for ANY vendor record in the overlay
… declarations query instead of using the vendor_id to conditionally filter. - define the vendor_relationship_record early so we can assign its basic attributes in one place.
OK I think this is ready when CI passes - I'm waiting on whether separate #4256 should be merged to main since these sets of changes need to be reconciled. Wanted to double check since I've missed some context around the integration branch the last couple of days. |
great, i'll go ahead and merge this one first since i think it's got more dependencies on it 🤷 we need to make both work, and i don't think the conflicts will be different no matter which we merge first... |
Closes #4227
Description Of Changes
Adds support in fides for the new fields on the
System
andPrivacyDeclaration
model added to fideslang.System.cookie_max_age_seconds
System.cookie_refresh
System.uses_cookies
System.uses_non_cookie_access
System.legitimate_interest_disclosure_url
PrivacyDeclaration.flexible_legal_basis_for_processing
Code Changes
System
db modelctl_systems
table(maybe) update admin UI forms to expose these new fields?split into admin UI: support new TCF related fields on System model #4248(maybe) update TCF overlay UI to leverage these new fields?split into TCF overlay FE: support new TCF related fields on System model #4255Steps to Confirm
PUT /system
Pre-Merge Checklist
CHANGELOG.md