Simplify test accounts #6221

macobo · 2021-10-04T07:02:56Z

This PR introduces the concept of "simplifying" filters. See #5877 for more detail

This is the first part of refactoring for #5854 - I'm planning on introducing "simplification" for cohort filters as well, but want to avoid excessive bloat.

Going forward, all controllers should pass team to filters on creation - we'll eventually make this mandatory as well.

How did you test this code?

Manual testing + existing code coverage

codecov-commenter · 2021-10-04T10:21:02Z

Codecov Report

❗ No coverage uploaded for pull request base (master@25163d0). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #6221   +/-   ##
=========================================
  Coverage          ?   91.76%           
=========================================
  Files             ?      392           
  Lines             ?    29561           
  Branches          ?     2500           
=========================================
  Hits              ?    27126           
  Misses            ?     1907           
  Partials          ?      528

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25163d0...6fe0db3. Read the comment docs.

neilkakkar · 2021-10-07T12:24:58Z

ee/clickhouse/queries/test/test_trends.py

        )
        result = ClickhouseTrends().run(filter, self.team,)
        self.assertEqual(result[0]["count"], 1)
-        result = ClickhouseTrends().run(filter.with_data({"filter_test_accounts": "false"}), self.team,)
+        filter2 = Filter(


optional nit: This test is clearer if you run filter2 first, and then with_data({'filter_test_accounts': 'true'})

neilkakkar · 2021-10-07T12:29:03Z

posthog/models/filters/filter.py

@@ -106,3 +108,7 @@ def __init__(

        self._data = data
        self.kwargs = kwargs
+
+        if "team" in kwargs and not self.is_simplified:


Why do you need to do this here when you're doing it in the BaseFilter? O.o

Ah, is it because this class overrides self._data ? Hmmmm.

Because BaseFilter init is not called here :)

neilkakkar · 2021-10-07T12:41:41Z

posthog/models/filters/mixins/simplify.py

@@ -0,0 +1,28 @@
+from typing import TYPE_CHECKING, TypeVar
+
+if TYPE_CHECKING:  # Avoid circular import


when you already have the type in a quoted string, why do you need to do this?

So mypy actually checks those quoted strings so you need to import them for typechecking to occur 😅

neilkakkar

Some minor comments, but otherwise looks good to me! Great work - really simplifies things around filter test accounts.

EDsCODE

super helpful refactor. filter_test_accounts was weird to work with

timgl temporarily deployed to posthog-pr-6221 October 4, 2021 07:05 Inactive

macobo force-pushed the simplify-test-accounts-2 branch from d6577bf to 21951cf Compare October 4, 2021 07:17

macobo temporarily deployed to posthog-pr-6221 October 4, 2021 07:17 Inactive

macobo force-pushed the simplify-test-accounts-2 branch from 21951cf to e507b54 Compare October 4, 2021 07:22

macobo temporarily deployed to posthog-pr-6221 October 4, 2021 07:22 Inactive

macobo temporarily deployed to posthog-pr-6221 October 4, 2021 07:30 Inactive

macobo force-pushed the simplify-test-accounts-2 branch from 1c59c90 to eee2132 Compare October 4, 2021 07:31

macobo temporarily deployed to posthog-pr-6221 October 4, 2021 07:31 Inactive

macobo force-pushed the simplify-test-accounts-2 branch from eee2132 to a899225 Compare October 4, 2021 07:39

macobo temporarily deployed to posthog-pr-6221 October 4, 2021 07:40 Inactive

macobo had a problem deploying to posthog-pr-6221 October 4, 2021 07:43 Failure

macobo force-pushed the simplify-test-accounts-2 branch from 5119589 to dbfc99e Compare October 4, 2021 07:48

macobo temporarily deployed to posthog-pr-6221 October 4, 2021 07:48 Inactive

macobo force-pushed the simplify-test-accounts-2 branch from dbfc99e to 1ebd13d Compare October 4, 2021 07:49

macobo temporarily deployed to posthog-pr-6221 October 4, 2021 07:49 Inactive

macobo force-pushed the simplify-test-accounts-2 branch from 1ebd13d to 859a5b5 Compare October 4, 2021 08:41

macobo temporarily deployed to posthog-pr-6221 October 4, 2021 08:41 Inactive

macobo temporarily deployed to posthog-pr-6221 October 4, 2021 09:53 Inactive

macobo temporarily deployed to posthog-pr-6221 October 4, 2021 10:04 Inactive

macobo marked this pull request as ready for review October 5, 2021 07:12

macobo force-pushed the simplify-test-accounts-2 branch from 6fe0db3 to f0dd66e Compare October 5, 2021 09:26

macobo temporarily deployed to posthog-pr-6221 October 5, 2021 09:27 Inactive

macobo force-pushed the simplify-test-accounts-2 branch from f0dd66e to ca1232b Compare October 6, 2021 10:25

macobo requested review from EDsCODE and neilkakkar October 6, 2021 10:25

macobo temporarily deployed to posthog-pr-6221 October 6, 2021 10:26 Inactive

macobo force-pushed the simplify-test-accounts-2 branch from ca1232b to d941d6c Compare October 7, 2021 06:17

macobo had a problem deploying to posthog-pr-6221 October 7, 2021 06:17 Failure

macobo temporarily deployed to posthog-pr-6221 October 7, 2021 06:26 Inactive

macobo force-pushed the simplify-test-accounts-2 branch from 92bf70b to 0a495c3 Compare October 7, 2021 08:51

macobo temporarily deployed to posthog-pr-6221 October 7, 2021 08:51 Inactive

macobo mentioned this pull request Oct 7, 2021

Simplify cohort filters #6277

Merged

neilkakkar reviewed Oct 7, 2021

View reviewed changes

neilkakkar approved these changes Oct 7, 2021

View reviewed changes

EDsCODE approved these changes Oct 7, 2021

View reviewed changes

macobo added 10 commits October 7, 2021 22:49

Simplify filters code

c1b5f49

Simplify filters ASAP if filter is created

8e52253

Simplify route

bef66f6

Remove simplification-specific logic from queries

f644c89

Remove recursion, update tests

18392dd

Pass team in more cases

74f992e

Update column optimizer specs

b498fee

Test simplify

bfe625a

Update trends test

c691a37

Fix rebase fail

182837d

macobo force-pushed the simplify-test-accounts-2 branch from 0a495c3 to 182837d Compare October 7, 2021 19:49

macobo temporarily deployed to posthog-pr-6221 October 7, 2021 19:50 Inactive

macobo merged commit ef7f31c into master Oct 7, 2021

macobo deleted the simplify-test-accounts-2 branch October 7, 2021 20:14

macobo mentioned this pull request Oct 15, 2021

Filtering new recordings page by cohorts does not work #6467

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify test accounts #6221

Simplify test accounts #6221

macobo commented Oct 4, 2021 •

edited

Loading

codecov-commenter commented Oct 4, 2021 •

edited

Loading

neilkakkar Oct 7, 2021

neilkakkar Oct 7, 2021

macobo Oct 7, 2021

neilkakkar Oct 7, 2021

macobo Oct 7, 2021

neilkakkar left a comment

EDsCODE left a comment

		@@ -0,0 +1,28 @@
		from typing import TYPE_CHECKING, TypeVar

		if TYPE_CHECKING: # Avoid circular import

Simplify test accounts #6221

Simplify test accounts #6221

Conversation

macobo commented Oct 4, 2021 • edited Loading

How did you test this code?

codecov-commenter commented Oct 4, 2021 • edited Loading

Codecov Report

neilkakkar Oct 7, 2021

Choose a reason for hiding this comment

neilkakkar Oct 7, 2021

Choose a reason for hiding this comment

macobo Oct 7, 2021

Choose a reason for hiding this comment

neilkakkar Oct 7, 2021

Choose a reason for hiding this comment

macobo Oct 7, 2021

Choose a reason for hiding this comment

neilkakkar left a comment

Choose a reason for hiding this comment

EDsCODE left a comment

Choose a reason for hiding this comment

macobo commented Oct 4, 2021 •

edited

Loading

codecov-commenter commented Oct 4, 2021 •

edited

Loading