Script v2: Sync event.props.path for special path-based events from event.pathname (#5559)
* Sync pathname to event.props.path for special path-based goals * Stop adding event.props.path for 'Form: Submission' events * Update tracker script version * Fix test expectations * Fix format * Simplify maybe_put_props_path with 'with' * Add specs, fix factory * Update tracker changelog * Update EE/CE changelog * Remove business logic from factory * Refactor event.props.path sync result to be validated with the rest of the custom props * Clarify doctests and update function name
This commit is contained in:
parent
c982caab9b
commit
3bdbe83383
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
@ -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) != []
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 "})
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -1,5 +1,5 @@
|
|||
{
|
||||
"tracker_script_version": 22,
|
||||
"tracker_script_version": 23,
|
||||
"type": "module",
|
||||
"scripts": {
|
||||
"deploy": "node compile.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');
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
}
|
||||
]
|
||||
})
|
||||
|
|
|
|||
Loading…
Reference in New Issue