Initialize TCF earlier and set gdpr to apply always #4453

allisonking · 2023-11-22T01:52:37Z

Closes https://ethyca.atlassian.net/browse/PROD-1433

Description Of Changes

Write some things here about the changes and any potential caveats

Code Changes

list your code changes here

Steps to Confirm

list any manual steps for reviewers to confirm the changes

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- documentation complete, PR opened in fidesdocs
- documentation issue created in fidesdocs
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md
For API changes, the Postman collection has been updated

vercel · 2023-11-22T01:52:43Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		Nov 22, 2023 9:40pm

cypress · 2023-11-22T02:03:30Z

Passing run #5355 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge `5bc7311` into `cb8089f`...
Project: fides	Commit: `148bbef878 ℹ️`
Status: Passed	Duration: 00:36 💡
Started: Nov 22, 2023 9:43 PM	Ended: Nov 22, 2023 9:44 PM

Review all test suite changes for PR #4453 ↗︎

NevilleS

Thanks! Quick functionality ask for some future proofing

NevilleS · 2023-11-22T13:35:54Z

clients/fides-js/src/lib/tcf/stub.ts

+   * Fides modification (PROD#1433): gdprApplies defaults to true
+   * since we only return TCF when gdpr does apply!
+   */
+  let gdprApplies: boolean = true;


One ask: for our future benefit, let's do this as an override option fides_tcf_gdpr_applies and default it to true there. This gives us an out if we need one 👍

eastandwestwind · 2023-11-22T14:59:15Z

clients/fides-js/src/fides-tcf.ts

@@ -241,6 +242,7 @@ const updateFidesCookieFromString = (
 * Initialize the global Fides object with the given configuration values
 */
 const init = async (config: FidesConfig) => {
+  makeStub();


allisonking · 2023-11-22T16:27:08Z

clients/fides-js/src/lib/tcf.ts

  // Initialize api with TC str, we don't yet show UI, so we use false
  // see https://github.com/InteractiveAdvertisingBureau/iabtcf-es/tree/master/modules/cmpapi#dont-show-ui--tc-string-does-not-need-an-update
  window.addEventListener("FidesInitialized", (event) => {
    const tcString = fidesEventToTcString(event);
-    cmpApi.update(tcString ?? null, false);


fidesEventToTcString now does the work of defaulting to a value. since if there's a tc string at all (including an empty string) the library sets gdprApplies = true. so fidesEventToTcString additionally checks if gdpr applies first. this might not actually be what we want though since it can effectively turn off setting a tc string at all. perhaps the override should just be for the initial default value? I did this at first, but I couldn't actually test it since the period where there's a default value until this line was too short for cypress to catch

maybe only FidesInitialized should do the check for fides_tcf_gdpr_applies and the other events should stick to what they were?

Hmmm... a philosophical question eh?

I would recommend we check for that option override in one place, so I think it's OK to centralize into the helper function that extracts the string.

It's a bit weird that the CmpApi class sets GDPR applies based on a string though. Looking at their docs a bit to form a stronger POV

OK, their docs are super explicit: https://github.com/InteractiveAdvertisingBureau/iabtcf-es/blob/master/modules/cmpapi/README.md#trigger-change-event

update() may be called either an encoded TC string an empty string ('') or null.

Encoded TC string, CmpApi will decode the string and respond to TCData with the decoded values.

gdprApplies will be set to true

Empty string (''), CmpApi will respond to TCData with the correct structure but all primitive values will be empty.

gdprApplies will be set to true

null, CmpApi will respond to TCData with the correct structure but all primitive values will be empty.

gdprApplies will be set to false

In that case I think we can document this more clearly but I agree with your implementation 👍

allisonking · 2023-11-22T16:32:58Z

clients/privacy-center/cypress/fixtures/consent/test_banner_options.json

@@ -102,6 +102,7 @@
    "isGeolocationEnabled": false,
    "geolocationApiUrl": "",
    "privacyCenterUrl": "http://localhost:3000",
-    "fidesApiUrl": "http://localhost:8080/api/v1"
+    "fidesApiUrl": "http://localhost:8080/api/v1",
+    "fidesTcfGdprApplies": true


since this is an option that defaults to true, and because our test harness doesn't go through fides-js the API (which defaults this value to true), we set the default to true here.

NevilleS

Leaving a quick comment now but doing a bit more reading!

clients/privacy-center/app/server-environment.ts

NevilleS · 2023-11-22T21:09:53Z

clients/fides-js/src/lib/tcf.ts

  // Initialize api with TC str, we don't yet show UI, so we use false
  // see https://github.com/InteractiveAdvertisingBureau/iabtcf-es/tree/master/modules/cmpapi#dont-show-ui--tc-string-does-not-need-an-update
  window.addEventListener("FidesInitialized", (event) => {
    const tcString = fidesEventToTcString(event);
-    cmpApi.update(tcString ?? null, false);


Hmmm... a philosophical question eh?

I would recommend we check for that option override in one place, so I think it's OK to centralize into the helper function that extracts the string.

It's a bit weird that the CmpApi class sets GDPR applies based on a string though. Looking at their docs a bit to form a stronger POV

NevilleS

Nitpicks (including a renamed function!) but otherwise this looks good.

NevilleS · 2023-11-22T21:13:05Z

clients/fides-js/src/lib/tcf.ts

  // Initialize api with TC str, we don't yet show UI, so we use false
  // see https://github.com/InteractiveAdvertisingBureau/iabtcf-es/tree/master/modules/cmpapi#dont-show-ui--tc-string-does-not-need-an-update
  window.addEventListener("FidesInitialized", (event) => {
    const tcString = fidesEventToTcString(event);
-    cmpApi.update(tcString ?? null, false);


OK, their docs are super explicit: https://github.com/InteractiveAdvertisingBureau/iabtcf-es/blob/master/modules/cmpapi/README.md#trigger-change-event

update() may be called either an encoded TC string an empty string ('') or null.

Encoded TC string, CmpApi will decode the string and respond to TCData with the decoded values.

gdprApplies will be set to true

Empty string (''), CmpApi will respond to TCData with the correct structure but all primitive values will be empty.

gdprApplies will be set to true

null, CmpApi will respond to TCData with the correct structure but all primitive values will be empty.

gdprApplies will be set to false

In that case I think we can document this more clearly but I agree with your implementation 👍

clients/fides-js/src/lib/tcf/events.ts

NevilleS · 2023-11-28T22:53:57Z

clients/fides-js/src/lib/tcf/stub.ts

+}: {
+  gdprAppliesDefault?: boolean;
+}) => {
+  console.log({ gdprAppliesDefault });


I missed this one...

Initialize TCF earlier and set gdpr to apply always

86e2472

NevilleS requested changes Nov 22, 2023

View reviewed changes

eastandwestwind reviewed Nov 22, 2023

View reviewed changes

allisonking added 2 commits November 22, 2023 11:19

New override option fides_tcf_gdpr_applies

0a583ce

Cypress tests

8e7555d

allisonking commented Nov 22, 2023

View reviewed changes

allisonking marked this pull request as ready for review November 22, 2023 16:34

Merge branch 'main' into aking/prod-1433/tcf-initialization

111e924

vercel bot deployed to Preview November 22, 2023 19:49 View deployment

NevilleS reviewed Nov 22, 2023

View reviewed changes

allisonking added 2 commits November 22, 2023 16:15

Merge branch 'main' into aking/prod-1433/tcf-initialization

7bfaf9b

Remove console.log

9bd06e9

NevilleS approved these changes Nov 22, 2023

View reviewed changes

allisonking added 2 commits November 22, 2023 16:33

Rename function and add docstring

5bc7311

Update changelog

2579ae6

allisonking merged commit fe31cb5 into main Nov 22, 2023

allisonking deleted the aking/prod-1433/tcf-initialization branch November 22, 2023 21:46

NevilleS reviewed Nov 28, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize TCF earlier and set gdpr to apply always #4453

Initialize TCF earlier and set gdpr to apply always #4453

allisonking commented Nov 22, 2023

vercel bot commented Nov 22, 2023 •

edited

Loading

cypress bot commented Nov 22, 2023 •

edited

Loading

NevilleS left a comment

NevilleS Nov 22, 2023

eastandwestwind Nov 22, 2023

allisonking Nov 22, 2023

allisonking Nov 22, 2023

NevilleS Nov 22, 2023

NevilleS Nov 22, 2023

allisonking Nov 22, 2023

NevilleS left a comment

NevilleS Nov 22, 2023

NevilleS left a comment

NevilleS Nov 22, 2023

NevilleS Nov 28, 2023

Initialize TCF earlier and set gdpr to apply always #4453

Initialize TCF earlier and set gdpr to apply always #4453

Conversation

allisonking commented Nov 22, 2023

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

vercel bot commented Nov 22, 2023 • edited Loading

cypress bot commented Nov 22, 2023 • edited Loading

Passing run #5355 ↗︎

Review all test suite changes for PR #4453 ↗︎

NevilleS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NevilleS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NevilleS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Nov 22, 2023 •

edited

Loading

cypress bot commented Nov 22, 2023 •

edited

Loading