plausible/analytics
1
1
Fork 0
mirror of https://github.com/plausible/analytics.git synced 2025-01-03 15:17:58 +03:00
Code Issues Packages Projects Releases Wiki Activity

Associate goals with sites, not domains (#2828)

Browse Source 
* Revert "Rephrase error message"

This reverts commit f624443a96.

* Revert "Temporarily disable goal creation"

This reverts commit a091635b9d.

* Update ecto schema

* Make sure goal operations are per site

* Update tests

* Split postgres migrations
This commit is contained in:
hq1 2023-04-10 10:51:36 +02:00 committed by GitHub
parent 3cb089eab4
commit c04e9286dd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 127 additions and 92 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
lib/plausible/goal/schema.ex
View File

@ -6,9 +6,12 @@ defimpl Jason.Encoder, for: Plausible.Goal do
value.page_path -> :page
end
domain = value.site.domain
value
|> Map.put(:goal_type, goal_type)
|> Map.take([:id, :domain, :goal_type, :event_name, :page_path])
|> Map.take([:id, :goal_type, :event_name, :page_path])
|> Map.put(:domain, domain)
|> Jason.Encode.map(opts)
end
end
@ -18,17 +21,19 @@ defmodule Plausible.Goal do
import Ecto.Changeset
schema "goals" do
field :domain, :string
field :event_name, :string
field :page_path, :string
belongs_to :site, Plausible.Site
timestamps()
end
def changeset(goal, attrs \\ %{}) do
goal
|> cast(attrs, [:domain, :event_name, :page_path])
|> validate_required([:domain])
|> cast(attrs, [:site_id, :event_name, :page_path])
|> validate_required([:site_id])
|> cast_assoc(:site)
|> validate_event_name_and_page_path()
|> update_change(:event_name, &String.trim/1)
|> update_change(:page_path, &String.trim/1)

44
lib/plausible/goals.ex
View File

@ -3,22 +3,23 @@ defmodule Plausible.Goals do
alias Plausible.Goal
def create(site, params) do
params = Map.merge(params, %{"domain" => site.domain})
params = Map.merge(params, %{"site_id" => site.id})
Goal.changeset(%Goal{}, params)
|> Ecto.Changeset.add_error(
:event_name,
"Sorry! Due to the ongoing maintenance, adding new goals is currently paused. Please try again in one hour. Thanks for your patience!"
)
|> Ecto.Changeset.add_error(
:page_path,
"Sorry! Due to the ongoing maintenance, adding new goals is currently paused. Please try again in one hour. Thanks for your patience!"
)
|> Repo.insert()
case Repo.insert(Goal.changeset(%Goal{}, params)) do
{:ok, goal} -> {:ok, Repo.preload(goal, :site)}
error -> error
end
end
def find_or_create(site, %{"goal_type" => "event", "event_name" => event_name}) do
goal = Repo.get_by(Plausible.Goal, domain: site.domain, event_name: event_name)
query =
from g in Goal,
inner_join: assoc(g, :site),
where: g.site_id == ^site.id,
where: g.event_name == ^event_name,
preload: [:site]
goal = Repo.one(query)
case goal do
nil -> create(site, %{"event_name" => event_name})
@ -29,7 +30,14 @@ defmodule Plausible.Goals do
def find_or_create(_, %{"goal_type" => "event"}), do: {:missing, "event_name"}
def find_or_create(site, %{"goal_type" => "page", "page_path" => page_path}) do
goal = Repo.get_by(Plausible.Goal, domain: site.domain, page_path: page_path)
query =
from g in Goal,
inner_join: assoc(g, :site),
where: g.site_id == ^site.id,
where: g.page_path == ^page_path,
preload: [:site]
goal = Repo.one(query)
case goal do
nil -> create(site, %{"page_path" => page_path})
@ -39,21 +47,23 @@ defmodule Plausible.Goals do
def find_or_create(_, %{"goal_type" => "page"}), do: {:missing, "page_path"}
def for_domain(domain) do
def for_site(site) do
query =
from g in Goal,
where: g.domain == ^domain
inner_join: assoc(g, :site),
where: g.site_id == ^site.id,
preload: [:site]
query
|> Repo.all()
|> Enum.map(&maybe_trim/1)
end
def delete(id, domain) do
def delete(id, site) do
case Repo.delete_all(
from g in Goal,
where: g.id == ^id,
where: g.domain == ^domain
where: g.site_id == ^site.id
) do
{1, _} -> :ok
{0, _} -> {:error, :not_found}

2
lib/plausible/sites.ex
View File

@ -115,7 +115,7 @@ defmodule Plausible.Sites do
def has_goals?(site) do
Repo.exists?(
from g in Plausible.Goal,
where: g.domain == ^site.domain
where: g.site_id == ^site.id
)
end

4
lib/plausible/stats/breakdown.ex
View File

@ -13,8 +13,8 @@ defmodule Plausible.Stats.Breakdown do
def breakdown(site, query, "event:goal" = property, metrics, pagination) do
{event_goals, pageview_goals} =
site.domain
|> Goals.for_domain()
site
|> Goals.for_site()
|> Enum.split_with(fn goal -> goal.event_name end)
events = Enum.map(event_goals, & &1.event_name)

4
lib/plausible/stats/filter_suggestions.ex
View File

@ -118,8 +118,8 @@ defmodule Plausible.Stats.FilterSuggestions do
end
def filter_suggestions(site, _query, "goal", filter_search) do
site.domain
|> Plausible.Goals.for_domain()
site
|> Plausible.Goals.for_site()
|> Enum.map(fn x -> if x.event_name, do: x.event_name, else: "Visit #{x.page_path}" end)
|> Enum.filter(fn goal ->
String.contains?(

7
lib/plausible_web/controllers/api/external_sites_controller.ex
View File

@ -124,11 +124,6 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
{:missing, param} ->
H.bad_request(conn, "Parameter `#{param}` is required to create a goal")
{:error, changeset} ->
conn
|> put_status(400)
|> json(serialize_errors(changeset))
e ->
H.bad_request(conn, "Something went wrong: #{inspect(e)}")
end
@ -139,7 +134,7 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
{:ok, goal_id} <- expect_param_key(params, "goal_id"),
site when not is_nil(site) <-
Sites.get_for_user(conn.assigns[:current_user].id, site_id, [:owner, :admin]) do
case Goals.delete(goal_id, site.domain) do
case Goals.delete(goal_id, site) do
:ok ->
json(conn, %{"deleted" => true})

4
lib/plausible_web/controllers/site_controller.ex
View File

@ -158,7 +158,7 @@ defmodule PlausibleWeb.SiteController do
end
def delete_goal(conn, %{"id" => goal_id}) do
case Plausible.Goals.delete(goal_id, conn.assigns[:site].domain) do
case Plausible.Goals.delete(goal_id, conn.assigns[:site]) do
:ok ->
conn
|> put_flash(:success, "Goal deleted successfully")
@ -225,7 +225,7 @@ defmodule PlausibleWeb.SiteController do
def settings_goals(conn, _params) do
site = conn.assigns[:site] |> Repo.preload(:custom_domain)
goals = Goals.for_domain(site.domain)
goals = Goals.for_site(site)
conn
|> assign(:skip_plausible_tracking, true)

10
priv/repo/migrations/20230410070312_fixup_goals_sites_assoc.exs Normal file
View File

@ -0,0 +1,10 @@
defmodule Plausible.Repo.Migrations.FixupGoalsSitesAssoc do
use Ecto.Migration
def change do
alter table(:goals) do
modify :site_id, references(:sites, on_delete: :delete_all), null: false
remove :domain
end
end
end

30
test/plausible/goals_test.exs
View File

@ -3,7 +3,6 @@ defmodule Plausible.GoalsTest do
alias Plausible.Goals
@tag :skip
test "create/2 trims input" do
site = insert(:site)
{:ok, goal} = Goals.create(site, %{"page_path" => "/foo bar "})
@ -13,13 +12,34 @@ defmodule Plausible.GoalsTest do
assert goal.event_name == "some event name"
end
test "for_domain/2 returns trimmed input even if it was saved with trailing whitespace" do
test "for_site2 returns trimmed input even if it was saved with trailing whitespace" do
site = insert(:site)
insert(:goal, %{domain: site.domain, event_name: " Signup "})
insert(:goal, %{domain: site.domain, page_path: " /Signup "})
insert(:goal, %{site: site, event_name: " Signup "})
insert(:goal, %{site: site, page_path: " /Signup "})
goals = Goals.for_domain(site.domain)
goals = Goals.for_site(site)
assert [%{event_name: "Signup"}, %{page_path: "/Signup"}] = goals
end
@tag :v2_only
test "goals are present after domain change" do
site = insert(:site)
insert(:goal, %{site: site, event_name: " Signup "})
insert(:goal, %{site: site, page_path: " /Signup "})
{:ok, site} = Plausible.Site.Domain.change(site, "goals.example.com")
assert [_, _] = Goals.for_site(site)
end
test "goals are removed when site is deleted" do
site = insert(:site)
insert(:goal, %{site: site, event_name: " Signup "})
insert(:goal, %{site: site, page_path: " /Signup "})
Plausible.Site.Removal.run(site.domain)
assert [] = Goals.for_site(site)
end
end

9
test/plausible_web/controllers/api/external_sites_controller_test.exs
View File

@ -245,7 +245,6 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
describe "PUT /api/v1/sites/goals" do
setup :create_site
@tag :skip
test "can add a goal as event to a site", %{conn: conn, site: site} do
conn =
put(conn, "/api/v1/sites/goals", %{
@ -257,9 +256,9 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
res = json_response(conn, 200)
assert res["goal_type"] == "event"
assert res["event_name"] == "Signup"
assert res["domain"] == site.domain
end
@tag :skip
test "can add a goal as page to a site", %{conn: conn, site: site} do
conn =
put(conn, "/api/v1/sites/goals", %{
@ -271,10 +270,10 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
res = json_response(conn, 200)
assert res["goal_type"] == "page"
assert res["page_path"] == "/signup"
assert res["domain"] == site.domain
end
@tag :v2_only
@tag :skip
test "can add a goal using old site_id after domain change", %{conn: conn, site: site} do
old_domain = site.domain
new_domain = "new.example.com"
@ -291,9 +290,9 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
res = json_response(conn, 200)
assert res["goal_type"] == "event"
assert res["event_name"] == "Signup"
assert res["domain"] == new_domain
end
@tag :skip
test "is idempotent find or create op", %{conn: conn, site: site} do
conn =
put(conn, "/api/v1/sites/goals", %{
@ -394,7 +393,6 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
end
end
@tag :skip
describe "DELETE /api/v1/sites/goals/:goal_id" do
setup :create_new_site
@ -417,7 +415,6 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
end
@tag :v2_only
@tag :skip
test "delete a goal using old site_id after domain change", %{conn: conn, site: site} do
old_domain = site.domain
new_domain = "new.example.com"

6
test/plausible_web/controllers/api/external_stats_controller/breakdown_test.exs
View File

@ -1061,9 +1061,9 @@ defmodule PlausibleWeb.Api.ExternalStatsController.BreakdownTest do
describe "breakdown by event:goal" do
test "custom properties from custom events are returned", %{conn: conn, site: site} do
insert(:goal, %{domain: site.domain, event_name: "404"})
insert(:goal, %{domain: site.domain, event_name: "Purchase"})
insert(:goal, %{domain: site.domain, page_path: "/test"})
insert(:goal, %{site: site, event_name: "404"})
insert(:goal, %{site: site, event_name: "Purchase"})
insert(:goal, %{site: site, page_path: "/test"})
populate_stats(site, [
build(:pageview,

74
test/plausible_web/controllers/api/stats_controller/conversions_test.exs
View File

@ -27,8 +27,8 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
)
])
insert(:goal, %{domain: site.domain, page_path: "/register"})
insert(:goal, %{domain: site.domain, event_name: "Signup"})
insert(:goal, %{site: site, page_path: "/register"})
insert(:goal, %{site: site, event_name: "Signup"})
conn = get(conn, "/api/stats/#{site.domain}/conversions?period=day")
@ -76,7 +76,7 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
)
])
insert(:goal, %{domain: site.domain, event_name: "Payment"})
insert(:goal, %{site: site, event_name: "Payment"})
filters = Jason.encode!(%{props: %{"logged_in" => "true"}})
conn = get(conn, "/api/stats/#{site.domain}/conversions?period=day&filters=#{filters}")
@ -117,7 +117,7 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
build(:event, name: "Payment")
])
insert(:goal, %{domain: site.domain, event_name: "Payment"})
insert(:goal, %{site: site, event_name: "Payment"})
filters = Jason.encode!(%{props: %{"logged_in" => "!true"}})
conn = get(conn, "/api/stats/#{site.domain}/conversions?period=day&filters=#{filters}")
@ -156,7 +156,7 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
build(:event, name: "Payment")
])
insert(:goal, %{domain: site.domain, event_name: "Payment"})
insert(:goal, %{site: site, event_name: "Payment"})
filters = Jason.encode!(%{props: %{"logged_in" => "(none)"}})
conn = get(conn, "/api/stats/#{site.domain}/conversions?period=day&filters=#{filters}")
@ -197,7 +197,7 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
build(:event, name: "Payment")
])
insert(:goal, %{domain: site.domain, event_name: "Payment"})
insert(:goal, %{site: site, event_name: "Payment"})
filters = Jason.encode!(%{props: %{"logged_in" => "!(none)"}})
conn = get(conn, "/api/stats/#{site.domain}/conversions?period=day&filters=#{filters}")
@ -227,8 +227,8 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
build(:event, name: "Signup", "meta.key": ["variant"], "meta.value": ["B"])
])
insert(:goal, %{domain: site.domain, page_path: "/register"})
insert(:goal, %{domain: site.domain, event_name: "Signup"})
insert(:goal, %{site: site, page_path: "/register"})
insert(:goal, %{site: site, event_name: "Signup"})
filters = Jason.encode!(%{goal: "Signup"})
@ -259,8 +259,8 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
build(:event, name: "Signup")
])
insert(:goal, %{domain: site.domain, page_path: "/register"})
insert(:goal, %{domain: site.domain, event_name: "Signup"})
insert(:goal, %{site: site, page_path: "/register"})
insert(:goal, %{site: site, event_name: "Signup"})
filters = Jason.encode!(%{goal: "Signup|Visit /register"})
@ -298,10 +298,10 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
build(:event, name: "Signup")
])
insert(:goal, %{domain: site.domain, page_path: "/register"})
insert(:goal, %{domain: site.domain, page_path: "/another"})
insert(:goal, %{domain: site.domain, event_name: "CTA"})
insert(:goal, %{domain: site.domain, event_name: "Signup"})
insert(:goal, %{site: site, page_path: "/register"})
insert(:goal, %{site: site, page_path: "/another"})
insert(:goal, %{site: site, event_name: "CTA"})
insert(:goal, %{site: site, event_name: "Signup"})
filters = Jason.encode!(%{goal: "!Signup|Visit /another"})
@ -339,9 +339,9 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
build(:event, name: "Signup")
])
insert(:goal, %{domain: site.domain, page_path: "/blog**"})
insert(:goal, %{domain: site.domain, event_name: "CTA"})
insert(:goal, %{domain: site.domain, event_name: "Signup"})
insert(:goal, %{site: site, page_path: "/blog**"})
insert(:goal, %{site: site, event_name: "CTA"})
insert(:goal, %{site: site, event_name: "Signup"})
filters = Jason.encode!(%{goal: "Signup|Visit /blog**"})
@ -379,10 +379,10 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
build(:event, name: "Signup")
])
insert(:goal, %{domain: site.domain, page_path: "/blog**"})
insert(:goal, %{domain: site.domain, page_path: "/ano**"})
insert(:goal, %{domain: site.domain, event_name: "CTA"})
insert(:goal, %{domain: site.domain, event_name: "Signup"})
insert(:goal, %{site: site, page_path: "/blog**"})
insert(:goal, %{site: site, page_path: "/ano**"})
insert(:goal, %{site: site, event_name: "CTA"})
insert(:goal, %{site: site, event_name: "Signup"})
filters = Jason.encode!(%{goal: "!Signup|Visit /blog**"})
@ -422,7 +422,7 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
build(:event, name: "Signup", user_id: 2)
])
insert(:goal, %{domain: site.domain, event_name: "Signup"})
insert(:goal, %{site: site, event_name: "Signup"})
filters = Jason.encode!(%{goal: "Signup", props: %{variant: "(none)"}})
@ -457,7 +457,7 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
build(:event, name: "Signup", "meta.key": ["variant"], "meta.value": ["B"])
])
insert(:goal, %{domain: site.domain, event_name: "Signup"})
insert(:goal, %{site: site, event_name: "Signup"})
filters = Jason.encode!(%{goal: "Signup"})
prop_key = "variant"
@ -493,7 +493,7 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
build(:event, name: "Signup", "meta.key": ["variant"], "meta.value": ["A"])
])
insert(:goal, %{domain: site.domain, event_name: "Signup"})
insert(:goal, %{site: site, event_name: "Signup"})
filters = Jason.encode!(%{goal: "Signup"})
prop_key = "variant"
@ -527,7 +527,7 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
build(:event, user_id: 2, name: "Signup", "meta.key": ["variant"], "meta.value": ["B"])
])
insert(:goal, %{domain: site.domain, event_name: "Signup"})
insert(:goal, %{site: site, event_name: "Signup"})
filters = Jason.encode!(%{goal: "Signup", props: %{"variant" => "B"}})
prop_key = "variant"
@ -565,7 +565,7 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
)
])
insert(:goal, %{domain: site.domain, event_name: "ButtonClick"})
insert(:goal, %{site: site, event_name: "ButtonClick"})
filters =
Jason.encode!(%{
@ -611,7 +611,7 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
)
])
insert(:goal, %{domain: site.domain, event_name: "ButtonClick"})
insert(:goal, %{site: site, event_name: "ButtonClick"})
filters =
Jason.encode!(%{
@ -642,16 +642,16 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
setup [:create_user, :log_in, :create_site]
test "returns correct and sorted glob goal counts", %{conn: conn, site: site} do
insert(:goal, %{domain: site.domain, page_path: "/register"})
insert(:goal, %{domain: site.domain, page_path: "/reg*"})
insert(:goal, %{domain: site.domain, page_path: "/*/register"})
insert(:goal, %{domain: site.domain, page_path: "/billing**/success"})
insert(:goal, %{domain: site.domain, page_path: "/billing*/success"})
insert(:goal, %{domain: site.domain, page_path: "/signup"})
insert(:goal, %{domain: site.domain, page_path: "/signup/*"})
insert(:goal, %{domain: site.domain, page_path: "/signup/**"})
insert(:goal, %{domain: site.domain, page_path: "/*"})
insert(:goal, %{domain: site.domain, page_path: "/**"})
insert(:goal, %{site: site, page_path: "/register"})
insert(:goal, %{site: site, page_path: "/reg*"})
insert(:goal, %{site: site, page_path: "/*/register"})
insert(:goal, %{site: site, page_path: "/billing**/success"})
insert(:goal, %{site: site, page_path: "/billing*/success"})
insert(:goal, %{site: site, page_path: "/signup"})
insert(:goal, %{site: site, page_path: "/signup/*"})
insert(:goal, %{site: site, page_path: "/signup/**"})
insert(:goal, %{site: site, page_path: "/*"})
insert(:goal, %{site: site, page_path: "/**"})
populate_stats(site, [
build(:pageview,

10
test/plausible_web/controllers/site_controller_test.exs
View File

@ -334,8 +334,8 @@ defmodule PlausibleWeb.SiteControllerTest do
setup [:create_user, :log_in, :create_site]
test "lists goals for the site", %{conn: conn, site: site} do
insert(:goal, domain: site.domain, event_name: "Custom event")
insert(:goal, domain: site.domain, page_path: "/register")
insert(:goal, site: site, event_name: "Custom event")
insert(:goal, site: site, page_path: "/register")
conn = get(conn, "/#{site.domain}/settings/goals")
@ -620,7 +620,6 @@ defmodule PlausibleWeb.SiteControllerTest do
end
end
@tag :skip
describe "POST /:website/goals" do
setup [:create_user, :log_in, :create_site]
@ -640,7 +639,6 @@ defmodule PlausibleWeb.SiteControllerTest do
assert redirected_to(conn, 302) == "/#{site.domain}/settings/goals"
end
@tag :skip
test "creates a custom event goal for the website", %{conn: conn, site: site} do
conn =
post(conn, "/#{site.domain}/goals", %{
@ -662,7 +660,7 @@ defmodule PlausibleWeb.SiteControllerTest do
setup [:create_user, :log_in, :create_site]
test "deletes goal", %{conn: conn, site: site} do
goal = insert(:goal, domain: site.domain, event_name: "Custom event")
goal = insert(:goal, site: site, event_name: "Custom event")
conn = delete(conn, "/#{site.domain}/goals/#{goal.id}")
@ -672,7 +670,7 @@ defmodule PlausibleWeb.SiteControllerTest do
test "fails to delete goal for a foreign site", %{conn: conn, site: site} do
another_site = insert(:site)
goal = insert(:goal, domain: another_site.domain, event_name: "Custom event")
goal = insert(:goal, site: another_site, event_name: "Custom event")
conn = delete(conn, "/#{site.domain}/goals/#{goal.id}")

2
test/plausible_web/controllers/stats_controller_test.exs
View File

@ -250,7 +250,7 @@ defmodule PlausibleWeb.StatsControllerTest do
)
])
insert(:goal, %{domain: site.domain, event_name: "Signup"})
insert(:goal, %{site: site, event_name: "Signup"})
end
describe "GET /:website/export - with goal filter" do