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.

Sign up for GitHub

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

Jump to bottom

Start point for paths #897

Merged
merged 13 commits into from
Jun 1, 2020
Merged

Start point for paths #897

merged 13 commits into from
Jun 1, 2020

Conversation

EDsCODE
Copy link
Member

@EDsCODE EDsCODE commented May 29, 2020 •
edited
Loading

Changes

  • add start point filter for paths
  • refactor propfiltervalue to use hooks
  • prop filter caches results for previous properties it's come across

Todo:

  • add breakpoint to propvalue retrieval/better caching
  • tests

Checklist

  • All querysets/queries filter by Team (if applicable)
  • Backend tests (if applicable)
  • Cypress E2E tests (if applicable)

@timgl timgl temporarily deployed to posthog-start-point-for-be2jld May 29, 2020 16:33 Inactive
@timgl timgl temporarily deployed to posthog-start-point-for-be2jld May 29, 2020 20:00 Inactive
@timgl timgl temporarily deployed to posthog-start-point-for-be2jld May 29, 2020 20:23 Inactive
@timgl timgl temporarily deployed to posthog-start-point-for-be2jld May 29, 2020 20:29 Inactive
@timgl timgl temporarily deployed to posthog-start-point-for-be2jld May 30, 2020 00:18 Inactive
@timgl timgl temporarily deployed to posthog-start-point-for-be2jld May 30, 2020 00:22 Inactive
@EDsCODE EDsCODE mentioned this pull request May 30, 2020
Merged
3 tasks
@timgl timgl temporarily deployed to posthog-start-point-for-be2jld May 31, 2020 00:07 Inactive
@EDsCODE EDsCODE marked this pull request as ready for review May 31, 2020 00:07
EDsCODE
EDsCODE commented May 31, 2020
View reviewed changes
posthog/api/paths.py
return event, path_type, event_filter, start_comparator

@action(methods=['GET'], detail=False)
def elements(self, request: request.Request):
Copy link
Member Author

Choose a reason for hiding this comment

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

added an endpoint off the /paths that will retrieve the top 50 elements that a user can use as a start point. could possibly belong somewhere else but we don't have a dedicated elements endpoint and this is a specific need here

@EDsCODE EDsCODE requested a review from timgl June 1, 2020 01:15
timgl
timgl reviewed Jun 1, 2020
View reviewed changes
Copy link
Collaborator

@timgl timgl left a comment •
edited by EDsCODE
Loading

Choose a reason for hiding this comment

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

This has made paths very useful!

Some small points:
image

  • The hover says undefined -> undefined
  • In my case, there's 2 posthog.com starting points. Looking at what the api returns, this might be an issue in the frontend.

Trying to debug that 2nd issue further, this is what happens when I specify app.posthog.com/trends:
image

Api seems to be correct but there's still irrelevant pages in position 1

image

  • These dropdowns don't seem 100% aligned

@timgl timgl temporarily deployed to posthog-start-point-for-be2jld June 1, 2020 14:39 Inactive
@EDsCODE
Copy link
Member Author

EDsCODE commented Jun 1, 2020

@timgl Ready for another look!

@timgl timgl merged commit 03db444 into master Jun 1, 2020
@timgl timgl deleted the start-point-for-paths branch June 1, 2020 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timgl timgl timgl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@EDsCODE @timgl