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

Closes #714 allow multiple filters with same key #738

Merged
merged 7 commits into from
May 12, 2020

Conversation

timgl
Copy link
Collaborator

@timgl timgl commented May 8, 2020

Changes

  • Add Property model in array form: [{key, value, operator, type}]
  • Make PropertyFilters use new format
  • Backwards compatibility with the old format
  • Added filtering by person property in backend (not yet in frontend to avoid PR getting too big)

Checklist

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

@timgl timgl temporarily deployed to posthog-714-multiple-fi-zv7dfz May 8, 2020 20:12 Inactive
@timgl timgl requested a review from mariusandra May 11, 2020 11:42
@mariusandra
Copy link
Collaborator

mariusandra commented May 11, 2020

I'm getting an error saving and then loading funnels with this.

AttributeError at /api/funnel/1/ --> 'list' object has no attribute 'items'
  File "/Users/marius/Projects/PostHog/posthog/posthog/utils.py", line 76, in properties_to_Q
    for key, value in properties.items():

The rest seems fine.

And running through all these steps manually makes me wish for integration tests :). We'll miss something one of these days...

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.

So like I wrote in the comment, funnels are broken.

Since funnels store these properties as JSON, all the old funnels still work, but if I save any of them, they will break.

@timgl timgl temporarily deployed to posthog-714-multiple-fi-zv7dfz May 11, 2020 15:10 Inactive
@timgl timgl temporarily deployed to posthog-714-multiple-fi-zv7dfz May 11, 2020 15:16 Inactive
@timgl timgl temporarily deployed to posthog-714-multiple-fi-zv7dfz May 11, 2020 15:20 Inactive
@timgl
Copy link
Collaborator Author

timgl commented May 11, 2020

Should be good now 👍

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.

Works much better with funnels! I just found this issue now when I removed filters:
screencast 2020-05-11 17-37-42
It would save and work fine, just it would show two "new filter" boxes

@timgl timgl temporarily deployed to posthog-714-multiple-fi-zv7dfz May 12, 2020 10:11 Inactive
@timgl
Copy link
Collaborator Author

timgl commented May 12, 2020

good catch, that's fixed now

@timgl timgl temporarily deployed to posthog-714-multiple-fi-zv7dfz May 12, 2020 10:42 Inactive
@timgl timgl merged commit 80484e7 into master May 12, 2020
@timgl timgl deleted the 714-multiple-filters-same-key branch May 12, 2020 12:59
fuziontech added a commit to fuziontech/posthog that referenced this pull request May 14, 2020
* upstream/master:
  Updated Funnels to use antd (PostHog#751)
  Release 1.5.0 (PostHog#762)
  Speed up action/people by prefetching distinct_ids (PostHog#736)
  Fix all time no results (PostHog#725)
  add breakpoint (PostHog#754)
  Add ES Lint (PostHog#719)
  Resize dashboard items (PostHog#746)
  added conditions (PostHog#748)
  fixed undo bug (PostHog#750)
  Event Partitioning (PostHog#733)
  Closes PostHog#714 allow multiple filters with same key (PostHog#738)
  Fix Error404 (PostHog#744)
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