From 49a3f33fa088465a6e55ba4a9da5e4fd5170d6a2 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 17 Jun 2026 16:15:30 +0300 Subject: [PATCH 01/23] Annotations BE --- lib/plausible/annotations/annotation.ex | 149 ++++ lib/plausible/annotations/annotations.ex | 378 +++++++++ lib/plausible/helpers/changeset.ex | 21 + lib/plausible/segments/segments.ex | 15 - lib/plausible/teams/memberships.ex | 1 + lib/plausible/teams/memberships/leave.ex | 5 + lib/plausible/teams/memberships/remove.ex | 5 + .../api/internal/annotations_controller.ex | 187 +++++ .../api/internal/segments_controller.ex | 5 +- lib/plausible_web/router.ex | 8 + .../templates/stats/stats.html.heex | 5 +- .../plausible/annotations/annotation_test.exs | 212 +++++ test/plausible/billing/feature_test.exs | 1 - .../annotations_controller_test.exs | 780 ++++++++++++++++++ test/support/factory.ex | 9 + 15 files changed, 1761 insertions(+), 20 deletions(-) create mode 100644 lib/plausible/annotations/annotation.ex create mode 100644 lib/plausible/annotations/annotations.ex create mode 100644 lib/plausible_web/controllers/api/internal/annotations_controller.ex create mode 100644 test/plausible/annotations/annotation_test.exs create mode 100644 test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs diff --git a/lib/plausible/annotations/annotation.ex b/lib/plausible/annotations/annotation.ex new file mode 100644 index 000000000000..dc75d0ca1627 --- /dev/null +++ b/lib/plausible/annotations/annotation.ex @@ -0,0 +1,149 @@ +defmodule Plausible.Annotations.Annotation do + @moduledoc """ + Schema for annotations. Annotations are notes attached to a point on the graph. + + Annotations can have two granularities, date or minute. + + To create date granularity annotations, only the date part must be specified for `datetime` field. + It's interpreted to be for that date, no matter what happens with the site timezone. + + Examples: + - `{"granularity": "date", "datetime": "2026-06-30", ...}`. + + To create minute granularity annotations, the full datetime needs to be specified for `datetime` field. + It can be set in local time, as a naive datetime string ("2026-06-29 10:00:00"), in which case it's + interpreted as that datetime in the site's timezone and stored as that particular moment in UTC. + Alternatively, it can be sent with TZ information ("2026-05-31T12:00:00Z", "2026-05-31T10:00:00-02:00"), + in which case it's interpreted to be that particular moment and stored as that in UTC. + + Examples: + - `{"granularity": "minute", "datetime": "2026-06-29 10:00:00", ...}` + - `{"granularity": "minute", "datetime": "2026-06-29T12:00:00Z", ...}` + """ + + use Plausible + use Ecto.Schema + import Ecto.Changeset + + @annotation_types [:personal, :site] + @annotation_granularities [:date, :minute] + + @type t() :: %__MODULE__{} + + schema "annotations" do + field :note, :string + field :type, Ecto.Enum, values: @annotation_types + field :datetime, :utc_datetime + field :granularity, Ecto.Enum, values: @annotation_granularities + + # owner ID can be null (aka note is dangling) when the original owner is deassociated from the site + # the note is dangling until another user edits it: the editor becomes the new owner + belongs_to :owner, Plausible.Auth.User, foreign_key: :owner_id + belongs_to :site, Plausible.Site + + timestamps() + end + + def changeset(annotation, attrs) do + attrs = stringify_keys(attrs) + {attrs, invalid_datetime_for_granularity?} = coerce_datetime(attrs) + + annotation + |> cast(attrs, [:note, :site_id, :type, :owner_id, :datetime, :granularity]) + |> validate_required([:note, :site_id, :type, :owner_id, :datetime, :granularity]) + |> validate_length(:note, count: :bytes, min: 1, max: 255) + |> foreign_key_constraint(:site_id) + |> foreign_key_constraint(:owner_id) + |> validate_datetime_supplied_on_granularity_change() + |> maybe_add_datetime_error(invalid_datetime_for_granularity?) + end + + defp stringify_keys(%{} = params) do + Map.new(params, fn + {k, v} when is_atom(k) -> {Atom.to_string(k), v} + {k, v} when is_binary(k) -> {k, v} + end) + end + + defp coerce_datetime(attrs) do + granularity = normalize_granularity(attrs["granularity"]) + + case coerce_for_granularity(granularity, attrs["datetime"]) do + {:ok, %DateTime{} = utc_dt} -> {Map.put(attrs, "datetime", utc_dt), false} + :skip -> {attrs, false} + :invalid -> {Map.delete(attrs, "datetime"), true} + end + end + + defp normalize_granularity(:date), do: :date + defp normalize_granularity("date"), do: :date + defp normalize_granularity(:minute), do: :minute + defp normalize_granularity("minute"), do: :minute + defp normalize_granularity(other), do: other + + # nil datetime will be caught by validate_required step + defp coerce_for_granularity(_granularity, nil), do: :skip + + defp coerce_for_granularity(:date, %Date{} = date), + do: {:ok, serialize_date_granularity_datetime(date)} + + defp coerce_for_granularity(:date, <<_::binary-size(10)>> = str) do + case Date.from_iso8601(str) do + {:ok, date} -> {:ok, serialize_date_granularity_datetime(date)} + _ -> :invalid + end + end + + defp coerce_for_granularity(:minute, %DateTime{} = dt), + do: {:ok, DateTime.shift_zone!(dt, "Etc/UTC")} + + defp coerce_for_granularity(:minute, str) when is_binary(str) do + case DateTime.from_iso8601(str) do + {:ok, dt, _offset} -> {:ok, DateTime.shift_zone!(dt, "Etc/UTC")} + _ -> :invalid + end + end + + defp coerce_for_granularity(granularity, _datetime) when granularity in [:date, :minute], + do: :invalid + + defp coerce_for_granularity(_granularity, _datetime), do: :skip + + defp maybe_add_datetime_error(changeset, false), do: changeset + + defp maybe_add_datetime_error(changeset, true), + do: add_error(changeset, :datetime, "is invalid for granularity") + + defp validate_datetime_supplied_on_granularity_change(changeset) do + with {:ok, _new_granularity} <- fetch_change(changeset, :granularity), + false <- is_nil(changeset.data.granularity), + :error <- fetch_change(changeset, :datetime) do + add_error(changeset, :datetime, "must be supplied when granularity changes") + else + _ -> changeset + end + end + + def serialize_date_granularity_datetime(%Date{} = date), + do: DateTime.new!(date, ~T[00:00:00], "Etc/UTC") + + def parse_date_granularity_datetime(%DateTime{} = datetime), + do: DateTime.to_date(datetime) +end + +defimpl Jason.Encoder, for: Plausible.Annotations.Annotation do + def encode(%Plausible.Annotations.Annotation{} = annotation, opts) do + %{ + id: annotation.id, + note: annotation.note, + type: annotation.type, + datetime: annotation.datetime, + granularity: annotation.granularity, + owner_id: annotation.owner_id, + owner_name: if(is_nil(annotation.owner_id), do: nil, else: annotation.owner.name), + inserted_at: annotation.inserted_at, + updated_at: annotation.updated_at + } + |> Jason.Encode.map(opts) + end +end diff --git a/lib/plausible/annotations/annotations.ex b/lib/plausible/annotations/annotations.ex new file mode 100644 index 000000000000..72943249b4b6 --- /dev/null +++ b/lib/plausible/annotations/annotations.ex @@ -0,0 +1,378 @@ +defmodule Plausible.Annotations do + @moduledoc """ + Module for accessing Annotations. + """ + alias Plausible.Annotations.Annotation + alias Plausible.Repo + import Ecto.Query + + @roles_with_personal_annotations [:billing, :viewer, :editor, :admin, :owner, :super_admin] + @roles_with_maybe_site_annotations [:editor, :admin, :owner, :super_admin] + + @type error_not_enough_permissions() :: {:error, :not_enough_permissions} + @type error_annotation_not_found() :: {:error, :annotation_not_found} + @type error_annotation_limit_reached() :: {:error, :annotations_limit_reached} + @type error_invalid_annotation() :: {:error, {:invalid_annotation, Keyword.t()}} + @type unknown_error() :: {:error, any()} + + @max_annotations 500 + + @spec get_all_for_site(Plausible.Site.t(), atom(), pos_integer() | nil, QueryPeriod.t()) :: + {:error, :not_enough_permissions} | {:ok, list(Annotation.t())} + def get_all_for_site( + %Plausible.Site{} = site, + site_role, + user_id, + %Plausible.Stats.DateTimeRange{} = range_in_site_tz + ) do + # Minute granularity annotations are stored for the particular UTC moment they're for, + # so they must be in the range of the UTC query period. + minute_granularity_range = + range_in_site_tz |> Plausible.Stats.DateTimeRange.to_timezone("Etc/UTC") + + # Date granularity annotations are stored for the UTC midnight of the date they're for, + # so the range for querying these must reflect that. + [date_granularity_range_first, date_granularity_range_last] = + [range_in_site_tz.first, range_in_site_tz.last] + |> Enum.map(fn utc_datetime -> + utc_datetime |> DateTime.to_date() |> Annotation.serialize_date_granularity_datetime() + end) + + fields = [:id, :note, :type, :datetime, :granularity, :inserted_at, :updated_at] + + in_range_clause = + dynamic( + [annotation], + (annotation.granularity == :minute and + annotation.datetime >= ^minute_granularity_range.first and + annotation.datetime <= ^minute_granularity_range.last) or + (annotation.granularity == :date and + annotation.datetime >= + ^date_granularity_range_first and + annotation.datetime <= + ^date_granularity_range_last) + ) + + cond do + site_role in [:public] -> + annotations = + Repo.all( + from(annotation in Annotation, + select: ^fields, + where: annotation.site_id == ^site.id, + where: annotation.type == :site, + where: ^in_range_clause, + 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() -> + 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), + where: ^in_range_clause, + order_by: [desc: annotation.updated_at, desc: annotation.id], + preload: [:owner] + ) + ) + + {:ok, Enum.map(annotations, &localize_annotation(&1, site.timezone))} + + true -> + {:error, :not_enough_permissions} + end + end + + @spec get_one(pos_integer(), Plausible.Site.t(), atom(), pos_integer() | nil) :: + {:ok, Annotation.t()} + | error_not_enough_permissions() + | error_annotation_not_found() + def get_one(user_id, site, site_role, annotation_id) do + if site_role in roles_with_personal_annotations() do + case do_get_one(user_id, site.id, annotation_id) do + %Annotation{} = annotation -> {:ok, annotation} + nil -> {:error, :annotation_not_found} + end + else + {:error, :not_enough_permissions} + end + end + + @spec insert_one(pos_integer(), Plausible.Site.t(), atom(), map()) :: + {:ok, Annotation.t()} + | error_not_enough_permissions() + | error_invalid_annotation() + | error_annotation_limit_reached() + | unknown_error() + + def insert_one( + user_id, + %Plausible.Site{} = site, + site_role, + %{} = params + ) do + params = maybe_coerce_naive_datetime(params, site.timezone) + + with :ok <- can_insert_one?(site, site_role, params), + %{valid?: true} = changeset <- + Annotation.changeset( + %Annotation{}, + Map.merge(params, %{"site_id" => site.id, "owner_id" => user_id}) + ) do + {:ok, + changeset |> Repo.insert!() |> Repo.preload(:owner) |> localize_annotation(site.timezone)} + else + %{valid?: false, errors: errors} -> + {:error, {:invalid_annotation, errors}} + + {:error, _type} = error -> + error + end + end + + @spec update_one(pos_integer(), Plausible.Site.t(), atom(), pos_integer(), map()) :: + {:ok, Annotation.t()} + | error_not_enough_permissions() + | error_invalid_annotation() + | unknown_error() + + def update_one( + user_id, + %Plausible.Site{} = site, + site_role, + annotation_id, + %{} = params + ) do + params = maybe_coerce_naive_datetime(params, site.timezone) + + with {:ok, annotation} <- get_one(user_id, site, site_role, annotation_id), + :ok <- can_update_one?(site, site_role, params, annotation.type), + %{valid?: true} = changeset <- + Annotation.changeset( + annotation, + Map.merge(params, %{"owner_id" => user_id}) + ) do + Repo.update!(changeset) + + {:ok, + Repo.reload!(annotation) |> Repo.preload(:owner) |> localize_annotation(site.timezone)} + else + %{valid?: false, errors: errors} -> + {:error, {:invalid_annotation, errors}} + + {:error, _type} = error -> + error + end + end + + def after_user_removed_from_site(site, user) do + Repo.delete_all( + from(annotation in Annotation, + where: annotation.site_id == ^site.id, + where: annotation.owner_id == ^user.id, + where: annotation.type == :personal + ) + ) + + Repo.update_all( + from(annotation in Annotation, + where: annotation.site_id == ^site.id, + where: annotation.owner_id == ^user.id, + where: annotation.type == :site, + update: [set: [owner_id: nil]] + ), + [] + ) + end + + def after_user_removed_from_team(team, user) do + team_sites_q = + from( + site in Plausible.Site, + where: site.team_id == ^team.id, + where: parent_as(:annotation).site_id == site.id + ) + + Repo.delete_all( + from(annotation in Annotation, + as: :annotation, + where: annotation.owner_id == ^user.id, + where: annotation.type == :personal, + where: exists(team_sites_q) + ) + ) + + Repo.update_all( + from(annotation in Annotation, + as: :annotation, + where: annotation.owner_id == ^user.id, + where: annotation.type == :site, + where: exists(team_sites_q), + update: [set: [owner_id: nil]] + ), + [] + ) + end + + def user_removed(user) do + Repo.delete_all( + from(annotation in Annotation, + as: :annotation, + where: annotation.owner_id == ^user.id, + where: annotation.type == :personal + ) + ) + + # Site annotations are set to owner=null via ON DELETE SET NULL + end + + def delete_one(user_id, %Plausible.Site{} = site, site_role, annotation_id) do + with {:ok, annotation} <- get_one(user_id, site, site_role, annotation_id) do + cond do + annotation.type == :site and site_role in roles_with_maybe_site_annotations() -> + {:ok, do_delete_one(annotation) |> localize_annotation(site.timezone)} + + annotation.type == :personal and site_role in roles_with_personal_annotations() -> + {:ok, do_delete_one(annotation) |> localize_annotation(site.timezone)} + + true -> + {:error, :not_enough_permissions} + end + end + end + + @spec do_get_one(pos_integer(), pos_integer(), pos_integer() | nil) :: + Annotation.t() | nil + defp do_get_one(user_id, site_id, annotation_id) + + defp do_get_one(_user_id, _site_id, nil) do + nil + end + + defp do_get_one(user_id, site_id, annotation_id) do + query = + from(annotation in Annotation, + where: annotation.site_id == ^site_id, + where: annotation.id == ^annotation_id, + where: + annotation.type == :site or + (annotation.type == :personal and annotation.owner_id == ^user_id), + preload: [:owner] + ) + + Repo.one(query) + end + + defp do_delete_one(annotation) do + Repo.delete!(annotation) + annotation + end + + defp can_update_one?(%Plausible.Site{} = site, site_role, params, existing_annotation_type) do + updating_to_site_annotation? = params["type"] == "site" + + cond do + (existing_annotation_type == :site or + updating_to_site_annotation?) and site_role in roles_with_maybe_site_annotations() and + site_annotations_available?(site) -> + :ok + + existing_annotation_type == :personal and not updating_to_site_annotation? and + site_role in roles_with_personal_annotations() -> + :ok + + true -> + {:error, :not_enough_permissions} + end + end + + defp can_insert_one?(%Plausible.Site{} = site, site_role, params) do + cond do + count_annotations(site.id) >= @max_annotations -> + {:error, :annotations_limit_reached} + + params["type"] == "site" and site_role in roles_with_maybe_site_annotations() and + site_annotations_available?(site) -> + :ok + + params["type"] == "personal" and + site_role in roles_with_personal_annotations() -> + :ok + + true -> + {:error, :not_enough_permissions} + end + end + + defp count_annotations(site_id) do + from(annotation in Annotation, + where: annotation.site_id == ^site_id + ) + |> Repo.aggregate(:count, :id) + end + + def roles_with_personal_annotations(), do: @roles_with_personal_annotations + def roles_with_maybe_site_annotations(), do: @roles_with_maybe_site_annotations + + def site_annotations_available?(%Plausible.Site{} = site), + # this feature is bundled with SiteSegments + do: Plausible.Billing.Feature.SiteSegments.check_availability(site.team) == :ok + + # For date granularity, the UTC date component IS the annotation date — callers + # store UTC midnight of their intended local date, so no timezone shift is needed. + # Return just the Date so the JSON response matches the bare-date input format. + defp localize_annotation(%Annotation{granularity: :date} = annotation, _timezone) do + %{annotation | datetime: Annotation.parse_date_granularity_datetime(annotation.datetime)} + end + + # For minute granularity, shift the stored UTC moment to the site's local timezone + # and strip the offset so the response is a naive local time string. + defp localize_annotation(%Annotation{granularity: :minute} = annotation, timezone) do + naive_local = + annotation.datetime + |> DateTime.shift_zone!(timezone) + |> DateTime.to_naive() + + %{annotation | datetime: naive_local} + end + + # If `datetime` is a naive ISO 8601 string (no UTC offset or Z suffix), interpret + # it as a local time in the site's timezone and convert to UTC before the changeset + # runs. This lets callers supply times in their local context without manually + # computing offsets. + # + # DST edge cases: + # - gap (spring-forward): the missing hour is resolved to just-after the gap + # - ambiguous (fall-back): the earlier of the two possibilities is used + # + # All other `datetime` values (bare dates, full UTC strings, invalid strings) pass + # through unchanged and are handled downstream by the changeset. + defp maybe_coerce_naive_datetime(%{"datetime" => dt} = params, timezone) + when is_binary(dt) do + with {:error, _} <- DateTime.from_iso8601(dt), + {:ok, naive_dt} <- NaiveDateTime.from_iso8601(dt) do + utc_dt = + case DateTime.from_naive(naive_dt, timezone) do + {:ok, local_dt} -> DateTime.shift_zone!(local_dt, "Etc/UTC") + {:ambiguous, first, _second} -> DateTime.shift_zone!(first, "Etc/UTC") + {:gap, _just_before, just_after} -> DateTime.shift_zone!(just_after, "Etc/UTC") + end + + Map.put(params, "datetime", utc_dt) + else + _ -> params + end + end + + defp maybe_coerce_naive_datetime(params, _timezone), do: params +end diff --git a/lib/plausible/helpers/changeset.ex b/lib/plausible/helpers/changeset.ex index e7b6d88221ba..41fb0556899e 100644 --- a/lib/plausible/helpers/changeset.ex +++ b/lib/plausible/helpers/changeset.ex @@ -10,4 +10,25 @@ defmodule Plausible.ChangesetHelpers do end) end) end + + @doc """ + iex> serialize_first_error([{"name", {"should be at most %{count} byte(s)", [count: 255]}}]) + "name should be at most 255 byte(s)" + """ + def serialize_first_error(errors) do + {field, {message, opts}} = List.first(errors) + + formatted_message = + Enum.reduce(opts, message, fn {key, value}, acc -> + placeholder = "%{#{key}}" + + if String.contains?(acc, placeholder) do + String.replace(acc, placeholder, to_string(value)) + else + acc + end + end) + + "#{field} #{formatted_message}" + end end diff --git a/lib/plausible/segments/segments.ex b/lib/plausible/segments/segments.ex index de247316b4e5..8b1556e41c88 100644 --- a/lib/plausible/segments/segments.ex +++ b/lib/plausible/segments/segments.ex @@ -461,19 +461,4 @@ defmodule Plausible.Segments do def site_segments_available?(%Plausible.Site{} = site), 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]}}]) - "name should be at most 255 byte(s)" - """ - def serialize_first_error(errors) do - {field, {message, opts}} = List.first(errors) - - formatted_message = - Enum.reduce(opts, message, fn {key, value}, acc -> - String.replace(acc, "%{#{key}}", to_string(value)) - end) - - "#{field} #{formatted_message}" - end end diff --git a/lib/plausible/teams/memberships.ex b/lib/plausible/teams/memberships.ex index 26460c8af3c4..b3186422561f 100644 --- a/lib/plausible/teams/memberships.ex +++ b/lib/plausible/teams/memberships.ex @@ -167,6 +167,7 @@ defmodule Plausible.Teams.Memberships do Repo.delete!(guest_membership) prune_guests(guest_membership.team_membership.team) Plausible.Segments.after_user_removed_from_site(site, user) + Plausible.Annotations.after_user_removed_from_site(site, user) end) send_site_member_removed_email(guest_membership) diff --git a/lib/plausible/teams/memberships/leave.ex b/lib/plausible/teams/memberships/leave.ex index c0b94fdb5183..18436fcf6ba8 100644 --- a/lib/plausible/teams/memberships/leave.ex +++ b/lib/plausible/teams/memberships/leave.ex @@ -22,6 +22,11 @@ defmodule Plausible.Teams.Memberships.Leave do team_membership.team, team_membership.user ) + + Plausible.Annotations.after_user_removed_from_team( + team_membership.team, + team_membership.user + ) end) if Keyword.get(opts, :send_email?, true) do diff --git a/lib/plausible/teams/memberships/remove.ex b/lib/plausible/teams/memberships/remove.ex index 28bf1eedbf8a..52261f365027 100644 --- a/lib/plausible/teams/memberships/remove.ex +++ b/lib/plausible/teams/memberships/remove.ex @@ -23,6 +23,11 @@ defmodule Plausible.Teams.Memberships.Remove do team_membership.team, team_membership.user ) + + Plausible.Annotations.after_user_removed_from_team( + team_membership.team, + team_membership.user + ) end) if Keyword.get(opts, :send_email?, true) do diff --git a/lib/plausible_web/controllers/api/internal/annotations_controller.ex b/lib/plausible_web/controllers/api/internal/annotations_controller.ex new file mode 100644 index 000000000000..c484811c37f3 --- /dev/null +++ b/lib/plausible_web/controllers/api/internal/annotations_controller.ex @@ -0,0 +1,187 @@ +defmodule PlausibleWeb.Api.Internal.AnnotationsController do + @moduledoc """ + Internal API controller for annotations. + """ + use Plausible + use PlausibleWeb, :controller + use PlausibleWeb.Plugs.ErrorHandler + alias PlausibleWeb.Api.Helpers, as: H + alias Plausible.Annotations + alias Plausible.ChangesetHelpers + alias Plausible.Stats.{ApiQueryParser, QueryBuilder, QueryPeriod} + + def index( + %Plug.Conn{ + assigns: + %{ + site: site, + site_role: site_role + } = assigns + } = conn, + %{} = params + ) do + user_id = + case assigns[:current_user] do + %{id: id} -> id + nil -> nil + end + + with {:ok, input_date_range} <- parse_input_date_range(params), + {:ok, relative_date} <- parse_relative_date(params) do + now = DateTime.utc_now(:second) + + range_in_site_tz = + QueryPeriod.build_range_for_site(input_date_range, site, relative_date, now) + + case Annotations.get_all_for_site(site, site_role, user_id, range_in_site_tz) do + {:ok, result} -> + json(conn, result) + + {:error, :not_enough_permissions} -> + json(conn, "Not enough permissions to get annotations") + end + else + {:error, message} -> H.bad_request(conn, message) + end + end + + defp parse_input_date_range(%{ + "date_range_start" => start, + "date_range_end" => end_ + }) + when is_binary(start) and is_binary(end_) do + case ApiQueryParser.parse_input_date_range([start, end_]) do + {:ok, parsed} -> {:ok, parsed} + {:error, %{message: message}} -> {:error, message} + end + end + + defp parse_input_date_range(%{"date_range" => "realtime"}), do: {:ok, :realtime} + defp parse_input_date_range(%{"date_range" => "realtime_30m"}), do: {:ok, :realtime_30m} + + defp parse_input_date_range(%{"date_range" => date_range}) do + case ApiQueryParser.parse_input_date_range(date_range) do + {:ok, parsed} -> {:ok, parsed} + {:error, %{message: message}} -> {:error, message} + end + end + + defp parse_input_date_range(_), do: {:error, "Required 'date_range' parameter missing"} + + defp parse_relative_date(%{"relative_date" => date}) when is_binary(date) do + case Date.from_iso8601(date) do + {:ok, date} -> {:ok, date} + _ -> {:error, "Failed to convert '#{date}' to date"} + end + end + + defp parse_relative_date(_), do: {:ok, nil} + + def create( + %Plug.Conn{ + assigns: %{ + site: site, + current_user: %{id: user_id}, + site_role: site_role + } + } = conn, + %{} = params + ) do + case Annotations.insert_one(user_id, site, site_role, params) do + {:error, :not_enough_permissions} -> + H.not_enough_permissions(conn, "Not enough permissions to create annotation") + + {:error, :annotations_limit_reached} -> + H.not_enough_permissions(conn, "Annotations limit reached") + + {:error, {:invalid_annotation, errors}} when is_list(errors) -> + conn + |> put_status(400) + |> json(%{ + error: ChangesetHelpers.serialize_first_error(errors) + }) + + {:ok, annotation} -> + json(conn, annotation) + end + end + + def create(%Plug.Conn{} = conn, _params), do: invalid_request(conn) + + def update( + %Plug.Conn{ + assigns: %{ + site: site, + current_user: %{id: user_id}, + site_role: site_role + } + } = + conn, + %{} = params + ) do + annotation_id = normalize_annotation_id_param(params["annotation_id"]) + + case Annotations.update_one(user_id, site, site_role, annotation_id, params) do + {:error, :not_enough_permissions} -> + H.not_enough_permissions(conn, "Not enough permissions to edit annotation") + + {:error, :annotation_not_found} -> + annotation_not_found(conn, params["annotation_id"]) + + {:error, {:invalid_annotation, errors}} when is_list(errors) -> + conn + |> put_status(400) + |> json(%{ + error: ChangesetHelpers.serialize_first_error(errors) + }) + + {:ok, annotation} -> + json(conn, annotation) + end + end + + def update(%Plug.Conn{} = conn, _params), do: invalid_request(conn) + + def delete( + %Plug.Conn{ + assigns: %{ + site: site, + current_user: %{id: user_id}, + site_role: site_role + } + } = + conn, + %{} = params + ) do + annotation_id = normalize_annotation_id_param(params["annotation_id"]) + + case Annotations.delete_one(user_id, site, site_role, annotation_id) do + {:error, :not_enough_permissions} -> + H.not_enough_permissions(conn, "Not enough permissions to delete annotation") + + {:error, :annotation_not_found} -> + annotation_not_found(conn, params["annotation_id"]) + + {:ok, annotation} -> + json(conn, annotation) + end + end + + def delete(%Plug.Conn{} = conn, _params), do: invalid_request(conn) + + @spec normalize_annotation_id_param(any()) :: nil | pos_integer() + defp normalize_annotation_id_param(input) do + case Integer.parse(input) do + {int_value, ""} when int_value > 0 -> int_value + _ -> nil + end + end + + defp annotation_not_found(%Plug.Conn{} = conn, annotation_id_param) do + H.not_found(conn, "Annotation not found with ID #{inspect(annotation_id_param)}") + end + + defp invalid_request(%Plug.Conn{} = conn) do + H.bad_request(conn, "Invalid request") + end +end diff --git a/lib/plausible_web/controllers/api/internal/segments_controller.ex b/lib/plausible_web/controllers/api/internal/segments_controller.ex index 30e55c336c16..623ebf8788a1 100644 --- a/lib/plausible_web/controllers/api/internal/segments_controller.ex +++ b/lib/plausible_web/controllers/api/internal/segments_controller.ex @@ -6,6 +6,7 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do use PlausibleWeb, :controller use PlausibleWeb.Plugs.ErrorHandler alias PlausibleWeb.Api.Helpers, as: H + alias Plausible.ChangesetHelpers alias Plausible.Segments def create( @@ -29,7 +30,7 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do conn |> put_status(400) |> json(%{ - error: Segments.serialize_first_error(errors) + error: ChangesetHelpers.serialize_first_error(errors) }) {:ok, segment} -> @@ -63,7 +64,7 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do conn |> put_status(400) |> json(%{ - error: Segments.serialize_first_error(errors) + error: ChangesetHelpers.serialize_first_error(errors) }) {:ok, segment} -> diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index d91ed832055f..a6939bfef399 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -303,6 +303,14 @@ defmodule PlausibleWeb.Router do delete "/:segment_id", SegmentsController, :delete get "/:segment_id/shared-links", SegmentsController, :get_related_shared_links end + + scope "/:domain/annotations", PlausibleWeb.Api.Internal, + private: %{allow_consolidated_views: true} do + get "/", AnnotationsController, :index + post "/", AnnotationsController, :create + patch "/:annotation_id", AnnotationsController, :update + delete "/:annotation_id", AnnotationsController, :delete + end end scope "/api/v1/stats", PlausibleWeb.Api, diff --git a/lib/plausible_web/templates/stats/stats.html.heex b/lib/plausible_web/templates/stats/stats.html.heex index 633430e04a46..6a8058bb7d73 100644 --- a/lib/plausible_web/templates/stats/stats.html.heex +++ b/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)} diff --git a/test/plausible/annotations/annotation_test.exs b/test/plausible/annotations/annotation_test.exs new file mode 100644 index 000000000000..5cc3d5d50621 --- /dev/null +++ b/test/plausible/annotations/annotation_test.exs @@ -0,0 +1,212 @@ +defmodule Plausible.Annotations.AnnotationTest do + use ExUnit.Case, async: true + alias Plausible.Annotations.Annotation + + describe "date-granularity" do + test "a full datetime string is rejected" do + changeset = + Annotation.changeset(%Annotation{}, %{ + "granularity" => "date", + "datetime" => "2026-06-30T00:00:00Z" + }) + + assert {"is invalid for granularity", []} = changeset.errors[:datetime] + end + + test "a non-midnight %DateTime{} is rejected" do + changeset = + Annotation.changeset(%Annotation{}, %{ + "granularity" => "date", + "datetime" => ~U[2026-06-30 14:30:00Z] + }) + + assert {"is invalid for granularity", []} = changeset.errors[:datetime] + end + + test "an unparseable date string is rejected" do + changeset = + Annotation.changeset(%Annotation{}, %{ + "granularity" => "date", + "datetime" => "not-a-date" + }) + + assert {"is invalid for granularity", []} = changeset.errors[:datetime] + end + + test "an invalid calendar date is rejected" do + changeset = + Annotation.changeset(%Annotation{}, %{ + "granularity" => "date", + "datetime" => "2026-13-45" + }) + + assert {"is invalid for granularity", []} = changeset.errors[:datetime] + end + + test "a bare YYYY-MM-DD string becomes UTC midnight" do + changeset = + Annotation.changeset(%Annotation{}, %{ + "note" => "feature released", + "type" => "personal", + "site_id" => 1, + "owner_id" => 1, + "granularity" => "date", + "datetime" => "2026-06-30" + }) + + assert changeset.valid? + assert changeset.changes.datetime == ~U[2026-06-30 00:00:00Z] + end + + test "a %Date{} struct becomes UTC midnight" do + changeset = + Annotation.changeset(%Annotation{}, %{ + "note" => "feature released", + "type" => "personal", + "site_id" => 1, + "owner_id" => 1, + "granularity" => "date", + "datetime" => ~D[2026-06-30] + }) + + assert changeset.valid? + assert changeset.changes.datetime == ~U[2026-06-30 00:00:00Z] + end + end + + describe "minute-granularity" do + test "a bare date string is rejected" do + changeset = + Annotation.changeset(%Annotation{}, %{ + "granularity" => "minute", + "datetime" => "2026-06-30" + }) + + assert {"is invalid for granularity", []} = changeset.errors[:datetime] + end + + test "an unparseable datetime is rejected" do + changeset = + Annotation.changeset(%Annotation{}, %{ + "granularity" => "minute", + "datetime" => "garbage" + }) + + assert {"is invalid for granularity", []} = changeset.errors[:datetime] + end + + test "a Z-suffixed datetime string is kept in UTC" do + changeset = + Annotation.changeset(%Annotation{}, %{ + "note" => "feature released", + "type" => "personal", + "site_id" => 1, + "owner_id" => 1, + "granularity" => "minute", + "datetime" => "2026-06-30T14:30:00Z" + }) + + assert changeset.valid? + assert changeset.changes.datetime == ~U[2026-06-30 14:30:00Z] + end + + test "an offset datetime string is shifted to UTC" do + changeset = + Annotation.changeset(%Annotation{}, %{ + "note" => "feature released", + "type" => "personal", + "site_id" => 1, + "owner_id" => 1, + "granularity" => "minute", + "datetime" => "2026-06-30T10:00:00-02:00" + }) + + assert changeset.valid? + assert changeset.changes.datetime == ~U[2026-06-30 12:00:00Z] + end + + test "a %DateTime{} is passed through in UTC" do + changeset = + Annotation.changeset(%Annotation{}, %{ + "note" => "feature released", + "type" => "personal", + "site_id" => 1, + "owner_id" => 1, + "granularity" => "minute", + "datetime" => ~U[2026-06-30 14:30:00Z] + }) + + assert changeset.valid? + assert changeset.changes.datetime == ~U[2026-06-30 14:30:00Z] + end + end + + describe "unknown granularity" do + test "defers to Ecto's enum cast, not our coercion error" do + changeset = + Annotation.changeset(%Annotation{}, %{ + "granularity" => "hour", + "datetime" => "2026-06-30T10:00:00Z" + }) + + assert changeset.errors[:granularity] + refute changeset.errors[:datetime] + end + end + + describe "granularity change" do + test "flipping :date to :minute without a datetime is rejected" do + existing = %Annotation{granularity: :date, datetime: ~U[2026-06-15 00:00:00Z]} + changeset = Annotation.changeset(existing, %{"granularity" => "minute"}) + + assert {"must be supplied when granularity changes", []} = changeset.errors[:datetime] + end + + test "flipping :minute to :date without a datetime is rejected" do + existing = %Annotation{granularity: :minute, datetime: ~U[2026-06-15 14:30:00Z]} + changeset = Annotation.changeset(existing, %{"granularity" => "date"}) + + assert {"must be supplied when granularity changes", []} = changeset.errors[:datetime] + end + + test "flipping granularity to minute with an appropriate datetime is accepted" do + existing = %Annotation{ + note: "feature released", + type: :personal, + site_id: 1, + owner_id: 1, + granularity: :date, + datetime: ~U[2026-06-15 00:00:00Z] + } + + changeset = + Annotation.changeset(existing, %{ + "granularity" => "minute", + "datetime" => "2026-06-15T14:30:00Z" + }) + + assert changeset.valid? + assert changeset.changes.datetime == ~U[2026-06-15 14:30:00Z] + end + + test "flipping granularity to date with an appropriate date is accepted" do + existing = %Annotation{ + note: "feature released", + type: :personal, + site_id: 1, + owner_id: 1, + granularity: :minute, + datetime: ~U[2026-06-15 10:00:00Z] + } + + changeset = + Annotation.changeset(existing, %{ + "granularity" => "date", + "datetime" => "2026-06-16" + }) + + assert changeset.valid? + assert changeset.changes.datetime == ~U[2026-06-16 00:00:00Z] + end + end +end diff --git a/test/plausible/billing/feature_test.exs b/test/plausible/billing/feature_test.exs index 82dfab0a4e42..c74e0e4cfa77 100644 --- a/test/plausible/billing/feature_test.exs +++ b/test/plausible/billing/feature_test.exs @@ -1,5 +1,4 @@ defmodule Plausible.Billing.FeatureTest do - alias Plausible.Billing.Feature.SiteSegments use Plausible.DataCase alias Plausible.Billing.Feature.{ diff --git a/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs b/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs new file mode 100644 index 000000000000..1fc48c6e70e0 --- /dev/null +++ b/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs @@ -0,0 +1,780 @@ +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?date_range=day&relative_date=2026-01-04" + ) + + 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?date_range=day&relative_date=2026-01-04" + ) + + 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?date_range=day&relative_date=2026-01-04") + + 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?date_range=day&relative_date=2026-01-04") + + 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?date_range=day&relative_date=2026-01-04" + ) + + assert json_response(conn, 404) + end + end + + describe "GET /api/:domain/annotations - period filtering" do + setup [:create_user, :log_in] + + test "returns 400 when date_range is missing", %{conn: conn, user: user} do + site = new_site(owner: user) + + conn = get(conn, "/api/#{site.domain}/annotations") + + assert %{"error" => _} = json_response(conn, 400) + end + + test "returns 400 when date_range is unrecognized", %{conn: conn, user: user} do + site = new_site(owner: user) + + conn = get(conn, "/api/#{site.domain}/annotations?date_range=banana") + + assert %{"error" => _} = json_response(conn, 400) + end + + test "filters out annotations outside a 28d window", %{conn: conn, user: user} do + site = new_site(owner: user) + + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :date, + datetime: ~U[2026-06-26 00:00:00Z], + note: "in range" + ) + + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :date, + datetime: ~U[2026-05-01 00:00:00Z], + note: "out of range" + ) + + conn = + get(conn, "/api/#{site.domain}/annotations?date_range=28d&relative_date=2026-06-29") + + results = json_response(conn, 200) + assert Enum.map(results, & &1["note"]) == ["in range"] + end + + test "filters by a custom [from,to] date_range", %{conn: conn, user: user} do + site = new_site(owner: user) + + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :date, + datetime: ~U[2026-06-22 00:00:00Z], + note: "in range" + ) + + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :date, + datetime: ~U[2026-06-26 00:00:00Z], + note: "out of range" + ) + + conn = + get( + conn, + "/api/#{site.domain}/annotations?date_range_start=2026-06-20&date_range_end=2026-06-25" + ) + + results = json_response(conn, 200) + assert Enum.map(results, & &1["note"]) == ["in range"] + end + + test "minute granularity is filtered against the UTC window of a day period", + %{conn: conn, user: user} do + # NY is UTC-4 in June (DST). Local 2026-06-28 -> UTC [04:00 2026-06-28, 03:59:59 2026-06-29]. + # Using a past relative_date so :day builds the full local day window + # rather than [00:00, now], which would make the test time-of-day dependent. + site = new_site(owner: user, timezone: "America/New_York") + + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :minute, + # 11:00 local — inside the UTC window + datetime: ~U[2026-06-28 15:00:00Z], + note: "in window" + ) + + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :minute, + # next-day 01:00 local — past the UTC window end + datetime: ~U[2026-06-29 05:00:00Z], + note: "past window" + ) + + conn = + get(conn, "/api/#{site.domain}/annotations?date_range=day&relative_date=2026-06-28") + + results = json_response(conn, 200) + assert Enum.map(results, & &1["note"]) == ["in window"] + end + + test "date-granularity annotation for the dashboard's local date is included even though its UTC moment falls outside the UTC window", + %{conn: conn, user: user} do + # Date-granularity annotations store UTC midnight of the intended local + # date, so the stored 2026-06-29T00:00:00Z is *outside* NY's UTC window + # for local 2026-06-29 ([04:00, next 03:59:59]). It must still be returned + # because its local date matches the dashboard's local date range. + site = new_site(owner: user, timezone: "America/New_York") + + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :date, + datetime: ~U[2026-06-29 00:00:00Z], + note: "today date-annotation" + ) + + conn = + get(conn, "/api/#{site.domain}/annotations?date_range=day&relative_date=2026-06-29") + + results = json_response(conn, 200) + assert Enum.map(results, & &1["note"]) == ["today date-annotation"] + end + + test "returns both date and minute annotations when both fall inside the period", + %{conn: conn, user: user} do + site = new_site(owner: user) + + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :date, + datetime: ~U[2026-06-25 00:00:00Z], + note: "date" + ) + + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :minute, + datetime: ~U[2026-06-26 10:30:00Z], + note: "minute" + ) + + conn = + get( + conn, + "/api/#{site.domain}/annotations?date_range_start=2026-06-20&date_range_end=2026-06-29" + ) + + results = json_response(conn, 200) + notes = results |> Enum.map(& &1["note"]) |> Enum.sort() + assert notes == ["date", "minute"] + end + + test "fetches annotations for realtime date_range", %{conn: conn, user: user} do + site = new_site(owner: user) + + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :minute, + datetime: DateTime.utc_now(), + note: "now" + ) + + conn = get(conn, "/api/#{site.domain}/annotations?date_range=realtime") + + results = json_response(conn, 200) + assert Enum.map(results, & &1["note"]) == ["now"] + end + end + + describe "POST /api/:domain/annotations - datetime coercion" do + setup [:create_user, :log_in, :create_site] + + test "accepts bare date string when granularity is date", + %{conn: conn, site: site} do + response = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "date", + "datetime" => "2026-01-04" + }) + |> json_response(200) + + assert_matches ^strict_map(%{ + "id" => ^any(:pos_integer), + "note" => "feature released", + "type" => "personal", + "granularity" => "date", + "datetime" => "2026-01-04", + "owner_id" => ^any(:pos_integer), + "owner_name" => ^any(:string), + "inserted_at" => ^any(:iso8601_naive_datetime), + "updated_at" => ^any(:iso8601_naive_datetime) + }) = response + end + + test "rejects full datetime string when granularity is date", + %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "date", + "datetime" => "2026-01-04T00:00:00Z" + }) + + assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + end + + test "accepts full datetime string when granularity is minute", + %{conn: conn, site: site} do + # Site is Etc/UTC by default; UTC moment is returned as naive local time + response = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "minute", + "datetime" => "2026-01-04T14:32:00Z" + }) + |> json_response(200) + + assert response["datetime"] == "2026-01-04T14:32:00" + end + + test "rejects bare date string when granularity is minute", + %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "minute", + "datetime" => "2026-01-04" + }) + + assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + end + + test "rejects invalid calendar date when granularity is date", + %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "date", + "datetime" => "2026-13-45" + }) + + assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + end + + test "rejects non-date string when granularity is date", + %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "date", + "datetime" => "not-a-date" + }) + + assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + end + + test "rejects non-datetime string when granularity is minute", + %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "minute", + "datetime" => "not-a-datetime" + }) + + assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + end + + test "rejects empty datetime string when granularity is date", + %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "date", + "datetime" => "" + }) + + assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + end + + test "rejects empty datetime string when granularity is minute", + %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "minute", + "datetime" => "" + }) + + assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + end + + test "rejects date shorter than 10 characters when granularity is date", + %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "date", + "datetime" => "2026-1-4" + }) + + assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + end + + test "rejects invalid calendar datetime when granularity is minute", + %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "minute", + "datetime" => "2026-13-45T14:30:00Z" + }) + + assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + end + end + + describe "POST /api/:domain/annotations - naive local time coercion" do + setup [:create_user, :log_in] + + test "converts naive local time to UTC and returns it back as local time", + %{conn: conn, user: user} do + # America/New_York is UTC-5 in January; input and output should be the same + # local time (round-trip), while the DB stores the UTC equivalent. + site = new_site(owner: user, timezone: "America/New_York") + + response = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "minute", + "datetime" => "2026-01-04T14:30:00" + }) + |> json_response(200) + + assert response["datetime"] == "2026-01-04T14:30:00" + end + + test "rejects naive datetime when granularity is date", + %{conn: conn, user: user} do + site = new_site(owner: user, timezone: "America/New_York") + + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "date", + "datetime" => "2026-01-04T00:00:00" + }) + + assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + end + + test "UTC datetime string is stored as UTC and returned as site local time", + %{conn: conn, user: user} do + # America/New_York is UTC-5 in January, so 14:30 UTC = 09:30 local + site = new_site(owner: user, timezone: "America/New_York") + + response = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "minute", + "datetime" => "2026-01-04T14:30:00Z" + }) + |> json_response(200) + + assert response["datetime"] == "2026-01-04T09:30:00" + end + end + + describe "PATCH /api/:domain/annotations/:annotation_id - naive local time coercion" do + setup [:create_user, :log_in] + + test "converts naive local time to UTC and returns it back as local time", + %{conn: conn, user: user} do + # America/New_York is UTC-4 in June (DST), so input and output are the same + site = new_site(owner: user, timezone: "America/New_York") + + annotation = + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :minute, + datetime: ~U[2026-01-01 00:00:00Z] + ) + + response = + patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ + "granularity" => "minute", + "datetime" => "2026-06-15T10:00:00" + }) + |> json_response(200) + + assert response["datetime"] == "2026-06-15T10:00:00" + end + end + + describe "PATCH /api/:domain/annotations/:annotation_id - datetime coercion" do + setup [:create_user, :log_in, :create_site] + + test "accepts bare date string when granularity is date", + %{conn: conn, site: site, user: user} do + annotation = + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :date, + datetime: ~U[2026-01-01 00:00:00Z] + ) + + response = + patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ + "granularity" => "date", + "datetime" => "2026-06-15" + }) + |> json_response(200) + + assert response["datetime"] == "2026-06-15" + end + + test "rejects bare date string when granularity is minute", + %{conn: conn, site: site, user: user} do + annotation = + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :minute, + datetime: ~U[2026-01-01 10:00:00Z] + ) + + conn = + patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ + "granularity" => "minute", + "datetime" => "2026-06-15" + }) + + assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + end + + test "rejects full datetime string when granularity switched to date", + %{conn: conn, site: site, user: user} do + annotation = + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :minute, + datetime: ~U[2026-01-01 10:00:00Z] + ) + + conn = + patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ + "granularity" => "date", + "datetime" => "2026-06-15T10:00:00Z" + }) + + assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + end + + test "rejects granularity change from date to minute without a new datetime", + %{conn: conn, site: site, user: user} do + annotation = + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :date, + datetime: ~U[2026-06-15 00:00:00Z] + ) + + conn = + patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ + "granularity" => "minute" + }) + + assert json_response(conn, 400) == %{ + "error" => "datetime must be supplied when granularity changes" + } + + reloaded = Plausible.Repo.get!(Plausible.Annotations.Annotation, annotation.id) + assert reloaded.granularity == :date + assert reloaded.datetime == ~U[2026-06-15 00:00:00Z] + end + + test "rejects granularity change from minute to date without a new datetime", + %{conn: conn, site: site, user: user} do + annotation = + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :minute, + datetime: ~U[2026-06-15 14:30:00Z] + ) + + conn = + patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ + "granularity" => "date" + }) + + assert json_response(conn, 400) == %{ + "error" => "datetime must be supplied when granularity changes" + } + + reloaded = Plausible.Repo.get!(Plausible.Annotations.Annotation, annotation.id) + assert reloaded.granularity == :minute + assert reloaded.datetime == ~U[2026-06-15 14:30:00Z] + end + + test "accepts granularity change from date to minute with a new datetime", + %{conn: conn, site: site, user: user} do + annotation = + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :date, + datetime: ~U[2026-06-15 00:00:00Z] + ) + + response = + patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ + "granularity" => "minute", + "datetime" => "2026-06-15T14:30:00Z" + }) + |> json_response(200) + + assert response["granularity"] == "minute" + assert response["datetime"] == "2026-06-15T14:30:00" + + reloaded = Plausible.Repo.get!(Plausible.Annotations.Annotation, annotation.id) + assert reloaded.granularity == :minute + assert reloaded.datetime == ~U[2026-06-15 14:30:00Z] + end + + test "accepts granularity change from minute to date with a new datetime", + %{conn: conn, site: site, user: user} do + annotation = + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :minute, + datetime: ~U[2026-06-15 14:30:00Z] + ) + + response = + patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ + "granularity" => "date", + "datetime" => "2026-06-20" + }) + |> json_response(200) + + assert response["granularity"] == "date" + assert response["datetime"] == "2026-06-20" + + reloaded = Plausible.Repo.get!(Plausible.Annotations.Annotation, annotation.id) + assert reloaded.granularity == :date + assert reloaded.datetime == ~U[2026-06-20 00:00:00Z] + end + end + + describe "POST /api/:domain/annotations - required fields and length" do + setup [:create_user, :log_in, :create_site] + + test "rejects missing note", %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "type" => "personal", + "granularity" => "date", + "datetime" => "2026-01-04" + }) + + assert json_response(conn, 400) == %{"error" => "note can't be blank"} + end + + test "rejects empty note", %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "", + "type" => "personal", + "granularity" => "date", + "datetime" => "2026-01-04" + }) + + assert json_response(conn, 400) == %{"error" => "note can't be blank"} + end + + test "rejects note over 255 bytes", %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => String.duplicate("a", 256), + "type" => "personal", + "granularity" => "date", + "datetime" => "2026-01-04" + }) + + assert json_response(conn, 400) == %{"error" => "note should be at most 255 byte(s)"} + end + + test "accepts note of exactly 255 bytes", %{conn: conn, site: site} do + response = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => String.duplicate("a", 255), + "type" => "personal", + "granularity" => "date", + "datetime" => "2026-01-04" + }) + |> json_response(200) + + assert response["note"] == String.duplicate("a", 255) + end + + test "rejects missing datetime", %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "date" + }) + + assert json_response(conn, 400) == %{"error" => "datetime can't be blank"} + end + + test "rejects unknown granularity", %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "granularity" => "hour", + "datetime" => "2026-01-04T00:00:00Z" + }) + + assert json_response(conn, 400) == %{"error" => "granularity is invalid"} + end + + test "rejects missing granularity", %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "personal", + "datetime" => "2026-01-04T00:00:00Z" + }) + + assert json_response(conn, 400) == %{"error" => "granularity can't be blank"} + end + + test "rejects unknown type as insufficient permissions", %{conn: conn, site: site} do + conn = + post(conn, "/api/#{site.domain}/annotations", %{ + "note" => "feature released", + "type" => "team", + "granularity" => "date", + "datetime" => "2026-01-04" + }) + + assert json_response(conn, 403) == %{ + "error" => "Not enough permissions to create annotation" + } + end + end +end diff --git a/test/support/factory.ex b/test/support/factory.ex index 5396ee4475d2..3ca8fbfe8d28 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -392,6 +392,15 @@ defmodule Plausible.Factory do } end + def annotation_factory do + %Plausible.Annotations.Annotation{ + note: "a test annotation", + type: :personal, + datetime: ~U[2026-01-04 00:00:00Z], + granularity: :date + } + end + defp hash_key() do Keyword.fetch!( Application.get_env(:plausible, PlausibleWeb.Endpoint), From 31f39a4f78c38e97816bd4a37290462851e48591 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 2 Jul 2026 14:01:41 +0300 Subject: [PATCH 02/23] Block access to annotations API without FF --- .../plugs/feature_flag_check_plug.ex | 31 +++++++++++++ lib/plausible_web/router.ex | 4 ++ .../plugs/feature_flag_check_plug_test.exs | 46 +++++++++++++++++++ test/test_helper.exs | 2 + 4 files changed, 83 insertions(+) create mode 100644 lib/plausible_web/plugs/feature_flag_check_plug.ex create mode 100644 test/plausible_web/plugs/feature_flag_check_plug_test.exs diff --git a/lib/plausible_web/plugs/feature_flag_check_plug.ex b/lib/plausible_web/plugs/feature_flag_check_plug.ex new file mode 100644 index 000000000000..c04e23e38d33 --- /dev/null +++ b/lib/plausible_web/plugs/feature_flag_check_plug.ex @@ -0,0 +1,31 @@ +defmodule PlausibleWeb.Plugs.FeatureFlagCheckPlug do + @moduledoc """ + plug(PlausibleWeb.Plugs.FeatureFlagCheckPlug, [:flag_foo, :flag_bar]) + to halt any API connections with 404 where conn.assigns.current_user or conn.assigns.site + don't have both feature flags true. + """ + def init(feature_flags) when is_list(feature_flags) and length(feature_flags) > 0 do + feature_flags + end + + def init(_), + do: raise(ArgumentError, "The first argument must be a non-empty list of feature flags") + + def call(%Plug.Conn{} = conn, flags) do + if validate(conn.assigns[:current_user], conn.assigns.site, flags) do + conn + else + PlausibleWeb.Api.Helpers.not_found(conn, "Not found") + end + end + + defp validate(nil = _current_user, site, flags), + do: Enum.all?(flags, fn flag -> FunWithFlags.enabled?(flag, for: site) end) + + defp validate(current_user, site, flags), + do: + Enum.all?(flags, fn flag -> + FunWithFlags.enabled?(flag, for: current_user) || + FunWithFlags.enabled?(flag, for: site) + end) +end diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index a6939bfef399..5ac770746d8c 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -306,6 +306,10 @@ defmodule PlausibleWeb.Router do scope "/:domain/annotations", PlausibleWeb.Api.Internal, private: %{allow_consolidated_views: true} do + pipeline :annotations_endpoints, + do: plug(PlausibleWeb.Plugs.FeatureFlagCheckPlug, [:annotations]) + + pipe_through :annotations_endpoints get "/", AnnotationsController, :index post "/", AnnotationsController, :create patch "/:annotation_id", AnnotationsController, :update diff --git a/test/plausible_web/plugs/feature_flag_check_plug_test.exs b/test/plausible_web/plugs/feature_flag_check_plug_test.exs new file mode 100644 index 000000000000..ead037ed4b9d --- /dev/null +++ b/test/plausible_web/plugs/feature_flag_check_plug_test.exs @@ -0,0 +1,46 @@ +defmodule PlausibleWeb.Plugs.FeatureFlagCheckPlugTest do + use PlausibleWeb.ConnCase, async: true + use Plausible.Teams.Test + alias PlausibleWeb.Plugs.{AuthorizeSiteAccess, FeatureFlagCheckPlug} + + setup [:create_user, :log_in, :create_site] + + test "returns 404 when any of the expected feature flags is not enabled", %{ + conn: conn, + site: site + } do + # currently enabled flags are defined test/test_helper.exs + required_flags = + FeatureFlagCheckPlug.init([:missing_feature_flag, :annotations]) + + conn = + conn + |> get_conn_with_current_user_and_site(site) + |> FeatureFlagCheckPlug.call(required_flags) + + assert conn.halted + assert %{"error" => "Not found"} == json_response(conn, 404) + end + + test "passes conn when required flags are enabled", %{ + conn: conn, + site: site + } do + # currently enabled flags are defined test/test_helper.exs + required_flags = FeatureFlagCheckPlug.init([:annotations]) + + conn = + conn + |> get_conn_with_current_user_and_site(site) + |> FeatureFlagCheckPlug.call(required_flags) + + assert !conn.halted + end + + defp get_conn_with_current_user_and_site(%Plug.Conn{} = conn, %Plausible.Site{} = site) do + conn + |> bypass_through(PlausibleWeb.Router) + |> get("/plug-tests/api-basic?site=#{site.domain}") + |> AuthorizeSiteAccess.call(AuthorizeSiteAccess.init({:all_roles, "site"})) + end +end diff --git a/test/test_helper.exs b/test/test_helper.exs index bd6ac0e85ed4..63f401e79122 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -13,6 +13,8 @@ if Mix.env() != :e2e_test do Application.ensure_all_started(:double) + FunWithFlags.enable(:annotations) + Ecto.Adapters.SQL.Sandbox.mode(Plausible.Repo, :manual) end From fbc12983d1a2c7e47339fc0a6f068bbb5ff187af Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 2 Jul 2026 14:33:23 +0300 Subject: [PATCH 03/23] Fix spelling --- test/plausible/annotations/annotation_test.exs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/plausible/annotations/annotation_test.exs b/test/plausible/annotations/annotation_test.exs index 5cc3d5d50621..f41fb952d254 100644 --- a/test/plausible/annotations/annotation_test.exs +++ b/test/plausible/annotations/annotation_test.exs @@ -23,7 +23,7 @@ defmodule Plausible.Annotations.AnnotationTest do assert {"is invalid for granularity", []} = changeset.errors[:datetime] end - test "an unparseable date string is rejected" do + test "an unparsable date string is rejected" do changeset = Annotation.changeset(%Annotation{}, %{ "granularity" => "date", @@ -85,7 +85,7 @@ defmodule Plausible.Annotations.AnnotationTest do assert {"is invalid for granularity", []} = changeset.errors[:datetime] end - test "an unparseable datetime is rejected" do + test "an unparsable datetime is rejected" do changeset = Annotation.changeset(%Annotation{}, %{ "granularity" => "minute", From 76098ee4807c144fe75dc7ca2445a940e0f931e5 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 2 Jul 2026 15:05:27 +0300 Subject: [PATCH 04/23] Fix unused alias --- .../controllers/api/internal/annotations_controller.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/plausible_web/controllers/api/internal/annotations_controller.ex b/lib/plausible_web/controllers/api/internal/annotations_controller.ex index c484811c37f3..071c9204b185 100644 --- a/lib/plausible_web/controllers/api/internal/annotations_controller.ex +++ b/lib/plausible_web/controllers/api/internal/annotations_controller.ex @@ -8,7 +8,7 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsController do alias PlausibleWeb.Api.Helpers, as: H alias Plausible.Annotations alias Plausible.ChangesetHelpers - alias Plausible.Stats.{ApiQueryParser, QueryBuilder, QueryPeriod} + alias Plausible.Stats.{ApiQueryParser, QueryPeriod} def index( %Plug.Conn{ From 432c65e9a551f289fc3ea1d05f693331788348a1 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 2 Jul 2026 15:10:52 +0300 Subject: [PATCH 05/23] Add moduledoc to factory.ex --- test/support/factory.ex | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/support/factory.ex b/test/support/factory.ex index 3ca8fbfe8d28..482ba21c7c07 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -1,4 +1,7 @@ defmodule Plausible.Factory do + @moduledoc """ + See https://ecto.hexdocs.pm/test-factories.html + """ use ExMachina.Ecto, repo: Plausible.Repo require Plausible.Billing.Subscription.Status alias Plausible.Billing.Subscription From 8f693f1ba6ab2cc7291cd778740cd34d2e59969e Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 2 Jul 2026 15:13:04 +0300 Subject: [PATCH 06/23] Fix spec --- lib/plausible/annotations/annotations.ex | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/plausible/annotations/annotations.ex b/lib/plausible/annotations/annotations.ex index 72943249b4b6..1e86d08545ac 100644 --- a/lib/plausible/annotations/annotations.ex +++ b/lib/plausible/annotations/annotations.ex @@ -4,6 +4,7 @@ defmodule Plausible.Annotations do """ alias Plausible.Annotations.Annotation alias Plausible.Repo + alias Plausible.Stats.DateTimeRange import Ecto.Query @roles_with_personal_annotations [:billing, :viewer, :editor, :admin, :owner, :super_admin] @@ -17,18 +18,18 @@ defmodule Plausible.Annotations do @max_annotations 500 - @spec get_all_for_site(Plausible.Site.t(), atom(), pos_integer() | nil, QueryPeriod.t()) :: + @spec get_all_for_site(Plausible.Site.t(), atom(), pos_integer() | nil, DateTimeRange.t()) :: {:error, :not_enough_permissions} | {:ok, list(Annotation.t())} def get_all_for_site( %Plausible.Site{} = site, site_role, user_id, - %Plausible.Stats.DateTimeRange{} = range_in_site_tz + %DateTimeRange{} = range_in_site_tz ) do # Minute granularity annotations are stored for the particular UTC moment they're for, # so they must be in the range of the UTC query period. minute_granularity_range = - range_in_site_tz |> Plausible.Stats.DateTimeRange.to_timezone("Etc/UTC") + range_in_site_tz |> DateTimeRange.to_timezone("Etc/UTC") # Date granularity annotations are stored for the UTC midnight of the date they're for, # so the range for querying these must reflect that. From 2d270142fa9a69e7a66e2e852aacb86ad92cd9d8 Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Mon, 6 Jul 2026 15:36:06 +0200 Subject: [PATCH 07/23] Pass full structs to annotation API and don't validate how Phoenix calls actions --- lib/plausible/annotations/annotation.ex | 19 ++++- lib/plausible/annotations/annotations.ex | 52 +++++------- .../api/internal/annotations_controller.ex | 82 +++++-------------- 3 files changed, 58 insertions(+), 95 deletions(-) diff --git a/lib/plausible/annotations/annotation.ex b/lib/plausible/annotations/annotation.ex index dc75d0ca1627..a0e89ff889b1 100644 --- a/lib/plausible/annotations/annotation.ex +++ b/lib/plausible/annotations/annotation.ex @@ -44,16 +44,27 @@ defmodule Plausible.Annotations.Annotation do timestamps() end + def create_changeset(attrs, site, owner) do + %__MODULE__{} + |> changeset(attrs) + |> put_assoc(:site, site) + |> put_assoc(:owner, owner) + end + + def update_changeset(annotation, attrs, owner) do + annotation + |> changeset(attrs) + |> put_assoc(:owner, owner) + end + def changeset(annotation, attrs) do attrs = stringify_keys(attrs) {attrs, invalid_datetime_for_granularity?} = coerce_datetime(attrs) annotation - |> cast(attrs, [:note, :site_id, :type, :owner_id, :datetime, :granularity]) - |> validate_required([:note, :site_id, :type, :owner_id, :datetime, :granularity]) + |> cast(attrs, [:note, :type, :datetime, :granularity]) + |> validate_required([:note, :type, :datetime, :granularity]) |> validate_length(:note, count: :bytes, min: 1, max: 255) - |> foreign_key_constraint(:site_id) - |> foreign_key_constraint(:owner_id) |> validate_datetime_supplied_on_granularity_change() |> maybe_add_datetime_error(invalid_datetime_for_granularity?) end diff --git a/lib/plausible/annotations/annotations.ex b/lib/plausible/annotations/annotations.ex index 1e86d08545ac..5e47ce9164fb 100644 --- a/lib/plausible/annotations/annotations.ex +++ b/lib/plausible/annotations/annotations.ex @@ -3,6 +3,7 @@ defmodule Plausible.Annotations do Module for accessing Annotations. """ alias Plausible.Annotations.Annotation + alias Plausible.Auth.User alias Plausible.Repo alias Plausible.Stats.DateTimeRange import Ecto.Query @@ -18,12 +19,12 @@ defmodule Plausible.Annotations do @max_annotations 500 - @spec get_all_for_site(Plausible.Site.t(), atom(), pos_integer() | nil, DateTimeRange.t()) :: + @spec get_all_for_site(Plausible.Site.t(), atom(), User.t() | nil, DateTimeRange.t()) :: {:error, :not_enough_permissions} | {:ok, list(Annotation.t())} def get_all_for_site( %Plausible.Site{} = site, site_role, - user_id, + user, %DateTimeRange{} = range_in_site_tz ) do # Minute granularity annotations are stored for the particular UTC moment they're for, @@ -80,7 +81,7 @@ defmodule Plausible.Annotations do where: annotation.site_id == ^site.id, where: annotation.type == :site or - (annotation.type == :personal and annotation.owner_id == ^user_id), + (annotation.type == :personal and annotation.owner_id == ^user.id), where: ^in_range_clause, order_by: [desc: annotation.updated_at, desc: annotation.id], preload: [:owner] @@ -94,13 +95,13 @@ defmodule Plausible.Annotations do end end - @spec get_one(pos_integer(), Plausible.Site.t(), atom(), pos_integer() | nil) :: + @spec get_one(User.t(), Plausible.Site.t(), atom(), pos_integer() | nil) :: {:ok, Annotation.t()} | error_not_enough_permissions() | error_annotation_not_found() - def get_one(user_id, site, site_role, annotation_id) do + def get_one(user, site, site_role, annotation_id) do if site_role in roles_with_personal_annotations() do - case do_get_one(user_id, site.id, annotation_id) do + case do_get_one(user, site, annotation_id) do %Annotation{} = annotation -> {:ok, annotation} nil -> {:error, :annotation_not_found} end @@ -109,7 +110,7 @@ defmodule Plausible.Annotations do end end - @spec insert_one(pos_integer(), Plausible.Site.t(), atom(), map()) :: + @spec insert_one(User.t(), Plausible.Site.t(), atom(), map()) :: {:ok, Annotation.t()} | error_not_enough_permissions() | error_invalid_annotation() @@ -117,7 +118,7 @@ defmodule Plausible.Annotations do | unknown_error() def insert_one( - user_id, + user, %Plausible.Site{} = site, site_role, %{} = params @@ -125,11 +126,7 @@ defmodule Plausible.Annotations do params = maybe_coerce_naive_datetime(params, site.timezone) with :ok <- can_insert_one?(site, site_role, params), - %{valid?: true} = changeset <- - Annotation.changeset( - %Annotation{}, - Map.merge(params, %{"site_id" => site.id, "owner_id" => user_id}) - ) do + %{valid?: true} = changeset <- Annotation.create_changeset(params, site, user) do {:ok, changeset |> Repo.insert!() |> Repo.preload(:owner) |> localize_annotation(site.timezone)} else @@ -141,14 +138,14 @@ defmodule Plausible.Annotations do end end - @spec update_one(pos_integer(), Plausible.Site.t(), atom(), pos_integer(), map()) :: + @spec update_one(User.t(), Plausible.Site.t(), atom(), pos_integer(), map()) :: {:ok, Annotation.t()} | error_not_enough_permissions() | error_invalid_annotation() | unknown_error() def update_one( - user_id, + user, %Plausible.Site{} = site, site_role, annotation_id, @@ -156,13 +153,9 @@ defmodule Plausible.Annotations do ) do params = maybe_coerce_naive_datetime(params, site.timezone) - with {:ok, annotation} <- get_one(user_id, site, site_role, annotation_id), + with {:ok, annotation} <- get_one(user, site, site_role, annotation_id), :ok <- can_update_one?(site, site_role, params, annotation.type), - %{valid?: true} = changeset <- - Annotation.changeset( - annotation, - Map.merge(params, %{"owner_id" => user_id}) - ) do + %{valid?: true} = changeset <- Annotation.update_changeset(annotation, params, user) do Repo.update!(changeset) {:ok, @@ -237,8 +230,8 @@ defmodule Plausible.Annotations do # Site annotations are set to owner=null via ON DELETE SET NULL end - def delete_one(user_id, %Plausible.Site{} = site, site_role, annotation_id) do - with {:ok, annotation} <- get_one(user_id, site, site_role, annotation_id) do + def delete_one(user, %Plausible.Site{} = site, site_role, annotation_id) do + with {:ok, annotation} <- get_one(user, site, site_role, annotation_id) do cond do annotation.type == :site and site_role in roles_with_maybe_site_annotations() -> {:ok, do_delete_one(annotation) |> localize_annotation(site.timezone)} @@ -252,22 +245,21 @@ defmodule Plausible.Annotations do end end - @spec do_get_one(pos_integer(), pos_integer(), pos_integer() | nil) :: - Annotation.t() | nil - defp do_get_one(user_id, site_id, annotation_id) + @spec do_get_one(User.t(), Plausible.Site.t(), pos_integer() | nil) :: Annotation.t() | nil + defp do_get_one(user, site, annotation_id) - defp do_get_one(_user_id, _site_id, nil) do + defp do_get_one(_user, _site, nil) do nil end - defp do_get_one(user_id, site_id, annotation_id) do + defp do_get_one(user, site, annotation_id) do query = from(annotation in Annotation, - where: annotation.site_id == ^site_id, + where: annotation.site_id == ^site.id, where: annotation.id == ^annotation_id, where: annotation.type == :site or - (annotation.type == :personal and annotation.owner_id == ^user_id), + (annotation.type == :personal and annotation.owner_id == ^user.id), preload: [:owner] ) diff --git a/lib/plausible_web/controllers/api/internal/annotations_controller.ex b/lib/plausible_web/controllers/api/internal/annotations_controller.ex index 071c9204b185..f5c9f9c78fc5 100644 --- a/lib/plausible_web/controllers/api/internal/annotations_controller.ex +++ b/lib/plausible_web/controllers/api/internal/annotations_controller.ex @@ -10,21 +10,10 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsController do alias Plausible.ChangesetHelpers alias Plausible.Stats.{ApiQueryParser, QueryPeriod} - def index( - %Plug.Conn{ - assigns: - %{ - site: site, - site_role: site_role - } = assigns - } = conn, - %{} = params - ) do - user_id = - case assigns[:current_user] do - %{id: id} -> id - nil -> nil - end + def index(conn, params) do + user = conn.assigns.current_user + site = conn.assigns.site + site_role = conn.assigns.site_role with {:ok, input_date_range} <- parse_input_date_range(params), {:ok, relative_date} <- parse_relative_date(params) do @@ -33,7 +22,7 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsController do range_in_site_tz = QueryPeriod.build_range_for_site(input_date_range, site, relative_date, now) - case Annotations.get_all_for_site(site, site_role, user_id, range_in_site_tz) do + case Annotations.get_all_for_site(site, site_role, user, range_in_site_tz) do {:ok, result} -> json(conn, result) @@ -77,17 +66,12 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsController do defp parse_relative_date(_), do: {:ok, nil} - def create( - %Plug.Conn{ - assigns: %{ - site: site, - current_user: %{id: user_id}, - site_role: site_role - } - } = conn, - %{} = params - ) do - case Annotations.insert_one(user_id, site, site_role, params) do + def create(conn, params) do + user = conn.assigns.current_user + site = conn.assigns.site + site_role = conn.assigns.site_role + + case Annotations.insert_one(user, site, site_role, params) do {:error, :not_enough_permissions} -> H.not_enough_permissions(conn, "Not enough permissions to create annotation") @@ -106,22 +90,13 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsController do end end - def create(%Plug.Conn{} = conn, _params), do: invalid_request(conn) - - def update( - %Plug.Conn{ - assigns: %{ - site: site, - current_user: %{id: user_id}, - site_role: site_role - } - } = - conn, - %{} = params - ) do + def update(conn, params) do + user = conn.assigns.current_user + site = conn.assigns.site + site_role = conn.assigns.site_role annotation_id = normalize_annotation_id_param(params["annotation_id"]) - case Annotations.update_one(user_id, site, site_role, annotation_id, params) do + case Annotations.update_one(user, site, site_role, annotation_id, params) do {:error, :not_enough_permissions} -> H.not_enough_permissions(conn, "Not enough permissions to edit annotation") @@ -140,22 +115,13 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsController do end end - def update(%Plug.Conn{} = conn, _params), do: invalid_request(conn) - - def delete( - %Plug.Conn{ - assigns: %{ - site: site, - current_user: %{id: user_id}, - site_role: site_role - } - } = - conn, - %{} = params - ) do + def delete(conn, params) do + user = conn.assigns.current_user + site = conn.assigns.site + site_role = conn.assigns.site_role annotation_id = normalize_annotation_id_param(params["annotation_id"]) - case Annotations.delete_one(user_id, site, site_role, annotation_id) do + case Annotations.delete_one(user, site, site_role, annotation_id) do {:error, :not_enough_permissions} -> H.not_enough_permissions(conn, "Not enough permissions to delete annotation") @@ -167,8 +133,6 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsController do end end - def delete(%Plug.Conn{} = conn, _params), do: invalid_request(conn) - @spec normalize_annotation_id_param(any()) :: nil | pos_integer() defp normalize_annotation_id_param(input) do case Integer.parse(input) do @@ -180,8 +144,4 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsController do defp annotation_not_found(%Plug.Conn{} = conn, annotation_id_param) do H.not_found(conn, "Annotation not found with ID #{inspect(annotation_id_param)}") end - - defp invalid_request(%Plug.Conn{} = conn) do - H.bad_request(conn, "Invalid request") - end end From b4615bec8d866f8879a4f32e3da0174bc2a45429 Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Mon, 6 Jul 2026 15:56:36 +0200 Subject: [PATCH 08/23] Refactor permission checking and remove arg matches in typed API funcs --- lib/plausible/annotations/annotations.ex | 59 +++++++++--------------- 1 file changed, 21 insertions(+), 38 deletions(-) diff --git a/lib/plausible/annotations/annotations.ex b/lib/plausible/annotations/annotations.ex index 5e47ce9164fb..00cd18cf5cda 100644 --- a/lib/plausible/annotations/annotations.ex +++ b/lib/plausible/annotations/annotations.ex @@ -21,12 +21,7 @@ defmodule Plausible.Annotations do @spec get_all_for_site(Plausible.Site.t(), atom(), User.t() | nil, DateTimeRange.t()) :: {:error, :not_enough_permissions} | {:ok, list(Annotation.t())} - def get_all_for_site( - %Plausible.Site{} = site, - site_role, - user, - %DateTimeRange{} = range_in_site_tz - ) do + def get_all_for_site(site, site_role, user, range_in_site_tz) do # Minute granularity annotations are stored for the particular UTC moment they're for, # so they must be in the range of the UTC query period. minute_granularity_range = @@ -116,21 +111,17 @@ defmodule Plausible.Annotations do | error_invalid_annotation() | error_annotation_limit_reached() | unknown_error() - - def insert_one( - user, - %Plausible.Site{} = site, - site_role, - %{} = params - ) do + def insert_one(user, site, site_role, params) do params = maybe_coerce_naive_datetime(params, site.timezone) - with :ok <- can_insert_one?(site, site_role, params), - %{valid?: true} = changeset <- Annotation.create_changeset(params, site, user) do - {:ok, - changeset |> Repo.insert!() |> Repo.preload(:owner) |> localize_annotation(site.timezone)} + changeset = Annotation.create_changeset(params, site, user) + annotation_type = Ecto.Changeset.get_field(changeset, :type) + + with :ok <- can_insert_one?(site, site_role, annotation_type), + {:ok, annotation} <- Repo.insert(changeset) do + {:ok, annotation |> Repo.preload(:owner) |> localize_annotation(site.timezone)} else - %{valid?: false, errors: errors} -> + {:error, %Ecto.Changeset{errors: errors}} -> {:error, {:invalid_annotation, errors}} {:error, _type} = error -> @@ -143,25 +134,17 @@ defmodule Plausible.Annotations do | error_not_enough_permissions() | error_invalid_annotation() | unknown_error() - - def update_one( - user, - %Plausible.Site{} = site, - site_role, - annotation_id, - %{} = params - ) do + def update_one(user, site, site_role, annotation_id, params) do params = maybe_coerce_naive_datetime(params, site.timezone) with {:ok, annotation} <- get_one(user, site, site_role, annotation_id), - :ok <- can_update_one?(site, site_role, params, annotation.type), - %{valid?: true} = changeset <- Annotation.update_changeset(annotation, params, user) do - Repo.update!(changeset) - - {:ok, - Repo.reload!(annotation) |> Repo.preload(:owner) |> localize_annotation(site.timezone)} + changeset = Annotation.update_changeset(annotation, params, user), + new_annotation_type = Ecto.Changeset.get_field(changeset, :type), + :ok <- can_update_one?(site, site_role, new_annotation_type, annotation.type), + {:ok, annotation} <- Repo.update(changeset) do + {:ok, annotation |> Repo.preload(:owner) |> localize_annotation(site.timezone)} else - %{valid?: false, errors: errors} -> + {:error, %Ecto.Changeset{errors: errors}} -> {:error, {:invalid_annotation, errors}} {:error, _type} = error -> @@ -271,8 +254,8 @@ defmodule Plausible.Annotations do annotation end - defp can_update_one?(%Plausible.Site{} = site, site_role, params, existing_annotation_type) do - updating_to_site_annotation? = params["type"] == "site" + defp can_update_one?(site, site_role, new_annotation_type, existing_annotation_type) do + updating_to_site_annotation? = new_annotation_type == :site cond do (existing_annotation_type == :site or @@ -289,16 +272,16 @@ defmodule Plausible.Annotations do end end - defp can_insert_one?(%Plausible.Site{} = site, site_role, params) do + defp can_insert_one?(site, site_role, annotation_type) do cond do count_annotations(site.id) >= @max_annotations -> {:error, :annotations_limit_reached} - params["type"] == "site" and site_role in roles_with_maybe_site_annotations() and + annotation_type == :site and site_role in roles_with_maybe_site_annotations() and site_annotations_available?(site) -> :ok - params["type"] == "personal" and + annotation_type == :personal and site_role in roles_with_personal_annotations() -> :ok From f0dc0f4d8fb571d92c6d0280811b0d3bc6b90521 Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Mon, 6 Jul 2026 16:44:52 +0200 Subject: [PATCH 09/23] Move localising annotation to JSON serialisation stage --- lib/plausible/annotations/annotation.ex | 29 +++++++++++++-- lib/plausible/annotations/annotations.ex | 45 +++++++++--------------- 2 files changed, 42 insertions(+), 32 deletions(-) diff --git a/lib/plausible/annotations/annotation.ex b/lib/plausible/annotations/annotation.ex index a0e89ff889b1..bab47f44ca77 100644 --- a/lib/plausible/annotations/annotation.ex +++ b/lib/plausible/annotations/annotation.ex @@ -138,12 +138,34 @@ defmodule Plausible.Annotations.Annotation do def serialize_date_granularity_datetime(%Date{} = date), do: DateTime.new!(date, ~T[00:00:00], "Etc/UTC") - def parse_date_granularity_datetime(%DateTime{} = datetime), + defp parse_date_granularity_datetime(%DateTime{} = datetime), do: DateTime.to_date(datetime) + + # Used only by encoder + @doc false + def localize(annotation, timezone) + + # For date granularity, the UTC date component IS the annotation date — callers + # store UTC midnight of their intended local date, so no timezone shift is needed. + # Return just the Date so the JSON response matches the bare-date input format. + def localize(%{granularity: :date} = annotation, _timezone) do + %{annotation | datetime: parse_date_granularity_datetime(annotation.datetime)} + end + + # For minute granularity, shift the stored UTC moment to the site's local timezone + # and strip the offset so the response is a naive local time string. + def localize(%{granularity: :minute} = annotation, timezone) do + naive_local = + annotation.datetime + |> DateTime.shift_zone!(timezone) + |> DateTime.to_naive() + + %{annotation | datetime: naive_local} + end end defimpl Jason.Encoder, for: Plausible.Annotations.Annotation do - def encode(%Plausible.Annotations.Annotation{} = annotation, opts) do + def encode(annotation, opts) do %{ id: annotation.id, note: annotation.note, @@ -151,10 +173,11 @@ defimpl Jason.Encoder, for: Plausible.Annotations.Annotation do datetime: annotation.datetime, granularity: annotation.granularity, owner_id: annotation.owner_id, - owner_name: if(is_nil(annotation.owner_id), do: nil, else: annotation.owner.name), + owner_name: if(annotation.owner_id, do: annotation.owner.name), inserted_at: annotation.inserted_at, updated_at: annotation.updated_at } + |> Plausible.Annotations.Annotation.localize(annotation.site.timezone) |> Jason.Encode.map(opts) end end diff --git a/lib/plausible/annotations/annotations.ex b/lib/plausible/annotations/annotations.ex index 00cd18cf5cda..9f68299a6c42 100644 --- a/lib/plausible/annotations/annotations.ex +++ b/lib/plausible/annotations/annotations.ex @@ -35,7 +35,7 @@ defmodule Plausible.Annotations do utc_datetime |> DateTime.to_date() |> Annotation.serialize_date_granularity_datetime() end) - fields = [:id, :note, :type, :datetime, :granularity, :inserted_at, :updated_at] + fields = [:id, :note, :type, :datetime, :granularity, :site_id, :inserted_at, :updated_at] in_range_clause = dynamic( @@ -55,15 +55,17 @@ defmodule Plausible.Annotations do annotations = Repo.all( from(annotation in Annotation, + inner_join: site in assoc(annotation, :site), select: ^fields, where: annotation.site_id == ^site.id, where: annotation.type == :site, where: ^in_range_clause, - order_by: [desc: annotation.updated_at, desc: annotation.id] + order_by: [desc: annotation.updated_at, desc: annotation.id], + preload: [site: site] ) ) - {:ok, Enum.map(annotations, &localize_annotation(&1, site.timezone))} + {:ok, annotations} site_role in roles_with_personal_annotations() or site_role in roles_with_maybe_site_annotations() -> @@ -72,6 +74,8 @@ defmodule Plausible.Annotations do annotations = Repo.all( from(annotation in Annotation, + inner_join: site in assoc(annotation, :site), + inner_join: owner in assoc(annotation, :owner), select: ^fields, where: annotation.site_id == ^site.id, where: @@ -79,11 +83,11 @@ defmodule Plausible.Annotations do (annotation.type == :personal and annotation.owner_id == ^user.id), where: ^in_range_clause, order_by: [desc: annotation.updated_at, desc: annotation.id], - preload: [:owner] + preload: [site: site, owner: owner] ) ) - {:ok, Enum.map(annotations, &localize_annotation(&1, site.timezone))} + {:ok, annotations} true -> {:error, :not_enough_permissions} @@ -119,7 +123,7 @@ defmodule Plausible.Annotations do with :ok <- can_insert_one?(site, site_role, annotation_type), {:ok, annotation} <- Repo.insert(changeset) do - {:ok, annotation |> Repo.preload(:owner) |> localize_annotation(site.timezone)} + {:ok, Repo.preload(annotation, [:site, :owner])} else {:error, %Ecto.Changeset{errors: errors}} -> {:error, {:invalid_annotation, errors}} @@ -142,7 +146,7 @@ defmodule Plausible.Annotations do new_annotation_type = Ecto.Changeset.get_field(changeset, :type), :ok <- can_update_one?(site, site_role, new_annotation_type, annotation.type), {:ok, annotation} <- Repo.update(changeset) do - {:ok, annotation |> Repo.preload(:owner) |> localize_annotation(site.timezone)} + {:ok, Repo.preload(annotation, [:site, :owner])} else {:error, %Ecto.Changeset{errors: errors}} -> {:error, {:invalid_annotation, errors}} @@ -217,10 +221,10 @@ defmodule Plausible.Annotations do with {:ok, annotation} <- get_one(user, site, site_role, annotation_id) do cond do annotation.type == :site and site_role in roles_with_maybe_site_annotations() -> - {:ok, do_delete_one(annotation) |> localize_annotation(site.timezone)} + {:ok, do_delete_one(annotation)} annotation.type == :personal and site_role in roles_with_personal_annotations() -> - {:ok, do_delete_one(annotation) |> localize_annotation(site.timezone)} + {:ok, do_delete_one(annotation)} true -> {:error, :not_enough_permissions} @@ -243,15 +247,16 @@ defmodule Plausible.Annotations do where: annotation.type == :site or (annotation.type == :personal and annotation.owner_id == ^user.id), - preload: [:owner] + preload: [:site, :owner] ) Repo.one(query) end defp do_delete_one(annotation) do - Repo.delete!(annotation) annotation + |> Repo.preload([:site, :owner]) + |> Repo.delete!() end defp can_update_one?(site, site_role, new_annotation_type, existing_annotation_type) do @@ -304,24 +309,6 @@ defmodule Plausible.Annotations do # this feature is bundled with SiteSegments do: Plausible.Billing.Feature.SiteSegments.check_availability(site.team) == :ok - # For date granularity, the UTC date component IS the annotation date — callers - # store UTC midnight of their intended local date, so no timezone shift is needed. - # Return just the Date so the JSON response matches the bare-date input format. - defp localize_annotation(%Annotation{granularity: :date} = annotation, _timezone) do - %{annotation | datetime: Annotation.parse_date_granularity_datetime(annotation.datetime)} - end - - # For minute granularity, shift the stored UTC moment to the site's local timezone - # and strip the offset so the response is a naive local time string. - defp localize_annotation(%Annotation{granularity: :minute} = annotation, timezone) do - naive_local = - annotation.datetime - |> DateTime.shift_zone!(timezone) - |> DateTime.to_naive() - - %{annotation | datetime: naive_local} - end - # If `datetime` is a naive ISO 8601 string (no UTC offset or Z suffix), interpret # it as a local time in the site's timezone and convert to UTC before the changeset # runs. This lets callers supply times in their local context without manually From 70cd3e603b3bc3e6d543876451952db6969d61d5 Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Mon, 6 Jul 2026 19:46:34 +0200 Subject: [PATCH 10/23] Refactor annotations to use different time fields depending on granularity --- lib/plausible/annotations/annotation.ex | 96 +++++++------------ .../plausible/annotations/annotation_test.exs | 42 +++++--- .../annotations_controller_test.exs | 58 ++++++----- 3 files changed, 98 insertions(+), 98 deletions(-) diff --git a/lib/plausible/annotations/annotation.ex b/lib/plausible/annotations/annotation.ex index bab47f44ca77..a4fee2a37350 100644 --- a/lib/plausible/annotations/annotation.ex +++ b/lib/plausible/annotations/annotation.ex @@ -33,6 +33,7 @@ defmodule Plausible.Annotations.Annotation do schema "annotations" do field :note, :string field :type, Ecto.Enum, values: @annotation_types + field :date, :date, virtual: true field :datetime, :utc_datetime field :granularity, Ecto.Enum, values: @annotation_granularities @@ -58,89 +59,57 @@ defmodule Plausible.Annotations.Annotation do end def changeset(annotation, attrs) do - attrs = stringify_keys(attrs) - {attrs, invalid_datetime_for_granularity?} = coerce_datetime(attrs) - annotation - |> cast(attrs, [:note, :type, :datetime, :granularity]) - |> validate_required([:note, :type, :datetime, :granularity]) + |> cast(attrs, [:note, :type]) + |> cast(attrs, [:date, :datetime, :granularity], force_changes: true) + |> validate_required([:note, :type, :granularity]) |> validate_length(:note, count: :bytes, min: 1, max: 255) - |> validate_datetime_supplied_on_granularity_change() - |> maybe_add_datetime_error(invalid_datetime_for_granularity?) - end - - defp stringify_keys(%{} = params) do - Map.new(params, fn - {k, v} when is_atom(k) -> {Atom.to_string(k), v} - {k, v} when is_binary(k) -> {k, v} - end) + |> coerce_datetime() end - defp coerce_datetime(attrs) do - granularity = normalize_granularity(attrs["granularity"]) + defp coerce_datetime(%{valid?: true} = changeset) do + granularity = get_change(changeset, :granularity) + datetime = get_change(changeset, :datetime) + date = get_change(changeset, :date) - case coerce_for_granularity(granularity, attrs["datetime"]) do - {:ok, %DateTime{} = utc_dt} -> {Map.put(attrs, "datetime", utc_dt), false} - :skip -> {attrs, false} - :invalid -> {Map.delete(attrs, "datetime"), true} - end - end + case coerce_for_granularity(granularity, date, datetime) do + {:ok, %DateTime{} = utc_dt} -> + put_change(changeset, :datetime, utc_dt) - defp normalize_granularity(:date), do: :date - defp normalize_granularity("date"), do: :date - defp normalize_granularity(:minute), do: :minute - defp normalize_granularity("minute"), do: :minute - defp normalize_granularity(other), do: other + {:error, :not_supplied, field} -> + add_error(changeset, field, "must be supplied for chosen granularity") - # nil datetime will be caught by validate_required step - defp coerce_for_granularity(_granularity, nil), do: :skip + {:error, :both_set} -> + add_error(changeset, :granularity, "expects either date or datetime to be set") - defp coerce_for_granularity(:date, %Date{} = date), - do: {:ok, serialize_date_granularity_datetime(date)} - - defp coerce_for_granularity(:date, <<_::binary-size(10)>> = str) do - case Date.from_iso8601(str) do - {:ok, date} -> {:ok, serialize_date_granularity_datetime(date)} - _ -> :invalid + :skip -> + changeset end end - defp coerce_for_granularity(:minute, %DateTime{} = dt), - do: {:ok, DateTime.shift_zone!(dt, "Etc/UTC")} + defp coerce_datetime(changeset), do: changeset - defp coerce_for_granularity(:minute, str) when is_binary(str) do - case DateTime.from_iso8601(str) do - {:ok, dt, _offset} -> {:ok, DateTime.shift_zone!(dt, "Etc/UTC")} - _ -> :invalid - end - end + defp coerce_for_granularity(:date, %Date{} = date, nil), + do: {:ok, serialize_date_granularity_datetime(date)} - defp coerce_for_granularity(granularity, _datetime) when granularity in [:date, :minute], - do: :invalid + defp coerce_for_granularity(:minute, nil, %DateTime{} = dt), + do: {:ok, dt} - defp coerce_for_granularity(_granularity, _datetime), do: :skip + defp coerce_for_granularity(:date, nil, _), + do: {:error, :not_supplied, :date} - defp maybe_add_datetime_error(changeset, false), do: changeset + defp coerce_for_granularity(:minute, _, nil), + do: {:error, :not_supplied, :datetime} - defp maybe_add_datetime_error(changeset, true), - do: add_error(changeset, :datetime, "is invalid for granularity") + defp coerce_for_granularity(granularity, _date, _datetime) + when granularity in [:date, :minute], + do: {:error, :both_set} - defp validate_datetime_supplied_on_granularity_change(changeset) do - with {:ok, _new_granularity} <- fetch_change(changeset, :granularity), - false <- is_nil(changeset.data.granularity), - :error <- fetch_change(changeset, :datetime) do - add_error(changeset, :datetime, "must be supplied when granularity changes") - else - _ -> changeset - end - end + defp coerce_for_granularity(_granularity, _date, _datetime), do: :skip def serialize_date_granularity_datetime(%Date{} = date), do: DateTime.new!(date, ~T[00:00:00], "Etc/UTC") - defp parse_date_granularity_datetime(%DateTime{} = datetime), - do: DateTime.to_date(datetime) - # Used only by encoder @doc false def localize(annotation, timezone) @@ -162,6 +131,9 @@ defmodule Plausible.Annotations.Annotation do %{annotation | datetime: naive_local} end + + defp parse_date_granularity_datetime(%DateTime{} = datetime), + do: DateTime.to_date(datetime) end defimpl Jason.Encoder, for: Plausible.Annotations.Annotation do diff --git a/test/plausible/annotations/annotation_test.exs b/test/plausible/annotations/annotation_test.exs index f41fb952d254..ccc77a7fff84 100644 --- a/test/plausible/annotations/annotation_test.exs +++ b/test/plausible/annotations/annotation_test.exs @@ -6,21 +6,25 @@ defmodule Plausible.Annotations.AnnotationTest do test "a full datetime string is rejected" do changeset = Annotation.changeset(%Annotation{}, %{ + "note" => "test", + "type" => "personal", "granularity" => "date", "datetime" => "2026-06-30T00:00:00Z" }) - assert {"is invalid for granularity", []} = changeset.errors[:datetime] + assert {"must be supplied for chosen granularity", []} = changeset.errors[:date] end test "a non-midnight %DateTime{} is rejected" do changeset = Annotation.changeset(%Annotation{}, %{ + "note" => "test", + "type" => "personal", "granularity" => "date", "datetime" => ~U[2026-06-30 14:30:00Z] }) - assert {"is invalid for granularity", []} = changeset.errors[:datetime] + assert {"must be supplied for chosen granularity", []} = changeset.errors[:date] end test "an unparsable date string is rejected" do @@ -30,7 +34,7 @@ defmodule Plausible.Annotations.AnnotationTest do "datetime" => "not-a-date" }) - assert {"is invalid for granularity", []} = changeset.errors[:datetime] + assert {"is invalid", _} = changeset.errors[:datetime] end test "an invalid calendar date is rejected" do @@ -40,7 +44,7 @@ defmodule Plausible.Annotations.AnnotationTest do "datetime" => "2026-13-45" }) - assert {"is invalid for granularity", []} = changeset.errors[:datetime] + assert {"is invalid", _} = changeset.errors[:datetime] end test "a bare YYYY-MM-DD string becomes UTC midnight" do @@ -51,7 +55,7 @@ defmodule Plausible.Annotations.AnnotationTest do "site_id" => 1, "owner_id" => 1, "granularity" => "date", - "datetime" => "2026-06-30" + "date" => "2026-06-30" }) assert changeset.valid? @@ -66,7 +70,7 @@ defmodule Plausible.Annotations.AnnotationTest do "site_id" => 1, "owner_id" => 1, "granularity" => "date", - "datetime" => ~D[2026-06-30] + "date" => ~D[2026-06-30] }) assert changeset.valid? @@ -82,7 +86,7 @@ defmodule Plausible.Annotations.AnnotationTest do "datetime" => "2026-06-30" }) - assert {"is invalid for granularity", []} = changeset.errors[:datetime] + assert {"is invalid", _} = changeset.errors[:datetime] end test "an unparsable datetime is rejected" do @@ -92,7 +96,7 @@ defmodule Plausible.Annotations.AnnotationTest do "datetime" => "garbage" }) - assert {"is invalid for granularity", []} = changeset.errors[:datetime] + assert {"is invalid", _} = changeset.errors[:datetime] end test "a Z-suffixed datetime string is kept in UTC" do @@ -156,17 +160,29 @@ defmodule Plausible.Annotations.AnnotationTest do describe "granularity change" do test "flipping :date to :minute without a datetime is rejected" do - existing = %Annotation{granularity: :date, datetime: ~U[2026-06-15 00:00:00Z]} + existing = %Annotation{ + granularity: :date, + datetime: ~U[2026-06-15 00:00:00Z], + note: "test", + type: :personal + } + changeset = Annotation.changeset(existing, %{"granularity" => "minute"}) - assert {"must be supplied when granularity changes", []} = changeset.errors[:datetime] + assert {"must be supplied for chosen granularity", []} = changeset.errors[:datetime] end test "flipping :minute to :date without a datetime is rejected" do - existing = %Annotation{granularity: :minute, datetime: ~U[2026-06-15 14:30:00Z]} + existing = %Annotation{ + granularity: :minute, + datetime: ~U[2026-06-15 14:30:00Z], + note: "test", + type: :personal + } + changeset = Annotation.changeset(existing, %{"granularity" => "date"}) - assert {"must be supplied when granularity changes", []} = changeset.errors[:datetime] + assert {"must be supplied for chosen granularity", []} = changeset.errors[:date] end test "flipping granularity to minute with an appropriate datetime is accepted" do @@ -202,7 +218,7 @@ defmodule Plausible.Annotations.AnnotationTest do changeset = Annotation.changeset(existing, %{ "granularity" => "date", - "datetime" => "2026-06-16" + "date" => "2026-06-16" }) assert changeset.valid? diff --git a/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs b/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs index 1fc48c6e70e0..c3c06b95bf14 100644 --- a/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs +++ b/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs @@ -279,7 +279,7 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "note" => "feature released", "type" => "personal", "granularity" => "date", - "datetime" => "2026-01-04" + "date" => "2026-01-04" }) |> json_response(200) @@ -306,7 +306,9 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "datetime" => "2026-01-04T00:00:00Z" }) - assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + assert json_response(conn, 400) == %{ + "error" => "date must be supplied for chosen granularity" + } end test "accepts full datetime string when granularity is minute", @@ -334,7 +336,7 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "datetime" => "2026-01-04" }) - assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + assert json_response(conn, 400) == %{"error" => "datetime is invalid"} end test "rejects invalid calendar date when granularity is date", @@ -344,10 +346,10 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "note" => "feature released", "type" => "personal", "granularity" => "date", - "datetime" => "2026-13-45" + "date" => "2026-13-45" }) - assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + assert json_response(conn, 400) == %{"error" => "date is invalid"} end test "rejects non-date string when granularity is date", @@ -357,10 +359,10 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "note" => "feature released", "type" => "personal", "granularity" => "date", - "datetime" => "not-a-date" + "date" => "not-a-date" }) - assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + assert json_response(conn, 400) == %{"error" => "date is invalid"} end test "rejects non-datetime string when granularity is minute", @@ -373,7 +375,7 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "datetime" => "not-a-datetime" }) - assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + assert json_response(conn, 400) == %{"error" => "datetime is invalid"} end test "rejects empty datetime string when granularity is date", @@ -383,10 +385,12 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "note" => "feature released", "type" => "personal", "granularity" => "date", - "datetime" => "" + "date" => "" }) - assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + assert json_response(conn, 400) == %{ + "error" => "date must be supplied for chosen granularity" + } end test "rejects empty datetime string when granularity is minute", @@ -399,7 +403,9 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "datetime" => "" }) - assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + assert json_response(conn, 400) == %{ + "error" => "datetime must be supplied for chosen granularity" + } end test "rejects date shorter than 10 characters when granularity is date", @@ -409,10 +415,10 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "note" => "feature released", "type" => "personal", "granularity" => "date", - "datetime" => "2026-1-4" + "date" => "2026-1-4" }) - assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + assert json_response(conn, 400) == %{"error" => "date is invalid"} end test "rejects invalid calendar datetime when granularity is minute", @@ -425,7 +431,7 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "datetime" => "2026-13-45T14:30:00Z" }) - assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + assert json_response(conn, 400) == %{"error" => "datetime is invalid"} end end @@ -462,7 +468,9 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "datetime" => "2026-01-04T00:00:00" }) - assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + assert json_response(conn, 400) == %{ + "error" => "date must be supplied for chosen granularity" + } end test "UTC datetime string is stored as UTC and returned as site local time", @@ -528,7 +536,7 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do response = patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ "granularity" => "date", - "datetime" => "2026-06-15" + "date" => "2026-06-15" }) |> json_response(200) @@ -552,7 +560,7 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "datetime" => "2026-06-15" }) - assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + assert json_response(conn, 400) == %{"error" => "datetime is invalid"} end test "rejects full datetime string when granularity switched to date", @@ -572,7 +580,9 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "datetime" => "2026-06-15T10:00:00Z" }) - assert json_response(conn, 400) == %{"error" => "datetime is invalid for granularity"} + assert json_response(conn, 400) == %{ + "error" => "date must be supplied for chosen granularity" + } end test "rejects granularity change from date to minute without a new datetime", @@ -592,7 +602,7 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do }) assert json_response(conn, 400) == %{ - "error" => "datetime must be supplied when granularity changes" + "error" => "datetime must be supplied for chosen granularity" } reloaded = Plausible.Repo.get!(Plausible.Annotations.Annotation, annotation.id) @@ -617,7 +627,7 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do }) assert json_response(conn, 400) == %{ - "error" => "datetime must be supplied when granularity changes" + "error" => "date must be supplied for chosen granularity" } reloaded = Plausible.Repo.get!(Plausible.Annotations.Annotation, annotation.id) @@ -665,7 +675,7 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do response = patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ "granularity" => "date", - "datetime" => "2026-06-20" + "date" => "2026-06-20" }) |> json_response(200) @@ -722,7 +732,7 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "note" => String.duplicate("a", 255), "type" => "personal", "granularity" => "date", - "datetime" => "2026-01-04" + "date" => "2026-01-04" }) |> json_response(200) @@ -737,7 +747,9 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do "granularity" => "date" }) - assert json_response(conn, 400) == %{"error" => "datetime can't be blank"} + assert json_response(conn, 400) == %{ + "error" => "date must be supplied for chosen granularity" + } end test "rejects unknown granularity", %{conn: conn, site: site} do From 7d03e2970719cafc35c737587d4c350e3b2f1cdf Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Mon, 6 Jul 2026 20:09:32 +0200 Subject: [PATCH 11/23] Move naive datetime coercion inside changeset --- lib/plausible/annotations/annotation.ex | 36 ++- lib/plausible/annotations/annotations.ex | 34 --- .../plausible/annotations/annotation_test.exs | 220 +++++++++++------- 3 files changed, 171 insertions(+), 119 deletions(-) diff --git a/lib/plausible/annotations/annotation.ex b/lib/plausible/annotations/annotation.ex index a4fee2a37350..234aa2b02019 100644 --- a/lib/plausible/annotations/annotation.ex +++ b/lib/plausible/annotations/annotation.ex @@ -47,26 +47,56 @@ defmodule Plausible.Annotations.Annotation do def create_changeset(attrs, site, owner) do %__MODULE__{} - |> changeset(attrs) + |> changeset(attrs, site.timezone) |> put_assoc(:site, site) |> put_assoc(:owner, owner) end def update_changeset(annotation, attrs, owner) do annotation - |> changeset(attrs) + |> changeset(attrs, annotation.site.timezone) |> put_assoc(:owner, owner) end - def changeset(annotation, attrs) do + def changeset(annotation, attrs, site_timezone) do annotation |> cast(attrs, [:note, :type]) |> cast(attrs, [:date, :datetime, :granularity], force_changes: true) |> validate_required([:note, :type, :granularity]) |> validate_length(:note, count: :bytes, min: 1, max: 255) + |> maybe_coerce_naive_datetime(site_timezone) |> coerce_datetime() end + # If `datetime` is a naive ISO 8601 string (no UTC offset or Z suffix), interpret + # it as a local time in the site's timezone and convert to UTC before the changeset + # runs. This lets callers supply times in their local context without manually + # computing offsets. + # + # DST edge cases: + # - gap (spring-forward): the missing hour is resolved to just-after the gap + # - ambiguous (fall-back): the earlier of the two possibilities is used + # + # All other `datetime` values (bare dates, full UTC strings, invalid strings) pass + # through unchanged and are handled downstream by the changeset. + defp maybe_coerce_naive_datetime(changeset, timezone) do + with false <- is_nil(get_change(changeset, :datetime)), + dt when is_binary(dt) <- changeset.params["datetime"], + {:error, _} <- DateTime.from_iso8601(dt), + {:ok, naive_dt} <- NaiveDateTime.from_iso8601(dt) do + utc_dt = + case DateTime.from_naive(naive_dt, timezone) do + {:ok, local_dt} -> DateTime.shift_zone!(local_dt, "Etc/UTC") + {:ambiguous, first, _second} -> DateTime.shift_zone!(first, "Etc/UTC") + {:gap, _just_before, just_after} -> DateTime.shift_zone!(just_after, "Etc/UTC") + end + + force_change(changeset, :datetime, utc_dt) + else + _ -> changeset + end + end + defp coerce_datetime(%{valid?: true} = changeset) do granularity = get_change(changeset, :granularity) datetime = get_change(changeset, :datetime) diff --git a/lib/plausible/annotations/annotations.ex b/lib/plausible/annotations/annotations.ex index 9f68299a6c42..1d9e6148621a 100644 --- a/lib/plausible/annotations/annotations.ex +++ b/lib/plausible/annotations/annotations.ex @@ -116,8 +116,6 @@ defmodule Plausible.Annotations do | error_annotation_limit_reached() | unknown_error() def insert_one(user, site, site_role, params) do - params = maybe_coerce_naive_datetime(params, site.timezone) - changeset = Annotation.create_changeset(params, site, user) annotation_type = Ecto.Changeset.get_field(changeset, :type) @@ -139,8 +137,6 @@ defmodule Plausible.Annotations do | error_invalid_annotation() | unknown_error() def update_one(user, site, site_role, annotation_id, params) do - params = maybe_coerce_naive_datetime(params, site.timezone) - with {:ok, annotation} <- get_one(user, site, site_role, annotation_id), changeset = Annotation.update_changeset(annotation, params, user), new_annotation_type = Ecto.Changeset.get_field(changeset, :type), @@ -308,34 +304,4 @@ defmodule Plausible.Annotations do def site_annotations_available?(%Plausible.Site{} = site), # this feature is bundled with SiteSegments do: Plausible.Billing.Feature.SiteSegments.check_availability(site.team) == :ok - - # If `datetime` is a naive ISO 8601 string (no UTC offset or Z suffix), interpret - # it as a local time in the site's timezone and convert to UTC before the changeset - # runs. This lets callers supply times in their local context without manually - # computing offsets. - # - # DST edge cases: - # - gap (spring-forward): the missing hour is resolved to just-after the gap - # - ambiguous (fall-back): the earlier of the two possibilities is used - # - # All other `datetime` values (bare dates, full UTC strings, invalid strings) pass - # through unchanged and are handled downstream by the changeset. - defp maybe_coerce_naive_datetime(%{"datetime" => dt} = params, timezone) - when is_binary(dt) do - with {:error, _} <- DateTime.from_iso8601(dt), - {:ok, naive_dt} <- NaiveDateTime.from_iso8601(dt) do - utc_dt = - case DateTime.from_naive(naive_dt, timezone) do - {:ok, local_dt} -> DateTime.shift_zone!(local_dt, "Etc/UTC") - {:ambiguous, first, _second} -> DateTime.shift_zone!(first, "Etc/UTC") - {:gap, _just_before, just_after} -> DateTime.shift_zone!(just_after, "Etc/UTC") - end - - Map.put(params, "datetime", utc_dt) - else - _ -> params - end - end - - defp maybe_coerce_naive_datetime(params, _timezone), do: params end diff --git a/test/plausible/annotations/annotation_test.exs b/test/plausible/annotations/annotation_test.exs index ccc77a7fff84..bb56d2fa06c9 100644 --- a/test/plausible/annotations/annotation_test.exs +++ b/test/plausible/annotations/annotation_test.exs @@ -5,58 +5,78 @@ defmodule Plausible.Annotations.AnnotationTest do describe "date-granularity" do test "a full datetime string is rejected" do changeset = - Annotation.changeset(%Annotation{}, %{ - "note" => "test", - "type" => "personal", - "granularity" => "date", - "datetime" => "2026-06-30T00:00:00Z" - }) + Annotation.changeset( + %Annotation{}, + %{ + "note" => "test", + "type" => "personal", + "granularity" => "date", + "datetime" => "2026-06-30T00:00:00Z" + }, + "Etc/UTC" + ) assert {"must be supplied for chosen granularity", []} = changeset.errors[:date] end test "a non-midnight %DateTime{} is rejected" do changeset = - Annotation.changeset(%Annotation{}, %{ - "note" => "test", - "type" => "personal", - "granularity" => "date", - "datetime" => ~U[2026-06-30 14:30:00Z] - }) + Annotation.changeset( + %Annotation{}, + %{ + "note" => "test", + "type" => "personal", + "granularity" => "date", + "datetime" => ~U[2026-06-30 14:30:00Z] + }, + "Etc/UTC" + ) assert {"must be supplied for chosen granularity", []} = changeset.errors[:date] end test "an unparsable date string is rejected" do changeset = - Annotation.changeset(%Annotation{}, %{ - "granularity" => "date", - "datetime" => "not-a-date" - }) + Annotation.changeset( + %Annotation{}, + %{ + "granularity" => "date", + "datetime" => "not-a-date" + }, + "Etc/UTC" + ) assert {"is invalid", _} = changeset.errors[:datetime] end test "an invalid calendar date is rejected" do changeset = - Annotation.changeset(%Annotation{}, %{ - "granularity" => "date", - "datetime" => "2026-13-45" - }) + Annotation.changeset( + %Annotation{}, + %{ + "granularity" => "date", + "datetime" => "2026-13-45" + }, + "Etc/UTC" + ) assert {"is invalid", _} = changeset.errors[:datetime] end test "a bare YYYY-MM-DD string becomes UTC midnight" do changeset = - Annotation.changeset(%Annotation{}, %{ - "note" => "feature released", - "type" => "personal", - "site_id" => 1, - "owner_id" => 1, - "granularity" => "date", - "date" => "2026-06-30" - }) + Annotation.changeset( + %Annotation{}, + %{ + "note" => "feature released", + "type" => "personal", + "site_id" => 1, + "owner_id" => 1, + "granularity" => "date", + "date" => "2026-06-30" + }, + "Etc/UTC" + ) assert changeset.valid? assert changeset.changes.datetime == ~U[2026-06-30 00:00:00Z] @@ -64,14 +84,18 @@ defmodule Plausible.Annotations.AnnotationTest do test "a %Date{} struct becomes UTC midnight" do changeset = - Annotation.changeset(%Annotation{}, %{ - "note" => "feature released", - "type" => "personal", - "site_id" => 1, - "owner_id" => 1, - "granularity" => "date", - "date" => ~D[2026-06-30] - }) + Annotation.changeset( + %Annotation{}, + %{ + "note" => "feature released", + "type" => "personal", + "site_id" => 1, + "owner_id" => 1, + "granularity" => "date", + "date" => ~D[2026-06-30] + }, + "Etc/UTC" + ) assert changeset.valid? assert changeset.changes.datetime == ~U[2026-06-30 00:00:00Z] @@ -81,34 +105,46 @@ defmodule Plausible.Annotations.AnnotationTest do describe "minute-granularity" do test "a bare date string is rejected" do changeset = - Annotation.changeset(%Annotation{}, %{ - "granularity" => "minute", - "datetime" => "2026-06-30" - }) + Annotation.changeset( + %Annotation{}, + %{ + "granularity" => "minute", + "datetime" => "2026-06-30" + }, + "Etc/UTC" + ) assert {"is invalid", _} = changeset.errors[:datetime] end test "an unparsable datetime is rejected" do changeset = - Annotation.changeset(%Annotation{}, %{ - "granularity" => "minute", - "datetime" => "garbage" - }) + Annotation.changeset( + %Annotation{}, + %{ + "granularity" => "minute", + "datetime" => "garbage" + }, + "Etc/UTC" + ) assert {"is invalid", _} = changeset.errors[:datetime] end test "a Z-suffixed datetime string is kept in UTC" do changeset = - Annotation.changeset(%Annotation{}, %{ - "note" => "feature released", - "type" => "personal", - "site_id" => 1, - "owner_id" => 1, - "granularity" => "minute", - "datetime" => "2026-06-30T14:30:00Z" - }) + Annotation.changeset( + %Annotation{}, + %{ + "note" => "feature released", + "type" => "personal", + "site_id" => 1, + "owner_id" => 1, + "granularity" => "minute", + "datetime" => "2026-06-30T14:30:00Z" + }, + "Etc/UTC" + ) assert changeset.valid? assert changeset.changes.datetime == ~U[2026-06-30 14:30:00Z] @@ -116,14 +152,18 @@ defmodule Plausible.Annotations.AnnotationTest do test "an offset datetime string is shifted to UTC" do changeset = - Annotation.changeset(%Annotation{}, %{ - "note" => "feature released", - "type" => "personal", - "site_id" => 1, - "owner_id" => 1, - "granularity" => "minute", - "datetime" => "2026-06-30T10:00:00-02:00" - }) + Annotation.changeset( + %Annotation{}, + %{ + "note" => "feature released", + "type" => "personal", + "site_id" => 1, + "owner_id" => 1, + "granularity" => "minute", + "datetime" => "2026-06-30T10:00:00-02:00" + }, + "Etc/UTC" + ) assert changeset.valid? assert changeset.changes.datetime == ~U[2026-06-30 12:00:00Z] @@ -131,14 +171,18 @@ defmodule Plausible.Annotations.AnnotationTest do test "a %DateTime{} is passed through in UTC" do changeset = - Annotation.changeset(%Annotation{}, %{ - "note" => "feature released", - "type" => "personal", - "site_id" => 1, - "owner_id" => 1, - "granularity" => "minute", - "datetime" => ~U[2026-06-30 14:30:00Z] - }) + Annotation.changeset( + %Annotation{}, + %{ + "note" => "feature released", + "type" => "personal", + "site_id" => 1, + "owner_id" => 1, + "granularity" => "minute", + "datetime" => ~U[2026-06-30 14:30:00Z] + }, + "Etc/UTC" + ) assert changeset.valid? assert changeset.changes.datetime == ~U[2026-06-30 14:30:00Z] @@ -148,10 +192,14 @@ defmodule Plausible.Annotations.AnnotationTest do describe "unknown granularity" do test "defers to Ecto's enum cast, not our coercion error" do changeset = - Annotation.changeset(%Annotation{}, %{ - "granularity" => "hour", - "datetime" => "2026-06-30T10:00:00Z" - }) + Annotation.changeset( + %Annotation{}, + %{ + "granularity" => "hour", + "datetime" => "2026-06-30T10:00:00Z" + }, + "Etc/UTC" + ) assert changeset.errors[:granularity] refute changeset.errors[:datetime] @@ -167,7 +215,7 @@ defmodule Plausible.Annotations.AnnotationTest do type: :personal } - changeset = Annotation.changeset(existing, %{"granularity" => "minute"}) + changeset = Annotation.changeset(existing, %{"granularity" => "minute"}, "Etc/UTC") assert {"must be supplied for chosen granularity", []} = changeset.errors[:datetime] end @@ -180,7 +228,7 @@ defmodule Plausible.Annotations.AnnotationTest do type: :personal } - changeset = Annotation.changeset(existing, %{"granularity" => "date"}) + changeset = Annotation.changeset(existing, %{"granularity" => "date"}, "Etc/UTC") assert {"must be supplied for chosen granularity", []} = changeset.errors[:date] end @@ -196,10 +244,14 @@ defmodule Plausible.Annotations.AnnotationTest do } changeset = - Annotation.changeset(existing, %{ - "granularity" => "minute", - "datetime" => "2026-06-15T14:30:00Z" - }) + Annotation.changeset( + existing, + %{ + "granularity" => "minute", + "datetime" => "2026-06-15T14:30:00Z" + }, + "Etc/UTC" + ) assert changeset.valid? assert changeset.changes.datetime == ~U[2026-06-15 14:30:00Z] @@ -216,10 +268,14 @@ defmodule Plausible.Annotations.AnnotationTest do } changeset = - Annotation.changeset(existing, %{ - "granularity" => "date", - "date" => "2026-06-16" - }) + Annotation.changeset( + existing, + %{ + "granularity" => "date", + "date" => "2026-06-16" + }, + "Etc/UTC" + ) assert changeset.valid? assert changeset.changes.datetime == ~U[2026-06-16 00:00:00Z] From 97abedf4b8625ef7416d9c79443e3182f015f6df Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Mon, 6 Jul 2026 20:15:49 +0200 Subject: [PATCH 12/23] Extract filtering by range to a function --- lib/plausible/annotations/annotations.ex | 57 ++++++++++++------------ 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/lib/plausible/annotations/annotations.ex b/lib/plausible/annotations/annotations.ex index 1d9e6148621a..4d5f557b105b 100644 --- a/lib/plausible/annotations/annotations.ex +++ b/lib/plausible/annotations/annotations.ex @@ -22,34 +22,8 @@ defmodule Plausible.Annotations do @spec get_all_for_site(Plausible.Site.t(), atom(), User.t() | nil, DateTimeRange.t()) :: {:error, :not_enough_permissions} | {:ok, list(Annotation.t())} def get_all_for_site(site, site_role, user, range_in_site_tz) do - # Minute granularity annotations are stored for the particular UTC moment they're for, - # so they must be in the range of the UTC query period. - minute_granularity_range = - range_in_site_tz |> DateTimeRange.to_timezone("Etc/UTC") - - # Date granularity annotations are stored for the UTC midnight of the date they're for, - # so the range for querying these must reflect that. - [date_granularity_range_first, date_granularity_range_last] = - [range_in_site_tz.first, range_in_site_tz.last] - |> Enum.map(fn utc_datetime -> - utc_datetime |> DateTime.to_date() |> Annotation.serialize_date_granularity_datetime() - end) - fields = [:id, :note, :type, :datetime, :granularity, :site_id, :inserted_at, :updated_at] - in_range_clause = - dynamic( - [annotation], - (annotation.granularity == :minute and - annotation.datetime >= ^minute_granularity_range.first and - annotation.datetime <= ^minute_granularity_range.last) or - (annotation.granularity == :date and - annotation.datetime >= - ^date_granularity_range_first and - annotation.datetime <= - ^date_granularity_range_last) - ) - cond do site_role in [:public] -> annotations = @@ -59,10 +33,10 @@ defmodule Plausible.Annotations do select: ^fields, where: annotation.site_id == ^site.id, where: annotation.type == :site, - where: ^in_range_clause, order_by: [desc: annotation.updated_at, desc: annotation.id], preload: [site: site] ) + |> filter_by_range(range_in_site_tz) ) {:ok, annotations} @@ -81,10 +55,10 @@ defmodule Plausible.Annotations do where: annotation.type == :site or (annotation.type == :personal and annotation.owner_id == ^user.id), - where: ^in_range_clause, order_by: [desc: annotation.updated_at, desc: annotation.id], preload: [site: site, owner: owner] ) + |> filter_by_range(range_in_site_tz) ) {:ok, annotations} @@ -94,6 +68,33 @@ defmodule Plausible.Annotations do end end + defp filter_by_range(query, range_in_site_tz) do + # Minute granularity annotations are stored for the particular UTC moment they're for, + # so they must be in the range of the UTC query period. + minute_granularity_range = + range_in_site_tz |> DateTimeRange.to_timezone("Etc/UTC") + + # Date granularity annotations are stored for the UTC midnight of the date they're for, + # so the range for querying these must reflect that. + [date_granularity_range_first, date_granularity_range_last] = + [range_in_site_tz.first, range_in_site_tz.last] + |> Enum.map(fn utc_datetime -> + utc_datetime |> DateTime.to_date() |> Annotation.serialize_date_granularity_datetime() + end) + + from(annotation in query, + where: + (annotation.granularity == :minute and + annotation.datetime >= ^minute_granularity_range.first and + annotation.datetime <= ^minute_granularity_range.last) or + (annotation.granularity == :date and + annotation.datetime >= + ^date_granularity_range_first and + annotation.datetime <= + ^date_granularity_range_last) + ) + end + @spec get_one(User.t(), Plausible.Site.t(), atom(), pos_integer() | nil) :: {:ok, Annotation.t()} | error_not_enough_permissions() From 3881fae818839e9ac1824844226c74ba26b4592a Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Mon, 6 Jul 2026 20:26:44 +0200 Subject: [PATCH 13/23] Refactor and fix error typespecs --- lib/plausible/annotations/annotations.ex | 29 ++++++++++++------------ 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/lib/plausible/annotations/annotations.ex b/lib/plausible/annotations/annotations.ex index 4d5f557b105b..776f1eb5e36f 100644 --- a/lib/plausible/annotations/annotations.ex +++ b/lib/plausible/annotations/annotations.ex @@ -11,16 +11,15 @@ defmodule Plausible.Annotations do @roles_with_personal_annotations [:billing, :viewer, :editor, :admin, :owner, :super_admin] @roles_with_maybe_site_annotations [:editor, :admin, :owner, :super_admin] - @type error_not_enough_permissions() :: {:error, :not_enough_permissions} - @type error_annotation_not_found() :: {:error, :annotation_not_found} - @type error_annotation_limit_reached() :: {:error, :annotations_limit_reached} - @type error_invalid_annotation() :: {:error, {:invalid_annotation, Keyword.t()}} - @type unknown_error() :: {:error, any()} + @type error_not_enough_permissions() :: :not_enough_permissions + @type error_annotation_not_found() :: :annotation_not_found + @type error_annotation_limit_reached() :: :annotations_limit_reached + @type error_invalid_annotation() :: {:invalid_annotation, Keyword.t()} @max_annotations 500 @spec get_all_for_site(Plausible.Site.t(), atom(), User.t() | nil, DateTimeRange.t()) :: - {:error, :not_enough_permissions} | {:ok, list(Annotation.t())} + {:error, error_not_enough_permissions()} | {:ok, list(Annotation.t())} def get_all_for_site(site, site_role, user, range_in_site_tz) do fields = [:id, :note, :type, :datetime, :granularity, :site_id, :inserted_at, :updated_at] @@ -97,8 +96,7 @@ defmodule Plausible.Annotations do @spec get_one(User.t(), Plausible.Site.t(), atom(), pos_integer() | nil) :: {:ok, Annotation.t()} - | error_not_enough_permissions() - | error_annotation_not_found() + | {:error, error_not_enough_permissions() | error_annotation_not_found()} def get_one(user, site, site_role, annotation_id) do if site_role in roles_with_personal_annotations() do case do_get_one(user, site, annotation_id) do @@ -112,10 +110,10 @@ defmodule Plausible.Annotations do @spec insert_one(User.t(), Plausible.Site.t(), atom(), map()) :: {:ok, Annotation.t()} - | error_not_enough_permissions() - | error_invalid_annotation() - | error_annotation_limit_reached() - | unknown_error() + | {:error, + error_not_enough_permissions() + | error_invalid_annotation() + | error_annotation_limit_reached()} def insert_one(user, site, site_role, params) do changeset = Annotation.create_changeset(params, site, user) annotation_type = Ecto.Changeset.get_field(changeset, :type) @@ -134,9 +132,10 @@ defmodule Plausible.Annotations do @spec update_one(User.t(), Plausible.Site.t(), atom(), pos_integer(), map()) :: {:ok, Annotation.t()} - | error_not_enough_permissions() - | error_invalid_annotation() - | unknown_error() + | {:error, + error_not_enough_permissions() + | error_invalid_annotation() + | error_annotation_not_found()} def update_one(user, site, site_role, annotation_id, params) do with {:ok, annotation} <- get_one(user, site, site_role, annotation_id), changeset = Annotation.update_changeset(annotation, params, user), From e8eb66f87a03eab695e38b867e233dded3bacd33 Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Tue, 7 Jul 2026 10:28:55 +0200 Subject: [PATCH 14/23] Add plug requiring user in actions other than index --- .../api/internal/annotations_controller.ex | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/plausible_web/controllers/api/internal/annotations_controller.ex b/lib/plausible_web/controllers/api/internal/annotations_controller.ex index f5c9f9c78fc5..f26fcfca8a6a 100644 --- a/lib/plausible_web/controllers/api/internal/annotations_controller.ex +++ b/lib/plausible_web/controllers/api/internal/annotations_controller.ex @@ -5,10 +5,12 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsController do use Plausible use PlausibleWeb, :controller use PlausibleWeb.Plugs.ErrorHandler - alias PlausibleWeb.Api.Helpers, as: H alias Plausible.Annotations alias Plausible.ChangesetHelpers alias Plausible.Stats.{ApiQueryParser, QueryPeriod} + alias PlausibleWeb.Api.Helpers, as: H + + plug :require_user when action not in [:index] def index(conn, params) do user = conn.assigns.current_user @@ -144,4 +146,12 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsController do defp annotation_not_found(%Plug.Conn{} = conn, annotation_id_param) do H.not_found(conn, "Annotation not found with ID #{inspect(annotation_id_param)}") end + + defp require_user(conn, _opts) do + if conn.assigns.current_user do + conn + else + H.bad_request(conn, "Invalid request") + end + end end From 5df53eb17aa9cab3ab1f1ecb4a5503c89c411dff Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Tue, 7 Jul 2026 10:55:53 +0200 Subject: [PATCH 15/23] Replace ad-hoc plug with checks on Annotations API level and clean up API --- lib/plausible/annotations/annotations.ex | 131 +++++++++++------- .../api/internal/annotations_controller.ex | 10 -- 2 files changed, 82 insertions(+), 59 deletions(-) diff --git a/lib/plausible/annotations/annotations.ex b/lib/plausible/annotations/annotations.ex index 776f1eb5e36f..e446eb1f87b4 100644 --- a/lib/plausible/annotations/annotations.ex +++ b/lib/plausible/annotations/annotations.ex @@ -18,6 +18,17 @@ defmodule Plausible.Annotations do @max_annotations 500 + @spec roles_with_personal_annotations() :: [atom()] + def roles_with_personal_annotations(), do: @roles_with_personal_annotations + + @spec roles_with_maybe_site_annotations() :: [atom()] + def roles_with_maybe_site_annotations(), do: @roles_with_maybe_site_annotations + + @spec site_annotations_available?(Plausible.Site.t()) :: boolean() + def site_annotations_available?(site), + # this feature is bundled with SiteSegments + do: Plausible.Billing.Feature.SiteSegments.check_availability(site.team) == :ok + @spec get_all_for_site(Plausible.Site.t(), atom(), User.t() | nil, DateTimeRange.t()) :: {:error, error_not_enough_permissions()} | {:ok, list(Annotation.t())} def get_all_for_site(site, site_role, user, range_in_site_tz) do @@ -67,36 +78,13 @@ defmodule Plausible.Annotations do end end - defp filter_by_range(query, range_in_site_tz) do - # Minute granularity annotations are stored for the particular UTC moment they're for, - # so they must be in the range of the UTC query period. - minute_granularity_range = - range_in_site_tz |> DateTimeRange.to_timezone("Etc/UTC") - - # Date granularity annotations are stored for the UTC midnight of the date they're for, - # so the range for querying these must reflect that. - [date_granularity_range_first, date_granularity_range_last] = - [range_in_site_tz.first, range_in_site_tz.last] - |> Enum.map(fn utc_datetime -> - utc_datetime |> DateTime.to_date() |> Annotation.serialize_date_granularity_datetime() - end) - - from(annotation in query, - where: - (annotation.granularity == :minute and - annotation.datetime >= ^minute_granularity_range.first and - annotation.datetime <= ^minute_granularity_range.last) or - (annotation.granularity == :date and - annotation.datetime >= - ^date_granularity_range_first and - annotation.datetime <= - ^date_granularity_range_last) - ) - end - - @spec get_one(User.t(), Plausible.Site.t(), atom(), pos_integer() | nil) :: + @spec get_one(User.t() | nil, Plausible.Site.t(), atom(), pos_integer() | nil) :: {:ok, Annotation.t()} | {:error, error_not_enough_permissions() | error_annotation_not_found()} + def get_one(nil, _site, _site_role, _annotation_id) do + {:error, :not_enough_permissions} + end + def get_one(user, site, site_role, annotation_id) do if site_role in roles_with_personal_annotations() do case do_get_one(user, site, annotation_id) do @@ -108,12 +96,16 @@ defmodule Plausible.Annotations do end end - @spec insert_one(User.t(), Plausible.Site.t(), atom(), map()) :: + @spec insert_one(User.t() | nil, Plausible.Site.t(), atom(), map()) :: {:ok, Annotation.t()} | {:error, error_not_enough_permissions() | error_invalid_annotation() | error_annotation_limit_reached()} + def insert_one(nil, _site, _site_role, _params) do + {:error, :not_enough_permissions} + end + def insert_one(user, site, site_role, params) do changeset = Annotation.create_changeset(params, site, user) annotation_type = Ecto.Changeset.get_field(changeset, :type) @@ -130,12 +122,16 @@ defmodule Plausible.Annotations do end end - @spec update_one(User.t(), Plausible.Site.t(), atom(), pos_integer(), map()) :: + @spec update_one(User.t() | nil, Plausible.Site.t(), atom(), pos_integer(), map()) :: {:ok, Annotation.t()} | {:error, error_not_enough_permissions() | error_invalid_annotation() | error_annotation_not_found()} + def update_one(nil, _site, _site_role, _annotation_id, _params) do + {:error, :not_enough_permissions} + end + def update_one(user, site, site_role, annotation_id, params) do with {:ok, annotation} <- get_one(user, site, site_role, annotation_id), changeset = Annotation.update_changeset(annotation, params, user), @@ -152,6 +148,31 @@ defmodule Plausible.Annotations do end end + @spec delete_one(User.t() | nil, Plausible.Site.t(), atom(), pos_integer()) :: + {:ok, Annotation.t()} + | {:error, + error_not_enough_permissions() + | error_annotation_not_found()} + def delete_one(nil, _site, _site_role, _annotation_id) do + {:error, :not_enough_permissions} + end + + def delete_one(user, site, site_role, annotation_id) do + with {:ok, annotation} <- get_one(user, site, site_role, annotation_id) do + cond do + annotation.type == :site and site_role in roles_with_maybe_site_annotations() -> + {:ok, do_delete_one(annotation)} + + annotation.type == :personal and site_role in roles_with_personal_annotations() -> + {:ok, do_delete_one(annotation)} + + true -> + {:error, :not_enough_permissions} + end + end + end + + @spec after_user_removed_from_site(Plausible.Site.t(), User.t()) :: :ok def after_user_removed_from_site(site, user) do Repo.delete_all( from(annotation in Annotation, @@ -170,8 +191,11 @@ defmodule Plausible.Annotations do ), [] ) + + :ok end + @spec after_user_removed_from_team(Plausible.Teams.Team.t(), User.t()) :: :ok def after_user_removed_from_team(team, user) do team_sites_q = from( @@ -199,8 +223,11 @@ defmodule Plausible.Annotations do ), [] ) + + :ok end + @spec user_removed(User.t()) :: :ok def user_removed(user) do Repo.delete_all( from(annotation in Annotation, @@ -211,24 +238,37 @@ defmodule Plausible.Annotations do ) # Site annotations are set to owner=null via ON DELETE SET NULL + + :ok end - def delete_one(user, %Plausible.Site{} = site, site_role, annotation_id) do - with {:ok, annotation} <- get_one(user, site, site_role, annotation_id) do - cond do - annotation.type == :site and site_role in roles_with_maybe_site_annotations() -> - {:ok, do_delete_one(annotation)} + defp filter_by_range(query, range_in_site_tz) do + # Minute granularity annotations are stored for the particular UTC moment they're for, + # so they must be in the range of the UTC query period. + minute_granularity_range = + range_in_site_tz |> DateTimeRange.to_timezone("Etc/UTC") - annotation.type == :personal and site_role in roles_with_personal_annotations() -> - {:ok, do_delete_one(annotation)} + # Date granularity annotations are stored for the UTC midnight of the date they're for, + # so the range for querying these must reflect that. + [date_granularity_range_first, date_granularity_range_last] = + [range_in_site_tz.first, range_in_site_tz.last] + |> Enum.map(fn utc_datetime -> + utc_datetime |> DateTime.to_date() |> Annotation.serialize_date_granularity_datetime() + end) - true -> - {:error, :not_enough_permissions} - end - end + from(annotation in query, + where: + (annotation.granularity == :minute and + annotation.datetime >= ^minute_granularity_range.first and + annotation.datetime <= ^minute_granularity_range.last) or + (annotation.granularity == :date and + annotation.datetime >= + ^date_granularity_range_first and + annotation.datetime <= + ^date_granularity_range_last) + ) end - @spec do_get_one(User.t(), Plausible.Site.t(), pos_integer() | nil) :: Annotation.t() | nil defp do_get_one(user, site, annotation_id) defp do_get_one(_user, _site, nil) do @@ -297,11 +337,4 @@ defmodule Plausible.Annotations do ) |> Repo.aggregate(:count, :id) end - - def roles_with_personal_annotations(), do: @roles_with_personal_annotations - def roles_with_maybe_site_annotations(), do: @roles_with_maybe_site_annotations - - def site_annotations_available?(%Plausible.Site{} = site), - # this feature is bundled with SiteSegments - do: Plausible.Billing.Feature.SiteSegments.check_availability(site.team) == :ok end diff --git a/lib/plausible_web/controllers/api/internal/annotations_controller.ex b/lib/plausible_web/controllers/api/internal/annotations_controller.ex index f26fcfca8a6a..71fe6266d6ef 100644 --- a/lib/plausible_web/controllers/api/internal/annotations_controller.ex +++ b/lib/plausible_web/controllers/api/internal/annotations_controller.ex @@ -10,8 +10,6 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsController do alias Plausible.Stats.{ApiQueryParser, QueryPeriod} alias PlausibleWeb.Api.Helpers, as: H - plug :require_user when action not in [:index] - def index(conn, params) do user = conn.assigns.current_user site = conn.assigns.site @@ -146,12 +144,4 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsController do defp annotation_not_found(%Plug.Conn{} = conn, annotation_id_param) do H.not_found(conn, "Annotation not found with ID #{inspect(annotation_id_param)}") end - - defp require_user(conn, _opts) do - if conn.assigns.current_user do - conn - else - H.bad_request(conn, "Invalid request") - end - end end From 3df590d5a288f36441b2c6f8cb252ecc4dae8ceb Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Tue, 7 Jul 2026 17:20:14 +0200 Subject: [PATCH 16/23] Fix annotation owner update and add a provisional test --- lib/plausible/annotations/annotation.ex | 2 +- .../plausible/annotations/annotation_test.exs | 29 ++++++++++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/lib/plausible/annotations/annotation.ex b/lib/plausible/annotations/annotation.ex index 234aa2b02019..ca36905d64fa 100644 --- a/lib/plausible/annotations/annotation.ex +++ b/lib/plausible/annotations/annotation.ex @@ -55,7 +55,7 @@ defmodule Plausible.Annotations.Annotation do def update_changeset(annotation, attrs, owner) do annotation |> changeset(attrs, annotation.site.timezone) - |> put_assoc(:owner, owner) + |> put_change(:owner_id, owner.id) end def changeset(annotation, attrs, site_timezone) do diff --git a/test/plausible/annotations/annotation_test.exs b/test/plausible/annotations/annotation_test.exs index bb56d2fa06c9..eec34623840e 100644 --- a/test/plausible/annotations/annotation_test.exs +++ b/test/plausible/annotations/annotation_test.exs @@ -1,5 +1,7 @@ defmodule Plausible.Annotations.AnnotationTest do - use ExUnit.Case, async: true + use Plausible.DataCase, async: true + use Plausible.Teams.Test + alias Plausible.Annotations.Annotation describe "date-granularity" do @@ -280,5 +282,30 @@ defmodule Plausible.Annotations.AnnotationTest do assert changeset.valid? assert changeset.changes.datetime == ~U[2026-06-16 00:00:00Z] end + + test "the latest editing user becomes the owner of an annotation owned by someone else" do + owner = new_user() + new_owner = new_user() + site = new_site(owner: owner) + + existing = + %{ + note: "feature released", + type: :site, + granularity: :date, + date: ~D[2026-06-15] + } + |> Annotation.create_changeset(site, owner) + |> Repo.insert!() + + changeset = Annotation.update_changeset(existing, %{"note" => "renamed"}, new_owner) + + assert changeset.valid? + assert Ecto.Changeset.get_change(changeset, :owner_id) == new_owner.id + + assert {:ok, annotation} = Repo.update(changeset) + assert annotation.owner_id == new_owner.id + assert Repo.preload(annotation, :owner).owner.id == new_owner.id + end end end From 24599856797922a993fe1359747281bcd1b96850 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 8 Jul 2026 12:17:20 +0300 Subject: [PATCH 17/23] Move where FF plug is called --- .../controllers/api/internal/annotations_controller.ex | 2 ++ lib/plausible_web/router.ex | 3 --- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/plausible_web/controllers/api/internal/annotations_controller.ex b/lib/plausible_web/controllers/api/internal/annotations_controller.ex index 71fe6266d6ef..bbd1dac6fde9 100644 --- a/lib/plausible_web/controllers/api/internal/annotations_controller.ex +++ b/lib/plausible_web/controllers/api/internal/annotations_controller.ex @@ -10,6 +10,8 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsController do alias Plausible.Stats.{ApiQueryParser, QueryPeriod} alias PlausibleWeb.Api.Helpers, as: H + plug(PlausibleWeb.Plugs.FeatureFlagCheckPlug, [:annotations]) + def index(conn, params) do user = conn.assigns.current_user site = conn.assigns.site diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index 5ac770746d8c..c06fbc428c59 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -306,10 +306,7 @@ defmodule PlausibleWeb.Router do scope "/:domain/annotations", PlausibleWeb.Api.Internal, private: %{allow_consolidated_views: true} do - pipeline :annotations_endpoints, - do: plug(PlausibleWeb.Plugs.FeatureFlagCheckPlug, [:annotations]) - pipe_through :annotations_endpoints get "/", AnnotationsController, :index post "/", AnnotationsController, :create patch "/:annotation_id", AnnotationsController, :update From cdbcc9814b5ec21b9ad1dafa910d0da0ee07c636 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 8 Jul 2026 12:18:22 +0300 Subject: [PATCH 18/23] Make annotation_factory a bit more realistic --- test/support/factory.ex | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/support/factory.ex b/test/support/factory.ex index 482ba21c7c07..a70c8f9b9ba0 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -398,8 +398,9 @@ defmodule Plausible.Factory do def annotation_factory do %Plausible.Annotations.Annotation{ note: "a test annotation", - type: :personal, + type: :site, datetime: ~U[2026-01-04 00:00:00Z], + date: ~D[2026-01-04], granularity: :date } end From 2b852f6bb907290481317f317b1fb7da918461d1 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 8 Jul 2026 16:09:22 +0300 Subject: [PATCH 19/23] Simplify annotation tests, add cases --- lib/plausible/timezones.ex | 3 + lib/plausible_web/router.ex | 1 - .../plausible/annotations/annotation_test.exs | 425 ++++------- .../annotations_controller_test.exs | 697 ++++++++---------- .../segments_controller_test.exs | 90 ++- 5 files changed, 496 insertions(+), 720 deletions(-) diff --git a/lib/plausible/timezones.ex b/lib/plausible/timezones.ex index 030c41c95b42..1998f90a034c 100644 --- a/lib/plausible/timezones.ex +++ b/lib/plausible/timezones.ex @@ -8,6 +8,9 @@ defmodule Plausible.Timezones do Timex.is_valid_timezone?(tz) end + @spec zone_list() :: [String.t()] + def zone_list, do: Tzdata.zone_list() + @spec options(DateTime.t()) :: [{:key, String.t()}, {:value, String.t()}, {:offset, integer()}] def options(now \\ DateTime.utc_now()) do Tzdata.zone_list() diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index c06fbc428c59..a6939bfef399 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -306,7 +306,6 @@ defmodule PlausibleWeb.Router do scope "/:domain/annotations", PlausibleWeb.Api.Internal, private: %{allow_consolidated_views: true} do - get "/", AnnotationsController, :index post "/", AnnotationsController, :create patch "/:annotation_id", AnnotationsController, :update diff --git a/test/plausible/annotations/annotation_test.exs b/test/plausible/annotations/annotation_test.exs index eec34623840e..6a0a8c681420 100644 --- a/test/plausible/annotations/annotation_test.exs +++ b/test/plausible/annotations/annotation_test.exs @@ -1,311 +1,156 @@ defmodule Plausible.Annotations.AnnotationTest do - use Plausible.DataCase, async: true - use Plausible.Teams.Test - + use ExUnit.Case, async: true alias Plausible.Annotations.Annotation - describe "date-granularity" do - test "a full datetime string is rejected" do - changeset = - Annotation.changeset( - %Annotation{}, - %{ - "note" => "test", - "type" => "personal", - "granularity" => "date", - "datetime" => "2026-06-30T00:00:00Z" - }, - "Etc/UTC" - ) - - assert {"must be supplied for chosen granularity", []} = changeset.errors[:date] - end - - test "a non-midnight %DateTime{} is rejected" do - changeset = - Annotation.changeset( - %Annotation{}, - %{ - "note" => "test", - "type" => "personal", - "granularity" => "date", - "datetime" => ~U[2026-06-30 14:30:00Z] - }, - "Etc/UTC" - ) - - assert {"must be supplied for chosen granularity", []} = changeset.errors[:date] - end - - test "an unparsable date string is rejected" do - changeset = - Annotation.changeset( - %Annotation{}, - %{ - "granularity" => "date", - "datetime" => "not-a-date" - }, - "Etc/UTC" - ) - - assert {"is invalid", _} = changeset.errors[:datetime] - end - - test "an invalid calendar date is rejected" do + describe "changeset/3 for date granularity" do + for {dt, expected_errors} <- [ + {"2026-06-30T00:00:00", [date: {"must be supplied for chosen granularity", []}]}, + {"2026-06-30T00:00:00Z", [date: {"must be supplied for chosen granularity", []}]}, + {~U[2026-06-30 14:30:00Z], [date: {"must be supplied for chosen granularity", []}]}, + {"2026-07-05", [datetime: {"is invalid", [type: :utc_datetime, validation: :cast]}]}, + {~D[2026-07-06], [datetime: {"is invalid", [type: :utc_datetime, validation: :cast]}]} + ] do + test "rejects datetime #{dt} with appropriate error" do + changeset = + Annotation.changeset( + %Annotation{}, + %{ + note: "feature released", + type: "personal", + granularity: "date", + datetime: unquote(Macro.escape(dt)) + }, + "Etc/UTC" + ) + + assert changeset.errors == unquote(Macro.escape(expected_errors)) + end + end + + test "rejects invalid date value" do changeset = Annotation.changeset( %Annotation{}, %{ - "granularity" => "date", - "datetime" => "2026-13-45" + note: "feature released", + type: "personal", + granularity: "date", + date: "2026-13-45" }, "Etc/UTC" ) - assert {"is invalid", _} = changeset.errors[:datetime] - end - - test "a bare YYYY-MM-DD string becomes UTC midnight" do - changeset = - Annotation.changeset( - %Annotation{}, - %{ - "note" => "feature released", - "type" => "personal", - "site_id" => 1, - "owner_id" => 1, - "granularity" => "date", - "date" => "2026-06-30" - }, - "Etc/UTC" - ) - - assert changeset.valid? - assert changeset.changes.datetime == ~U[2026-06-30 00:00:00Z] - end - - test "a %Date{} struct becomes UTC midnight" do - changeset = - Annotation.changeset( - %Annotation{}, - %{ - "note" => "feature released", - "type" => "personal", - "site_id" => 1, - "owner_id" => 1, - "granularity" => "date", - "date" => ~D[2026-06-30] - }, - "Etc/UTC" - ) - - assert changeset.valid? - assert changeset.changes.datetime == ~U[2026-06-30 00:00:00Z] + assert [date: {"is invalid", _}] = changeset.errors + end + + for {d, expected} <- [ + {"2026-06-30", ~U[2026-06-30 00:00:00Z]}, + {~D[2026-07-01], ~U[2026-07-01 00:00:00Z]} + ] do + test "accepts date #{d}, parsing it to that date at UTC midnight (#{expected})" do + changeset = + Annotation.changeset( + %Annotation{}, + %{ + note: "feature released", + type: "personal", + granularity: "date", + date: unquote(Macro.escape(d)) + }, + any_timezone() + ) + + assert changeset.valid? + assert changeset.changes.datetime == unquote(Macro.escape(expected)) + end end end - describe "minute-granularity" do - test "a bare date string is rejected" do - changeset = - Annotation.changeset( - %Annotation{}, - %{ - "granularity" => "minute", - "datetime" => "2026-06-30" - }, - "Etc/UTC" - ) - - assert {"is invalid", _} = changeset.errors[:datetime] - end - - test "an unparsable datetime is rejected" do - changeset = - Annotation.changeset( - %Annotation{}, - %{ - "granularity" => "minute", - "datetime" => "garbage" - }, - "Etc/UTC" - ) - - assert {"is invalid", _} = changeset.errors[:datetime] - end - - test "a Z-suffixed datetime string is kept in UTC" do - changeset = - Annotation.changeset( - %Annotation{}, - %{ - "note" => "feature released", - "type" => "personal", - "site_id" => 1, - "owner_id" => 1, - "granularity" => "minute", - "datetime" => "2026-06-30T14:30:00Z" - }, - "Etc/UTC" - ) - - assert changeset.valid? - assert changeset.changes.datetime == ~U[2026-06-30 14:30:00Z] - end - - test "an offset datetime string is shifted to UTC" do - changeset = - Annotation.changeset( - %Annotation{}, - %{ - "note" => "feature released", - "type" => "personal", - "site_id" => 1, - "owner_id" => 1, - "granularity" => "minute", - "datetime" => "2026-06-30T10:00:00-02:00" - }, - "Etc/UTC" - ) - - assert changeset.valid? - assert changeset.changes.datetime == ~U[2026-06-30 12:00:00Z] - end - - test "a %DateTime{} is passed through in UTC" do - changeset = - Annotation.changeset( - %Annotation{}, - %{ - "note" => "feature released", - "type" => "personal", - "site_id" => 1, - "owner_id" => 1, - "granularity" => "minute", - "datetime" => ~U[2026-06-30 14:30:00Z] - }, - "Etc/UTC" - ) - - assert changeset.valid? - assert changeset.changes.datetime == ~U[2026-06-30 14:30:00Z] + describe "changeset/3 for minute granularity" do + for d <- ["2026-07-05", ~D[2026-07-06]] do + test "requires :datetime to be present, given date: #{d}" do + changeset = + Annotation.changeset( + %Annotation{}, + %{ + note: "feature released", + type: "personal", + granularity: "minute", + date: unquote(Macro.escape(d)) + }, + "Etc/UTC" + ) + + assert changeset.errors == [datetime: {"must be supplied for chosen granularity", []}] + end + end + + for dt <- ["2026-06-30", ~D[2026-07-01], "invalid"] do + test "rejects invalid :datetime, given #{dt}" do + changeset = + Annotation.changeset( + %Annotation{}, + %{ + note: "feature released", + type: "personal", + granularity: "minute", + datetime: unquote(Macro.escape(dt)) + }, + "Etc/UTC" + ) + + assert [datetime: {"is invalid", _}] = changeset.errors + end + end + + for {dt, expected, tz} <- [ + {"2026-06-30T14:30:00Z", ~U[2026-06-30 14:30:00Z], any_timezone()}, + {"2026-06-30T10:00:00-02:00", ~U[2026-06-30 12:00:00Z], any_timezone()}, + {~U[2026-06-30 14:30:00Z], ~U[2026-06-30 14:30:00Z], any_timezone()}, + {"2026-06-30T14:30:00", ~U[2026-06-30 14:30:00Z], "Etc/UTC"}, + {"2026-06-30T14:30:00", ~U[2026-06-30 11:30:00Z], "Europe/Tallinn"} + ] do + test "accepts :datetime #{dt} and parses it to #{expected} UTC point in time for the site with timezone #{tz}" do + changeset = + Annotation.changeset( + %Annotation{}, + %{ + note: "feature released", + type: "personal", + granularity: "minute", + datetime: unquote(Macro.escape(dt)) + }, + unquote(tz) + ) + + assert changeset.valid? + assert changeset.changes.datetime == unquote(Macro.escape(expected)) + end end end - describe "unknown granularity" do - test "defers to Ecto's enum cast, not our coercion error" do - changeset = - Annotation.changeset( - %Annotation{}, - %{ - "granularity" => "hour", - "datetime" => "2026-06-30T10:00:00Z" - }, - "Etc/UTC" - ) - - assert changeset.errors[:granularity] - refute changeset.errors[:datetime] + describe "changeset/3 when both date and datetime are provided" do + for granularity <- ["date", "minute"] do + test "rejects both being set for #{granularity} granularity" do + changeset = + Annotation.changeset( + %Annotation{}, + %{ + note: "feature released", + type: "personal", + granularity: unquote(granularity), + date: "2026-06-30", + datetime: "2026-06-30T14:30:00Z" + }, + "Etc/UTC" + ) + + assert changeset.errors == [ + granularity: {"expects either date or datetime to be set", []} + ] + end end end - describe "granularity change" do - test "flipping :date to :minute without a datetime is rejected" do - existing = %Annotation{ - granularity: :date, - datetime: ~U[2026-06-15 00:00:00Z], - note: "test", - type: :personal - } - - changeset = Annotation.changeset(existing, %{"granularity" => "minute"}, "Etc/UTC") - - assert {"must be supplied for chosen granularity", []} = changeset.errors[:datetime] - end - - test "flipping :minute to :date without a datetime is rejected" do - existing = %Annotation{ - granularity: :minute, - datetime: ~U[2026-06-15 14:30:00Z], - note: "test", - type: :personal - } - - changeset = Annotation.changeset(existing, %{"granularity" => "date"}, "Etc/UTC") - - assert {"must be supplied for chosen granularity", []} = changeset.errors[:date] - end - - test "flipping granularity to minute with an appropriate datetime is accepted" do - existing = %Annotation{ - note: "feature released", - type: :personal, - site_id: 1, - owner_id: 1, - granularity: :date, - datetime: ~U[2026-06-15 00:00:00Z] - } - - changeset = - Annotation.changeset( - existing, - %{ - "granularity" => "minute", - "datetime" => "2026-06-15T14:30:00Z" - }, - "Etc/UTC" - ) - - assert changeset.valid? - assert changeset.changes.datetime == ~U[2026-06-15 14:30:00Z] - end - - test "flipping granularity to date with an appropriate date is accepted" do - existing = %Annotation{ - note: "feature released", - type: :personal, - site_id: 1, - owner_id: 1, - granularity: :minute, - datetime: ~U[2026-06-15 10:00:00Z] - } - - changeset = - Annotation.changeset( - existing, - %{ - "granularity" => "date", - "date" => "2026-06-16" - }, - "Etc/UTC" - ) - - assert changeset.valid? - assert changeset.changes.datetime == ~U[2026-06-16 00:00:00Z] - end - - test "the latest editing user becomes the owner of an annotation owned by someone else" do - owner = new_user() - new_owner = new_user() - site = new_site(owner: owner) - - existing = - %{ - note: "feature released", - type: :site, - granularity: :date, - date: ~D[2026-06-15] - } - |> Annotation.create_changeset(site, owner) - |> Repo.insert!() - - changeset = Annotation.update_changeset(existing, %{"note" => "renamed"}, new_owner) - - assert changeset.valid? - assert Ecto.Changeset.get_change(changeset, :owner_id) == new_owner.id - - assert {:ok, annotation} = Repo.update(changeset) - assert annotation.owner_id == new_owner.id - assert Repo.preload(annotation, :owner).owner.id == new_owner.id - end - end + defp any_timezone(), do: Enum.random(Plausible.Timezones.zone_list()) end diff --git a/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs b/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs index c3c06b95bf14..a11663c29507 100644 --- a/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs +++ b/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs @@ -2,70 +2,23 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do use PlausibleWeb.ConnCase, async: true use Plausible.Repo - describe "GET /api/:domain/annotations - index" do + describe "GET /api/:domain/annotations" 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?date_range=day&relative_date=2026-01-04" - ) - - 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?date_range=day&relative_date=2026-01-04" - ) - - 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") + test "returns 400 when date_range is missing", %{conn: conn, user: user} do + site = new_site(owner: user) - conn = get(conn, "/api/#{site.domain}/annotations?date_range=day&relative_date=2026-01-04") + 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 + assert %{"error" => _} = json_response(conn, 400) end - test "authenticated owner response includes owner info", %{conn: conn, user: user} do + test "returns 400 when date_range is unrecognized", %{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?date_range=day&relative_date=2026-01-04") + conn = get(conn, "/api/#{site.domain}/annotations?date_range=banana") - assert [result] = json_response(conn, 200) - assert result["owner_id"] == user.id - assert result["owner_name"] == user.name + assert %{"error" => _} = json_response(conn, 400) end test "private site returns 404 for non-member", %{conn: conn} do @@ -79,25 +32,96 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do assert json_response(conn, 404) end - end - describe "GET /api/:domain/annotations - period filtering" do - setup [:create_user, :log_in] + test "only site annotations are shown when viewing a public site, with owners info hidden", + %{conn: conn} do + site_owner = new_user() + public_site = new_site(public: true, owner: site_owner) + 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") - test "returns 400 when date_range is missing", %{conn: conn, user: user} do - site = new_site(owner: user) + conn = + get( + conn, + "/api/#{public_site.domain}/annotations?date_range=day&relative_date=2026-01-04" + ) - conn = get(conn, "/api/#{site.domain}/annotations") + assert_matches [ + ^strict_map(%{ + "id" => ^any(:pos_integer), + "note" => "site note", + "type" => "site", + "datetime" => "2026-01-04", + "granularity" => "date", + "owner_id" => nil, + "owner_name" => nil, + "inserted_at" => ^any(:iso8601_naive_datetime), + "updated_at" => ^any(:iso8601_naive_datetime) + }) + ] = json_response(conn, 200) + end + + test "personal annotations of the user and all site annotations are shown with annotation owner info when viewing their site, sorted by updated_at, ascending", + %{conn: conn, user: user} do + site = new_site() + other_user = new_user() + add_guest(site, user: user, role: :viewer) - assert %{"error" => _} = json_response(conn, 400) - end + insert(:annotation, + site: site, + owner: user, + type: :personal, + note: "mine", + inserted_at: ~U[2026-07-01 10:00:00Z], + updated_at: ~U[2026-07-01 10:00:00Z] + ) - test "returns 400 when date_range is unrecognized", %{conn: conn, user: user} do - site = new_site(owner: user) + insert(:annotation, + site: site, + owner: other_user, + type: :personal, + note: "not mine", + inserted_at: ~U[2026-07-01 10:00:00Z], + updated_at: ~U[2026-07-01 11:00:00Z] + ) - conn = get(conn, "/api/#{site.domain}/annotations?date_range=banana") + insert(:annotation, + site: site, + owner: other_user, + type: :site, + note: "shared", + inserted_at: ~U[2026-07-01 10:00:00Z], + updated_at: ~U[2026-07-01 12:00:00Z] + ) - assert %{"error" => _} = json_response(conn, 400) + conn = get(conn, "/api/#{site.domain}/annotations?date_range=day&relative_date=2026-01-04") + + results = json_response(conn, 200) + + assert_matches [ + ^strict_map(%{ + "id" => ^any(:pos_integer), + "note" => "shared", + "type" => "site", + "datetime" => "2026-01-04", + "granularity" => "date", + "owner_id" => ^other_user.id, + "owner_name" => ^other_user.name, + "inserted_at" => ^any(:iso8601_naive_datetime), + "updated_at" => ^any(:iso8601_naive_datetime) + }), + ^strict_map(%{ + "id" => ^any(:pos_integer), + "note" => "mine", + "type" => "personal", + "datetime" => "2026-01-04", + "granularity" => "date", + "owner_id" => ^user.id, + "owner_name" => ^user.name, + "inserted_at" => ^any(:iso8601_naive_datetime), + "updated_at" => ^any(:iso8601_naive_datetime) + }) + ] = results end test "filters out annotations outside a 28d window", %{conn: conn, user: user} do @@ -159,11 +183,10 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do assert Enum.map(results, & &1["note"]) == ["in range"] end - test "minute granularity is filtered against the UTC window of a day period", + test "filters minute granularity annotations to be within the UTC datetime range", %{conn: conn, user: user} do - # NY is UTC-4 in June (DST). Local 2026-06-28 -> UTC [04:00 2026-06-28, 03:59:59 2026-06-29]. - # Using a past relative_date so :day builds the full local day window - # rather than [00:00, now], which would make the test time-of-day dependent. + # NY is UTC-4 in June (DST). + # Full day of 2026-06-28 -> [~U[2026-06-28 04:00:00Z], ~U[2026-06-29 03:59:59]]. site = new_site(owner: user, timezone: "America/New_York") insert(:annotation, @@ -193,13 +216,9 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do assert Enum.map(results, & &1["note"]) == ["in window"] end - test "date-granularity annotation for the dashboard's local date is included even though its UTC moment falls outside the UTC window", + test "filters date granularity annotations that are within the local date range", %{conn: conn, user: user} do - # Date-granularity annotations store UTC midnight of the intended local - # date, so the stored 2026-06-29T00:00:00Z is *outside* NY's UTC window - # for local 2026-06-29 ([04:00, next 03:59:59]). It must still be returned - # because its local date matches the dashboard's local date range. - site = new_site(owner: user, timezone: "America/New_York") + site = new_site(owner: user, timezone: "Asia/Tokyo") insert(:annotation, site: site, @@ -207,47 +226,23 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do type: :personal, granularity: :date, datetime: ~U[2026-06-29 00:00:00Z], - note: "today date-annotation" + note: "in range" ) - conn = - get(conn, "/api/#{site.domain}/annotations?date_range=day&relative_date=2026-06-29") - - results = json_response(conn, 200) - assert Enum.map(results, & &1["note"]) == ["today date-annotation"] - end - - test "returns both date and minute annotations when both fall inside the period", - %{conn: conn, user: user} do - site = new_site(owner: user) - insert(:annotation, site: site, owner: user, type: :personal, granularity: :date, - datetime: ~U[2026-06-25 00:00:00Z], - note: "date" - ) - - insert(:annotation, - site: site, - owner: user, - type: :personal, - granularity: :minute, - datetime: ~U[2026-06-26 10:30:00Z], - note: "minute" + datetime: ~U[2026-06-30 00:00:00Z], + note: "out of range" ) conn = - get( - conn, - "/api/#{site.domain}/annotations?date_range_start=2026-06-20&date_range_end=2026-06-29" - ) + get(conn, "/api/#{site.domain}/annotations?date_range=7d&relative_date=2026-06-30") results = json_response(conn, 200) - notes = results |> Enum.map(& &1["note"]) |> Enum.sort() - assert notes == ["date", "minute"] + assert Enum.map(results, & &1["note"]) == ["in range"] end test "fetches annotations for realtime date_range", %{conn: conn, user: user} do @@ -269,174 +264,189 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do end end - describe "POST /api/:domain/annotations - datetime coercion" do + describe "POST /api/:domain/annotations" do setup [:create_user, :log_in, :create_site] - test "accepts bare date string when granularity is date", - %{conn: conn, site: site} do - response = + test "rejects missing note", %{conn: conn, site: site} do + conn = post(conn, "/api/#{site.domain}/annotations", %{ - "note" => "feature released", "type" => "personal", "granularity" => "date", - "date" => "2026-01-04" + "datetime" => "2026-01-04" }) - |> json_response(200) - assert_matches ^strict_map(%{ - "id" => ^any(:pos_integer), - "note" => "feature released", - "type" => "personal", - "granularity" => "date", - "datetime" => "2026-01-04", - "owner_id" => ^any(:pos_integer), - "owner_name" => ^any(:string), - "inserted_at" => ^any(:iso8601_naive_datetime), - "updated_at" => ^any(:iso8601_naive_datetime) - }) = response + assert json_response(conn, 400) == %{"error" => "note can't be blank"} end - test "rejects full datetime string when granularity is date", - %{conn: conn, site: site} do + test "rejects empty note", %{conn: conn, site: site} do conn = post(conn, "/api/#{site.domain}/annotations", %{ - "note" => "feature released", + "note" => "", "type" => "personal", "granularity" => "date", - "datetime" => "2026-01-04T00:00:00Z" - }) - - assert json_response(conn, 400) == %{ - "error" => "date must be supplied for chosen granularity" - } - end - - test "accepts full datetime string when granularity is minute", - %{conn: conn, site: site} do - # Site is Etc/UTC by default; UTC moment is returned as naive local time - response = - post(conn, "/api/#{site.domain}/annotations", %{ - "note" => "feature released", - "type" => "personal", - "granularity" => "minute", - "datetime" => "2026-01-04T14:32:00Z" + "datetime" => "2026-01-04" }) - |> json_response(200) - assert response["datetime"] == "2026-01-04T14:32:00" + assert json_response(conn, 400) == %{"error" => "note can't be blank"} end - test "rejects bare date string when granularity is minute", - %{conn: conn, site: site} do + test "rejects note over 255 bytes", %{conn: conn, site: site} do conn = post(conn, "/api/#{site.domain}/annotations", %{ - "note" => "feature released", + "note" => String.duplicate("a", 256), "type" => "personal", - "granularity" => "minute", + "granularity" => "date", "datetime" => "2026-01-04" }) - assert json_response(conn, 400) == %{"error" => "datetime is invalid"} + assert json_response(conn, 400) == %{"error" => "note should be at most 255 byte(s)"} end - test "rejects invalid calendar date when granularity is date", - %{conn: conn, site: site} do + test "rejects missing datetime", %{conn: conn, site: site} do conn = post(conn, "/api/#{site.domain}/annotations", %{ "note" => "feature released", "type" => "personal", - "granularity" => "date", - "date" => "2026-13-45" + "granularity" => "date" }) - assert json_response(conn, 400) == %{"error" => "date is invalid"} + assert json_response(conn, 400) == %{ + "error" => "date must be supplied for chosen granularity" + } end - test "rejects non-date string when granularity is date", - %{conn: conn, site: site} do + test "rejects unknown granularity", %{conn: conn, site: site} do conn = post(conn, "/api/#{site.domain}/annotations", %{ "note" => "feature released", "type" => "personal", - "granularity" => "date", - "date" => "not-a-date" + "granularity" => "hour", + "datetime" => "2026-01-04T00:00:00Z" }) - assert json_response(conn, 400) == %{"error" => "date is invalid"} + assert json_response(conn, 400) == %{"error" => "granularity is invalid"} end - test "rejects non-datetime string when granularity is minute", - %{conn: conn, site: site} do + test "rejects missing granularity", %{conn: conn, site: site} do conn = post(conn, "/api/#{site.domain}/annotations", %{ "note" => "feature released", "type" => "personal", - "granularity" => "minute", - "datetime" => "not-a-datetime" + "datetime" => "2026-01-04T00:00:00Z" }) - assert json_response(conn, 400) == %{"error" => "datetime is invalid"} + assert json_response(conn, 400) == %{"error" => "granularity can't be blank"} end - test "rejects empty datetime string when granularity is date", - %{conn: conn, site: site} do + test "rejects unknown type as insufficient permissions", %{conn: conn, site: site} do conn = post(conn, "/api/#{site.domain}/annotations", %{ "note" => "feature released", - "type" => "personal", + "type" => "team", "granularity" => "date", - "date" => "" + "datetime" => "2026-01-04" }) - assert json_response(conn, 400) == %{ - "error" => "date must be supplied for chosen granularity" + assert json_response(conn, 403) == %{ + "error" => "Not enough permissions to create annotation" } end - test "rejects empty datetime string when granularity is minute", - %{conn: conn, site: site} do - conn = - post(conn, "/api/#{site.domain}/annotations", %{ - "note" => "feature released", - "type" => "personal", - "granularity" => "minute", - "datetime" => "" - }) - - assert json_response(conn, 400) == %{ - "error" => "datetime must be supplied for chosen granularity" - } + for {params, error} <- [ + { + %{"granularity" => "date", "datetime" => "2026-01-04T00:00:00Z"}, + "date must be supplied for chosen granularity" + }, + { + %{"granularity" => "date", "datetime" => "2026-01-04T00:00:00"}, + "date must be supplied for chosen granularity" + }, + { + %{"granularity" => "date", "date" => ""}, + "date must be supplied for chosen granularity" + }, + { + %{"granularity" => "date", "date" => "2026-1-4"}, + "date is invalid" + }, + { + %{"granularity" => "date", "date" => "2026-13-45"}, + "date is invalid" + }, + { + %{"granularity" => "date", "date" => "not-a-date"}, + "date is invalid" + }, + { + %{"granularity" => "minute", "datetime" => "2026-01-04"}, + "datetime is invalid" + }, + { + %{"granularity" => "minute", "datetime" => "not-a-datetime"}, + "datetime is invalid" + }, + { + %{"granularity" => "minute", "datetime" => ""}, + "datetime must be supplied for chosen granularity" + }, + { + %{"granularity" => "minute", "datetime" => "2026-13-45T14:30:00Z"}, + "datetime is invalid" + } + ] do + test "rejects #{inspect(params)} with error #{error}", %{conn: conn, site: site} do + conn = + post( + conn, + "/api/#{site.domain}/annotations", + Map.merge( + %{"note" => "feature released", "type" => "personal"}, + unquote(Macro.escape(params)) + ) + ) + + assert json_response(conn, 400) == %{"error" => unquote(error)} + end end - test "rejects date shorter than 10 characters when granularity is date", + test "accepts bare date string when granularity is date", %{conn: conn, site: site} do - conn = + response = post(conn, "/api/#{site.domain}/annotations", %{ "note" => "feature released", "type" => "personal", "granularity" => "date", - "date" => "2026-1-4" + "date" => "2026-01-04" }) + |> json_response(200) - assert json_response(conn, 400) == %{"error" => "date is invalid"} + assert_matches ^strict_map(%{ + "id" => ^any(:pos_integer), + "note" => "feature released", + "type" => "personal", + "granularity" => "date", + "datetime" => "2026-01-04", + "owner_id" => ^any(:pos_integer), + "owner_name" => ^any(:string), + "inserted_at" => ^any(:iso8601_naive_datetime), + "updated_at" => ^any(:iso8601_naive_datetime) + }) = response end - test "rejects invalid calendar datetime when granularity is minute", + test "accepts full datetime string when granularity is minute", %{conn: conn, site: site} do - conn = + # Site is Etc/UTC by default; UTC moment is returned as naive local time + response = post(conn, "/api/#{site.domain}/annotations", %{ "note" => "feature released", "type" => "personal", "granularity" => "minute", - "datetime" => "2026-13-45T14:30:00Z" + "datetime" => "2026-01-04T14:32:00Z" }) + |> json_response(200) - assert json_response(conn, 400) == %{"error" => "datetime is invalid"} + assert response["datetime"] == "2026-01-04T14:32:00" end - end - - describe "POST /api/:domain/annotations - naive local time coercion" do - setup [:create_user, :log_in] test "converts naive local time to UTC and returns it back as local time", %{conn: conn, user: user} do @@ -454,23 +464,10 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do |> json_response(200) assert response["datetime"] == "2026-01-04T14:30:00" - end - test "rejects naive datetime when granularity is date", - %{conn: conn, user: user} do - site = new_site(owner: user, timezone: "America/New_York") - - conn = - post(conn, "/api/#{site.domain}/annotations", %{ - "note" => "feature released", - "type" => "personal", - "granularity" => "date", - "datetime" => "2026-01-04T00:00:00" - }) - - assert json_response(conn, 400) == %{ - "error" => "date must be supplied for chosen granularity" - } + reloaded = Plausible.Repo.get!(Plausible.Annotations.Annotation, response["id"]) + assert reloaded.granularity == :minute + assert reloaded.datetime == ~U[2026-01-04 19:30:00Z] end test "UTC datetime string is stored as UTC and returned as site local time", @@ -491,100 +488,9 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do end end - describe "PATCH /api/:domain/annotations/:annotation_id - naive local time coercion" do - setup [:create_user, :log_in] - - test "converts naive local time to UTC and returns it back as local time", - %{conn: conn, user: user} do - # America/New_York is UTC-4 in June (DST), so input and output are the same - site = new_site(owner: user, timezone: "America/New_York") - - annotation = - insert(:annotation, - site: site, - owner: user, - type: :personal, - granularity: :minute, - datetime: ~U[2026-01-01 00:00:00Z] - ) - - response = - patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ - "granularity" => "minute", - "datetime" => "2026-06-15T10:00:00" - }) - |> json_response(200) - - assert response["datetime"] == "2026-06-15T10:00:00" - end - end - - describe "PATCH /api/:domain/annotations/:annotation_id - datetime coercion" do + describe "PATCH /api/:domain/annotations/:annotation_id" do setup [:create_user, :log_in, :create_site] - test "accepts bare date string when granularity is date", - %{conn: conn, site: site, user: user} do - annotation = - insert(:annotation, - site: site, - owner: user, - type: :personal, - granularity: :date, - datetime: ~U[2026-01-01 00:00:00Z] - ) - - response = - patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ - "granularity" => "date", - "date" => "2026-06-15" - }) - |> json_response(200) - - assert response["datetime"] == "2026-06-15" - end - - test "rejects bare date string when granularity is minute", - %{conn: conn, site: site, user: user} do - annotation = - insert(:annotation, - site: site, - owner: user, - type: :personal, - granularity: :minute, - datetime: ~U[2026-01-01 10:00:00Z] - ) - - conn = - patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ - "granularity" => "minute", - "datetime" => "2026-06-15" - }) - - assert json_response(conn, 400) == %{"error" => "datetime is invalid"} - end - - test "rejects full datetime string when granularity switched to date", - %{conn: conn, site: site, user: user} do - annotation = - insert(:annotation, - site: site, - owner: user, - type: :personal, - granularity: :minute, - datetime: ~U[2026-01-01 10:00:00Z] - ) - - conn = - patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ - "granularity" => "date", - "datetime" => "2026-06-15T10:00:00Z" - }) - - assert json_response(conn, 400) == %{ - "error" => "date must be supplied for chosen granularity" - } - end - test "rejects granularity change from date to minute without a new datetime", %{conn: conn, site: site, user: user} do annotation = @@ -686,107 +592,116 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do assert reloaded.granularity == :date assert reloaded.datetime == ~U[2026-06-20 00:00:00Z] end - end - describe "POST /api/:domain/annotations - required fields and length" do - setup [:create_user, :log_in, :create_site] - - test "rejects missing note", %{conn: conn, site: site} do - conn = - post(conn, "/api/#{site.domain}/annotations", %{ - "type" => "personal", - "granularity" => "date", - "datetime" => "2026-01-04" - }) - - assert json_response(conn, 400) == %{"error" => "note can't be blank"} - end - - test "rejects empty note", %{conn: conn, site: site} do - conn = - post(conn, "/api/#{site.domain}/annotations", %{ - "note" => "", - "type" => "personal", - "granularity" => "date", - "datetime" => "2026-01-04" - }) - - assert json_response(conn, 400) == %{"error" => "note can't be blank"} - end - - test "rejects note over 255 bytes", %{conn: conn, site: site} do - conn = - post(conn, "/api/#{site.domain}/annotations", %{ - "note" => String.duplicate("a", 256), - "type" => "personal", - "granularity" => "date", - "datetime" => "2026-01-04" - }) + test "converts naive local time to UTC and returns it back as local time", + %{conn: conn, user: user} do + # America/New_York is UTC-4 in June (DST), so input and output are the same + site = new_site(owner: user, timezone: "America/New_York") - assert json_response(conn, 400) == %{"error" => "note should be at most 255 byte(s)"} - end + annotation = + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :minute, + datetime: ~U[2026-01-01 00:00:00Z] + ) - test "accepts note of exactly 255 bytes", %{conn: conn, site: site} do response = - post(conn, "/api/#{site.domain}/annotations", %{ - "note" => String.duplicate("a", 255), - "type" => "personal", - "granularity" => "date", - "date" => "2026-01-04" + patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ + "granularity" => "minute", + "datetime" => "2026-06-15T10:00:00" }) |> json_response(200) - assert response["note"] == String.duplicate("a", 255) - end - - test "rejects missing datetime", %{conn: conn, site: site} do - conn = - post(conn, "/api/#{site.domain}/annotations", %{ - "note" => "feature released", - "type" => "personal", - "granularity" => "date" - }) + assert response["datetime"] == "2026-06-15T10:00:00" - assert json_response(conn, 400) == %{ - "error" => "date must be supplied for chosen granularity" - } + reloaded = Plausible.Repo.get!(Plausible.Annotations.Annotation, annotation.id) + assert reloaded.granularity == :minute + assert reloaded.datetime == ~U[2026-06-15 14:00:00Z] end - test "rejects unknown granularity", %{conn: conn, site: site} do - conn = - post(conn, "/api/#{site.domain}/annotations", %{ - "note" => "feature released", - "type" => "personal", - "granularity" => "hour", - "datetime" => "2026-01-04T00:00:00Z" - }) - - assert json_response(conn, 400) == %{"error" => "granularity is invalid"} - end + test "accepts bare date string when granularity is date", + %{conn: conn, site: site, user: user} do + annotation = + insert(:annotation, + site: site, + owner: user, + type: :personal, + granularity: :date, + datetime: ~U[2026-01-01 00:00:00Z] + ) - test "rejects missing granularity", %{conn: conn, site: site} do - conn = - post(conn, "/api/#{site.domain}/annotations", %{ - "note" => "feature released", - "type" => "personal", - "datetime" => "2026-01-04T00:00:00Z" + response = + patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ + "granularity" => "date", + "date" => "2026-06-15" }) + |> json_response(200) - assert json_response(conn, 400) == %{"error" => "granularity can't be blank"} + assert response["datetime"] == "2026-06-15" end - test "rejects unknown type as insufficient permissions", %{conn: conn, site: site} do - conn = - post(conn, "/api/#{site.domain}/annotations", %{ - "note" => "feature released", - "type" => "team", - "granularity" => "date", - "datetime" => "2026-01-04" - }) - - assert json_response(conn, 403) == %{ - "error" => "Not enough permissions to create annotation" - } + for {casename, note_has_owner?} <- [ + {"dangling site note", false}, + {"site note left by other user", true} + ] do + test "users with site note permissions can edit a #{casename}, becoming its new owner", + %{ + conn: conn, + site: site, + user: user + } do + note_owner = + if(unquote(note_has_owner?), + do: + site.team + |> add_member( + user: new_user(name: "other user"), + role: :editor + ), + else: nil + ) + + annotation = + insert(:annotation, + owner: note_owner, + site: site, + type: :site, + granularity: :date, + datetime: "2026-06-01T00:00:00Z" + ) + + response = + patch(conn, "/api/#{site.domain}/annotations/#{annotation.id}", %{ + "note" => "updated" + }) + |> json_response(200) + + assert_matches ^strict_map(%{ + "id" => ^annotation.id, + "note" => "updated", + "type" => + ^any( + :string, + ~r/#{annotation.type}/ + ), + "granularity" => + ^any( + :string, + ~r/#{annotation.granularity}/ + ), + "datetime" => "2026-06-01", + "owner_id" => ^user.id, + "owner_name" => ^user.name, + "inserted_at" => + ^any( + :string, + ~r/#{Calendar.strftime(annotation.inserted_at, "%Y-%m-%dT%H:%M:%S")}/ + ), + "updated_at" => ^any(:iso8601_naive_datetime) + }) = response + end end end end diff --git a/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs b/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs index e73d58a3cf67..ddbf41a4c505 100644 --- a/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs +++ b/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs @@ -358,44 +358,58 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do }) = response end - test "the last editor of a dangling site segment (segment with owner_id: nil) becomes the new owner", - %{ - conn: conn, - user: user - } do - site = new_site() - add_guest(site, user: user, role: :editor) + for {casename, segment_has_owner?} <- [ + {"dangling site segment", false}, + {"site segment by other user", true} + ] do + test "the last editor of a #{casename} becomes the new owner", + %{ + conn: conn, + site: site, + user: user + } do + segment_owner = + if(unquote(segment_has_owner?), + do: + site.team + |> add_member( + user: new_user(name: "other user"), + role: :editor + ), + else: nil + ) - segment = - insert(:segment, - site: site, - name: "original name", - type: :site, - owner: nil - ) + segment = + insert(:segment, + site: site, + name: "original name", + type: :site, + owner: segment_owner + ) - response = - patch(conn, "/api/#{site.domain}/segments/#{segment.id}", %{"name" => "updated name"}) - |> json_response(200) + response = + patch(conn, "/api/#{site.domain}/segments/#{segment.id}", %{"name" => "updated name"}) + |> json_response(200) - assert_matches ^strict_map(%{ - "id" => ^segment.id, - "name" => "updated name", - "type" => - ^any( - :string, - ~r/#{segment.type}/ - ), - "owner_id" => ^user.id, - "owner_name" => ^user.name, - "segment_data" => ^segment.segment_data, - "inserted_at" => - ^any( - :string, - ~r/#{Calendar.strftime(segment.inserted_at, "%Y-%m-%dT%H:%M:%S")}/ - ), - "updated_at" => ^any(:iso8601_naive_datetime) - }) = response + assert_matches ^strict_map(%{ + "id" => ^segment.id, + "name" => "updated name", + "type" => + ^any( + :string, + ~r/#{segment.type}/ + ), + "owner_id" => ^user.id, + "owner_name" => ^user.name, + "segment_data" => ^segment.segment_data, + "inserted_at" => + ^any( + :string, + ~r/#{Calendar.strftime(segment.inserted_at, "%Y-%m-%dT%H:%M:%S")}/ + ), + "updated_at" => ^any(:iso8601_naive_datetime) + }) = response + end end test "cannot move segment to another site by passing site_id param", %{ @@ -403,19 +417,19 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do user: user, site: site } do - victim_site = new_site() + other_site = new_site() segment = insert(:segment, site: site, owner: user, type: :personal, name: "test segment") patch(conn, "/api/#{site.domain}/segments/#{segment.id}", %{ "name" => "updated name", "type" => "personal", "segment_data" => %{"filters" => [["is", "visit:entry_page", ["/blog"]]], "labels" => %{}}, - "site_id" => victim_site.id + "site_id" => other_site.id }) reloaded = Repo.reload!(segment) assert reloaded.site_id == site.id - assert reloaded.site_id != victim_site.id + assert reloaded.site_id != other_site.id end end From 579c60108b104d539187db01fbb490ab0dfc7f53 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 8 Jul 2026 16:09:43 +0300 Subject: [PATCH 20/23] Fix issue with reassigning note owner --- lib/plausible/annotations/annotation.ex | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/plausible/annotations/annotation.ex b/lib/plausible/annotations/annotation.ex index ca36905d64fa..a2b78a78a5c2 100644 --- a/lib/plausible/annotations/annotation.ex +++ b/lib/plausible/annotations/annotation.ex @@ -53,9 +53,19 @@ defmodule Plausible.Annotations.Annotation do end def update_changeset(annotation, attrs, owner) do - annotation - |> changeset(attrs, annotation.site.timezone) - |> put_change(:owner_id, owner.id) + changeset = + annotation + |> changeset(attrs, annotation.site.timezone) + + # Users that can edit site annotations can edit + # 1) dangling site annotations (owner_id: nil), + # 2) site annotations owned by other users, + # in both cases becoming the new owners. + case fetch_field!(changeset, :owner_id) do + nil -> changeset |> put_assoc(:owner, owner) + # This case works around Ecto's limitations in overwriting owner association + _ -> changeset |> put_change(:owner_id, owner.id) + end end def changeset(annotation, attrs, site_timezone) do From 285ab55f2321ae07ad4df1285fa21ccea7485d3f Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 8 Jul 2026 16:18:45 +0300 Subject: [PATCH 21/23] Stop using any_timezone() helper --- test/plausible/annotations/annotation_test.exs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/test/plausible/annotations/annotation_test.exs b/test/plausible/annotations/annotation_test.exs index 6a0a8c681420..12dbfd59ba97 100644 --- a/test/plausible/annotations/annotation_test.exs +++ b/test/plausible/annotations/annotation_test.exs @@ -57,7 +57,7 @@ defmodule Plausible.Annotations.AnnotationTest do granularity: "date", date: unquote(Macro.escape(d)) }, - any_timezone() + Enum.random(Plausible.Timezones.zone_list()) ) assert changeset.valid? @@ -104,9 +104,12 @@ defmodule Plausible.Annotations.AnnotationTest do end for {dt, expected, tz} <- [ - {"2026-06-30T14:30:00Z", ~U[2026-06-30 14:30:00Z], any_timezone()}, - {"2026-06-30T10:00:00-02:00", ~U[2026-06-30 12:00:00Z], any_timezone()}, - {~U[2026-06-30 14:30:00Z], ~U[2026-06-30 14:30:00Z], any_timezone()}, + {"2026-06-30T14:30:00Z", ~U[2026-06-30 14:30:00Z], + Enum.random(Plausible.Timezones.zone_list())}, + {"2026-06-30T10:00:00-02:00", ~U[2026-06-30 12:00:00Z], + Enum.random(Plausible.Timezones.zone_list())}, + {~U[2026-06-30 14:30:00Z], ~U[2026-06-30 14:30:00Z], + Enum.random(Plausible.Timezones.zone_list())}, {"2026-06-30T14:30:00", ~U[2026-06-30 14:30:00Z], "Etc/UTC"}, {"2026-06-30T14:30:00", ~U[2026-06-30 11:30:00Z], "Europe/Tallinn"} ] do @@ -151,6 +154,4 @@ defmodule Plausible.Annotations.AnnotationTest do end end end - - defp any_timezone(), do: Enum.random(Plausible.Timezones.zone_list()) end From fcd5c4ed4e4a08674291241b2be7b697df301fb6 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 8 Jul 2026 16:48:38 +0300 Subject: [PATCH 22/23] Fix issue with dangling site notes shown to public role but not shown to authed role --- lib/plausible/annotations/annotations.ex | 2 +- .../annotations_controller_test.exs | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/lib/plausible/annotations/annotations.ex b/lib/plausible/annotations/annotations.ex index e446eb1f87b4..64d38b038795 100644 --- a/lib/plausible/annotations/annotations.ex +++ b/lib/plausible/annotations/annotations.ex @@ -59,7 +59,7 @@ defmodule Plausible.Annotations do Repo.all( from(annotation in Annotation, inner_join: site in assoc(annotation, :site), - inner_join: owner in assoc(annotation, :owner), + left_join: owner in assoc(annotation, :owner), select: ^fields, where: annotation.site_id == ^site.id, where: diff --git a/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs b/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs index a11663c29507..4821624af364 100644 --- a/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs +++ b/test/plausible_web/controllers/api/internal_controller/annotations_controller_test.exs @@ -94,11 +94,31 @@ defmodule PlausibleWeb.Api.Internal.AnnotationsControllerTest do updated_at: ~U[2026-07-01 12:00:00Z] ) + insert(:annotation, + site: site, + owner: nil, + type: :site, + note: "dangling", + inserted_at: ~U[2026-07-01 10:00:00Z], + updated_at: ~U[2026-07-01 13:00:00Z] + ) + conn = get(conn, "/api/#{site.domain}/annotations?date_range=day&relative_date=2026-01-04") results = json_response(conn, 200) assert_matches [ + ^strict_map(%{ + "id" => ^any(:pos_integer), + "note" => "dangling", + "type" => "site", + "datetime" => "2026-01-04", + "granularity" => "date", + "owner_id" => nil, + "owner_name" => nil, + "inserted_at" => ^any(:iso8601_naive_datetime), + "updated_at" => ^any(:iso8601_naive_datetime) + }), ^strict_map(%{ "id" => ^any(:pos_integer), "note" => "shared", From 7542f6e5bc1d4cdbfe0d28e2f3b7feaccb790f6f Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 8 Jul 2026 17:10:52 +0300 Subject: [PATCH 23/23] Handle failure to convert naive datetime to UTC for invalid timezone --- lib/plausible/annotations/annotation.ex | 27 +++++++++++++------ .../plausible/annotations/annotation_test.exs | 14 +++++++--- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/lib/plausible/annotations/annotation.ex b/lib/plausible/annotations/annotation.ex index a2b78a78a5c2..b01e13bd6197 100644 --- a/lib/plausible/annotations/annotation.ex +++ b/lib/plausible/annotations/annotation.ex @@ -94,19 +94,30 @@ defmodule Plausible.Annotations.Annotation do dt when is_binary(dt) <- changeset.params["datetime"], {:error, _} <- DateTime.from_iso8601(dt), {:ok, naive_dt} <- NaiveDateTime.from_iso8601(dt) do - utc_dt = - case DateTime.from_naive(naive_dt, timezone) do - {:ok, local_dt} -> DateTime.shift_zone!(local_dt, "Etc/UTC") - {:ambiguous, first, _second} -> DateTime.shift_zone!(first, "Etc/UTC") - {:gap, _just_before, just_after} -> DateTime.shift_zone!(just_after, "Etc/UTC") - end - - force_change(changeset, :datetime, utc_dt) + case DateTime.from_naive(naive_dt, timezone) do + {:ok, local_dt} -> + force_change(changeset, :datetime, to_utc(local_dt)) + + {:ambiguous, first, _second} -> + force_change(changeset, :datetime, to_utc(first)) + + {:gap, _just_before, just_after} -> + force_change(changeset, :datetime, to_utc(just_after)) + + {:error, _} -> + add_error( + changeset, + :datetime, + "cannot be parsed for the site timezone \"#{timezone}\"" + ) + end else _ -> changeset end end + defp to_utc(%DateTime{} = dt), do: DateTime.shift_zone!(dt, "Etc/UTC") + defp coerce_datetime(%{valid?: true} = changeset) do granularity = get_change(changeset, :granularity) datetime = get_change(changeset, :datetime) diff --git a/test/plausible/annotations/annotation_test.exs b/test/plausible/annotations/annotation_test.exs index 12dbfd59ba97..07b30ecdef25 100644 --- a/test/plausible/annotations/annotation_test.exs +++ b/test/plausible/annotations/annotation_test.exs @@ -85,8 +85,14 @@ defmodule Plausible.Annotations.AnnotationTest do end end - for dt <- ["2026-06-30", ~D[2026-07-01], "invalid"] do - test "rejects invalid :datetime, given #{dt}" do + for {dt, tz, message} <- [ + {"2026-06-30", "Etc/UTC", "is invalid"}, + {~D[2026-07-01], "Etc/UTC", "is invalid"}, + {"invalid", "Etc/UTC", "is invalid"}, + {"2026-06-30T14:30:00", "Invalid/Timezone", + "cannot be parsed for the site timezone \"Invalid/Timezone\""} + ] do + test "rejects invalid :datetime, given #{dt} for timezone #{tz}" do changeset = Annotation.changeset( %Annotation{}, @@ -96,10 +102,10 @@ defmodule Plausible.Annotations.AnnotationTest do granularity: "minute", datetime: unquote(Macro.escape(dt)) }, - "Etc/UTC" + unquote(tz) ) - assert [datetime: {"is invalid", _}] = changeset.errors + assert [datetime: {unquote(message), _}] = changeset.errors end end