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(data-exploration): convert funnel correlation to data exploration #14963

Merged
merged 9 commits into from
Apr 6, 2023

Conversation

thmsobrmlr
Copy link
Contributor

@thmsobrmlr thmsobrmlr commented Apr 3, 2023

Overview: Conversion of Funnel Correlation to Data Exploration

Conversion of funnel correlation to data exploration is a wrap!

I've stacked the relevant PRs on top of each other for easier review-ability, but for clicking around feel free to go straight to the last one #14992.

Here's a brief summary of the PRs:

In broad terms, I've split out the correlations related code from funnelLogic into separate logics with a clear purpose:

  • funnelCorrelationLogic: event and event property correlation
  • funnelPropertyCorrelationLogic: property correlation
  • funnelCorrelationDetailsLogic: the details popup
  • funnelCorrelationFeedbackLogic: the feedback form
  • funnelCorrelationUsageLogic: usage tracking with posthog.js

The components and logics then disambiguate between data exploration and legacy mode for everything that is pulled in from the funnel.

I'm going to give this one more review before merging in myself, but the whole thing is ready for review now.


Problem

In order to convert the funnel correlation work to data exploration, the <FunnelCorrelation /> component needed a bit of preparation. I'm going to stack other PRs converting the funnel correlations on top.

Changes

This PR:

  • removes the correlation analysis available feature check, after it became obsolete by style: Replace Ant icons with Lemon ones #13738
  • moves the individual funnel correlation components to their own files, so that they ideally can be converted one-by-one
  • re-factors the "correlation actions cell" into a unified component

How did you test this code?

Manual testing

@thmsobrmlr thmsobrmlr force-pushed the data-exploration-funnel-correlation-1 branch from e34b1d8 to 90df0a0 Compare April 3, 2023 17:39
@thmsobrmlr thmsobrmlr changed the title wip: convert funnel correlation to data exporation feat(data-exploration): prepare funnel correlation conversion Apr 3, 2023
@PostHog PostHog deleted a comment from posthog-bot Apr 3, 2023
@PostHog PostHog deleted a comment from posthog-bot Apr 3, 2023
@thmsobrmlr thmsobrmlr marked this pull request as ready for review April 4, 2023 11:52
@thmsobrmlr thmsobrmlr requested a review from a team April 4, 2023 11:52
@PostHog PostHog deleted a comment from posthog-bot Apr 5, 2023
@PostHog PostHog deleted a comment from posthog-bot Apr 5, 2023
@thmsobrmlr
Copy link
Contributor Author

I don't know why GitHub thinks this should need Django tests.

@thmsobrmlr thmsobrmlr mentioned this pull request Apr 5, 2023
34 tasks
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Everything worked fine when clicking around in the interface. Going through the code now. This chunk seems legit 👍

@thmsobrmlr thmsobrmlr changed the title feat(data-exploration): prepare funnel correlation conversion feat(data-exploration): convert funnel correlation to data exploration Apr 6, 2023
Copy link
Collaborator

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Looks great to me

@@ -30,12 +30,12 @@ import {
import { PathCanvasLabel } from 'scenes/paths/PathsLabel'
import { InsightLegend } from 'lib/components/InsightLegend/InsightLegend'
import { InsightLegendButtonDataExploration } from 'lib/components/InsightLegend/InsightLegendButton'
// import { FunnelCorrelation } from './views/Funnels/FunnelCorrelation'
// import { AlertMessage } from 'lib/lemon-ui/AlertMessage'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems unnecessary too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'll go in again when I implement the isUsingSessionAnalysis flag.

@thmsobrmlr thmsobrmlr merged commit 4cae867 into master Apr 6, 2023
@thmsobrmlr thmsobrmlr deleted the data-exploration-funnel-correlation-1 branch April 6, 2023 21:40
fuziontech added a commit that referenced this pull request Apr 7, 2023
* master:
  fix(flags): don't enclose in overall transaction so we get latest reads (#15003)
  fix(tests): make getEventsByPerson output stable to avoid flakes (#15009)
  feat(data-exploration): convert funnel correlation to data exploration (#14963)
  fix: Set hobby deployments to 'latest' by default (#14956)
  feat(hogql): lambdas (#14987)
  feat(hogql): arrays and tuples (#14986)
  fix(funnel): always use total step count for funnel chart label (#14993)
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.

3 participants