Skip to content

fix(browser): gate group() on person processing #4713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
posthog wants to merge 1 commit into
base: main
Choose a base branch
from
+17 −10
Open

fix(browser): gate group() on person processing #4713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/gate-group-on-person-processing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'posthog-js': patch
---

Gate `group()` on person processing, like the other identity methods. Under `identified_only` a `group()` call now promotes the user to identified, so the `$groupidentify` event is kept instead of dropped. Under `never` the call is ignored and logs an error, instead of sending an event the server always drops.
18 changes: 8 additions & 10 deletions packages/browser/src/__tests__/personProcessing.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,8 +606,9 @@ describe('person processing', () => {
expect(eventAfterGroup[0].properties.$process_person_profile).toEqual(true)
})

it('should send the $groupidentify event even if person_processing is set to never', async () => {
// Groups are separate from person processing - $groupidentify should always be sent
it('should be ignored and log an error if person_processing is set to never', async () => {
// $groupidentify needs person processing, so the plugin server drops it when
// person processing is off. Gate group() like the other identity methods.
// arrange
const { posthog, beforeSendMock } = await setup('never')

Expand All @@ -617,20 +618,17 @@ describe('person processing', () => {
posthog.capture('custom event after group')

// assert
// setGroupPropertiesForFlags still has a person processing check
expect(mockLogger.error).toBeCalledTimes(1)
expect(mockLogger.error).toHaveBeenCalledWith(
'posthog.setGroupPropertiesForFlags was called, but process_person is set to "never". This call will be ignored.'
'posthog.group was called, but process_person is set to "never". This call will be ignored.'
)

// $groupidentify is sent (groups are independent of person processing)
expect(beforeSendMock).toBeCalledTimes(3)
// no $groupidentify is sent, only the two custom events
expect(beforeSendMock).toBeCalledTimes(2)
const eventBeforeGroup = beforeSendMock.mock.calls[0]
expect(eventBeforeGroup[0].properties.$process_person_profile).toEqual(false)
const groupIdentify = beforeSendMock.mock.calls[1]
expect(groupIdentify[0].event).toEqual('$groupidentify')
// $groupidentify doesn't set $process_person_profile since it doesn't process persons
const eventAfterGroup = beforeSendMock.mock.calls[2]
const eventAfterGroup = beforeSendMock.mock.calls[1]
expect(eventAfterGroup[0].event).toEqual('custom event after group')
expect(eventAfterGroup[0].properties.$process_person_profile).toEqual(false)
})
})
Expand Down
4 changes: 4 additions & 0 deletions packages/browser/src/posthog-core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3195,6 +3195,10 @@ export class PostHog implements PostHogInterface {
return
}

if (!this._requirePersonProcessing('posthog.group')) {
return
}

// Apply a sibling reset before reading or writing groups so this explicit
// mutation is newer than the adopted cookie snapshot.
this.persistence?.syncCookieProperties()
Expand Down