diff --git a/lib/plausible/goals.ex b/lib/plausible/goals.ex index 20c6fc78c6..3b4bbf2c7d 100644 --- a/lib/plausible/goals.ex +++ b/lib/plausible/goals.ex @@ -5,12 +5,35 @@ defmodule Plausible.Goals do use Plausible.Funnel - def create(site, params) do + @doc """ + Creates a Goal for a site. + + If the created goal is a revenue goal, it sets site.updated_at to be + refreshed by the sites cache, as revenue goals are used during ingestion. + """ + def create(site, params, now \\ DateTime.utc_now()) do params = Map.merge(params, %{"site_id" => site.id}) - case Repo.insert(Goal.changeset(%Goal{}, params)) do - {:ok, goal} -> {:ok, Repo.preload(goal, :site)} - error -> error + Ecto.Multi.new() + |> Ecto.Multi.insert(:goal, Goal.changeset(%Goal{}, params)) + |> Ecto.Multi.run(:site, fn repo, %{goal: goal} -> + if Goal.revenue?(goal) do + now = + now + |> DateTime.truncate(:second) + |> DateTime.to_naive() + + site + |> Ecto.Changeset.change(updated_at: now) + |> repo.update() + else + {:ok, site} + end + end) + |> Repo.transaction() + |> case do + {:ok, %{goal: goal}} -> {:ok, Repo.preload(goal, :site)} + {:error, _failed_operation, failed_value, _changes_so_far} -> {:error, failed_value} end end diff --git a/lib/plausible/ingestion/event.ex b/lib/plausible/ingestion/event.ex index b603175c8b..a2c5747eb6 100644 --- a/lib/plausible/ingestion/event.ex +++ b/lib/plausible/ingestion/event.ex @@ -10,7 +10,7 @@ defmodule Plausible.Ingestion.Event do alias Plausible.Site.GateKeeper defstruct domain: nil, - site_id: nil, + site: nil, clickhouse_event_attrs: %{}, clickhouse_event: nil, dropped?: false, @@ -27,7 +27,7 @@ defmodule Plausible.Ingestion.Event do @type t() :: %__MODULE__{ domain: String.t() | nil, - site_id: pos_integer() | nil, + site: %Plausible.Site{} | nil, clickhouse_event_attrs: map(), clickhouse_event: %ClickhouseEventV2{} | nil, dropped?: boolean(), @@ -45,10 +45,10 @@ defmodule Plausible.Ingestion.Event do else Enum.reduce(domains, [], fn domain, acc -> case GateKeeper.check(domain) do - {:allow, site_id} -> + {:allow, site} -> processed = domain - |> new(site_id, request) + |> new(site, request) |> process_unless_dropped(pipeline()) [processed | acc] @@ -120,8 +120,8 @@ defmodule Plausible.Ingestion.Event do struct!(__MODULE__, domain: domain, request: request) end - defp new(domain, site_id, request) do - struct!(__MODULE__, domain: domain, site_id: site_id, request: request) + defp new(domain, site, request) do + struct!(__MODULE__, domain: domain, site: site, request: request) end defp drop(%__MODULE__{} = event, reason, attrs \\ []) do @@ -163,7 +163,7 @@ defmodule Plausible.Ingestion.Event do defp put_basic_info(%__MODULE__{} = event) do update_attrs(event, %{ domain: event.domain, - site_id: event.site_id, + site_id: event.site.id, timestamp: event.request.timestamp, name: event.request.event_name, hostname: event.request.hostname, @@ -208,10 +208,8 @@ defmodule Plausible.Ingestion.Event do defp put_props(%__MODULE__{} = event), do: event defp put_revenue(%__MODULE__{request: %{revenue_source: %Money{} = revenue_source}} = event) do - revenue_goals = Plausible.Site.Cache.get(event.domain).revenue_goals || [] - matching_goal = - Enum.find(revenue_goals, &(&1.event_name == event.clickhouse_event_attrs.name)) + Enum.find(event.site.revenue_goals, &(&1.event_name == event.clickhouse_event_attrs.name)) cond do is_nil(matching_goal) -> diff --git a/lib/plausible/site/cache.ex b/lib/plausible/site/cache.ex index d4108eccf7..8c62675c68 100644 --- a/lib/plausible/site/cache.ex +++ b/lib/plausible/site/cache.ex @@ -102,9 +102,9 @@ defmodule Plausible.Site.Cache do @spec refresh_updated_recently(Keyword.t()) :: :ok def refresh_updated_recently(opts \\ []) do recently_updated_sites_query = - from [s, mg] in sites_by_domain_query(), + from [s, _rg] in sites_by_domain_query(), order_by: [asc: s.updated_at], - where: s.updated_at > ago(^15, "minute") or mg.updated_at > ago(^15, "minute") + where: s.updated_at > ago(^15, "minute") refresh( :updated_recently, @@ -115,13 +115,13 @@ defmodule Plausible.Site.Cache do defp sites_by_domain_query do from s in Site, - left_join: mg in assoc(s, :revenue_goals), + left_join: rg in assoc(s, :revenue_goals), select: { s.domain, s.domain_changed_from, %{struct(s, ^@cached_schema_fields) | from_cache?: true} }, - preload: [revenue_goals: mg] + preload: [revenue_goals: rg] end @spec merge(new_items :: [Site.t()], opts :: Keyword.t()) :: :ok diff --git a/lib/plausible/site/gate_keeper.ex b/lib/plausible/site/gate_keeper.ex index b7dcc65d1f..8b064f8e25 100644 --- a/lib/plausible/site/gate_keeper.ex +++ b/lib/plausible/site/gate_keeper.ex @@ -1,9 +1,8 @@ defmodule Plausible.Site.GateKeeper do @type policy() :: :allow | :not_found | :block | :throttle - @type site_id() :: pos_integer() @policy_for_non_existing_sites :not_found - @type t() :: {:allow, site_id()} | {:deny, policy()} + @type t() :: {:allow, Plausible.Site.t()} | {:deny, policy()} @moduledoc """ Thin wrapper around Hammer for gate keeping domain-specific events @@ -33,7 +32,7 @@ defmodule Plausible.Site.GateKeeper do @spec check(String.t(), Keyword.t()) :: t() def check(domain, opts \\ []) when is_binary(domain) do case policy(domain, opts) do - {:allow, site_id} -> {:allow, site_id} + {:allow, site} -> {:allow, site} other -> {:deny, other} end end @@ -56,15 +55,15 @@ defmodule Plausible.Site.GateKeeper do result end - defp check_rate_limit(%Site{id: site_id, ingest_rate_limit_threshold: nil}, _opts) do - {:allow, site_id} + defp check_rate_limit(%Site{ingest_rate_limit_threshold: nil} = site, _opts) do + {:allow, site} end defp check_rate_limit(%Site{ingest_rate_limit_threshold: 0}, _opts) do :block end - defp check_rate_limit(%Site{id: site_id, ingest_rate_limit_threshold: threshold} = site, opts) + defp check_rate_limit(%Site{ingest_rate_limit_threshold: threshold} = site, opts) when is_integer(threshold) do key = Keyword.get(opts, :key, key(site.domain)) scale_ms = site.ingest_rate_limit_scale_seconds * 1_000 @@ -74,14 +73,14 @@ defmodule Plausible.Site.GateKeeper do :throttle {:allow, _} -> - {:allow, site_id} + {:allow, site} {:error, reason} -> Logger.error( "Error checking rate limit for '#{key}': #{inspect(reason)}. Falling back to: :allow" ) - {:allow, site_id} + {:allow, site} end end end diff --git a/test/plausible/goals_test.exs b/test/plausible/goals_test.exs index 2df4c2b782..54f026a55c 100644 --- a/test/plausible/goals_test.exs +++ b/test/plausible/goals_test.exs @@ -18,6 +18,20 @@ defmodule Plausible.GoalsTest do assert {"should be at most %{count} character(s)", _} = changeset.errors[:event_name] end + test "create/2 sets site.updated_at for revenue goal" do + site_1 = insert(:site, updated_at: DateTime.add(DateTime.utc_now(), -3600)) + {:ok, _goal_1} = Goals.create(site_1, %{"event_name" => "Checkout", "currency" => "BRL"}) + + assert NaiveDateTime.compare(site_1.updated_at, Plausible.Repo.reload!(site_1).updated_at) == + :lt + + site_2 = insert(:site, updated_at: DateTime.add(DateTime.utc_now(), -3600)) + {:ok, _goal_2} = Goals.create(site_2, %{"event_name" => "Read Article", "currency" => nil}) + + assert NaiveDateTime.compare(site_2.updated_at, Plausible.Repo.reload!(site_2).updated_at) == + :eq + end + test "for_site2 returns trimmed input even if it was saved with trailing whitespace" do site = insert(:site) insert(:goal, %{site: site, event_name: " Signup "}) diff --git a/test/plausible/site/cache_test.exs b/test/plausible/site/cache_test.exs index 9f6233a82f..b6733892e6 100644 --- a/test/plausible/site/cache_test.exs +++ b/test/plausible/site/cache_test.exs @@ -55,15 +55,21 @@ defmodule Plausible.Site.CacheTest do test "cache caches revenue goals", %{test: test} do {:ok, _} = - Supervisor.start_link([{Cache, [cache_name: test, child_id: :test_cache_caches_id]}], + Supervisor.start_link( + [{Cache, [cache_name: test, child_id: :test_cache_caches_revenue_goals]}], strategy: :one_for_one, name: Test.Supervisor.Cache ) %{id: site_id} = site = insert(:site, domain: "site1.example.com") - insert(:goal, site: site, event_name: "Purchase", currency: :BRL) - insert(:goal, site: site, event_name: "Add to Cart", currency: :USD) - insert(:goal, site: site, event_name: "Click", currency: nil) + + {:ok, _goal} = + Plausible.Goals.create(site, %{"event_name" => "Purchase", "currency" => :BRL}) + + {:ok, _goal} = + Plausible.Goals.create(site, %{"event_name" => "Add to Cart", "currency" => :USD}) + + {:ok, _goal} = Plausible.Goals.create(site, %{"event_name" => "Click", "currency" => nil}) :ok = Cache.refresh_all(cache_name: test) @@ -78,6 +84,47 @@ defmodule Plausible.Site.CacheTest do ] = Enum.sort_by(cached_goals, & &1.event_name) end + test "cache caches revenue goals with event refresh", %{test: test} do + {:ok, _} = + Supervisor.start_link( + [{Cache, [cache_name: test, child_id: :test_revenue_goals_event_refresh]}], + strategy: :one_for_one, + name: Test.Supervisor.Cache + ) + + yesterday = DateTime.utc_now() |> DateTime.add(-1 * 60 * 60 * 24) + + # the site was added yesterday so full refresh will pick it up + %{id: site_id} = site = insert(:site, domain: "site1.example.com", updated_at: yesterday) + # the goal was added yesterday so full refresh will pick it up + Plausible.Goals.create(site, %{"event_name" => "Purchase", "currency" => :BRL}, yesterday) + # this goal is added "just now" + Plausible.Goals.create(site, %{"event_name" => "Add to Cart", "currency" => :USD}) + # and this one does not matter + Plausible.Goals.create(site, %{"event_name" => "Click", "currency" => nil}) + + # at this point, we have 3 goals associated with the cached struct + :ok = Cache.refresh_all(cache_name: test) + + # the goal was added 70 seconds ago so partial refresh should pick it up and merge with the rest of goals + Plausible.Goals.create( + site, + %{"event_name" => "Purchase2", "currency" => :BRL}, + DateTime.add(DateTime.utc_now(), -70) + ) + + :ok = Cache.refresh_updated_recently(cache_name: test) + + assert %Site{from_cache?: true, id: ^site_id, revenue_goals: cached_goals} = + Cache.get("site1.example.com", force?: true, cache_name: test) + + assert [ + %Goal{event_name: "Add to Cart", currency: :USD}, + %Goal{event_name: "Purchase", currency: :BRL}, + %Goal{event_name: "Purchase2", currency: :BRL} + ] = Enum.sort_by(cached_goals, & &1.event_name) + end + test "cache is ready when no sites exist in the db", %{test: test} do {:ok, _} = start_test_cache(test) assert Cache.ready?(test) diff --git a/test/plausible/site/gate_keeper_test.exs b/test/plausible/site/gate_keeper_test.exs index 44a1233a37..e5f34510b9 100644 --- a/test/plausible/site/gate_keeper_test.exs +++ b/test/plausible/site/gate_keeper_test.exs @@ -20,7 +20,9 @@ defmodule Plausible.Site.GateKeeperTest do domain = "site1.example.com" %{id: site_id} = add_site_and_refresh_cache(test, domain: domain) - assert {:allow, ^site_id} = GateKeeper.check(domain, opts) + + assert {:allow, %Plausible.Site{id: ^site_id, from_cache?: true}} = + GateKeeper.check(domain, opts) end test "rate limiting works with threshold", %{test: test, opts: opts} do @@ -33,7 +35,9 @@ defmodule Plausible.Site.GateKeeperTest do ingest_rate_limit_scale_seconds: 60 ) - assert {:allow, ^site_id} = GateKeeper.check(domain, opts) + assert {:allow, %Plausible.Site{id: ^site_id, from_cache?: true}} = + GateKeeper.check(domain, opts) + assert {:deny, :throttle} = GateKeeper.check(domain, opts) assert {:deny, :throttle} = GateKeeper.check(domain, opts) end @@ -48,11 +52,15 @@ defmodule Plausible.Site.GateKeeperTest do ingest_rate_limit_scale_seconds: 1 ) - assert {:allow, ^site_id} = GateKeeper.check(domain, opts) + assert {:allow, %Plausible.Site{id: ^site_id, from_cache?: true}} = + GateKeeper.check(domain, opts) + Process.sleep(1) assert {:deny, :throttle} = GateKeeper.check(domain, opts) Process.sleep(1_000) - assert {:allow, ^site_id} = GateKeeper.check(domain, opts) + + assert {:allow, %Plausible.Site{id: ^site_id, from_cache?: true}} = + GateKeeper.check(domain, opts) end test "rate limiting prioritises cache lookups", %{test: test, opts: opts} do @@ -71,7 +79,9 @@ defmodule Plausible.Site.GateKeeperTest do insert(:site) deleted_site_id = site.id - assert {:allow, ^deleted_site_id} = GateKeeper.check(domain, opts) + assert {:allow, %Plausible.Site{id: ^deleted_site_id, from_cache?: true}} = + GateKeeper.check(domain, opts) + :ok = Cache.refresh_all(opts[:cache_opts]) assert {:deny, :not_found} = GateKeeper.check(domain, opts) end @@ -90,14 +100,18 @@ defmodule Plausible.Site.GateKeeperTest do ) site_id = site.id - assert {:allow, ^site_id} = GateKeeper.check(domain, opts) + + assert {:allow, %Plausible.Site{id: ^site_id, from_cache?: true}} = + GateKeeper.check(domain, opts) + assert {:deny, :throttle} = GateKeeper.check(domain, opts) {:ok, :broken} = break_hammer(site) log = capture_log(fn -> - assert {:allow, ^site_id} = GateKeeper.check(domain, opts) + assert {:allow, %Plausible.Site{id: ^site_id, from_cache?: true}} = + GateKeeper.check(domain, opts) end) assert log =~ "Error checking rate limit for 'ingest:site:causingerrors.example.com'"