Static cohort on person modal #2952

EDsCODE · 2021-01-14T21:19:25Z

Changes

Please describe.

support for trends and stickiness to be able to click create cohort when viewing people
will submit a worker job to async calculate the cohort so that large cohorts to hamper response time
Will create a static cohort extending logic in upload-static-cohort-csv
behind ff 'save-cohort-on-modal'

If this affects the frontend, include screenshots.

Checklist

Clickhouse support
Django backend tests
frontend naming the cohort
closing modal and creating another datapoint cohort without refreshing

…modal

EDsCODE · 2021-01-14T21:20:00Z

frontend/src/scenes/insights/PersonModal.js

+                        <Button type="primary" onClick={() => saveCohortWithFilters()}>
+                            {'Save cohort'}
+                        </Button>
+                    </div>
                    {user?.is_multi_tenancy && featureFlags['filter_by_session_props'] ? (


@macobo put the button here temporarily. i think it should be clear from conflicts

macobo

Added some initial comments but I'm still unsure about the general logic, but might be best to chat about this IRL.

ee/clickhouse/queries/clickhouse_stickiness.py

ee/clickhouse/views/cohort.py

frontend/src/loadPostHogJS.tsx

frontend/src/models/cohortsModel.ts

ee/kafka_client/topics.py

macobo · 2021-02-17T13:57:46Z

frontend/src/scenes/insights/trendsLogic.ts

+                    is_static: true,
+                    name: cohortName,
+                }
+                cohortLogic({


This feels slightly sketchy.

yup. I originally had this in the connect handler, but I think kea typegen doesn't handle logics that are initialized with props in connect yet so it was erroring unless I did this. Need to QA to make sure the logics work as expected here

frontend/src/scenes/insights/PersonModal.js

EDsCODE · 2021-02-18T00:53:46Z

Button could have better placement

macobo · 2021-02-18T15:50:39Z

frontend/src/scenes/insights/PersonModal.js

-                    Found {people.count === 99 ? '99+' : people.count} {people.count === 1 ? 'user' : 'users'}
+                    <div style={{ display: 'flex', flexDirection: 'column', alignItems: 'flex-start' }}>
+                        Found {people.count === 99 ? '99+' : people.count} {people.count === 1 ? 'user' : 'users'}
+                        {(view === ViewType.TRENDS || view === ViewType.STICKINESS) && (


WDYT about putting this behind a feature flag?

Advantage: We can turn it off more easily if/as we discover bugs
Disadvantage: More management work, won't work well with self-hosted.

macobo

Didn't have a chance to test this out - deploy wasn't working? Left some comments, we're not far from merging.

frontend/src/scenes/insights/Insights.js

macobo · 2021-02-18T16:02:12Z

frontend/src/scenes/insights/Insights.js

@@ -328,9 +328,42 @@ function _Insights() {
    )
 }

-function TrendInsight({ view }) {
-    const { filters: _filters, loading, showingPeople } = useValues(trendsLogic({ dashboardItemId: null, view }))
+function SaveCohortModal({ onOk, onCancel, visible }) {


Non-actionable: This is causing some duplication while at the same time missing e.g. validation for cohort name length - might cause pain down the line. I don't think worth abstracting out though, given the "extra" bits in the main modal.

frontend/src/scenes/insights/Insights.js

macobo · 2021-02-18T16:05:17Z

ee/clickhouse/views/cohort.py

+from posthog.tasks.calculate_cohort import insert_cohort_from_query
+
+
+class ClickhouseCohortSerializer(CohortSerializer):


This is missing tests it seems.

Perhaps we can generalize the posthog.api.test.test_cohort and reuse it here?

do you mean the base serializer is missing tests or the clickhousecohortserializer? There's no logic to test in the clickhouseserializer functions as they're essentially just overrides to the base function call

I meant the whole view as a whole - we have tests for /api/cohort for postgres but not for this I think?

ah i see the confusion. So the tests in posthog/api/cohort only have top level testing where they just make sure the celery task is eventually called. Adding this to the ee flow would be redundant as these tests are not testing any specific logic from EE.

The actual ee logic is tested but in different places which makes this slightly confusing. For example, ee/clickhouse/models/test/test_cohort has a test for the insert_users_by_list function and then ee/tasks/test/test_calculate_cohort contains the tests for the newly added functionality in this PR

ee/clickhouse/views/cohort.py

macobo · 2021-02-18T16:07:41Z

ee/clickhouse/views/cohort.py

+        insert_cohort_from_query.delay(cohort.pk, INSIGHT_STICKINESS, filter.to_dict())
+
+    def _handle_trend_people(self, cohort: Cohort, filter: Filter) -> None:
+        if len(filter.entities) >= 1:


I think I need a bit of context for this, code is unclear.

Which entities are we sending over here? Only the one relevant for the modal? If so, there should only ever be exactly one entity right?

If all, then choosing the first seems incorrect and not sure when the else case can happen.

fixed in #3428 could also use a frontend test but I think we should merge the fix first so this PR isn't stuck. I have a log of component tests that I will write separately

ee/clickhouse/views/actions.py

macobo

Things are looking good, we should get this in. There's a test failure though.

Perhaps worth putting the new frontend code behind a feature flag though?

EDsCODE · 2021-02-23T15:09:33Z

feature flag added! test fixed! #3428 should be merged first

timgl and others added 21 commits January 13, 2021 14:20

Upload static cohort using CSV, closes #2868

ff56ded

Merge branch 'master' into upload-static-cohort-csv

ea482b0

fix tests

d18dce9

Fix tests

09a5c71

Fix e2e test

4c3d4e0

Avoid double inserts

def8c4c

Speed up query

e0442e0

Move to params

9945cdc

fix tests

b1f1898

initial cleanup

33c6117

initial scaffold for cohort static logic

a50eff6

intiial frontend changes

cfbde92

initial working

4d1be3d

fix error on iterable

dd51ed5

stickiness people func refactor

ba17d72

Use JSON instead of protobuf

ac56920

stickiness working

3588b69

clickhouse route

8c1c8d4

unused import

e42c069

move button

4eda42f

Merge branch 'upload-static-cohort-csv' into static-cohort-on-person-…

9fe6c1b

…modal

EDsCODE commented Jan 14, 2021

View reviewed changes

timgl temporarily deployed to posthog-static-cohort-o-z5juam January 14, 2021 21:21 Inactive

EDsCODE added 2 commits January 14, 2021 16:44

make interface easier to user

af88564

clickhouse support

d373a6e

EDsCODE temporarily deployed to posthog-static-cohort-o-z5juam January 14, 2021 21:52 Inactive

Base automatically changed from upload-static-cohort-csv to master January 15, 2021 10:19

merge master

47f8310

EDsCODE temporarily deployed to posthog-static-cohort-o-z5juam January 15, 2021 14:27 Inactive

restore cache decorator

38651d2

EDsCODE requested a review from timgl February 16, 2021 19:04

macobo reviewed Feb 17, 2021

View reviewed changes

EDsCODE added 5 commits February 17, 2021 09:55

remove unnecessary kafka topic

245b539

remove brackets

acf7af5

shift default

93a745d

raise exceptions when debug or test is true

8affe17

remove test flag

9786fee

macobo reviewed Feb 18, 2021

View reviewed changes

macobo reviewed Feb 19, 2021

View reviewed changes

EDsCODE mentioned this pull request Feb 22, 2021

Release 1.23.0 parity project #3338

Closed

6 tasks

EDsCODE added 4 commits February 22, 2021 09:53

move trends insight related components to different folder

c540ea8

change get_earliest_timestamp

087dbf8

merge master

787618b

remove extraneous condition

334ed32

macobo reviewed Feb 23, 2021

View reviewed changes

EDsCODE added 4 commits February 23, 2021 09:14

fix test call

ca65f3f

merge master

71712d0

put behind ff

ce546c4

fix test x2

83088c8

EDsCODE added 6 commits February 23, 2021 11:29

merge master

d7bd4c1

fix tests

a504b8c

restore exceptions hog

f78cf3e

fix tests

9547525

restore exceptions hog

7a474bb

fix test x20

92bc6b1

EDsCODE merged commit 9514e29 into master Feb 24, 2021

EDsCODE deleted the static-cohort-on-person-modal branch February 24, 2021 14:59

EDsCODE mentioned this pull request Mar 3, 2021

bump version #3571

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Static cohort on person modal #2952

Static cohort on person modal #2952

EDsCODE commented Jan 14, 2021 •

edited

Loading

EDsCODE Jan 14, 2021

macobo left a comment

macobo Feb 17, 2021

EDsCODE Feb 17, 2021

EDsCODE commented Feb 18, 2021

macobo Feb 18, 2021

macobo left a comment

macobo Feb 18, 2021

macobo Feb 18, 2021

EDsCODE Feb 22, 2021

macobo Feb 23, 2021

EDsCODE Feb 23, 2021 •

edited

Loading

macobo Feb 18, 2021

EDsCODE Feb 22, 2021

macobo left a comment

EDsCODE commented Feb 23, 2021

		from posthog.tasks.calculate_cohort import insert_cohort_from_query


		class ClickhouseCohortSerializer(CohortSerializer):

Static cohort on person modal #2952

Static cohort on person modal #2952

Conversation

EDsCODE commented Jan 14, 2021 • edited Loading

Changes

Checklist

Choose a reason for hiding this comment

macobo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EDsCODE commented Feb 18, 2021

Choose a reason for hiding this comment

macobo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EDsCODE Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macobo left a comment

Choose a reason for hiding this comment

EDsCODE commented Feb 23, 2021

EDsCODE commented Jan 14, 2021 •

edited

Loading

EDsCODE Feb 23, 2021 •

edited

Loading