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

Fix total aggregate values #2723

Merged
merged 14 commits into from
Dec 15, 2020
Merged

Fix total aggregate values #2723

merged 14 commits into from
Dec 15, 2020

Conversation

EDsCODE
Copy link
Member

@EDsCODE EDsCODE commented Dec 10, 2020

Changes

Please describe.

  • addresses Bug/UX issue with table views #2722
  • currently when you use trends and select table or pie chart, it uses the count attribute that we return alongside trend data. This count value is a simple sum over the results in the trend aggregation. If there's a trend graph of active users that's on day interval, the count value will sum the aggregated values for each day but this will double count active users that appeared multiple days in the range. When a pie chart is used to visualize this same data, it will be showing this double counted value.

What's Left:

  • Clickhouse aggregate value for breakdown query
  • Aggregate value tests for breakdown
  • Aggregate value for sessions related queries
  • Frontend Logic swapover (from count to aggregated_value)

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests

@timgl timgl temporarily deployed to posthog-fix-single-aggr-guz7rn December 10, 2020 03:56 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-fix-single-aggr-guz7rn December 10, 2020 04:09 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-fix-single-aggr-guz7rn December 10, 2020 20:33 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-fix-single-aggr-guz7rn December 10, 2020 22:45 Inactive
@EDsCODE EDsCODE marked this pull request as ready for review December 11, 2020 14:33
@EDsCODE
Copy link
Member Author

EDsCODE commented Dec 11, 2020

Leaving out the complete fix for sessions from this PR and will complete after the other session PRs are merged

@EDsCODE EDsCODE requested a review from timgl December 11, 2020 14:34
@timgl
Copy link
Collaborator

timgl commented Dec 14, 2020

Merge conflicts (not sure that file should be committed anyway?)

@EDsCODE EDsCODE temporarily deployed to posthog-fix-single-aggr-guz7rn December 14, 2020 14:16 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-fix-single-aggr-guz7rn December 14, 2020 14:17 Inactive
@EDsCODE EDsCODE requested a review from timgl December 14, 2020 14:58
@EDsCODE EDsCODE temporarily deployed to posthog-fix-single-aggr-guz7rn December 15, 2020 17:08 Inactive
@timgl timgl merged commit 826182b into master Dec 15, 2020
@timgl timgl deleted the fix-single-aggregate-values branch December 15, 2020 17:37
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.

None yet

2 participants