Fix Fides.shouldShouldShowExperience() to return false for modal-only experiences #5281

NevilleS · 2024-09-12T11:20:04Z

Closes PROD-2763

Description Of Changes

This adds a tiny bit of logic to the FidesJS SDK function shouldShowExperience() to fix a bug where we would return true confusingly for modal-only experiences that would not automatically show an experience to prompt for consent. The expected behaviour of this function is to return true only when the user should be prompted via a consent banner.

The bulk of this PR adds a thorough set of E2E tests to ensure that this SDK function is tested over all the FidesJS experience types, with & without saved consent.

Code Changes

Fix shouldResurfaceConsent helper to always return false for modal-only experiences
Fix unrelated bug in updateExperienceFromCookieConsentNotices to correctly handle prefetched experiences without any notices (e.g. an empty experience)
Add automated E2E tests for modal, banner+modal, tcf, and empty experiences
Remove lingering references to the show_banner field that was removed

Steps to Confirm

Load a default US Modal experience in Cookie House and confirm that Fides.shouldShowExperience() returns false as expected
Load a default EEA Banner + Modal experience in Cookie House and confirm that Fides.shouldShowExperience() returns true as expected and the banner is shown
Opt-in or opt-out of the EEA Banner, refresh the page, and confirm that Fides.shouldShowExperience() returns false as expected and no banner is shown
Load a default TCF Overlay experience in Cookie House and confirm that Fides.shouldShowExperience() returns true as expected and the banner is shown
Opt-in or opt-out of the TCF Overlay, refresh the page, and confirm that Fides.shouldShowExperience() returns false as expected and no banner is shown

Pre-Merge Checklist

… true

vercel · 2024-09-12T11:20:08Z

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

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		Sep 12, 2024 0:17am

…uld-show-experience

cypress · 2024-09-12T11:33:30Z

fides Run #9922

Run Properties: Passed #9922 • 6a797ccd74 ℹ️: Merge 2fb7e21c75c8762ef267e8e3d74df2d83d713474 into aa37211d487f7ec3a5b38b58f923...

Project	`fides`
Branch Review	`refs/pull/5281/merge`
Run status	`Passed #9922`
Run duration	`00m 38s`
Commit	`6a797ccd74 ℹ️: Merge 2fb7e21c75c8762ef267e8e3d74df2d83d713474 into aa37211d487f7ec3a5b38b58f923...`
Committer	`Neville Samuell`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`4`
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

NevilleS

Couple guiding comments!

NevilleS · 2024-09-12T12:20:23Z

clients/fides-js/src/lib/consent-utils.ts

+  // Never surface consent for modal-only experiences
+  if (experience.experience_config?.component === ComponentType.MODAL) {
+    return false;
+  }


This is the fix - it's effectively a one-line adjustment to make this utility shouldResurfaceConsent() behave as expected when using a modal-only experience. This method is called directly by Fides.shouldShowExperience() to determine whether or not the experience will be shown to the user automatically during the init() sequence.

(the rest of the PR is E2E tests for all the experience types)

Great. Thanks for adding the e2e tests!

NevilleS · 2024-09-12T12:23:33Z

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

+  // If the given experience has no notices, return immediately and do not mutate
+  // the experience object in any way
+  if (!experience.privacy_notices) {
+    return experience;
+  }


This was an unrelated, but subtle bug. I found that if there was a saved consent cookie and a prefetched empty experience (e.g. experience: {}), this function would get called and edit the experience to become { privacy_notices: undefined }, which is unexpected by the rest of the code base.

The impact of this is probably zero-to-none, it'd be a really extreme edge case where a user (1) visits a website and gets a valid experience, (2) saves their consent preference, (3) visits the website again from a different location without an experience, (4) this method causes the experience to appear non-empty but without notices, (5) client will then refetch the experience client-side, slowing down the init.

Just fixing this up.

Thanks. It's definitely not a common use case, but I can see how someone travelling or VPNing might get into this state.

NevilleS · 2024-09-12T12:24:28Z

clients/privacy-center/cypress/e2e/should-show-experience.cy.ts

+    {
+      fixture: "experience_modal.json",
+      savedConsent: false,
+      shouldShowExperience: false,
+    },


This was the failing case- without the fix to shouldResurfaceConsent, this test fails because shouldShowExperience returns true unexpectedly.

NevilleS · 2024-09-12T12:25:15Z

clients/privacy-center/cypress/e2e/should-show-experience.cy.ts

