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

Sessions filtering postgres support #2728

Merged
merged 14 commits into from Dec 11, 2020
Merged

Conversation

macobo
Copy link
Contributor

@macobo macobo commented Dec 10, 2020

Changes

After this:

  • Sessions list logic will be under a separate endpoint - /api/event/sessions
  • Filtering by session recording duration works under postgres

I did not put the offset handling code into this query as it would have become quite expensive. Will handle that in a separate PR.

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests

@timgl timgl temporarily deployed to posthog-sessions-filter-bscbut December 10, 2020 12:34 Inactive
@macobo macobo temporarily deployed to posthog-sessions-filter-bscbut December 10, 2020 12:50 Inactive
Copy link
Member

@EDsCODE EDsCODE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Endpoint swap lgtm. There's a few considerations where we could move the parameter processing/defaults into the dedicated filter but minor for the time being

if not filter._date_from:
filter._date_from = relative_date_parse("-7d")
if not filter._date_to:
filter._date_to = timezone.now()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could move this into SessionFilter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but the annoying part is this requires refactoring python sessions list as well which has subtly different code. Hope it's fine I'll leave it like this for now.

@@ -43,27 +38,9 @@ def trend(self, request: Request, *args: Any, **kwargs: Any) -> Response:

@action(methods=["GET"], detail=False)
def session(self, request: Request, *args: Any, **kwargs: Any) -> Response:
response = ClickhouseSessions().run(team=self.team, filter=Filter(request=request))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This split feels much better haha. There were too many conflicting functions here

Base automatically changed from sessions-filtering-ux-solutions to master December 11, 2020 08:44
@macobo macobo force-pushed the sessions-filtering-postgres-support branch from 4eed4cd to 29cd653 Compare December 11, 2020 08:49
@macobo macobo temporarily deployed to posthog-sessions-filter-bscbut December 11, 2020 08:49 Inactive
@macobo macobo merged commit 3f0f051 into master Dec 11, 2020
@macobo macobo deleted the sessions-filtering-postgres-support branch December 11, 2020 09:01
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.

None yet

3 participants