-
Notifications
You must be signed in to change notification settings - Fork 70
feat: add groups parameter to capture() for event-level group association #248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,10 +89,31 @@ class PosthogFlutterWeb extends PosthogFlutterPlatformInterface { | |
| Future<void> capture({ | ||
| required String eventName, | ||
| Map<String, Object>? properties, | ||
| Map<String, Object>? groups, | ||
| }) async { | ||
| Map<String, Object>? mergedProperties = | ||
| properties == null ? null : {...properties}; | ||
|
|
||
| if (groups != null && groups.isNotEmpty) { | ||
| mergedProperties ??= <String, Object>{}; | ||
| final existingGroups = mergedProperties['\$groups']; | ||
| if (existingGroups is Map) { | ||
| mergedProperties['\$groups'] = { | ||
| ...Map<String, Object>.from( | ||
| existingGroups.map( | ||
| (key, value) => MapEntry(key.toString(), value as Object), | ||
|
Comment on lines
+102
to
+104
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you dont need this if we make |
||
| ), | ||
| ), | ||
| ...groups, | ||
| }; | ||
| } else { | ||
| mergedProperties['\$groups'] = groups; | ||
| } | ||
| } | ||
|
|
||
| return handleWebMethodCall(MethodCall('capture', { | ||
| 'eventName': eventName, | ||
| if (properties != null) 'properties': properties, | ||
| if (mergedProperties != null) 'properties': mergedProperties, | ||
| })); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,18 +83,34 @@ class Posthog { | |
| Future<void> capture({ | ||
| required String eventName, | ||
| Map<String, Object>? properties, | ||
| /// Event-level group context. | ||
| /// | ||
| /// This associates *only this event* with the provided groups, without | ||
| /// persisting the group mapping for future events (unlike `group()`). | ||
| /// | ||
| /// On iOS/Android, this is passed to the native SDK's `groups` parameter | ||
| /// which properly merges with any sticky groups set via `group()`. | ||
| Map<String, Object>? groups, | ||
| }) { | ||
| final propertiesCopy = properties == null ? null : {...properties}; | ||
|
|
||
| final currentScreen = _currentScreen; | ||
| if (propertiesCopy != null && | ||
| !propertiesCopy.containsKey('\$screen_name') && | ||
| currentScreen != null) { | ||
| propertiesCopy['\$screen_name'] = currentScreen; | ||
| if (currentScreen != null) { | ||
| final props = propertiesCopy ?? <String, Object>{}; | ||
| if (!props.containsKey('\$screen_name')) { | ||
| props['\$screen_name'] = currentScreen; | ||
| } | ||
| return _posthog.capture( | ||
| eventName: eventName, | ||
| properties: props, | ||
| groups: groups, | ||
| ); | ||
|
Comment on lines
+103
to
+107
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not needed since it will return below anyway |
||
| } | ||
|
|
||
| return _posthog.capture( | ||
| eventName: eventName, | ||
| properties: propertiesCopy, | ||
| groups: groups, | ||
| ); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import 'util/platform_io_stub.dart' | |
| if (dart.library.io) 'util/platform_io_real.dart'; | ||
|
|
||
| import 'package:flutter/services.dart'; | ||
| import 'package:meta/meta.dart'; | ||
|
|
||
| import 'package:posthog_flutter/src/surveys/survey_service.dart'; | ||
| import 'package:posthog_flutter/src/util/logging.dart'; | ||
|
|
@@ -21,6 +22,11 @@ class PosthogFlutterIO extends PosthogFlutterPlatformInterface { | |
| _methodChannel.setMethodCallHandler(_handleMethodCall); | ||
| } | ||
|
|
||
| @visibleForTesting | ||
| Future<dynamic> handleMethodCallForTest(MethodCall call) { | ||
| return _handleMethodCall(call); | ||
| } | ||
|
|
||
| /// The method channel used to interact with the native platform. | ||
| final _methodChannel = const MethodChannel('posthog_flutter'); | ||
|
|
||
|
|
@@ -175,6 +181,7 @@ class PosthogFlutterIO extends PosthogFlutterPlatformInterface { | |
| Future<void> capture({ | ||
| required String eventName, | ||
| Map<String, Object>? properties, | ||
| Map<String, Object>? groups, | ||
| }) async { | ||
| if (!isSupportedPlatform()) { | ||
| return; | ||
|
|
@@ -184,9 +191,18 @@ class PosthogFlutterIO extends PosthogFlutterPlatformInterface { | |
| final normalizedProperties = | ||
| properties != null ? PropertyNormalizer.normalize(properties) : null; | ||
|
|
||
| // Convert groups to Map<String, String> for native SDK compatibility | ||
| Map<String, String>? normalizedGroups; | ||
| if (groups != null && groups.isNotEmpty) { | ||
| normalizedGroups = groups.map( | ||
| (key, value) => MapEntry(key, value.toString()), | ||
| ); | ||
| } | ||
|
Comment on lines
+194
to
+200
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| await _methodChannel.invokeMethod('capture', { | ||
| 'eventName': eventName, | ||
| if (normalizedProperties != null) 'properties': normalizedProperties, | ||
| if (normalizedGroups != null) 'groups': normalizedGroups, | ||
| }); | ||
| } on PlatformException catch (exception) { | ||
| printIfDebug('Exeption on capture: $exception'); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ import 'package:flutter/services.dart'; | |||
| import 'package:flutter_test/flutter_test.dart'; | ||||
| import 'package:posthog_flutter/src/posthog_config.dart'; | ||||
| import 'package:posthog_flutter/src/posthog_flutter_io.dart'; | ||||
| import 'dart:io' show Platform; | ||||
|
|
||||
| // Simplified void callback for feature flags | ||||
| void emptyCallback() {} | ||||
|
|
@@ -40,6 +41,8 @@ void main() { | |||
| }); | ||||
|
|
||||
| group('PosthogFlutterIO onFeatureFlags via setup', () { | ||||
| final bool isUnsupportedPlatform = Platform.isLinux || Platform.isWindows; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not needed, the tests should run on any machine posthog-flutter/.github/workflows/ci.yml Line 39 in c95d00e
those tests wont run on CI, so we should mock results if needed |
||||
|
|
||||
| test( | ||||
| 'setup initializes method call handler and registers callback if provided', | ||||
| () async { | ||||
|
|
@@ -51,17 +54,12 @@ void main() { | |||
| testConfig = PostHogConfig('test_api_key', onFeatureFlags: testCallback); | ||||
| await posthogFlutterIO.setup(testConfig); | ||||
|
|
||||
| // To verify handler is set, we trigger the callback from native side | ||||
| await TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger | ||||
| .handlePlatformMessage( | ||||
| channel.name, | ||||
| channel.codec | ||||
| .encodeMethodCall(const MethodCall('onFeatureFlagsCallback', {})), | ||||
| (ByteData? data) {}, | ||||
| ); | ||||
| // Trigger the callback using the IO implementation's test hook. | ||||
| await posthogFlutterIO.handleMethodCallForTest( | ||||
| const MethodCall('onFeatureFlagsCallback', {})); | ||||
| expect(callbackInvoked, isTrue); | ||||
| expect(log.any((call) => call.method == 'setup'), isTrue); | ||||
| }); | ||||
| }, skip: isUnsupportedPlatform); | ||||
|
|
||||
| test('invokes callback when native sends onFeatureFlagsCallback event', | ||||
| () async { | ||||
|
|
@@ -76,17 +74,11 @@ void main() { | |||
|
|
||||
| // Native sends empty map (iOS/Android behavior) | ||||
| final mockNativeArgs = <String, dynamic>{}; | ||||
|
|
||||
| await TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger | ||||
| .handlePlatformMessage( | ||||
| channel.name, | ||||
| channel.codec.encodeMethodCall( | ||||
| MethodCall('onFeatureFlagsCallback', mockNativeArgs)), | ||||
| (ByteData? data) {}, | ||||
| ); | ||||
| await posthogFlutterIO.handleMethodCallForTest( | ||||
| MethodCall('onFeatureFlagsCallback', mockNativeArgs)); | ||||
|
|
||||
| expect(callbackInvoked, isTrue); | ||||
| }); | ||||
| }, skip: isUnsupportedPlatform); | ||||
|
|
||||
| test( | ||||
| 'invokes callback when native sends onFeatureFlagsCallback with empty map (mobile behavior)', | ||||
|
|
@@ -103,16 +95,11 @@ void main() { | |||
| // Simulate mobile sending an empty map | ||||
| final mockNativeArgs = <String, dynamic>{}; | ||||
|
|
||||
| await TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger | ||||
| .handlePlatformMessage( | ||||
| channel.name, | ||||
| channel.codec.encodeMethodCall( | ||||
| MethodCall('onFeatureFlagsCallback', mockNativeArgs)), | ||||
| (ByteData? data) {}, | ||||
| ); | ||||
| await posthogFlutterIO.handleMethodCallForTest( | ||||
| MethodCall('onFeatureFlagsCallback', mockNativeArgs)); | ||||
|
|
||||
| expect(callbackInvoked, isTrue); | ||||
| }); | ||||
| }, skip: isUnsupportedPlatform); | ||||
|
|
||||
| test('invokes callback even with malformed native args', () async { | ||||
| bool callbackInvoked = false; | ||||
|
|
@@ -129,30 +116,20 @@ void main() { | |||
| 'flags': 123, // Invalid type, but callback is void so it doesn't matter | ||||
| }; | ||||
|
|
||||
| await TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger | ||||
| .handlePlatformMessage( | ||||
| channel.name, | ||||
| channel.codec.encodeMethodCall( | ||||
| MethodCall('onFeatureFlagsCallback', mockNativeArgsMalformed)), | ||||
| (ByteData? data) {}, | ||||
| ); | ||||
| await posthogFlutterIO.handleMethodCallForTest( | ||||
| MethodCall('onFeatureFlagsCallback', mockNativeArgsMalformed)); | ||||
|
|
||||
| expect(callbackInvoked, isTrue); | ||||
| }); | ||||
| }, skip: isUnsupportedPlatform); | ||||
|
|
||||
| test('does not invoke callback when no callback is registered', () async { | ||||
| // Setup without callback | ||||
| testConfig = PostHogConfig('test_api_key'); | ||||
| await posthogFlutterIO.setup(testConfig); | ||||
|
|
||||
| // This should not throw - just silently do nothing | ||||
| await TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger | ||||
| .handlePlatformMessage( | ||||
| channel.name, | ||||
| channel.codec | ||||
| .encodeMethodCall(const MethodCall('onFeatureFlagsCallback', {})), | ||||
| (ByteData? data) {}, | ||||
| ); | ||||
| await posthogFlutterIO.handleMethodCallForTest( | ||||
| const MethodCall('onFeatureFlagsCallback', {})); | ||||
|
|
||||
| // If we get here without exception, the test passes | ||||
| expect(true, isTrue); | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,5 +31,56 @@ void main() { | |
| expect(fakePlatformInterface.registeredOnFeatureFlagsCallback, | ||
| equals(testCallback)); | ||
| }); | ||
|
|
||
| test('capture supports event-level groups via \$groups', () async { | ||
| await Posthog().capture( | ||
| eventName: 'thing_happened', | ||
| groups: { | ||
| 'company': 'c_123', | ||
| 'project': 42, | ||
| }, | ||
| ); | ||
|
|
||
| expect(fakePlatformInterface.capturedEvents, hasLength(1)); | ||
| final call = fakePlatformInterface.capturedEvents.single; | ||
| expect(call.eventName, equals('thing_happened')); | ||
|
|
||
| // groups are now passed as a separate parameter (not embedded in properties) | ||
| final groups = call.groups; | ||
| expect(groups, isNotNull); | ||
| expect(groups, equals({'company': 'c_123', 'project': 42})); | ||
| }); | ||
|
|
||
| test('capture passes groups separately from properties', () async { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this test is the same as the above, the only difference is that you pass |
||
| await Posthog().capture( | ||
| eventName: 'merged_groups', | ||
| properties: { | ||
| 'some_prop': 'value', | ||
| }, | ||
| groups: { | ||
| 'company': 'c_new', | ||
| 'project': 'p_9', | ||
| }, | ||
| ); | ||
|
|
||
| final call = fakePlatformInterface.capturedEvents.single; | ||
| // properties should not contain $groups anymore | ||
| expect(call.properties?['some_prop'], equals('value')); | ||
| // groups passed separately | ||
| expect(call.groups, equals({'company': 'c_new', 'project': 'p_9'})); | ||
| }); | ||
|
|
||
| test('capture adds \$screen_name when groups provided', () async { | ||
| await Posthog().screen(screenName: 'Checkout'); | ||
|
|
||
| await Posthog().capture( | ||
| eventName: 'purchase_clicked', | ||
| groups: {'project': 'p1'}, | ||
| ); | ||
|
|
||
| final call = fakePlatformInterface.capturedEvents.last; | ||
| expect(call.properties?['\$screen_name'], equals('Checkout')); | ||
| expect(call.groups, equals({'project': 'p1'})); | ||
| }); | ||
| }); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on Android and iOS:
groups: Map<String, String>, so lets make it the same instead ofString, Object, double check the rest of the PR