From e1485f4509a624412c8e555617e49cb6f979bed1 Mon Sep 17 00:00:00 2001
From: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>
Date: Tue, 2 Jul 2024 16:55:03 +0200
Subject: [PATCH 01/10] Add listing sites, goals and custom props to Sites API

---
 .../api/external_sites_controller.ex          | 145 +++++++----
 lib/plausible/sites.ex                        |   9 +
 lib/plausible_web/router.ex                   |   2 +
 .../api/external_sites_controller_test.exs    | 227 +++++++++++++++++-
 4 files changed, 332 insertions(+), 51 deletions(-)

diff --git a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
index 78da9e0dd8ca..fa6f2c5f4eb2 100644
--- a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
+++ b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
@@ -2,12 +2,52 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
   use PlausibleWeb, :controller
   use Plausible.Repo
   use PlausibleWeb.Plugs.ErrorHandler
+
+  import Plausible.Pagination
+
   alias Plausible.Sites
   alias Plausible.Goals
   alias PlausibleWeb.Api.Helpers, as: H
 
+  def index(conn, params) do
+    user = conn.assigns.current_user
+
+    page =
+      user
+      |> Sites.for_user_query()
+      |> paginate(params, cursor_fields: [{:id, :desc}])
+
+    json(conn, %{
+      sites: page.entries,
+      meta: pagination_meta(page.metadata)
+    })
+  end
+
+  def goals_index(conn, params) do
+    user = conn.assigns.current_user
+
+    with {:ok, site_id} <- expect_param_key(params, "site_id"),
+         {:ok, site} <- get_site(user, site_id, [:owner, :admin]) do
+      page =
+        site
+        |> Plausible.Goals.for_site_query()
+        |> paginate(params, cursor_fields: [{:id, :desc}])
+
+      json(conn, %{
+        goals: page.entries,
+        meta: pagination_meta(page.metadata)
+      })
+    else
+      {:missing, "site_id"} ->
+        H.bad_request(conn, "Parameter `site_id` is required to list goals")
+
+      {:error, :site_not_found} ->
+        H.not_found(conn, "Site could not be found")
+    end
+  end
+
   def create_site(conn, params) do
-    user = conn.assigns[:current_user]
+    user = conn.assigns.current_user
 
     case Sites.create(user, params) do
       {:ok, %{site: site}} ->
@@ -29,57 +69,50 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
   end
 
   def get_site(conn, %{"site_id" => site_id}) do
-    site = Sites.get_for_user(conn.assigns[:current_user].id, site_id, [:owner, :admin])
+    case get_site(conn.assigns.current_user, site_id, [:owner, :admin]) do
+      {:ok, site} ->
+        json(conn, %{
+          domain: site.domain,
+          timezone: site.timezone,
+          allowed_custom_props: site.allowed_event_props || []
+        })
 
-    if site do
-      json(conn, site)
-    else
-      H.not_found(conn, "Site could not be found")
+      {:error, :site_not_found} ->
+        H.not_found(conn, "Site could not be found")
     end
   end
 
   def delete_site(conn, %{"site_id" => site_id}) do
-    site = Sites.get_for_user(conn.assigns[:current_user].id, site_id, [:owner])
+    case get_site(conn.assigns.current_user, site_id, [:owner]) do
+      {:ok, site} ->
+        {:ok, _} = Plausible.Site.Removal.run(site.domain)
+        json(conn, %{"deleted" => true})
 
-    if site do
-      {:ok, _} = Plausible.Site.Removal.run(site.domain)
-      json(conn, %{"deleted" => true})
-    else
-      H.not_found(conn, "Site could not be found")
+      {:error, :site_not_found} ->
+        H.not_found(conn, "Site could not be found")
     end
   end
 
   def update_site(conn, %{"site_id" => site_id} = params) do
     # for now this only allows to change the domain
-    site = Sites.get_for_user(conn.assigns[:current_user].id, site_id, [:owner, :admin])
-
-    if site do
-      case Plausible.Site.Domain.change(site, params["domain"]) do
-        {:ok, site} ->
-          json(conn, site)
-
-        {:error, changeset} ->
-          conn
-          |> put_status(400)
-          |> json(serialize_errors(changeset))
-      end
+    with {:ok, site} <- get_site(conn.assigns.current_user, site_id, [:owner, :admin]),
+         {:ok, site} <- Plausible.Site.Domain.change(site, params["domain"]) do
+      json(conn, site)
     else
-      H.not_found(conn, "Site could not be found")
-    end
-  end
+      {:error, :site_not_found} ->
+        H.not_found(conn, "Site could not be found")
 
-  defp expect_param_key(params, key) do
-    case Map.fetch(params, key) do
-      :error -> {:missing, key}
-      res -> res
+      {:error, %Ecto.Changeset{} = changeset} ->
+        conn
+        |> put_status(400)
+        |> json(serialize_errors(changeset))
     end
   end
 
   def find_or_create_shared_link(conn, params) do
     with {:ok, site_id} <- expect_param_key(params, "site_id"),
          {:ok, link_name} <- expect_param_key(params, "name"),
-         site when not is_nil(site) <-
-           Sites.get_for_user(conn.assigns[:current_user].id, site_id, [:owner, :admin]) do
+         {:ok, site} <- get_site(conn.assigns.current_user, site_id, [:owner, :admin]) do
       shared_link = Repo.get_by(Plausible.Site.SharedLink, site_id: site.id, name: link_name)
 
       shared_link =
@@ -96,7 +129,7 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
           })
       end
     else
-      nil ->
+      {:error, :site_not_found} ->
         H.not_found(conn, "Site could not be found")
 
       {:missing, "site_id"} ->
@@ -113,12 +146,11 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
   def find_or_create_goal(conn, params) do
     with {:ok, site_id} <- expect_param_key(params, "site_id"),
          {:ok, _} <- expect_param_key(params, "goal_type"),
-         site when not is_nil(site) <-
-           Sites.get_for_user(conn.assigns[:current_user].id, site_id, [:owner, :admin]),
+         {:ok, site} <- get_site(conn.assigns.current_user, site_id, [:owner, :admin]),
          {:ok, goal} <- Goals.find_or_create(site, params) do
       json(conn, goal)
     else
