Skip to content
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

Add support for hasUnmappedDestinations #905

Merged
merged 2 commits into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 19 additions & 0 deletions packages/core/src/__tests__/__helpers__/mockSegmentStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type {
DestinationFilters,
IntegrationSettings,
RoutingRule,
SegmentAPIConsentSettings,
SegmentAPIIntegrations,
UserInfoState,
} from '../../types';
Expand All @@ -22,6 +23,7 @@ export type StoreData = {
isReady: boolean;
context?: DeepPartial<Context>;
settings: SegmentAPIIntegrations;
consentSettings?: SegmentAPIConsentSettings;
filters: DestinationFilters;
userInfo: UserInfoState;
deepLinkData: DeepLinkData;
Expand All @@ -33,6 +35,7 @@ const INITIAL_VALUES: StoreData = {
settings: {
[SEGMENT_DESTINATION_KEY]: {},
},
consentSettings: undefined,
filters: {},
userInfo: {
anonymousId: 'anonymousId',
Expand Down Expand Up @@ -71,6 +74,9 @@ export class MockSegmentStore implements Storage {
private callbacks = {
context: createCallbackManager<DeepPartial<Context> | undefined>(),
settings: createCallbackManager<SegmentAPIIntegrations>(),
consentSettings: createCallbackManager<
SegmentAPIConsentSettings | undefined
>(),
filters: createCallbackManager<DestinationFilters>(),
userInfo: createCallbackManager<UserInfoState>(),
deepLinkData: createCallbackManager<DeepLinkData>(),
Expand Down Expand Up @@ -123,6 +129,19 @@ export class MockSegmentStore implements Storage {
},
};

readonly consentSettings: Watchable<SegmentAPIConsentSettings | undefined> &
Settable<SegmentAPIConsentSettings | undefined> = {
get: createMockStoreGetter(() => this.data.consentSettings),
onChange: (callback: (value?: SegmentAPIConsentSettings) => void) =>
this.callbacks.consentSettings.register(callback),
set: (value) => {
this.data.consentSettings =
value instanceof Function ? value(this.data.consentSettings) : value;
this.callbacks.consentSettings.run(this.data.consentSettings);
return this.data.consentSettings;
},
};

readonly filters: Watchable<DestinationFilters | undefined> &
Settable<DestinationFilters> &
Dictionary<string, RoutingRule, DestinationFilters> = {
Expand Down
13 changes: 13 additions & 0 deletions packages/core/src/analytics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
SegmentError,
translateHTTPError,
} from './errors';
import type { SegmentAPIConsentSettings } from '.';

type OnPluginAddedCallback = (plugin: Plugin) => void;

Expand Down Expand Up @@ -116,6 +117,11 @@
*/
readonly settings: Watchable<SegmentAPIIntegrations | undefined>;

/**
* Access or subscribe to integration settings
*/
readonly consentSettings: Watchable<SegmentAPIConsentSettings | undefined>;

/**
* Access or subscribe to destination filter settings
*/
Expand Down Expand Up @@ -198,6 +204,11 @@
onChange: this.store.settings.onChange,
};

this.consentSettings = {
get: this.store.consentSettings.get,
onChange: this.store.consentSettings.onChange,
};

this.filters = {
get: this.store.filters.get,
onChange: this.store.filters.onChange,
Expand Down Expand Up @@ -305,12 +316,14 @@
const resJson: SegmentAPISettings =
(await res.json()) as SegmentAPISettings;
const integrations = resJson.integrations;
const consentSettings = resJson.consentSettings;
const filters = this.generateFiltersMap(
resJson.middlewareSettings?.routingRules ?? []
);
this.logger.info(`Received settings from Segment succesfully.`);
await Promise.all([
this.store.settings.set(integrations),
this.store.consentSettings.set(consentSettings),
this.store.filters.set(filters),
]);
} catch (e) {
Expand Down Expand Up @@ -870,7 +883,7 @@
return {
anonymousId: userInfo.anonymousId,
userId: event.userId,
previousId: previousUserId!,

Check warning on line 886 in packages/core/src/analytics.ts

View workflow job for this annotation

GitHub Actions / build-and-test

Forbidden non-null assertion
};
}

Expand Down
24 changes: 18 additions & 6 deletions packages/core/src/plugins/ConsentPlugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,18 +80,20 @@ export class ConsentPlugin extends Plugin {
if (this.isDestinationPlugin(plugin)) {
plugin.add(
new ConsentFilterPlugin((event) => {
const allCategories =
this.analytics?.consentSettings.get()?.allCategories || [];
const settings = this.analytics?.settings.get() || {};
const preferences = event.context?.consent?.categoryPreferences || {};

if (plugin.key === SEGMENT_DESTINATION_KEY) {
const noneConsented = allCategories.every(
(category) => !preferences[category]
);

return (
this.isConsentUpdateEvent(event) ||
!(
Object.values(preferences).every((consented) => !consented) &&
Object.entries(settings)
.filter(([k]) => k !== SEGMENT_DESTINATION_KEY)
.every(([_, v]) => this.containsConsentSettings(v))
)
!this.isConsentFeatureSetup() ||
!(noneConsented && !this.hasUnmappedDestinations())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@silesky @oscb I'm tunnel visioned, can you folks help me simplify line 94? I'm sure there's a way to not have these nested negations, just can't figure how??

Note that if expression on line 91 resolves to true, then that means event is allowed to flow, if false it is blocked.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the isConsentFeatureSetup is an interesting one to have here. Cause potentially it might throw a "false" positive and let all events flow if the settings object is not loaded (f.e. you have bad connection). I guess this is more of an scenario where we should let the client control what they want to do in that scenario. Not sure really if this has been talked in Consent Management before. Anyway. We don't have to solve this yet.

I think the line !(noneConsented && !this.hasUnmappedDestinations()) could be rewritten as !noneConsented || !this.hasUnmappedDestinations(). If I'm understanding correctly the spec you want to let things flow if everything is consented or when there's at least an unmapped destination.

Copy link
Contributor

Choose a reason for hiding this comment

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

another idea to avoid many negations which make it harder to understand is to swap noneConsented to allConsented so that you don't have to negate.

);
}

Expand Down Expand Up @@ -127,6 +129,16 @@ export class ConsentPlugin extends Plugin {
private isConsentUpdateEvent(event: SegmentEvent): boolean {
return (event as TrackEventType).event === CONSENT_PREF_UPDATE_EVENT;
}

private hasUnmappedDestinations(): boolean {
return (
this.analytics?.consentSettings.get()?.hasUnmappedDestinations === true
);
}

private isConsentFeatureSetup(): boolean {
return typeof this.analytics?.consentSettings.get() === 'object';
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ describe('Destinations multiple categories', () => {
createTestClient(
{
settings: destinationsMultipleCategories.integrations,
consentSettings: destinationsMultipleCategories.consentSettings,
},
{ autoAddSegmentDestination: true }
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,3 @@
"legacyVideoPluginsEnabled": false,
"remotePlugins": []
}

Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@
"legacyVideoPluginsEnabled": false,
"remotePlugins": [],
"consentSettings": {
"hasUnmappedDestinations": false,
"allCategories": [
"C0001",
"C0002"
]
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@
"legacyVideoPluginsEnabled": false,
"remotePlugins": [],
"consentSettings": {
"hasUnmappedDestinations": false,
"allCategories": [
"C0001",
"C0002",
Expand All @@ -102,4 +103,3 @@
]
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
"legacyVideoPluginsEnabled": false,
"remotePlugins": [],
"consentSettings": {
"hasUnmappedDestinations": true,
"allCategories": [
"C0001",
"C0002",
Expand All @@ -98,4 +99,3 @@
]
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ describe('No unmapped destinations', () => {
const createClient = () =>
createTestClient({
settings: noUnmappedDestinations.integrations,
consentSettings: noUnmappedDestinations.consentSettings,
});

test('no to all', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ describe('Unmapped destinations', () => {
createTestClient(
{
settings: unmappedDestinations.integrations,
consentSettings: unmappedDestinations.consentSettings,
},
{ autoAddSegmentDestination: true }
);
Expand Down
47 changes: 47 additions & 0 deletions packages/core/src/storage/sovranStorage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {
UserInfoState,
RoutingRule,
DestinationFilters,
SegmentAPIConsentSettings,
} from '..';
import { getUUID } from '../uuid';
import { createGetter } from './helpers';
Expand All @@ -34,6 +35,7 @@ type Data = {
eventsToRetry: SegmentEvent[];
context: DeepPartial<Context>;
settings: SegmentAPIIntegrations;
consentSettings: SegmentAPIConsentSettings | undefined;
userInfo: UserInfoState;
filters: DestinationFilters;
};
Expand All @@ -43,6 +45,7 @@ const INITIAL_VALUES: Data = {
eventsToRetry: [],
context: {},
settings: {},
consentSettings: undefined,
filters: {},
userInfo: {
anonymousId: getUUID(),
Expand Down Expand Up @@ -141,6 +144,9 @@ export class SovranStorage implements Storage {
private storePersistorSaveDelay?: number;
private readinessStore: Store<ReadinessStore>;
private contextStore: Store<{ context: DeepPartial<Context> }>;
private consentSettingsStore: Store<{
consentSettings: SegmentAPIConsentSettings | undefined;
}>;
private settingsStore: Store<{ settings: SegmentAPIIntegrations }>;
private userInfoStore: Store<{ userInfo: UserInfoState }>;
private deepLinkStore: Store<DeepLinkData> = deepLinkStore;
Expand All @@ -155,6 +161,9 @@ export class SovranStorage implements Storage {
Settable<SegmentAPIIntegrations> &
Dictionary<string, IntegrationSettings, SegmentAPIIntegrations>;

readonly consentSettings: Watchable<SegmentAPIConsentSettings | undefined> &
Settable<SegmentAPIConsentSettings | undefined>;

readonly filters: Watchable<DestinationFilters | undefined> &
Settable<DestinationFilters> &
Dictionary<string, RoutingRule, DestinationFilters>;
Expand Down Expand Up @@ -271,6 +280,44 @@ export class SovranStorage implements Storage {
},
};

// Consent settings

this.consentSettingsStore = createStore(
{ consentSettings: INITIAL_VALUES.consentSettings },
{
persist: {
storeId: `${this.storeId}-consentSettings`,
persistor: this.storePersistor,
saveDelay: this.storePersistorSaveDelay,
onInitialized: markAsReadyGenerator('hasRestoredSettings'),
},
}
);

this.consentSettings = {
get: createStoreGetter(this.consentSettingsStore, 'consentSettings'),
onChange: (
callback: (value?: SegmentAPIConsentSettings | undefined) => void
) =>
this.consentSettingsStore.subscribe((store) =>
callback(store.consentSettings)
),
set: async (value) => {
const { consentSettings } = await this.consentSettingsStore.dispatch(
(state) => {
let newState: typeof state.consentSettings;
if (value instanceof Function) {
newState = value(state.consentSettings);
} else {
newState = Object.assign({}, state.consentSettings, value);
}
return { consentSettings: newState };
}
);
return consentSettings;
},
};

// Filters

this.filtersStore = createStore(INITIAL_VALUES.filters, {
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/storage/types.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { Unsubscribe, Persistor } from '@segment/sovran-react-native';
import type { SegmentAPIConsentSettings } from '..';
import type {
Context,
DeepPartial,
Expand Down Expand Up @@ -71,6 +72,9 @@ export interface Storage {
Settable<SegmentAPIIntegrations> &
Dictionary<string, IntegrationSettings, SegmentAPIIntegrations>;

readonly consentSettings: Watchable<SegmentAPIConsentSettings | undefined> &
Settable<SegmentAPIConsentSettings | undefined>;

readonly filters: Watchable<DestinationFilters | undefined> &
Settable<DestinationFilters> &
Dictionary<string, RoutingRule, DestinationFilters>;
Expand Down
6 changes: 6 additions & 0 deletions packages/core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,11 @@ export type SegmentAPIIntegrations = {
[key: string]: IntegrationSettings;
};

export type SegmentAPIConsentSettings = {
allCategories: string[];
hasUnmappedDestinations: boolean;
};

export type RoutingRule = Rule;

export interface MetricsOptions {
Expand All @@ -309,6 +314,7 @@ export type SegmentAPISettings = {
routingRules: RoutingRule[];
};
metrics?: MetricsOptions;
consentSettings?: SegmentAPIConsentSettings;
};

export type DestinationMetadata = {
Expand Down
Loading