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(data-management): add custom events list #11463

Merged
merged 9 commits into from
Aug 25, 2022
Merged

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Aug 24, 2022

Problem

I'm trying to remove the last references to the "load all data"-powered eventDefinitionsModel.

We have a dropdown for custom events under insights/paths, which uses this model. I want to convert it to a regular paginated taxonomic filter dropdown.

The shortest amount of changes to get this working is to split the event_type used by the event_definitions API endpoint to contain two new sub-types:

export enum CombinedEventType {
    All = 'all',
    ActionEvent = 'action_event',
    Event = 'event',
    EventCustom = 'event_custom', // new
    EventPostHog = 'event_posthog', // new
}

As a side effect, we can now add these two items under data management event list as well:

2022-08-24 14 24 11

Changes

  • Adds two new evert types for event definitions: event_posthog and event_custom, which take all events and either keep or remove those that start with a dollar $.
  • Adds the two new types to the filter on the data management page (do we want this? how to call them?)
  • Makes the custom elements list in the user paths filter use the new endpoint, instead of the model

2022-08-24 14 27 17

How did you test this code?

Wrote tests for the two new filters in the API.

@mariusandra
Copy link
Collaborator Author

If it helps, I'm also happy to hide the UI changes (two new filter options) and split them to a different PR to get the backend stuff merged quicker.

@@ -77,6 +77,7 @@ def get_queryset(self):

params = {
"team_id": self.team_id,
"starts_with_dollar": "$%",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the key name sounds pretty specific. Thoughts on calling this is_posthog_event?

@@ -41,6 +41,18 @@ const eventTypeOptions: LemonSelectOptions<CombinedEventType> = [
icon: <UnverifiedEvent />,
'data-attr': 'event-type-option-event',
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

If its not too much work, it'd be nice to use a divider or indent these sub types since these extra entries could be confusing. Otherwise +1 to hiding this in the UX for this PR and adding in a smaller future one

Copy link
Contributor

@clarkus clarkus left a comment

Choose a reason for hiding this comment

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

Looks like a good change. I think the event option labels would be easier to use if we changed the format of a few. I drew a quick update. Have we flagged these changes for documentation? I can see how users might need more explanation on what each type represents.

What about the other event classifications we've used - specifically verified and raw events. Should those be available here? @alexkim205 thoughts on those other event types?

https://www.figma.com/file/rnaKSVMfWTcpX4cmDbvWsi/Data-Management-%26-Live-Events?node-id=4%3A18853

Screen Shot 2022-08-24 at 3 32 25 PM

@mariusandra mariusandra enabled auto-merge (squash) August 25, 2022 10:34
@mariusandra mariusandra disabled auto-merge August 25, 2022 10:40
@mariusandra
Copy link
Collaborator Author

Thanks @alexkim205 and @clarkus ! I'll split the frontend/product work into a separate PR for another round of feedback, and merge in the ✅ 'd backend changes to unblock #11467

@mariusandra mariusandra dismissed clarkus’s stale review August 25, 2022 10:42

going to move this to a separate PR

@mariusandra mariusandra enabled auto-merge (squash) August 25, 2022 10:42
@mariusandra mariusandra merged commit 7cf3f71 into master Aug 25, 2022
@mariusandra mariusandra deleted the custom-events branch August 25, 2022 11:00
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.

3 participants