-
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
Surface AC Systems in Vendor Consents section of the TCF Overlay #4266
Surface AC Systems in Vendor Consents section of the TCF Overlay #4266
Conversation
Passing run #4678 ↗︎
Details:
Review all test suite changes for PR #4266 ↗︎ |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4266 +/- ##
==========================================
+ Coverage 87.75% 87.76% +0.01%
==========================================
Files 331 331
Lines 20907 20922 +15
Branches 2711 2714 +3
==========================================
+ Hits 18346 18362 +16
+ Misses 2094 2093 -1
Partials 467 467
☔ View full report in Codecov by Sentry. |
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 looks great—very clean and easy to follow, well done! will test it out with the FE now to see if things Just Work and report back
ah I guess this will also need the prefix change before I can test E2E with local compass |
I'll start on that now! |
…fix) in the TCF overlay under the Vendor Consents section, even if it has no privacy declarations. - Separately update the "relevant systems" when vendor consent preferences are saved to include ac systems in the lookup -
- Dry up defining AC and GVL prefixes.
5d1e697
to
035dfce
Compare
Rebased onto main due to messy diff when this was originally based off of the compass integration branch. @adamsachs |
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 changes look great here @pattisdr, this is a lot less painful than i expected, which owes a lot to your clean implementation! just had some minor nits, none of which i'd consider blocking - but would be curious to get your thoughts on them.
i didn't get a chance to smoke test, but if you've done some manual testing, then i think that's sufficient since the changes are straightforward enough. i can circle back and do some testing though if you'd like that extra confirmation!
if not tcf_vendor_component_type == TCFVendorConsentRecord: | ||
return | ||
|
||
if not system_identifier.startswith(AC_PREFIX): | ||
return | ||
|
||
if system_identifier in vendor_map: | ||
return |
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.
out of curiosity - do you think this reads easier as 3 separate conditionals rather than joining them in a single conditional with or
s? just caught my eye since it's not the way i would have written it, but i'm guessing this was deliberate so i'm curious to hear! no need to adjust obviously.
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 do think it's easier to parse this way 😎
- Update docstring - Add test for experience contents with an AC system with a valid privacy declaration
Thanks for the review Adam - I always do manual testing! I'll go ahead and merge and get the follow-up PR up that is building AC strings on the backend, building off of this work - |
Closes https://github.com/ethyca/fidesplus/issues/1155
Description Of Changes
AC systems should show up in the TCF overlay and need vendor consent toggles. They are not required to have privacy declarations though, which currently drives what is in the TCF overlay.
Code Changes
Surface AC Systems (systems whose vendor_id starts with an "ac.*" prefix) in the TCF overlay under the Vendor Consents section, even if it has no privacy declarations.
Separately update the "relevant systems" when vendor consent preferences are saved to include ac systems in the lookup
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md