Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds same glob patterns as exclusions for pageview goals #750

Merged
merged 9 commits into from
Mar 2, 2021
Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ All notable changes to this project will be documented in this file.
- Ability to view and filter by entry and exit pages, in addition to regular page hits plausible/analytics#712
- 30 day and 6 month keybindings (`T` and `S`, respectively) plausible/analytics#709
- Site switching keybinds (1-9 for respective sites) plausible/analytics#735
- Glob (wildcard) based pageview goals plausible/analytics#750

### Fixed
- Capitalized date/time selection keybinds not working plausible/analytics#709
Expand Down
17 changes: 11 additions & 6 deletions lib/plausible/goal/schema.ex
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,22 @@ defmodule Plausible.Goal do
end

defp validate_event_name_and_page_path(changeset) do
if present?(changeset, :event_name) || present?(changeset, :page_path) do
if validate_page_path(changeset) || validate_event_name(changeset) do
changeset
else
changeset
|> add_error(:event_name, "this field is required")
|> add_error(:page_path, "this field is required")
|> add_error(:event_name, "this field is required and cannot be blank")
|> add_error(:page_path, "this field is required and must start with a /")
end
end

defp present?(changeset, field) do
value = get_field(changeset, field)
value && value != ""
defp validate_page_path(changeset) do
value = get_field(changeset, :page_path)
value && String.match?(value, ~r/^\/.*/)
Copy link
Contributor Author

@Vigasaurus Vigasaurus Feb 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to do better validation of pathnames here, and add good validation for preventing duplicate goals; but I didn't want to make a disgusting regex if you knew of an easy Elixir helper for validating URLs (this may not work I guess because of the globs), and I couldn't get the latter to work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK as is.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about it to extract this regex and put in the module attribute? And use a better name to describe the intentions of what this regex do...

end

defp validate_event_name(changeset) do
value = get_field(changeset, :event_name)
value && String.match?(value, ~r/^.+/)
end
end
41 changes: 27 additions & 14 deletions lib/plausible/stats/clickhouse.ex
Original file line number Diff line number Diff line change
Expand Up @@ -924,24 +924,37 @@ defmodule Plausible.Stats.Clickhouse do
end

defp fetch_pageview_goals(goals, site, query) do
pages =
goals =
Enum.map(goals, fn goal -> goal.page_path end)
|> Enum.filter(& &1)

if Enum.count(pages) > 0 do
q =
from(
e in base_query_w_sessions(site, query),
where: fragment("? IN tuple(?)", e.pathname, ^pages),
group_by: e.pathname,
select: %{
name: fragment("concat('Visit ', ?) as name", e.pathname),
count: fragment("uniq(user_id) as count"),
total_count: fragment("count(*) as total_count")
}
)
if Enum.count(goals) > 0 do
regex_goals =
Enum.map(goals, fn g ->
"^#{g}\/?$"
|> String.replace(~r/\*\*/, ".*")
|> String.replace(~r/(?<!\.)\*/, "[^/]*")
end)

ClickhouseRepo.all(q)
from(
e in base_query_w_sessions(site, query),
where:
fragment(
"notEmpty(multiMatchAllIndices(?, array(?)) as indices)",
Vigasaurus marked this conversation as resolved.
Show resolved Hide resolved
e.pathname,
^regex_goals
),
select: %{
index: fragment("arrayJoin(indices) as index"),
count: fragment("uniq(user_id) as count"),
total_count: fragment("count(*) as total_count")
},
group_by: fragment("index")
)
|> ClickhouseRepo.all()
|> Enum.map(fn x ->
Map.put(x, :name, "Visit #{Enum.at(goals, x[:index] - 1)}") |> Map.delete(:index)
end)
else
[]
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,53 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
]
end
end

describe "GET /api/stats/:domain/conversions - with glob goals" do
setup [:create_user, :log_in, :create_site]

test "returns correct and sorted glob goal counts", %{conn: conn, site: site} do
Vigasaurus marked this conversation as resolved.
Show resolved Hide resolved
insert(:goal, %{domain: site.domain, page_path: "/register"})
insert(:goal, %{domain: site.domain, page_path: "/reg*"})
insert(:goal, %{domain: site.domain, page_path: "/reg*ster"})
insert(:goal, %{domain: site.domain, page_path: "/*/contact"})
insert(:goal, %{domain: site.domain, page_path: "/*"})

conn =
get(
conn,
"/api/stats/#{site.domain}/conversions?period=12mo&date=2019-01-01"
)

assert json_response(conn, 200) == [
%{
"conversion_rate" => 100.0,
"count" => 8,
"name" => "Visit /*",
"total_count" => 8,
"prop_names" => nil
},
%{
"conversion_rate" => 25.0,
"count" => 2,
"name" => "Visit /reg*ster",
"total_count" => 2,
"prop_names" => nil
},
%{
"conversion_rate" => 25.0,
"count" => 2,
"name" => "Visit /reg*",
"total_count" => 2,
"prop_names" => nil
},
%{
"conversion_rate" => 25.0,
"count" => 2,
"name" => "Visit /register",
"total_count" => 2,
"prop_names" => nil
}
]
end
end
end