Permit :has_not_done filter in segments

lib/plausible/segments/filters.ex

@@ -64,7 +64,7 @@ defmodule Plausible.Segments.Filters do
   end
 
   defp replace_segment_with_filter_tree([_, "segment", clauses], preloaded_segments) do
-    if length(clauses) === 1 do
+    if length(clauses) == 1 do
       [[:and, Map.get(preloaded_segments, Enum.at(clauses, 0))]]
     else
       [[:or, Enum.map(clauses, fn id -> [:and, Map.get(preloaded_segments, id)] end)]]

lib/plausible/segments/segment.ex

@@ -197,9 +197,15 @@ defmodule Plausible.Segments.Segment do
   end
 
   defp dashboard_compatible_filter?(filter) do
-    is_list(filter) and length(filter) === 3 and
-      is_atom(Enum.at(filter, 0)) and
-      is_binary(Enum.at(filter, 1)) and
-      is_list(Enum.at(filter, 2))
+    regular_filter? =
+      is_list(filter) and length(filter) == 3 and
+        is_atom(Enum.at(filter, 0)) and
+        is_binary(Enum.at(filter, 1)) and
+        is_list(Enum.at(filter, 2))
+
+    has_not_done_filter? =
+      is_list(filter) and length(filter) == 2 and Enum.at(filter, 0) == :has_not_done
+
+    regular_filter? or has_not_done_filter?
   end
 end

lib/plausible/stats/filters/query_parser.ex

@@ -566,7 +566,7 @@ defmodule Plausible.Stats.Filters.QueryParser do
       :ok
     else
       {:error,
-       "The goal `#{clause}` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals"}
+       "Invalid filters. The goal `#{clause}` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals"}
     end
   end
 

test/plausible/stats/query_parser_test.exs

@@ -1137,7 +1137,7 @@ defmodule Plausible.Stats.Filters.QueryParserTest do
       }
       |> check_error(
         site,
-        "The goal `Signup` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals"
+        "Invalid filters. The goal `Signup` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals"
       )
     end
 
@@ -1152,7 +1152,7 @@ defmodule Plausible.Stats.Filters.QueryParserTest do
       }
       |> check_error(
         site,
-        "The goal `Visit /thank-you` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals"
+        "Invalid filters. The goal `Visit /thank-you` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals"
       )
     end
 
@@ -1255,7 +1255,7 @@ defmodule Plausible.Stats.Filters.QueryParserTest do
       }
       |> check_error(
         site,
-        "The goal `Unknown` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals",
+        "Invalid filters. The goal `Unknown` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals",
         :internal
       )
     end

test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs

@@ -341,12 +341,18 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do
            %{conn: conn, user: user} do
         site = new_site()
         add_guest(site, user: user, role: unquote(role))
+        insert(:goal, site: site, event_name: "Conversion")
 
         response =
           post(conn, "/api/#{site.domain}/segments", %{
             "name" => "Some segment",
             "type" => "#{unquote(type)}",
-            "segment_data" => %{"filters" => [["is", "visit:entry_page", ["/blog"]]]}
+            "segment_data" => %{
+              "filters" => [
+                ["is", "visit:entry_page", ["/blog"]],
+                ["has_not_done", ["is", "event:goal", ["Conversion"]]]
+              ]
+            }
           })
           |> json_response(200)
 
@@ -355,7 +361,12 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do
                          "name" => "Some segment",
                          "type" => ^"#{unquote(type)}",
                          "segment_data" =>
-                           ^strict_map(%{"filters" => [["is", "visit:entry_page", ["/blog"]]]}),
+                           ^strict_map(%{
+                             "filters" => [
+                               ["is", "visit:entry_page", ["/blog"]],
+                               ["has_not_done", ["is", "event:goal", ["Conversion"]]]
+                             ]
+                           }),
                          "owner_id" => ^user.id,
                          "inserted_at" => ^any(:iso8601_naive_datetime),
                          "updated_at" => ^any(:iso8601_naive_datetime)
@@ -477,6 +488,75 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do
              }
     end
 
