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

feat(insights): add missing filter response for StickinessQueryRunner #24199

Conversation

sanskar-soni-9
Copy link

Problem

Fixes issue #22209 by adding filter in query response

Changes

Added _query_to_filter method in StickinessQueryRunner (Also added TODO comment as similar found in trends_query_runner.py)

Does this work well for both Cloud and self-hosted?

it doesn't have an impact.

How did you test this code?

tested with updated tests

@Twixes Twixes requested a review from a team August 5, 2024 21:51
@thmsobrmlr
Copy link
Contributor

Hey @sanskar-soni-9, thanks for your work here! I'm tempted to merge this as it solves the immediate customer issue. However this adds back usage of legacy filters, that we're trying to get rid of. It would be better if this PR would change the tooltip to use the interval from the query. We seem to do so already for the x-axis on the chart where the dates are correct - maybe you can have a look there and see if a more long term solution is viable?

@sanskar-soni-9 sanskar-soni-9 marked this pull request as draft August 8, 2024 07:24
@sanskar-soni-9 sanskar-soni-9 force-pushed the feat/insights-query-to-filter-for-stickiness branch from 40bbe95 to eb94455 Compare August 8, 2024 10:02
@sanskar-soni-9
Copy link
Author

sanskar-soni-9 commented Aug 8, 2024

@thmsobrmlr As suggested I took the title from x-axis and its working perfectly when we have "no comparison between periods" but in case of comparison its generating wrong date as x-axis is filled with days (ex: [1, 2]) and not labels (ex:. ['1 day', '2 days'])

here are the previews

Without comparison
240808_15h09m50s_screenshot

With comparison
240808_15h12m10s_screenshot

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@thmsobrmlr thmsobrmlr reopened this Sep 2, 2024
@thmsobrmlr thmsobrmlr removed the stale label Sep 2, 2024
@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@thmsobrmlr thmsobrmlr removed the stale label Sep 18, 2024
@thmsobrmlr thmsobrmlr reopened this Sep 18, 2024
@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@posthog-bot posthog-bot closed this Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants