-
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
Integrate vendor legal basis #4216
Integrate vendor legal basis #4216
Conversation
Passing run #4509 ↗︎
Details:
Review all test suite changes for PR #4216 ↗︎ |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## fidesplus_1128_legal_basis_dimension #4216 +/- ##
=====================================================================
Coverage 87.70% 87.70%
=====================================================================
Files 333 333
Lines 20814 20819 +5
Branches 2697 2698 +1
=====================================================================
+ Hits 18254 18259 +5
Misses 2095 2095
Partials 465 465 ☔ View full report in Codecov by Sentry. |
…ntegrate-vendor-legal-basis
tcf_special_features: experienceSpecialFeatures = [], | ||
} = experience; | ||
|
||
const uniquePurposeIds = useMemo(() => { |
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.
we now have two fields: tcf_consent_purposes
and tcf_legitimate_interests_purposes
which can overlap! this means, if we only want one instance of a purpose, we need to dedupe. Stacks don't care if a purpose is consent or legint, so we combine them for this file.
} = experience; | ||
|
||
const vendorsAndSystems = [...(vendors || []), ...(systems || [])]; |
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 was the bulk of the tech debt I removed 🧹
}; | ||
}) => ( | ||
<div> | ||
<PurposeBlock |
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.
Right now we are only rendering consent purposes—legint will follow in #4210
vendors, | ||
LegalBasisForProcessingEnum.LEGITIMATE_INTERESTS | ||
).length; | ||
const consent = consentSystems.length + consentVendors.length; |
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.
counting also got much easier with the backend update 👍
@@ -5,6 +5,16 @@ import type { | |||
UserConsentPreference, | |||
} from "../consent-types"; | |||
|
|||
export enum LegalBasisForProcessingEnum { |
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.
these are mostly all copied from the updated admin-ui types. I rearranged them a bit in the file to make more intuitive sense (hopefully)
return ( | ||
hasApplicablePurposes?.length || hasApplicableSpecialPurposes?.length | ||
); | ||
const transformVendorDataToVendorRecords = ({ |
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 function is one of the larger changes in this PR. basically the vendor tab wants both vendors and systems together. on top of that, it also wants it combined across legal basis. this function returns a type that makes that easy
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 walking through this one with me on screenshare @allisonking !
[vendor], | ||
LegalBasisForProcessingEnum.LEGITIMATE_INTERESTS | ||
).length === 1; | ||
experience.gvl?.dataCategories; |
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.
🎆
Updated a bunch of types after PR review changes to 8933d7d |
e03bca0
into
fidesplus_1128_legal_basis_dimension
Closes #4209
Partially addresses #4210
Description Of Changes
Whew ok this is kind of a big one! The backend API had significant changes in order to add legal basis as another dimension. Purposes, Vendors, and Systems all have this new dimension. Whereas before, our experience object looked like
it now looks like
In other words, we have a lot more fields to work with! the
*relationships
fields are to deal with fields that the vendor/system fields have in common, in order not to repeat too much in the payload. More context about that here.At the same time, the FE had gone ahead and implemented double vendor toggles, saving only to the TC string. Now that the backend can save it, we can get rid of some tech debt code 💥
Code Changes
CookieTable
(never used) andLegalBasisDropdown
(no longer needed))Steps to Confirm
/fides-js-demo.html?geolocation=fi-18
. You should see the prefetched experience in the HTML. this should have all the new fieldsPre-Merge Checklist
CHANGELOG.md