+      expiredTcfVersionHash,
+      shouldShowExperience,
+    }) => {
+      describe(`when rendering ${fixture} and saved consent ${savedConsent ? "exists" : "does not exist"} ${expiredTcfVersionHash ? "(with expired tcf_version_hash)" : ""}`, () => {


Thanks to the parameterized tests above, this'll exercise 9 different combinations of experiences + saved consent.

NevilleS · 2024-09-12T12:26:54Z

clients/privacy-center/cypress/e2e/should-show-experience.cy.ts

+  interface TestCaseOptions {
+    fixture:
+      | "experience_modal.json"
+      | "experience_banner_modal.json"
+      | "experience_tcf_minimal.json"
+      | "experience_none.json";
+    savedConsent: boolean;
+    expiredTcfVersionHash?: boolean;
+    shouldShowExperience: boolean;
+  }


This test fixtures modify three inputs:

fixture: the type of experience to test

savedConsent: whether or not the user has a previously saved consent cookie

expiredTcfVersionHash: special case for TCF, whether the user should have a previously saved consent cookie but with an expired version hash (that doesn't match the latest experience)

It then asserts to see if Fides.shouldShowExperience() returns the expected result of shouldShowExperience in the param.

NevilleS · 2024-09-12T12:27:34Z

clients/privacy-center/cypress/e2e/show-modal.cy.ts

@@ -10,9 +10,6 @@ describe("Fides.showModal", () => {
        options: {
          isOverlayEnabled: true,
        },
-        experience: {


This was a red herring and didn't affect these tests, removing show_banner entirely as it's not returned by the API anymore.

NevilleS · 2024-09-12T12:28:15Z

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

+        "auto_detect_language": true,
+        "disabled": false,
+        "id": "pri_exp-modal-000",
+        "component": "modal",


Added this fixture to ensure we had a mock API response for the US-only modal component type.

NevilleS · 2024-09-12T12:28:36Z

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

@@ -0,0 +1,6 @@
+{
+  "items": [],


Added this fixture to ensure we had a mock response for an empty experience, not matching any location.

lucanovera

Tested through each use case and found the shouldShowExperience returned value to be correct each time. Found no issues with the code. Approved!

lucanovera · 2024-09-12T17:43:24Z

clients/fides-js/src/lib/consent-utils.ts

+  // Never surface consent for modal-only experiences
+  if (experience.experience_config?.component === ComponentType.MODAL) {
+    return false;
+  }


Great. Thanks for adding the e2e tests!

lucanovera · 2024-09-12T17:44:57Z

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

+  // If the given experience has no notices, return immediately and do not mutate
+  // the experience object in any way
+  if (!experience.privacy_notices) {
+    return experience;
+  }


Thanks. It's definitely not a common use case, but I can see how someone travelling or VPNing might get into this state.

… experiences (#5281)

cypress · 2024-09-12T19:05:16Z

fides Run #9933

Run Properties: Passed #9933 • 47934f2610: Fix Fides.shouldShouldShowExperience() to return false for modal-only experience...

Project	`fides`
Branch Review	`main`
Run status	`Passed #9933`
Run duration	`00m 38s`
Commit	`47934f2610: Fix Fides.shouldShouldShowExperience() to return false for modal-only experience...`
Committer	`Neville Samuell`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`4`
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

NevilleS added 6 commits September 12, 2024 00:34

Add failing test for shouldShowExperience for opt-out modal

04aee87

Fix a subtle bug when showing empty experiences with saved consent

718f719

Remove lingering, redundant reference to show_banner

9cc082a

Fix mock modal fixture

92f14ba

Add a defensive test that the banner is shown when show experience is…

f3e578f

… true

Fix shouldShowExperience to return false for modal-only experiences

88724e3

Merge branch 'main' of github.com:ethyca/fides into PROD-2763-fix-sho…

8abea01

…uld-show-experience

Update CHANGELOG

2fb7e21

NevilleS commented Sep 12, 2024

View reviewed changes

lucanovera approved these changes Sep 12, 2024

View reviewed changes

NevilleS merged commit 47934f2 into main Sep 12, 2024
13 checks passed

NevilleS deleted the PROD-2763-fix-should-show-experience branch September 12, 2024 18:52

NevilleS added a commit that referenced this pull request Sep 12, 2024

Fix Fides.shouldShouldShowExperience() to return false for modal-only…

5384a1d

… experiences (#5281)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Fides.shouldShouldShowExperience() to return false for modal-only experiences #5281

Fix Fides.shouldShouldShowExperience() to return false for modal-only experiences #5281

NevilleS commented Sep 12, 2024 •

edited by lucanovera

Loading

vercel bot commented Sep 12, 2024 •

edited

Loading

cypress bot commented Sep 12, 2024 •

edited

Loading

NevilleS left a comment

NevilleS Sep 12, 2024

lucanovera Sep 12, 2024

NevilleS Sep 12, 2024

lucanovera Sep 12, 2024

NevilleS Sep 12, 2024

NevilleS Sep 12, 2024

NevilleS Sep 12, 2024

NevilleS Sep 12, 2024

NevilleS Sep 12, 2024

NevilleS Sep 12, 2024

lucanovera left a comment

lucanovera Sep 12, 2024

lucanovera Sep 12, 2024

cypress bot commented Sep 12, 2024

Fix Fides.shouldShouldShowExperience() to return false for modal-only experiences #5281

Fix Fides.shouldShouldShowExperience() to return false for modal-only experiences #5281

Conversation

NevilleS commented Sep 12, 2024 • edited by lucanovera Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

vercel bot commented Sep 12, 2024 • edited Loading

cypress bot commented Sep 12, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucanovera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Sep 12, 2024

NevilleS commented Sep 12, 2024 •

edited by lucanovera

Loading

vercel bot commented Sep 12, 2024 •

edited

Loading

cypress bot commented Sep 12, 2024 •

edited

Loading