Recording interactions with TCF notices #4161

galvana · 2023-09-26T03:07:57Z

Description Of Changes

Adding served_notice_history_id to the TCF fields of the PATCH /privacy-preference request.

Code Changes

Created a distinction between banner and tcf_banner for FidesUIShown events
PATCH /notices-served is called from the tcf_banner and the consent overlay
Updated Cypress tests for the TCF banner worfklows

Steps to Confirm

Set FIDES__CONSENT__TCF_ENABLED to true
Start fidesplus
Navigate to http://localhost:3000/fides-js-demo.html?geolocation=fi-18 to view the TCF banner
Look for the Dispatching event type FidesUIShown from tcf_banner in the browser console
Opt-in or opt-out and verify a call to PATCH /privacy-preferences is made, the TCF fields should have the served_notice_history_id

Pre-Merge Checklist

All CI Pipelines Succeeded
Issue Requirements are Met
Update CHANGELOG.md

cypress · 2023-09-26T03:23:15Z

Passing run #4799 ↗︎

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

Details:

Merge `9a00147` into `cf5814b`...
Project: fides	Commit: `e507d1d864 ℹ️`
Status: Passed	Duration: 00:55 💡
Started: Oct 25, 2023 3:23 AM	Ended: Oct 25, 2023 3:24 AM

Review all test suite changes for PR #4161 ↗︎

allisonking

nice job @galvana this is looking like the right approach so far! we'll want to make sure the types are right—they make sense to me, but we should verify with a npm run openapi:generate. let me know if you'd like help with that!

allisonking · 2023-09-26T17:13:31Z

clients/fides-js/src/components/tcf/TcfOverlay.tsx

+
+  const isAllNoticeOnly = privacyNotices.every(
+    (n) => n.consent_mechanism === ConsentMechanism.NOTICE_ONLY
+  );


