Skip to content
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

chore: add personProfiles support #187

Merged
merged 11 commits into from
Sep 10, 2024
Merged

chore: add personProfiles support #187

merged 11 commits into from
Sep 10, 2024

Conversation

marandaneto
Copy link
Member

@marandaneto marandaneto commented Sep 9, 2024

💡 Motivation and Context

Closes #185

When you call identify, alias, or group, if the personProfiles is set to never, the call is NoOp.
If personProfiles is set to always, then $process_person_profile is always true
if personProfiles is set to identified_only, then $process_person_profile is only true is the user is identified.
Otherwise $process_person_profile is always false.

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

@marandaneto marandaneto marked this pull request as ready for review September 9, 2024 15:45
@marandaneto marandaneto requested review from raquelmsmith, a team and robbie-c September 9, 2024 15:45
@marandaneto
Copy link
Member Author

@robbie-c @raquelmsmith can you double-check the implementation? since the JS bits changed with a few PRs in the last couple le months, just need to be sure that this is the correct behavior.

Copy link
Member

@robbie-c robbie-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far, but needs some logic to change the personless mode for alias, set, etc.

The list in posthog-js is identify, setPersonProperties i.e. set, group, setPersonPropertiesForFlags, setGroupPropertiesForFlags, alias, and createPersonProfile.

IMO you don't need to do it in setPersonPropertiesForFlags and setGroupPropertiesForFlags as these are for local evulation and don't touch person profiles, and we should change posthog-js to match this.

createPersonProfile was added to posthog-js to give people control over when a person profile is created, even if they don't have e.g. a user id or an email address to call identify with. It's less important on mobile than it is for web where you might want to make sure someone has a person profile before they change to a different subdomain.

@@ -30,6 +30,8 @@ import Foundation
/// Hook that allows to sanitize the event properties
/// The hook is called before the event is cached or sent over the wire
@objc public var propertiesSanitizer: PostHogPropertiesSanitizer?
/// Determines the behavior for processing user profiles.
@objc public var personProfiles: PostHogPersonProfiles = .identifiedOnly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, In posthog-js the default is currently always, though the intention is to change this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I saw that but the RFC mentions to use the identifiedOnly
https://github.com/PostHog/product-internal/pull/637/files

we should probs update the other stateful SDKs (flutter, iOS, android?) to have the person_profiles config as well, and use the identified_only behavior by default.

So is that OK I guess?

PostHog/PostHogSDK.swift Show resolved Hide resolved
PostHogTests/PostHogSDKTest.swift Outdated Show resolved Hide resolved
@marandaneto
Copy link
Member Author

Looks good so far, but needs some logic to change the personless mode for alias, set, etc.

The list in posthog-js is identify, setPersonProperties i.e. set, group, setPersonPropertiesForFlags, setGroupPropertiesForFlags, alias, and createPersonProfile.

IMO you don't need to do it in setPersonPropertiesForFlags and setGroupPropertiesForFlags as these are for local evulation and don't touch person profiles, and we should change posthog-js to match this.

createPersonProfile was added to posthog-js to give people control over when a person profile is created, even if they don't have e.g. a user id or an email address to call identify with. It's less important on mobile than it is for web where you might want to make sure someone has a person profile before they change to a different subdomain.

On this SDK we only have identify, group, and alias.
We don't have setPersonProperties, setPersonPropertiesForFlags, setGroupPropertiesForFlags and createPersonProfile (no one ever asked so far).
So I don't follow what has to be changed, mind clarifying?

Copy link
Member

@robbie-c robbie-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

(I'm assuming you'll add tests for sending person properties along with a capture)

@marandaneto
Copy link
Member Author

Looks good!

(I'm assuming you'll add tests for sending person properties along with a capture)

Already did, thanks.

@marandaneto marandaneto merged commit 2948e77 into main Sep 10, 2024
6 checks passed
@marandaneto marandaneto deleted the chore/person-profiles branch September 10, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support person_profiles
2 participants