Establish saved segments API

[apata]

Changes

Establishes the API for segments. Based on plan in segments.md.

Tests

• Automated tests have been added

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

[apata]

@ukutaht @aerosol
I tried out testing this endpoint with permission overrides, so we'd not have to mentally map from roles to permissions in tests.

Comparing this describe block with the one below it (old style tests for the same endpoint), what's your preference?

[zoldar]

I think we should really weigh the added complexity against the potential benefits we gain from using that granular permissions model.

From what I understand, the main concern here is ensuring exhaustive test coverage. Everything else aside, this is of course always desirable.

The very important aspect here is a safe failure mode. In case of your solution this is ensured by:

• unidentified role always returning empty list of permissions
• feature check explicitly matching on success case (:ok), treating all else as a fail

Which is great! New unknown role? Permission denied. Feature check changing contract by returning an {:ok, ...} instead? Permission denied.

This alone though can be achieved equally well with more coarse grained permission predicate checks like:

  def can_view_segment_data?(site, user) do
    case Plausible.Teams.Memberships.site_role(site, user) do
      {:ok, role} -> role in [:viewer, :editor, :admin, :owner, :super_admin]
      _ -> false
    end
  end

  def has_personal_segments?(site, user) do
    case Plausible.Teams.Memberships.site_role(site, user) do
      {:ok, role} -> role in [:viewer, :editor, :admin, :owner, :super_admin]
      _ -> false
    end
  end

  def has_site_segments?(site, _user) do
    Plausible.Billing.Feature.Props.check_availability(site.team) == :ok
  end

  def can_manage_site_segments?(site, user) do
    valid_role? =
      case Plausible.Teams.Memberships.site_role(site, user) do
        {:ok, role} -> role in [:editor, :admin, :owner, :super_admin]
        _ -> false
      end

    has_site_segments?(site, user) and valid_role?
  end

Although relatively simple, those predicates can be tested exhaustively in isolation as well.

We could treat them even as very coarse-grained permissions, with a plug like:

@checks %{
  :can_view_segment_data? => &can_view_segment_data?/2,
  :has_personal_segments? => &has_personal_segments?/2,
  :has_site_segments? => &has_site_segments?/2,
  :can_manage_site_segments? => &can_manage_site_segments?/2
}

def permissions_plug(conn, opts \\ []) do
  site = conn.assigns.site
  user = conn.assigns[:current_user]
  # we expect explicit enumeration of considered checks
  # it's also an optimization to avoid excess
  expected = opts

  permissions =
    Enum.reduce(expected, %{}, fn {check, permissions} ->
      check_fn = Map.fetch!(@checks, check)
      Map.put(permissions, check, check_fn.(site, user))
    end)
    
    
end

plug :permissions_plug, [:has_site_segments?, :has_personal_segments?]
          when action == :get_all_segments
...

# we can also use conn.assigns.permissions inside the body
# and switch logic inside with a conditional or delegate
# to another function;
# too elaborate pattern matching in function head really
# hinders readability
def get_all_segments(%{assigns: %{permissions: %{
  has_site_segments?: true, 
  has_personal_segments?: true}}} = conn, 
  _params
) do

Given plug config declaration must be explicit for every action, it's relatively easy to map them to particular predicates which in then map to expected roles and feature states.

For exhaustiveness, we will of course have to check every combination of feature and role involved, but test cases are going to be largely repetitive, which can be handled easily by "templating" tests with macros:

# role hierarchy could be even abstracted in a central set
# of helpers so that if we introduce anything new, tests will actually
# start failing, something like `TestHelpers.member_roles_from(:editor)`.
for role <- @editor_and_up do
  test "..." do
    ...
  end
end

While there is certain excess involved in tests like this, the more integration-like nature of them brings more confidence when testing permissions and controller actions separately (the current branch lacks permissions tests or at least some compile time guarantees).

Just my 2c 😅