Refactor Segments controller to be thinner

[apata]

Changes

Refactors Segments controller to be thinner.

Tests

• Automated tests are still in effect on the model through controller tests

Changelog

• This PR does not make a user-facing change

Documentation

• This change does not need a documentation update

Dark mode

• This PR does not change the UI

[RobertJoonas]

Let's move these to the top of the file, as they're pretty significant in this module.

I would also write them as module attributes instead of functions, since this way they'd only be evaluated once at compile time (instead of every time this function is called), e.g.:

@roles_with_personal_segments [:viewer, :editor, :admin, :owner, :super_admin]

Currently I don't see them being used elsewhere, other than in this file, so perhaps a module attribute should be enough?

But if we do need to expose them to other modules:

@roles_with_personal_segments [:viewer, :editor, :admin, :owner, :super_admin]

def roles_with_personal_segments, do: @roles_with_personal_segments

[RobertJoonas]

Considering the "site segments" feature equivalent to Custom Props, a large proportion of grandfathered customers on legacy plans (any plan before the plans_v4.json generation) will have access to it.

If we want to make it a new "Business feature", Funnels for example would be more appropriate at this stage. In long term though, I guess we're planning to introduce Feature.SiteSegments too right?

[RobertJoonas]

Sounds like a good candidate for a module attribute, so that it could be referenced by the query functions directly without the need to pass it around.

@index_query_fields [:id, :name, :type, :inserted_at, :updated_at, :owner_id]

[RobertJoonas]

I don't see a clear benefit of having functions that return an Ecto.Query here. WDYT about creating two functions that execute Repo.all straight away?

I also feel like making a clear separation between getting the "public site segments" vs all other segments is a good idea, due to the difference in the fields we want to fetch.

Proposal:

@spec get_public_site_segments(pos_integer()) :: [Segments.Segment.t()]
defp get_public_site_segments(site_id) do
  fields = @index_query_fields -- [:owner_id]

  Repo.all(
    from(s in Plausible.Segments.Segment,
      select: ^fields,
      where: s.site_id == ^site_id,
      where: s.type == :site,
      order_by: [desc: s.updated_at, desc: s.id]
    )
  )
end

@spec get_segments(pos_integer(), pos_integer()) :: [Segments.Segment.t()]
defp get_segments(user_id, site_id, opts \\ []) do
  q =
    from(s in Plausible.Segments.Segment,
      select: ^@index_query_fields,
      where: s.site_id == ^site_id,
      order_by: [desc: s.updated_at, desc: s.id]
    )

  q =
    if Keyword.get(opts, :only) == :personal do
      where(q, [s], s.type == :personal and s.owner_id == ^user_id)
    else
      where(q, [s], s.type == :site or (s.type == :personal and s.owner_id == ^user_id))
    end

  Repo.all(q)
end

This would make the index function much cleaner IMO:

def index(user_id, %Plausible.Site{} = site, site_role) do
  site_segments_available? =
    site_segments_available?(site)

  cond do
    site_role in [:public] and
        site_segments_available? ->
      {:ok, get_public_site_segments(site.id)}

    site_role in Segments.roles_with_maybe_site_segments() and
        site_segments_available? ->
      {:ok, get_segments(user_id, site.id)}

    site_role in Segments.roles_with_personal_segments() ->
      {:ok, get_segments(user_id, site.id, only: :personal)}

    true ->
      {:error, :not_enough_permissions}
  end
end

[RobertJoonas]

This looks redundant. Any function we want to reference from this module can be called without specifying any module in front of it.

I understand this currently makes it possible to call Segments.Segment, but I think that's redundant too. In this context module, the name Segment alone gives a perfectly clear idea of what it is, and doesn't need any prefix, I think.

Proposal: let's set alias Plausible.Segments.Segment here, and replace all occurrences of Segments.Segment and Plausible.Segments.Segment with just Segment.

[RobertJoonas]

Suggested change

	use Plausible.Repo
	alias Plausible.Repo
	import Ecto.Query

I left a comment below with some code suggestions that require Ecto.Query.where/3 as well as from/2 that is currently imported through use Plausible.Repo. This change is required for those functions to work.

[RobertJoonas]

If segment_id is missing or invalid, shouldn't we respond with a "400 bad request" here already? Unlike current_user_id it's something that is definitely required for operations like get / delete / update.

I'd suggest using a with statement here (and also in update and delete actions). Something like:

user_id = normalize_current_user_id(conn)

with {:ok, segment_id} <- get_segment_id_param(params["segment_id"]),
     {:ok, segment} <- Segments.get_one(user_id, site, site_role, segment_id) do
  json(conn, segment)
else
  {:error, :missing_or_invalid_segment_id} ->
    invalid_request(conn)

  {:error, :not_enough_permissions} ->
    H.not_enough_permissions(conn, "Not enough permissions to get segment data")

  {:error, :segment_not_found} ->
    segment_not_found(conn, params["segment_id"])
end

[RobertJoonas]

Good catch! 😁