-
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
Double vendor toggles #4189
Double vendor toggles #4189
Conversation
@@ -17,6 +17,7 @@ const DataUseToggle = ({ | |||
disabled, | |||
isHeader, | |||
includeToggle = true, | |||
secondToggle, |
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 not a good pattern! it's weird that there'd be a toggle defined in the component, and a separate one passed in. but it works quite well, and time is short 🥲
eventually, I think we should totally rework TcfVendors
to be a table, and maybe not even use DataUseToggle
which was never really meant to be a table.
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.
follow up ticket: #4190
const vendors = [...(allVendors || []), ...(allSystems || [])]; | ||
const enabledIds = [...enabledVendorIds, ...enabledSystemIds]; | ||
|
||
if (vendors.length === 0) { |
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 snuck in a refactor here because it was getting unwieldy to pass in enabledVendorConsentIds
, enabledVendorLegintIds
, enabledSystemLegintIds
, enabledSystemConsentIds
. instead, the parent component combines vendors and systems (this component used to do that). the parent component is also responsible for separating back out into vendors and systems to save to the backend.
Passing run #4439 ↗︎
Details:
Review all test suite changes for PR #4189 ↗︎ |
badge={gvlVendor ? "IAB TCF" : undefined} | ||
secondToggle={ | ||
<div | ||
style={{ width: "50px", display: "flex", marginLeft: ".2em" }} |
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.
because we aren't using a table, we force positioning this way 😬 follow up ticket to refactor to make things into proper tables: #4190
clients/fides-js/src/fides-tcf.ts
Outdated
preference: getInitialPreference(system), | ||
})), | ||
}; | ||
// const tcStringPreferences: TcfSavePreferences = { |
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.
looking for a second opinion on the commented out code in this file! the purpose of this logic was for the use case of we have an experience that has user preferences filled in, and now we are populating the cookie/tc string based on those values.
however, with our recent prefetch work, and the decision that TCF needs prefetch (where the cookie determines user preference rather than the backend) I don't think this makes sense anymore. In particular, it is difficult to work with in this PR because the info we get from the backend doesn't have vendorConsents
/vendorLegints
yet.
I'd like to just get rid of it to simplify code paths, but that feels like a big decision!
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.
yes you're right! Since TCF requires prefetch, this code path isn't necessary. Let's go ahead and remove this code and leave a code comment describing something like: for TCF we don't need to update the cookie on init, because we populate the pre-fetched experience with consent from a pre-existing cookie
@@ -48,7 +48,7 @@ export const generateTcString = async ({ | |||
experience, | |||
tcStringPreferences, | |||
}: { | |||
tcStringPreferences?: TcfSavePreferences; | |||
tcStringPreferences?: EnabledIds; |
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.
the logic in this func should be mostly the same as before—we just get a different input obj, so things are a little different (EnabledIds
actually maps well to this use case, because, like TC strings, it only stores opt ins)
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.
Looking good overall @allisonking ! Left a couple Qs for you
* because the backend has not implemented separate vendor legint/consents yet. | ||
* Therefore, we need both entities right now, but eventually we should be able to | ||
* only use one. In other words, `enabledIds` has a field for `vendorsConsent` and | ||
* `vendorsLegint` but `tcf` only has `vendors`. |
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 code comment, it helps me understand approach here!
|
||
// Initialize vendor values from the TC string if it's available. The cookie only | ||
// stores what the backend has, while the TC string has a little more right now. | ||
// (fidesplus#1128) |
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.
could you explain this a bit more? From what I see, we initialize from the TC string, which comes from the cookie itself, right? You mean vendorConsents
and vendorLegint
cannot be obtained from other properties on cookie
, rather we have to look in tc_string
?
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.
Yes—you have it right! Let me reword the comment to make that clearer.
clients/fides-js/src/fides-tcf.ts
Outdated
preference: getInitialPreference(system), | ||
})), | ||
}; | ||
// const tcStringPreferences: TcfSavePreferences = { |
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.
yes you're right! Since TCF requires prefetch, this code path isn't necessary. Let's go ahead and remove this code and leave a code comment describing something like: for TCF we don't need to update the cookie on init, because we populate the pre-fetched experience with consent from a pre-existing cookie
Oh, and could you add a |
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 making these changes, looks great!
Closes #4175
Description Of Changes
Screen.Recording.2023-09-28.at.8.57.58.PM.mov
Code Changes
vendor_preferences
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md