Page Scroll Goals #5029
Page Scroll Goals #5029
Conversation
test/plausible_web/controllers/api/external_stats_controller/query_test.exs
Outdated
Show resolved
Hide resolved
test/plausible_web/controllers/api/external_stats_controller/query_test.exs
Outdated
Show resolved
Hide resolved
test/plausible_web/controllers/api/stats_controller/conversions_test.exs
Outdated
Show resolved
Hide resolved
macobo
force-pushed
the
page-scroll-goals
branch
2 times, most recently
from
9978623 to
3c63b79
Compare
macobo
force-pushed
the
page-scroll-goals
branch
from
3c63b79 to
7e2e780
Compare
macobo
force-pushed
the
page-scroll-goals
branch
from
da3f88a to
ebf0dfb
Compare
| @@ -130,4 +130,365 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryGoalDimensionTest do | |||
| ] | |||
| end | |||
| end | |||
|
|
|||
| describe "page scroll goals" do | |||
There was a problem hiding this comment.
I think we're missing tests in test/plausible_web/controllers/api/external_stats_controller/query_imported_test.exs for:
- Tests filtering with scroll goals filter
- Tests doing event:goal group by with scroll goals
group_conversion_ratetests
There was a problem hiding this comment.
I think we're still missing a test showing the behavior with imported data + conversion rate + goal group by.
|
|
||
| insert(:goal, site: site, page_path: "/blog**") | ||
|
|
||
| site_import = insert(:site_import, site: site) |
There was a problem hiding this comment.
Redundant code in all the tests - you could do above
test "rejects imported data with warning when page scroll goal is filtered by", %{
conn: conn,
site: site,
site_import: site_import
} do
| } | ||
| } = json_response(conn, 200) | ||
|
|
||
| assert results == [%{"dimensions" => [], "metrics" => [1, 0, 100.0]}] |
There was a problem hiding this comment.
Nitpick: This could be inlined in the previous assert? Or is this intentional?
There was a problem hiding this comment.
This is more explicit. Inlining would be pattern matching which means there could be more keys in the map. Not a huge issue to inline it but not a concern to keep it this way either.
| @@ -130,4 +130,365 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryGoalDimensionTest do | |||
| ] | |||
| end | |||
| end | |||
|
|
|||
| describe "page scroll goals" do | |||
There was a problem hiding this comment.
I think we're still missing a test showing the behavior with imported data + conversion rate + goal group by.
Co-authored-by: Karl-Aksel Puulmann <[email protected]>
@macobo test/plausible_web/controllers/api/external_stats_controller/query_imported_test.exs:1468 is testing it. If you meant to say imported data + group conversion rate + goal group by - that's not possible with imported data, as mixing multiple dimensions will make the query ineligible for imported data. |
|
Required migration steps
Migrating from the old unique index to the new one is a bit tricky, requiring multiple steps:
scroll_thresholdcolumn toGoaland add the new unique index. Page Scroll Goals: migration step 1 #5033Once the old index is dropped, this PR is ready to go.
Changes
This PR implements page scroll goals.
A page scroll goal can be created by simply adding a value into an optional
scroll_thresholdfield when creating a pageview goal:A user can filter and break down page scroll goals like any other goals.
TODO: figure out what to do with the total conversions metric
Tests
Changelog
Documentation
Dark mode