-
Notifications
You must be signed in to change notification settings - Fork 77
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
Save consent method ("accept", "reject", "save", etc.) to fides_consent
cookie as extra metadata
#4529
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Passing run #5713 ↗︎
Details:
Review all test suite changes for PR #4529 ↗︎ |
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.
code looks good! will UAT a bit later, unless @galvana can get to it first!
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.
My own notes, for review help!
@@ -109,23 +111,47 @@ describe("getOrMakeFidesCookie", () => { | |||
const SAVED_CONSENT = { data_sales: false, performance: true }; | |||
|
|||
describe("in v0.9.0 format", () => { | |||
const V090_COOKIE = JSON.stringify({ | |||
const V090_COOKIE_OBJECT: FidesCookie = { |
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.
Added the type here to ensure we're keeping this test accurate - and then updated it to include the required tcf_consent
field 👍
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.
Would it be a good idea to make this change to other places in this test file that still use const V090_COOKIE = JSON.stringify({...
?
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.
Good suggestion! Updated
fides_consent
cookie as extra metadata
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 the test cleanup! I just made a small comment on the cookie.test.ts
file but overall this looks good 👍
@@ -109,23 +111,47 @@ describe("getOrMakeFidesCookie", () => { | |||
const SAVED_CONSENT = { data_sales: false, performance: true }; | |||
|
|||
describe("in v0.9.0 format", () => { | |||
const V090_COOKIE = JSON.stringify({ | |||
const V090_COOKIE_OBJECT: FidesCookie = { |
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.
Would it be a good idea to make this change to other places in this test file that still use const V090_COOKIE = JSON.stringify({...
?
UAT@NevilleS I verified locally that the Fides cookie contains TCF dismiss banner
TCF dismiss modal
Consent dismiss banner
Consent dismiss modal
|
Description Of Changes
This updates both
fides.js
andfides-privacy-center
to record the "consent method" to the local cookie when preferences are saved, in the same way that we record this information on the server-side preference that is recorded. This'll allow us to inspect this on the frontend, both for troubleshooting (helpful!) but more importantly because I'd like to treatconsentMethod: "dismiss"
differently in a future release so I want to be recording that metadata now 👍Code Changes
ConsentMethod
enum definition across all client applicationsfides_meta
on cookieconsentMethod
to be stored on cookieconsentMethod
to be stored on cookiebutton
consent methodsave
actionsave
,accept
,reject
, anddismiss
Steps to Confirm
fides_consent
cookie on the Cookie House after using the overlayPre-Merge Checklist
CHANGELOG.md