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

fixed type error on breakdown filter #758

Merged
merged 8 commits into from
May 14, 2020
Merged

fixed type error on breakdown filter #758

merged 8 commits into from
May 14, 2020

Conversation

EDsCODE
Copy link
Member

@EDsCODE EDsCODE commented May 13, 2020

Addresses #759

Changes

  • fixed numeric breakdown values not working

Checklist

  • All querysets/queries filter by Team (if applicable)
  • Backend tests (if applicable)

@EDsCODE EDsCODE requested a review from timgl May 13, 2020 15:57
@timgl timgl temporarily deployed to posthog-breakdown-filte-lpjmmw May 13, 2020 15:58 Inactive
@EDsCODE EDsCODE removed the request for review from timgl May 13, 2020 16:20
@EDsCODE EDsCODE marked this pull request as draft May 13, 2020 16:20
@timgl timgl temporarily deployed to posthog-breakdown-filte-lpjmmw May 13, 2020 17:11 Inactive
@EDsCODE
Copy link
Member Author

EDsCODE commented May 13, 2020

@timgl This should solve the issue but I found something funny that we should address. When you breakdown by a property, are the leftovers (anything that doesn't have the property) supposed to be displayed?

Right now, if you breakdown by browser_version it will give you the line with the browser version but also a line with 'nan'. Also, when clicking on the points of the 'nan' line even if the datapoint is 0, a list of users will be query-able.

@timgl timgl temporarily deployed to posthog-breakdown-filte-lpjmmw May 13, 2020 17:16 Inactive
@timgl
Copy link
Collaborator

timgl commented May 13, 2020

i don't have a strong opinion either way. I think it can be useful to have the non-set line too so if it's easy to make it work.

Re the 0 being clickable, it just shows '0 users found' so I think that's fine. I did find a bug where setting properties on entities doesn't correctly filter /people, will create a ticket for that

@timgl timgl temporarily deployed to posthog-breakdown-filte-lpjmmw May 14, 2020 15:09 Inactive
@EDsCODE
Copy link
Member Author

EDsCODE commented May 14, 2020

Fixed! The 0 person error was happening on numerical breakdowns also. So I was seeing a line with 0 but when you clicked on a datapoint you'd see actual people related to the event. The problem was that when something was nan we were tossing the results rather than aggregating them. I changed it so that it treats nan and nones as strings and become a breakdown category which will be displayed as "other" in the label

@EDsCODE EDsCODE marked this pull request as ready for review May 14, 2020 15:21
@EDsCODE EDsCODE requested a review from timgl May 14, 2020 15:21
posthog/api/action.py Outdated Show resolved Hide resolved
@timgl timgl temporarily deployed to posthog-breakdown-filte-lpjmmw May 14, 2020 15:27 Inactive
@timgl timgl temporarily deployed to posthog-breakdown-filte-lpjmmw May 14, 2020 15:35 Inactive
@timgl timgl merged commit ba37c57 into master May 14, 2020
@timgl timgl deleted the breakdown-filter-error branch May 14, 2020 15:43
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