Skip to content

Treat non-applicable keys as defaults which then get overridden #6090

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.

Sign up for GitHub

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

Merged
gilluminate merged 4 commits into from
Apr 25, 2025
Merged

Treat non-applicable keys as defaults which then get overridden #6090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f75b148
Fix inacurate vendor count
gilluminate Apr 24, 2025
e2c11f8
Treat non-applicable as defaults then override
gilluminate Apr 24, 2025
a551fd9
Update CHANGELOG.md
gilluminate Apr 24, 2025
ff4b8f7
refactor to extract consent composition
gilluminate Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ Changes can also be flagged with a GitHub label for tracking purposes. The URL o
- Cleaning up test fixtures [#6008](https://github.com/ethyca/fides/pull/6008)

### Fixed
- Fixed GTM integration to properly handle duplicate notice keys [#6090](https://github.com/ethyca/fides/pull/6090)
- Fix Special-purpose only vendors not correctly encoded in TC string [#6086](https://github.com/ethyca/fides/pull/6086)

## [2.60.0](https://github.com/ethyca/fides/compare/2.59.2...2.60.0)
Expand Down
19 changes: 15 additions & 4 deletions clients/admin-ui/src/features/privacy-experience/preview/Preview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import Script from "next/script";
import React, { useEffect, useMemo, useState } from "react";

import { PREVIEW_CONTAINER_ID } from "~/constants";
import { useGetVendorReportQuery } from "~/features/plus/plus.slice";
import { TranslationWithLanguageName } from "~/features/privacy-experience/form/helpers";
import {
buildBaseConfig,
Expand All @@ -21,7 +22,6 @@ import {
PrivacyNoticeResponse,
} from "~/types/api";

import { useFeatures } from "../../common/features";
import { COMPONENT_MAP } from "../constants";

declare global {
Expand Down Expand Up @@ -83,7 +83,18 @@ const Preview = ({
ComponentType.TCF_OVERLAY,
].includes(values.component);

const { systemsCount } = useFeatures();
/**
* Selects the number of vendors
* By using the paginated getVendorReport endpoint, we can get the total number of vendors
*/
const { data: vendorReport } = useGetVendorReportQuery({
pageIndex: 1,
pageSize: 1,
});
const vendorCount = useMemo(
() => vendorReport?.total || 0,
[vendorReport?.total],
);

useEffect(() => {
if (
Expand Down Expand Up @@ -192,7 +203,7 @@ const Preview = ({
? values.layer1_button_options
: Layer1ButtonOption.OPT_IN_OPT_OUT;
updatedConfig.options.preventDismissal = !values.dismissable;
updatedConfig.experience.vendor_count = systemsCount;
updatedConfig.experience.vendor_count = vendorCount;
updatedConfig.experience.experience_config.component = values.component;
// reinitialize fides.js each time the form changes
window.Fides.init(updatedConfig);
Expand All @@ -207,7 +218,7 @@ const Preview = ({
baseConfig,
allPrivacyNotices,
isPreviewAvailable,
systemsCount,
vendorCount,
fidesScriptLoaded,
previewMode,
values.privacy_notice_ids,
Expand Down
50 changes: 37 additions & 13 deletions clients/fides-js/src/integrations/gtm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,33 @@ export interface GtmOptions {
flag_type?: GtmFlagType;
}

/**
* Composes consent values based on the consent mechanism and flag type
*/
const composeConsent = (
consent: Record<string, boolean>,
privacyNotices: any[] | undefined,
flagType: GtmFlagType,
): Record<string, boolean | string> => {
const consentValues: Record<string, boolean | string> = {};

Object.entries(consent).forEach(([key, value]) => {
if (privacyNotices && flagType === GtmFlagType.CONSENT_MECHANISM) {
const relevantNotice = privacyNotices.find(
(notice) => notice.notice_key === key,
);
consentValues[key] = transformConsentToFidesUserPreference(
value,
relevantNotice?.consent_mechanism,
);
} else {
consentValues[key] = value;
}
});

return consentValues;
};

// Helper function to push the Fides variable to the GTM data layer from a FidesEvent
const pushFidesVariableToGTM = (
fidesEvent: {
Expand All @@ -51,23 +78,12 @@ const pushFidesVariableToGTM = (
nonApplicableFlagMode = GtmNonApplicableFlagMode.OMIT,
flag_type: flagType = GtmFlagType.BOOLEAN,
} = options ?? {};
const consentValues: FidesVariable["consent"] = JSON.parse(
JSON.stringify(consent),
);
const consentValues: FidesVariable["consent"] = {};
const privacyNotices = window.Fides?.experience?.privacy_notices;
const nonApplicablePrivacyNotices =
window.Fides?.experience?.non_applicable_privacy_notices;

if (privacyNotices && flagType === GtmFlagType.CONSENT_MECHANISM) {
Object.entries(consent).forEach(([key, value]) => {
consentValues[key] = transformConsentToFidesUserPreference(
value,
privacyNotices.find((notice) => notice.notice_key === key)
?.consent_mechanism,
);
});
}

// First set defaults for non-applicable privacy notices if needed
if (
nonApplicableFlagMode === GtmNonApplicableFlagMode.INCLUDE &&
nonApplicablePrivacyNotices
Expand All @@ -78,6 +94,14 @@ const pushFidesVariableToGTM = (
});
}

// Then override with actual consent values
if (consent) {
Copy link
Copy Markdown
Contributor

@tvandort tvandort Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit but I feel like we'd be better served by reducing the overall cyclomatic complexity of this code.

Instead of wrapping this in an if statement we could:

const nextConsent = consent ?? {}
if (privacyNotices && flagType === GtmFlagType.CONSENT_MECHANISM) {
  Object.entries(nextConsent).forEach(([key, value]) => {
    consentValues[key] = transformConsentToFidesUserPreference(
      value,
      privacyNotices.find((notice) => notice.notice_key === key)
        ?.consent_mechanism,
    );
  });
} else {
  Object.entries(nextConsent).forEach(([key, value]) => {
    consentValues[key] = value;
  });
}

Copy link
Copy Markdown
Contributor Author

@gilluminate gilluminate Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there's no consent, it doesn't do anything else, which I think is less cyclomatic, no?

Copy link
Copy Markdown
Contributor Author

@gilluminate gilluminate Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, I'm betting the rollup algorithm is going to make this nice and clean anyway, and I'd favor readability in the source code.

Copy link
Copy Markdown
Contributor

@tvandort tvandort Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find less if statements (well... really less execution branches) more readable personally. :)

Something that always happens is easier to reason about than something that sometimes happens.

if there's no consent, it doesn't do anything else, which I think is less cyclomatic, no?

No I don't believe so. At least to my understand cyclomatic complexity goes up with mutually exclusive execution paths in the code. If a path always executes it's less complex than one that does.

Let's take a simpler example case:

function trim(str) {
	if (str) {
		return str.trim();
	}
	
	return "";
}

This code has two paths, one where a another function, .trim() will be executed and one where it will not. CC of 2.

function trim(str) {
	var trimmedStr = str ?? "";
	return trimmedStr.trim();
}

This code has one path, .trim() is always called. Therefore (assuming I understand correctly) it has a lower cyclomatic complexity. CC of 1.

Object.assign(
consentValues,
composeConsent(consent, privacyNotices, flagType),
);
}

// Construct the Fides variable that will be pushed to GTM
const Fides: FidesVariable = {
consent: consentValues,
Expand Down