Connect funnel persons to the frontend by liyiy · Pull Request #4953 · PostHog/posthog

liyiy · 2021-07-01T00:44:06Z

Changes

Please describe.
If this affects the frontend, include screenshots.

This PR is just to connect the funnel persons api call from #4883 to the frontend
I still have to fix the filters, which I'll do in a separate PR

note: This only works for clickhouse right now and repeated steps aren't working quite 100% yet because the new funnel query has to be implemented first

Screen.Recording.2021-07-02.at.9.40.44.AM.mov

Checklist

All querysets/queries filter by Organization, by Team, and by User
Django backend tests
Jest frontend tests
Cypress end-to-end tests
Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
Frontend/CSS is usable at 320px (iPhone SE) and decent at 360px (most phones)
Breaking changes are backwards-compatible. Ensure old/new frontend requests work with new/old backends, and vice versa.

neilkakkar · 2021-07-01T10:38:51Z

Just a heads up: there's going to be a slight API change here!: #4946

The funnel_order_type and funnel_viz_type query params determine what kind of Funnel you get. (The default is what it is right now, so not a breaking change).

Oh, and also, we switched to POST because we were getting some "url too long" errors, and since Persons expects the same parameters + 1 extra (funnel_step), I've enabled POST for them too. Maybe a good idea to default to using POST here as well?

liyiy · 2021-07-02T13:54:01Z

oops give me one sec to fix the bar click before anyone reviews this 😅

edit: we're good now 😄

liyiy · 2021-07-02T17:45:31Z

ospath module errors :(

mariusandra

I gave a look through the code, but couldn't really test it. Got this:

Is that because I'm running postgres? Checking on my M1 now, so can't really switch...

I found some things to remark on, but nothing serious... except for the feature flag typo.

Once you check these things, someone other than me should probably give this a final review.

mariusandra · 2021-07-02T18:45:29Z

+            trendsLogic().actions.loadPeople(
+                { id: step.action_id, name: step.name, properties: [], type: step.type },
+                `Persons who completed Step #${stepNumber} - "${step.name}"`,
+                '',
+                '',
+                '',
+                true,
+                '',
+                stepNumber
+            )


A nit for now and outside the scope of the PR, but this loadPeople could get a refactor into basically loadPeople(action, options). Having that many '', in a row feels like something could be improved :).

yeah, I've kind of been able to connect the funnel/trends logics together so I think I'll try to refactor this into the personsModalLogic as originally planned, in a separate PR 😃

mariusandra · 2021-07-02T18:47:23Z

    return (
        <Modal
-            title={title}
+            title={<b>{title}</b>}


I think we should prefer <strong> over <b>, <em> over <i>, etc.

mariusandra · 2021-07-02T18:49:10Z

            saveOriginal?: boolean,
-            searchTerm?: string
+            searchTerm?: string,
+            funnel_step?: number


It's a bit of a mess in this action, but normally we should stick to camelCase in JS, except when really and directly dealing with responses from API endpoint that follow different conventions.

Here the funnel_step var starts from the action, so it makes sense to use the JS convention I think.

Yeah, I definitely get confused sometimes when to use camel vs snake case in the frontend because it seems like we use snake case in a lot of places on the frontend so we can pass in params like { funnel_step } instead of { funnel_step: funnelStep }

mariusandra · 2021-07-02T18:53:42Z

        ],
+        funnelPersonsEnabled: [
+            () => [featureFlagLogic.selectors.featureFlags],
+            (featureFlags) => featureFlags[FEATURE_FLAGS.FUNNEL_PERSON_MODAL],


Type here, it should be FUNNEL_PERSONS_MODAL (person+"S").

I think we should remove the explicit Record<string, string> type for the FEATURE_FLAGS object, but again outside the scope here :)

neilkakkar · 2021-07-06T14:48:56Z

Tested locally for the API connections, that seems to be working fine 👍

liyiy · 2021-07-06T16:01:27Z

Anything else I need to update/fix to get this PR in? 😄

samwinslow

Code LGTM although I can't run Clickhouse portions locally. Let's just make sure any UI that depends on CH is not shown for Postgres-only installs. Should the selector for funnelPersonsEnabled also check if Clickhouse is supported?

