From aed7e2694e32d3f8cb991a6a79a841111921dd6e Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Fri, 2 Aug 2024 18:04:47 +0200 Subject: [PATCH 01/16] update unit test to match expectation Test do not pass yet on purpose We have to validate the test use case logic and then update implementation --- app/core/Analytics/MetaMetrics.test.ts | 154 ++++++++++++++++++------- 1 file changed, 111 insertions(+), 43 deletions(-) diff --git a/app/core/Analytics/MetaMetrics.test.ts b/app/core/Analytics/MetaMetrics.test.ts index 0420b3837f2..6ecf02ff416 100644 --- a/app/core/Analytics/MetaMetrics.test.ts +++ b/app/core/Analytics/MetaMetrics.test.ts @@ -161,7 +161,7 @@ describe('MetaMetrics', () => { }); }); - it('does not track event when diabled', async () => { + it('does not track event when disabled', async () => { const metaMetrics = TestMetaMetrics.getInstance(); expect(await metaMetrics.configure()).toBeTruthy(); const event: IMetaMetricsEvent = { category: 'event1' }; @@ -176,80 +176,148 @@ describe('MetaMetrics', () => { expect(segmentMockClient.track).not.toHaveBeenCalled(); }); - it('tracks anonymous event', async () => { + it('tracks event without updating dataRecorded status', async () => { const metaMetrics = TestMetaMetrics.getInstance(); + expect(await metaMetrics.configure()).toBeTruthy(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { category: 'event1' }; const properties = { prop1: 'value1' }; - metaMetrics.trackAnonymousEvent(event, properties); + metaMetrics.trackEvent(event, properties, false); + expect(StorageWrapper.getItem).toHaveBeenCalledWith(METRICS_OPT_IN); // TODO: Replace "any" with type // eslint-disable-next-line @typescript-eslint/no-explicit-any const { segmentMockClient } = global as any; - // the anonymous part should not have a user id expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: true, + anonymous: false, ...properties, }); - // non anonymous part should not have properties - expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: true, - }); + expect(metaMetrics.isDataRecorded()).toBeFalsy(); }); - it('tracks anonymous event without param', async () => { + it('tracks non-anonymous event when some props are anonymous', async () => { const metaMetrics = TestMetaMetrics.getInstance(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { category: 'event1' }; + const nonAnonProp = { non_anon_prop: 'value1' }; + const someAnonProps = { + anon_prop: { anonymous: true, value: 'anonValue2' }, + }; + const properties = { ...nonAnonProp, ...someAnonProps }; - metaMetrics.trackAnonymousEvent(event); + metaMetrics.trackEvent(event, properties); // TODO: Replace "any" with type // eslint-disable-next-line @typescript-eslint/no-explicit-any const { segmentMockClient } = global as any; - // the anonymous part should not have a user id - expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: true, - ...{}, - }); - // non anonymous part should not have properties + + // Only one non-anonymous event should be tracked + expect(segmentMockClient.track).toHaveBeenCalledTimes(1); + + // When tracking is not anonymous, event should have both non-anonymous and anonymous properties. + // This makes no sense and should be fixed in the code as I believe no one should send anonymous properties + // in a non-anonymous event. But in case it happens, we should track it this way. expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: true, + anonymous: false, + ...someAnonProps, + ...nonAnonProp, }); }); - it('does not track anonymous event if disabled', async () => { - const metaMetrics = TestMetaMetrics.getInstance(); - const event: IMetaMetricsEvent = { category: 'event1' }; - const properties = { prop1: 'value1' }; + describe('anonymous event', () => { + it('tracks two events when all props are non anonymous', async () => { + const metaMetrics = TestMetaMetrics.getInstance(); + await metaMetrics.enable(); + const event: IMetaMetricsEvent = { category: 'event1' }; + const properties = { prop1: 'value1' }; - metaMetrics.trackAnonymousEvent(event, properties); + metaMetrics.trackAnonymousEvent(event, properties); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; - expect(segmentMockClient.track).not.toHaveBeenCalled(); - }); + // TODO: Replace "any" with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const { segmentMockClient } = global as any; - it('tracks event without updating dataRecorded status', async () => { - const metaMetrics = TestMetaMetrics.getInstance(); - expect(await metaMetrics.configure()).toBeTruthy(); - await metaMetrics.enable(); - const event: IMetaMetricsEvent = { category: 'event1' }; - const properties = { prop1: 'value1' }; + // anonymous event has all the properties + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: true, + ...properties, + }); - metaMetrics.trackEvent(event, properties, false); + // non-anonymous event has no properties + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: false, + }); + }); - expect(StorageWrapper.getItem).toHaveBeenCalledWith(METRICS_OPT_IN); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; - expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: false, - ...properties, + it('tracks two empty events when no props', async () => { + const metaMetrics = TestMetaMetrics.getInstance(); + await metaMetrics.enable(); + const event: IMetaMetricsEvent = { category: 'event1' }; + + metaMetrics.trackAnonymousEvent(event); + + // TODO: Replace "any" with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const { segmentMockClient } = global as any; + + // anonymous event has no properties + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: true, + }); + + // non-anonymous event has no properties + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: false, + }); + }); + + it('tracks two events when some props are anonymous', async () => { + const metaMetrics = TestMetaMetrics.getInstance(); + await metaMetrics.enable(); + const event: IMetaMetricsEvent = { category: 'event1' }; + const nonAnonProp = { non_anon_prop: 'value1' }; + const anonProp = { + anon_prop: { anonymous: true, value: 'anonValue2' }, + }; + const properties = { ...nonAnonProp, ...anonProp }; + + metaMetrics.trackAnonymousEvent(event, properties); + + // TODO: Replace "any" with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const { segmentMockClient } = global as any; + + // non-anonymous event only has the non-anonymous properties. + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: false, + ...nonAnonProp, + }); + + // anonymous event has all properties + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: true, + ...anonProp, + ...nonAnonProp, + }); + + // Only two events should be tracked, one anonymous and one non-anonymous + expect(segmentMockClient.track).toHaveBeenCalledTimes(2); + }); + + it('does not track any event if disabled', async () => { + const metaMetrics = TestMetaMetrics.getInstance(); + const event: IMetaMetricsEvent = { category: 'event1' }; + const properties = { prop1: 'value1' }; + + metaMetrics.trackAnonymousEvent(event, properties); + + // TODO: Replace "any" with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const { segmentMockClient } = global as any; + + expect(segmentMockClient.track).not.toHaveBeenCalled(); }); - expect(metaMetrics.isDataRecorded()).toBeFalsy(); }); describe('Legacy events', () => { From 77733325b0c95c540e8533bb7f5daab3612fdd40 Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Fri, 16 Aug 2024 14:48:09 +0200 Subject: [PATCH 02/16] fix client types --- app/core/Analytics/MetaMetrics.test.ts | 128 +++++++++++++------------ 1 file changed, 67 insertions(+), 61 deletions(-) diff --git a/app/core/Analytics/MetaMetrics.test.ts b/app/core/Analytics/MetaMetrics.test.ts index 6ecf02ff416..73dda328254 100644 --- a/app/core/Analytics/MetaMetrics.test.ts +++ b/app/core/Analytics/MetaMetrics.test.ts @@ -14,6 +14,7 @@ import { DataDeleteResponseStatus, DataDeleteStatus, IMetaMetricsEvent, + ISegmentClient, } from './MetaMetrics.types'; jest.mock('../../store/storage-wrapper'); @@ -32,6 +33,10 @@ class TestMetaMetrics extends MetaMetrics { } } +interface GlobalWithSegmentClient { + segmentMockClient: ISegmentClient; +} + describe('MetaMetrics', () => { beforeEach(async () => { StorageWrapper.getItem = mockGet; @@ -124,19 +129,19 @@ describe('MetaMetrics', () => { }); describe('Tracking', () => { - it('tracks event', async () => { + it('tracks regular event', async () => { const metaMetrics = TestMetaMetrics.getInstance(); expect(await metaMetrics.configure()).toBeTruthy(); await metaMetrics.enable(); - const event: IMetaMetricsEvent = { category: 'event1' }; - const properties = { prop1: 'value1' }; + const event: IMetaMetricsEvent = { category: 'test event' }; + const properties = { regular_prop: 'test value' }; metaMetrics.trackEvent(event, properties); + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + expect(StorageWrapper.getItem).toHaveBeenCalledWith(METRICS_OPT_IN); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: false, ...properties, @@ -147,14 +152,14 @@ describe('MetaMetrics', () => { const metaMetrics = TestMetaMetrics.getInstance(); expect(await metaMetrics.configure()).toBeTruthy(); await metaMetrics.enable(); - const event: IMetaMetricsEvent = { category: 'event1' }; + const event: IMetaMetricsEvent = { category: 'test event' }; metaMetrics.trackEvent(event); + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + expect(StorageWrapper.getItem).toHaveBeenCalledWith(METRICS_OPT_IN); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: false, ...{}, @@ -169,10 +174,10 @@ describe('MetaMetrics', () => { metaMetrics.trackEvent(event, properties); + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + expect(StorageWrapper.getItem).toHaveBeenCalledWith(METRICS_OPT_IN); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; expect(segmentMockClient.track).not.toHaveBeenCalled(); }); @@ -185,10 +190,10 @@ describe('MetaMetrics', () => { metaMetrics.trackEvent(event, properties, false); + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + expect(StorageWrapper.getItem).toHaveBeenCalledWith(METRICS_OPT_IN); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: false, ...properties, @@ -208,9 +213,8 @@ describe('MetaMetrics', () => { metaMetrics.trackEvent(event, properties); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; // Only one non-anonymous event should be tracked expect(segmentMockClient.track).toHaveBeenCalledTimes(1); @@ -234,9 +238,8 @@ describe('MetaMetrics', () => { metaMetrics.trackAnonymousEvent(event, properties); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; // anonymous event has all the properties expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { @@ -257,9 +260,8 @@ describe('MetaMetrics', () => { metaMetrics.trackAnonymousEvent(event); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; // anonymous event has no properties expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { @@ -284,9 +286,8 @@ describe('MetaMetrics', () => { metaMetrics.trackAnonymousEvent(event, properties); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; // non-anonymous event only has the non-anonymous properties. expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { @@ -312,9 +313,8 @@ describe('MetaMetrics', () => { metaMetrics.trackAnonymousEvent(event, properties); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; expect(segmentMockClient.track).not.toHaveBeenCalled(); }); @@ -332,9 +332,9 @@ describe('MetaMetrics', () => { metaMetrics.trackEvent(event); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: false, ...event.properties, @@ -353,9 +353,9 @@ describe('MetaMetrics', () => { metaMetrics.trackEvent(event, properties); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: false, ...properties, @@ -373,9 +373,9 @@ describe('MetaMetrics', () => { // @ts-expect-error: Testing untyped legacy JS call with undefined event metaMetrics.trackEvent(event); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + expect(segmentMockClient.track).toHaveBeenCalledWith(undefined, { anonymous: false, undefined, @@ -391,9 +391,10 @@ describe('MetaMetrics', () => { const groupId = 'group1'; const groupTraits = { trait1: 'value1' }; metaMetrics.group(groupId, groupTraits); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; + + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + expect(segmentMockClient.group).toHaveBeenCalledWith( groupId, groupTraits, @@ -405,9 +406,10 @@ describe('MetaMetrics', () => { const groupId = 'group1'; const groupTraits = { trait1: 'value1' }; metaMetrics.group(groupId, groupTraits); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; + + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + expect(segmentMockClient.group).not.toHaveBeenCalled(); }); }); @@ -419,9 +421,10 @@ describe('MetaMetrics', () => { await metaMetrics.enable(); const userTraits = { trait1: 'value1' }; await metaMetrics.addTraitsToUser(userTraits); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; + + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + expect(segmentMockClient.identify).toHaveBeenCalledWith( expect.any(String), userTraits, @@ -432,9 +435,10 @@ describe('MetaMetrics', () => { const metaMetrics = TestMetaMetrics.getInstance(); const userTraits = { trait1: 'value1' }; await metaMetrics.addTraitsToUser(userTraits); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; + + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + expect(segmentMockClient.identify).not.toHaveBeenCalled(); }); }); @@ -443,9 +447,10 @@ describe('MetaMetrics', () => { it('resets', async () => { const metaMetrics = TestMetaMetrics.getInstance(); await metaMetrics.reset(); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; + + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + expect(segmentMockClient.reset).toHaveBeenCalledWith(true); expect(StorageWrapper.setItem).toHaveBeenCalledWith(METAMETRICS_ID, ''); }); @@ -453,9 +458,10 @@ describe('MetaMetrics', () => { it('flushes the segment client', async () => { const metaMetrics = TestMetaMetrics.getInstance(); await metaMetrics.flush(); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; + + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + expect(segmentMockClient.flush).toHaveBeenCalled(); }); }); @@ -552,10 +558,10 @@ describe('MetaMetrics', () => { '', ); + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + // Check MetaMerics class calls the Segment SDK reset - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { segmentMockClient } = global as any; expect(segmentMockClient.reset).toHaveBeenCalledTimes(1); expect(segmentMockClient.reset).toHaveBeenCalledWith(true); From 576ffa7a945014445ffa240df9094336415f59bc Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Fri, 16 Aug 2024 18:10:45 +0200 Subject: [PATCH 03/16] Rework tests --- app/core/Analytics/MetaMetrics.test.ts | 299 +++++++++++++++++-------- 1 file changed, 205 insertions(+), 94 deletions(-) diff --git a/app/core/Analytics/MetaMetrics.test.ts b/app/core/Analytics/MetaMetrics.test.ts index 73dda328254..842793c8373 100644 --- a/app/core/Analytics/MetaMetrics.test.ts +++ b/app/core/Analytics/MetaMetrics.test.ts @@ -126,13 +126,10 @@ describe('MetaMetrics', () => { ); expect(metaMetrics.isEnabled()).toBeFalsy(); }); - }); - describe('Tracking', () => { - it('tracks regular event', async () => { + it('does not track event when disabled', async () => { const metaMetrics = TestMetaMetrics.getInstance(); expect(await metaMetrics.configure()).toBeTruthy(); - await metaMetrics.enable(); const event: IMetaMetricsEvent = { category: 'test event' }; const properties = { regular_prop: 'test value' }; @@ -141,14 +138,15 @@ describe('MetaMetrics', () => { const { segmentMockClient } = global as unknown as GlobalWithSegmentClient; + expect(StorageWrapper.setItem).not.toHaveBeenCalledWith( + METRICS_OPT_IN, + AGREED, + ); expect(StorageWrapper.getItem).toHaveBeenCalledWith(METRICS_OPT_IN); - expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: false, - ...properties, - }); + expect(segmentMockClient.track).not.toHaveBeenCalled(); }); - it('tracks event without param', async () => { + it('tracks event when enabled', async () => { const metaMetrics = TestMetaMetrics.getInstance(); expect(await metaMetrics.configure()).toBeTruthy(); await metaMetrics.enable(); @@ -159,132 +157,230 @@ describe('MetaMetrics', () => { const { segmentMockClient } = global as unknown as GlobalWithSegmentClient; + // check tracking enabling + expect(StorageWrapper.setItem).toHaveBeenCalledWith( + METRICS_OPT_IN, + AGREED, + ); expect(StorageWrapper.getItem).toHaveBeenCalledWith(METRICS_OPT_IN); - expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: false, - ...{}, - }); + + // check that the tracking was called + expect(segmentMockClient.track).toHaveBeenCalledTimes(1); }); + }); - it('does not track event when disabled', async () => { - const metaMetrics = TestMetaMetrics.getInstance(); - expect(await metaMetrics.configure()).toBeTruthy(); - const event: IMetaMetricsEvent = { category: 'event1' }; - const properties = { prop1: 'value1' }; + describe('Tracking', () => { + /* This is the matrix of tracking use cases. + * How to read it? Here are some examples: + * - Test NA1 means non-anonymous tracking (NA) but it has no props at all: + * The result must be only one non-anonymous event without any props (EMPTY) and no anonymous event at all (NONE). + * - Test A4 means anonymous tracking (A) and it has both non anonymous and anonymous props: + * The result must be a non-anonymous event with non-anonymous props (NA PROPS) and an anonymous event with all props (NA PROPS + A PROPS). + * + * | Test | Track | Non-anon prop | Anon prop | Result non-anon (NA) event | Result anon (A) event | + * |--------|---------|---------------|-----------|----------------------------|-----------------------| + * | NA1 | NA | NO | NO | EMPTY | NONE | + * | A1 | A | NO | NO | EMPTY | NONE | + * | NA2 | NA | YES | NO | NA PROPS | NONE | + * | A2 | A | YES | NO | EMPTY | NA PROPS | + * | NA3 | NA | NO | YES | EMPTY | A PROPS | + * | A3 | A | NO | YES | EMPTY | A PROPS | + * | NA4 | NA | YES | YES | NA PROPS | NA PROPS + A PROPS | + * | A4 | A | YES | YES | NA PROPS | NA PROPS + A PROPS | + * + * the following test cases include the code (NAX or AX) of the test in the table for reference. + * + * NOTE: some NAX and AX test cases (all except NA2/A2) have similar result but differ only by calling the + * anonymous (trackAnonymousEvent) or non-anonymous tracking (trackEvent). + * They are grouped together here and both implementations uses trackEvent under the hood. + * Ultimately these duplicates will disappear when we deprecate trackAnonymousEvent. + * But that's another issue to address. + */ + describe('tracks non-anonymous event (NA)', () => { + it('without properties (NA1)', async () => { + const metaMetrics = TestMetaMetrics.getInstance(); + expect(await metaMetrics.configure()).toBeTruthy(); + await metaMetrics.enable(); + const event: IMetaMetricsEvent = { category: 'test event' }; - metaMetrics.trackEvent(event, properties); + metaMetrics.trackEvent(event); - const { segmentMockClient } = - global as unknown as GlobalWithSegmentClient; + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; - expect(StorageWrapper.getItem).toHaveBeenCalledWith(METRICS_OPT_IN); - expect(segmentMockClient.track).not.toHaveBeenCalled(); - }); + // check if the event was tracked + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: false, + ...{}, + }); + expect(segmentMockClient.track).toHaveBeenCalledTimes(1); + }); - it('tracks event without updating dataRecorded status', async () => { - const metaMetrics = TestMetaMetrics.getInstance(); - expect(await metaMetrics.configure()).toBeTruthy(); - await metaMetrics.enable(); - const event: IMetaMetricsEvent = { category: 'event1' }; - const properties = { prop1: 'value1' }; + it('with non-anonymous properties (NA2)', async () => { + const metaMetrics = TestMetaMetrics.getInstance(); + expect(await metaMetrics.configure()).toBeTruthy(); + await metaMetrics.enable(); + const event: IMetaMetricsEvent = { category: 'test event' }; + const nonAnonProp = { non_anon_prop: 'test value' }; - metaMetrics.trackEvent(event, properties, false); + metaMetrics.trackEvent(event, nonAnonProp); - const { segmentMockClient } = - global as unknown as GlobalWithSegmentClient; + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; - expect(StorageWrapper.getItem).toHaveBeenCalledWith(METRICS_OPT_IN); - expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: false, - ...properties, + // check if the event was tracked + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: false, + ...nonAnonProp, + }); + expect(segmentMockClient.track).toHaveBeenCalledTimes(1); }); - expect(metaMetrics.isDataRecorded()).toBeFalsy(); - }); - it('tracks non-anonymous event when some props are anonymous', async () => { - const metaMetrics = TestMetaMetrics.getInstance(); - await metaMetrics.enable(); - const event: IMetaMetricsEvent = { category: 'event1' }; - const nonAnonProp = { non_anon_prop: 'value1' }; - const someAnonProps = { - anon_prop: { anonymous: true, value: 'anonValue2' }, - }; - const properties = { ...nonAnonProp, ...someAnonProps }; + it('with only anonymous properties (NA3)', async () => { + const metaMetrics = TestMetaMetrics.getInstance(); + await metaMetrics.enable(); + const event: IMetaMetricsEvent = { category: 'test event' }; + const anonProperties = { + anon_property: { anonymous: true, value: 'anon value' }, + }; - metaMetrics.trackEvent(event, properties); + metaMetrics.trackEvent(event, anonProperties); - const { segmentMockClient } = - global as unknown as GlobalWithSegmentClient; + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; - // Only one non-anonymous event should be tracked - expect(segmentMockClient.track).toHaveBeenCalledTimes(1); + // check if the event was tracked + // non-anonymous event has no properties. + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: false, + ...{}, + }); - // When tracking is not anonymous, event should have both non-anonymous and anonymous properties. - // This makes no sense and should be fixed in the code as I believe no one should send anonymous properties - // in a non-anonymous event. But in case it happens, we should track it this way. - expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: false, - ...someAnonProps, - ...nonAnonProp, + // anonymous event has anon properties + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: true, + ...anonProperties, + }); + + // two events should be tracked, one anonymous and one non-anonymous + expect(segmentMockClient.track).toHaveBeenCalledTimes(2); }); - }); - describe('anonymous event', () => { - it('tracks two events when all props are non anonymous', async () => { + it('with anonymous and non-anonymous properties (NA4)', async () => { const metaMetrics = TestMetaMetrics.getInstance(); await metaMetrics.enable(); - const event: IMetaMetricsEvent = { category: 'event1' }; - const properties = { prop1: 'value1' }; + const event: IMetaMetricsEvent = { category: 'test event' }; + const nonAnonProperties = { non_anon_prop: 'non anon value' }; + const anonProperties = { + anon_prop: { anonymous: true, value: 'anon value' }, + }; + const properties = { ...nonAnonProperties, ...anonProperties }; - metaMetrics.trackAnonymousEvent(event, properties); + metaMetrics.trackEvent(event, properties); const { segmentMockClient } = global as unknown as GlobalWithSegmentClient; - // anonymous event has all the properties + // non-anonymous event only has the non-anonymous properties. + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: false, + ...nonAnonProperties, + }); + + // anonymous event has all properties expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: true, - ...properties, + ...nonAnonProperties, + ...anonProperties, }); - // non-anonymous event has no properties + // Only two events should be tracked, one anonymous and one non-anonymous + expect(segmentMockClient.track).toHaveBeenCalledTimes(2); + }); + }); + + describe('tracks anonymous event (A)', () => { + it('without properties (A1)', async () => { + const metaMetrics = TestMetaMetrics.getInstance(); + expect(await metaMetrics.configure()).toBeTruthy(); + await metaMetrics.enable(); + const event: IMetaMetricsEvent = { category: 'test event' }; + + metaMetrics.trackAnonymousEvent(event); + + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: false, + ...{}, }); + expect(segmentMockClient.track).toHaveBeenCalledTimes(1); }); - it('tracks two empty events when no props', async () => { + it('with non-anonymous properties (A2)', async () => { const metaMetrics = TestMetaMetrics.getInstance(); + expect(await metaMetrics.configure()).toBeTruthy(); await metaMetrics.enable(); - const event: IMetaMetricsEvent = { category: 'event1' }; + const event: IMetaMetricsEvent = { category: 'test event' }; + const nonAnonProp = { non_anon_prop: 'test value' }; - metaMetrics.trackAnonymousEvent(event); + metaMetrics.trackAnonymousEvent(event, nonAnonProp); const { segmentMockClient } = global as unknown as GlobalWithSegmentClient; - // anonymous event has no properties + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: false, + ...{}, + }); expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: true, + ...nonAnonProp, }); + expect(segmentMockClient.track).toHaveBeenCalledTimes(2); + }); + + it('with only anonymous properties (A3)', async () => { + const metaMetrics = TestMetaMetrics.getInstance(); + await metaMetrics.enable(); + const event: IMetaMetricsEvent = { category: 'test event' }; + const anonProperties = { + anon_property: { anonymous: true, value: 'anon value' }, + }; + + metaMetrics.trackEvent(event, anonProperties); + + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; - // non-anonymous event has no properties + // non-anonymous event has no properties. expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: false, + ...{}, + }); + + // anonymous event has anon properties + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: true, + ...anonProperties, }); + + // two events should be tracked, one anonymous and one non-anonymous + expect(segmentMockClient.track).toHaveBeenCalledTimes(2); }); - it('tracks two events when some props are anonymous', async () => { + it('with anonymous and non-anonymous properties (A4)', async () => { const metaMetrics = TestMetaMetrics.getInstance(); await metaMetrics.enable(); - const event: IMetaMetricsEvent = { category: 'event1' }; - const nonAnonProp = { non_anon_prop: 'value1' }; - const anonProp = { - anon_prop: { anonymous: true, value: 'anonValue2' }, + const event: IMetaMetricsEvent = { category: 'test event' }; + const nonAnonProperties = { non_anon_prop: 'non anon value' }; + const anonProperties = { + anon_prop: { anonymous: true, value: 'anon value' }, }; - const properties = { ...nonAnonProp, ...anonProp }; + const properties = { ...nonAnonProperties, ...anonProperties }; - metaMetrics.trackAnonymousEvent(event, properties); + metaMetrics.trackEvent(event, properties); const { segmentMockClient } = global as unknown as GlobalWithSegmentClient; @@ -292,31 +388,40 @@ describe('MetaMetrics', () => { // non-anonymous event only has the non-anonymous properties. expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: false, - ...nonAnonProp, + ...nonAnonProperties, }); // anonymous event has all properties expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: true, - ...anonProp, - ...nonAnonProp, + ...nonAnonProperties, + ...anonProperties, }); // Only two events should be tracked, one anonymous and one non-anonymous expect(segmentMockClient.track).toHaveBeenCalledTimes(2); }); + }); - it('does not track any event if disabled', async () => { + describe('saveDataRecording', () => { + it('tracks event without updating dataRecorded status', async () => { const metaMetrics = TestMetaMetrics.getInstance(); - const event: IMetaMetricsEvent = { category: 'event1' }; - const properties = { prop1: 'value1' }; + expect(await metaMetrics.configure()).toBeTruthy(); + await metaMetrics.enable(); + const event: IMetaMetricsEvent = { category: 'test event' }; + const properties = { regular_prop: 'test value' }; - metaMetrics.trackAnonymousEvent(event, properties); + metaMetrics.trackEvent(event, properties, false); const { segmentMockClient } = global as unknown as GlobalWithSegmentClient; - expect(segmentMockClient.track).not.toHaveBeenCalled(); + expect(StorageWrapper.getItem).toHaveBeenCalledWith(METRICS_OPT_IN); + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: false, + ...properties, + }); + expect(metaMetrics.isDataRecorded()).toBeFalsy(); }); }); @@ -326,8 +431,8 @@ describe('MetaMetrics', () => { expect(await metaMetrics.configure()).toBeTruthy(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { - category: 'event1', - properties: { action: 'action1', name: 'description1' }, + category: 'test event', + properties: { action: 'test action', name: 'test description' }, }; metaMetrics.trackEvent(event); @@ -346,10 +451,16 @@ describe('MetaMetrics', () => { expect(await metaMetrics.configure()).toBeTruthy(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { - category: 'event1', - properties: { action: 'action1', name: 'description1' }, + category: 'test event', + properties: { + action: 'legacy test action', + name: 'legacy test description', + }, + }; + const properties = { + action: 'overriding test action', + name: 'overriding test description', }; - const properties = { action: 'action2', name: 'description2' }; metaMetrics.trackEvent(event, properties); From 8cf34d266372a8fd9d155c678c3b26b3d3b7b151 Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Tue, 20 Aug 2024 16:08:45 +0200 Subject: [PATCH 04/16] Update A2 case based on Slack convo --- app/core/Analytics/MetaMetrics.test.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/app/core/Analytics/MetaMetrics.test.ts b/app/core/Analytics/MetaMetrics.test.ts index 842793c8373..1b9ec49b6c5 100644 --- a/app/core/Analytics/MetaMetrics.test.ts +++ b/app/core/Analytics/MetaMetrics.test.ts @@ -182,7 +182,7 @@ describe('MetaMetrics', () => { * | NA1 | NA | NO | NO | EMPTY | NONE | * | A1 | A | NO | NO | EMPTY | NONE | * | NA2 | NA | YES | NO | NA PROPS | NONE | - * | A2 | A | YES | NO | EMPTY | NA PROPS | + * | A2 | A | YES | NO | NA PROPS | NONE | * | NA3 | NA | NO | YES | EMPTY | A PROPS | * | A3 | A | NO | YES | EMPTY | A PROPS | * | NA4 | NA | YES | YES | NA PROPS | NA PROPS + A PROPS | @@ -190,11 +190,11 @@ describe('MetaMetrics', () => { * * the following test cases include the code (NAX or AX) of the test in the table for reference. * - * NOTE: some NAX and AX test cases (all except NA2/A2) have similar result but differ only by calling the + * NOTE: NAX and AX test cases have similar result but differ only by calling the * anonymous (trackAnonymousEvent) or non-anonymous tracking (trackEvent). * They are grouped together here and both implementations uses trackEvent under the hood. * Ultimately these duplicates will disappear when we deprecate trackAnonymousEvent. - * But that's another issue to address. + * But that's another issue to address, for now let's keep it simple and double test. */ describe('tracks non-anonymous event (NA)', () => { it('without properties (NA1)', async () => { @@ -330,15 +330,12 @@ describe('MetaMetrics', () => { const { segmentMockClient } = global as unknown as GlobalWithSegmentClient; + // check if the event was tracked expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: false, - ...{}, - }); - expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: true, ...nonAnonProp, }); - expect(segmentMockClient.track).toHaveBeenCalledTimes(2); + expect(segmentMockClient.track).toHaveBeenCalledTimes(1); }); it('with only anonymous properties (A3)', async () => { From 5531e93423e045bc77534a894e9405f1241dffdc Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Tue, 20 Aug 2024 16:19:02 +0200 Subject: [PATCH 05/16] remove trackAnonymousEvent test --- app/core/Analytics/MetaMetrics.test.ts | 139 +++---------------------- 1 file changed, 14 insertions(+), 125 deletions(-) diff --git a/app/core/Analytics/MetaMetrics.test.ts b/app/core/Analytics/MetaMetrics.test.ts index 1b9ec49b6c5..45497305f8e 100644 --- a/app/core/Analytics/MetaMetrics.test.ts +++ b/app/core/Analytics/MetaMetrics.test.ts @@ -172,32 +172,22 @@ describe('MetaMetrics', () => { describe('Tracking', () => { /* This is the matrix of tracking use cases. * How to read it? Here are some examples: - * - Test NA1 means non-anonymous tracking (NA) but it has no props at all: + * - Test A means non-anonymous tracking (NA) and it has no props at all: * The result must be only one non-anonymous event without any props (EMPTY) and no anonymous event at all (NONE). - * - Test A4 means anonymous tracking (A) and it has both non anonymous and anonymous props: + * - Test D means anonymous tracking (A) and it has both non-anonymous and anonymous props: * The result must be a non-anonymous event with non-anonymous props (NA PROPS) and an anonymous event with all props (NA PROPS + A PROPS). * - * | Test | Track | Non-anon prop | Anon prop | Result non-anon (NA) event | Result anon (A) event | - * |--------|---------|---------------|-----------|----------------------------|-----------------------| - * | NA1 | NA | NO | NO | EMPTY | NONE | - * | A1 | A | NO | NO | EMPTY | NONE | - * | NA2 | NA | YES | NO | NA PROPS | NONE | - * | A2 | A | YES | NO | NA PROPS | NONE | - * | NA3 | NA | NO | YES | EMPTY | A PROPS | - * | A3 | A | NO | YES | EMPTY | A PROPS | - * | NA4 | NA | YES | YES | NA PROPS | NA PROPS + A PROPS | - * | A4 | A | YES | YES | NA PROPS | NA PROPS + A PROPS | + * | Test | Track | Non-anon prop | Anon prop | Result non-anon (NA) event | Result anon (A) event | + * |------|---------|---------------|-----------|----------------------------|-----------------------| + * | A | NA | NO | NO | EMPTY | NONE | + * | B | NA | YES | NO | NA PROPS | NONE | + * | C | NA | NO | YES | EMPTY | A PROPS | + * | D | NA | YES | YES | NA PROPS | NA PROPS + A PROPS | * - * the following test cases include the code (NAX or AX) of the test in the table for reference. - * - * NOTE: NAX and AX test cases have similar result but differ only by calling the - * anonymous (trackAnonymousEvent) or non-anonymous tracking (trackEvent). - * They are grouped together here and both implementations uses trackEvent under the hood. - * Ultimately these duplicates will disappear when we deprecate trackAnonymousEvent. - * But that's another issue to address, for now let's keep it simple and double test. + * the following test cases include the code (A,B, C and D) of the test in the table for reference. */ - describe('tracks non-anonymous event (NA)', () => { - it('without properties (NA1)', async () => { + describe('tracks event', () => { + it('without properties (test A)', async () => { const metaMetrics = TestMetaMetrics.getInstance(); expect(await metaMetrics.configure()).toBeTruthy(); await metaMetrics.enable(); @@ -216,7 +206,7 @@ describe('MetaMetrics', () => { expect(segmentMockClient.track).toHaveBeenCalledTimes(1); }); - it('with non-anonymous properties (NA2)', async () => { + it('with only non-anonymous properties (test B)', async () => { const metaMetrics = TestMetaMetrics.getInstance(); expect(await metaMetrics.configure()).toBeTruthy(); await metaMetrics.enable(); @@ -236,7 +226,7 @@ describe('MetaMetrics', () => { expect(segmentMockClient.track).toHaveBeenCalledTimes(1); }); - it('with only anonymous properties (NA3)', async () => { + it('with only anonymous properties (test C)', async () => { const metaMetrics = TestMetaMetrics.getInstance(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { category: 'test event' }; @@ -266,108 +256,7 @@ describe('MetaMetrics', () => { expect(segmentMockClient.track).toHaveBeenCalledTimes(2); }); - it('with anonymous and non-anonymous properties (NA4)', async () => { - const metaMetrics = TestMetaMetrics.getInstance(); - await metaMetrics.enable(); - const event: IMetaMetricsEvent = { category: 'test event' }; - const nonAnonProperties = { non_anon_prop: 'non anon value' }; - const anonProperties = { - anon_prop: { anonymous: true, value: 'anon value' }, - }; - const properties = { ...nonAnonProperties, ...anonProperties }; - - metaMetrics.trackEvent(event, properties); - - const { segmentMockClient } = - global as unknown as GlobalWithSegmentClient; - - // non-anonymous event only has the non-anonymous properties. - expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: false, - ...nonAnonProperties, - }); - - // anonymous event has all properties - expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: true, - ...nonAnonProperties, - ...anonProperties, - }); - - // Only two events should be tracked, one anonymous and one non-anonymous - expect(segmentMockClient.track).toHaveBeenCalledTimes(2); - }); - }); - - describe('tracks anonymous event (A)', () => { - it('without properties (A1)', async () => { - const metaMetrics = TestMetaMetrics.getInstance(); - expect(await metaMetrics.configure()).toBeTruthy(); - await metaMetrics.enable(); - const event: IMetaMetricsEvent = { category: 'test event' }; - - metaMetrics.trackAnonymousEvent(event); - - const { segmentMockClient } = - global as unknown as GlobalWithSegmentClient; - - expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: false, - ...{}, - }); - expect(segmentMockClient.track).toHaveBeenCalledTimes(1); - }); - - it('with non-anonymous properties (A2)', async () => { - const metaMetrics = TestMetaMetrics.getInstance(); - expect(await metaMetrics.configure()).toBeTruthy(); - await metaMetrics.enable(); - const event: IMetaMetricsEvent = { category: 'test event' }; - const nonAnonProp = { non_anon_prop: 'test value' }; - - metaMetrics.trackAnonymousEvent(event, nonAnonProp); - - const { segmentMockClient } = - global as unknown as GlobalWithSegmentClient; - - // check if the event was tracked - expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: false, - ...nonAnonProp, - }); - expect(segmentMockClient.track).toHaveBeenCalledTimes(1); - }); - - it('with only anonymous properties (A3)', async () => { - const metaMetrics = TestMetaMetrics.getInstance(); - await metaMetrics.enable(); - const event: IMetaMetricsEvent = { category: 'test event' }; - const anonProperties = { - anon_property: { anonymous: true, value: 'anon value' }, - }; - - metaMetrics.trackEvent(event, anonProperties); - - const { segmentMockClient } = - global as unknown as GlobalWithSegmentClient; - - // non-anonymous event has no properties. - expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: false, - ...{}, - }); - - // anonymous event has anon properties - expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { - anonymous: true, - ...anonProperties, - }); - - // two events should be tracked, one anonymous and one non-anonymous - expect(segmentMockClient.track).toHaveBeenCalledTimes(2); - }); - - it('with anonymous and non-anonymous properties (A4)', async () => { + it('with anonymous and non-anonymous properties (test D)', async () => { const metaMetrics = TestMetaMetrics.getInstance(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { category: 'test event' }; From 48bfde035a844f431bdcda5648703d59927c086b Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Tue, 20 Aug 2024 19:25:45 +0200 Subject: [PATCH 06/16] Refactor trackEvent Add new type Keep legacy compatibility --- .../hooks/useMetrics/useMetrics.test.tsx | 9 -- app/components/hooks/useMetrics/useMetrics.ts | 39 +----- .../hooks/useMetrics/useMetrics.types.ts | 8 +- app/core/Analytics/MetaMetrics.test.ts | 121 ++++++++++++++++-- app/core/Analytics/MetaMetrics.ts | 42 ++---- app/core/Analytics/MetaMetrics.types.ts | 20 +-- 6 files changed, 131 insertions(+), 108 deletions(-) diff --git a/app/components/hooks/useMetrics/useMetrics.test.tsx b/app/components/hooks/useMetrics/useMetrics.test.tsx index 0307eed030b..7a6c594bd7c 100644 --- a/app/components/hooks/useMetrics/useMetrics.test.tsx +++ b/app/components/hooks/useMetrics/useMetrics.test.tsx @@ -30,7 +30,6 @@ const expectedDataDeleteRegulationId = 'TWV0YU1hc2t1c2Vzbm9wb2ludCE'; const mockMetrics = { trackEvent: jest.fn(), - trackAnonymousEvent: jest.fn(), enable: jest.fn(() => Promise.resolve()), addTraitsToUser: jest.fn(() => Promise.resolve()), createDataDeletionTask: jest.fn(() => @@ -65,7 +64,6 @@ describe('useMetrics', () => { "getMetaMetricsId": undefined, "isDataRecorded": [MockFunction], "isEnabled": [MockFunction], - "trackAnonymousEvent": [Function], "trackEvent": [Function], } `); @@ -80,7 +78,6 @@ describe('useMetrics', () => { const { trackEvent, - trackAnonymousEvent, enable, addTraitsToUser, createDataDeletionTask, @@ -100,7 +97,6 @@ describe('useMetrics', () => { await act(async () => { trackEvent(event); - trackAnonymousEvent(event); await enable(true); await addTraitsToUser({}); deletionTaskIdValue = await createDataDeletionTask(); @@ -112,11 +108,6 @@ describe('useMetrics', () => { }); expect(mockMetrics.trackEvent).toHaveBeenCalledWith(event, {}, true); - expect(mockMetrics.trackAnonymousEvent).toHaveBeenCalledWith( - event, - {}, - true, - ); expect(mockMetrics.enable).toHaveBeenCalledWith(true); expect(mockMetrics.addTraitsToUser).toHaveBeenCalledWith({}); diff --git a/app/components/hooks/useMetrics/useMetrics.ts b/app/components/hooks/useMetrics/useMetrics.ts index 47116425eaa..85aa2daae70 100644 --- a/app/components/hooks/useMetrics/useMetrics.ts +++ b/app/components/hooks/useMetrics/useMetrics.ts @@ -3,6 +3,7 @@ import { IMetaMetricsEvent, MetaMetrics } from '../../../core/Analytics'; import { JsonMap } from '@segment/analytics-react-native'; import { IUseMetricsHook } from './useMetrics.types'; import { useCallback } from 'react'; +import { EventProperties } from '../../../core/Analytics/MetaMetrics.types'; /** * Hook to use MetaMetrics @@ -23,7 +24,6 @@ import { useCallback } from 'react'; * @example a full destructuration of the hook: * const { * trackEvent, - * trackAnonymousEvent, * enable, * addTraitsToUser, * createDataDeletionTask, @@ -36,40 +36,6 @@ import { useCallback } from 'react'; * } = useMetrics(); */ const useMetrics = (): IUseMetricsHook => { - /** - * Track an anonymous event - * - * This will track the event twice: once with the anonymous ID and once with the user ID - * - * - The anynomous event has properties set so you can know *what* but not *who* - * - The non-anonymous event has no properties so you can know *who* but not *what* - * - * @param event - IMetaMetricsEvent event - * @param properties - Object containing any event relevant traits or properties (optional) - * @param saveDataRecording - param to skip saving the data recording flag (optional) - * - * @example - * const { trackAnonymousEvent } = useMetrics(); - * trackAnonymousEvent(MetaMetricsEvents.SWAP_COMPLETED); - * - * @see MetaMetrics.trackAnonymousEvent - */ - const trackAnonymousEvent = useCallback( - ( - event: IMetaMetricsEvent, - properties: JsonMap = {}, - saveDataRecording = true, - ) => { - InteractionManager.runAfterInteractions(async () => { - MetaMetrics.getInstance().trackAnonymousEvent( - event, - properties, - saveDataRecording, - ); - }); - }, - [], - ); /** * Track an event - the regular way * @@ -93,7 +59,7 @@ const useMetrics = (): IUseMetricsHook => { const trackEvent = useCallback( ( event: IMetaMetricsEvent, - properties: JsonMap = {}, + properties: JsonMap | EventProperties = {}, saveDataRecording = true, ) => { InteractionManager.runAfterInteractions(async () => { @@ -109,7 +75,6 @@ const useMetrics = (): IUseMetricsHook => { return { trackEvent, - trackAnonymousEvent, enable: MetaMetrics.getInstance().enable, addTraitsToUser: MetaMetrics.getInstance().addTraitsToUser, createDataDeletionTask: MetaMetrics.getInstance().createDataDeletionTask, diff --git a/app/components/hooks/useMetrics/useMetrics.types.ts b/app/components/hooks/useMetrics/useMetrics.types.ts index 9f02ef3d1f4..5847787d66b 100644 --- a/app/components/hooks/useMetrics/useMetrics.types.ts +++ b/app/components/hooks/useMetrics/useMetrics.types.ts @@ -1,6 +1,7 @@ import type { JsonMap, UserTraits } from '@segment/analytics-react-native'; import { DataDeleteDate, + EventProperties, IDeleteRegulationResponse, IDeleteRegulationStatus, IMetaMetricsEvent, @@ -18,14 +19,9 @@ export interface IUseMetricsHook { isEnabled(): boolean; enable(enable?: boolean): Promise; addTraitsToUser(userTraits: UserTraits): Promise; - trackAnonymousEvent( - event: IMetaMetricsEvent, - properties?: JsonMap, - saveDataRecording?: boolean, - ): void; trackEvent( event: IMetaMetricsEvent, - properties?: JsonMap, + properties?: JsonMap | EventProperties, // EventProperties is the new type, direct JsonMap is for retro compatibility saveDataRecording?: boolean, ): void; createDataDeletionTask(): Promise; diff --git a/app/core/Analytics/MetaMetrics.test.ts b/app/core/Analytics/MetaMetrics.test.ts index 45497305f8e..853feb5837c 100644 --- a/app/core/Analytics/MetaMetrics.test.ts +++ b/app/core/Analytics/MetaMetrics.test.ts @@ -170,7 +170,9 @@ describe('MetaMetrics', () => { }); describe('Tracking', () => { - /* This is the matrix of tracking use cases. + /* This is the matrix of tracking use cases based on extension behaviour. + * it's also retro compatible with our individual anonymous events. + * * How to read it? Here are some examples: * - Test A means non-anonymous tracking (NA) and it has no props at all: * The result must be only one non-anonymous event without any props (EMPTY) and no anonymous event at all (NONE). @@ -181,10 +183,16 @@ describe('MetaMetrics', () => { * |------|---------|---------------|-----------|----------------------------|-----------------------| * | A | NA | NO | NO | EMPTY | NONE | * | B | NA | YES | NO | NA PROPS | NONE | - * | C | NA | NO | YES | EMPTY | A PROPS | + * | C0 | NA | NO | YES(indiv)| EMPTY | A PROPS | + * | C1 | NA | NO | YES(group)| EMPTY | A PROPS | + * | C2 | NA | NO | YES(mixed)| EMPTY | A PROPS | * | D | NA | YES | YES | NA PROPS | NA PROPS + A PROPS | * - * the following test cases include the code (A,B, C and D) of the test in the table for reference. + * For C0/C1: + * - individual prop is one that is mixed with others but is of the form `prop = { anonymous: true, value: 'anon value' }` + * - group anonymous props are of the form `prop = 'anon value'` but are grouped in an object implementing the SensitiveProperties interface. + * + * The following test cases include the code (A,B, C0/C1 and D) of the test in the table for reference. */ describe('tracks event', () => { it('without properties (test A)', async () => { @@ -226,15 +234,54 @@ describe('MetaMetrics', () => { expect(segmentMockClient.track).toHaveBeenCalledTimes(1); }); - it('with only anonymous properties (test C)', async () => { + it('with only individual anonymous properties (test C0)', async () => { + const metaMetrics = TestMetaMetrics.getInstance(); + await metaMetrics.enable(); + const event: IMetaMetricsEvent = { category: 'test event' }; + + const individualAnonProperties = { + individual_anon_property: { anonymous: true, value: 'anon value' }, + }; + + // this call is backward-compatible with the previous system + metaMetrics.trackEvent(event, individualAnonProperties); + + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + + // check if the event was tracked + // non-anonymous event has no properties. + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: false, + ...{}, + }); + + // anonymous event has individual anon properties + // the prop value must be extracted and passed directly as a value. + // the original anonymous prop of the prop is discarded. + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: true, + ...{ + individual_anon_property: + individualAnonProperties.individual_anon_property.value, + }, + }); + + // two events should be tracked, one anonymous and one non-anonymous + expect(segmentMockClient.track).toHaveBeenCalledTimes(2); + }); + + it('with only anonymous properties group (test C1)', async () => { const metaMetrics = TestMetaMetrics.getInstance(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { category: 'test event' }; - const anonProperties = { - anon_property: { anonymous: true, value: 'anon value' }, + + const groupAnonProperties = { group_anon_property: 'group anon value' }; + const properties = { + sensitiveProperties: { ...groupAnonProperties }, }; - metaMetrics.trackEvent(event, anonProperties); + metaMetrics.trackEvent(event, properties); const { segmentMockClient } = global as unknown as GlobalWithSegmentClient; @@ -246,10 +293,49 @@ describe('MetaMetrics', () => { ...{}, }); - // anonymous event has anon properties + // anonymous event has group anon properties expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: true, - ...anonProperties, + ...groupAnonProperties, + }); + + // two events should be tracked, one anonymous and one non-anonymous + expect(segmentMockClient.track).toHaveBeenCalledTimes(2); + }); + + it('with mixed (group and individual) anonymous properties (test C2)', async () => { + const metaMetrics = TestMetaMetrics.getInstance(); + await metaMetrics.enable(); + const event: IMetaMetricsEvent = { category: 'test event' }; + + const individualAnonProperties = { + anon_prop: { anonymous: true, value: 'anon value' }, + }; + const groupAnonProperties = { group_anon_property: 'group anon value' }; + const properties = { + properties: { ...individualAnonProperties }, + sensitiveProperties: { ...groupAnonProperties }, + }; + + metaMetrics.trackEvent(event, properties); + + const { segmentMockClient } = + global as unknown as GlobalWithSegmentClient; + + // check if the event was tracked + // non-anonymous event has no properties. + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: false, + ...{}, + }); + + // anonymous event has both individual and group anon properties + expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { + anonymous: true, + ...{ + individual_anon_property: individualAnonProperties.anon_prop.value, + }, + ...groupAnonProperties, }); // two events should be tracked, one anonymous and one non-anonymous @@ -260,11 +346,21 @@ describe('MetaMetrics', () => { const metaMetrics = TestMetaMetrics.getInstance(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { category: 'test event' }; + const nonAnonProperties = { non_anon_prop: 'non anon value' }; - const anonProperties = { + const individualAnonProperties = { anon_prop: { anonymous: true, value: 'anon value' }, }; - const properties = { ...nonAnonProperties, ...anonProperties }; + const groupAnonProperties = { group_anon_property: 'group anon value' }; + + // Testing only the mixed non-anon/individual-anon/group-anon properties case as it covers all other cases. + const properties = { + properties: { + ...nonAnonProperties, + ...individualAnonProperties, + }, + sensitiveProperties: { ...groupAnonProperties }, + }; metaMetrics.trackEvent(event, properties); @@ -281,7 +377,8 @@ describe('MetaMetrics', () => { expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: true, ...nonAnonProperties, - ...anonProperties, + ...{ anon_prop: individualAnonProperties.anon_prop.value }, + ...groupAnonProperties, }); // Only two events should be tracked, one anonymous and one non-anonymous diff --git a/app/core/Analytics/MetaMetrics.ts b/app/core/Analytics/MetaMetrics.ts index 6a4628bfa89..e69ab322122 100644 --- a/app/core/Analytics/MetaMetrics.ts +++ b/app/core/Analytics/MetaMetrics.ts @@ -24,6 +24,7 @@ import { DataDeleteRegulationId, DataDeleteResponseStatus, DataDeleteStatus, + EventProperties, IDeleteRegulationResponse, IDeleteRegulationStatus, IDeleteRegulationStatusResponse, @@ -606,20 +607,21 @@ class MetaMetrics implements IMetaMetrics { return Promise.resolve(); }; - handleEvent = ( + #handleEvent = ( event: IMetaMetricsEvent, - params: JsonMap, + properties: JsonMap | EventProperties, saveDataRecording: boolean, - anon?: boolean, ) => { - if (!params || Object.keys(params).length === 0) { + if (!properties || Object.keys(properties).length === 0) { this.#trackEvent( event?.category, - { anonymous: anon || false, ...event?.properties }, + { anonymous: false, ...event?.properties }, saveDataRecording, ); } - const [userParams, anonymousParams] = preProcessAnalyticsEvent(params); + + //TODO refactor + const [userParams, anonymousParams] = preProcessAnalyticsEvent(properties); // Log all non-anonymous properties if (Object.keys(userParams).length) { @@ -641,42 +643,20 @@ class MetaMetrics implements IMetaMetrics { } }; - /** - * Track an anonymous event - * - * This will track the event twice: once with the anonymous ID and once with the user ID - * - * - The anynomous event has properties set so you can know *what* but not *who* - * - The non-anonymous event has no properties so you can know *who* but not *what* - * - * @param event - Analytics event name - * @param properties - Object containing any event relevant traits or properties (optional) - * @param saveDataRecording - param to skip saving the data recording flag (optional) - */ - trackAnonymousEvent( - event: IMetaMetricsEvent, - properties: JsonMap = {}, - saveDataRecording = true, - ): void { - if (this.enabled) { - this.handleEvent(event, properties, saveDataRecording, true); - } - } - /** * Track an event - the regular way * * @param event - Analytics event name - * @param properties - Object containing any event relevant traits or properties (optional) + * @param properties - Object containing any event relevant traits or properties (optional). * @param saveDataRecording - param to skip saving the data recording flag (optional) */ trackEvent = ( event: IMetaMetricsEvent, - properties: JsonMap = {}, + properties: JsonMap | EventProperties = {}, saveDataRecording = true, ): void => { if (this.enabled) { - this.handleEvent(event, properties, saveDataRecording); + this.#handleEvent(event, properties, saveDataRecording); } }; diff --git a/app/core/Analytics/MetaMetrics.types.ts b/app/core/Analytics/MetaMetrics.types.ts index 8e8a59a5632..de238b1b412 100644 --- a/app/core/Analytics/MetaMetrics.types.ts +++ b/app/core/Analytics/MetaMetrics.types.ts @@ -50,26 +50,15 @@ export interface IMetaMetrics { * @param groupTraits */ group(groupId: string, groupTraits?: GroupTraits): void; - /** - * track an anonymous event, providing only anonymousId - * @param event - Analytics event - * @param properties - Object containing any event relevant traits or properties (optional) - * @param saveDataRecording - param to skip saving the data recording flag (optional) - */ - trackAnonymousEvent( - event: IMetaMetricsEvent, - properties?: JsonMap, - saveDataRecording?: boolean, - ): void; /** * track an event * @param event - Analytics event - * @param properties - Object containing any event relevant traits or properties (optional) + * @param properties - Object containing any event relevant traits or properties (optional). * @param saveDataRecording - param to skip saving the data recording flag (optional) */ trackEvent( event: IMetaMetricsEvent, - properties?: JsonMap, + properties?: JsonMap | EventProperties, // EventProperties is the new type, direct JsonMap is for retro compatibility saveDataRecording?: boolean, ): void; /** @@ -152,3 +141,8 @@ export interface IDeleteRegulationStatus { hasCollectedDataSinceDeletionRequest: boolean; dataDeletionRequestStatus: DataDeleteStatus; } + +export interface EventProperties { + properties?: JsonMap; + sensitiveProperties?: JsonMap; +} From 75cb23018d409c5f68c5425686bbab9f0920c24b Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Wed, 21 Aug 2024 20:20:25 +0200 Subject: [PATCH 07/16] Refactor metrics according to tests and adjust tests --- app/core/Analytics/MetaMetrics.test.ts | 2 +- app/core/Analytics/MetaMetrics.ts | 75 +++++------ .../events/convertLegacyProperties.test.ts | 117 ++++++++++++++++++ app/util/events/convertLegacyProperties.ts | 55 ++++++++ .../events/preProcessAnalyticsEvent.test.ts | 75 ++++------- app/util/events/preProcessAnalyticsEvent.ts | 32 ++--- 6 files changed, 252 insertions(+), 104 deletions(-) create mode 100644 app/util/events/convertLegacyProperties.test.ts create mode 100644 app/util/events/convertLegacyProperties.ts diff --git a/app/core/Analytics/MetaMetrics.test.ts b/app/core/Analytics/MetaMetrics.test.ts index 853feb5837c..5dbb8045c3a 100644 --- a/app/core/Analytics/MetaMetrics.test.ts +++ b/app/core/Analytics/MetaMetrics.test.ts @@ -333,7 +333,7 @@ describe('MetaMetrics', () => { expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: true, ...{ - individual_anon_property: individualAnonProperties.anon_prop.value, + anon_prop: individualAnonProperties.anon_prop.value, }, ...groupAnonProperties, }); diff --git a/app/core/Analytics/MetaMetrics.ts b/app/core/Analytics/MetaMetrics.ts index e69ab322122..1f75d54e807 100644 --- a/app/core/Analytics/MetaMetrics.ts +++ b/app/core/Analytics/MetaMetrics.ts @@ -37,8 +37,8 @@ import { v4 as uuidv4 } from 'uuid'; import { Config } from '@segment/analytics-react-native/lib/typescript/src/types'; import generateDeviceAnalyticsMetaData from '../../util/metrics/DeviceAnalyticsMetaData/generateDeviceAnalyticsMetaData'; import generateUserSettingsAnalyticsMetaData from '../../util/metrics/UserSettingsAnalyticsMetaData/generateUserProfileAnalyticsMetaData'; -import preProcessAnalyticsEvent from '../../util/events/preProcessAnalyticsEvent'; import { isE2E } from '../../util/test/utils'; +import convertLegacyProperties from '../../util/events/convertLegacyProperties'; /** * MetaMetrics using Segment as the analytics provider. @@ -607,56 +607,59 @@ class MetaMetrics implements IMetaMetrics { return Promise.resolve(); }; - #handleEvent = ( + /** + * Track an event + * + * @param event - Analytics event name + * @param properties - Object containing any event relevant traits or properties (optional). + * @param saveDataRecording - param to skip saving the data recording flag (optional) + */ + trackEvent = ( event: IMetaMetricsEvent, - properties: JsonMap | EventProperties, - saveDataRecording: boolean, - ) => { + properties: JsonMap | EventProperties = {}, + saveDataRecording = true, + ): void => { + if (!this.enabled) { + return; + } + + // if event does not have properties, only send the non-anonymous empty event + // and return to prevent any additional processing if (!properties || Object.keys(properties).length === 0) { this.#trackEvent( event?.category, + // pass the IMetaMetricsEvent properties in the tracking props in case they exist(if it's a legacy event) { anonymous: false, ...event?.properties }, saveDataRecording, ); + return; } - //TODO refactor - const [userParams, anonymousParams] = preProcessAnalyticsEvent(properties); + // if event has properties, convert then to EventProperties, + const convertedProperties = convertLegacyProperties(properties); - // Log all non-anonymous properties - if (Object.keys(userParams).length) { - this.#trackEvent( - event?.category, - { anonymous: false, ...event?.properties, ...userParams }, - saveDataRecording, - ); - } + // Log all non-anonymous properties, or an empty event if there's no non-anon props. + // In any case, there's a non-anon event tracked, see MetaMetrics.test.ts Tracking table. + this.#trackEvent( + event?.category, + { anonymous: false, ...convertedProperties.properties }, + saveDataRecording, + ); - // Log all anonymous properties - if (Object.keys(anonymousParams).length) { + // Track all anonymous properties in an anonymous event + if ( + convertedProperties.sensitiveProperties && + Object.keys(convertedProperties.sensitiveProperties).length + ) { this.#trackEvent( event.category, - { anonymous: true, ...anonymousParams }, + { + anonymous: true, + ...convertedProperties.sensitiveProperties, + ...convertedProperties.properties, + }, saveDataRecording, ); - this.#trackEvent(event.category, { anonymous: true }, saveDataRecording); - } - }; - - /** - * Track an event - the regular way - * - * @param event - Analytics event name - * @param properties - Object containing any event relevant traits or properties (optional). - * @param saveDataRecording - param to skip saving the data recording flag (optional) - */ - trackEvent = ( - event: IMetaMetricsEvent, - properties: JsonMap | EventProperties = {}, - saveDataRecording = true, - ): void => { - if (this.enabled) { - this.#handleEvent(event, properties, saveDataRecording); } }; diff --git a/app/util/events/convertLegacyProperties.test.ts b/app/util/events/convertLegacyProperties.test.ts new file mode 100644 index 00000000000..5866d3a4a15 --- /dev/null +++ b/app/util/events/convertLegacyProperties.test.ts @@ -0,0 +1,117 @@ +import convertLegacyProperties from './convertLegacyProperties'; + +describe('convertLegacyProperties', () => { + it('processes empty input', () => { + const result = convertLegacyProperties({}); + expect(result).toEqual({ + properties: {}, + sensitiveProperties: {}, + }); + }); + + it('returns the same object if input is already EventProperties', () => { + const properties = { + properties: { prop1: 'value1' }, + sensitiveProperties: { prop2: 'value2' }, + }; + const result = convertLegacyProperties(properties); + expect(result).toEqual(properties); + }); + + it('processes EventProperties with legacy anonymous properties', () => { + const properties = { + properties: { + prop1: 'value1', + active_currency: { anonymous: true, value: 'FOXY' }, + }, + sensitiveProperties: { prop2: 'value2' }, + }; + const result = convertLegacyProperties(properties); + expect(result).toEqual({ + properties: { + prop1: 'value1', + }, + sensitiveProperties: { + active_currency: 'FOXY', + prop2: 'value2', + }, + }); + }); + + it('processes non-object properties', () => { + const properties = { + prop1: 'value1', + prop2: 123, + }; + const result = convertLegacyProperties(properties); + expect(result).toEqual({ + properties, + sensitiveProperties: {}, + }); + }); + + it('separates anonymous and non-anonymous object properties', () => { + const properties = { + account_type: 'Imported', + active_currency: { anonymous: true, value: 'FOXY' }, + chain_id: '59144', + gas_estimate_type: 'fee-market', + gas_mode: 'Basic', + request_source: 'In-App-Browser', + speed_set: 'medium', + }; + const result = convertLegacyProperties(properties); + expect(result).toEqual({ + properties: { + account_type: 'Imported', + chain_id: '59144', + gas_estimate_type: 'fee-market', + gas_mode: 'Basic', + request_source: 'In-App-Browser', + speed_set: 'medium', + }, + sensitiveProperties: { + active_currency: 'FOXY', + }, + }); + }); + + it('adds arrays to non-anonymous properties', () => { + const properties = { + arrayProp: [1, 2, 3], + }; + const result = convertLegacyProperties(properties); + expect(result).toEqual({ + properties, + sensitiveProperties: {}, + }); + }); + + it('handles mixed types', () => { + const properties = { + account_type: 'Imported', + active_currency: { anonymous: true, value: 'FOXY' }, + chain_id: '59144', + gas_estimate_type: 'fee-market', + gas_mode: 'Basic', + request_source: 'In-App-Browser', + speed_set: 'medium', + arrayProp: ['a', 'b', 'c'], + }; + const result = convertLegacyProperties(properties); + expect(result).toEqual({ + properties: { + account_type: 'Imported', + chain_id: '59144', + gas_estimate_type: 'fee-market', + gas_mode: 'Basic', + request_source: 'In-App-Browser', + speed_set: 'medium', + arrayProp: ['a', 'b', 'c'], + }, + sensitiveProperties: { + active_currency: 'FOXY', + }, + }); + }); +}); diff --git a/app/util/events/convertLegacyProperties.ts b/app/util/events/convertLegacyProperties.ts new file mode 100644 index 00000000000..48da7fc3e92 --- /dev/null +++ b/app/util/events/convertLegacyProperties.ts @@ -0,0 +1,55 @@ +import { JsonMap } from '@segment/analytics-react-native'; +import { EventProperties } from '../../core/Analytics/MetaMetrics.types'; +import preProcessAnalyticsEvent from './preProcessAnalyticsEvent'; + +type CombinedProperties = JsonMap | EventProperties; + +function isEventProperties( + properties: CombinedProperties, +): properties is EventProperties { + return ( + properties && + typeof properties === 'object' && + ('properties' in properties || 'sensitiveProperties' in properties) + ); +} + +/** + * Convert legacy properties to the new EventProperties type if needed + * + * If the properties are already of the new type, they are returned as is + * @param properties + */ +function convertLegacyProperties( + properties: CombinedProperties, +): EventProperties { + if (isEventProperties(properties)) { + // EventProperties non-anonymous properties could have anonymous properties inside + // so we need to process them separately + if (properties.properties && Object.keys(properties.properties).length) { + const [nonAnonymousProperties, anonymousProperties] = + preProcessAnalyticsEvent(properties.properties); + return { + properties: nonAnonymousProperties, + // and concatenate all the anon props in sensitiveProperties + sensitiveProperties: { + ...anonymousProperties, + ...properties.sensitiveProperties, + }, + }; + } + // If there are no non-anonymous properties, we don't need to process them + // and we can return the object as is + return properties; + } + + // if the properties are not of the new type, we need to process them + const [nonAnonymousProperties, anonymousProperties] = + preProcessAnalyticsEvent(properties); + return { + properties: nonAnonymousProperties, + sensitiveProperties: anonymousProperties, + }; +} + +export default convertLegacyProperties; diff --git a/app/util/events/preProcessAnalyticsEvent.test.ts b/app/util/events/preProcessAnalyticsEvent.test.ts index b016d1cfb36..e49ad36d04c 100644 --- a/app/util/events/preProcessAnalyticsEvent.test.ts +++ b/app/util/events/preProcessAnalyticsEvent.test.ts @@ -1,36 +1,34 @@ +import { JsonMap } from '@segment/analytics-react-native'; import preProcessAnalyticsEvent from './preProcessAnalyticsEvent'; describe('preProcessAnalyticsEvent', () => { it('should correctly process empty input', () => { - const [userParams, anonymousParams] = preProcessAnalyticsEvent({}); - expect(userParams).toEqual({}); - expect(anonymousParams).toEqual({}); + const [nonAnonymousProperties, anonymousProperties] = + preProcessAnalyticsEvent({}); + expect(nonAnonymousProperties).toEqual({}); + expect(anonymousProperties).toEqual({}); }); - it('should return empty objects for both userParams and anonymousParams when params is undefined', () => { + it('should return empty objects for both nonAnonymousProperties and anonymousProperties when properties is undefined', () => { // Simulate calling the function with undefined by casting undefined to any - const [userParams, anonymousParams] = preProcessAnalyticsEvent( - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - undefined as any, - ); + const [nonAnonymousProperties, anonymousProperties] = + preProcessAnalyticsEvent(undefined as unknown as JsonMap); - expect(userParams).toEqual({}); - expect(anonymousParams).toEqual({}); + expect(nonAnonymousProperties).toEqual({}); + expect(anonymousProperties).toEqual({}); }); it('should process non-object properties correctly', () => { - const params = { + const properties = { prop1: 'value1', prop2: 123, }; - const [userParams, anonymousParams] = preProcessAnalyticsEvent(params); - expect(userParams).toEqual(params); - expect(anonymousParams).toEqual(params); + const [nonAnonymousProperties] = preProcessAnalyticsEvent(properties); + expect(nonAnonymousProperties).toEqual(properties); }); it('should separate anonymous and non-anonymous object properties', () => { - const params = { + const properties = { account_type: 'Imported', active_currency: { anonymous: true, value: 'FOXY' }, chain_id: '59144', @@ -39,8 +37,9 @@ describe('preProcessAnalyticsEvent', () => { request_source: 'In-App-Browser', speed_set: 'medium', }; - const [userParams, anonymousParams] = preProcessAnalyticsEvent(params); - expect(userParams).toEqual({ + const [nonAnonymousProperties, anonymousProperties] = + preProcessAnalyticsEvent(properties); + expect(nonAnonymousProperties).toEqual({ account_type: 'Imported', chain_id: '59144', gas_estimate_type: 'fee-market', @@ -48,28 +47,21 @@ describe('preProcessAnalyticsEvent', () => { request_source: 'In-App-Browser', speed_set: 'medium', }); - expect(anonymousParams).toEqual({ - account_type: 'Imported', + expect(anonymousProperties).toEqual({ active_currency: 'FOXY', - chain_id: '59144', - gas_estimate_type: 'fee-market', - gas_mode: 'Basic', - request_source: 'In-App-Browser', - speed_set: 'medium', }); }); - it('should ignore arrays and add them to both user and anonymous params', () => { - const params = { + it('should ignore arrays and add them to non-anonymous properties', () => { + const properties = { arrayProp: [1, 2, 3], }; - const [userParams, anonymousParams] = preProcessAnalyticsEvent(params); - expect(userParams).toEqual(params); - expect(anonymousParams).toEqual(params); + const [nonAnonymousProperties] = preProcessAnalyticsEvent(properties); + expect(nonAnonymousProperties).toEqual(properties); }); it('should handle mixed types of properties correctly', () => { - const params = { + const properties = { account_type: 'Imported', active_currency: { anonymous: true, value: 'FOXY' }, chain_id: '59144', @@ -79,8 +71,9 @@ describe('preProcessAnalyticsEvent', () => { speed_set: 'medium', arrayProp: ['a', 'b', 'c'], }; - const [userParams, anonymousParams] = preProcessAnalyticsEvent(params); - expect(userParams).toEqual({ + const [nonAnonymousProperties, anonymousProperties] = + preProcessAnalyticsEvent(properties); + expect(nonAnonymousProperties).toEqual({ account_type: 'Imported', chain_id: '59144', gas_estimate_type: 'fee-market', @@ -89,22 +82,8 @@ describe('preProcessAnalyticsEvent', () => { speed_set: 'medium', arrayProp: ['a', 'b', 'c'], }); - expect(anonymousParams).toEqual({ - account_type: 'Imported', + expect(anonymousProperties).toEqual({ active_currency: 'FOXY', - chain_id: '59144', - gas_estimate_type: 'fee-market', - gas_mode: 'Basic', - request_source: 'In-App-Browser', - speed_set: 'medium', - arrayProp: ['a', 'b', 'c'], }); }); }); - -/* - -{"category": "Send Flow", "properties": {"action": "Send Flow", "name": "Adds Amount"}} {"network": "linea-mainnet"} -{"category": "Send Transaction Started"} {"account_type": "Imported", "active_currency": {"anonymous": true, "value": "FOXY"}, "chain_id": "59144", "gas_estimate_type": "fee-market", "gas_mode": "Basic", "request_source": "In-App-Browser", "speed_set": "medium"} - -*/ diff --git a/app/util/events/preProcessAnalyticsEvent.ts b/app/util/events/preProcessAnalyticsEvent.ts index 72afa6fa618..607f3ae3835 100644 --- a/app/util/events/preProcessAnalyticsEvent.ts +++ b/app/util/events/preProcessAnalyticsEvent.ts @@ -1,16 +1,12 @@ import { JsonMap } from '@segment/analytics-react-native'; -function preProcessAnalyticsEvent(params: JsonMap) { - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const userParams = {} as any; - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const anonymousParams = {} as any; +function preProcessAnalyticsEvent(properties: JsonMap) { + const nonAnonymousProperties: JsonMap = {}; + const anonymousProperties: JsonMap = {}; - if (params) { - Object.keys(params).forEach((key) => { - const property = params[key]; + if (properties) { + Object.keys(properties).forEach((key) => { + const property = properties[key]; if ( property && @@ -18,22 +14,20 @@ function preProcessAnalyticsEvent(params: JsonMap) { !Array.isArray(property) ) { if (property.anonymous) { - // Anonymous property - add only to anonymous params - anonymousParams[key] = property.value; + // Anonymous property - add only to anonymous properties + anonymousProperties[key] = property.value; } else { - // Non-anonymous property - add to both - userParams[key] = property.value; - anonymousParams[key] = property.value; + // Non-anonymous property - add only to non-anonymous properties + nonAnonymousProperties[key] = property.value; } } else { - // Non-anonymous properties - add to both - userParams[key] = property; - anonymousParams[key] = property; + // Non-anonymous properties - add only to non-anonymous properties + nonAnonymousProperties[key] = property; } }); } - return [userParams, anonymousParams]; + return [nonAnonymousProperties, anonymousProperties]; } export default preProcessAnalyticsEvent; From c748abecfd50caf427d206bab848e7ca8d679e46 Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Fri, 23 Aug 2024 00:01:47 +0200 Subject: [PATCH 08/16] update unit test names --- app/util/events/preProcessAnalyticsEvent.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/util/events/preProcessAnalyticsEvent.test.ts b/app/util/events/preProcessAnalyticsEvent.test.ts index e49ad36d04c..0cb8cc517f4 100644 --- a/app/util/events/preProcessAnalyticsEvent.test.ts +++ b/app/util/events/preProcessAnalyticsEvent.test.ts @@ -2,14 +2,14 @@ import { JsonMap } from '@segment/analytics-react-native'; import preProcessAnalyticsEvent from './preProcessAnalyticsEvent'; describe('preProcessAnalyticsEvent', () => { - it('should correctly process empty input', () => { + it('processes empty input', () => { const [nonAnonymousProperties, anonymousProperties] = preProcessAnalyticsEvent({}); expect(nonAnonymousProperties).toEqual({}); expect(anonymousProperties).toEqual({}); }); - it('should return empty objects for both nonAnonymousProperties and anonymousProperties when properties is undefined', () => { + it('returns empty objects for both nonAnonymousProperties and anonymousProperties when properties is undefined', () => { // Simulate calling the function with undefined by casting undefined to any const [nonAnonymousProperties, anonymousProperties] = preProcessAnalyticsEvent(undefined as unknown as JsonMap); @@ -18,7 +18,7 @@ describe('preProcessAnalyticsEvent', () => { expect(anonymousProperties).toEqual({}); }); - it('should process non-object properties correctly', () => { + it('processes non-object properties', () => { const properties = { prop1: 'value1', prop2: 123, @@ -27,7 +27,7 @@ describe('preProcessAnalyticsEvent', () => { expect(nonAnonymousProperties).toEqual(properties); }); - it('should separate anonymous and non-anonymous object properties', () => { + it('separates anonymous and non-anonymous object properties', () => { const properties = { account_type: 'Imported', active_currency: { anonymous: true, value: 'FOXY' }, @@ -52,7 +52,7 @@ describe('preProcessAnalyticsEvent', () => { }); }); - it('should ignore arrays and add them to non-anonymous properties', () => { + it('ignores arrays and add them to non-anonymous properties', () => { const properties = { arrayProp: [1, 2, 3], }; @@ -60,7 +60,7 @@ describe('preProcessAnalyticsEvent', () => { expect(nonAnonymousProperties).toEqual(properties); }); - it('should handle mixed types of properties correctly', () => { + it('handles mixed types of properties', () => { const properties = { account_type: 'Imported', active_currency: { anonymous: true, value: 'FOXY' }, From 07c231cae26be6da668f629f6d76b8e7d7cd5274 Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Fri, 23 Aug 2024 12:30:53 +0200 Subject: [PATCH 09/16] fix test coverage --- app/util/events/preProcessAnalyticsEvent.test.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/app/util/events/preProcessAnalyticsEvent.test.ts b/app/util/events/preProcessAnalyticsEvent.test.ts index 0cb8cc517f4..4907dcf20e3 100644 --- a/app/util/events/preProcessAnalyticsEvent.test.ts +++ b/app/util/events/preProcessAnalyticsEvent.test.ts @@ -86,4 +86,16 @@ describe('preProcessAnalyticsEvent', () => { active_currency: 'FOXY', }); }); + + it('adds non-anonymous object properties without anonymous key to nonAnonymousProperties', () => { + const properties = { + non_anonymous_object: { value: 'testValue' }, + }; + const [nonAnonymousProperties, anonymousProperties] = + preProcessAnalyticsEvent(properties); + expect(nonAnonymousProperties).toEqual({ + non_anonymous_object: 'testValue', + }); + expect(anonymousProperties).toEqual({}); + }); }); From b26ed1c0fc3f826161d2d4950f7d328e192ab6a1 Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Fri, 23 Aug 2024 13:26:53 +0200 Subject: [PATCH 10/16] migrate all calls from trackAnonymousEvent to trackEvent and update unit tests for ramp hook --- .../PermissionApproval.test.tsx | 1 - app/components/Nav/Main/RootRPCMethodsUI.js | 11 ++--- app/components/UI/OptinMetrics/index.test.tsx | 1 - .../UI/Ramp/hooks/useAnalytics.test.ts | 17 ++++--- app/components/UI/Ramp/hooks/useAnalytics.ts | 6 +-- app/components/UI/Swaps/QuotesView.js | 49 ++++++++++++------- .../UI/Swaps/components/TokenSelectModal.js | 14 +++--- app/components/UI/Swaps/index.js | 6 ++- ...taMetricsAndDataCollectionSection.test.tsx | 1 - 9 files changed, 59 insertions(+), 47 deletions(-) diff --git a/app/components/Approvals/PermissionApproval/PermissionApproval.test.tsx b/app/components/Approvals/PermissionApproval/PermissionApproval.test.tsx index fd84b0f9915..4c47bf6d0a1 100644 --- a/app/components/Approvals/PermissionApproval/PermissionApproval.test.tsx +++ b/app/components/Approvals/PermissionApproval/PermissionApproval.test.tsx @@ -73,7 +73,6 @@ describe('PermissionApproval', () => { jest.resetAllMocks(); (useMetrics as jest.MockedFn).mockReturnValue({ trackEvent: mockTrackEvent, - trackAnonymousEvent: jest.fn(), enable: jest.fn(), addTraitsToUser: jest.fn(), createDataDeletionTask: jest.fn(), diff --git a/app/components/Nav/Main/RootRPCMethodsUI.js b/app/components/Nav/Main/RootRPCMethodsUI.js index c602a8acebf..63d697669b5 100644 --- a/app/components/Nav/Main/RootRPCMethodsUI.js +++ b/app/components/Nav/Main/RootRPCMethodsUI.js @@ -129,7 +129,7 @@ export const useSwapConfirmedEvent = ({ trackSwaps }) => { }; const RootRPCMethodsUI = (props) => { - const { trackEvent, trackAnonymousEvent } = useMetrics(); + const { trackEvent } = useMetrics(); const [transactionModalType, setTransactionModalType] = useState(undefined); const tokenList = useSelector(selectTokenList); const setTransactionObject = props.setTransactionObject; @@ -237,7 +237,7 @@ const RootRPCMethodsUI = (props) => { ...smartTransactionMetricsProperties, }; - trackAnonymousEvent(event, parameters); + trackEvent(event, parameters); } catch (e) { Logger.error(e, MetaMetricsEvents.SWAP_TRACKING_FAILED); trackEvent(MetaMetricsEvents.SWAP_TRACKING_FAILED, { @@ -245,12 +245,7 @@ const RootRPCMethodsUI = (props) => { }); } }, - [ - props.selectedAddress, - props.shouldUseSmartTransaction, - trackAnonymousEvent, - trackEvent, - ], + [props.selectedAddress, props.shouldUseSmartTransaction, trackEvent], ); const { addTransactionMetaIdForListening } = useSwapConfirmedEvent({ diff --git a/app/components/UI/OptinMetrics/index.test.tsx b/app/components/UI/OptinMetrics/index.test.tsx index 5ed7961c75d..af8826ea872 100644 --- a/app/components/UI/OptinMetrics/index.test.tsx +++ b/app/components/UI/OptinMetrics/index.test.tsx @@ -14,7 +14,6 @@ jest.mock('../../../core/Analytics/MetaMetrics'); const mockMetrics = { trackEvent: jest.fn().mockImplementation(() => Promise.resolve()), - trackAnonymousEvent: jest.fn(), enable: jest.fn(() => Promise.resolve()), addTraitsToUser: jest.fn(() => Promise.resolve()), isEnabled: jest.fn(() => true), diff --git a/app/components/UI/Ramp/hooks/useAnalytics.test.ts b/app/components/UI/Ramp/hooks/useAnalytics.test.ts index 11c78363984..9d4a3336a04 100644 --- a/app/components/UI/Ramp/hooks/useAnalytics.test.ts +++ b/app/components/UI/Ramp/hooks/useAnalytics.test.ts @@ -7,7 +7,6 @@ jest.mock('../../../../core/Analytics', () => ({ MetaMetrics: { getInstance: jest.fn().mockReturnValue({ trackEvent: jest.fn(), - trackAnonymousEvent: jest.fn(), }), }, })); @@ -21,7 +20,7 @@ describe('useAnalytics', () => { jest.clearAllMocks(); }); - it('calls trackEvent with the correct params', () => { + it('calls trackEvent for non-anonymous params', () => { const { result } = renderHookWithProvider(() => useAnalytics()); const testEvent = 'BUY_BUTTON_CLICKED'; @@ -34,13 +33,15 @@ describe('useAnalytics', () => { expect(MetaMetrics.getInstance().trackEvent).toHaveBeenCalledWith( MetaMetricsEvents[testEvent], { - location: 'Amount to Buy Screen', - text: 'Buy', + properties: { + location: 'Amount to Buy Screen', + text: 'Buy', + }, }, ); }); - it('calls trackAnonymousEvent with the correct params', () => { + it('calls trackEvent for anonymous params', () => { const { result } = renderHookWithProvider(() => useAnalytics()); const testEvent = 'RAMP_REGION_SELECTED'; @@ -52,9 +53,11 @@ describe('useAnalytics', () => { result.current(testEvent, testPayload); - expect(MetaMetrics.getInstance().trackAnonymousEvent).toHaveBeenCalledWith( + expect(MetaMetrics.getInstance().trackEvent).toHaveBeenCalledWith( MetaMetricsEvents[testEvent], - testPayload, + { + sensitiveProperties: testPayload, + }, ); }); }); diff --git a/app/components/UI/Ramp/hooks/useAnalytics.ts b/app/components/UI/Ramp/hooks/useAnalytics.ts index 14d9791a1a0..7ab851826f7 100644 --- a/app/components/UI/Ramp/hooks/useAnalytics.ts +++ b/app/components/UI/Ramp/hooks/useAnalytics.ts @@ -41,12 +41,12 @@ export function trackEvent( const anonymous = AnonymousEvents.includes(eventType); InteractionManager.runAfterInteractions(() => { if (anonymous) { - metrics.trackAnonymousEvent(event, { - ...params, + metrics.trackEvent(event, { + sensitiveProperties: { ...params }, }); } else { metrics.trackEvent(event, { - ...params, + properties: { ...params }, }); } }); diff --git a/app/components/UI/Swaps/QuotesView.js b/app/components/UI/Swaps/QuotesView.js index 7674757342f..5e5e1fc219f 100644 --- a/app/components/UI/Swaps/QuotesView.js +++ b/app/components/UI/Swaps/QuotesView.js @@ -409,7 +409,7 @@ function SwapsQuotesView({ const navigation = useNavigation(); /* Get params from navigation */ const route = useRoute(); - const { trackAnonymousEvent, trackEvent } = useMetrics(); + const { trackEvent } = useMetrics(); const { colors } = useTheme(); const styles = createStyles(colors); @@ -744,7 +744,9 @@ function SwapsQuotesView({ chain_id: getDecimalChainId(chainId), }; - trackAnonymousEvent(MetaMetricsEvents.GAS_FEES_CHANGED, parameters); + trackEvent(MetaMetricsEvents.GAS_FEES_CHANGED, { + sensitiveProperties: { ...parameters }, + }); }, [ chainId, @@ -752,7 +754,7 @@ function SwapsQuotesView({ currentCurrency, gasEstimateType, gasLimit, - trackAnonymousEvent, + trackEvent, ], ); @@ -904,7 +906,9 @@ function SwapsQuotesView({ chain_id: getDecimalChainId(chainId), is_smart_transaction: shouldUseSmartTransaction, }; - trackAnonymousEvent(MetaMetricsEvents.SWAP_STARTED, parameters); + trackEvent(MetaMetricsEvents.SWAP_STARTED, { + sensitiveProperties: { ...parameters }, + }); }, // eslint-disable-next-line react-hooks/exhaustive-deps [ @@ -1150,7 +1154,9 @@ function SwapsQuotesView({ custom_spend_limit_amount: currentAmount, chain_id: getDecimalChainId(chainId), }; - trackAnonymousEvent(MetaMetricsEvents.EDIT_SPEND_LIMIT_OPENED, parameters); + trackEvent(MetaMetricsEvents.EDIT_SPEND_LIMIT_OPENED, { + sensitiveProperties: { ...parameters }, + }); }, [ chainId, allQuotes, @@ -1166,7 +1172,7 @@ function SwapsQuotesView({ slippage, sourceAmount, sourceToken, - trackAnonymousEvent, + trackEvent, ]); const handleQuotesReceivedMetric = useCallback(() => { @@ -1196,7 +1202,9 @@ function SwapsQuotesView({ available_quotes: allQuotes.length, chain_id: getDecimalChainId(chainId), }; - trackAnonymousEvent(MetaMetricsEvents.QUOTES_RECEIVED, parameters); + trackEvent(MetaMetricsEvents.QUOTES_RECEIVED, { + sensitiveProperties: { ...parameters }, + }); }, [ chainId, sourceToken, @@ -1209,7 +1217,7 @@ function SwapsQuotesView({ selectedQuoteValue, allQuotes, conversionRate, - trackAnonymousEvent, + trackEvent, ]); const handleOpenQuotesModal = useCallback(() => { @@ -1241,10 +1249,9 @@ function SwapsQuotesView({ chain_id: getDecimalChainId(chainId), }; - trackAnonymousEvent( - MetaMetricsEvents.ALL_AVAILABLE_QUOTES_OPENED, - parameters, - ); + trackEvent(MetaMetricsEvents.ALL_AVAILABLE_QUOTES_OPENED, { + sensitiveProperties: { ...parameters }, + }); }, [ chainId, selectedQuote, @@ -1258,7 +1265,7 @@ function SwapsQuotesView({ allQuotesFetchTime, conversionRate, allQuotes.length, - trackAnonymousEvent, + trackEvent, ]); const handleQuotesErrorMetric = useCallback( @@ -1281,12 +1288,16 @@ function SwapsQuotesView({ gas_fees: '', }; - trackAnonymousEvent(MetaMetricsEvents.QUOTES_TIMED_OUT, parameters); + trackEvent(MetaMetricsEvents.QUOTES_TIMED_OUT, { + sensitiveProperties: { ...parameters }, + }); } else if ( error?.key === swapsUtils.SwapsError.QUOTES_NOT_AVAILABLE_ERROR ) { const parameters = { ...data }; - trackAnonymousEvent(MetaMetricsEvents.NO_QUOTES_AVAILABLE, parameters); + trackEvent(MetaMetricsEvents.NO_QUOTES_AVAILABLE, { + sensitiveProperties: { ...parameters }, + }); } else { trackErrorAsAnalytics(`Swaps: ${error?.key}`, error?.description); } @@ -1298,7 +1309,7 @@ function SwapsQuotesView({ destinationToken, hasEnoughTokenBalance, slippage, - trackAnonymousEvent, + trackEvent, ], ); @@ -1563,7 +1574,9 @@ function SwapsQuotesView({ navigation.setParams({ selectedQuote: undefined }); navigation.setParams({ quoteBegin: Date.now() }); - trackAnonymousEvent(MetaMetricsEvents.QUOTES_REQUESTED, data); + trackEvent(MetaMetricsEvents.QUOTES_REQUESTED, { + sensitiveProperties: { ...data }, + }); }, [ chainId, destinationToken, @@ -1574,7 +1587,7 @@ function SwapsQuotesView({ sourceAmount, sourceToken, trackedRequestedQuotes, - trackAnonymousEvent, + trackEvent, ]); /* Metrics: Quotes received */ diff --git a/app/components/UI/Swaps/components/TokenSelectModal.js b/app/components/UI/Swaps/components/TokenSelectModal.js index 0b4a61778a3..2290ff9feda 100644 --- a/app/components/UI/Swaps/components/TokenSelectModal.js +++ b/app/components/UI/Swaps/components/TokenSelectModal.js @@ -157,7 +157,7 @@ function TokenSelectModal({ balances, }) { const navigation = useNavigation(); - const { trackAnonymousEvent } = useMetrics(); + const { trackEvent } = useMetrics(); const searchInput = useRef(null); const list = useRef(); @@ -304,15 +304,17 @@ function TokenSelectModal({ const handlePressImportToken = useCallback( (item) => { const { address, symbol } = item; - trackAnonymousEvent(MetaMetricsEvents.CUSTOM_TOKEN_IMPORTED, { - address, - symbol, - chain_id: getDecimalChainId(chainId), + trackEvent(MetaMetricsEvents.CUSTOM_TOKEN_IMPORTED, { + sensitiveProperties: { + address, + symbol, + chain_id: getDecimalChainId(chainId), + }, }); hideTokenImportModal(); onItemPress(item); }, - [chainId, hideTokenImportModal, onItemPress, trackAnonymousEvent], + [chainId, hideTokenImportModal, onItemPress, trackEvent], ); const handleBlockExplorerPress = useCallback(() => { diff --git a/app/components/UI/Swaps/index.js b/app/components/UI/Swaps/index.js index fdfcd2aca6a..3fe569643e4 100644 --- a/app/components/UI/Swaps/index.js +++ b/app/components/UI/Swaps/index.js @@ -201,7 +201,7 @@ function SwapsAmountView({ const navigation = useNavigation(); const route = useRoute(); const { colors } = useTheme(); - const { trackAnonymousEvent } = useMetrics(); + const { trackEvent } = useMetrics(); const styles = createStyles(colors); const previousSelectedAddress = useRef(); @@ -277,7 +277,9 @@ function SwapsAmountView({ chain_id: getDecimalChainId(chainId), }; - trackAnonymousEvent(MetaMetricsEvents.SWAPS_OPENED, parameters); + trackEvent(MetaMetricsEvents.SWAPS_OPENED, { + sensitiveProperties: { ...parameters }, + }); }); } else { navigation.pop(); diff --git a/app/components/Views/Settings/SecuritySettings/Sections/MetaMetricsAndDataCollectionSection/MetaMetricsAndDataCollectionSection.test.tsx b/app/components/Views/Settings/SecuritySettings/Sections/MetaMetricsAndDataCollectionSection/MetaMetricsAndDataCollectionSection.test.tsx index 10de17bb5cf..194ce42b003 100644 --- a/app/components/Views/Settings/SecuritySettings/Sections/MetaMetricsAndDataCollectionSection/MetaMetricsAndDataCollectionSection.test.tsx +++ b/app/components/Views/Settings/SecuritySettings/Sections/MetaMetricsAndDataCollectionSection/MetaMetricsAndDataCollectionSection.test.tsx @@ -41,7 +41,6 @@ jest.mock('../../../../../../core/Analytics/MetaMetrics'); const mockMetrics = { trackEvent: jest.fn(), - trackAnonymousEvent: jest.fn(), enable: jest.fn(() => Promise.resolve()), addTraitsToUser: jest.fn(() => Promise.resolve()), isEnabled: jest.fn(() => false), From 0378bc011df90054a3b21becf49283558d5c2588 Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Fri, 23 Aug 2024 13:43:22 +0200 Subject: [PATCH 11/16] use CombinedProperties when possible --- app/components/hooks/useMetrics/useMetrics.ts | 5 ++--- app/components/hooks/useMetrics/useMetrics.types.ts | 6 +++--- app/core/Analytics/MetaMetrics.ts | 6 +++--- app/core/Analytics/MetaMetrics.types.ts | 8 +++++++- app/util/events/convertLegacyProperties.ts | 8 ++++---- 5 files changed, 19 insertions(+), 14 deletions(-) diff --git a/app/components/hooks/useMetrics/useMetrics.ts b/app/components/hooks/useMetrics/useMetrics.ts index 85aa2daae70..d41c559d071 100644 --- a/app/components/hooks/useMetrics/useMetrics.ts +++ b/app/components/hooks/useMetrics/useMetrics.ts @@ -1,9 +1,8 @@ import { InteractionManager } from 'react-native'; import { IMetaMetricsEvent, MetaMetrics } from '../../../core/Analytics'; -import { JsonMap } from '@segment/analytics-react-native'; import { IUseMetricsHook } from './useMetrics.types'; import { useCallback } from 'react'; -import { EventProperties } from '../../../core/Analytics/MetaMetrics.types'; +import { CombinedProperties } from '../../../core/Analytics/MetaMetrics.types'; /** * Hook to use MetaMetrics @@ -59,7 +58,7 @@ const useMetrics = (): IUseMetricsHook => { const trackEvent = useCallback( ( event: IMetaMetricsEvent, - properties: JsonMap | EventProperties = {}, + properties: CombinedProperties = {}, saveDataRecording = true, ) => { InteractionManager.runAfterInteractions(async () => { diff --git a/app/components/hooks/useMetrics/useMetrics.types.ts b/app/components/hooks/useMetrics/useMetrics.types.ts index 5847787d66b..356896ee05a 100644 --- a/app/components/hooks/useMetrics/useMetrics.types.ts +++ b/app/components/hooks/useMetrics/useMetrics.types.ts @@ -1,7 +1,7 @@ -import type { JsonMap, UserTraits } from '@segment/analytics-react-native'; +import type { UserTraits } from '@segment/analytics-react-native'; import { + CombinedProperties, DataDeleteDate, - EventProperties, IDeleteRegulationResponse, IDeleteRegulationStatus, IMetaMetricsEvent, @@ -21,7 +21,7 @@ export interface IUseMetricsHook { addTraitsToUser(userTraits: UserTraits): Promise; trackEvent( event: IMetaMetricsEvent, - properties?: JsonMap | EventProperties, // EventProperties is the new type, direct JsonMap is for retro compatibility + properties?: CombinedProperties, saveDataRecording?: boolean, ): void; createDataDeletionTask(): Promise; diff --git a/app/core/Analytics/MetaMetrics.ts b/app/core/Analytics/MetaMetrics.ts index 1f75d54e807..bed1c2d1763 100644 --- a/app/core/Analytics/MetaMetrics.ts +++ b/app/core/Analytics/MetaMetrics.ts @@ -20,11 +20,11 @@ import { } from '../../constants/storage'; import { + CombinedProperties, DataDeleteDate, DataDeleteRegulationId, DataDeleteResponseStatus, DataDeleteStatus, - EventProperties, IDeleteRegulationResponse, IDeleteRegulationStatus, IDeleteRegulationStatusResponse, @@ -616,7 +616,7 @@ class MetaMetrics implements IMetaMetrics { */ trackEvent = ( event: IMetaMetricsEvent, - properties: JsonMap | EventProperties = {}, + properties: CombinedProperties = {}, saveDataRecording = true, ): void => { if (!this.enabled) { @@ -635,7 +635,7 @@ class MetaMetrics implements IMetaMetrics { return; } - // if event has properties, convert then to EventProperties, + // if event has properties, convert then to the new EventProperties format, const convertedProperties = convertLegacyProperties(properties); // Log all non-anonymous properties, or an empty event if there's no non-anon props. diff --git a/app/core/Analytics/MetaMetrics.types.ts b/app/core/Analytics/MetaMetrics.types.ts index de238b1b412..c90f05c1d27 100644 --- a/app/core/Analytics/MetaMetrics.types.ts +++ b/app/core/Analytics/MetaMetrics.types.ts @@ -58,7 +58,7 @@ export interface IMetaMetrics { */ trackEvent( event: IMetaMetricsEvent, - properties?: JsonMap | EventProperties, // EventProperties is the new type, direct JsonMap is for retro compatibility + properties?: CombinedProperties, saveDataRecording?: boolean, ): void; /** @@ -142,7 +142,13 @@ export interface IDeleteRegulationStatus { dataDeletionRequestStatus: DataDeleteStatus; } +// event properties structure with two distinct properties lists +// for sensitive (anonymous) and regular (non-anonymous) properties +// this structure and naming is mirroring how the extension metrics works. export interface EventProperties { properties?: JsonMap; sensitiveProperties?: JsonMap; } + +// EventProperties is the new type, direct JsonMap is for backward compatibility +export type CombinedProperties = JsonMap | EventProperties; diff --git a/app/util/events/convertLegacyProperties.ts b/app/util/events/convertLegacyProperties.ts index 48da7fc3e92..74628fd5040 100644 --- a/app/util/events/convertLegacyProperties.ts +++ b/app/util/events/convertLegacyProperties.ts @@ -1,9 +1,9 @@ -import { JsonMap } from '@segment/analytics-react-native'; -import { EventProperties } from '../../core/Analytics/MetaMetrics.types'; +import { + CombinedProperties, + EventProperties, +} from '../../core/Analytics/MetaMetrics.types'; import preProcessAnalyticsEvent from './preProcessAnalyticsEvent'; -type CombinedProperties = JsonMap | EventProperties; - function isEventProperties( properties: CombinedProperties, ): properties is EventProperties { From 148d7c029bee1336edab025a7811ed0e0dcb3761 Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Fri, 23 Aug 2024 17:56:06 +0200 Subject: [PATCH 12/16] track call and comment fixes --- app/components/Nav/Main/RootRPCMethodsUI.js | 2 +- app/components/UI/Ramp/hooks/useAnalytics.ts | 2 +- app/components/hooks/useMetrics/useMetrics.ts | 46 ++++++++++++++-- app/core/Analytics/MetaMetrics.test.ts | 3 +- app/core/Analytics/MetaMetrics.ts | 54 +++++++++++++++++++ 5 files changed, 99 insertions(+), 8 deletions(-) diff --git a/app/components/Nav/Main/RootRPCMethodsUI.js b/app/components/Nav/Main/RootRPCMethodsUI.js index 63d697669b5..3a46a71eb46 100644 --- a/app/components/Nav/Main/RootRPCMethodsUI.js +++ b/app/components/Nav/Main/RootRPCMethodsUI.js @@ -237,7 +237,7 @@ const RootRPCMethodsUI = (props) => { ...smartTransactionMetricsProperties, }; - trackEvent(event, parameters); + trackEvent(event, { sensitiveProperties: { ...parameters } }); } catch (e) { Logger.error(e, MetaMetricsEvents.SWAP_TRACKING_FAILED); trackEvent(MetaMetricsEvents.SWAP_TRACKING_FAILED, { diff --git a/app/components/UI/Ramp/hooks/useAnalytics.ts b/app/components/UI/Ramp/hooks/useAnalytics.ts index 7ab851826f7..3dc3c369af4 100644 --- a/app/components/UI/Ramp/hooks/useAnalytics.ts +++ b/app/components/UI/Ramp/hooks/useAnalytics.ts @@ -46,7 +46,7 @@ export function trackEvent( }); } else { metrics.trackEvent(event, { - properties: { ...params }, + ...params, }); } }); diff --git a/app/components/hooks/useMetrics/useMetrics.ts b/app/components/hooks/useMetrics/useMetrics.ts index d41c559d071..068a6f08cac 100644 --- a/app/components/hooks/useMetrics/useMetrics.ts +++ b/app/components/hooks/useMetrics/useMetrics.ts @@ -7,18 +7,54 @@ import { CombinedProperties } from '../../../core/Analytics/MetaMetrics.types'; /** * Hook to use MetaMetrics * + * The hook allows to track non-anonymous and anonymous events, + * with properties and without properties, + * with a unique trackEvent function + * + * ## Regular non-anonymous events + * Regular events are tracked with the user ID and can have properties set + * + * ## Anonymous events + * Anonymous tracking track sends two events: one with the anonymous ID and one with the user ID + * - The anonymous event includes sensitive properties so you can know **what** but not **who** + * - The non-anonymous event has either no properties or not sensitive one so you can know **who** but not **what** + * * @returns MetaMetrics functions * - * @example Most of the time, the only function you will need is trackEvent: + * @example basic non-anonymous tracking with no properties: * const { trackEvent } = useMetrics(); * trackEvent(MetaMetricsEvents.ONBOARDING_STARTED); * - * @example track with properties: + * @example track with non-anonymous properties: + * const { trackEvent } = useMetrics(); + * trackEvent(MetaMetricsEvents.BROWSER_SEARCH_USED, { + * option_chosen: 'Browser Bottom Bar Menu', + * number_of_tabs: undefined, + * }); + * + * @example you can also track with non-anonymous properties (new properties structure): * const { trackEvent } = useMetrics(); * trackEvent(MetaMetricsEvents.BROWSER_SEARCH_USED, { - * option_chosen: 'Browser Bottom Bar Menu', - * number_of_tabs: undefined, - * }); + * properties: { + * option_chosen: 'Browser Bottom Bar Menu', + * number_of_tabs: undefined, + * }, + * }); + * + * @example track an anonymous event (without properties) + * const { trackEvent } = useMetrics(); + * trackEvent(MetaMetricsEvents.SWAP_COMPLETED); + * + * @example track an anonymous event with properties + * trackEvent(MetaMetricsEvents.GAS_FEES_CHANGED, { + * sensitiveProperties: { ...parameters }, + * }); + * + * @example track an event with both anonymous and non-anonymous properties + * trackEvent(MetaMetricsEvents.MY_EVENT, { + * properties: { ...nonAnonymousParameters }, + * sensitiveProperties: { ...anonymousParameters }, + * }); * * @example a full destructuration of the hook: * const { diff --git a/app/core/Analytics/MetaMetrics.test.ts b/app/core/Analytics/MetaMetrics.test.ts index 5dbb8045c3a..7a70c5f2ab2 100644 --- a/app/core/Analytics/MetaMetrics.test.ts +++ b/app/core/Analytics/MetaMetrics.test.ts @@ -188,9 +188,10 @@ describe('MetaMetrics', () => { * | C2 | NA | NO | YES(mixed)| EMPTY | A PROPS | * | D | NA | YES | YES | NA PROPS | NA PROPS + A PROPS | * - * For C0/C1: + * For C0/C1/C2: * - individual prop is one that is mixed with others but is of the form `prop = { anonymous: true, value: 'anon value' }` * - group anonymous props are of the form `prop = 'anon value'` but are grouped in an object implementing the SensitiveProperties interface. + * - mixed means both types in the same event * * The following test cases include the code (A,B, C0/C1 and D) of the test in the table for reference. */ diff --git a/app/core/Analytics/MetaMetrics.ts b/app/core/Analytics/MetaMetrics.ts index bed1c2d1763..29335e46488 100644 --- a/app/core/Analytics/MetaMetrics.ts +++ b/app/core/Analytics/MetaMetrics.ts @@ -58,6 +58,15 @@ import convertLegacyProperties from '../../util/events/convertLegacyProperties'; * metrics.trackEvent(event, { property: 'value' }); * ``` * + * or using the new properties structure: + * ``` + * const metrics = MetaMetrics.getInstance(); + * metrics.trackEvent(event, { + * properties: {property: 'value' }, + * sensitiveProperties: {sensitiveProperty: 'sensitiveValue' } + * ); + * ``` + * * ## Enabling MetaMetrics * Enable the metrics when user agrees (optin or settings). * ``` @@ -610,6 +619,51 @@ class MetaMetrics implements IMetaMetrics { /** * Track an event * + * The function allows to track non-anonymous and anonymous events: + * - with properties and without properties, + * - with a unique trackEvent function + * + * ## Regular non-anonymous events + * Regular events are tracked with the user ID and can have properties set + * + * ## Anonymous events + * Anonymous tracking track sends two events: one with the anonymous ID and one with the user ID + * - The anonymous event includes sensitive properties so you can know **what** but not **who** + * - The non-anonymous event has either no properties or not sensitive one so you can know **who** but not **what** + * + * @returns MetaMetrics functions + * + * @example basic non-anonymous tracking with no properties: + * trackEvent(MetaMetricsEvents.ONBOARDING_STARTED); + * + * @example track with non-anonymous properties: + * trackEvent(MetaMetricsEvents.BROWSER_SEARCH_USED, { + * option_chosen: 'Browser Bottom Bar Menu', + * number_of_tabs: undefined, + * }); + * + * @example you can also track with non-anonymous properties (new properties structure): + * trackEvent(MetaMetricsEvents.BROWSER_SEARCH_USED, { + * properties: { + * option_chosen: 'Browser Bottom Bar Menu', + * number_of_tabs: undefined, + * }, + * }); + * + * @example track an anonymous event (without properties) + * trackEvent(MetaMetricsEvents.SWAP_COMPLETED); + * + * @example track an anonymous event with properties + * trackEvent(MetaMetricsEvents.GAS_FEES_CHANGED, { + * sensitiveProperties: { ...parameters }, + * }); + * + * @example track an event with both anonymous and non-anonymous properties + * trackEvent(MetaMetricsEvents.MY_EVENT, { + * properties: { ...nonAnonymousParameters }, + * sensitiveProperties: { ...anonymousParameters }, + * }); + * * @param event - Analytics event name * @param properties - Object containing any event relevant traits or properties (optional). * @param saveDataRecording - param to skip saving the data recording flag (optional) From 8d4341faba5669775dd3e93c0b612fe635dd830a Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Fri, 23 Aug 2024 18:05:13 +0200 Subject: [PATCH 13/16] remove tsdoc return --- app/core/Analytics/MetaMetrics.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/core/Analytics/MetaMetrics.ts b/app/core/Analytics/MetaMetrics.ts index 29335e46488..2b2bcf1738d 100644 --- a/app/core/Analytics/MetaMetrics.ts +++ b/app/core/Analytics/MetaMetrics.ts @@ -631,8 +631,6 @@ class MetaMetrics implements IMetaMetrics { * - The anonymous event includes sensitive properties so you can know **what** but not **who** * - The non-anonymous event has either no properties or not sensitive one so you can know **who** but not **what** * - * @returns MetaMetrics functions - * * @example basic non-anonymous tracking with no properties: * trackEvent(MetaMetricsEvents.ONBOARDING_STARTED); * From 866b18bd359133475ef3b8a9a1a752db6e18d63b Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Fri, 23 Aug 2024 18:06:55 +0200 Subject: [PATCH 14/16] update test --- app/components/UI/Ramp/hooks/useAnalytics.test.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/components/UI/Ramp/hooks/useAnalytics.test.ts b/app/components/UI/Ramp/hooks/useAnalytics.test.ts index 9d4a3336a04..27d288e377b 100644 --- a/app/components/UI/Ramp/hooks/useAnalytics.test.ts +++ b/app/components/UI/Ramp/hooks/useAnalytics.test.ts @@ -33,10 +33,8 @@ describe('useAnalytics', () => { expect(MetaMetrics.getInstance().trackEvent).toHaveBeenCalledWith( MetaMetricsEvents[testEvent], { - properties: { - location: 'Amount to Buy Screen', - text: 'Buy', - }, + location: 'Amount to Buy Screen', + text: 'Buy', }, ); }); From 7f1cce6dfb9c1d8e29ce1e5259b3be926008531f Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Fri, 23 Aug 2024 19:51:05 +0200 Subject: [PATCH 15/16] update unit test thanks copilot! --- app/core/Analytics/MetaMetrics.test.ts | 27 ++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/app/core/Analytics/MetaMetrics.test.ts b/app/core/Analytics/MetaMetrics.test.ts index 7a70c5f2ab2..598af6de14f 100644 --- a/app/core/Analytics/MetaMetrics.test.ts +++ b/app/core/Analytics/MetaMetrics.test.ts @@ -179,14 +179,14 @@ describe('MetaMetrics', () => { * - Test D means anonymous tracking (A) and it has both non-anonymous and anonymous props: * The result must be a non-anonymous event with non-anonymous props (NA PROPS) and an anonymous event with all props (NA PROPS + A PROPS). * - * | Test | Track | Non-anon prop | Anon prop | Result non-anon (NA) event | Result anon (A) event | - * |------|---------|---------------|-----------|----------------------------|-----------------------| - * | A | NA | NO | NO | EMPTY | NONE | - * | B | NA | YES | NO | NA PROPS | NONE | - * | C0 | NA | NO | YES(indiv)| EMPTY | A PROPS | - * | C1 | NA | NO | YES(group)| EMPTY | A PROPS | - * | C2 | NA | NO | YES(mixed)| EMPTY | A PROPS | - * | D | NA | YES | YES | NA PROPS | NA PROPS + A PROPS | + * | Test | Non-anon prop | Anon prop | Result non-anon (NA) event | Result anon (A) event | + * |------|---------------|-----------|----------------------------|-----------------------| + * | A | NO | NO | EMPTY | NONE | + * | B | YES | NO | NA PROPS | NONE | + * | C0 | NO | YES(indiv)| EMPTY | A PROPS | + * | C1 | NO | YES(group)| EMPTY | A PROPS | + * | C2 | NO | YES(mixed)| EMPTY | A PROPS | + * | D | YES | YES | NA PROPS | NA PROPS + A PROPS | * * For C0/C1/C2: * - individual prop is one that is mixed with others but is of the form `prop = { anonymous: true, value: 'anon value' }` @@ -198,7 +198,7 @@ describe('MetaMetrics', () => { describe('tracks event', () => { it('without properties (test A)', async () => { const metaMetrics = TestMetaMetrics.getInstance(); - expect(await metaMetrics.configure()).toBeTruthy(); + await metaMetrics.configure(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { category: 'test event' }; @@ -210,14 +210,13 @@ describe('MetaMetrics', () => { // check if the event was tracked expect(segmentMockClient.track).toHaveBeenCalledWith(event.category, { anonymous: false, - ...{}, }); expect(segmentMockClient.track).toHaveBeenCalledTimes(1); }); it('with only non-anonymous properties (test B)', async () => { const metaMetrics = TestMetaMetrics.getInstance(); - expect(await metaMetrics.configure()).toBeTruthy(); + await metaMetrics.configure(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { category: 'test event' }; const nonAnonProp = { non_anon_prop: 'test value' }; @@ -237,6 +236,7 @@ describe('MetaMetrics', () => { it('with only individual anonymous properties (test C0)', async () => { const metaMetrics = TestMetaMetrics.getInstance(); + await metaMetrics.configure(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { category: 'test event' }; @@ -274,6 +274,7 @@ describe('MetaMetrics', () => { it('with only anonymous properties group (test C1)', async () => { const metaMetrics = TestMetaMetrics.getInstance(); + await metaMetrics.configure(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { category: 'test event' }; @@ -306,6 +307,7 @@ describe('MetaMetrics', () => { it('with mixed (group and individual) anonymous properties (test C2)', async () => { const metaMetrics = TestMetaMetrics.getInstance(); + await metaMetrics.configure(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { category: 'test event' }; @@ -345,6 +347,7 @@ describe('MetaMetrics', () => { it('with anonymous and non-anonymous properties (test D)', async () => { const metaMetrics = TestMetaMetrics.getInstance(); + await metaMetrics.configure(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { category: 'test event' }; @@ -390,7 +393,7 @@ describe('MetaMetrics', () => { describe('saveDataRecording', () => { it('tracks event without updating dataRecorded status', async () => { const metaMetrics = TestMetaMetrics.getInstance(); - expect(await metaMetrics.configure()).toBeTruthy(); + await metaMetrics.configure(); await metaMetrics.enable(); const event: IMetaMetricsEvent = { category: 'test event' }; const properties = { regular_prop: 'test value' }; From 64e5a3983fd16977a64e3af5c81d7022121f8f4a Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Mon, 26 Aug 2024 10:08:01 +0200 Subject: [PATCH 16/16] change properties param name add comment format --- app/util/events/convertLegacyProperties.ts | 23 ++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/app/util/events/convertLegacyProperties.ts b/app/util/events/convertLegacyProperties.ts index 74628fd5040..153f1b9aeb9 100644 --- a/app/util/events/convertLegacyProperties.ts +++ b/app/util/events/convertLegacyProperties.ts @@ -17,35 +17,42 @@ function isEventProperties( /** * Convert legacy properties to the new EventProperties type if needed * + * There are two types of legacy properties: + * - properties with the new structure (properties and sensitiveProperties) but with anonymous properties inside properties + * - properties with the old structure (just a JsonMap) and possibly anonymous properties inside + * * If the properties are already of the new type, they are returned as is - * @param properties + * @param propertiesParam the properties to check for conversion and convert if needed */ function convertLegacyProperties( - properties: CombinedProperties, + propertiesParam: CombinedProperties, ): EventProperties { - if (isEventProperties(properties)) { + if (isEventProperties(propertiesParam)) { // EventProperties non-anonymous properties could have anonymous properties inside // so we need to process them separately - if (properties.properties && Object.keys(properties.properties).length) { + if ( + propertiesParam.properties && + Object.keys(propertiesParam.properties).length + ) { const [nonAnonymousProperties, anonymousProperties] = - preProcessAnalyticsEvent(properties.properties); + preProcessAnalyticsEvent(propertiesParam.properties); return { properties: nonAnonymousProperties, // and concatenate all the anon props in sensitiveProperties sensitiveProperties: { ...anonymousProperties, - ...properties.sensitiveProperties, + ...propertiesParam.sensitiveProperties, }, }; } // If there are no non-anonymous properties, we don't need to process them // and we can return the object as is - return properties; + return propertiesParam; } // if the properties are not of the new type, we need to process them const [nonAnonymousProperties, anonymousProperties] = - preProcessAnalyticsEvent(properties); + preProcessAnalyticsEvent(propertiesParam); return { properties: nonAnonymousProperties, sensitiveProperties: anonymousProperties,