diff --git a/CHANGELOG.md b/CHANGELOG.md index ff6c5e4be1..91cde9f1ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ All notable changes to this project will be documented in this file. - All dropmenus on dashboard are navigable with Tab (used to be a mix between tab and arrow keys), and no two dropmenus can be open at once on the dashboard +- Special path-based events like "404" don't need `event.props.path` to be explicitly defined when tracking: it is set to be the same as `event.pathname` in event ingestion. If it is explicitly defined, it is not overridden for backwards compatibility. + ### Fixed - Make clicking Compare / Disable Comparison in period picker menu close the menu diff --git a/lib/plausible/exports.ex b/lib/plausible/exports.ex index 0cd6eebf5f..f5d7439baa 100644 --- a/lib/plausible/exports.ex +++ b/lib/plausible/exports.ex @@ -564,7 +564,7 @@ defmodule Plausible.Exports do fragment( "if(? in ?, ?, '')", e.name, - ^Plausible.Imported.goals_with_url(), + ^Plausible.Goals.SystemGoals.goals_with_url(), get_by_key(e, :meta, "url") ), :link_url @@ -573,7 +573,7 @@ defmodule Plausible.Exports do fragment( "if(? in ?, ?, '')", e.name, - ^Plausible.Imported.goals_with_path(), + ^Plausible.Goals.SystemGoals.goals_with_path(), get_by_key(e, :meta, "path") ), :path diff --git a/lib/plausible/goals/system_goals.ex b/lib/plausible/goals/system_goals.ex new file mode 100644 index 0000000000..0c923ac590 --- /dev/null +++ b/lib/plausible/goals/system_goals.ex @@ -0,0 +1,48 @@ +defmodule Plausible.Goals.SystemGoals do + @moduledoc """ + This module contains logic for special goals + """ + + # Special system goals which can be filtered by 'url' custom property + @goals_with_url ["Outbound Link: Click", "Cloaked Link: Click", "File Download"] + + # Special system goals which can be filtered by 'path' custom property + @goals_with_path ["404", "WP Form Completions", "Form: Submission"] + + @spec goals_with_url() :: [String.t()] + def goals_with_url() do + @goals_with_url + end + + @spec goals_with_path() :: [String.t()] + def goals_with_path() do + @goals_with_path + end + + @spec special_goals_for(String.t()) :: [String.t()] + def special_goals_for("event:props:url"), do: goals_with_url() + def special_goals_for("event:props:path"), do: goals_with_path() + + @doc """ + Checks if the event name is for a special goal that should have the event.props.path synced with the event.pathname property. + + ### Examples + iex> sync_props_path_with_pathname?("404", [{"path", "/foo"}]) + false + + Note: Should not override event.props.path if it is set deliberately to nil + iex> sync_props_path_with_pathname?("404", [{"path", nil}]) + false + + iex> sync_props_path_with_pathname?("404", [{"other", "value"}]) + true + + iex> sync_props_path_with_pathname?("404", []) + true + """ + @spec sync_props_path_with_pathname?(String.t(), [{String.t(), String.t()}]) :: boolean() + def sync_props_path_with_pathname?(event_name, props_in_request) do + event_name in goals_with_path() and + not Enum.any?(props_in_request, fn {k, _} -> k == "path" end) + end +end diff --git a/lib/plausible/imported.ex b/lib/plausible/imported.ex index 7a67aed616..0c6f0eab77 100644 --- a/lib/plausible/imported.ex +++ b/lib/plausible/imported.ex @@ -32,11 +32,6 @@ defmodule Plausible.Imported do # Maximum number of complete imports to account for when querying stats @max_complete_imports 5 - # Goals which can be filtered by url property - @goals_with_url ["Outbound Link: Click", "Cloaked Link: Click", "File Download"] - # Goals which can be filtered by path property - @goals_with_path ["404", "WP Form Completions"] - @spec schemas() :: [module()] def schemas, do: @tables @@ -56,16 +51,6 @@ defmodule Plausible.Imported do Enum.map(~w(url path), &("event:props:" <> &1)) end - @spec goals_with_url() :: [String.t()] - def goals_with_url() do - @goals_with_url - end - - @spec goals_with_path() :: [String.t()] - def goals_with_path() do - @goals_with_path - end - @spec any_completed_imports?(Site.t()) :: boolean() def any_completed_imports?(site) do get_completed_imports(site) != [] diff --git a/lib/plausible/ingestion/request.ex b/lib/plausible/ingestion/request.ex index 95831febe0..a8cb3e86f3 100644 --- a/lib/plausible/ingestion/request.ex +++ b/lib/plausible/ingestion/request.ex @@ -91,9 +91,9 @@ defmodule Plausible.Ingestion.Request do |> put_user_agent(conn) |> put_request_params(request_body) |> put_referrer(request_body) + |> put_pathname() |> put_props(request_body) |> put_engagement_fields(request_body) - |> put_pathname() |> put_query_params() |> put_revenue_source(request_body) |> put_interactive(request_body) @@ -171,6 +171,18 @@ defmodule Plausible.Ingestion.Request do Changeset.put_change(changeset, :pathname, pathname) end + defp maybe_set_props_path_to_pathname(props_in_request, changeset) do + if Plausible.Goals.SystemGoals.sync_props_path_with_pathname?( + Changeset.get_field(changeset, :event_name), + props_in_request + ) do + # "path" props is added to the head of the props enum to avoid it being cut off + [{"path", Changeset.get_field(changeset, :pathname)}] ++ props_in_request + else + props_in_request + end + end + defp map_domains(changeset, %{} = request_body) do raw = request_body["d"] || request_body["domain"] raw = if is_binary(raw), do: String.trim(raw) @@ -227,6 +239,7 @@ defmodule Plausible.Ingestion.Request do (request_body["m"] || request_body["meta"] || request_body["p"] || request_body["props"]) |> Plausible.Helpers.JSON.decode_or_fallback() |> Enum.reduce([], &filter_bad_props/2) + |> maybe_set_props_path_to_pathname(changeset) |> Enum.take(@max_props) |> Map.new() diff --git a/lib/plausible/stats/imported/base.ex b/lib/plausible/stats/imported/base.ex index e0720acb70..1a354c56d3 100644 --- a/lib/plausible/stats/imported/base.ex +++ b/lib/plausible/stats/imported/base.ex @@ -130,7 +130,7 @@ defmodule Plausible.Stats.Imported.Base do [:is, "event:goal", names | _rest] -> names _ -> [] end) - |> Enum.any?(&(&1 in special_goals_for(property))) + |> Enum.any?(&(&1 in Plausible.Goals.SystemGoals.special_goals_for(property))) has_unsupported_filters? = query.filters @@ -201,7 +201,4 @@ defmodule Plausible.Stats.Imported.Base do _ -> [] end end - - def special_goals_for("event:props:url"), do: Imported.goals_with_url() - def special_goals_for("event:props:path"), do: Imported.goals_with_path() end diff --git a/lib/plausible/stats/imported/imported.ex b/lib/plausible/stats/imported/imported.ex index 9c76c32aa6..29f9d96da3 100644 --- a/lib/plausible/stats/imported/imported.ex +++ b/lib/plausible/stats/imported/imported.ex @@ -11,23 +11,15 @@ defmodule Plausible.Stats.Imported do @property_to_table_mappings Imported.Base.property_to_table_mappings() - @goals_with_url Plausible.Imported.goals_with_url() - - def goals_with_url(), do: @goals_with_url - - @goals_with_path Plausible.Imported.goals_with_path() - - def goals_with_path(), do: @goals_with_path - @doc """ Returns a boolean indicating whether the combination of filters and breakdown property is possible to query from the imported tables. Usually, when no filters are used, the imported schema supports the query. There is one exception though - breakdown by a custom property. - We are currently importing only two custom properties - `url` and `path. + We are currently importing only two custom properties - `url` and `path`. Both these properties can only be used with their special goal filter - (see `@goals_with_url` and `@goals_with_path`). + (see Plausible.Goals.SystemGoals). """ def schema_supports_query?(query) do length(Imported.Base.decide_tables(query)) > 0 diff --git a/test/plausible/goals_test.exs b/test/plausible/goals_test.exs index 256640e5fa..3458e4e2a4 100644 --- a/test/plausible/goals_test.exs +++ b/test/plausible/goals_test.exs @@ -4,6 +4,8 @@ defmodule Plausible.GoalsTest do use Plausible.Teams.Test alias Plausible.Goals + doctest Plausible.Goals.SystemGoals, import: true + test "create/2 creates goals and trims input" do site = new_site() {:ok, goal} = Goals.create(site, %{"page_path" => "/foo bar "}) diff --git a/test/plausible/ingestion/request_test.exs b/test/plausible/ingestion/request_test.exs index 17105a1986..f2c90e036e 100644 --- a/test/plausible/ingestion/request_test.exs +++ b/test/plausible/ingestion/request_test.exs @@ -292,6 +292,55 @@ defmodule Plausible.Ingestion.RequestTest do assert request.pathname == "/pictures/index.html#foo" end + for event_name <- Plausible.Goals.SystemGoals.goals_with_path() do + test "event.props.path is synced from event.pathname for special path-based event '#{event_name}'" do + payload = %{ + name: unquote(event_name), + domain: "dummy.site", + url: "http://dummy.site/pictures/index.html#foo" + } + + conn = build_conn(:post, "/api/events", payload) + + assert {:ok, request} = Request.build(conn) + + assert request.pathname == "/pictures/index.html" + assert request.props == %{"path" => "/pictures/index.html"} + end + + test "event.props.path is synced from event.pathname for special path-based event '#{event_name}' with hashMode" do + payload = %{ + name: unquote(event_name), + domain: "dummy.site", + url: "http://dummy.site/pictures/index.html#foo", + hashMode: 1 + } + + conn = build_conn(:post, "/api/events", payload) + + assert {:ok, request} = Request.build(conn) + + assert request.pathname == "/pictures/index.html#foo" + assert request.props == %{"path" => "/pictures/index.html#foo"} + end + + test "event.props.path is not synced from event.pathname for special path-based event '#{event_name}' if it's set explicitly (legacy support)" do + payload = %{ + name: unquote(event_name), + domain: "dummy.site", + url: "http://dummy.site/pictures/index.html#foo", + props: %{"path" => "/album"} + } + + conn = build_conn(:post, "/api/events", payload) + + assert {:ok, request} = Request.build(conn) + + assert request.pathname == "/pictures/index.html" + assert request.props == %{"path" => "/album"} + end + end + test "query params are set" do payload = %{ name: "pageview", diff --git a/test/plausible_web/controllers/api/external_stats_controller/breakdown_test.exs b/test/plausible_web/controllers/api/external_stats_controller/breakdown_test.exs index bcd5264487..7251682ebc 100644 --- a/test/plausible_web/controllers/api/external_stats_controller/breakdown_test.exs +++ b/test/plausible_web/controllers/api/external_stats_controller/breakdown_test.exs @@ -3372,7 +3372,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.BreakdownTest do assert %{"source" => "Google", "events" => 1} = breakdown_and_first.("visit:source") end - for goal_name <- Plausible.Imported.goals_with_url() do + for goal_name <- Plausible.Goals.SystemGoals.goals_with_url() do test "returns url breakdown for #{goal_name} goal", %{conn: conn, site: site} do insert(:goal, event_name: unquote(goal_name), site: site) site_import = insert(:site_import, site: site) @@ -3432,7 +3432,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.BreakdownTest do end end - for goal_name <- Plausible.Imported.goals_with_path() do + for goal_name <- Plausible.Goals.SystemGoals.goals_with_path() do test "returns path breakdown for #{goal_name} goal", %{conn: conn, site: site} do insert(:goal, event_name: unquote(goal_name), site: site) site_import = insert(:site_import, 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 4d06e5abb3..ca48404cb0 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 @@ -681,7 +681,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do breakdown_and_first.("visit:source") end - for goal_name <- Plausible.Imported.goals_with_url() do + for goal_name <- Plausible.Goals.SystemGoals.goals_with_url() do test "returns url breakdown for #{goal_name} goal", %{conn: conn, site: site} do insert(:goal, event_name: unquote(goal_name), site: site) site_import = insert(:site_import, site: site) @@ -734,7 +734,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do end end - for goal_name <- Plausible.Imported.goals_with_path() do + for goal_name <- Plausible.Goals.SystemGoals.goals_with_path() do test "returns path breakdown for #{goal_name} goal", %{conn: conn, site: site} do insert(:goal, event_name: unquote(goal_name), site: site) site_import = insert(:site_import, site: site) diff --git a/test/plausible_web/controllers/api/stats_controller/custom_prop_breakdown_test.exs b/test/plausible_web/controllers/api/stats_controller/custom_prop_breakdown_test.exs index 16753a1b0f..f4acbd59ca 100644 --- a/test/plausible_web/controllers/api/stats_controller/custom_prop_breakdown_test.exs +++ b/test/plausible_web/controllers/api/stats_controller/custom_prop_breakdown_test.exs @@ -1261,7 +1261,7 @@ defmodule PlausibleWeb.Api.StatsController.CustomPropBreakdownTest do ] end - for goal_name <- Plausible.Imported.goals_with_path() do + for goal_name <- Plausible.Goals.SystemGoals.goals_with_path() do test "returns path breakdown for #{goal_name} goal", %{conn: conn, site: site} do insert(:goal, event_name: unquote(goal_name), site: site) site_import = insert(:site_import, site: site) @@ -1320,7 +1320,7 @@ defmodule PlausibleWeb.Api.StatsController.CustomPropBreakdownTest do end end - for goal_name <- Plausible.Imported.goals_with_url() do + for goal_name <- Plausible.Goals.SystemGoals.goals_with_url() do test "returns url breakdown for #{goal_name} goal", %{conn: conn, site: site} do insert(:goal, event_name: unquote(goal_name), site: site) site_import = insert(:site_import, site: site) diff --git a/tracker/npm_package/CHANGELOG.md b/tracker/npm_package/CHANGELOG.md index e9e2ead949..9924cab397 100644 --- a/tracker/npm_package/CHANGELOG.md +++ b/tracker/npm_package/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +- "Form: Submission" event payload does not need to contain props.path any more: it is saved to be the same as the pathname of the event + ## [0.3.1] - 2025-07-08 - Do not send "Form: Submission" event if the form is tagged diff --git a/tracker/package.json b/tracker/package.json index da87a7dc95..dd222e3e7c 100644 --- a/tracker/package.json +++ b/tracker/package.json @@ -1,5 +1,5 @@ { - "tracker_script_version": 22, + "tracker_script_version": 23, "type": "module", "scripts": { "deploy": "node compile.js", diff --git a/tracker/src/custom-events.js b/tracker/src/custom-events.js index 552ee7baa9..efee6f8b24 100644 --- a/tracker/src/custom-events.js +++ b/tracker/src/custom-events.js @@ -191,7 +191,7 @@ export function init() { // If the form is tagged, we don't track it as a generic form submission. return } - track('Form: Submission', { props: { path: location.pathname } }); + track('Form: Submission'); } } diff --git a/tracker/test/form-submissions.spec.ts b/tracker/test/form-submissions.spec.ts index 6aa5b5d16c..4dd26ae58d 100644 --- a/tracker/test/form-submissions.spec.ts +++ b/tracker/test/form-submissions.spec.ts @@ -1,6 +1,7 @@ import { test } from '@playwright/test' import { LOCAL_SERVER_ADDR } from './support/server' import { + e, ensurePlausibleInitialized, expectPlausibleInAction, isEngagementEvent, @@ -45,9 +46,7 @@ test('does not track form submissions when the feature is disabled', async ({ }) test.describe('form submissions feature is enabled', () => { - test('tracks forms that use GET method', async ({ page }, { - testId - }) => { + test('tracks forms that use GET method', async ({ page }, { testId }) => { const { url } = await initializePageDynamically(page, { testId, scriptConfig: { ...DEFAULT_CONFIG, formSubmissions: true }, @@ -69,7 +68,8 @@ test.describe('form submissions feature is enabled', () => { expectedRequests: [ { n: 'Form: Submission', - p: { path: url } + u: `${LOCAL_SERVER_ADDR}${url}`, + p: e.toBeUndefined() } ] }) @@ -98,7 +98,8 @@ test.describe('form submissions feature is enabled', () => { expectedRequests: [ { n: 'Form: Submission', - p: { path: url } + u: `${LOCAL_SERVER_ADDR}${url}`, + p: e.toBeUndefined() } ] }) @@ -137,7 +138,8 @@ test.describe('form submissions feature is enabled', () => { expectedRequests: [ { n: 'Form: Submission', - p: { path: url } + u: `${LOCAL_SERVER_ADDR}${url}`, + p: e.toBeUndefined() } ] }) @@ -169,7 +171,8 @@ test.describe('form submissions feature is enabled', () => { expectedRequests: [ { n: 'Form: Submission', - p: { path: url } + u: `${LOCAL_SERVER_ADDR}${url}`, + p: e.toBeUndefined() } ] }) @@ -265,7 +268,8 @@ test.describe('form submissions feature is enabled', () => { expectedRequests: [ { n: 'Form: Submission', - p: { path: url } + u: `${LOCAL_SERVER_ADDR}${url}`, + p: e.toBeUndefined() } ] }) @@ -279,7 +283,8 @@ test.describe('form submissions feature is enabled', () => { expectedRequests: [ { n: 'Form: Submission', - p: { path: url } + u: `${LOCAL_SERVER_ADDR}${url}`, + p: e.toBeUndefined() } ] })