Are there still styling issues after merging master (from a point after #4965 was merged)? I can help resolve those in another PR if so.

liyiy · 2021-07-06T17:13:15Z

Code LGTM although I can't run Clickhouse portions locally. Let's just make sure any UI that depends on CH is not shown for Postgres-only installs. Should the selector for funnelPersonsEnabled also check if Clickhouse is supported?

Are there still styling issues after merging master (from a point after #4965 was merged)? I can help resolve those in another PR if so.

I skipped it originally because I thought we could just release that funnel persons feature flag to people who had clickhouse enabled

This reverts commit 05418cd.

timgl temporarily deployed to posthog-pr-4953 July 1, 2021 00:46 Inactive

timgl temporarily deployed to posthog-pr-4953 July 1, 2021 17:39 Inactive

liyiy force-pushed the 4819-funnel-persons branch from 390923a to 969e027 Compare July 1, 2021 19:50

timgl temporarily deployed to posthog-pr-4953 July 1, 2021 19:50 Inactive

liyiy added 4 commits July 1, 2021 15:50

connect funnel persons to the frontend

e296bcb

fixes

56b147a

simplify feature flag call and open persons modal on bar click

fa6f129

persons dropoff call

f5c2387

liyiy force-pushed the 4819-funnel-persons branch from 969e027 to f5c2387 Compare July 1, 2021 19:50

timgl temporarily deployed to posthog-pr-4953 July 1, 2021 19:51 Inactive

move methods to kea and clean up code

79662e4

timgl temporarily deployed to posthog-pr-4953 July 2, 2021 00:45 Inactive

liyiy added 2 commits July 1, 2021 21:34

Merge branch 'master' into 4819-funnel-persons

f93df98

remove unused imports

189f462

timgl temporarily deployed to posthog-pr-4953 July 2, 2021 01:35 Inactive

round the percentage down

dd9c62f

timgl temporarily deployed to posthog-pr-4953 July 2, 2021 13:52 Inactive

liyiy requested review from alexkim205 and samwinslow July 2, 2021 13:52

liyiy marked this pull request as ready for review July 2, 2021 13:53

pass in correct step number

5ed7e44

timgl temporarily deployed to posthog-pr-4953 July 2, 2021 13:58 Inactive

type fix

db3de0a

timgl temporarily deployed to posthog-pr-4953 July 2, 2021 14:16 Inactive

Merge branch 'master' into 4819-funnel-persons

97b3ea0

timgl temporarily deployed to posthog-pr-4953 July 2, 2021 14:28 Inactive

liyiy requested a review from EDsCODE July 2, 2021 16:48

liyiy requested a review from mariusandra July 2, 2021 17:45

EDsCODE mentioned this pull request Jul 2, 2021

Replace old funnel step logic #4923

Merged

7 tasks

mariusandra reviewed Jul 2, 2021

View reviewed changes

small fixes

b45bd7e

timgl temporarily deployed to posthog-pr-4953 July 2, 2021 20:37 Inactive

Merge branch 'master' into 4819-funnel-persons

025d8aa

timgl temporarily deployed to posthog-pr-4953 July 6, 2021 13:51 Inactive

prettier

e0fdd02

timgl temporarily deployed to posthog-pr-4953 July 6, 2021 13:58 Inactive

liyiy requested a review from macobo July 6, 2021 15:05

clickhouse enabled check

a6c06ff

timgl temporarily deployed to posthog-pr-4953 July 6, 2021 16:00 Inactive

liyiy enabled auto-merge (squash) July 6, 2021 16:32

liyiy disabled auto-merge July 6, 2021 16:32

samwinslow approved these changes Jul 6, 2021

View reviewed changes

EDsCODE merged commit 05418cd into master Jul 6, 2021

EDsCODE deleted the 4819-funnel-persons branch July 6, 2021 16:51

timgl added a commit that referenced this pull request Jul 6, 2021

Revert "Connect funnel persons to the frontend (#4953)"

d12f861

This reverts commit 05418cd.

timgl mentioned this pull request Jul 6, 2021

Revert "Connect funnel persons to the frontend" #5012

Merged

timgl added a commit that referenced this pull request Jul 6, 2021

Revert "Connect funnel persons to the frontend (#4953)" (#5012)

5299490

This reverts commit 05418cd.

liyiy mentioned this pull request Jul 6, 2021

Connect funnel persons #5013

Merged

6 tasks

Uh oh!

Conversation

liyiy commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Checklist

Uh oh!

neilkakkar commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liyiy commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liyiy commented Jul 2, 2021

Uh oh!

mariusandra left a comment

Choose a reason for hiding this comment

Uh oh!

mariusandra Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

liyiy Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

mariusandra Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

mariusandra Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

liyiy Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

mariusandra Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

neilkakkar commented Jul 6, 2021

Uh oh!

liyiy commented Jul 6, 2021

Uh oh!

samwinslow left a comment

Choose a reason for hiding this comment

Uh oh!

liyiy commented Jul 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

liyiy commented Jul 1, 2021 •

edited

Loading

neilkakkar commented Jul 1, 2021 •

edited

Loading

liyiy commented Jul 2, 2021 •

edited

Loading