-      nil ->
+      {:error, :site_not_found} ->
         H.not_found(conn, "Site could not be found")
 
       {:missing, param} ->
@@ -132,19 +164,16 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
   def delete_goal(conn, params) do
     with {:ok, site_id} <- expect_param_key(params, "site_id"),
          {: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) do
-        :ok ->
-          json(conn, %{"deleted" => true})
-
-        {:error, :not_found} ->
-          H.not_found(conn, "Goal could not be found")
-      end
+         {:ok, site} <- get_site(conn.assigns.current_user, site_id, [:owner, :admin]),
+         :ok <- Goals.delete(goal_id, site) do
+      json(conn, %{"deleted" => true})
     else
-      nil ->
+      {:error, :site_not_found} ->
         H.not_found(conn, "Site could not be found")
 
+      {:error, :not_found} ->
+        H.not_found(conn, "Goal could not be found")
+
       {:missing, "site_id"} ->
         H.bad_request(conn, "Parameter `site_id` is required to delete a goal")
 
@@ -156,9 +185,31 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
     end
   end
 
+  defp pagination_meta(meta) do
+    %{
+      after: meta.after,
+      before: meta.before,
+      limit: meta.limit
+    }
+  end
+
+  defp get_site(user, site_id, roles) do
+    case Sites.get_for_user(user.id, site_id, roles) do
+      nil -> {:error, :site_not_found}
+      site -> {:ok, site}
+    end
+  end
+
   defp serialize_errors(changeset) do
     {field, {msg, _opts}} = List.first(changeset.errors)
     error_msg = Atom.to_string(field) <> ": " <> msg
     %{"error" => error_msg}
   end
+
+  defp expect_param_key(params, key) do
+    case Map.fetch(params, key) do
+      :error -> {:missing, key}
+      res -> res
+    end
+  end
 end
diff --git a/lib/plausible/sites.ex b/lib/plausible/sites.ex
index a23c72952d4b..abd33e438da8 100644
--- a/lib/plausible/sites.ex
+++ b/lib/plausible/sites.ex
@@ -159,6 +159,15 @@ defmodule Plausible.Sites do
     %{result | entries: entries}
   end
 
+  @spec for_user_query(Auth.User.t()) :: Ecto.Query.t()
+  def for_user_query(user) do
+    from(s in Site,
+      inner_join: sm in assoc(s, :memberships),
+      on: sm.user_id == ^user.id,
+      order_by: [desc: s.id]
+    )
+  end
+
   defp maybe_filter_by_domain(query, domain)
        when byte_size(domain) >= 1 and byte_size(domain) <= 64 do
     where(query, [s], ilike(s.domain, ^"%#{domain}%"))
diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex
index 4cdea939c8e5..7944890db442 100644
--- a/lib/plausible_web/router.ex
+++ b/lib/plausible_web/router.ex
@@ -188,6 +188,8 @@ defmodule PlausibleWeb.Router do
       scope assigns: %{api_scope: "sites:read:*"} do
         pipe_through PlausibleWeb.Plugs.AuthorizePublicAPI
 
+        get "/", ExternalSitesController, :index
+        get "/goals", ExternalSitesController, :goals_index
         get "/:site_id", ExternalSitesController, :get_site
       end
 
diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs
index 0a079648ac8b..d8dd56c9ac7d 100644
--- a/test/plausible_web/controllers/api/external_sites_controller_test.exs
+++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs
@@ -489,13 +489,104 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
       end
     end
 
+    describe "GET /api/v1/sites" do
+      test "returns empty when there are no sites for user", %{conn: conn} do
+        conn = get(conn, "/api/v1/sites")
+
+        assert json_response(conn, 200) == %{
+                 "sites" => [],
+                 "meta" => %{
+                   "before" => nil,
+                   "after" => nil,
+                   "limit" => 10
+                 }
+               }
+      end
+
+      test "returns sites when present", %{conn: conn, user: user} do
+        [site1, site2] = insert_list(2, :site, members: [user])
+
+        conn = get(conn, "/api/v1/sites")
+
+        assert json_response(conn, 200) == %{
+                 "sites" => [
+                   %{"domain" => site2.domain, "timezone" => site2.timezone},
+                   %{"domain" => site1.domain, "timezone" => site1.timezone}
+                 ],
+                 "meta" => %{
+                   "before" => nil,
+                   "after" => nil,
+                   "limit" => 10
+                 }
+               }
+      end
+
+      test "handles pagination correctly", %{conn: conn, user: user} do
+        [
+          %{domain: site1_domain},
+          %{domain: site2_domain},
+          %{domain: site3_domain}
+        ] = insert_list(3, :site, members: [user])
+
+        conn1 = get(conn, "/api/v1/sites?limit=2")
+
+        assert %{
+                 "sites" => [
+                   %{"domain" => ^site3_domain},
+                   %{"domain" => ^site2_domain}
+                 ],
+                 "meta" => %{
+                   "before" => nil,
+                   "after" => after_cursor,
+                   "limit" => 2
+                 }
+               } = json_response(conn1, 200)
+
+        conn2 = get(conn, "/api/v1/sites?limit=2&after=" <> after_cursor)
+
+        assert %{
+                 "sites" => [
+                   %{"domain" => ^site1_domain}
+                 ],
+                 "meta" => %{
+                   "before" => before_cursor,
+                   "after" => nil,
+                   "limit" => 2
+                 }
+               } = json_response(conn2, 200)
+
+        assert is_binary(before_cursor)
+      end
+
+      test "lists sites for user with read-only scope", %{conn: conn, user: user} do
+        %{domain: site_domain} = insert(:site, members: [user])
+        api_key = insert(:api_key, user: user, scopes: ["stats:read:*"])
+
+        conn =
+          conn
+          |> Plug.Conn.put_req_header("authorization", "Bearer #{api_key.key}")
+          |> get("/api/v1/sites")
+
+        assert %{"sites" => [%{"domain" => ^site_domain}]} = json_response(conn, 200)
+      end
+    end
+
     describe "GET /api/v1/sites/:site_id" do
       setup :create_new_site
 
       test "get a site by its domain", %{conn: conn, site: site} do