+    test "updating a segment containing a goal that has been deleted, with the deleted goal still in filters, fails",
+         %{
+           conn: conn,
+           user: user,
+           site: site
+         } do
+      segment =
+        insert(:segment,
+          site: site,
+          name: "any name",
+          type: :personal,
+          owner: user,
+          segment_data: %{"filters" => [["is", "event:goal", ["Signup"]]]}
+        )
+
+      conn =
+        patch(conn, "/api/#{site.domain}/segments/#{segment.id}", %{
+          "segment_data" => %{
+            "filters" => [["is", "event:goal", ["Signup"]], ["is", "event:page", ["/register"]]]
+          }
+        })
+
+      assert json_response(conn, 400) == %{
+               "error" =>
+                 "segment_data Invalid filters. The goal `Signup` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals"
+             }
+    end
+
+    test "a segment containing a goal that has been deleted can be updated to not contain the goal",
+         %{
+           conn: conn,
+           user: user,
+           site: site
+         } do
+      segment =
+        insert(:segment,
+          site: site,
+          name: "any name",
+          type: :personal,
+          owner: user,
+          segment_data: %{"filters" => [["is", "event:goal", ["Signup"]]]}
+        )
+
+      insert(:goal, site: site, event_name: "a new goal")
+
+      response =
+        patch(conn, "/api/#{site.domain}/segments/#{segment.id}", %{
+          "segment_data" => %{
+            "filters" => [["is", "event:goal", ["a new goal"]]]
+          }
+        })
+        |> json_response(200)
+
+      assert_matches ^strict_map(%{
+                       "id" => ^segment.id,
+                       "name" => ^segment.name,
+                       "type" => ^any(:string, ~r/#{segment.type}/),
+                       "segment_data" =>
+                         ^strict_map(%{
+                           "filters" => [
+                             ["is", "event:goal", ["a new goal"]]
+                           ]
+                         }),
+                       "owner_id" => ^user.id,
+                       "inserted_at" => ^any(:iso8601_naive_datetime),
+                       "updated_at" => ^any(:iso8601_naive_datetime)
+                     }) = response
+    end
+
     test "editors can update a segment", %{conn: conn, user: user} do
       site = new_site()
       add_guest(site, user: user, role: :editor)
@@ -536,12 +616,33 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do
       verify_segment_in_db(segment)
     end
 
+    test "even site owners can't delete personal segments of other users",
+         %{conn: conn, site: site} do
+      other_user = add_guest(site, role: :editor)
+
+      segment =
+        insert(:segment,
+          site: site,
+          owner_id: other_user.id,
+          name: "any",
+          type: :personal
+        )
+
+      conn =
+        delete(conn, "/api/#{site.domain}/segments/#{segment.id}")
+
+      assert %{"error" => "Segment not found with ID \"#{segment.id}\""} ==
+               json_response(conn, 404)
+
+      verify_segment_in_db(segment)
+    end
+
     for %{role: role, type: type} <- [
           %{role: :viewer, type: :personal},
           %{role: :editor, type: :personal},
           %{role: :editor, type: :site}
         ] do
-      test "#{role} can delete segment with type \"#{type}\" successfully",
+      test "#{role} can delete their own segment with type \"#{type}\" successfully",
            %{conn: conn, user: user} do
         site = new_site()
         add_guest(site, user: user, role: unquote(role))
@@ -571,6 +672,36 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do
         verify_no_segment_in_db(segment)
       end
     end
+
+    test "site owner can delete a site segment owned by someone else, even if it contains a non-existing goal",
+         %{conn: conn, site: site} do
+      other_user = add_guest(site, role: :editor)
+
+      segment =
+        insert(:segment,
+          site: site,
+          owner: other_user,
+          name: "any",
+          type: :site,
+          segment_data: %{"filters" => [["is", "event:goal", ["non-existing goal"]]]}
+        )
+
+      response =
+        delete(conn, "/api/#{site.domain}/segments/#{segment.id}")
+        |> json_response(200)
+
+      assert %{
+               "owner_id" => other_user.id,
+               "id" => segment.id,
+               "name" => segment.name,
+               "segment_data" => segment.segment_data,
+               "type" => "site",
+               "inserted_at" => NaiveDateTime.to_iso8601(segment.inserted_at),
+               "updated_at" => NaiveDateTime.to_iso8601(segment.updated_at)
+             } == response
+
+      verify_no_segment_in_db(segment)
+    end
   end
 
   defp verify_segment_in_db(segment) do