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

feat(flags): Add distinct id and group key matching #19902

Merged
merged 6 commits into from
Jan 22, 2024
Merged

Conversation

neilkakkar
Copy link
Collaborator

@neilkakkar neilkakkar commented Jan 22, 2024

Problem

Split out from #19792 -> add group and distinct id targeting to flags

2024-01-19 13 25 02
2024-01-19 13 27 07

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

👀

Copy link
Contributor

github-actions bot commented Jan 22, 2024

Size Change: +244 B (0%)

Total Size: 2 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 2 MB +244 B (0%)

compressed-size-action

@neilkakkar neilkakkar marked this pull request as ready for review January 22, 2024 13:29
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Is it possible to merge metadataTaxonomicGroupToPropertyFilterType and optionsFromProp? So that you'd pass along something like:

const taxonomicOptions: TaxonomicFilterProps['optionsFromProp'] = {
    [TaxonomicFilterGroupType.Metadata]: [{ name: 'distinct_id', return_type: PropertyFilterType.Person }],
}

This would make the metadata field much more meta, and avoid passing through one large and very specific key.

Or is that complicated somehow? 🤔

@neilkakkar
Copy link
Collaborator Author

Slightly complicated, leads to a bigger refactor I'm not yet keen on, because propertyType is determined per taxonomicGroup, not specific selected items inside the group 🫠 -> https://github.com/PostHog/posthog/blob/metadata-flags/frontend/src/lib/components/PropertyFilters/components/taxonomicPropertyFilterLogic.ts#L92

@neilkakkar
Copy link
Collaborator Author

... actually, doesn't seem too bad? Let me try it 👀 , would make things nicer for sure, and prevent the 'metadata needs to be single type' in future problem

@neilkakkar
Copy link
Collaborator Author

Ok, done, this is nice 🙌

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Nice.

Comment on lines +137 to +141
// :TRICKY: When we have a GroupNamesPrefix taxonomic filter, selecting the group name
// is the equivalent of selecting a property value
const property: AnyPropertyFilter = {
key: propertyKey.toString(),
value: null,
key: isGroupNameFilter ? '$group_key' : propertyKey.toString(),
value: isGroupNameFilter ? propertyKey.toString() : null,
Copy link
Collaborator

@mariusandra mariusandra Jan 22, 2024

Choose a reason for hiding this comment

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

This override is the last thing that raises eyebrows... Happy to let is pass though, the impact is very minimal.

The bigger UX sadness here, which can also be for later, is that once you select an organization, the filter says "Group key", not "Organization key".

2024-01-19 13 27 07

The title there can probably be fixed/overridden easily, but up to you if you want to...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Want to keep it is group_key for now, given it's in context of matching org(s), and this is the actual field when, say, calling $groupIdentify(). Will revisit though if this raises confusion

@neilkakkar neilkakkar merged commit 527e7fb into master Jan 22, 2024
98 checks passed
@neilkakkar neilkakkar deleted the metadata-flags branch January 22, 2024 15:31
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.

2 participants