Setup Funnel Option Type, and proper Funnel[Persons] API routes #4946
Conversation
posthog/constants.py
Outdated
| UNORDERED = "unordered" | ||
| TRENDS = "trends" |
There was a problem hiding this comment.
Ordering vs. viz type are kind of orthogonal actually – you can have steps, trends, or time to convert graphs, and each of them can work with regular ordering OR with any ordering. I'm pretty sure we will allow mixing and matching this in the course of nailing funnels, as do other analytics tools, so I suggest to split this into two enums: FunnelOrdering and FunnelVizType.
BTW, is "strict" the best notion for what we have now? For instance Amplitude has a concept of "exact" order, meaning events must happen exactly one after another to count (none in between). This doesn't seem ideal for us (especially considering we do autocapture), but also seems like our way, while "ordered", is not particularly "strict". Feel free to disregard me rambling about this detail though. :D
There was a problem hiding this comment.
This makes A lot of sense! Didn't realise Trends was just a visualisation type.
| @@ -53,6 +54,7 @@ def test_basic_format(self): | |||
| self.assertTrue("id" in j["results"][0] and "name" in j["results"][0] and "distinct_ids" in j["results"][0]) | |||
|
|
|||
| def test_basic_pagination(self): | |||
| cache.clear() | |||
There was a problem hiding this comment.
Didn't set this in setUp, because I want things to be cached around so we know it's working fine generally. (it wasn't, so I fixed it a bit xD )
| ORDERED = "ordered" | ||
|
|
||
|
|
||
| class FunnelVizType(str, Enum): |
There was a problem hiding this comment.
Better names very welcome.
I'm still not quite sure how we settled on ActionsLineGraph display type for Funnel Trends, but I think making it cleaner like this makes sense^ (thanks Twixes!)
neilkakkar
changed the title
| if not filter.funnel_viz_type and filter.display == TRENDS_LINEAR: | ||
| filter.funnel_viz_type = FunnelVizType.TRENDS | ||
|
|
||
| if filter.funnel_viz_type == FunnelVizType.TRENDS: |
There was a problem hiding this comment.
this checking will probably become more general later on, but fine (i think) as is for now.
| class ClickhousePersonViewSet(PersonViewSet): | ||
|
|
||
| lifecycle_class = ClickhouseLifecycle | ||
| retention_class = ClickhouseRetention | ||
| stickiness_class = ClickhouseStickiness | ||
|
|
||
| @action(methods=["GET"], detail=False) | ||
| def funnel(self, request: request.Request, **kwargs) -> response.Response: | ||
| @action(methods=["GET", "POST"], detail=False) |
There was a problem hiding this comment.
POST is important, because GET has issues with the URL being too long (on /insight/funnel/) and persons take all the same parameters!
There was a problem hiding this comment.
I'm tempted to remove GET all together, since this is a new API endpoint.
Changes
WIP, need to heavily test test test for API breakagesAll good.Checklist