+        site =
+          site
+          |> Ecto.Changeset.change(allowed_event_props: ["goals", "funnels"])
+          |> Repo.update!()
+
         conn = get(conn, "/api/v1/sites/" <> site.domain)
 
-        assert json_response(conn, 200) == %{"domain" => site.domain, "timezone" => site.timezone}
+        assert json_response(conn, 200) == %{
+                 "domain" => site.domain,
+                 "timezone" => site.timezone,
+                 "allowed_custom_props" => ["goals", "funnels"]
+               }
       end
 
       test "get a site by old site_id after domain change", %{conn: conn, site: site} do
@@ -506,10 +597,14 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
 
         conn = get(conn, "/api/v1/sites/" <> old_domain)
 
-        assert json_response(conn, 200) == %{"domain" => new_domain, "timezone" => site.timezone}
+        assert json_response(conn, 200) == %{
+                 "domain" => new_domain,
+                 "timezone" => site.timezone,
+                 "allowed_custom_props" => []
+               }
       end
 
-      test "get a site with basic scope config", %{conn: conn, user: user, site: site} do
+      test "get a site for user with read-only scope", %{conn: conn, user: user, site: site} do
         api_key = insert(:api_key, user: user, scopes: ["stats:read:*"])
 
         conn =
@@ -517,7 +612,11 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
           |> Plug.Conn.put_req_header("authorization", "Bearer #{api_key.key}")
           |> get("/api/v1/sites/" <> site.domain)
 
-        assert json_response(conn, 200) == %{"domain" => site.domain, "timezone" => site.timezone}
+        assert json_response(conn, 200) == %{
+                 "domain" => site.domain,
+                 "timezone" => site.timezone,
+                 "allowed_custom_props" => []
+               }
       end
 
       test "is 404 when site cannot be found", %{conn: conn} do
@@ -527,6 +626,126 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
       end
     end
 
+    describe "GET /api/v1/goals" do
+      setup :create_new_site
+
+      test "returns empty when there are no goals for site", %{conn: conn, site: site} do
+        conn = get(conn, "/api/v1/sites/goals?site_id=" <> site.domain)
+
+        assert json_response(conn, 200) == %{
+                 "goals" => [],
+                 "meta" => %{
+                   "before" => nil,
+                   "after" => nil,
+                   "limit" => 10
+                 }
+               }
+      end
+
+      test "returns goals when present", %{conn: conn, site: site} do
+        goal1 = insert(:goal, %{site: site, page_path: "/login"})
+        goal2 = insert(:goal, %{site: site, event_name: "Signup"})
+        goal3 = insert(:goal, %{site: site, event_name: "Purchase", currency: "USD"})
+
+        conn = get(conn, "/api/v1/sites/goals?site_id=" <> site.domain)
+
+        assert json_response(conn, 200) == %{
+                 "goals" => [
+                   %{
+                     "id" => goal3.id,
+                     "domain" => site.domain,
+                     "event_name" => goal3.event_name,
+                     "page_path" => goal3.page_path,
+                     "goal_type" => "event"
+                   },
+                   %{
+                     "id" => goal2.id,
+                     "domain" => site.domain,
+                     "event_name" => goal2.event_name,
+                     "page_path" => goal2.page_path,
+                     "goal_type" => "event"
+                   },
+                   %{
+                     "id" => goal1.id,
+                     "domain" => site.domain,
+                     "event_name" => goal1.event_name,
+                     "page_path" => goal1.page_path,
+                     "goal_type" => "page"
+                   }
+                 ],
+                 "meta" => %{
+                   "before" => nil,
+                   "after" => nil,
+                   "limit" => 10
+                 }
+               }
+      end
+
+      test "handles pagination correctly", %{conn: conn, site: site} do
+        %{id: goal1_id} = insert(:goal, %{site: site, page_path: "/login"})
+        %{id: goal2_id} = insert(:goal, %{site: site, event_name: "Signup"})
+        %{id: goal3_id} = insert(:goal, %{site: site, event_name: "Purchase", currency: "USD"})
+
+        conn1 = get(conn, "/api/v1/sites/goals?limit=2&site_id=" <> site.domain)
+
+        assert %{
+                 "goals" => [
+                   %{"id" => ^goal3_id},
+                   %{"id" => ^goal2_id}
+                 ],
+                 "meta" => %{
+                   "before" => nil,
+                   "after" => after_cursor,
+                   "limit" => 2
+                 }
+               } = json_response(conn1, 200)
+
+        conn2 =
+          get(conn, "/api/v1/sites/goals?limit=2&after=#{after_cursor}&site_id=" <> site.domain)
+
+        assert %{
+                 "goals" => [
+                   %{"id" => ^goal1_id}
+                 ],
+                 "meta" => %{
+                   "before" => before_cursor,
+                   "after" => nil,
+                   "limit" => 2
+                 }
+               } = json_response(conn2, 200)
+
+        assert is_binary(before_cursor)
+      end
+
+      test "lists goals for user with read-only scope", %{conn: conn, user: user, site: site} do
+        %{id: goal_id} = insert(:goal, %{site: site, page_path: "/login"})
+        api_key = insert(:api_key, user: user, scopes: ["stats:read:*"])
+
+        conn =
+          conn
+          |> Plug.Conn.put_req_header("authorization", "Bearer #{api_key.key}")
+          |> get("/api/v1/sites/goals?site_id=" <> site.domain)
+
+        assert %{"goals" => [%{"id" => ^goal_id}]} = json_response(conn, 200)
+      end
+
+      test "returns error when `site_id` parameter is missing", %{conn: conn} do
+        conn = get(conn, "/api/v1/sites/goals")
+
+        assert json_response(conn, 400) == %{
+                 "error" => "Parameter `site_id` is required to list goals"
+               }
+      end
+
+      test "returns error when `site_id` parameter is invalid", %{conn: conn} do
+        conn = get(conn, "/api/v1/sites/goals?site_id=does.not.exist")
+
+        assert json_response(conn, 404) == %{
+                 "error" => "Site could not be found"
+               }
+      end
+    end
+
     describe "PUT /api/v1/sites/:site_id" do
       setup :create_new_site
 

From 9d87c43713e92b2aa7890897a27f092a0f089b07 Mon Sep 17 00:00:00 2001
From: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>
Date: Thu, 4 Jul 2024 12:50:17 +0200
Subject: [PATCH 02/10] Rename exmaple props in tests

