-
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
Narrow vendors disclosed to only the vendors we show in the UI #4250
Conversation
Passing run #4635 ↗︎
Details:
Review all test suite changes for PR #4250 ↗︎ |
if ( | ||
tcStringPreferences.vendorsConsent && | ||
tcStringPreferences.vendorsConsent.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 don't think we need these if
s: it's always an array, and even if it's empty, the forEach
below won't do anything
clients/fides-js/src/lib/tcf.ts
Outdated
// Narrow the GVL to say we've only showed these vendors provided by our experience | ||
const vendorIds = [ | ||
...(experience.tcf_vendor_consents?.map((v) => +v.id) || []), | ||
...(experience.tcf_vendor_legitimate_interests?.map((v) => +v.id) || []), |
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.
ah, Dawn's commit here 7b86c15 reminded me that in addition we should filter to just the IDs that are GVL, now that we are supporting AC. I will do that shortly
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 @allisonking, verified your vendors disclosed sections for accept all/reject all also match what the backend is generating.
Just that gvl filter left -
clients/fides-js/src/lib/tcf.ts
Outdated
...(experience.tcf_vendor_consents?.map((v) => +v.id) || []), | ||
...(experience.tcf_vendor_legitimate_interests?.map((v) => +v.id) || []), | ||
]; | ||
tcModel.gvl.narrowVendorsTo(vendorIds); |
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.
ah! narrowVendorsTo, good find
narrowVendorsTo - narrows vendors represented in this GVL to the list of ids passed in
thank you @pattisdr !!
thank you for the reminder—I knew there was something I left off of a PR somewhere but totally forgot it was this one! will address that now |
Closes #4165
Description Of Changes
The library we use wants to put every vendor in the GVL as part of the
vendors_disclosed
string. According to the docs, we should only put the vendors we've disclosed, i.e. the ones we've shown in the UI (makes sense!). There's avendors_disclosed
field we can set, but that appears to always be overwritten by the library to set the whole GVL. I believe the way around this is to use the library'snarrowVendorsTo
functionPart of this ticket is also to not surface the vendor disclosed part of the string to the CMP API—that's covered in #4244
Code Changes
Steps to Confirm
tc_string
object. it should have two parts, separated by a period. The second part should be very short (it used to be a very long string that ended withAAAAAAAAAAAAAAAAQ
)Pre-Merge Checklist
CHANGELOG.md