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

Make Breakdown limit customizable and Allow empty breakdown value in trends and funnels #5357

Merged
merged 12 commits into from
Aug 3, 2021

Conversation

neilkakkar
Copy link
Collaborator

@neilkakkar neilkakkar commented Jul 28, 2021

Changes

Adds a customizable limit to the number of breakdown values. Resolves #5341

Also does the same for trends, and changes how none breakdown used to work: Now, none isn't returned if it's empty, and is treated just like any other breakdown value (so works with limit/offset as well)

For the frontend: Since now '' is a valid breakdown value, I update the labels to work with this (and added tests for this)

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
  • New/changed UI is decent on smartphones (viewport width around 360px)

@neilkakkar neilkakkar requested a review from EDsCODE July 28, 2021 13:51
@timgl timgl temporarily deployed to posthog-pr-5357 July 28, 2021 13:53 Inactive
@neilkakkar neilkakkar marked this pull request as draft July 28, 2021 14:22
@timgl timgl temporarily deployed to posthog-pr-5357 July 28, 2021 14:32 Inactive
@timgl timgl temporarily deployed to posthog-pr-5357 July 28, 2021 14:48 Inactive
@timgl timgl temporarily deployed to posthog-pr-5357 July 28, 2021 16:27 Inactive
@timgl timgl temporarily deployed to posthog-pr-5357 July 29, 2021 12:06 Inactive
@neilkakkar neilkakkar marked this pull request as ready for review July 29, 2021 12:21
@timgl timgl temporarily deployed to posthog-pr-5357 July 29, 2021 12:26 Inactive
@neilkakkar neilkakkar changed the title Make Breakdown limit customizable and allow NULLs in Funnels Make Breakdown limit customizable and Allow empty breakdown value in trends and funnels Jul 29, 2021
@neilkakkar
Copy link
Collaborator Author

Labels still broken for Funnels, fixing that

@timgl timgl temporarily deployed to posthog-pr-5357 July 29, 2021 13:41 Inactive
@timgl timgl temporarily deployed to posthog-pr-5357 July 29, 2021 14:10 Inactive
@neilkakkar neilkakkar requested a review from liyiy July 30, 2021 11:33
@@ -51,14 +51,16 @@ export function FunnelStepTable({}: FunnelStepTableProps): JSX.Element | null {
render: function RenderLabel({}, step: FlattenedFunnelStep): JSX.Element {
const isBreakdownChild = !!filters.breakdown && !step.isBreakdownParent
const color = getStepColor(step, !!filters.breakdown)
console.log(step)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops, rm!

@timgl timgl temporarily deployed to posthog-pr-5357 July 30, 2021 11:52 Inactive
@timgl timgl temporarily deployed to posthog-pr-5357 August 2, 2021 10:21 Inactive
Copy link
Member

@EDsCODE EDsCODE left a comment

Choose a reason for hiding this comment

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

backend changes look good

@@ -6,7 +6,6 @@
FROM events e
WHERE
team_id = %(team_id)s {entity_query} {parsed_date_from} {parsed_date_to} {prop_filters}
AND JSONHas(properties, %(key)s)
Copy link
Member

Choose a reason for hiding this comment

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

Oh, was this what was preventing none results from returning? I think that may have led me to implement the workaround for unioning the events with no property

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

correct!

@neilkakkar neilkakkar merged commit a5d9953 into master Aug 3, 2021
@neilkakkar neilkakkar deleted the breakdown_limits branch August 3, 2021 09:59
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.

Inconsistency between breakdown and roll-up of funnel
4 participants