---
 .../controllers/api/external_sites_controller_test.exs        | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs
index d8dd56c9ac7d..b209d717847b 100644
--- a/test/plausible_web/controllers/api/external_sites_controller_test.exs
+++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs
@@ -577,7 +577,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
       test "get a site by its domain", %{conn: conn, site: site} do
         site =
           site
-          |> Ecto.Changeset.change(allowed_event_props: ["goals", "funnels"])
+          |> Ecto.Changeset.change(allowed_event_props: ["logged_in", "author"])
           |> Repo.update!()
 
         conn = get(conn, "/api/v1/sites/" <> site.domain)
@@ -585,7 +585,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
         assert json_response(conn, 200) == %{
                  "domain" => site.domain,
                  "timezone" => site.timezone,
-                 "allowed_custom_props" => ["goals", "funnels"]
+                 "allowed_custom_props" => ["logged_in", "author"]
                }
       end
 

From 63ca8a5e3d2912b84466458f3e2c946f2bb86bfe Mon Sep 17 00:00:00 2001
From: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>
Date: Thu, 4 Jul 2024 13:28:43 +0200
Subject: [PATCH 03/10] Rename `allowed_custom_props` -> `custom_properties`

---
 .../controllers/api/external_sites_controller.ex            | 2 +-
 .../controllers/api/external_sites_controller_test.exs      | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
index fa6f2c5f4eb2..173af0ec7c75 100644
--- a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
+++ b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
@@ -74,7 +74,7 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
         json(conn, %{
           domain: site.domain,
           timezone: site.timezone,
-          allowed_custom_props: site.allowed_event_props || []
+          custom_properties: site.allowed_event_props || []
         })
 
       {:error, :site_not_found} ->
diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs
index b209d717847b..ab0bcf49e410 100644
--- a/test/plausible_web/controllers/api/external_sites_controller_test.exs
+++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs
@@ -585,7 +585,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
         assert json_response(conn, 200) == %{
                  "domain" => site.domain,
                  "timezone" => site.timezone,
-                 "allowed_custom_props" => ["logged_in", "author"]
+                 "custom_properties" => ["logged_in", "author"]
                }
       end
 
@@ -600,7 +600,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
         assert json_response(conn, 200) == %{
                  "domain" => new_domain,
                  "timezone" => site.timezone,
-                 "allowed_custom_props" => []
+                 "custom_properties" => []
                }
       end
 
@@ -615,7 +615,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
         assert json_response(conn, 200) == %{
                  "domain" => site.domain,
                  "timezone" => site.timezone,
-                 "allowed_custom_props" => []
+                 "custom_properties" => []
                }
       end
 

From dae30c9cd2da743c2bdc99fe7f16941084fb3fb4 Mon Sep 17 00:00:00 2001
From: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>
Date: Thu, 4 Jul 2024 13:36:55 +0200
Subject: [PATCH 04/10] Expose goal name in GET endpoints for goals in Sites
 API

---
 .../api/external_sites_controller.ex           |  8 +++++++-
 lib/plausible/goal/schema.ex                   |  1 +
 .../api/external_sites_controller_test.exs     | 18 ++++++------------
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
index 173af0ec7c75..d9027264a23b 100644
--- a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
+++ b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
@@ -34,7 +34,13 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
         |> paginate(params, cursor_fields: [{:id, :desc}])
 
       json(conn, %{
-        goals: page.entries,
+        goals:
+          Enum.map(page.entries, fn goal ->
+            %{
+              id: goal.id,
+              name: to_string(goal)
+            }
+          end),
         meta: pagination_meta(page.metadata)
       })
     else
diff --git a/lib/plausible/goal/schema.ex b/lib/plausible/goal/schema.ex
index 0a8ca41cace3..78736c8d0eaa 100644
--- a/lib/plausible/goal/schema.ex
+++ b/lib/plausible/goal/schema.ex
@@ -103,6 +103,7 @@ defimpl Jason.Encoder, for: Plausible.Goal do
     |> Map.put(:goal_type, goal_type)
     |> Map.take([:id, :goal_type, :event_name, :page_path])
     |> Map.put(:domain, domain)
+    |> Map.put(:name, to_string(value))
     |> Jason.Encode.map(opts)
   end
 end
diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs
index ab0bcf49e410..1b0c14b88378 100644
--- a/test/plausible_web/controllers/api/external_sites_controller_test.exs
+++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs
@@ -265,6 +265,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
 
         res = json_response(conn, 200)
         assert res["goal_type"] == "event"
+        assert res["name"] == "Signup"
         assert res["event_name"] == "Signup"
         assert res["domain"] == site.domain
       end
@@ -280,6 +281,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
         res = json_response(conn, 200)
         assert res["goal_type"] == "page"
         assert res["page_path"] == "/signup"
+        assert res["name"] == "Visit /signup"
         assert res["domain"] == site.domain
       end
 
@@ -299,6 +301,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
         res = json_response(conn, 200)
         assert res["goal_type"] == "event"
         assert res["event_name"] == "Signup"
+        assert res["name"] == "Signup"
         assert res["domain"] == new_domain
       end
 