we don't need this right now—all TCF payloads will return an empty privacy_notices array, so we can just pass [] to notices. also, TCF doesn't have a concept of notice-only yet (see #4141), so for now, let's just pass in false for acknowledgeMode

allisonking · 2023-09-26T17:16:30Z

clients/fides-js/src/components/tcf/TcfOverlay.tsx

  const handleUpdateAllPreferences = useCallback(
-    (enabledIds: EnabledIds) => {
-      const tcf = createTcfSavePayload({ experience, enabledIds });
+    (enabledIds: EnabledIds, servedNotices: LastServedNoticeSchema[]) => {


I don't think servedNotices needs to be an arg here—I think you can just reference it above (since it's function inside the component, it has access to its vars)

That's what I tried originally but when I logged out the value of servedNotices inside handleUpdateAllPreferences it was empty. The only way I could get it to work is by passing it in as a param. Could it be something to do with useConsentServed?

Never mind, this is working without the servedNotices arg

…ices

codecov · 2023-09-29T04:15:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cf5814b) 87.83% compared to head (9a00147) 87.84%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4161   +/-   ##
=======================================
  Coverage   87.83%   87.84%           
=======================================
  Files         333      333           
  Lines       21065    21066    +1     
  Branches     2742     2742           
=======================================
+ Hits        18503    18505    +2     
+ Misses       2097     2096    -1     
  Partials      465      465

Files	Coverage Δ
src/fides/api/models/privacy_preference.py	`97.72% <100.00%> (+<0.01%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ices

allisonking

awesome job figuring this out @galvana ! I left a comment for something I think we can improve, curious to hear your thoughts!

allisonking · 2023-10-12T20:15:18Z

clients/fides-js/src/components/tcf/TcfOverlay.tsx

+  | "vendor_legitimate_interests"
+  | "system_consent"
+  | "system_legitimate_interests";
+type NoticeSubMap = {


these aren't technically Notices (we typically don't use notices for TCF). maybe we can change this to be TcfEntitiesMap?

allisonking · 2023-10-12T20:15:59Z

clients/fides-js/src/components/tcf/TcfOverlay.tsx

+
+type NoticeMap = Record<Category, NoticeSubMap>;
+
+const noticeMap = (servedNotices: LastServedConsentSchema[]): NoticeMap => {


heh I guess these shouldn't really be called servedNotices either (you probably noticed the backend type changed from LastServedNoticeSchema --> LastServedConsentSchema) so I guess this should be servedConsent. it's definitely a nit, but I think not conflating notices and TCF is a good distinction that helps with understanding all that's going on here

allisonking · 2023-10-12T20:18:28Z

clients/fides-js/src/components/tcf/TcfOverlay.tsx

+
+  servedNotices.forEach((notice) => {
+    (Object.keys(map) as Category[]).forEach((key) => {
+      const value = notice[key as keyof LastServedConsentSchema];


it might help TS to do something like this https://github.com/ethyca/fides/blob/main/clients/fides-js/src/lib/tcf/types.ts/#L177-L186 for Category and LastServedConsentSchema. that said, it might not get you as far as you want, sometimes TS is picky about this kind of thing. but it might save you an as here or there

Let's discuss this next week, I couldn't figure out a way to remove the as keyof without adding more code than we currently have for the related type definitions.

okay no worries—I figured TS might get picky about it. let's leave it as-is then!

allisonking · 2023-10-12T20:21:04Z

clients/fides-js/src/components/tcf/TcfOverlay.tsx

@@ -104,17 +147,21 @@ const transformTcfModelToTcfSave = ({
    return {
      id: model.id,
      preference,
+      served_notice_history_id: noticeSubMap[String(model.id)],


ah I guess this type is still called notice in the backend 😅 ok I take back what I said about distinguishing between notices and TCF...

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

clients/privacy-center/cypress/e2e/consent-banner-tcf.cy.ts

allisonking · 2023-10-12T20:37:23Z

clients/fides-js/src/components/tcf/TcfOverlay.tsx

@@ -246,14 +312,14 @@ const TcfOverlay: FunctionComponent<OverlayProps> = ({
        userLocationString: fidesRegionString,
        cookie,
        debug: options.debug,
-        servedNotices: null, // TODO: served notices
+        servedNotices: null,


I think the thing to do here is to actually refactor NoticeOverlay a little too... I know that's not what this ticket calls for, but I think it'd be a good thing to do if you're up for it. basically, before TCF ever came along, we would:

In NoticeOverlay get the servedNotices

When the user saves, pass the servedNotices to updateConsentPreferences along with the consentPreferencesToSave

updateConsentPreferences would then do the unenviable task of mapping servedNotices to consentPreferencesToSave

Now that we have TCF, we:

In TcfOverlay get the servedNotices

TcfOverlay does the unenviable task of mapping servedNotices to each of the tcf entities (what your noticeSubMap is doing

We pass the tcf obj directly to updateConsentPreferences and set servedNotices=null

This is a good approach, and I think we should do the same for notices. In other words, I think updateConsentPreferences should no longer take a servedNotices, and we should pass an object that already has the servedNotices matched into updateConsentPreferences, same as we do for TCF. so then we can remove some code from preferences.ts. this might involve updating the type that updateConsentPreferences takes but hopefully it's not arduous. what do you think?

Sounds good! I gave it a shot, let me know if that's what you had in mind.

nice job, I think you handled it perfectly! 💯

…ices

allisonking

I'm pretty much ready to approve this as I tested locally and it looks great, and I super appreciate the refactor you did for notices! I'm just going to go ask product one more question, and then hopefully we can get this merged 💥

allisonking · 2023-10-16T14:18:23Z

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

+        (event.detail.extraDetails.servingComponent !==
+          ServingComponent.OVERLAY &&
+          event.detail.extraDetails.servingComponent !==
+            ServingComponent.TCF_BANNER)


now that I'm testing this out, I think there's something we should ask product for clarity on... we had a discussion back when we did this for notices of "when" to say consent was served. for notices, we decided only for when the modal pops up, since the banner doesn't actually render notices.

for TCF, as you know, we decided the banner does render what the user is consenting to (via the "stacks") and so we put it on the banner. however, nothing happens if only the modal is opened. for example, if I've already opted in to everything, the banner doesn't render the next time I visit the site. but if I decide I want to make a change and open the modal (by clicking "Manage Consent" on the page in the screenshot), then we don't send another notices-served patch.

I suspect we want to send it for both banner and overlay for TCF, but that we don't want to send it twice, if the user goes through the flow of 1. banner opens 2. user clicks "manage preferences" in the banner 3. the modal pops up. that was actually my first implementation of notices back when I was POC-ing it—we can add a state to the hook and it should sort itself out: 5ad7ebc

but let's confirm with product first, I can ask...

…ices

galvana · 2023-10-25T03:10:30Z

I was finally able to get back to this. I updated the conditions to trigger the notices served to include the TCF_OVERLAY. The only time we don't trigger the notice is for the consent banner, since that's the only component where notices aren't shown directly.

allisonking

whew, way to persist through this PR! I know it's probably been haunting you for a while now 😄 great job! ⭐

allisonking · 2023-10-25T21:30:32Z

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

-          ServingComponent.OVERLAY &&
-          event.detail.extraDetails.servingComponent !==
-            ServingComponent.TCF_BANNER)
+        event.detail.extraDetails.servingComponent === ServingComponent.BANNER


ah nice, this is clean, and the comment helps a lot 👍

…ices

galvana · 2023-10-25T22:10:45Z

whew, way to persist through this PR! I know it's probably been haunting you for a while now 😄 great job! ⭐

I'm free!

First pass

9739820

allisonking reviewed Sep 26, 2023

View reviewed changes

galvana added 2 commits September 26, 2023 19:34

Hard-coding values for TcfOverlay useConsentServed

7865a6a

Merge branch 'main' into PROD-914-recording-interactions-with-tcf-not…

f6e1644

…ices

galvana linked an issue Sep 28, 2023 that may be closed by this pull request

Recording interactions with TCF notices #3877

Closed

galvana added 2 commits September 28, 2023 10:53

Merge branch 'main' into PROD-914-recording-interactions-with-tcf-not…

3d20547

…ices

Making distinction between banner and tcf_banner

66a9966

galvana changed the title ~~First pass~~ Recording interactions with TCF notices Sep 28, 2023

galvana added 2 commits September 28, 2023 20:19

Merge branch 'main' into PROD-914-recording-interactions-with-tcf-not…

aa36a5e

…ices

Removing unused imports

2f72a39

galvana added 9 commits October 6, 2023 16:19

Merge branch 'main' into PROD-914-recording-interactions-with-tcf-not…

36c495c

…ices

Fixing merge conflict resolution issues

f575144

Adding new TCF mappings

101bd42

Fixing typo

104ccfd

Served notice cleanup

fc63ef8

Merge branch 'main' into PROD-914-recording-interactions-with-tcf-not…

157a44d

…ices

Updating tests and raising bundle size

c3209ba

Updating TCF tests

ba83ca6

Merge branch 'main' into PROD-914-recording-interactions-with-tcf-not…

28e0335

…ices

galvana marked this pull request as ready for review October 12, 2023 07:45

Merge branch 'main' into PROD-914-recording-interactions-with-tcf-not…

cc6bc37

…ices

galvana requested a review from allisonking October 12, 2023 16:55

allisonking reviewed Oct 12, 2023

View reviewed changes

galvana added 3 commits October 12, 2023 16:41

Merge branch 'main' into PROD-914-recording-interactions-with-tcf-not…

b50987c

…ices

Merge branch 'main' into PROD-914-recording-interactions-with-tcf-not…

fa39dae

…ices

Refactoring updateConsentPreferences call for NoticeOverlay

ec6e6ba

galvana requested a review from allisonking October 14, 2023 00:34

Lint fix

e47ae05

Additional lint fixes

4ccaea3

allisonking reviewed Oct 16, 2023

View reviewed changes

galvana added 3 commits October 24, 2023 19:22

Merge branch 'main' into PROD-914-recording-interactions-with-tcf-not…

11c51a3

…ices

Increasing allowed bundle size

cbe77bf

Updating conditions to trigger a notices served API call

9a00147

galvana requested a review from allisonking October 25, 2023 03:09

allisonking approved these changes Oct 25, 2023

View reviewed changes

galvana added 2 commits October 25, 2023 15:07

Updating change log

e2775d9

Merge branch 'main' into PROD-914-recording-interactions-with-tcf-not…

4c6f008

…ices

galvana merged commit 0dfa690 into main Oct 25, 2023

galvana deleted the PROD-914-recording-interactions-with-tcf-notices branch October 25, 2023 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recording interactions with TCF notices #4161

Recording interactions with TCF notices #4161

galvana commented Sep 26, 2023 •

edited

Loading

cypress bot commented Sep 26, 2023 •

edited

Loading

allisonking left a comment

allisonking Sep 26, 2023

allisonking Sep 26, 2023

galvana Sep 27, 2023

galvana Oct 10, 2023

codecov bot commented Sep 29, 2023 •

edited

Loading

allisonking left a comment

allisonking Oct 12, 2023

allisonking Oct 12, 2023

allisonking Oct 12, 2023

galvana Oct 14, 2023

allisonking Oct 16, 2023

allisonking Oct 12, 2023

allisonking Oct 12, 2023

galvana Oct 14, 2023

allisonking Oct 16, 2023

allisonking left a comment

allisonking Oct 16, 2023

galvana commented Oct 25, 2023

allisonking left a comment

allisonking Oct 25, 2023

galvana commented Oct 25, 2023


		type NoticeMap = Record<Category, NoticeSubMap>;

		const noticeMap = (servedNotices: LastServedConsentSchema[]): NoticeMap => {

Recording interactions with TCF notices #4161

Recording interactions with TCF notices #4161

Conversation

galvana commented Sep 26, 2023 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

cypress bot commented Sep 26, 2023 • edited Loading

Passing run #4799 ↗︎

Review all test suite changes for PR #4161 ↗︎

allisonking 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

codecov bot commented Sep 29, 2023 • edited Loading

Codecov Report

allisonking 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galvana commented Oct 25, 2023

allisonking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galvana commented Oct 25, 2023

galvana commented Sep 26, 2023 •

edited

Loading

cypress bot commented Sep 26, 2023 •

edited

Loading

codecov bot commented Sep 29, 2023 •

edited

Loading