diff --git a/lib/plausible/goals/goals.ex b/lib/plausible/goals/goals.ex index af17a254f4eb..c3229c96d639 100644 --- a/lib/plausible/goals/goals.ex +++ b/lib/plausible/goals/goals.ex @@ -56,16 +56,20 @@ defmodule Plausible.Goals do {:ok, Goal.t()} | {:error, Ecto.Changeset.t()} | {:error, :upgrade_required} def update(goal, params) do changeset = Goal.changeset(goal, params) - site = Repo.preload(goal, :site).site - with :ok <- maybe_check_feature_access(site, changeset), - {:ok, goal} <- Repo.update(changeset) do - on_ee do - {:ok, Repo.preload(goal, :funnels)} - else - {:ok, goal} + Repo.transaction(fn -> + site = Repo.preload(goal, :site).site + + with :ok <- maybe_check_feature_access(site, changeset), + {:ok, updated_goal} <- Repo.update(changeset), + :ok <- Plausible.Segments.update_goal_in_segments(site, goal, updated_goal) do + on_ee do + Repo.preload(updated_goal, :funnels) + else + updated_goal + end end - end + end) end def find_or_create( diff --git a/lib/plausible/segments/segments.ex b/lib/plausible/segments/segments.ex index 37f420e6d604..a80950180c45 100644 --- a/lib/plausible/segments/segments.ex +++ b/lib/plausible/segments/segments.ex @@ -131,6 +131,48 @@ defmodule Plausible.Segments do end end + def update_goal_in_segments( + %Plausible.Site{} = site, + %Plausible.Goal{} = stale_goal, + %Plausible.Goal{} = updated_goal + ) do + goal_filter_regex = + ~s(.*?\\[\s*"is",\s*"event:goal",\s*\\[.*?"#{Regex.escape(stale_goal.display_name)}".*?\\]\s*\\].*?) + + segments_to_update = + from( + s in Segment, + where: s.site_id == ^site.id, + where: fragment("?['filters']::text ~ ?", s.segment_data, ^goal_filter_regex) + ) + + stale_goal_name = stale_goal.display_name + + for segment <- Repo.all(segments_to_update) do + updated_filters = + Plausible.Stats.Filters.transform_filters(segment.segment_data["filters"], fn + ["is", "event:goal", clauses] -> + new_clauses = + Enum.map(clauses, fn + ^stale_goal_name -> updated_goal.display_name + clause -> clause + end) + + [["is", "event:goal", new_clauses]] + + _ -> + nil + end) + + updated_segment_data = Map.put(segment.segment_data, "filters", updated_filters) + + Segment.changeset(segment, %{segment_data: updated_segment_data}) + |> Repo.update!() + end + + :ok + end + def delete_one(user_id, %Plausible.Site{} = site, site_role, segment_id) do with {:ok, segment} <- get_one(user_id, site, site_role, segment_id) do cond do diff --git a/test/plausible/goals_test.exs b/test/plausible/goals_test.exs index ede9287ba3a6..64f411f94ffa 100644 --- a/test/plausible/goals_test.exs +++ b/test/plausible/goals_test.exs @@ -117,6 +117,53 @@ defmodule Plausible.GoalsTest do assert goal2.display_name == "Homepage" end + test "update/2 also updates all segments the goal is a part of" do + user = new_user() + site = new_site(owner: user) + {:ok, goal1} = Goals.create(site, %{"event_name" => "Signup"}) + {:ok, _goal2} = Goals.create(site, %{"event_name" => "Signup from nav"}) + + {:ok, segment1} = + Plausible.Segments.insert_one(user.id, site, :editor, %{ + "type" => "site", + "segment_data" => %{ + "filters" => [ + ["is", "event:page", ["/blog"]], + ["is", "event:goal", ["Signup from nav", "Signup"]], + ["is", "event:props:variant", ["A"]] + ] + }, + "name" => "Site segment" + }) + + {:ok, segment2} = + Plausible.Segments.insert_one(user.id, site, :editor, %{ + "type" => "personal", + "segment_data" => %{ + "filters" => [ + ["is", "event:goal", ["Signup"]] + ] + }, + "name" => "Personal segment" + }) + + Goals.update(goal1, %{"display_name" => "SIGNUP"}) + + assert Repo.reload!(segment1).segment_data == %{ + "filters" => [ + ["is", "event:page", ["/blog"]], + ["is", "event:goal", ["Signup from nav", "SIGNUP"]], + ["is", "event:props:variant", ["A"]] + ] + } + + assert Repo.reload!(segment2).segment_data == %{ + "filters" => [ + ["is", "event:goal", ["SIGNUP"]] + ] + } + end + @tag :ee_only test "list_revenue_goals/1 lists event_names and currencies for each revenue goal" do site = new_site()