From ecba7d7610855e273c8603d8939495d02e7431a9 Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Thu, 31 Oct 2024 12:09:21 +0100 Subject: [PATCH 01/26] migration: add scroll_threshold to goals --- ...28161815_add_scroll_threshold_to_goals.exs | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 priv/repo/migrations/20250128161815_add_scroll_threshold_to_goals.exs diff --git a/priv/repo/migrations/20250128161815_add_scroll_threshold_to_goals.exs b/priv/repo/migrations/20250128161815_add_scroll_threshold_to_goals.exs new file mode 100644 index 000000000000..935f2919e8db --- /dev/null +++ b/priv/repo/migrations/20250128161815_add_scroll_threshold_to_goals.exs @@ -0,0 +1,39 @@ +defmodule Plausible.Repo.Migrations.AddScrollThresholdToGoals do + use Ecto.Migration + + @disable_ddl_transaction true + @disable_migration_lock true + + # Plausible.Repo.Migrations.GoalsUnique + @old_index unique_index( + :goals, + [:site_id, :page_path], + where: "page_path IS NOT NULL", + name: :goals_page_path_unique + ) + + @new_index unique_index( + :goals, + [:site_id, :page_path, :scroll_threshold], + where: "page_path IS NOT NULL", + name: :goals_page_path_and_scroll_threshold_unique + ) + + def up do + alter table(:goals) do + add :scroll_threshold, :smallint, null: false, default: -1 + end + + drop(@old_index) + create(@new_index) + end + + def down do + drop(@new_index) + create(@old_index) + + alter table(:goals) do + remove :scroll_threshold + end + end +end From ecbdc8e5dcf9bb02d6edcfbcf62009e8838cfe70 Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Fri, 24 Jan 2025 18:21:54 +0100 Subject: [PATCH 02/26] update goal schema --- lib/plausible/goal/schema.ex | 12 +++++-- test/plausible/goals_test.exs | 60 +++++++++++++++++++++++++++++++++-- 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/lib/plausible/goal/schema.ex b/lib/plausible/goal/schema.ex index 8f2e29febe01..1df520c77741 100644 --- a/lib/plausible/goal/schema.ex +++ b/lib/plausible/goal/schema.ex @@ -8,6 +8,7 @@ defmodule Plausible.Goal do schema "goals" do field :event_name, :string field :page_path, :string + field :scroll_threshold, :integer, default: -1 field :display_name, :string on_ee do @@ -23,7 +24,7 @@ defmodule Plausible.Goal do timestamps() end - @fields [:id, :site_id, :event_name, :page_path, :display_name] ++ + @fields [:id, :site_id, :event_name, :page_path, :scroll_threshold, :display_name] ++ on_ee(do: [:currency], else: []) @max_event_name_length 120 @@ -39,9 +40,16 @@ defmodule Plausible.Goal do |> validate_event_name_and_page_path() |> maybe_put_display_name() |> unique_constraint(:event_name, name: :goals_event_name_unique) - |> unique_constraint(:page_path, name: :goals_page_path_unique) + |> unique_constraint([:page_path, :scroll_threshold], + name: :goals_page_path_and_scroll_threshold_unique + ) |> unique_constraint(:display_name, name: :goals_site_id_display_name_index) |> validate_length(:event_name, max: @max_event_name_length) + |> validate_number(:scroll_threshold, + greater_than_or_equal_to: -1, + less_than_or_equal_to: 100, + message: "Should be -1 (missing) or in range [0, 100]" + ) |> check_constraint(:event_name, name: :check_event_name_or_page_path, message: "cannot co-exist with page_path" diff --git a/test/plausible/goals_test.exs b/test/plausible/goals_test.exs index ede9287ba3a6..0e26aac24ba3 100644 --- a/test/plausible/goals_test.exs +++ b/test/plausible/goals_test.exs @@ -32,11 +32,65 @@ defmodule Plausible.GoalsTest do assert {"should be at most %{count} character(s)", _} = changeset.errors[:event_name] end + test "create/2 validates scroll_threshold in range [-1, 100]" do + site = new_site() + + {:error, changeset} = + Goals.create(site, %{"page_path" => "/blog/post-1", "scroll_threshold" => -2}) + + assert {"Should be -1 (missing) or in range [0, 100]", _} = + changeset.errors[:scroll_threshold] + + {:error, changeset} = + Goals.create(site, %{"page_path" => "/blog/post-1", "scroll_threshold" => 101}) + + assert {"Should be -1 (missing) or in range [0, 100]", _} = + changeset.errors[:scroll_threshold] + + assert {:ok, _} = + Goals.create(site, %{"page_path" => "/blog/post-1", "scroll_threshold" => -1}) + + assert {:ok, _} = + Goals.create(site, %{"page_path" => "/blog/post-2", "scroll_threshold" => 50}) + end + + test "create/2 validates uniqueness across page_path and scroll_threshold" do + site = new_site() + + {:ok, _} = + Goals.create(site, %{ + "page_path" => "/blog/post-1", + "scroll_threshold" => 50, + "display_name" => "Scroll 50" + }) + + {:ok, _} = + Goals.create(site, %{ + "page_path" => "/blog/post-1", + "scroll_threshold" => 75, + "display_name" => "Scroll 75" + }) + + {:error, changeset} = + Goals.create(site, %{ + "page_path" => "/blog/post-1", + "scroll_threshold" => 50, + "display_name" => "Scroll 50 another" + }) + + assert {"has already been taken", _} = + changeset.errors[:page_path] + end + test "create/2 fails to create the same pageview goal twice" do site = new_site() - {:ok, _} = Goals.create(site, %{"page_path" => "foo bar"}) - assert {:error, changeset} = Goals.create(site, %{"page_path" => "foo bar"}) - assert {"has already been taken", _} = changeset.errors[:page_path] + {:ok, _} = Goals.create(site, %{"page_path" => "foo bar", "display_name" => "one"}) + + assert {:error, changeset} = + Goals.create(site, %{"page_path" => "foo bar", "display_name" => "two"}) + + assert {"has already been taken", _} = + changeset.errors[:page_path] end test "create/2 fails to create the same custom event goal twice" do From d5631de38d2a57e66a044daf07543af62ad0e27a Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Fri, 24 Jan 2025 19:11:13 +0100 Subject: [PATCH 03/26] setup simple UI for creating scroll goals --- lib/plausible_web/live/goal_settings/form.ex | 14 +++++++++++ lib/plausible_web/live/goal_settings/list.ex | 25 ++++++++++++++++--- .../live/goal_settings/form_test.exs | 3 ++- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/lib/plausible_web/live/goal_settings/form.ex b/lib/plausible_web/live/goal_settings/form.ex index 669ceee695e8..cd4a950df0f9 100644 --- a/lib/plausible_web/live/goal_settings/form.ex +++ b/lib/plausible_web/live/goal_settings/form.ex @@ -84,6 +84,7 @@ defmodule PlausibleWeb.Live.GoalSettings.Form do f={f} goal={@goal} suffix={@context_unique_id} + current_user={@current_user} site={@site} /> @@ -127,6 +128,7 @@ defmodule PlausibleWeb.Live.GoalSettings.Form do x-show="!tabSelectionInProgress" f={f} suffix={suffix(@context_unique_id, @tab_sequence_id)} + current_user={@current_user} site={@site} x-init="tabSelectionInProgress = false" /> @@ -156,6 +158,7 @@ defmodule PlausibleWeb.Live.GoalSettings.Form do end attr(:f, Phoenix.HTML.Form) + attr(:current_user, Plausible.Auth.User) attr(:site, Plausible.Site) attr(:suffix, :string) attr(:goal, Plausible.Goal, default: nil) @@ -193,6 +196,17 @@ defmodule PlausibleWeb.Live.GoalSettings.Form do x-data="{ firstFocus: true }" x-on:focus="if (firstFocus) { $el.select(); firstFocus = false; }" /> + + <.input + :if={Plausible.Stats.ScrollDepth.feature_visible?(@site, @current_user)} + label="Scroll Depth Threshold" + field={@f[:scroll_threshold]} + type="number" + value={if @goal && @goal.scroll_threshold > -1, do: @goal.scroll_threshold, else: nil} + min="0" + max="100" + step="1" + /> """ end diff --git a/lib/plausible_web/live/goal_settings/list.ex b/lib/plausible_web/live/goal_settings/list.ex index 82e0df7ad952..0d652623dffc 100644 --- a/lib/plausible_web/live/goal_settings/list.ex +++ b/lib/plausible_web/live/goal_settings/list.ex @@ -99,7 +99,14 @@ defmodule PlausibleWeb.Live.GoalSettings.List do """ end - def pageview_description(goal) do + defp page_scroll_description(goal) do + case pageview_description(goal) do + "" -> "Scroll > #{goal.scroll_threshold}" + path -> "Scroll > #{goal.scroll_threshold} on #{path}" + end + end + + defp pageview_description(goal) do path = goal.page_path case goal.display_name do @@ -108,13 +115,23 @@ defmodule PlausibleWeb.Live.GoalSettings.List do end end - def custom_event_description(goal) do + defp custom_event_description(goal) do if goal.display_name == goal.event_name, do: "", else: "#{goal.event_name}" end - def goal_description(assigns) do + defp goal_description(assigns) do ~H""" - + -1} + class="block truncate text-gray-400 dark:text-gray-600" + > + {page_scroll_description(@goal)} + + + {pageview_description(@goal)} diff --git a/test/plausible_web/live/goal_settings/form_test.exs b/test/plausible_web/live/goal_settings/form_test.exs index 177cae03f462..bbbeb34b33f7 100644 --- a/test/plausible_web/live/goal_settings/form_test.exs +++ b/test/plausible_web/live/goal_settings/form_test.exs @@ -54,7 +54,8 @@ defmodule PlausibleWeb.Live.GoalSettings.FormTest do assert input_names == [ "display-page_path_input_modalseq0-tabseq1", "goal[page_path]", - "goal[display_name]" + "goal[display_name]", + "goal[scroll_threshold]" ] end From 58e930fd9ce0dad90308e5ee71b21d6cb933362f Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Wed, 29 Jan 2025 13:13:19 +0100 Subject: [PATCH 04/26] add ability to filter and breakdown scroll goals --- lib/plausible/goal/schema.ex | 11 +- lib/plausible/goals/filters.ex | 45 +++-- lib/plausible/goals/goals.ex | 23 +++ lib/plausible/stats/filters/utils.ex | 14 -- lib/plausible/stats/imported/imported.ex | 34 +++- lib/plausible/stats/query_runner.ex | 11 +- lib/plausible/stats/sql/query_builder.ex | 55 +++++- .../external_stats_controller/query_test.exs | 163 ++++++++++++++++++ .../api/stats_controller/conversions_test.exs | 109 +++++++++++- .../api/stats_controller/top_stats_test.exs | 120 +++++++++++++ 10 files changed, 525 insertions(+), 60 deletions(-) diff --git a/lib/plausible/goal/schema.ex b/lib/plausible/goal/schema.ex index 1df520c77741..b86f0879666e 100644 --- a/lib/plausible/goal/schema.ex +++ b/lib/plausible/goal/schema.ex @@ -62,9 +62,14 @@ defmodule Plausible.Goal do goal.display_name end - @spec type(t()) :: :event | :page - def type(%{event_name: event_name}) when is_binary(event_name), do: :event - def type(%{page_path: page_path}) when is_binary(page_path), do: :page + @spec type(t()) :: :event | :scroll | :page + def type(goal) do + cond do + is_binary(goal.event_name) -> :event + is_binary(goal.page_path) && goal.scroll_threshold > -1 -> :scroll + is_binary(goal.page_path) -> :page + end + end defp update_leading_slash(changeset) do case get_field(changeset, :page_path) do diff --git a/lib/plausible/goals/filters.ex b/lib/plausible/goals/filters.ex index 0caf5e4e5179..d7ae58dbb38a 100644 --- a/lib/plausible/goals/filters.ex +++ b/lib/plausible/goals/filters.ex @@ -106,33 +106,44 @@ defmodule Plausible.Goals.Filters do defp build_condition(filtered_goals, imported?) do Enum.reduce(filtered_goals, false, fn goal, dynamic_statement -> - case goal do - nil -> + cond do + is_nil(goal) -> dynamic([e], ^dynamic_statement) - %Plausible.Goal{event_name: event_name} when is_binary(event_name) -> - dynamic([e], e.name == ^event_name or ^dynamic_statement) + Plausible.Goal.type(goal) == :event -> + dynamic([e], e.name == ^goal.event_name or ^dynamic_statement) - %Plausible.Goal{page_path: page_path} when is_binary(page_path) -> - dynamic([e], ^page_filter_condition(page_path, imported?) or ^dynamic_statement) + Plausible.Goal.type(goal) == :scroll -> + dynamic([e], ^page_scroll_goal_condition(goal) or ^dynamic_statement) + + Plausible.Goal.type(goal) == :page -> + dynamic([e], ^pageview_goal_condition(goal, imported?) or ^dynamic_statement) end end) end - defp page_filter_condition(page_path, imported?) do - db_field = page_path_db_field(imported?) + defp page_scroll_goal_condition(goal) do + pathname_condition = page_path_condition(goal.page_path, _imported = false) + name_condition = dynamic([e], e.name == "pageleave") + scroll_condition = dynamic([e], e.scroll_depth >= ^goal.scroll_threshold) - page_condition = - if String.contains?(page_path, "*") do - dynamic([e], fragment("match(?, ?)", field(e, ^db_field), ^page_regex(page_path))) - else - dynamic([e], field(e, ^db_field) == ^page_path) - end + dynamic([e], ^pathname_condition and ^name_condition and ^scroll_condition) + end + + defp pageview_goal_condition(goal, imported?) do + pathname_condition = page_path_condition(goal.page_path, imported?) + name_condition = if imported?, do: true, else: dynamic([e], e.name == "pageview") + + dynamic([e], ^pathname_condition and ^name_condition) + end + + defp page_path_condition(page_path, imported?) do + db_field = page_path_db_field(imported?) - if imported? do - dynamic([e], ^page_condition) + if String.contains?(page_path, "*") do + dynamic([e], fragment("match(?, ?)", field(e, ^db_field), ^page_regex(page_path))) else - dynamic([e], ^page_condition and e.name == "pageview") + dynamic([e], field(e, ^db_field) == ^page_path) end end diff --git a/lib/plausible/goals/goals.ex b/lib/plausible/goals/goals.ex index af17a254f4eb..4579ee0ed4eb 100644 --- a/lib/plausible/goals/goals.ex +++ b/lib/plausible/goals/goals.ex @@ -6,6 +6,7 @@ defmodule Plausible.Goals do import Ecto.Query alias Plausible.Goal + alias Plausible.Stats.Filters alias Ecto.Multi @spec get(Plausible.Site.t(), pos_integer()) :: nil | Plausible.Goal.t() @@ -287,6 +288,28 @@ defmodule Plausible.Goals do :ok end + @type goal_decomposition() :: %{ + indices: [non_neg_integer()], + types: [String.t()], + event_names: [String.t()], + page_regexes: [String.t()], + scroll_thresholds: [non_neg_integer()] + } + + @spec decompose([Plausible.Goal.t()]) :: goal_decomposition() + def decompose(goals) do + %{ + indices: Enum.with_index(goals, 1) |> Enum.map(fn {_goal, idx} -> idx end), + types: Enum.map(goals, &to_string(Plausible.Goal.type(&1))), + event_names: Enum.map(goals, &to_string(&1.event_name)), + page_regexes: + Enum.map(goals, fn goal -> + if goal.page_path, do: Filters.Utils.page_regex(goal.page_path), else: "" + end), + scroll_thresholds: Enum.map(goals, & &1.scroll_threshold) + } + end + defp insert_goal(site, params, upsert?) do params = Map.delete(params, "site_id") diff --git a/lib/plausible/stats/filters/utils.ex b/lib/plausible/stats/filters/utils.ex index 957ef2ab3bc0..48dd501a870c 100644 --- a/lib/plausible/stats/filters/utils.ex +++ b/lib/plausible/stats/filters/utils.ex @@ -1,20 +1,6 @@ defmodule Plausible.Stats.Filters.Utils do @moduledoc false - def split_goals(goals) do - Enum.split_with(goals, fn goal -> Plausible.Goal.type(goal) == :event end) - end - - def split_goals_query_expressions(goals) do - {event_goals, pageview_goals} = split_goals(goals) - events = Enum.map(event_goals, fn goal -> goal.event_name end) - - page_regexes = - Enum.map(pageview_goals, fn goal -> page_regex(goal.page_path) end) - - {events, page_regexes} - end - def page_regex(expr) do escaped = expr diff --git a/lib/plausible/stats/imported/imported.ex b/lib/plausible/stats/imported/imported.ex index 61d99373de11..8b6876f04496 100644 --- a/lib/plausible/stats/imported/imported.ex +++ b/lib/plausible/stats/imported/imported.ex @@ -5,7 +5,6 @@ defmodule Plausible.Stats.Imported do import Ecto.Query import Plausible.Stats.Imported.SQL.Expression - alias Plausible.Stats.Filters alias Plausible.Stats.Imported alias Plausible.Stats.Query alias Plausible.Stats.SQL.QueryBuilder @@ -239,8 +238,14 @@ defmodule Plausible.Stats.Imported do end def merge_imported(q, site, %Query{dimensions: ["event:goal"]} = query, metrics) do - {events, page_regexes} = - Filters.Utils.split_goals_query_expressions(query.preloaded_goals.matching_toplevel_filters) + %{ + indices: goal_indices, + types: goal_types, + event_names: goal_event_names, + page_regexes: goal_page_regexes + } = + query.preloaded_goals.matching_toplevel_filters + |> Plausible.Goals.decompose() Imported.Base.decide_tables(query) |> Enum.map(fn @@ -248,7 +253,15 @@ defmodule Plausible.Stats.Imported do Imported.Base.query_imported("imported_custom_events", site, query) |> where([i], i.visitors > 0) |> select_merge_as([i], %{ - dim0: fragment("-indexOf(?, ?)", type(^events, {:array, :string}), i.name) + dim0: + type( + fragment( + "indexOf(?, ?)", + type(^goal_event_names, {:array, :string}), + i.name + ), + :integer + ) }) |> select_imported_metrics(metrics) |> group_by([], selected_as(:dim0)) @@ -260,9 +273,18 @@ defmodule Plausible.Stats.Imported do |> where( [i], fragment( - "notEmpty(multiMatchAllIndices(?, ?) as indices)", + """ + notEmpty( + arrayFilter( + goal_idx -> ?[goal_idx] = 'page' AND match(?, ?[goal_idx]), + ? + ) as indices + ) + """, + type(^goal_types, {:array, :string}), i.page, - type(^page_regexes, {:array, :string}) + type(^goal_page_regexes, {:array, :string}), + type(^goal_indices, {:array, :integer}) ) ) |> join(:inner, [_i], index in fragment("indices"), hints: "ARRAY", on: true) diff --git a/lib/plausible/stats/query_runner.ex b/lib/plausible/stats/query_runner.ex index a4abbce93b55..99f3c60c2523 100644 --- a/lib/plausible/stats/query_runner.ex +++ b/lib/plausible/stats/query_runner.ex @@ -18,7 +18,6 @@ defmodule Plausible.Stats.QueryRunner do QueryOptimizer, QueryResult, Legacy, - Filters, SQL, Util, Time @@ -137,15 +136,11 @@ defmodule Plausible.Stats.QueryRunner do end defp dimension_label("event:goal", entry, query) do - {events, paths} = Filters.Utils.split_goals(query.preloaded_goals.matching_toplevel_filters) - goal_index = Map.get(entry, Util.shortname(query, "event:goal")) - # Closely coupled logic with SQL.Expression.event_goal_join/2 - cond do - goal_index < 0 -> Enum.at(events, -goal_index - 1) |> Plausible.Goal.display_name() - goal_index > 0 -> Enum.at(paths, goal_index - 1) |> Plausible.Goal.display_name() - end + query.preloaded_goals.matching_toplevel_filters + |> Enum.at(goal_index - 1) + |> Plausible.Goal.display_name() end defp dimension_label("time:" <> _ = time_dimension, entry, query) do diff --git a/lib/plausible/stats/sql/query_builder.ex b/lib/plausible/stats/sql/query_builder.ex index f0b1e2b56f6e..9538449477ea 100644 --- a/lib/plausible/stats/sql/query_builder.ex +++ b/lib/plausible/stats/sql/query_builder.ex @@ -8,7 +8,7 @@ defmodule Plausible.Stats.SQL.QueryBuilder do import Plausible.Stats.Imported import Plausible.Stats.Util - alias Plausible.Stats.{Filters, Query, QueryOptimizer, TableDecider, SQL} + alias Plausible.Stats.{Query, QueryOptimizer, TableDecider, SQL} alias Plausible.Stats.SQL.Expression require Plausible.Stats.SQL.Expression @@ -139,18 +139,61 @@ defmodule Plausible.Stats.SQL.QueryBuilder do end defp dimension_group_by(q, _table, query, "event:goal" = dimension) do - {events, page_regexes} = - Filters.Utils.split_goals_query_expressions(query.preloaded_goals.matching_toplevel_filters) + %{ + indices: goal_indices, + types: goal_types, + event_names: goal_event_names, + page_regexes: goal_page_regexes, + scroll_thresholds: goal_scroll_thresholds + } = + query.preloaded_goals.matching_toplevel_filters + |> Plausible.Goals.decompose() from(e in q, - join: goal in Expression.event_goal_join(events, page_regexes), + where: + fragment( + """ + notEmpty( + arrayFilter( + goal_index -> + CASE + WHEN ?[goal_index] = 'event' THEN + ? = ?[goal_index] + WHEN ?[goal_index] = 'page' THEN + ? = 'pageview' AND match(?, ?[goal_index]) + ELSE + ? = 'pageleave' AND match(?, ?[goal_index]) AND ? >= ?[goal_index] + end + , + ? + ) as indices + ) + """, + # CASE 1 - custom event goals + type(^goal_types, {:array, :string}), + e.name, + type(^goal_event_names, {:array, :string}), + # CASE 2 - pageview goals + type(^goal_types, {:array, :string}), + e.name, + e.pathname, + type(^goal_page_regexes, {:array, :string}), + # CASE 3 - page scroll goals + e.name, + e.pathname, + type(^goal_page_regexes, {:array, :string}), + e.scroll_depth, + type(^goal_scroll_thresholds, {:array, :integer}), + # Array of indices getting filtered + type(^goal_indices, {:array, :integer}) + ), + join: goal in fragment("indices"), hints: "ARRAY", on: true, select_merge: %{ ^shortname(query, dimension) => fragment("?", goal) }, - group_by: goal, - where: goal != 0 and (e.name == "pageview" or goal < 0) + group_by: goal ) end diff --git a/test/plausible_web/controllers/api/external_stats_controller/query_test.exs b/test/plausible_web/controllers/api/external_stats_controller/query_test.exs index 33da8e25e69e..80a935a09faf 100644 --- a/test/plausible_web/controllers/api/external_stats_controller/query_test.exs +++ b/test/plausible_web/controllers/api/external_stats_controller/query_test.exs @@ -4156,6 +4156,169 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryTest do end end + describe "page scroll goals" do + test "returns page scroll goals and pageview goals in breakdown with page filter", %{ + conn: conn, + site: site + } do + insert(:goal, site: site, page_path: "/blog") + + insert(:goal, + site: site, + page_path: "/blog", + scroll_threshold: 25, + display_name: "Scroll /blog 25" + ) + + populate_stats(site, [ + build(:pageview, user_id: 12, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), + build(:pageleave, + user_id: 12, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:00], + scroll_depth: 10 + ), + build(:pageview, user_id: 34, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), + build(:pageleave, + user_id: 34, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:00], + scroll_depth: 30 + ), + build(:pageview) + ]) + + conn = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "metrics" => ["visitors", "events", "conversion_rate"], + "filters" => [["is", "event:page", ["/blog"]]], + "date_range" => "all", + "dimensions" => ["event:goal"] + }) + + assert json_response(conn, 200)["results"] == [ + %{"dimensions" => ["Visit /blog"], "metrics" => [2, 2, 100.0]}, + # TODO: make it possible to query total conversions from pageleave events + %{"dimensions" => ["Scroll /blog 25"], "metrics" => [1, 0, 50.0]} + ] + end + + test "custom props in combination with page scroll goals", %{ + conn: conn, + site: site + } do + for threshold <- [25, 50, 75] do + insert(:goal, + site: site, + page_path: "/blog**", + scroll_threshold: threshold, + display_name: "Scroll /blog #{threshold}" + ) + end + + populate_stats(site, [ + build(:pageview, + pathname: "/blog/john-post", + "meta.key": ["author"], + "meta.value": ["john"] + ), + build(:pageview, + user_id: 12, + pathname: "/blog/john-post", + timestamp: ~N[2021-01-01 00:00:00], + "meta.key": ["author"], + "meta.value": ["john"] + ), + build(:pageleave, + user_id: 12, + pathname: "/blog/john-post", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 30, + "meta.key": ["author"], + "meta.value": ["john"] + ), + build(:pageview, + user_id: 34, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:00], + "meta.key": ["author"], + "meta.value": ["jane"] + ), + build(:pageleave, + user_id: 34, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 50, + "meta.key": ["author"], + "meta.value": ["jane"] + ), + build(:pageview, + user_id: 56, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:00], + "meta.key": ["author"], + "meta.value": ["jane"] + ), + build(:pageleave, + user_id: 56, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 75, + "meta.key": ["author"], + "meta.value": ["jane"] + ) + ]) + + # Double-dimension breakdown + conn1 = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "metrics" => ["visitors", "events", "conversion_rate"], + "date_range" => "all", + "dimensions" => ["event:goal", "event:props:author"] + }) + + assert json_response(conn1, 200)["results"] == [ + %{"dimensions" => ["Scroll /blog 25", "jane"], "metrics" => [2, 0, 50.0]}, + %{"dimensions" => ["Scroll /blog 50", "jane"], "metrics" => [2, 0, 50.0]}, + %{"dimensions" => ["Scroll /blog 25", "john"], "metrics" => [1, 0, 25.0]}, + %{"dimensions" => ["Scroll /blog 75", "jane"], "metrics" => [1, 0, 25.0]} + ] + + # Breakdown by event:props:author with a page scroll goal filter + conn2 = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "metrics" => ["visitors", "events", "conversion_rate"], + "date_range" => "all", + "filters" => [["is", "event:goal", ["Scroll /blog 25"]]], + "dimensions" => ["event:props:author"] + }) + + assert json_response(conn2, 200)["results"] == [ + %{"dimensions" => ["jane"], "metrics" => [2, 0, 50.0]}, + %{"dimensions" => ["john"], "metrics" => [1, 0, 25.0]} + ] + + # Breaks down page scroll goals with a custom prop filter + conn3 = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "metrics" => ["visitors", "events", "conversion_rate"], + "date_range" => "all", + "filters" => [["is", "event:props:author", ["jane"]]], + "dimensions" => ["event:goal"] + }) + + assert json_response(conn3, 200)["results"] == [ + %{"dimensions" => ["Scroll /blog 25"], "metrics" => [2, 0, 50.0]}, + %{"dimensions" => ["Scroll /blog 50"], "metrics" => [2, 0, 50.0]}, + %{"dimensions" => ["Scroll /blog 75"], "metrics" => [1, 0, 25.0]} + ] + end + end + test "can filter by utm_medium case insensitively", %{conn: conn, site: site} do populate_stats(site, [ build(:pageview, diff --git a/test/plausible_web/controllers/api/stats_controller/conversions_test.exs b/test/plausible_web/controllers/api/stats_controller/conversions_test.exs index 738c5b6e6b43..2d358efee31e 100644 --- a/test/plausible_web/controllers/api/stats_controller/conversions_test.exs +++ b/test/plausible_web/controllers/api/stats_controller/conversions_test.exs @@ -7,7 +7,10 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do describe "GET /api/stats/:domain/conversions" do setup [:create_user, :log_in, :create_site] - test "returns mixed conversions in ordered by count", %{conn: conn, site: site} do + test "returns mixed pageview and custom event goal conversions ordered by count", %{ + conn: conn, + site: site + } do populate_stats(site, [ build(:pageview, pathname: "/"), build(:pageview, pathname: "/"), @@ -25,7 +28,8 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do name: "Signup", "meta.key": ["variant"], "meta.value": ["B"] - ) + ), + build(:event, name: "Signup") ]) insert(:goal, %{site: site, page_path: "/register"}) @@ -36,15 +40,108 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do assert json_response(conn, 200)["results"] == [ %{ "name" => "Signup", - "visitors" => 2, - "events" => 3, - "conversion_rate" => 33.3 + "visitors" => 3, + "events" => 4, + "conversion_rate" => 42.9 }, %{ "name" => "Visit /register", "visitors" => 2, "events" => 2, - "conversion_rate" => 33.3 + "conversion_rate" => 28.6 + } + ] + end + + test "returns page scroll goals ordered by count", %{conn: conn, site: site} do + populate_stats(site, [ + # user 1: /blog -> /another -> blog/posts/1 + build(:pageview, user_id: 1, pathname: "/blog", timestamp: ~N[2020-01-01 00:00:00]), + build(:pageleave, + user_id: 1, + pathname: "/blog", + timestamp: ~N[2020-01-01 00:01:00], + scroll_depth: 20 + ), + build(:pageview, user_id: 1, pathname: "/another", timestamp: ~N[2020-01-01 00:01:00]), + build(:pageleave, + user_id: 1, + pathname: "/another", + timestamp: ~N[2020-01-01 00:02:00], + scroll_depth: 100 + ), + build(:pageview, + user_id: 1, + pathname: "/blog/posts/1", + timestamp: ~N[2020-01-01 00:02:00] + ), + build(:pageleave, + user_id: 1, + pathname: "/blog/posts/1", + timestamp: ~N[2020-01-01 00:03:00], + scroll_depth: 55 + ), + # user 2: /blog -> /blog/posts/1 -> /blog/posts/2 + build(:pageview, user_id: 2, pathname: "/blog", timestamp: ~N[2020-01-01 00:00:00]), + build(:pageleave, + user_id: 2, + pathname: "/blog", + timestamp: ~N[2020-01-01 00:01:00], + scroll_depth: 60 + ), + build(:pageview, + user_id: 2, + pathname: "/blog/posts/1", + timestamp: ~N[2020-01-01 00:02:00] + ), + build(:pageleave, + user_id: 2, + pathname: "/blog/posts/1", + timestamp: ~N[2020-01-01 00:03:00], + scroll_depth: 100 + ), + build(:pageview, + user_id: 2, + pathname: "/blog/posts/2", + timestamp: ~N[2020-01-01 00:02:00] + ), + build(:pageleave, + user_id: 2, + pathname: "/blog/posts/2", + timestamp: ~N[2020-01-01 00:03:00], + scroll_depth: 100 + ) + ]) + + insert(:goal, %{ + site: site, + page_path: "/blog/**", + scroll_threshold: 50, + display_name: "Scroll 50 /blog/**" + }) + + insert(:goal, %{ + site: site, + page_path: "/blog/posts/1", + scroll_threshold: 75, + display_name: "Scroll 75 /blog/posts/1" + }) + + conn = get(conn, "/api/stats/#{site.domain}/conversions?period=day&date=2020-01-01") + + # TODO: make it possible to query `events` metric + assert json_response(conn, 200)["results"] == [ + %{ + "name" => "Scroll 50 /blog/**", + "visitors" => 2, + "events" => 0, + "conversion_rate" => 100.0 + }, + %{ + "name" => "Scroll 75 /blog/posts/1", + "visitors" => 1, + "events" => 0, + "conversion_rate" => 50.0 } ] end diff --git a/test/plausible_web/controllers/api/stats_controller/top_stats_test.exs b/test/plausible_web/controllers/api/stats_controller/top_stats_test.exs index 286a1073f6f5..0ae31cedfcc6 100644 --- a/test/plausible_web/controllers/api/stats_controller/top_stats_test.exs +++ b/test/plausible_web/controllers/api/stats_controller/top_stats_test.exs @@ -1742,6 +1742,126 @@ defmodule PlausibleWeb.Api.StatsController.TopStatsTest do refute "Average revenue" in metrics refute "Total revenue" in metrics end + + test "page scroll goal filter", %{conn: conn, site: site} do + insert(:goal, site: site, page_path: "/blog", scroll_threshold: 50) + + populate_stats(site, [ + build(:pageview, user_id: 123, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), + build(:pageleave, + user_id: 123, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 60 + ), + build(:pageview, user_id: 456, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), + build(:pageleave, + user_id: 456, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 40 + ) + ]) + + filters = Jason.encode!([[:is, "event:goal", ["Visit /blog"]]]) + conn = get(conn, "/api/stats/#{site.domain}/top-stats?period=all&filters=#{filters}") + assert %{"top_stats" => top_stats} = json_response(conn, 200) + + assert [unique_conversions, total_conversions, conversion_rate] = top_stats + + assert %{"name" => "Unique conversions", "value" => 1} = unique_conversions + # TODO: make it possible to query total conversions from pageleave events + assert %{"name" => "Total conversions", "value" => 0} = total_conversions + assert %{"name" => "Conversion rate", "value" => 50.0} = conversion_rate + end + + test "goal is page scroll OR custom event", %{conn: conn, site: site} do + insert(:goal, site: site, page_path: "/blog", scroll_threshold: 50) + insert(:goal, site: site, event_name: "Signup") + + populate_stats(site, [ + build(:pageview, user_id: 123, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), + build(:event, + user_id: 123, + pathname: "/blog", + name: "Signup", + timestamp: ~N[2021-01-01 00:00:05] + ), + build(:pageleave, + user_id: 123, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 60 + ), + build(:pageview, user_id: 456, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), + build(:pageleave, + user_id: 456, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 40 + ), + build(:pageview, user_id: 789, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), + build(:pageleave, + user_id: 789, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 100 + ), + build(:event, name: "Signup", timestamp: ~N[2021-01-01 00:01:00]), + build(:pageview, timestamp: ~N[2021-01-01 00:01:00]) + ]) + + filters = Jason.encode!([[:is, "event:goal", ["Visit /blog", "Signup"]]]) + conn = get(conn, "/api/stats/#{site.domain}/top-stats?period=all&filters=#{filters}") + assert %{"top_stats" => top_stats} = json_response(conn, 200) + + assert [unique_conversions, total_conversions, conversion_rate] = top_stats + + assert %{"name" => "Unique conversions", "value" => 3} = unique_conversions + # TODO: make it possible to query total conversions from pageleave events + assert %{"name" => "Total conversions", "value" => 2} = total_conversions + assert %{"name" => "Conversion rate", "value" => 60.0} = conversion_rate + end + + test "goal is page scroll OR pageview goal", %{conn: conn, site: site} do + insert(:goal, + site: site, + page_path: "/blog**", + scroll_threshold: 50, + display_name: "Scroll 50 /blog**" + ) + + insert(:goal, site: site, page_path: "/blog") + + populate_stats(site, [ + build(:pageview, user_id: 123, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), + build(:pageleave, + user_id: 123, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 60 + ), + build(:pageview, user_id: 456, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), + build(:pageleave, + user_id: 456, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 40 + ), + build(:pageview, timestamp: ~N[2021-01-01 00:01:00]) + ]) + + filters = Jason.encode!([[:is, "event:goal", ["Visit /blog", "Scroll 50 /blog**"]]]) + conn = get(conn, "/api/stats/#{site.domain}/top-stats?period=all&filters=#{filters}") + assert %{"top_stats" => top_stats} = json_response(conn, 200) + + assert [unique_conversions, total_conversions, conversion_rate] = top_stats + + assert %{"name" => "Unique conversions", "value" => 2} = unique_conversions + # TODO: make it possible to query total conversions from pageleave events + assert %{"name" => "Total conversions", "value" => 2} = total_conversions + assert %{"name" => "Conversion rate", "value" => 66.7} = conversion_rate + end end describe "GET /api/stats/top-stats - with comparisons" do From 6c30e0f73f5fdfc0b1e2672ddc725af316e22a7b Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Wed, 29 Jan 2025 14:12:55 +0100 Subject: [PATCH 05/26] fix goals form tests --- .../live/goal_settings/form_test.exs | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/test/plausible_web/live/goal_settings/form_test.exs b/test/plausible_web/live/goal_settings/form_test.exs index bbbeb34b33f7..fee362210a76 100644 --- a/test/plausible_web/live/goal_settings/form_test.exs +++ b/test/plausible_web/live/goal_settings/form_test.exs @@ -54,8 +54,7 @@ defmodule PlausibleWeb.Live.GoalSettings.FormTest do assert input_names == [ "display-page_path_input_modalseq0-tabseq1", "goal[page_path]", - "goal[display_name]", - "goal[scroll_threshold]" + "goal[display_name]" ] end @@ -89,6 +88,26 @@ defmodule PlausibleWeb.Live.GoalSettings.FormTest do ] end + test "renders scroll_threshold input in pageview goal form if scroll_depth feature visible for site/user", + %{conn: conn, site: site} do + Plausible.Sites.set_scroll_depth_visible_at(site) + + lv = get_liveview(conn, site) + lv |> element(~s/a#pageview-tab/) |> render_click() + html = render(lv) + input_names = html |> find("#pageviews-form input") |> Enum.map(&name_of/1) + assert "goal[scroll_threshold]" in input_names + end + + test "does not render scroll_threshold input in pageview goal form if scroll_depth feature not visible for site/user", + %{conn: conn, site: site} do + lv = get_liveview(conn, site) + lv |> element(~s/a#pageview-tab/) |> render_click() + html = render(lv) + input_names = html |> find("#pageviews-form input") |> Enum.map(&name_of/1) + refute "goal[scroll_threshold]" in input_names + end + test "renders error on empty submission", %{conn: conn, site: site} do lv = get_liveview(conn, site) lv |> element("#goals-form-modalseq0 form") |> render_submit() From 7efca9202590bd5ad9cfda580e570235f1afc10b Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Wed, 29 Jan 2025 14:33:08 +0100 Subject: [PATCH 06/26] add valiation for page path exists --- lib/plausible/goal/schema.ex | 13 +++++++++++++ test/plausible/goals_test.exs | 10 ++++++++++ 2 files changed, 23 insertions(+) diff --git a/lib/plausible/goal/schema.ex b/lib/plausible/goal/schema.ex index b86f0879666e..2900c598d4d7 100644 --- a/lib/plausible/goal/schema.ex +++ b/lib/plausible/goal/schema.ex @@ -38,6 +38,7 @@ defmodule Plausible.Goal do |> cast_assoc(:site) |> update_leading_slash() |> validate_event_name_and_page_path() + |> validate_page_path_for_scroll_goal() |> maybe_put_display_name() |> unique_constraint(:event_name, name: :goals_event_name_unique) |> unique_constraint([:page_path, :scroll_threshold], @@ -99,6 +100,18 @@ defmodule Plausible.Goal do end end + defp validate_page_path_for_scroll_goal(changeset) do + scroll_threshold = get_field(changeset, :scroll_threshold) + page_path = get_field(changeset, :page_path) + + if scroll_threshold > -1 and is_nil(page_path) do + changeset + |> add_error(:scroll_threshold, "page_path field missing for page scroll goal") + else + changeset + end + end + defp validate_page_path(changeset) do value = get_field(changeset, :page_path) diff --git a/test/plausible/goals_test.exs b/test/plausible/goals_test.exs index 0e26aac24ba3..4a519f0cd818 100644 --- a/test/plausible/goals_test.exs +++ b/test/plausible/goals_test.exs @@ -54,6 +54,16 @@ defmodule Plausible.GoalsTest do Goals.create(site, %{"page_path" => "/blog/post-2", "scroll_threshold" => 50}) end + test "create/2 validates page path exists for scroll goals" do + site = new_site() + + {:error, changeset} = + Goals.create(site, %{"event_name" => "Signup", "scroll_threshold" => 50}) + + assert {"page_path field missing for page scroll goal", _} = + changeset.errors[:scroll_threshold] + end + test "create/2 validates uniqueness across page_path and scroll_threshold" do site = new_site() From d19fe6553161981c1e142436b2a1be9b1ba570f7 Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Wed, 29 Jan 2025 14:40:02 +0100 Subject: [PATCH 07/26] move todo comments to expression.ex --- lib/plausible/stats/sql/expression.ex | 1 + .../controllers/api/external_stats_controller/query_test.exs | 1 - .../controllers/api/stats_controller/conversions_test.exs | 1 - .../controllers/api/stats_controller/top_stats_test.exs | 3 --- 4 files changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/plausible/stats/sql/expression.ex b/lib/plausible/stats/sql/expression.ex index 34311dcb76a2..8c97fe783771 100644 --- a/lib/plausible/stats/sql/expression.ex +++ b/lib/plausible/stats/sql/expression.ex @@ -196,6 +196,7 @@ defmodule Plausible.Stats.SQL.Expression do }) end + # TODO: make it possible to query events from pageleave events (total conversions for page scroll goals) def event_metric(:events) do wrap_alias([e], %{ events: fragment("toUInt64(round(countIf(? != 'pageleave') * any(_sample_factor)))", e.name) diff --git a/test/plausible_web/controllers/api/external_stats_controller/query_test.exs b/test/plausible_web/controllers/api/external_stats_controller/query_test.exs index 80a935a09faf..e729dc63c325 100644 --- a/test/plausible_web/controllers/api/external_stats_controller/query_test.exs +++ b/test/plausible_web/controllers/api/external_stats_controller/query_test.exs @@ -4199,7 +4199,6 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryTest do assert json_response(conn, 200)["results"] == [ %{"dimensions" => ["Visit /blog"], "metrics" => [2, 2, 100.0]}, - # TODO: make it possible to query total conversions from pageleave events %{"dimensions" => ["Scroll /blog 25"], "metrics" => [1, 0, 50.0]} ] end diff --git a/test/plausible_web/controllers/api/stats_controller/conversions_test.exs b/test/plausible_web/controllers/api/stats_controller/conversions_test.exs index 2d358efee31e..fe72cb1f46d9 100644 --- a/test/plausible_web/controllers/api/stats_controller/conversions_test.exs +++ b/test/plausible_web/controllers/api/stats_controller/conversions_test.exs @@ -129,7 +129,6 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do conn = get(conn, "/api/stats/#{site.domain}/conversions?period=day&date=2020-01-01") - # TODO: make it possible to query `events` metric assert json_response(conn, 200)["results"] == [ %{ "name" => "Scroll 50 /blog/**", diff --git a/test/plausible_web/controllers/api/stats_controller/top_stats_test.exs b/test/plausible_web/controllers/api/stats_controller/top_stats_test.exs index 0ae31cedfcc6..5ec5aee0bfc5 100644 --- a/test/plausible_web/controllers/api/stats_controller/top_stats_test.exs +++ b/test/plausible_web/controllers/api/stats_controller/top_stats_test.exs @@ -1770,7 +1770,6 @@ defmodule PlausibleWeb.Api.StatsController.TopStatsTest do assert [unique_conversions, total_conversions, conversion_rate] = top_stats assert %{"name" => "Unique conversions", "value" => 1} = unique_conversions - # TODO: make it possible to query total conversions from pageleave events assert %{"name" => "Total conversions", "value" => 0} = total_conversions assert %{"name" => "Conversion rate", "value" => 50.0} = conversion_rate end @@ -1818,7 +1817,6 @@ defmodule PlausibleWeb.Api.StatsController.TopStatsTest do assert [unique_conversions, total_conversions, conversion_rate] = top_stats assert %{"name" => "Unique conversions", "value" => 3} = unique_conversions - # TODO: make it possible to query total conversions from pageleave events assert %{"name" => "Total conversions", "value" => 2} = total_conversions assert %{"name" => "Conversion rate", "value" => 60.0} = conversion_rate end @@ -1858,7 +1856,6 @@ defmodule PlausibleWeb.Api.StatsController.TopStatsTest do assert [unique_conversions, total_conversions, conversion_rate] = top_stats assert %{"name" => "Unique conversions", "value" => 2} = unique_conversions - # TODO: make it possible to query total conversions from pageleave events assert %{"name" => "Total conversions", "value" => 2} = total_conversions assert %{"name" => "Conversion rate", "value" => 66.7} = conversion_rate end From 1848d4475cbb9b9d7a52ab86fc469fbc964f39ef Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Wed, 29 Jan 2025 15:01:27 +0100 Subject: [PATCH 08/26] move tests --- .../query_goal_dimension_test.exs | 293 ++++++++++++++++++ .../external_stats_controller/query_test.exs | 162 ---------- 2 files changed, 293 insertions(+), 162 deletions(-) diff --git a/test/plausible_web/controllers/api/external_stats_controller/query_goal_dimension_test.exs b/test/plausible_web/controllers/api/external_stats_controller/query_goal_dimension_test.exs index 64b517995fae..797d6d371818 100644 --- a/test/plausible_web/controllers/api/external_stats_controller/query_goal_dimension_test.exs +++ b/test/plausible_web/controllers/api/external_stats_controller/query_goal_dimension_test.exs @@ -130,4 +130,297 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryGoalDimensionTest do ] end end + + describe "page scroll goals" do + test "returns page scroll goals and pageview goals in breakdown with page filter", %{ + conn: conn, + site: site + } do + insert(:goal, site: site, page_path: "/blog") + + insert(:goal, + site: site, + page_path: "/blog", + scroll_threshold: 25, + display_name: "Scroll /blog 25" + ) + + populate_stats(site, [ + build(:pageview, user_id: 12, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), + build(:pageleave, + user_id: 12, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:00], + scroll_depth: 10 + ), + build(:pageview, user_id: 34, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), + build(:pageleave, + user_id: 34, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:00], + scroll_depth: 30 + ), + build(:pageview) + ]) + + conn = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "metrics" => ["visitors", "events", "conversion_rate"], + "filters" => [["is", "event:page", ["/blog"]]], + "date_range" => "all", + "dimensions" => ["event:goal"] + }) + + assert json_response(conn, 200)["results"] == [ + %{"dimensions" => ["Visit /blog"], "metrics" => [2, 2, 100.0]}, + %{"dimensions" => ["Scroll /blog 25"], "metrics" => [1, 0, 50.0]} + ] + end + + test "custom props and page scroll goals in a double-dimension breakdown", %{ + conn: conn, + site: site + } do + for threshold <- [25, 50, 75] do + insert(:goal, + site: site, + page_path: "/blog**", + scroll_threshold: threshold, + display_name: "Scroll /blog #{threshold}" + ) + end + + populate_stats(site, [ + build(:pageview, + pathname: "/blog/john-post", + "meta.key": ["author"], + "meta.value": ["john"] + ), + build(:pageview, + user_id: 12, + pathname: "/blog/john-post", + timestamp: ~N[2021-01-01 00:00:00], + "meta.key": ["author"], + "meta.value": ["john"] + ), + build(:pageleave, + user_id: 12, + pathname: "/blog/john-post", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 30, + "meta.key": ["author"], + "meta.value": ["john"] + ), + build(:pageview, + user_id: 34, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:00], + "meta.key": ["author"], + "meta.value": ["jane"] + ), + build(:pageleave, + user_id: 34, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 50, + "meta.key": ["author"], + "meta.value": ["jane"] + ), + build(:pageview, + user_id: 56, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:00], + "meta.key": ["author"], + "meta.value": ["jane"] + ), + build(:pageleave, + user_id: 56, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 75, + "meta.key": ["author"], + "meta.value": ["jane"] + ) + ]) + + conn = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "metrics" => ["visitors", "events", "conversion_rate"], + "date_range" => "all", + "dimensions" => ["event:goal", "event:props:author"] + }) + + assert json_response(conn, 200)["results"] == [ + %{"dimensions" => ["Scroll /blog 25", "jane"], "metrics" => [2, 0, 50.0]}, + %{"dimensions" => ["Scroll /blog 50", "jane"], "metrics" => [2, 0, 50.0]}, + %{"dimensions" => ["Scroll /blog 25", "john"], "metrics" => [1, 0, 25.0]}, + %{"dimensions" => ["Scroll /blog 75", "jane"], "metrics" => [1, 0, 25.0]} + ] + end + + test "breakdown by event:props:author with a page scroll goal filter", %{ + conn: conn, + site: site + } do + for threshold <- [25, 50, 75] do + insert(:goal, + site: site, + page_path: "/blog**", + scroll_threshold: threshold, + display_name: "Scroll /blog #{threshold}" + ) + end + + populate_stats(site, [ + build(:pageview, + pathname: "/blog/john-post", + "meta.key": ["author"], + "meta.value": ["john"] + ), + build(:pageview, + user_id: 12, + pathname: "/blog/john-post", + timestamp: ~N[2021-01-01 00:00:00], + "meta.key": ["author"], + "meta.value": ["john"] + ), + build(:pageleave, + user_id: 12, + pathname: "/blog/john-post", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 30, + "meta.key": ["author"], + "meta.value": ["john"] + ), + build(:pageview, + user_id: 34, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:00], + "meta.key": ["author"], + "meta.value": ["jane"] + ), + build(:pageleave, + user_id: 34, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 50, + "meta.key": ["author"], + "meta.value": ["jane"] + ), + build(:pageview, + user_id: 56, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:00], + "meta.key": ["author"], + "meta.value": ["jane"] + ), + build(:pageleave, + user_id: 56, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 75, + "meta.key": ["author"], + "meta.value": ["jane"] + ) + ]) + + conn = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "metrics" => ["visitors", "events", "conversion_rate"], + "date_range" => "all", + "filters" => [["is", "event:goal", ["Scroll /blog 25"]]], + "dimensions" => ["event:props:author"] + }) + + assert json_response(conn, 200)["results"] == [ + %{"dimensions" => ["jane"], "metrics" => [2, 0, 50.0]}, + %{"dimensions" => ["john"], "metrics" => [1, 0, 25.0]} + ] + end + + test "breaks down page scroll goals with a custom prop filter", %{ + conn: conn, + site: site + } do + for threshold <- [25, 50, 75] do + insert(:goal, + site: site, + page_path: "/blog**", + scroll_threshold: threshold, + display_name: "Scroll /blog #{threshold}" + ) + end + + populate_stats(site, [ + build(:pageview, + pathname: "/blog/john-post", + "meta.key": ["author"], + "meta.value": ["john"] + ), + build(:pageview, + user_id: 12, + pathname: "/blog/john-post", + timestamp: ~N[2021-01-01 00:00:00], + "meta.key": ["author"], + "meta.value": ["john"] + ), + build(:pageleave, + user_id: 12, + pathname: "/blog/john-post", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 30, + "meta.key": ["author"], + "meta.value": ["john"] + ), + build(:pageview, + user_id: 34, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:00], + "meta.key": ["author"], + "meta.value": ["jane"] + ), + build(:pageleave, + user_id: 34, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 50, + "meta.key": ["author"], + "meta.value": ["jane"] + ), + build(:pageview, + user_id: 56, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:00], + "meta.key": ["author"], + "meta.value": ["jane"] + ), + build(:pageleave, + user_id: 56, + pathname: "/blog/jane-post", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 75, + "meta.key": ["author"], + "meta.value": ["jane"] + ) + ]) + + conn = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "metrics" => ["visitors", "events", "conversion_rate"], + "date_range" => "all", + "filters" => [["is", "event:props:author", ["jane"]]], + "dimensions" => ["event:goal"] + }) + + assert json_response(conn, 200)["results"] == [ + %{"dimensions" => ["Scroll /blog 25"], "metrics" => [2, 0, 50.0]}, + %{"dimensions" => ["Scroll /blog 50"], "metrics" => [2, 0, 50.0]}, + %{"dimensions" => ["Scroll /blog 75"], "metrics" => [1, 0, 25.0]} + ] + end + end end diff --git a/test/plausible_web/controllers/api/external_stats_controller/query_test.exs b/test/plausible_web/controllers/api/external_stats_controller/query_test.exs index e729dc63c325..33da8e25e69e 100644 --- a/test/plausible_web/controllers/api/external_stats_controller/query_test.exs +++ b/test/plausible_web/controllers/api/external_stats_controller/query_test.exs @@ -4156,168 +4156,6 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryTest do end end - describe "page scroll goals" do - test "returns page scroll goals and pageview goals in breakdown with page filter", %{ - conn: conn, - site: site - } do - insert(:goal, site: site, page_path: "/blog") - - insert(:goal, - site: site, - page_path: "/blog", - scroll_threshold: 25, - display_name: "Scroll /blog 25" - ) - - populate_stats(site, [ - build(:pageview, user_id: 12, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), - build(:pageleave, - user_id: 12, - pathname: "/blog", - timestamp: ~N[2021-01-01 00:00:00], - scroll_depth: 10 - ), - build(:pageview, user_id: 34, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), - build(:pageleave, - user_id: 34, - pathname: "/blog", - timestamp: ~N[2021-01-01 00:00:00], - scroll_depth: 30 - ), - build(:pageview) - ]) - - conn = - post(conn, "/api/v2/query", %{ - "site_id" => site.domain, - "metrics" => ["visitors", "events", "conversion_rate"], - "filters" => [["is", "event:page", ["/blog"]]], - "date_range" => "all", - "dimensions" => ["event:goal"] - }) - - assert json_response(conn, 200)["results"] == [ - %{"dimensions" => ["Visit /blog"], "metrics" => [2, 2, 100.0]}, - %{"dimensions" => ["Scroll /blog 25"], "metrics" => [1, 0, 50.0]} - ] - end - - test "custom props in combination with page scroll goals", %{ - conn: conn, - site: site - } do - for threshold <- [25, 50, 75] do - insert(:goal, - site: site, - page_path: "/blog**", - scroll_threshold: threshold, - display_name: "Scroll /blog #{threshold}" - ) - end - - populate_stats(site, [ - build(:pageview, - pathname: "/blog/john-post", - "meta.key": ["author"], - "meta.value": ["john"] - ), - build(:pageview, - user_id: 12, - pathname: "/blog/john-post", - timestamp: ~N[2021-01-01 00:00:00], - "meta.key": ["author"], - "meta.value": ["john"] - ), - build(:pageleave, - user_id: 12, - pathname: "/blog/john-post", - timestamp: ~N[2021-01-01 00:00:10], - scroll_depth: 30, - "meta.key": ["author"], - "meta.value": ["john"] - ), - build(:pageview, - user_id: 34, - pathname: "/blog/jane-post", - timestamp: ~N[2021-01-01 00:00:00], - "meta.key": ["author"], - "meta.value": ["jane"] - ), - build(:pageleave, - user_id: 34, - pathname: "/blog/jane-post", - timestamp: ~N[2021-01-01 00:00:10], - scroll_depth: 50, - "meta.key": ["author"], - "meta.value": ["jane"] - ), - build(:pageview, - user_id: 56, - pathname: "/blog/jane-post", - timestamp: ~N[2021-01-01 00:00:00], - "meta.key": ["author"], - "meta.value": ["jane"] - ), - build(:pageleave, - user_id: 56, - pathname: "/blog/jane-post", - timestamp: ~N[2021-01-01 00:00:10], - scroll_depth: 75, - "meta.key": ["author"], - "meta.value": ["jane"] - ) - ]) - - # Double-dimension breakdown - conn1 = - post(conn, "/api/v2/query", %{ - "site_id" => site.domain, - "metrics" => ["visitors", "events", "conversion_rate"], - "date_range" => "all", - "dimensions" => ["event:goal", "event:props:author"] - }) - - assert json_response(conn1, 200)["results"] == [ - %{"dimensions" => ["Scroll /blog 25", "jane"], "metrics" => [2, 0, 50.0]}, - %{"dimensions" => ["Scroll /blog 50", "jane"], "metrics" => [2, 0, 50.0]}, - %{"dimensions" => ["Scroll /blog 25", "john"], "metrics" => [1, 0, 25.0]}, - %{"dimensions" => ["Scroll /blog 75", "jane"], "metrics" => [1, 0, 25.0]} - ] - - # Breakdown by event:props:author with a page scroll goal filter - conn2 = - post(conn, "/api/v2/query", %{ - "site_id" => site.domain, - "metrics" => ["visitors", "events", "conversion_rate"], - "date_range" => "all", - "filters" => [["is", "event:goal", ["Scroll /blog 25"]]], - "dimensions" => ["event:props:author"] - }) - - assert json_response(conn2, 200)["results"] == [ - %{"dimensions" => ["jane"], "metrics" => [2, 0, 50.0]}, - %{"dimensions" => ["john"], "metrics" => [1, 0, 25.0]} - ] - - # Breaks down page scroll goals with a custom prop filter - conn3 = - post(conn, "/api/v2/query", %{ - "site_id" => site.domain, - "metrics" => ["visitors", "events", "conversion_rate"], - "date_range" => "all", - "filters" => [["is", "event:props:author", ["jane"]]], - "dimensions" => ["event:goal"] - }) - - assert json_response(conn3, 200)["results"] == [ - %{"dimensions" => ["Scroll /blog 25"], "metrics" => [2, 0, 50.0]}, - %{"dimensions" => ["Scroll /blog 50"], "metrics" => [2, 0, 50.0]}, - %{"dimensions" => ["Scroll /blog 75"], "metrics" => [1, 0, 25.0]} - ] - end - end - test "can filter by utm_medium case insensitively", %{conn: conn, site: site} do populate_stats(site, [ build(:pageview, From 5bef111a0b9cb8da8685e9ef38d855b2ccbd52d7 Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Wed, 29 Jan 2025 15:08:33 +0100 Subject: [PATCH 09/26] make it clear that scroll_threshold is optional --- lib/plausible_web/live/goal_settings/form.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/plausible_web/live/goal_settings/form.ex b/lib/plausible_web/live/goal_settings/form.ex index cd4a950df0f9..c66c63b56d52 100644 --- a/lib/plausible_web/live/goal_settings/form.ex +++ b/lib/plausible_web/live/goal_settings/form.ex @@ -199,7 +199,7 @@ defmodule PlausibleWeb.Live.GoalSettings.Form do <.input :if={Plausible.Stats.ScrollDepth.feature_visible?(@site, @current_user)} - label="Scroll Depth Threshold" + label="Scroll Depth Threshold (optional)" field={@f[:scroll_threshold]} type="number" value={if @goal && @goal.scroll_threshold > -1, do: @goal.scroll_threshold, else: nil} From ec5f801635a50d52372676350ce913b9185a7468 Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Wed, 29 Jan 2025 15:33:42 +0100 Subject: [PATCH 10/26] avoid calling Plausible.Goal.type() too many times --- lib/plausible/goals/filters.ex | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/lib/plausible/goals/filters.ex b/lib/plausible/goals/filters.ex index d7ae58dbb38a..a1e6fbd123f4 100644 --- a/lib/plausible/goals/filters.ex +++ b/lib/plausible/goals/filters.ex @@ -106,31 +106,28 @@ defmodule Plausible.Goals.Filters do defp build_condition(filtered_goals, imported?) do Enum.reduce(filtered_goals, false, fn goal, dynamic_statement -> - cond do - is_nil(goal) -> - dynamic([e], ^dynamic_statement) - - Plausible.Goal.type(goal) == :event -> - dynamic([e], e.name == ^goal.event_name or ^dynamic_statement) - - Plausible.Goal.type(goal) == :scroll -> - dynamic([e], ^page_scroll_goal_condition(goal) or ^dynamic_statement) - - Plausible.Goal.type(goal) == :page -> - dynamic([e], ^pageview_goal_condition(goal, imported?) or ^dynamic_statement) + if is_nil(goal) do + dynamic([e], ^dynamic_statement) + else + type = Plausible.Goal.type(goal) + dynamic([e], ^goal_condition(type, goal, imported?) or ^dynamic_statement) end end) end - defp page_scroll_goal_condition(goal) do - pathname_condition = page_path_condition(goal.page_path, _imported = false) + defp goal_condition(:event, goal, _) do + dynamic([e], e.name == ^goal.event_name) + end + + defp goal_condition(:scroll, goal, _) do + pathname_condition = page_path_condition(goal.page_path, _imported? = false) name_condition = dynamic([e], e.name == "pageleave") scroll_condition = dynamic([e], e.scroll_depth >= ^goal.scroll_threshold) dynamic([e], ^pathname_condition and ^name_condition and ^scroll_condition) end - defp pageview_goal_condition(goal, imported?) do + defp goal_condition(:page, goal, imported?) do pathname_condition = page_path_condition(goal.page_path, imported?) name_condition = if imported?, do: true, else: dynamic([e], e.name == "pageview") From 1ac92fbe7601ad2696af7eac80e983e85cd244f5 Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Wed, 29 Jan 2025 15:55:46 +0100 Subject: [PATCH 11/26] do not consider 255 scroll depth a conversion --- lib/plausible/goals/filters.ex | 4 +- lib/plausible/stats/sql/query_builder.ex | 3 +- .../query_goal_dimension_test.exs | 68 +++++++++++++++++++ 3 files changed, 73 insertions(+), 2 deletions(-) diff --git a/lib/plausible/goals/filters.ex b/lib/plausible/goals/filters.ex index a1e6fbd123f4..59725a4e6d7a 100644 --- a/lib/plausible/goals/filters.ex +++ b/lib/plausible/goals/filters.ex @@ -122,7 +122,9 @@ defmodule Plausible.Goals.Filters do defp goal_condition(:scroll, goal, _) do pathname_condition = page_path_condition(goal.page_path, _imported? = false) name_condition = dynamic([e], e.name == "pageleave") - scroll_condition = dynamic([e], e.scroll_depth >= ^goal.scroll_threshold) + + scroll_condition = + dynamic([e], e.scroll_depth <= 100 and e.scroll_depth >= ^goal.scroll_threshold) dynamic([e], ^pathname_condition and ^name_condition and ^scroll_condition) end diff --git a/lib/plausible/stats/sql/query_builder.ex b/lib/plausible/stats/sql/query_builder.ex index 9538449477ea..040de0010865 100644 --- a/lib/plausible/stats/sql/query_builder.ex +++ b/lib/plausible/stats/sql/query_builder.ex @@ -162,7 +162,7 @@ defmodule Plausible.Stats.SQL.QueryBuilder do WHEN ?[goal_index] = 'page' THEN ? = 'pageview' AND match(?, ?[goal_index]) ELSE - ? = 'pageleave' AND match(?, ?[goal_index]) AND ? >= ?[goal_index] + ? = 'pageleave' AND match(?, ?[goal_index]) AND ? <= 100 AND ? >= ?[goal_index] end , ? @@ -183,6 +183,7 @@ defmodule Plausible.Stats.SQL.QueryBuilder do e.pathname, type(^goal_page_regexes, {:array, :string}), e.scroll_depth, + e.scroll_depth, type(^goal_scroll_thresholds, {:array, :integer}), # Array of indices getting filtered type(^goal_indices, {:array, :integer}) diff --git a/test/plausible_web/controllers/api/external_stats_controller/query_goal_dimension_test.exs b/test/plausible_web/controllers/api/external_stats_controller/query_goal_dimension_test.exs index 797d6d371818..a1d61286722a 100644 --- a/test/plausible_web/controllers/api/external_stats_controller/query_goal_dimension_test.exs +++ b/test/plausible_web/controllers/api/external_stats_controller/query_goal_dimension_test.exs @@ -132,6 +132,74 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryGoalDimensionTest do end describe "page scroll goals" do + test "a scroll depth of 255 (missing) is not considered a conversion (page scroll goal filter)", + %{ + conn: conn, + site: site + } do + insert(:goal, + site: site, + page_path: "/blog", + scroll_threshold: 90, + display_name: "Scroll /blog 90" + ) + + populate_stats(site, [ + build(:pageview, user_id: 12, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), + build(:pageleave, + user_id: 12, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:00], + scroll_depth: 255 + ) + ]) + + conn = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "metrics" => ["visitors"], + "filters" => [["is", "event:goal", ["Scroll /blog 90"]]], + "date_range" => "all" + }) + + assert json_response(conn, 200)["results"] == [ + %{"dimensions" => [], "metrics" => [0]} + ] + end + + test "a scroll depth of 255 (missing) is not considered a conversion (page scroll goal breakdown)", + %{ + conn: conn, + site: site + } do + insert(:goal, + site: site, + page_path: "/blog", + scroll_threshold: 90, + display_name: "Scroll /blog 90" + ) + + populate_stats(site, [ + build(:pageview, user_id: 12, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), + build(:pageleave, + user_id: 12, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:00], + scroll_depth: 255 + ) + ]) + + conn = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "metrics" => ["visitors"], + "date_range" => "all", + "dimensions" => ["event:goal"] + }) + + assert json_response(conn, 200)["results"] == [] + end + test "returns page scroll goals and pageview goals in breakdown with page filter", %{ conn: conn, site: site From 08883b08fa59f75ef41366cd532d1f3983d57714 Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Thu, 31 Oct 2024 12:09:21 +0100 Subject: [PATCH 12/26] migration: add scroll_threshold to goals --- ...28161815_add_scroll_threshold_to_goals.exs | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 priv/repo/migrations/20250128161815_add_scroll_threshold_to_goals.exs diff --git a/priv/repo/migrations/20250128161815_add_scroll_threshold_to_goals.exs b/priv/repo/migrations/20250128161815_add_scroll_threshold_to_goals.exs new file mode 100644 index 000000000000..935f2919e8db --- /dev/null +++ b/priv/repo/migrations/20250128161815_add_scroll_threshold_to_goals.exs @@ -0,0 +1,39 @@ +defmodule Plausible.Repo.Migrations.AddScrollThresholdToGoals do + use Ecto.Migration + + @disable_ddl_transaction true + @disable_migration_lock true + + # Plausible.Repo.Migrations.GoalsUnique + @old_index unique_index( + :goals, + [:site_id, :page_path], + where: "page_path IS NOT NULL", + name: :goals_page_path_unique + ) + + @new_index unique_index( + :goals, + [:site_id, :page_path, :scroll_threshold], + where: "page_path IS NOT NULL", + name: :goals_page_path_and_scroll_threshold_unique + ) + + def up do + alter table(:goals) do + add :scroll_threshold, :smallint, null: false, default: -1 + end + + drop(@old_index) + create(@new_index) + end + + def down do + drop(@new_index) + create(@old_index) + + alter table(:goals) do + remove :scroll_threshold + end + end +end From 04397934a7ab7004b7721861db45fc8dfdf9110f Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Thu, 30 Jan 2025 12:13:23 +0100 Subject: [PATCH 13/26] do not drop the old index yet --- .../20250128161815_add_scroll_threshold_to_goals.exs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/priv/repo/migrations/20250128161815_add_scroll_threshold_to_goals.exs b/priv/repo/migrations/20250128161815_add_scroll_threshold_to_goals.exs index 935f2919e8db..3392a11d1fde 100644 --- a/priv/repo/migrations/20250128161815_add_scroll_threshold_to_goals.exs +++ b/priv/repo/migrations/20250128161815_add_scroll_threshold_to_goals.exs @@ -4,14 +4,6 @@ defmodule Plausible.Repo.Migrations.AddScrollThresholdToGoals do @disable_ddl_transaction true @disable_migration_lock true - # Plausible.Repo.Migrations.GoalsUnique - @old_index unique_index( - :goals, - [:site_id, :page_path], - where: "page_path IS NOT NULL", - name: :goals_page_path_unique - ) - @new_index unique_index( :goals, [:site_id, :page_path, :scroll_threshold], @@ -24,13 +16,11 @@ defmodule Plausible.Repo.Migrations.AddScrollThresholdToGoals do add :scroll_threshold, :smallint, null: false, default: -1 end - drop(@old_index) create(@new_index) end def down do drop(@new_index) - create(@old_index) alter table(:goals) do remove :scroll_threshold From dbaf9b3bf56080ab3b40b50a06c118c6d5c3d9a5 Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Thu, 30 Jan 2025 13:23:34 +0200 Subject: [PATCH 14/26] More efficient goals join again --- lib/plausible/goals/goals.ex | 26 ++++++++++-- lib/plausible/stats/imported/imported.ex | 26 +++++------- lib/plausible/stats/sql/expression.ex | 22 +++++++---- lib/plausible/stats/sql/query_builder.ex | 50 ++---------------------- 4 files changed, 50 insertions(+), 74 deletions(-) diff --git a/lib/plausible/goals/goals.ex b/lib/plausible/goals/goals.ex index 4579ee0ed4eb..2e0d05221e27 100644 --- a/lib/plausible/goals/goals.ex +++ b/lib/plausible/goals/goals.ex @@ -288,10 +288,15 @@ defmodule Plausible.Goals do :ok end + # :TODO: Move to Plausible.Stats.Goals + + @match_all_ch_regex ".*" + @type goal_decomposition() :: %{ indices: [non_neg_integer()], types: [String.t()], - event_names: [String.t()], + event_names_imports: [String.t()], + event_names_by_type: [String.t()], page_regexes: [String.t()], scroll_thresholds: [non_neg_integer()] } @@ -301,10 +306,25 @@ defmodule Plausible.Goals do %{ indices: Enum.with_index(goals, 1) |> Enum.map(fn {_goal, idx} -> idx end), types: Enum.map(goals, &to_string(Plausible.Goal.type(&1))), - event_names: Enum.map(goals, &to_string(&1.event_name)), + # :TRICKY: This will contain "" for non-event goals + event_names_imports: Enum.map(goals, &to_string(&1.event_name)), + event_names_by_type: + Enum.map(goals, fn goal -> + case Plausible.Goal.type(goal) do + :event -> goal.event_name + :page -> "pageview" + :scroll -> "pageleave" + end + end), + # :TRICKY: event goals are considered to match everything for the sake of efficient queries in query_builder.ex + # See also Plausible.Stats.SQL.Expression#event_goal_join page_regexes: Enum.map(goals, fn goal -> - if goal.page_path, do: Filters.Utils.page_regex(goal.page_path), else: "" + case Plausible.Goal.type(goal) do + :event -> @match_all_ch_regex + :page -> Filters.Utils.page_regex(goal.page_path) + :scroll -> Filters.Utils.page_regex(goal.page_path) + end end), scroll_thresholds: Enum.map(goals, & &1.scroll_threshold) } diff --git a/lib/plausible/stats/imported/imported.ex b/lib/plausible/stats/imported/imported.ex index 8b6876f04496..8b1e46e9db2d 100644 --- a/lib/plausible/stats/imported/imported.ex +++ b/lib/plausible/stats/imported/imported.ex @@ -238,12 +238,7 @@ defmodule Plausible.Stats.Imported do end def merge_imported(q, site, %Query{dimensions: ["event:goal"]} = query, metrics) do - %{ - indices: goal_indices, - types: goal_types, - event_names: goal_event_names, - page_regexes: goal_page_regexes - } = + goal_join_data = query.preloaded_goals.matching_toplevel_filters |> Plausible.Goals.decompose() @@ -254,13 +249,10 @@ defmodule Plausible.Stats.Imported do |> where([i], i.visitors > 0) |> select_merge_as([i], %{ dim0: - type( - fragment( - "indexOf(?, ?)", - type(^goal_event_names, {:array, :string}), - i.name - ), - :integer + fragment( + "indexOf(?, ?)", + type(^goal_join_data.event_names_imports, {:array, :string}), + i.name ) }) |> select_imported_metrics(metrics) @@ -281,16 +273,16 @@ defmodule Plausible.Stats.Imported do ) as indices ) """, - type(^goal_types, {:array, :string}), + type(^goal_join_data.types, {:array, :string}), i.page, - type(^goal_page_regexes, {:array, :string}), - type(^goal_indices, {:array, :integer}) + type(^goal_join_data.page_regexes, {:array, :string}), + type(^goal_join_data.indices, {:array, :integer}) ) ) |> join(:inner, [_i], index in fragment("indices"), hints: "ARRAY", on: true) |> group_by([_i, index], index) |> select_merge_as([_i, index], %{ - dim0: type(fragment("?", index), :integer) + dim0: fragment("CAST(?, 'UInt64')", index) }) |> select_imported_metrics(metrics) end) diff --git a/lib/plausible/stats/sql/expression.ex b/lib/plausible/stats/sql/expression.ex index 8c97fe783771..ed6bbb66ff0c 100644 --- a/lib/plausible/stats/sql/expression.ex +++ b/lib/plausible/stats/sql/expression.ex @@ -328,19 +328,27 @@ defmodule Plausible.Stats.SQL.Expression do def session_metric(:conversion_rate, _query), do: %{} def session_metric(:group_conversion_rate, _query), do: %{} - defmacro event_goal_join(events, page_regexes) do + defmacro event_goal_join(goal_join_data) do quote do fragment( """ - arrayPushFront( - CAST(multiMatchAllIndices(?, ?) AS Array(Int64)), - -indexOf(?, ?) + arrayIntersect( + multiMatchAllIndices(?, ?), + arrayMap( + (expected_name, threshold, index) -> if(expected_name = ? and ? between threshold and 254, index, -1), + ?, + ?, + ? + ) ) """, e.pathname, - type(^unquote(page_regexes), {:array, :string}), - type(^unquote(events), {:array, :string}), - e.name + type(^unquote(goal_join_data).page_regexes, {:array, :string}), + e.name, + e.scroll_depth, + type(^unquote(goal_join_data).event_names_by_type, {:array, :string}), + type(^unquote(goal_join_data).scroll_thresholds, {:array, :integer}), + type(^unquote(goal_join_data).indices, {:array, :integer}) ) end end diff --git a/lib/plausible/stats/sql/query_builder.ex b/lib/plausible/stats/sql/query_builder.ex index 040de0010865..951f5094c610 100644 --- a/lib/plausible/stats/sql/query_builder.ex +++ b/lib/plausible/stats/sql/query_builder.ex @@ -138,57 +138,13 @@ defmodule Plausible.Stats.SQL.QueryBuilder do Enum.reduce(query.dimensions, q, &dimension_group_by(&2, table, query, &1)) end - defp dimension_group_by(q, _table, query, "event:goal" = dimension) do - %{ - indices: goal_indices, - types: goal_types, - event_names: goal_event_names, - page_regexes: goal_page_regexes, - scroll_thresholds: goal_scroll_thresholds - } = + defp dimension_group_by(q, :events, query, "event:goal" = dimension) do + goal_join_data = query.preloaded_goals.matching_toplevel_filters |> Plausible.Goals.decompose() from(e in q, - where: - fragment( - """ - notEmpty( - arrayFilter( - goal_index -> - CASE - WHEN ?[goal_index] = 'event' THEN - ? = ?[goal_index] - WHEN ?[goal_index] = 'page' THEN - ? = 'pageview' AND match(?, ?[goal_index]) - ELSE - ? = 'pageleave' AND match(?, ?[goal_index]) AND ? <= 100 AND ? >= ?[goal_index] - end - , - ? - ) as indices - ) - """, - # CASE 1 - custom event goals - type(^goal_types, {:array, :string}), - e.name, - type(^goal_event_names, {:array, :string}), - # CASE 2 - pageview goals - type(^goal_types, {:array, :string}), - e.name, - e.pathname, - type(^goal_page_regexes, {:array, :string}), - # CASE 3 - page scroll goals - e.name, - e.pathname, - type(^goal_page_regexes, {:array, :string}), - e.scroll_depth, - e.scroll_depth, - type(^goal_scroll_thresholds, {:array, :integer}), - # Array of indices getting filtered - type(^goal_indices, {:array, :integer}) - ), - join: goal in fragment("indices"), + join: goal in Expression.event_goal_join(goal_join_data), hints: "ARRAY", on: true, select_merge: %{ From 8f28b2837c6ded38432b9a0945cb8fbb74cd6dd2 Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Thu, 30 Jan 2025 13:25:50 +0200 Subject: [PATCH 15/26] Refactor: move goals stats code explicitly under Stats.Goals module --- lib/plausible/stats/filters/query_parser.ex | 2 +- lib/plausible/{goals/filters.ex => stats/goals.ex} | 6 ++++-- lib/plausible/stats/imported/sql/where_builder.ex | 2 +- lib/plausible/stats/sql/where_builder.ex | 2 +- 4 files changed, 7 insertions(+), 5 deletions(-) rename lib/plausible/{goals/filters.ex => stats/goals.ex} (97%) diff --git a/lib/plausible/stats/filters/query_parser.ex b/lib/plausible/stats/filters/query_parser.ex index 751761ac4589..38fd78fbe570 100644 --- a/lib/plausible/stats/filters/query_parser.ex +++ b/lib/plausible/stats/filters/query_parser.ex @@ -445,7 +445,7 @@ defmodule Plausible.Stats.Filters.QueryParser do def preload_goals_and_revenue(site, metrics, filters, dimensions) do preloaded_goals = - Plausible.Goals.Filters.preload_needed_goals(site, dimensions, filters) + Plausible.Stats.Goals.preload_needed_goals(site, dimensions, filters) {revenue_warning, revenue_currencies} = preload_revenue(site, preloaded_goals, metrics, dimensions) diff --git a/lib/plausible/goals/filters.ex b/lib/plausible/stats/goals.ex similarity index 97% rename from lib/plausible/goals/filters.ex rename to lib/plausible/stats/goals.ex index 59725a4e6d7a..d0612f060fdc 100644 --- a/lib/plausible/goals/filters.ex +++ b/lib/plausible/stats/goals.ex @@ -1,5 +1,7 @@ -defmodule Plausible.Goals.Filters do - @moduledoc false +defmodule Plausible.Stats.Goals do + @moduledoc """ + Stats code related to filtering and grouping by goals. + """ import Ecto.Query import Plausible.Stats.Filters.Utils, only: [page_regex: 1] diff --git a/lib/plausible/stats/imported/sql/where_builder.ex b/lib/plausible/stats/imported/sql/where_builder.ex index 93d847621032..a9b8437265af 100644 --- a/lib/plausible/stats/imported/sql/where_builder.ex +++ b/lib/plausible/stats/imported/sql/where_builder.ex @@ -53,7 +53,7 @@ defmodule Plausible.Stats.Imported.SQL.WhereBuilder do db_field = Plausible.Stats.Filters.without_prefix(dimension) if db_field == :goal do - Plausible.Goals.Filters.add_filter(query, filter, imported?: true) + Plausible.Stats.Goals.add_filter(query, filter, imported?: true) else mapped_db_field = Map.get(@db_field_mappings, db_field, db_field) diff --git a/lib/plausible/stats/sql/where_builder.ex b/lib/plausible/stats/sql/where_builder.ex index e19e691bd450..ad32ac961f77 100644 --- a/lib/plausible/stats/sql/where_builder.ex +++ b/lib/plausible/stats/sql/where_builder.ex @@ -113,7 +113,7 @@ defmodule Plausible.Stats.SQL.WhereBuilder do end defp add_filter(:events, query, [_, "event:goal" | _rest] = filter) do - Plausible.Goals.Filters.add_filter(query, filter) + Plausible.Stats.Goals.add_filter(query, filter) end defp add_filter(:events, _query, [_, "event:page" | _rest] = filter) do From 7e2e7805dc59f57603b5a6e14a18ade6c32f1104 Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Thu, 30 Jan 2025 13:31:54 +0200 Subject: [PATCH 16/26] Move code under Plausible.Stats.Goals --- lib/plausible/goals/goals.ex | 43 ------------------------ lib/plausible/stats/goals.ex | 43 ++++++++++++++++++++++++ lib/plausible/stats/imported/imported.ex | 4 +-- lib/plausible/stats/sql/query_builder.ex | 4 +-- 4 files changed, 45 insertions(+), 49 deletions(-) diff --git a/lib/plausible/goals/goals.ex b/lib/plausible/goals/goals.ex index 2e0d05221e27..af17a254f4eb 100644 --- a/lib/plausible/goals/goals.ex +++ b/lib/plausible/goals/goals.ex @@ -6,7 +6,6 @@ defmodule Plausible.Goals do import Ecto.Query alias Plausible.Goal - alias Plausible.Stats.Filters alias Ecto.Multi @spec get(Plausible.Site.t(), pos_integer()) :: nil | Plausible.Goal.t() @@ -288,48 +287,6 @@ defmodule Plausible.Goals do :ok end - # :TODO: Move to Plausible.Stats.Goals - - @match_all_ch_regex ".*" - - @type goal_decomposition() :: %{ - indices: [non_neg_integer()], - types: [String.t()], - event_names_imports: [String.t()], - event_names_by_type: [String.t()], - page_regexes: [String.t()], - scroll_thresholds: [non_neg_integer()] - } - - @spec decompose([Plausible.Goal.t()]) :: goal_decomposition() - def decompose(goals) do - %{ - indices: Enum.with_index(goals, 1) |> Enum.map(fn {_goal, idx} -> idx end), - types: Enum.map(goals, &to_string(Plausible.Goal.type(&1))), - # :TRICKY: This will contain "" for non-event goals - event_names_imports: Enum.map(goals, &to_string(&1.event_name)), - event_names_by_type: - Enum.map(goals, fn goal -> - case Plausible.Goal.type(goal) do - :event -> goal.event_name - :page -> "pageview" - :scroll -> "pageleave" - end - end), - # :TRICKY: event goals are considered to match everything for the sake of efficient queries in query_builder.ex - # See also Plausible.Stats.SQL.Expression#event_goal_join - page_regexes: - Enum.map(goals, fn goal -> - case Plausible.Goal.type(goal) do - :event -> @match_all_ch_regex - :page -> Filters.Utils.page_regex(goal.page_path) - :scroll -> Filters.Utils.page_regex(goal.page_path) - end - end), - scroll_thresholds: Enum.map(goals, & &1.scroll_threshold) - } - end - defp insert_goal(site, params, upsert?) do params = Map.delete(params, "site_id") diff --git a/lib/plausible/stats/goals.ex b/lib/plausible/stats/goals.ex index d0612f060fdc..e6ccb96fc3c4 100644 --- a/lib/plausible/stats/goals.ex +++ b/lib/plausible/stats/goals.ex @@ -61,6 +61,49 @@ defmodule Plausible.Stats.Goals do end) end + @type goal_join_data() :: %{ + indices: [non_neg_integer()], + types: [String.t()], + event_names_imports: [String.t()], + event_names_by_type: [String.t()], + page_regexes: [String.t()], + scroll_thresholds: [non_neg_integer()] + } + + @doc """ + Returns data needed to perform a GROUP BY on goals in an ecto query. + """ + @spec goal_join_data(Plausible.Stats.Query.t()) :: goal_join_data() + def goal_join_data(query) do + goals = query.preloaded_goals.matching_toplevel_filters + + %{ + indices: Enum.with_index(goals, 1) |> Enum.map(fn {_goal, idx} -> idx end), + types: Enum.map(goals, &to_string(Plausible.Goal.type(&1))), + # :TRICKY: This will contain "" for non-event goals + event_names_imports: Enum.map(goals, &to_string(&1.event_name)), + event_names_by_type: + Enum.map(goals, fn goal -> + case Plausible.Goal.type(goal) do + :event -> goal.event_name + :page -> "pageview" + :scroll -> "pageleave" + end + end), + # :TRICKY: event goals are considered to match everything for the sake of efficient queries in query_builder.ex + # See also Plausible.Stats.SQL.Expression#event_goal_join + page_regexes: + Enum.map(goals, fn goal -> + case Plausible.Goal.type(goal) do + :event -> ".?" + :page -> Filters.Utils.page_regex(goal.page_path) + :scroll -> Filters.Utils.page_regex(goal.page_path) + end + end), + scroll_thresholds: Enum.map(goals, & &1.scroll_threshold) + } + end + defp filter_preloaded(goals, filter, clause) do Enum.filter(goals, fn goal -> matches?(goal, filter, clause) end) end diff --git a/lib/plausible/stats/imported/imported.ex b/lib/plausible/stats/imported/imported.ex index 8b1e46e9db2d..f8373493ac43 100644 --- a/lib/plausible/stats/imported/imported.ex +++ b/lib/plausible/stats/imported/imported.ex @@ -238,9 +238,7 @@ defmodule Plausible.Stats.Imported do end def merge_imported(q, site, %Query{dimensions: ["event:goal"]} = query, metrics) do - goal_join_data = - query.preloaded_goals.matching_toplevel_filters - |> Plausible.Goals.decompose() + goal_join_data = Plausible.Stats.Goals.goal_join_data(query) Imported.Base.decide_tables(query) |> Enum.map(fn diff --git a/lib/plausible/stats/sql/query_builder.ex b/lib/plausible/stats/sql/query_builder.ex index 951f5094c610..942222f2e3a2 100644 --- a/lib/plausible/stats/sql/query_builder.ex +++ b/lib/plausible/stats/sql/query_builder.ex @@ -139,9 +139,7 @@ defmodule Plausible.Stats.SQL.QueryBuilder do end defp dimension_group_by(q, :events, query, "event:goal" = dimension) do - goal_join_data = - query.preloaded_goals.matching_toplevel_filters - |> Plausible.Goals.decompose() + goal_join_data = Plausible.Stats.Goals.goal_join_data(query) from(e in q, join: goal in Expression.event_goal_join(goal_join_data), From ebf0dfbc26edf290a456e164633f133be4b8b346 Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Thu, 30 Jan 2025 13:43:43 +0200 Subject: [PATCH 17/26] 254 -> 100 --- lib/plausible/stats/sql/expression.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/plausible/stats/sql/expression.ex b/lib/plausible/stats/sql/expression.ex index ed6bbb66ff0c..f4f9a57be8a5 100644 --- a/lib/plausible/stats/sql/expression.ex +++ b/lib/plausible/stats/sql/expression.ex @@ -335,7 +335,7 @@ defmodule Plausible.Stats.SQL.Expression do arrayIntersect( multiMatchAllIndices(?, ?), arrayMap( - (expected_name, threshold, index) -> if(expected_name = ? and ? between threshold and 254, index, -1), + (expected_name, threshold, index) -> if(expected_name = ? and ? between threshold and 100, index, -1), ?, ?, ? From 82e70dc907a6bbbf140c2e82b418b42adb78eb5b Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Thu, 30 Jan 2025 14:15:56 +0100 Subject: [PATCH 18/26] add scroll_threshold field to goal schema + new unique constraint --- lib/plausible/goal/schema.ex | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/plausible/goal/schema.ex b/lib/plausible/goal/schema.ex index 8f2e29febe01..84d76cb991ff 100644 --- a/lib/plausible/goal/schema.ex +++ b/lib/plausible/goal/schema.ex @@ -8,6 +8,7 @@ defmodule Plausible.Goal do schema "goals" do field :event_name, :string field :page_path, :string + field :scroll_threshold, :integer, default: -1 field :display_name, :string on_ee do @@ -23,7 +24,7 @@ defmodule Plausible.Goal do timestamps() end - @fields [:id, :site_id, :event_name, :page_path, :display_name] ++ + @fields [:id, :site_id, :event_name, :page_path, :scroll_threshold, :display_name] ++ on_ee(do: [:currency], else: []) @max_event_name_length 120 @@ -40,6 +41,9 @@ defmodule Plausible.Goal do |> maybe_put_display_name() |> unique_constraint(:event_name, name: :goals_event_name_unique) |> unique_constraint(:page_path, name: :goals_page_path_unique) + |> unique_constraint([:page_path, :scroll_threshold], + name: :goals_page_path_and_scroll_threshold_unique + ) |> unique_constraint(:display_name, name: :goals_site_id_display_name_index) |> validate_length(:event_name, max: @max_event_name_length) |> check_constraint(:event_name, From 3d0a13170221328e5d7a65f4d30d3dfb82fae5c3 Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Thu, 30 Jan 2025 14:29:39 +0100 Subject: [PATCH 19/26] adjust test to test what it claims to --- test/plausible/goals_test.exs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/plausible/goals_test.exs b/test/plausible/goals_test.exs index ede9287ba3a6..d553573271e3 100644 --- a/test/plausible/goals_test.exs +++ b/test/plausible/goals_test.exs @@ -34,8 +34,8 @@ defmodule Plausible.GoalsTest do test "create/2 fails to create the same pageview goal twice" do site = new_site() - {:ok, _} = Goals.create(site, %{"page_path" => "foo bar"}) - assert {:error, changeset} = Goals.create(site, %{"page_path" => "foo bar"}) + {:ok, _} = Goals.create(site, %{"page_path" => "foo bar", "display_name" => "one"}) + assert {:error, changeset} = Goals.create(site, %{"page_path" => "foo bar", "display_name" => "two"}) assert {"has already been taken", _} = changeset.errors[:page_path] end From 72e2934974a406cd2e9f3ad798172def7f4ce64a Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Thu, 30 Jan 2025 14:41:21 +0100 Subject: [PATCH 20/26] mix format --- test/plausible/goals_test.exs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/plausible/goals_test.exs b/test/plausible/goals_test.exs index d553573271e3..d57cedfceafc 100644 --- a/test/plausible/goals_test.exs +++ b/test/plausible/goals_test.exs @@ -35,7 +35,10 @@ defmodule Plausible.GoalsTest do test "create/2 fails to create the same pageview goal twice" do site = new_site() {:ok, _} = Goals.create(site, %{"page_path" => "foo bar", "display_name" => "one"}) - assert {:error, changeset} = Goals.create(site, %{"page_path" => "foo bar", "display_name" => "two"}) + + assert {:error, changeset} = + Goals.create(site, %{"page_path" => "foo bar", "display_name" => "two"}) + assert {"has already been taken", _} = changeset.errors[:page_path] end From d17893dc761fcc1cf80977f7487c816bd91501b9 Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Thu, 30 Jan 2025 14:27:43 +0100 Subject: [PATCH 21/26] add migration --- ...unique_page_path_constraint_from_goals.exs | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 priv/repo/migrations/20250130121019_drop_unique_page_path_constraint_from_goals.exs diff --git a/priv/repo/migrations/20250130121019_drop_unique_page_path_constraint_from_goals.exs b/priv/repo/migrations/20250130121019_drop_unique_page_path_constraint_from_goals.exs new file mode 100644 index 000000000000..aa4a86387fe0 --- /dev/null +++ b/priv/repo/migrations/20250130121019_drop_unique_page_path_constraint_from_goals.exs @@ -0,0 +1,22 @@ +defmodule Plausible.Repo.Migrations.DropUniquePagePathConstraintFromGoals do + use Ecto.Migration + + @disable_ddl_transaction true + @disable_migration_lock true + + # Plausible.Repo.Migrations.GoalsUnique + @old_index unique_index( + :goals, + [:site_id, :page_path], + where: "page_path IS NOT NULL", + name: :goals_page_path_unique + ) + + def up do + drop(@old_index) + end + + def down do + create(@old_index) + end +end From 890690e35ff8c8330177e47d89e13460efa31394 Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Mon, 3 Feb 2025 13:52:47 +0100 Subject: [PATCH 22/26] consider imported query unsupported when page scroll goal filter --- lib/plausible/stats/goals.ex | 2 +- lib/plausible/stats/imported/base.ex | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/plausible/stats/goals.ex b/lib/plausible/stats/goals.ex index e6ccb96fc3c4..85eff52a16f5 100644 --- a/lib/plausible/stats/goals.ex +++ b/lib/plausible/stats/goals.ex @@ -164,7 +164,7 @@ defmodule Plausible.Stats.Goals do dynamic([e], e.name == ^goal.event_name) end - defp goal_condition(:scroll, goal, _) do + defp goal_condition(:scroll, goal, _imported? = false) do pathname_condition = page_path_condition(goal.page_path, _imported? = false) name_condition = dynamic([e], e.name == "pageleave") diff --git a/lib/plausible/stats/imported/base.ex b/lib/plausible/stats/imported/base.ex index 4346f4d71fe1..e0720acb7040 100644 --- a/lib/plausible/stats/imported/base.ex +++ b/lib/plausible/stats/imported/base.ex @@ -191,6 +191,7 @@ defmodule Plausible.Stats.Imported.Base do |> Enum.map(fn :event -> "imported_custom_events" :page -> "imported_pages" + :scroll -> nil end) case Enum.uniq(table_candidates ++ filter_goal_table_candidates) do From f8914e6961e60123070890a1649433069d25717c Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Mon, 3 Feb 2025 13:53:10 +0100 Subject: [PATCH 23/26] add missing tests --- .../query_goal_dimension_test.exs | 55 ++++++ .../query_imported_test.exs | 172 +++++++++++++++++- 2 files changed, 218 insertions(+), 9 deletions(-) diff --git a/test/plausible_web/controllers/api/external_stats_controller/query_goal_dimension_test.exs b/test/plausible_web/controllers/api/external_stats_controller/query_goal_dimension_test.exs index a1d61286722a..fb6f6021eef6 100644 --- a/test/plausible_web/controllers/api/external_stats_controller/query_goal_dimension_test.exs +++ b/test/plausible_web/controllers/api/external_stats_controller/query_goal_dimension_test.exs @@ -328,6 +328,61 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryGoalDimensionTest do ] end + test "returns group_conversion_rate for a page scroll goal filter in source breakdown", %{ + conn: conn, + site: site + } do + insert(:goal, + site: site, + page_path: "/blog", + scroll_threshold: 50, + display_name: "Scroll /blog 50" + ) + + populate_stats(site, [ + build(:pageview, referrer_source: "Twitter"), + build(:pageview, + user_id: 12, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:00], + referrer_source: "Google" + ), + build(:pageleave, + user_id: 12, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 30, + referrer_source: "Google" + ), + build(:pageview, + user_id: 34, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:00], + referrer_source: "Twitter" + ), + build(:pageleave, + user_id: 34, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 50, + referrer_source: "Twitter" + ) + ]) + + conn = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "metrics" => ["visitors", "events", "group_conversion_rate"], + "date_range" => "all", + "filters" => [["is", "event:goal", ["Scroll /blog 50"]]], + "dimensions" => ["visit:source"] + }) + + assert json_response(conn, 200)["results"] == [ + %{"dimensions" => ["Twitter"], "metrics" => [1, 0, 50.0]} + ] + end + test "breakdown by event:props:author with a page scroll goal filter", %{ conn: conn, site: site diff --git a/test/plausible_web/controllers/api/external_stats_controller/query_imported_test.exs b/test/plausible_web/controllers/api/external_stats_controller/query_imported_test.exs index 47c2035f0214..764eb2a2a5c6 100644 --- a/test/plausible_web/controllers/api/external_stats_controller/query_imported_test.exs +++ b/test/plausible_web/controllers/api/external_stats_controller/query_imported_test.exs @@ -3,6 +3,9 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do @user_id Enum.random(1000..9999) + @unsupported_interval_warning "Imported stats are not included because the time dimension (i.e. the interval) is too short." + @unsupported_query_warning "Imported stats are not included in the results because query parameters are not supported. For more information, see: https://plausible.io/docs/stats-api#filtering-imported-stats" + setup [:create_user, :create_site, :create_api_key, :use_api_key] describe "aggregation with imported data" do @@ -880,8 +883,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do refute json_response(conn, 200)["meta"]["imports_included"] assert json_response(conn, 200)["meta"]["imports_skip_reason"] == "unsupported_interval" - assert json_response(conn, 200)["meta"]["imports_warning"] =~ - "Imported stats are not included because the time dimension (i.e. the interval) is too short." + assert json_response(conn, 200)["meta"]["imports_warning"] == @unsupported_interval_warning end test "adds a warning when query params are not supported for imported data", %{ @@ -920,8 +922,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do refute json_response(conn, 200)["meta"]["imports_included"] assert json_response(conn, 200)["meta"]["imports_skip_reason"] == "unsupported_query" - assert json_response(conn, 200)["meta"]["imports_warning"] =~ - "Imported stats are not included in the results because query parameters are not supported." + assert json_response(conn, 200)["meta"]["imports_warning"] == @unsupported_query_warning end test "does not add a warning when there are no site imports", %{conn: conn, site: site} do @@ -1052,7 +1053,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do } = json_response(conn, 200) refute json_response(conn, 200)["meta"]["imports_included"] - assert meta["imports_warning"] =~ "Imported stats are not included in the results" + assert meta["imports_warning"] == @unsupported_query_warning end test "imported country, region and city data", @@ -1322,8 +1323,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do assert json_response(conn, 200)["results"] == [%{"dimensions" => [], "metrics" => [2]}] refute json_response(conn, 200)["meta"]["imports_included"] - assert json_response(conn, 200)["meta"]["imports_warning"] =~ - "Imported stats are not included in the results" + assert json_response(conn, 200)["meta"]["imports_warning"] == @unsupported_query_warning end test "imports are skipped when has_not_done filter is used", %{ @@ -1358,8 +1358,162 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do assert json_response(conn, 200)["results"] == [] refute json_response(conn, 200)["meta"]["imports_included"] - assert json_response(conn, 200)["meta"]["imports_warning"] =~ - "Imported stats are not included in the results" + assert json_response(conn, 200)["meta"]["imports_warning"] == @unsupported_query_warning + end + end + + describe "page scroll goals" do + test "rejects imported data with warning when page scroll goal is filtered by", %{ + conn: conn, + site: site + } do + insert(:goal, + site: site, + page_path: "/blog", + scroll_threshold: 50, + display_name: "Scroll /blog 50" + ) + + insert(:goal, site: site, page_path: "/blog**") + + site_import = insert(:site_import, site: site) + + populate_stats(site, site_import.id, [ + build(:pageview, + user_id: 12, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:00] + ), + build(:pageleave, + user_id: 12, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 50 + ), + build(:imported_pages, page: "/blog", date: ~D[2021-01-01]) + ]) + + conn = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "metrics" => ["visitors", "events", "conversion_rate"], + "date_range" => "all", + "filters" => [["is", "event:goal", ["Scroll /blog 50"]]], + "include" => %{"imports" => true} + }) + + assert %{ + "results" => results, + "meta" => %{ + "imports_included" => false, + "imports_skip_reason" => "unsupported_query", + "imports_warning" => @unsupported_query_warning + } + } = json_response(conn, 200) + + assert results == [%{"dimensions" => [], "metrics" => [1, 0, 100.0]}] + end + + test "rejects imported data with warning when page scroll goal is filtered by (IN filter)", %{ + conn: conn, + site: site + } do + insert(:goal, + site: site, + page_path: "/blog", + scroll_threshold: 50, + display_name: "Scroll /blog 50" + ) + + insert(:goal, site: site, page_path: "/blog**") + + site_import = insert(:site_import, site: site) + + populate_stats(site, site_import.id, [ + build(:pageview, + user_id: 12, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:00] + ), + build(:pageleave, + user_id: 12, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 50 + ), + build(:imported_pages, page: "/blog", date: ~D[2021-01-01]) + ]) + + conn = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "metrics" => ["visitors", "events", "conversion_rate"], + "date_range" => "all", + "filters" => [["is", "event:goal", ["Scroll /blog 50", "Visit /blog**"]]], + "include" => %{"imports" => true} + }) + + assert %{ + "results" => results, + "meta" => %{ + "imports_included" => false, + "imports_skip_reason" => "unsupported_query", + "imports_warning" => @unsupported_query_warning + } + } = json_response(conn, 200) + + assert results == [%{"dimensions" => [], "metrics" => [1, 1, 100.0]}] + end + + test "ignores imported data for page scroll goal conversions in goal breakdown", %{ + conn: conn, + site: site + } do + insert(:goal, + site: site, + page_path: "/blog", + scroll_threshold: 50, + display_name: "Scroll /blog 50" + ) + + insert(:goal, site: site, page_path: "/blog**") + + site_import = insert(:site_import, site: site) + + populate_stats(site, site_import.id, [ + build(:pageview, + user_id: 12, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:00] + ), + build(:pageleave, + user_id: 12, + pathname: "/blog", + timestamp: ~N[2021-01-01 00:00:10], + scroll_depth: 50 + ), + build(:imported_pages, page: "/blog"), + build(:imported_visitors) + ]) + + conn = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "metrics" => ["visitors", "events", "conversion_rate"], + "date_range" => "all", + "include" => %{"imports" => true}, + "dimensions" => ["event:goal"] + }) + + assert %{ + "results" => results, + "meta" => %{"imports_included" => true} + } = json_response(conn, 200) + + assert results == [ + %{"dimensions" => ["Visit /blog**"], "metrics" => [2, 2, 100.0]}, + %{"dimensions" => ["Scroll /blog 50"], "metrics" => [1, 0, 50.0]} + ] end end end From 9c51ba801e39fb2af93d23d18580b21af7cf22fc Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Mon, 3 Feb 2025 14:18:48 +0100 Subject: [PATCH 24/26] pattern match imported argument --- lib/plausible/stats/goals.ex | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/plausible/stats/goals.ex b/lib/plausible/stats/goals.ex index 85eff52a16f5..9d19e2c52be2 100644 --- a/lib/plausible/stats/goals.ex +++ b/lib/plausible/stats/goals.ex @@ -174,9 +174,13 @@ defmodule Plausible.Stats.Goals do dynamic([e], ^pathname_condition and ^name_condition and ^scroll_condition) end - defp goal_condition(:page, goal, imported?) do - pathname_condition = page_path_condition(goal.page_path, imported?) - name_condition = if imported?, do: true, else: dynamic([e], e.name == "pageview") + defp goal_condition(:page, goal, _imported? = true) do + page_path_condition(goal.page_path, _imported? = true) + end + + defp goal_condition(:page, goal, _imported? = false) do + name_condition = dynamic([e], e.name == "pageview") + pathname_condition = page_path_condition(goal.page_path, _imported? = false) dynamic([e], ^pathname_condition and ^name_condition) end From 0becb4a7e4804d13e74637f6fba0ad2cebcda70d Mon Sep 17 00:00:00 2001 From: Robert Joonas Date: Mon, 3 Feb 2025 14:22:37 +0100 Subject: [PATCH 25/26] silence credo --- lib/plausible/stats/goals.ex | 6 +++--- lib/plausible/stats/sql/expression.ex | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/plausible/stats/goals.ex b/lib/plausible/stats/goals.ex index 9d19e2c52be2..92a9d9cf474d 100644 --- a/lib/plausible/stats/goals.ex +++ b/lib/plausible/stats/goals.ex @@ -164,7 +164,7 @@ defmodule Plausible.Stats.Goals do dynamic([e], e.name == ^goal.event_name) end - defp goal_condition(:scroll, goal, _imported? = false) do + defp goal_condition(:scroll, goal, false = _imported?) do pathname_condition = page_path_condition(goal.page_path, _imported? = false) name_condition = dynamic([e], e.name == "pageleave") @@ -174,11 +174,11 @@ defmodule Plausible.Stats.Goals do dynamic([e], ^pathname_condition and ^name_condition and ^scroll_condition) end - defp goal_condition(:page, goal, _imported? = true) do + defp goal_condition(:page, goal, true = _imported?) do page_path_condition(goal.page_path, _imported? = true) end - defp goal_condition(:page, goal, _imported? = false) do + defp goal_condition(:page, goal, false = _imported?) do name_condition = dynamic([e], e.name == "pageview") pathname_condition = page_path_condition(goal.page_path, _imported? = false) diff --git a/lib/plausible/stats/sql/expression.ex b/lib/plausible/stats/sql/expression.ex index f4f9a57be8a5..32b0895c2ed7 100644 --- a/lib/plausible/stats/sql/expression.ex +++ b/lib/plausible/stats/sql/expression.ex @@ -196,7 +196,7 @@ defmodule Plausible.Stats.SQL.Expression do }) end - # TODO: make it possible to query events from pageleave events (total conversions for page scroll goals) + # TO DO: make it possible to query events from pageleave events (total conversions for page scroll goals) def event_metric(:events) do wrap_alias([e], %{ events: fragment("toUInt64(round(countIf(? != 'pageleave') * any(_sample_factor)))", e.name) From 1896791fb3a70345e59f1a141898a68d17962289 Mon Sep 17 00:00:00 2001 From: RobertJoonas <56999674+RobertJoonas@users.noreply.github.com> Date: Mon, 3 Feb 2025 15:04:35 +0100 Subject: [PATCH 26/26] Update lib/plausible/stats/sql/expression.ex Co-authored-by: Karl-Aksel Puulmann --- lib/plausible/stats/sql/expression.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/plausible/stats/sql/expression.ex b/lib/plausible/stats/sql/expression.ex index 32b0895c2ed7..f4f9a57be8a5 100644 --- a/lib/plausible/stats/sql/expression.ex +++ b/lib/plausible/stats/sql/expression.ex @@ -196,7 +196,7 @@ defmodule Plausible.Stats.SQL.Expression do }) end - # TO DO: make it possible to query events from pageleave events (total conversions for page scroll goals) + # TODO: make it possible to query events from pageleave events (total conversions for page scroll goals) def event_metric(:events) do wrap_alias([e], %{ events: fragment("toUInt64(round(countIf(? != 'pageleave') * any(_sample_factor)))", e.name)