fix(flags): send feature_flag_called events with correct payload da…

…ta in the event that we need to fetch the payloads from the server (#315) * tests for new behavior * refactored code slightly * made the method a bit easier to read * linting * fix * big more * bump version add changelog --------- Co-authored-by: Manoel Aranda Neto <[email protected]>
PostHog · Nov 26, 2024 · 4452a31 · 4452a31
1 parent 8c4961b
commit 4452a31
Show file tree

Hide file tree

Showing 4 changed files with 91 additions and 33 deletions.
diff --git a/posthog-node/CHANGELOG.md b/posthog-node/CHANGELOG.md
@@ -1,5 +1,9 @@
 # Next
 
+# 4.3.1 - 2024-11-26
+
+1. Fix bug where this SDK incorrectly sent `$feature_flag_called` events with null values when using `getFeatureFlagPayload`.
+
 # 4.3.0 - 2024-11-25
 
 1. Add Sentry v8 support to the Sentry integration

diff --git a/posthog-node/package.json b/posthog-node/package.json
@@ -1,6 +1,6 @@
 {
   "name": "posthog-node",
-  "version": "4.3.0",
+  "version": "4.3.1",
   "description": "PostHog Node.js integration",
   "repository": {
     "type": "git",

diff --git a/posthog-node/src/posthog-node.ts b/posthog-node/src/posthog-node.ts
@@ -294,59 +294,80 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 {
       disableGeoip?: boolean
     }
   ): Promise<JsonType | undefined> {
-    const { groups, disableGeoip } = options || {}
-    let { onlyEvaluateLocally, sendFeatureFlagEvents, personProperties, groupProperties } = options || {}
+    const { groups, disableGeoip, onlyEvaluateLocally = false, personProperties, groupProperties } = options || {}
 
-    const adjustedProperties = this.addLocalPersonAndGroupProperties(
+    const { allPersonProperties, allGroupProperties } = this.addLocalPersonAndGroupProperties(
       distinctId,
       groups,
       personProperties,
       groupProperties
     )
 
-    personProperties = adjustedProperties.allPersonProperties
-    groupProperties = adjustedProperties.allGroupProperties
-
-    let response = undefined
-
-    // Try to get match value locally if not provided
-    if (!matchValue) {
+    if (matchValue === undefined) {
       matchValue = await this.getFeatureFlag(key, distinctId, {
         ...options,
         onlyEvaluateLocally: true,
+        sendFeatureFlagEvents: false,
       })
     }
 
-    if (matchValue) {
-      response = await this.featureFlagsPoller?.computeFeatureFlagPayloadLocally(key, matchValue)
-    }
+    let response: string | boolean | undefined
+    let payload: JsonType | undefined
 
-    // set defaults
-    if (onlyEvaluateLocally == undefined) {
-      onlyEvaluateLocally = false
-    }
-    if (sendFeatureFlagEvents == undefined) {
-      sendFeatureFlagEvents = true
+    if (matchValue) {
+      response = matchValue
+      payload = await this.featureFlagsPoller?.computeFeatureFlagPayloadLocally(key, matchValue)
+    } else {
+      response = undefined
+      payload = undefined
     }
 
-    // set defaults
-    if (onlyEvaluateLocally == undefined) {
-      onlyEvaluateLocally = false
-    }
+    // Determine if the payload was evaluated locally
+    const payloadWasLocallyEvaluated = payload !== undefined
 
-    const payloadWasLocallyEvaluated = response !== undefined
+    // Fetch final flags and payloads either locally or from the remote server
+    let fetchedOrLocalFlags: Record<string, string | boolean> | undefined
+    let fetchedOrLocalPayloads: Record<string, JsonType | undefined> | undefined
 
-    if (!payloadWasLocallyEvaluated && !onlyEvaluateLocally) {
-      response = await super.getFeatureFlagPayloadStateless(
-        key,
+    if (payloadWasLocallyEvaluated || onlyEvaluateLocally) {
+      if (response !== undefined) {
+        fetchedOrLocalFlags = { [key]: response }
+        fetchedOrLocalPayloads = { [key]: payload }
+      } else {
+        fetchedOrLocalFlags = {}
+        fetchedOrLocalPayloads = {}
+      }
+    } else {
+      const fetchedData = await super.getFeatureFlagsAndPayloadsStateless(
         distinctId,
         groups,
-        personProperties,
-        groupProperties,
+        allPersonProperties,
+        allGroupProperties,
         disableGeoip
       )
+      fetchedOrLocalFlags = fetchedData.flags || {}
+      fetchedOrLocalPayloads = fetchedData.payloads || {}
     }
-    return response
+
+    const finalResponse = fetchedOrLocalFlags[key]
+    const finalPayload = fetchedOrLocalPayloads[key]
+    const finalLocallyEvaluated = payloadWasLocallyEvaluated
+
+    this.capture({
+      distinctId,
+      event: '$feature_flag_called',
+      properties: {
+        $feature_flag: key,
+        $feature_flag_response: finalResponse,
+        $feature_flag_payload: finalPayload,
+        locally_evaluated: finalLocallyEvaluated,
+        [`$feature/${key}`]: finalResponse,
+      },
+      groups,
+      disableGeoip,
+    })
+
+    return finalPayload
   }
 
   async isFeatureEnabled(

diff --git a/posthog-node/test/posthog-node.spec.ts b/posthog-node/test/posthog-node.spec.ts
@@ -897,13 +897,18 @@ describe('PostHog Node.js', () => {
                   rollout_percentage: 100,
                 },
               ],
+              payloads: { true: { variant: 'A' } },
             },
           },
         ],
       }
 
       mockedFetch.mockImplementation(
-        apiImplementation({ localFlags: flags, decideFlags: { 'decide-flag': 'decide-value' } })
+        apiImplementation({
+          localFlags: flags,
+          decideFlags: { 'decide-flag': 'decide-value' },
+          decideFlagPayloads: { 'beta-feature': { variant: 'A' } },
+        })
       )
 
       posthog = new PostHog('TEST_API_KEY', {
@@ -945,6 +950,34 @@ describe('PostHog Node.js', () => {
       )
       mockedFetch.mockClear()
 
+      expect(
+        await posthog.getFeatureFlagPayload('beta-feature', 'some-distinct-id', undefined, {
+          personProperties: { region: 'USA', name: 'Aloha' },
+        })
+      ).toEqual({ variant: 'A' })
+
+      // TRICKY: There's now an extra step before events are queued, so need to wait for that to resolve
+      jest.runOnlyPendingTimers()
+      await waitForPromises()
+      await posthog.flush()
+
+      expect(mockedFetch).toHaveBeenCalledWith('http://example.com/batch/', expect.any(Object))
+
+      expect(getLastBatchEvents()?.[0]).toEqual(
+        expect.objectContaining({
+          distinct_id: 'some-distinct-id',
+          event: '$feature_flag_called',
+          properties: expect.objectContaining({
+            $feature_flag: 'beta-feature',
+            $feature_flag_response: true,
+            $feature_flag_payload: { variant: 'A' },
+            locally_evaluated: true,
+            [`$feature/${'beta-feature'}`]: true,
+          }),
+        })
+      )
+      mockedFetch.mockClear()
+
       // # called again for same user, shouldn't call capture again
       expect(
         await posthog.getFeatureFlag('beta-feature', 'some-distinct-id', {
@@ -1081,7 +1114,7 @@ describe('PostHog Node.js', () => {
       expect(mockedFetch).toHaveBeenCalledTimes(0)
 
       await expect(posthog.getFeatureFlagPayload('false-flag', '123', true)).resolves.toEqual(300)
-      expect(mockedFetch).toHaveBeenCalledTimes(0)
+      expect(mockedFetch).toHaveBeenCalledTimes(1) // this now calls the server, because in this case the flag is not locally evaluated but we have a payload that we need to calculate
     })
 
     it('should not double parse json with getFeatureFlagPayloads and server eval', async () => {