Skip to content

Commit a01a3d5

Browse files
authored
fix: send $groupidentify for new groups even without properties (core SDK) (#3320)
1 parent 274632c commit a01a3d5

File tree

5 files changed

+111
-5
lines changed

5 files changed

+111
-5
lines changed

.changeset/gentle-foxes-rest.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@posthog/core': patch
3+
---
4+
5+
fix: send $groupidentify for new groups even when no properties are provided

packages/core/src/__tests__/posthog.featureflags.spec.ts

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -728,8 +728,19 @@ describe('PostHog Feature Flags v4', () => {
728728
it('should reload if groups are set', async () => {
729729
posthog.group('my-group', 'is-great')
730730
await waitForPromises()
731-
expect(mocks.fetch).toHaveBeenCalledTimes(2)
732-
expect(JSON.parse((mocks.fetch.mock.calls[1][1].body as string) || '')).toMatchObject({
731+
// 3 calls: 1 for flags reload, 1 for $groupidentify batch, 1 for flags decide
732+
expect(mocks.fetch).toHaveBeenCalledTimes(3)
733+
// The flags reload call contains the group
734+
const flagsCall = mocks.fetch.mock.calls.find((call) => {
735+
try {
736+
const body = JSON.parse((call[1].body as string) || '')
737+
return body.groups?.['my-group'] === 'is-great'
738+
} catch {
739+
return false
740+
}
741+
})
742+
expect(flagsCall).toBeDefined()
743+
expect(JSON.parse((flagsCall![1].body as string) || '')).toMatchObject({
733744
groups: { 'my-group': 'is-great' },
734745
})
735746
})

packages/core/src/__tests__/posthog.featureflags.v1.spec.ts

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -411,8 +411,19 @@ describe('PostHog Feature Flags v1', () => {
411411
it('should reload if groups are set', async () => {
412412
posthog.group('my-group', 'is-great')
413413
await waitForPromises()
414-
expect(mocks.fetch).toHaveBeenCalledTimes(2)
415-
expect(JSON.parse((mocks.fetch.mock.calls[1][1].body as string) || '')).toMatchObject({
414+
// 3 calls: 1 for flags reload, 1 for $groupidentify batch, 1 for flags decide
415+
expect(mocks.fetch).toHaveBeenCalledTimes(3)
416+
// The flags reload call contains the group
417+
const flagsCall = mocks.fetch.mock.calls.find((call) => {
418+
try {
419+
const body = JSON.parse((call[1].body as string) || '')
420+
return body.groups?.['my-group'] === 'is-great'
421+
} catch {
422+
return false
423+
}
424+
})
425+
expect(flagsCall).toBeDefined()
426+
expect(JSON.parse((flagsCall![1].body as string) || '')).toMatchObject({
416427
groups: { 'my-group': 'is-great' },
417428
})
418429
})

packages/core/src/__tests__/posthog.groups.spec.ts

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,79 @@ describe('PostHog Core', () => {
6464
],
6565
})
6666
})
67+
68+
it('should call groupIdentify for a new group even without properties', async () => {
69+
posthog.group('other', 'team')
70+
await waitForPromises()
71+
72+
expect(mocks.fetch).toHaveBeenCalledTimes(2) // 1 for flags, 1 for groupIdentify
73+
const batchCall = mocks.fetch.mock.calls[1]
74+
expect(batchCall[0]).toEqual('https://us.i.posthog.com/batch/')
75+
expect(parseBody(batchCall)).toMatchObject({
76+
batch: [
77+
{
78+
event: '$groupidentify',
79+
distinct_id: posthog.getDistinctId(),
80+
properties: {
81+
$group_type: 'other',
82+
$group_key: 'team',
83+
},
84+
type: 'capture',
85+
},
86+
],
87+
})
88+
})
89+
90+
it('should not call groupIdentify when group already exists with same key and no properties', async () => {
91+
posthog.group('other', 'team')
92+
await waitForPromises()
93+
mocks.fetch.mockClear()
94+
95+
posthog.group('other', 'team')
96+
await waitForPromises()
97+
98+
// No new fetch calls for groupIdentify (only flags reload if needed)
99+
const groupIdentifyCalls = mocks.fetch.mock.calls.filter((call) => {
100+
try {
101+
const body = parseBody(call)
102+
return body.batch?.some((e: any) => e.event === '$groupidentify')
103+
} catch {
104+
return false
105+
}
106+
})
107+
expect(groupIdentifyCalls).toHaveLength(0)
108+
})
109+
110+
it('should call groupIdentify for an existing group when properties are provided', async () => {
111+
posthog.group('other', 'team')
112+
await waitForPromises()
113+
mocks.fetch.mockClear()
114+
115+
posthog.group('other', 'team', { name: 'My Team' })
116+
await waitForPromises()
117+
118+
const groupIdentifyCalls = mocks.fetch.mock.calls.filter((call) => {
119+
try {
120+
const body = parseBody(call)
121+
return body.batch?.some((e: any) => e.event === '$groupidentify')
122+
} catch {
123+
return false
124+
}
125+
})
126+
expect(groupIdentifyCalls).toHaveLength(1)
127+
expect(parseBody(groupIdentifyCalls[0])).toMatchObject({
128+
batch: [
129+
{
130+
event: '$groupidentify',
131+
properties: {
132+
$group_type: 'other',
133+
$group_key: 'team',
134+
$group_set: { name: 'My Team' },
135+
},
136+
},
137+
],
138+
})
139+
})
67140
})
68141

69142
describe('groupIdentify', () => {

packages/core/src/posthog-core.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -443,11 +443,17 @@ export abstract class PostHogCore extends PostHogCoreStateless {
443443
options?: PostHogCaptureOptions
444444
): void {
445445
this.wrap(() => {
446+
const existingGroups = (this.props.$groups as PostHogGroupProperties) || {}
447+
const isNewGroup = existingGroups[groupType] !== groupKey
448+
446449
this.groups({
447450
[groupType]: groupKey,
448451
})
449452

450-
if (groupProperties) {
453+
// Send $groupidentify when the group is new/changed OR when properties
454+
// are provided. Skip only when the group already exists with the same
455+
// key and no new properties are being set.
456+
if (isNewGroup || groupProperties) {
451457
this.groupIdentify(groupType, groupKey, groupProperties, options)
452458
}
453459
})

0 commit comments

Comments
 (0)