Add event exclusion feature to funnels

[alexkim205]

Changes

This PR adds the ability to exclude events between funnel steps.

Closes #5553.
Addresses part of #5367.

• adding some functionality to <ActionFilter/>
• adding new <FunnelExclusionsFilter/>
• consolidated and standardized how we render empty states (loading, error, timeout, and insight-specific empty states like - we have for funnels) across all of our insight graphs. This refactor fixes "There are no matching events for this query" message for a slow-loading funnel is confusing #5584 and a few of other general insight loading bugs we've seen in the past. (see BlockingEmptyState and CoexistingEmptyState in code).
• add warning empty state if exclusion filters are invalid b/c they overlap funnel steps.

Because this is a bit of a chunky PR, I'll CC a few people for focused reviews.

• @clarkus UX
• @macobo / @mariusandra code quality
• @paolodamico product/copy
• @neilkakkar interfacing with the api

Demo

Snip.mov

Horizontal view

Vertical view

Screen.Recording.2021-08-18.at.2.31.08.PM.mov

Exclusion filters are invalid

Screen.Recording.2021-08-23.at.4.30.54.PM.mov

Checklist

• All querysets/queries filter by Organization, by Team, and by User
• Django backend tests
• Jest frontend tests
• Cypress end-to-end tests
• Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
• New/changed UI is decent on smartphones (viewport width around 360px)

[paolodamico]

Great stuff @alexkim205! Some feedback inline and below,

• I can select exclusions within the same step.
  
• When adding a new exclusion step, maybe we should default to Pageview too? Not sure, CC @clarkus
• UI doesn't look great on smaller screens (13" MB), think we could fit the dropdowns in a single line and align the remove button better.
  
• While testing things out I realized that having a hide/disable button could be quite useful as when you apply these filters you want to quickly notice the difference in numbers with and without the filter, could adding this behavior be helpful? CC @clarkus
• Noticed some weird behavior (though not sure if this is Core Analytics scope, CC @neilkakkar). Tested using vanilla demo data. Without the exclusion step my universe is 43 users, yet when I apply the exclusions step, not only are users excluded from converting to step 2, but also they're removed from consideration since step 1.

Without exclusion step

With exclusion step

[alexkim205]

Thanks @paolodamico!

I can select exclusions within the same step.

Fixed!

• UI doesn't look great on smaller screens (13" MB), think we could fit the dropdowns in a single line and align the remove button better.

Hmm, I tested this out initially but the width of the left options space was too small to fit three dropdowns in one single line without hiding all the text in the fields. Perhaps keeping this column format, but expanding the step dropdowns to their full width would fill out the container a bit better? What do you think?

[paolodamico]

Think this definitely looks a lot better! Though I think my suggestion would be:
a) Move the trash icon to the first line (the event/action selector is already wide enough and also pops open)
b) When you have to move the "and" and second step dropdown to a new line, align the second dropdown horizontally to the first dropdown, so they start at the same x coordinate, IMO it'll look cleaner that way.

[clarkus]

a) Move the trash icon to the first line (the event/action selector is already wide enough and also pops open)

I think we should keep it right aligned. That's somewhat of an established pattern in other repeating series we have - tables, filters, etc. If it varies position it's going to cause more confusion than the wrapping rows of elements. I'd rather we drop the arrow icon instead if we're looking for a short term way to save space. I don't think we need to wrap rows. If we drop the arrow icons and optimize for a minimum width of 320px on the container, it will fit.

We could also benefit from some kind of row marker for each distinct exclusion step. We use numbers in the funnel steps, here we could do the same or just use some other indicator.

Longer term, this component would really benefit from some standardization. I've been working on that problem, but I need a bit more time to finish work on some edge cases.

[paolodamico]

Definitely on the right alignment, I meant keeping it to the right but on the same horizontal line as the event/action selector. Dropping the arrow also makes a lot of sense.

[alexkim205]

Thanks for waiting! It took a while to style it like this because Action filter rows are nested deeply inside a component that's used everywhere. I added a new prop called renderRow in <ActionFilter/> that lets you construct a row however you want to, without breaking any existing functionality. Hopefully one day we'll be able to refactor today's ActionFilters into subcomponents for these different layouts but I think this is a step in the right direction for future proofing/standardizing how we use this component today.

I also tweaked our funnel tab breakpoints so that all dropdowns can fit in a single row.

Here's how the new filter looks now:

Screen.Recording.2021-08-19.at.3.27.12.PM.mov

[mariusandra]

Hey, I gave the code a look and played with the feature a bit.

First, is this excluded on postgres?

Second, I think there are ways the data model can be used. Ideally we'd just store everything inside the filter without using any local state: no useState in a component and no extra reducers in a logic. There may be reasons why that's not a good idea (defaults? ux limitations?), and let me know if so.

Third, I'm not sure how it should work :D. I'm locally excluding all users that ever did a pageview between all the pageviews, but no matter what exclusion I apply, nothing changes:

I might be misunderstanding how it works, but I'd expect to see no results with something like this. CC @neilkakkar

[neilkakkar]

@mariusandra thanks for raising the functionality issue. Excluding the same event as the one in the funnel shouldn't be possible! I'll update the backend to return nothing here, but maybe the frontend should warn as well - that this operation doesn't make sense.

Specifically, when you have an exclusion on event A between step X and Y, then the funnel steps from X to Y can't be A.

Edit: Actually, no, I'm thinking I should raise a ValidationError, and you can then display the error message as you see fit?

[mariusandra]

I think there are still outstanding issues from my last review. In addition to that, now that #5668 is in, we should improve this flow somewhat:

It's weird to get such an error during seemingly normal operations. Perhaps leaving the default event for exclusions empty will be enough? Though I'd love to also see the error message somewhere in the interface.

[alexkim205]

Thanks for the review @mariusandra and @neilkakkar for implementing that error. I've updated the PR description w/ my updates!

First, is this excluded on postgres?

Thats a good catch. This is CH exclusive feature so we should protect this behind some flag.

It's weird to get such an error during seemingly normal operations. Perhaps leaving the default event for exclusions empty will be enough? Though I'd love to also see the error message somewhere in the interface.

Gotcha just built out the below warning state for this use case.

Screen.Recording.2021-08-23.at.4.30.54.PM.mov

Other changes made:

• consolidated exclusion filters into filters so that we have one source of truth, and did away with local state.
• consolidated and standardized how we render empty states (loading, error, timeout, and insight-specific empty states like we have for funnels) across all of our insight graphs.

addressed feedback

[mariusandra]

In general this looks good! I can't guarantee I caught all bugs during the review and testing, but did notice that now funnels on dashboards had a small regression. Something to still fix before this is ready.

[alexkim205]

Got some approvals and blocked on some funnel filters work so gonna go ahead and merge this one