Remove superfluous feature, add behaviour that diverges from segments

Author: apata

assets/js/dashboard/annotations/routeless-annotations-modals.tsx

@@ -117,7 +117,7 @@ export const RoutelessAnnotationModals = () => {
       {modal?.type === 'update-annotation' && (
         <UpdateAnnotationModal
           user={user}
-          siteAnnotationsAvailable={site.siteSegmentsAvailable}
+          siteAnnotationsAvailable={site.siteAnnotationsAvailable}
           annotation={modal.annotation}
           notePlaceholder={''}
           onClose={() => {
@@ -139,7 +139,7 @@ export const RoutelessAnnotationModals = () => {
       {modal?.type === 'create-annotation' && (
         <CreateAnnotationModal
           user={user}
-          siteAnnotationsAvailable={site.siteSegmentsAvailable}
+          siteAnnotationsAvailable={site.siteAnnotationsAvailable}
           notePlaceholder={modal.annotation.note}
           initialType={modal.annotation.type}
           initialDatetime={modal.annotation.datetime}

assets/js/dashboard/site-context.test.tsx

@@ -17,6 +17,7 @@ describe('parseSiteFromDataset', () => {
       data-funnels-available="true"
       data-exploration-available="false"
       data-site-segments-available="true"
+      data-site-annotations-available="true"
       data-props-available="true"
       data-revenue-goals='[{"currency":"USD","display_name":"Purchase"}]'
       data-funnels='[{"id":1,"name":"From homepage to login","steps_count":3}]'
@@ -46,6 +47,7 @@ describe('parseSiteFromDataset', () => {
     propsAvailable: true,
     explorationAvailable: false,
     siteSegmentsAvailable: true,
+    siteAnnotationsAvailable: true,
     revenueGoals: [{ currency: 'USD', display_name: 'Purchase' }],
     funnels: [{ id: 1, name: 'From homepage to login', steps_count: 3 }],
     hasProps: true,

assets/js/dashboard/site-context.tsx

@@ -10,6 +10,7 @@ export function parseSiteFromDataset(dataset: DOMStringMap): PlausibleSite {
     propsAvailable: dataset.propsAvailable === 'true',
     explorationAvailable: dataset.explorationAvailable === 'true',
     siteSegmentsAvailable: dataset.siteSegmentsAvailable === 'true',
+    siteAnnotationsAvailable: dataset.siteAnnotationsAvailable === 'true',
     conversionsOptedOut: dataset.conversionsOptedOut === 'true',
     funnelsOptedOut: dataset.funnelsOptedOut === 'true',
     propsOptedOut: dataset.propsOptedOut === 'true',
@@ -39,6 +40,7 @@ export const siteContextDefaultValue = {
   explorationAvailable: false,
   propsAvailable: false,
   siteSegmentsAvailable: false,
+  siteAnnotationsAvailable: false,
   conversionsOptedOut: false,
   funnelsOptedOut: false,
   propsOptedOut: false,

assets/test-utils/app-context-providers.tsx

@@ -34,6 +34,7 @@ export const DEFAULT_SITE: PlausibleSite = {
   explorationAvailable: false,
   propsAvailable: false,
   siteSegmentsAvailable: false,
+  siteAnnotationsAvailable: false,
   conversionsOptedOut: false,
   funnelsOptedOut: false,
   propsOptedOut: false,

lib/plausible/annotations/annotations.ex

@@ -17,7 +17,9 @@ defmodule Plausible.Annotations do
 
   @max_annotations 500
 
-  def get_all_for_site(%Plausible.Site{} = site, site_role) do
+  @spec get_all_for_site(Plausible.Site.t(), atom(), pos_integer()) ::
+          {:error, :not_enough_permissions} | {:ok, list(Annotation.t())}
+  def get_all_for_site(%Plausible.Site{} = site, site_role, user_id) do
     fields = [:id, :note, :type, :datetime, :granularity, :inserted_at, :updated_at]
 
     cond do
@@ -27,21 +29,25 @@ defmodule Plausible.Annotations do
             from(annotation in Annotation,
               select: ^fields,
               where: annotation.site_id == ^site.id,
+              where: annotation.type == :site,
               order_by: [desc: annotation.updated_at, desc: annotation.id]
             )
           )
 
         {:ok, Enum.map(annotations, &localize_annotation(&1, site.timezone))}
 
-      site_role in @roles_with_personal_annotations or
-          site_role in @roles_with_maybe_site_annotations ->
+      site_role in roles_with_personal_annotations() or
+          site_role in roles_with_maybe_site_annotations() ->
         fields = fields ++ [:owner_id]
 
         annotations =
           Repo.all(
             from(annotation in Annotation,
               select: ^fields,
               where: annotation.site_id == ^site.id,
+              where:
+                annotation.type == :site or
+                  (annotation.type == :personal and annotation.owner_id == ^user_id),
               order_by: [desc: annotation.updated_at, desc: annotation.id],
               preload: [:owner]
             )
@@ -90,7 +96,8 @@ defmodule Plausible.Annotations do
              %Annotation{},
              Map.merge(params, %{"site_id" => site.id, "owner_id" => user_id})
            ) do
-      {:ok, changeset |> Repo.insert!() |> Repo.preload(:owner) |> localize_annotation(site.timezone)}
+      {:ok,
+       changeset |> Repo.insert!() |> Repo.preload(:owner) |> localize_annotation(site.timezone)}
     else
       %{valid?: false, errors: errors} ->
         {:error, {:invalid_annotation, errors}}
@@ -124,7 +131,8 @@ defmodule Plausible.Annotations do
            ) do
       Repo.update!(changeset)
 
-      {:ok, Repo.reload!(annotation) |> Repo.preload(:owner) |> localize_annotation(site.timezone)}
+      {:ok,
+       Repo.reload!(annotation) |> Repo.preload(:owner) |> localize_annotation(site.timezone)}
     else
       %{valid?: false, errors: errors} ->
         {:error, {:invalid_annotation, errors}}
@@ -223,7 +231,9 @@ defmodule Plausible.Annotations do
       from(annotation in Annotation,
         where: annotation.site_id == ^site_id,
         where: annotation.id == ^annotation_id,
-        where: annotation.type == :site or annotation.owner_id == ^user_id,
+        where:
+          annotation.type == :site or
+            (annotation.type == :personal and annotation.owner_id == ^user_id),
         preload: [:owner]
       )
 
@@ -282,7 +292,7 @@ defmodule Plausible.Annotations do
   def roles_with_maybe_site_annotations(), do: @roles_with_maybe_site_annotations
 
   def site_annotations_available?(%Plausible.Site{} = site),
-    do: Plausible.Billing.Feature.SiteAnnotations.check_availability(site.team) == :ok
+    do: Plausible.Billing.Feature.SiteSegments.check_availability(site.team) == :ok
 
   @doc """
   iex> serialize_first_error([{"name", {"should be at most %{count} byte(s)", [count: 255]}}])

lib/plausible/billing/feature.ex

@@ -69,7 +69,6 @@ defmodule Plausible.Billing.Feature do
     Plausible.Billing.Feature.Goals,
     Plausible.Billing.Feature.RevenueGoals,
     Plausible.Billing.Feature.SiteSegments,
-    Plausible.Billing.Feature.SiteAnnotations,
     Plausible.Billing.Feature.SitesAPI,
     Plausible.Billing.Feature.StatsAPI,
     Plausible.Billing.Feature.SSO,
@@ -204,13 +203,6 @@ defmodule Plausible.Billing.Feature.SiteSegments do
     display_name: "Shared Segments"
 end
 
-defmodule Plausible.Billing.Feature.SiteAnnotations do
-  @moduledoc false
-  use Plausible.Billing.Feature,
-    name: :site_annotations,
-    display_name: "Shared Annotations"
-end
-
 defmodule Plausible.Billing.Feature.StatsAPI do
   use Plausible
 

lib/plausible_web/controllers/api/internal/annotations_controller.ex

@@ -10,14 +10,21 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsController do
 
   def index(
         %Plug.Conn{
-          assigns: %{
-            site: site,
-            site_role: site_role
-          }
+          assigns:
+            %{
+              site: site,
+              site_role: site_role
+            } = assigns
         } = conn,
         %{} = _params
       ) do
-    case Annotations.get_all_for_site(site, site_role) do
+    user_id =
+      case assigns[:current_user] do
+        %{id: id} -> id
+        nil -> nil
+      end
+
+    case Annotations.get_all_for_site(site, site_role, user_id) do
       {:ok, result} -> json(conn, result)
       {:error, :not_enough_permissions} -> json(conn, "Not enough permissions to get annotations")
     end

lib/plausible_web/templates/stats/stats.html.heex

@@ -26,8 +26,9 @@
     data-props-available={
       to_string(Plausible.Billing.Feature.Props.check_availability(@site.team) == :ok)
     }
-    data-site-segments-available={
-      to_string(Plausible.Billing.Feature.SiteSegments.check_availability(@site.team) == :ok)
+    data-site-segments-available={to_string(Plausible.Segments.site_segments_available?(@site))}
+    data-site-annotations-available={
+      to_string(Plausible.Annotations.site_annotations_available?(@site))
     }
     data-revenue-goals={Jason.encode!(@revenue_goals)}
     data-funnels={Jason.encode!(@funnels)}

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

@@ -2,6 +2,73 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do
   use PlausibleWeb.ConnCase, async: true
   use Plausible.Repo
 
+  describe "GET /api/:domain/annotations - index" do
+    setup [:create_user, :log_in]
+
+    test "public role sees only site annotations, not personal ones", %{conn: conn} do
+      public_site = new_site(public: true)
+      site_owner = new_user()
+      insert(:annotation, site: public_site, owner: site_owner, type: :site, note: "site note")
+      insert(:annotation, site: public_site, owner: site_owner, type: :personal, note: "private")
+
+      conn = get(conn, "/api/#{public_site.domain}/annotations")
+
+      assert [result] = json_response(conn, 200)
+      assert result["type"] == "site"
+      assert result["note"] == "site note"
+    end
+
+    test "public role response has null owner info", %{conn: conn} do
+      public_site = new_site(public: true)
+      site_owner = new_user()
+      insert(:annotation, site: public_site, owner: site_owner, type: :site, note: "deploy")
+
+      conn = get(conn, "/api/#{public_site.domain}/annotations")
+
+      assert [result] = json_response(conn, 200)
+      assert result["owner_id"] == nil
+      assert result["owner_name"] == nil
+    end
+
+    test "authenticated viewer sees their own personal annotations and all site annotations",
+         %{conn: conn, user: user} do
+      site = new_site()
+      other_user = new_user()
+      add_guest(site, user: user, role: :viewer)
+      insert(:annotation, site: site, owner: user, type: :personal, note: "mine")
+      insert(:annotation, site: site, owner: other_user, type: :personal, note: "not mine")
+      insert(:annotation, site: site, owner: other_user, type: :site, note: "shared")
+
+      conn = get(conn, "/api/#{site.domain}/annotations")
+
+      assert results = json_response(conn, 200)
+      assert length(results) == 2
+      notes = Enum.map(results, & &1["note"])
+      assert "mine" in notes
+      assert "shared" in notes
+      refute "not mine" in notes
+    end
+
+    test "authenticated owner response includes owner info", %{conn: conn, user: user} do
+      site = new_site(owner: user)
+      insert(:annotation, site: site, owner: user, type: :site, note: "deploy")
+
+      conn = get(conn, "/api/#{site.domain}/annotations")
+
+      assert [result] = json_response(conn, 200)
+      assert result["owner_id"] == user.id
+      assert result["owner_name"] == user.name
+    end
+
+    test "private site returns 404 for non-member", %{conn: conn} do
+      private_site = new_site()
+
+      conn = get(conn, "/api/#{private_site.domain}/annotations")
+
+      assert json_response(conn, 404)
+    end
+  end
+
   describe "POST /api/:domain/annotations - datetime coercion" do
     setup [:create_user, :log_in, :create_site]