-
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
GPP serving TCF strings #4433
GPP serving TCF strings #4433
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Passing run #5342 ↗︎
Details:
Review all test suite changes for PR #4433 ↗︎ |
import { makeStub } from "../lib/gpp/stub"; | ||
import { fidesEventToTcString } from "../lib/tcf/events"; | ||
|
||
const CMP_ID = 407; // TODO: is this supposed to be the same as TCF, or is this separate? |
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.
Rachel will confirm this on Monday! https://ethyca.slack.com/archives/C05E74TR0BB/p1700245618221449
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.
she has confirmed! We can just use our TCF CMP ID, so let's remove this comment
@@ -368,7 +368,7 @@ export const initialize = async ({ | |||
identity: cookie.identity, | |||
fides_string: cookie.fides_string, | |||
tcf_consent: cookie.tcf_consent, | |||
experience, | |||
experience: effectiveExperience, |
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 a potentially consequential change—before, if the experience was not server side provided, then window.Fides.experience
would be undefined
. I don't think there was a reason for this, and I didn't see why we shouldn't always have the effective experience here. but let me know if I'm missing something!
This basically makes it possible for the "FidesInitialized" listener in gpp.ts
to grab the experience easily, without it having to come through the event object.
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 good. The return obj built here is only used to update the window.Fides
obj, which we don't rely on to make any functional decisions within or outside of Fides. Regardless, we should absolutely return the most up-to-date experience here. Thanks!
@@ -149,25 +149,6 @@ export const generateFidesString = async ({ | |||
return Promise.resolve(encodedString); | |||
}; | |||
|
|||
/** |
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.
moved to a separate file so gpp can also use this function
}, [cookie, options.debug]); | ||
const dispatchCloseEvent = useCallback( | ||
({ saved = false }: { saved?: boolean }) => { | ||
dispatchFidesEvent("FidesModalClosed", cookie, options.debug, { saved }); |
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 need to know if the modal was closed without saving. gpp signals that it is not ready when the modal opens. once it saves, it signals it is ready again. but if you close the modal without saving, we'd be stuck in a "not ready" state
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.
Great work on this @allisonking !
Do we have a separate ticket to add e2e tests, once we actually start making calls to the GPP lib?
Thumbs up from me, just need to remove an unneeded code comment 👍
import { makeStub } from "../lib/gpp/stub"; | ||
import { fidesEventToTcString } from "../lib/tcf/events"; | ||
|
||
const CMP_ID = 407; // TODO: is this supposed to be the same as TCF, or is this separate? |
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.
she has confirmed! We can just use our TCF CMP ID, so let's remove this comment
@@ -368,7 +368,7 @@ export const initialize = async ({ | |||
identity: cookie.identity, | |||
fides_string: cookie.fides_string, | |||
tcf_consent: cookie.tcf_consent, | |||
experience, | |||
experience: effectiveExperience, |
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 good. The return obj built here is only used to update the window.Fides
obj, which we don't rely on to make any functional decisions within or outside of Fides. Regardless, we should absolutely return the most up-to-date experience here. Thanks!
Thanks for the review @eastandwestwind ! I'll clean that comment up.
Yes! It is here: https://ethyca.atlassian.net/browse/PROD-1420 |
Closes https://ethyca.atlassian.net/browse/PROD-1285
Description Of Changes
https://www.loom.com/share/ce626a97f570422389e9b0116a3f5bde
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md