@@ -653,24 +656,15 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
                  "goals" => [
                    %{
                      "id" => goal3.id,
-                     "domain" => site.domain,
-                     "event_name" => goal3.event_name,
-                     "page_path" => goal3.page_path,
-                     "goal_type" => "event"
+                     "name" => to_string(goal3)
                    },
                    %{
                      "id" => goal2.id,
-                     "domain" => site.domain,
-                     "event_name" => goal2.event_name,
-                     "page_path" => goal2.page_path,
-                     "goal_type" => "event"
+                     "name" => to_string(goal2)
                    },
                    %{
                      "id" => goal1.id,
-                     "domain" => site.domain,
-                     "event_name" => goal1.event_name,
-                     "page_path" => goal1.page_path,
-                     "goal_type" => "page"
+                     "name" => to_string(goal1)
                    }
                  ],
                  "meta" => %{

From eec771c97d60e5b60b36cb7ec0bdeb2da9bb4af2 Mon Sep 17 00:00:00 2001
From: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>
Date: Thu, 4 Jul 2024 13:43:54 +0200
Subject: [PATCH 05/10] Bump default pagination limit to 100 and max to 1000

---
 .../controllers/api/external_sites_controller.ex          | 6 ++++--
 .../controllers/api/external_sites_controller_test.exs    | 8 ++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
index d9027264a23b..16456314f012 100644
--- a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
+++ b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
@@ -9,13 +9,15 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
   alias Plausible.Goals
   alias PlausibleWeb.Api.Helpers, as: H
 
+  @pagination_opts [cursor_fields: [{:id, :desc}], limit: 100, maximum_limit: 1000]
+
   def index(conn, params) do
     user = conn.assigns.current_user
 
     page =
       user
       |> Sites.for_user_query()
-      |> paginate(params, cursor_fields: [{:id, :desc}])
+      |> paginate(params, @pagination_opts)
 
     json(conn, %{
       sites: page.entries,
@@ -31,7 +33,7 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
       page =
         site
         |> Plausible.Goals.for_site_query()
-        |> paginate(params, cursor_fields: [{:id, :desc}])
+        |> paginate(params, @pagination_opts)
 
       json(conn, %{
         goals:
diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs
index 1b0c14b88378..5496c2522587 100644
--- a/test/plausible_web/controllers/api/external_sites_controller_test.exs
+++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs
@@ -501,7 +501,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
                  "meta" => %{
                    "before" => nil,
                    "after" => nil,
-                   "limit" => 10
+                   "limit" => 100
                  }
                }
       end
@@ -519,7 +519,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
                  "meta" => %{
                    "before" => nil,
                    "after" => nil,
-                   "limit" => 10
+                   "limit" => 100
                  }
                }
       end
@@ -640,7 +640,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
                  "meta" => %{
                    "before" => nil,
                    "after" => nil,
-                   "limit" => 10
+                   "limit" => 100
                  }
                }
       end
@@ -670,7 +670,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
                  "meta" => %{
                    "before" => nil,
                    "after" => nil,
-                   "limit" => 10
+                   "limit" => 100
                  }
                }
       end

From 7ba28b9e6bb4eda6d3b3ea82c4bd0bc3b18b21bc Mon Sep 17 00:00:00 2001
From: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>
Date: Fri, 12 Jul 2024 10:02:21 +0200
Subject: [PATCH 06/10] Introduce Goal.name/display_name and use the first one
 for name in API

---
 .../api/external_sites_controller.ex          |  3 +-
 lib/plausible/goal/schema.ex                  | 34 ++++++++++++-------
 .../api/external_sites_controller_test.exs    |  6 ++--
 3 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
index 16456314f012..5080903d61ae 100644
--- a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
+++ b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
@@ -6,6 +6,7 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
   import Plausible.Pagination
 
   alias Plausible.Sites
+  alias Plausible.Goal
   alias Plausible.Goals
   alias PlausibleWeb.Api.Helpers, as: H
 
@@ -40,7 +41,7 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
           Enum.map(page.entries, fn goal ->
             %{
               id: goal.id,
-              name: to_string(goal)
+              name: Goal.name(goal)
             }
           end),
         meta: pagination_meta(page.metadata)
diff --git a/lib/plausible/goal/schema.ex b/lib/plausible/goal/schema.ex
index 78736c8d0eaa..b24d16b917d8 100644
--- a/lib/plausible/goal/schema.ex
+++ b/lib/plausible/goal/schema.ex
@@ -47,6 +47,24 @@ defmodule Plausible.Goal do
     |> maybe_drop_currency()
   end
 
+  @spec display_name(t()) :: String.t()
+  def display_name(%{currency: nil} = goal) do
+    name(goal)
+  end
+
+  def display_name(%{currency: currency} = goal) do
+    name(goal) <> " (#{currency})"
+  end
+
+  @spec name(t()) :: String.t()
+  def name(%{page_path: page_path}) when is_binary(page_path) do
+    "Visit " <> page_path
+  end
+
+  def name(%{event_name: name}) when is_binary(name) do
+    name
+  end
+
   defp update_leading_slash(changeset) do
     case get_field(changeset, :page_path) do
       "/" <> _ ->
@@ -101,24 +119,16 @@ defimpl Jason.Encoder, for: Plausible.Goal do
 
     value
     |> Map.put(:goal_type, goal_type)
-    |> Map.take([:id, :goal_type, :event_name, :page_path])
+    |> Map.take([:id, :goal_type, :event_name, :page_path, :currency])
     |> Map.put(:domain, domain)
-    |> Map.put(:name, to_string(value))
+    |> Map.put(:name, Plausible.Goal.name(value))
     |> Jason.Encode.map(opts)
   end
 end
 
 defimpl String.Chars, for: Plausible.Goal do
-  def to_string(%{page_path: page_path}) when is_binary(page_path) do
-    "Visit " <> page_path
-  end
-
-  def to_string(%{event_name: name, currency: nil}) when is_binary(name) do
-    name
-  end
-
-  def to_string(%{event_name: name, currency: currency}) when is_binary(name) do
-    name <> " (#{currency})"
+  def to_string(goal) do
+    Plausible.Goal.display_name(goal)
   end
 end
 
diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs
index 5496c2522587..eec81b15700c 100644
--- a/test/plausible_web/controllers/api/external_sites_controller_test.exs
+++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs
@@ -656,15 +656,15 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
                  "goals" => [
                    %{
                      "id" => goal3.id,
-                     "name" => to_string(goal3)
+                     "name" => Plausible.Goal.name(goal3)
                    },
                    %{
                      "id" => goal2.id,
-                     "name" => to_string(goal2)
+                     "name" => Plausible.Goal.name(goal2)
                    },
                    %{
                      "id" => goal1.id,
-                     "name" => to_string(goal1)
+                     "name" => Plausible.Goal.name(goal1)
                    }
                  ],
                  "meta" => %{

From d8398eb58bb786025c3445b3924f8814b5e0e8ee Mon Sep 17 00:00:00 2001
From: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>
Date: Tue, 16 Jul 2024 15:08:01 +0200
Subject: [PATCH 07/10] Extend goal list response but hide currency

---
 .../api/external_sites_controller.ex          |  5 ++-
 lib/plausible/goal/schema.ex                  | 31 +++++++------------
 .../api/external_sites_controller_test.exs    | 15 +++++++--
 3 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
index 5080903d61ae..3c1fe881326d 100644
--- a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
+++ b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
@@ -41,7 +41,10 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
           Enum.map(page.entries, fn goal ->
             %{
               id: goal.id,
-              name: Goal.name(goal)
+              name: Goal.name(goal),
+              goal_type: Goal.type(goal),
+              event_name: goal.event_name,
+              page_path: goal.page_path
             }
           end),
         meta: pagination_meta(page.metadata)
diff --git a/lib/plausible/goal/schema.ex b/lib/plausible/goal/schema.ex
index b24d16b917d8..364b94850124 100644
--- a/lib/plausible/goal/schema.ex
+++ b/lib/plausible/goal/schema.ex
@@ -47,15 +47,6 @@ defmodule Plausible.Goal do
     |> maybe_drop_currency()
   end
 
-  @spec display_name(t()) :: String.t()
-  def display_name(%{currency: nil} = goal) do
-    name(goal)
-  end
-
-  def display_name(%{currency: currency} = goal) do
-    name(goal) <> " (#{currency})"
-  end
-
   @spec name(t()) :: String.t()
   def name(%{page_path: page_path}) when is_binary(page_path) do
     "Visit " <> page_path
@@ -65,6 +56,10 @@ defmodule Plausible.Goal do
     name
   end
 
+  @spec type(t()) :: :event | :page
+  def type(%{event_name: event_name}) when is_binary(event_name), do: :event
+  def type(%{page_path: page_path}) when is_binary(page_path), do: :page
+
   defp update_leading_slash(changeset) do
     case get_field(changeset, :page_path) do
       "/" <> _ ->
@@ -109,17 +104,11 @@ end
 
 defimpl Jason.Encoder, for: Plausible.Goal do
   def encode(value, opts) do
-    goal_type =
-      cond do
-        value.event_name -> :event
-        value.page_path -> :page
-      end
-
     domain = value.site.domain
 
     value
-    |> Map.put(:goal_type, goal_type)
-    |> Map.take([:id, :goal_type, :event_name, :page_path, :currency])
+    |> Map.put(:goal_type, Plausible.Goal.type(value))
+    |> Map.take([:id, :goal_type, :event_name, :page_path])
     |> Map.put(:domain, domain)
     |> Map.put(:name, Plausible.Goal.name(value))
     |> Jason.Encode.map(opts)
@@ -127,8 +116,12 @@ defimpl Jason.Encoder, for: Plausible.Goal do
 end
 
 defimpl String.Chars, for: Plausible.Goal do
-  def to_string(goal) do
-    Plausible.Goal.display_name(goal)
+  def to_string(%{currency: nil} = goal) do
+    Plausible.Goal.name(goal)
+  end
+
+  def to_string(%{currency: currency} = goal) do
+    Plausible.Goal.name(goal) <> " (#{currency})"
   end
 end
 
diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs
index eec81b15700c..433facf859a5 100644
--- a/test/plausible_web/controllers/api/external_sites_controller_test.exs
+++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs
@@ -656,15 +656,24 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
                  "goals" => [
                    %{
                      "id" => goal3.id,
-                     "name" => Plausible.Goal.name(goal3)
+                     "name" => Plausible.Goal.name(goal3),
+                     "goal_type" => "event",
+                     "event_name" => "Purchase",
+                     "page_path" => nil
                    },
                    %{
                      "id" => goal2.id,
-                     "name" => Plausible.Goal.name(goal2)
+                     "name" => Plausible.Goal.name(goal2),
+                     "goal_type" => "event",
+                     "event_name" => "Signup",
+                     "page_path" => nil
                    },
                    %{
                      "id" => goal1.id,
-                     "name" => Plausible.Goal.name(goal1)
+                     "name" => Plausible.Goal.name(goal1),
+                     "goal_type" => "page",
+                     "event_name" => nil,
+                     "page_path" => "/login"
                    }
                  ],
                  "meta" => %{

From a092d27a13c9b441b01428a25a7939adbbafe667 Mon Sep 17 00:00:00 2001
From: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>
Date: Thu, 18 Jul 2024 09:09:53 +0200
Subject: [PATCH 08/10] Settle on `display_name` instead of `name`

---
 .../api/external_sites_controller.ex          |  2 +-
 lib/plausible/goal/schema.ex                  | 20 +++++++++----------
 .../api/external_sites_controller_test.exs    | 12 +++++------
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
index 3c1fe881326d..ccff1f9190d2 100644
--- a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
+++ b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
@@ -41,7 +41,7 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
           Enum.map(page.entries, fn goal ->
             %{
               id: goal.id,
-              name: Goal.name(goal),
+              display_name: Goal.display_name(goal),
               goal_type: Goal.type(goal),
               event_name: goal.event_name,
               page_path: goal.page_path
diff --git a/lib/plausible/goal/schema.ex b/lib/plausible/goal/schema.ex
index 364b94850124..d2d05867b07c 100644
--- a/lib/plausible/goal/schema.ex
+++ b/lib/plausible/goal/schema.ex
@@ -47,15 +47,19 @@ defmodule Plausible.Goal do
     |> maybe_drop_currency()
   end
 
-  @spec name(t()) :: String.t()
-  def name(%{page_path: page_path}) when is_binary(page_path) do
+  @spec display_name(t()) :: String.t()
+  def display_name(%{page_path: page_path}) when is_binary(page_path) do
     "Visit " <> page_path
   end
 
-  def name(%{event_name: name}) when is_binary(name) do
+  def display_name(%{event_name: name, currency: nil}) when is_binary(name) do
     name
   end
 
+  def display_name(%{event_name: name, currency: currency}) do
+    name <> " (#{currency})"
+  end
+
   @spec type(t()) :: :event | :page
   def type(%{event_name: event_name}) when is_binary(event_name), do: :event
   def type(%{page_path: page_path}) when is_binary(page_path), do: :page
@@ -110,18 +114,14 @@ defimpl Jason.Encoder, for: Plausible.Goal do
     |> Map.put(:goal_type, Plausible.Goal.type(value))
     |> Map.take([:id, :goal_type, :event_name, :page_path])
     |> Map.put(:domain, domain)
-    |> Map.put(:name, Plausible.Goal.name(value))
+    |> Map.put(:display_name, Plausible.Goal.display_name(value))
     |> Jason.Encode.map(opts)
   end
 end
 
 defimpl String.Chars, for: Plausible.Goal do
-  def to_string(%{currency: nil} = goal) do
-    Plausible.Goal.name(goal)
-  end
-
-  def to_string(%{currency: currency} = goal) do
-    Plausible.Goal.name(goal) <> " (#{currency})"
+  def to_string(goal) do
+    Plausible.Goal.display_name(goal)
   end
 end
 
diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs
index 433facf859a5..048a6d236a90 100644
--- a/test/plausible_web/controllers/api/external_sites_controller_test.exs
+++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs
@@ -265,7 +265,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
 
         res = json_response(conn, 200)
         assert res["goal_type"] == "event"
-        assert res["name"] == "Signup"
+        assert res["display_name"] == "Signup"
         assert res["event_name"] == "Signup"
         assert res["domain"] == site.domain
       end
@@ -281,7 +281,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
         res = json_response(conn, 200)
         assert res["goal_type"] == "page"
         assert res["page_path"] == "/signup"
-        assert res["name"] == "Visit /signup"
+        assert res["display_name"] == "Visit /signup"
         assert res["domain"] == site.domain
       end
 
@@ -301,7 +301,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
         res = json_response(conn, 200)
         assert res["goal_type"] == "event"
         assert res["event_name"] == "Signup"
-        assert res["name"] == "Signup"
+        assert res["display_name"] == "Signup"
         assert res["domain"] == new_domain
       end
 
@@ -656,21 +656,21 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
                  "goals" => [
                    %{
                      "id" => goal3.id,
-                     "name" => Plausible.Goal.name(goal3),
+                     "display_name" => "Purchase (USD)",
                      "goal_type" => "event",
                      "event_name" => "Purchase",
                      "page_path" => nil
                    },
                    %{
                      "id" => goal2.id,
-                     "name" => Plausible.Goal.name(goal2),
+                     "display_name" => "Signup",
                      "goal_type" => "event",
                      "event_name" => "Signup",
                      "page_path" => nil
                    },
                    %{
                      "id" => goal1.id,
-                     "name" => Plausible.Goal.name(goal1),
+                     "display_name" => "Visit /login",
                      "goal_type" => "page",
                      "event_name" => nil,
                      "page_path" => "/login"

From 8b91312789e13f7e1e2af2fef1bacdc95c7124ee Mon Sep 17 00:00:00 2001
From: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>
Date: Thu, 18 Jul 2024 10:59:53 +0200
Subject: [PATCH 09/10] Allow viewer members to get site details and list site
 goals

---
 .../api/external_sites_controller.ex          |  4 +-
 .../api/external_sites_controller_test.exs    | 49 +++++++++++++++++++
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
index ccff1f9190d2..2f0a5851bcf4 100644
--- a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
+++ b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex
@@ -30,7 +30,7 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
     user = conn.assigns.current_user
 
     with {:ok, site_id} <- expect_param_key(params, "site_id"),
-         {:ok, site} <- get_site(user, site_id, [:owner, :admin]) do
+         {:ok, site} <- get_site(user, site_id, [:owner, :admin, :viewer]) do
       page =
         site
         |> Plausible.Goals.for_site_query()
@@ -81,7 +81,7 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
   end
 
   def get_site(conn, %{"site_id" => site_id}) do
-    case get_site(conn.assigns.current_user, site_id, [:owner, :admin]) do
+    case get_site(conn.assigns.current_user, site_id, [:owner, :admin, :viewer]) do
       {:ok, site} ->
         json(conn, %{
           domain: site.domain,
diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs
index 048a6d236a90..e11c6d806096 100644
--- a/test/plausible_web/controllers/api/external_sites_controller_test.exs
+++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs
@@ -508,6 +508,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
 
       test "returns sites when present", %{conn: conn, user: user} do
         [site1, site2] = insert_list(2, :site, members: [user])
+        _unrelated_site = insert(:site)
 
         conn = get(conn, "/api/v1/sites")
 
@@ -524,6 +525,21 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
                }
       end
 
+      test "returns sites where user is only a viewer", %{conn: conn, user: user} do
+        %{domain: owned_site_domain} = insert(:site, members: [user])
+        other_site = %{domain: other_site_domain} = insert(:site)
+        insert(:site_membership, site: other_site, user: user, role: :viewer)
+
+        conn = get(conn, "/api/v1/sites")
+
+        assert %{
+                 "sites" => [
+                   %{"domain" => ^other_site_domain},
+                   %{"domain" => ^owned_site_domain}
+                 ]
+               } = json_response(conn, 200)
+      end
+
       test "handles pagination correctly", %{conn: conn, user: user} do
         [
           %{domain: site1_domain},
@@ -627,6 +643,14 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
 
         assert json_response(conn, 404) == %{"error" => "Site could not be found"}
       end
+
+      test "is 404 when user is not a member of the site", %{conn: conn} do
+        site = insert(:site)
+
+        conn = get(conn, "/api/v1/sites/" <> site.domain)
+
+        assert json_response(conn, 404) == %{"error" => "Site could not be found"}
+      end
     end
 
     describe "GET /api/v1/goals" do
@@ -684,6 +708,21 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
                }
       end
 
+      test "returns goals for site where user is viewer", %{conn: conn, user: user, site: site} do
+        Repo.update_all(
+          from(sm in Plausible.Site.Membership,
+            where: sm.site_id == ^site.id and sm.user_id == ^user.id
+          ),
+          set: [role: :viewer]
+        )
+
+        %{id: goal_id} = insert(:goal, %{site: site, event_name: "Signup"})
+
+        conn = get(conn, "/api/v1/sites/goals?site_id=" <> site.domain)
+
+        assert %{"goals" => [%{"id" => ^goal_id}]} = json_response(conn, 200)
+      end
+
       test "handles pagination correctly", %{conn: conn, site: site} do
         %{id: goal1_id} = insert(:goal, %{site: site, page_path: "/login"})
         %{id: goal2_id} = insert(:goal, %{site: site, event_name: "Signup"})
@@ -747,6 +786,16 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
                  "error" => "Site could not be found"
                }
       end
+
+      test "returns error when user is not a member of the site", %{conn: conn} do
+        site = insert(:site)
+
+        conn = get(conn, "/api/v1/sites/goals?site_id=" <> site.domain)
+
+        assert json_response(conn, 404) == %{
+                 "error" => "Site could not be found"
+               }
+      end
     end
 
     describe "PUT /api/v1/sites/:site_id" do

From a59186ea9d2bba0abbe881eebb53158638b4cdfe Mon Sep 17 00:00:00 2001
From: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>
Date: Fri, 19 Jul 2024 10:00:36 +0200
Subject: [PATCH 10/10] Don't include currency in goal's display name

---
 lib/plausible/goal/schema.ex                    | 10 +++-------
 lib/plausible_web/live/goal_settings/list.ex    | 17 +++++++++++++----
 .../api/external_sites_controller_test.exs      |  2 +-
 .../plugins/api/controllers/goals_test.exs      |  4 ++--
 4 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/lib/plausible/goal/schema.ex b/lib/plausible/goal/schema.ex
index d2d05867b07c..a0719a00d263 100644
--- a/lib/plausible/goal/schema.ex
+++ b/lib/plausible/goal/schema.ex
@@ -48,18 +48,14 @@ defmodule Plausible.Goal do
   end
 
   @spec display_name(t()) :: String.t()
-  def display_name(%{page_path: page_path}) when is_binary(page_path) do
-    "Visit " <> page_path
+  def display_name(%{page_path: path}) when is_binary(path) do
+    "Visit " <> path
   end
 
-  def display_name(%{event_name: name, currency: nil}) when is_binary(name) do
+  def display_name(%{event_name: name}) when is_binary(name) do
     name
   end
 
-  def display_name(%{event_name: name, currency: currency}) do
-    name <> " (#{currency})"
-  end
-
   @spec type(t()) :: :event | :page
   def type(%{event_name: event_name}) when is_binary(event_name), do: :event
   def type(%{page_path: page_path}) when is_binary(page_path), do: :page
diff --git a/lib/plausible_web/live/goal_settings/list.ex b/lib/plausible_web/live/goal_settings/list.ex
index 8bbb8e723cfd..79bee83d0a91 100644
--- a/lib/plausible_web/live/goal_settings/list.ex
+++ b/lib/plausible_web/live/goal_settings/list.ex
@@ -14,7 +14,8 @@ defmodule PlausibleWeb.Live.GoalSettings.List do
 
   def render(assigns) do
     revenue_goals_enabled? = Plausible.Billing.Feature.RevenueGoals.enabled?(assigns.site)
-    assigns = assign(assigns, :revenue_goals_enabled?, revenue_goals_enabled?)
+    goals = Enum.map(assigns.goals, &{goal_label(&1), &1})
+    assigns = assign(assigns, goals: goals, revenue_goals_enabled?: revenue_goals_enabled?)
 
     ~H"""
     <div>
@@ -55,7 +56,7 @@ defmodule PlausibleWeb.Live.GoalSettings.List do
       </div>
       <%= if Enum.count(@goals) > 0 do %>
         <div class="mt-12">
-          <%= for goal <- @goals do %>
+          <%= for {goal_label, goal} <- @goals do %>
             <div class="border-b border-gray-300 dark:border-gray-500 py-3 flex justify-between">
               <span class="text-sm font-medium text-gray-900 dark:text-gray-100 w-3/4">
                 <div class="flex">
@@ -63,10 +64,10 @@ defmodule PlausibleWeb.Live.GoalSettings.List do
                     <%= if not @revenue_goals_enabled? && goal.currency do %>
                       <div class="text-gray-600 flex items-center">
                         <Heroicons.lock_closed class="w-4 h-4 mr-1 inline" />
-                        <span><%= goal %></span>
+                        <span><%= goal_label %></span>
                       </div>
                     <% else %>
-                      <%= goal %>
+                      <%= goal_label %>
                     <% end %>
                     <span class="text-sm text-gray-400 block mt-1 font-normal">
                       <span :if={goal.page_path}>Pageview</span>
@@ -116,6 +117,14 @@ defmodule PlausibleWeb.Live.GoalSettings.List do
     """
   end
 
+  defp goal_label(%{currency: currency} = goal) when not is_nil(currency) do
+    to_string(goal) <> " (#{currency})"
+  end
+
+  defp goal_label(goal) do
+    to_string(goal)
+  end
+
   defp delete_confirmation_text(goal) do
     if Enum.empty?(goal.funnels) do
       """
diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs
index e11c6d806096..c938738c6ba5 100644
--- a/test/plausible_web/controllers/api/external_sites_controller_test.exs
+++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs
@@ -680,7 +680,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
                  "goals" => [
                    %{
                      "id" => goal3.id,
-                     "display_name" => "Purchase (USD)",
+                     "display_name" => "Purchase",
                      "goal_type" => "event",
                      "event_name" => "Purchase",
                      "page_path" => nil
diff --git a/test/plausible_web/plugins/api/controllers/goals_test.exs b/test/plausible_web/plugins/api/controllers/goals_test.exs
index ca9a9f08ca63..ab55f474af9f 100644
--- a/test/plausible_web/plugins/api/controllers/goals_test.exs
+++ b/test/plausible_web/plugins/api/controllers/goals_test.exs
@@ -496,7 +496,7 @@ defmodule PlausibleWeb.Plugins.API.Controllers.GoalsTest do
 
       assert resp.goal.id == goal.id
       assert resp.goal_type == "Goal.Revenue"
-      assert resp.goal.display_name == "Purchase (EUR)"
+      assert resp.goal.display_name == "Purchase"
     end
 
     test "retrieves pageview goal by ID", %{conn: conn, site: site, token: token} do
@@ -582,7 +582,7 @@ defmodule PlausibleWeb.Plugins.API.Controllers.GoalsTest do
       assert checkout.goal.id == g3.id
       assert checkout.goal_type == "Goal.Pageview"
 
-      assert purchase.goal.display_name == "Purchase (EUR)"
+      assert purchase.goal.display_name == "Purchase"
       assert purchase.goal.currency == "EUR"
       assert purchase.goal.event_name == "Purchase"
       assert purchase.goal.id == g2.id