From e3bea1782789e65a903b719f94f61615f9964a0f Mon Sep 17 00:00:00 2001
From: Moritz <moritz.m@local-it.org>
Date: Tue, 3 Feb 2026 23:52:24 +0100
Subject: [PATCH] Member show & MembershipFees: permissions, delete all,
 regenerate, errors

- Show: handle_info :member_updated and :put_flash; Linked User only when can_access_page? /users
- MembershipFeesComponent: can_create_cycle/can_destroy_cycle/can_update_cycle; buttons gated
- Delete all cycles via Ash.destroy (policy enforced); format_error Forbidden
- Regenerate cycles for normal_user and admin (no admin-only check)
- Member form: format_error tuple for membership_fee_type_id; Select a membership fee type (no None)
- show_membership_fees_test: read_only UI and policy tests
---
 lib/mv_web/live/member_live/form.ex           |   6 +-
 lib/mv_web/live/member_live/show.ex           |  53 +++--
 .../show/membership_fees_component.ex         | 222 +++++++++---------
 .../member_live/show_membership_fees_test.exs |  61 +++++
 4 files changed, 219 insertions(+), 123 deletions(-)

diff --git a/lib/mv_web/live/member_live/form.ex b/lib/mv_web/live/member_live/form.ex
index b72add6..f9588c0 100644
--- a/lib/mv_web/live/member_live/form.ex
+++ b/lib/mv_web/live/member_live/form.ex
@@ -177,7 +177,8 @@ defmodule MvWeb.MemberLive.Form do
                   phx-change="validate"
                   value={@form[:membership_fee_type_id].value || ""}
                 >
-                  <option value="">{gettext("None")}</option>
+                  <%!-- No "None" option: a membership fee type is required (validated in Member resource). --%>
+                  <option value="">{gettext("Select a membership fee type")}</option>
                   <%= for fee_type <- @available_fee_types do %>
                     <option
                       value={fee_type.id}
@@ -189,7 +190,8 @@ defmodule MvWeb.MemberLive.Form do
                     </option>
                   <% end %>
                 </select>
-                <%= for {msg, _opts} <- @form.errors[:membership_fee_type_id] || [] do %>
+                <%= for error <- List.wrap(@form.errors[:membership_fee_type_id] || []) do %>
+                  <% {msg, _opts} = if is_tuple(error), do: error, else: {error, []} %>
                   <p class="text-error text-sm mt-1">{msg}</p>
                 <% end %>
                 <%= if @interval_warning do %>
diff --git a/lib/mv_web/live/member_live/show.ex b/lib/mv_web/live/member_live/show.ex
index 604f166..40491cc 100644
--- a/lib/mv_web/live/member_live/show.ex
+++ b/lib/mv_web/live/member_live/show.ex
@@ -125,22 +125,26 @@ defmodule MvWeb.MemberLive.Show do
                   />
                 </div>
 
-                <%!-- Linked User --%>
-                <div>
-                  <.data_field label={gettext("Linked User")}>
-                    <%= if @member.user do %>
-                      <.link
-                        navigate={~p"/users/#{@member.user}"}
-                        class="text-blue-700 hover:text-blue-800 underline inline-flex items-center gap-1"
-                      >
-                        <.icon name="hero-user" class="size-4" />
-                        {@member.user.email}
-                      </.link>
-                    <% else %>
-                      <span class="text-base-content/70 italic">{gettext("No user linked")}</span>
-                    <% end %>
-                  </.data_field>
-                </div>
+                <%!-- Linked User: only show when current user can see other users (e.g. admin).
+                  read_only cannot see linked user, so hide the section to avoid "No user linked" when
+                  a user is linked but not visible. --%>
+                <%= if can_access_page?(@current_user, "/users") do %>
+                  <div>
+                    <.data_field label={gettext("Linked User")}>
+                      <%= if @member.user do %>
+                        <.link
+                          navigate={~p"/users/#{@member.user}"}
+                          class="text-blue-700 hover:text-blue-800 underline inline-flex items-center gap-1"
+                        >
+                          <.icon name="hero-user" class="size-4" />
+                          {@member.user.email}
+                        </.link>
+                      <% else %>
+                        <span class="text-base-content/70 italic">{gettext("No user linked")}</span>
+                      <% end %>
+                    </.data_field>
+                  </div>
+                <% end %>
 
                 <%!-- Notes --%>
                 <%= if @member.notes && String.trim(@member.notes) != "" do %>
@@ -287,6 +291,23 @@ defmodule MvWeb.MemberLive.Show do
     {:noreply, assign(socket, :active_tab, :membership_fees)}
   end
 
+  # Flash set in LiveComponent is not shown in parent layout; child sends this to display flash
+  @impl true
+  def handle_info({:put_flash, type, message}, socket) do
+    {:noreply, put_flash(socket, type, message)}
+  end
+
+  # MembershipFeesComponent sends this after cycles are created/deleted/regenerated so parent keeps member in sync
+  @impl true
+  def handle_info({:member_updated, updated_member}, socket) do
+    member =
+      updated_member
+      |> Map.put(:last_cycle_status, get_last_cycle_status(updated_member))
+      |> Map.put(:current_cycle_status, get_current_cycle_status(updated_member))
+
+    {:noreply, assign(socket, :member, member)}
+  end
+
   defp page_title(:show), do: gettext("Show Member")
   defp page_title(:edit), do: gettext("Edit Member")
 
diff --git a/lib/mv_web/live/member_live/show/membership_fees_component.ex b/lib/mv_web/live/member_live/show/membership_fees_component.ex
index 5350e9f..e839d16 100644
--- a/lib/mv_web/live/member_live/show/membership_fees_component.ex
+++ b/lib/mv_web/live/member_live/show/membership_fees_component.ex
@@ -14,6 +14,7 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do
 
   require Ash.Query
   import MvWeb.LiveHelpers, only: [current_actor: 1]
+  import MvWeb.Authorization, only: [can?: 3]
 
   alias Mv.Membership
   alias Mv.MembershipFees
@@ -49,9 +50,10 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do
           <% end %>
         </div>
 
-        <%!-- Action Buttons --%>
+        <%!-- Action Buttons (only when user has permission) --%>
         <div class="flex gap-2 mb-4">
           <.button
+            :if={@can_create_cycle}
             phx-click="regenerate_cycles"
             phx-target={@myself}
             class={["btn btn-sm btn-outline", if(@regenerating, do: "btn-disabled", else: "")]}
@@ -61,7 +63,7 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do
             {if(@regenerating, do: gettext("Regenerating..."), else: gettext("Regenerate Cycles"))}
           </.button>
           <.button
-            :if={Enum.any?(@cycles)}
+            :if={Enum.any?(@cycles) and @can_destroy_cycle}
             phx-click="delete_all_cycles"
             phx-target={@myself}
             class="btn btn-sm btn-error btn-outline"
@@ -71,7 +73,7 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do
             {gettext("Delete All Cycles")}
           </.button>
           <.button
-            :if={@member.membership_fee_type}
+            :if={@member.membership_fee_type != nil and @can_create_cycle}
             phx-click="open_create_cycle_modal"
             phx-target={@myself}
             class="btn btn-sm btn-primary"
@@ -103,15 +105,19 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do
             </:col>
 
             <:col :let={cycle} label={gettext("Amount")}>
-              <span
-                class="font-mono cursor-pointer hover:text-primary"
-                phx-click="edit_cycle_amount"
-                phx-value-cycle_id={cycle.id}
-                phx-target={@myself}
-                title={gettext("Click to edit amount")}
-              >
-                {MembershipFeeHelpers.format_currency(cycle.amount)}
-              </span>
+              <%= if @can_update_cycle do %>
+                <span
+                  class="font-mono cursor-pointer hover:text-primary"
+                  phx-click="edit_cycle_amount"
+                  phx-value-cycle_id={cycle.id}
+                  phx-target={@myself}
+                  title={gettext("Click to edit amount")}
+                >
+                  {MembershipFeeHelpers.format_currency(cycle.amount)}
+                </span>
+              <% else %>
+                <span class="font-mono">{MembershipFeeHelpers.format_currency(cycle.amount)}</span>
+              <% end %>
             </:col>
 
             <:col :let={cycle} label={gettext("Status")}>
@@ -125,56 +131,60 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do
 
             <:action :let={cycle}>
               <div class="flex gap-1">
-                <button
-                  :if={cycle.status != :paid}
-                  type="button"
-                  phx-click="mark_cycle_status"
-                  phx-value-cycle_id={cycle.id}
-                  phx-value-status="paid"
-                  phx-target={@myself}
-                  class="btn btn-sm btn-success"
-                  title={gettext("Mark as paid")}
-                >
-                  <.icon name="hero-check-circle" class="size-4" />
-                  {gettext("Paid")}
-                </button>
-                <button
-                  :if={cycle.status != :suspended}
-                  type="button"
-                  phx-click="mark_cycle_status"
-                  phx-value-cycle_id={cycle.id}
-                  phx-value-status="suspended"
-                  phx-target={@myself}
-                  class="btn btn-sm btn-outline btn-warning"
-                  title={gettext("Mark as suspended")}
-                >
-                  <.icon name="hero-pause-circle" class="size-4" />
-                  {gettext("Suspended")}
-                </button>
-                <button
-                  :if={cycle.status != :unpaid}
-                  type="button"
-                  phx-click="mark_cycle_status"
-                  phx-value-cycle_id={cycle.id}
-                  phx-value-status="unpaid"
-                  phx-target={@myself}
-                  class="btn btn-sm btn-error"
-                  title={gettext("Mark as unpaid")}
-                >
-                  <.icon name="hero-x-circle" class="size-4" />
-                  {gettext("Unpaid")}
-                </button>
-                <button
-                  type="button"
-                  phx-click="delete_cycle"
-                  phx-value-cycle_id={cycle.id}
-                  phx-target={@myself}
-                  class="btn btn-sm btn-error btn-outline"
-                  title={gettext("Delete cycle")}
-                >
-                  <.icon name="hero-trash" class="size-4" />
-                  {gettext("Delete")}
-                </button>
+                <%= if @can_update_cycle do %>
+                  <button
+                    :if={cycle.status != :paid}
+                    type="button"
+                    phx-click="mark_cycle_status"
+                    phx-value-cycle_id={cycle.id}
+                    phx-value-status="paid"
+                    phx-target={@myself}
+                    class="btn btn-sm btn-success"
+                    title={gettext("Mark as paid")}
+                  >
+                    <.icon name="hero-check-circle" class="size-4" />
+                    {gettext("Paid")}
+                  </button>
+                  <button
+                    :if={cycle.status != :suspended}
+                    type="button"
+                    phx-click="mark_cycle_status"
+                    phx-value-cycle_id={cycle.id}
+                    phx-value-status="suspended"
+                    phx-target={@myself}
+                    class="btn btn-sm btn-outline btn-warning"
+                    title={gettext("Mark as suspended")}
+                  >
+                    <.icon name="hero-pause-circle" class="size-4" />
+                    {gettext("Suspended")}
+                  </button>
+                  <button
+                    :if={cycle.status != :unpaid}
+                    type="button"
+                    phx-click="mark_cycle_status"
+                    phx-value-cycle_id={cycle.id}
+                    phx-value-status="unpaid"
+                    phx-target={@myself}
+                    class="btn btn-sm btn-error"
+                    title={gettext("Mark as unpaid")}
+                  >
+                    <.icon name="hero-x-circle" class="size-4" />
+                    {gettext("Unpaid")}
+                  </button>
+                <% end %>
+                <%= if @can_destroy_cycle do %>
+                  <button
+                    type="button"
+                    phx-click="delete_cycle"
+                    phx-value-cycle_id={cycle.id}
+                    phx-target={@myself}
+                    class="btn btn-sm btn-error btn-outline"
+                    title={gettext("Delete cycle")}
+                  >
+                    <.icon name="hero-trash" class="size-4" />
+                    {gettext("Delete")}
+                  </button>
+                <% end %>
               </div>
             </:action>
           </.table>
@@ -408,11 +418,19 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do
     # Get available fee types (filtered to same interval if member has a type)
     available_fee_types = get_available_fee_types(member, actor)
 
+    # Permission flags for cycle actions (so read_only does not see create/update/destroy UI)
+    can_create_cycle = can?(actor, :create, MembershipFeeCycle)
+    can_destroy_cycle = can?(actor, :destroy, MembershipFeeCycle)
+    can_update_cycle = can?(actor, :update, MembershipFeeCycle)
+
     {:ok,
      socket
      |> assign(assigns)
      |> assign(:cycles, cycles)
      |> assign(:available_fee_types, available_fee_types)
+     |> assign(:can_create_cycle, can_create_cycle)
+     |> assign(:can_destroy_cycle, can_destroy_cycle)
+     |> assign(:can_update_cycle, can_update_cycle)
      |> assign_new(:interval_warning, fn -> nil end)
      |> assign_new(:editing_cycle, fn -> nil end)
      |> assign_new(:deleting_cycle, fn -> nil end)
@@ -554,55 +572,45 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do
   end
 
   def handle_event("regenerate_cycles", _params, socket) do
-    actor = current_actor(socket)
+    # Button is only shown when can_create_cycle (normal_user and admin). Cycle generation uses system actor.
+    socket = assign(socket, :regenerating, true)
+    member = socket.assigns.member
 
-    # SECURITY: Only admins can manually regenerate cycles via UI
-    # Cycle generation itself uses system actor, but UI access should be restricted
-    if actor.role && actor.role.permission_set_name == "admin" do
-      socket = assign(socket, :regenerating, true)
-      member = socket.assigns.member
+    case CycleGenerator.generate_cycles_for_member(member.id) do
+      {:ok, _new_cycles, _notifications} ->
+        actor = current_actor(socket)
 
-      case CycleGenerator.generate_cycles_for_member(member.id) do
-        {:ok, _new_cycles, _notifications} ->
-          # Reload member with cycles
-          actor = current_actor(socket)
+        updated_member =
+          member
+          |> Ash.load!(
+            [
+              :membership_fee_type,
+              membership_fee_cycles: [:membership_fee_type]
+            ],
+            actor: actor
+          )
 
-          updated_member =
-            member
-            |> Ash.load!(
-              [
-                :membership_fee_type,
-                membership_fee_cycles: [:membership_fee_type]
-              ],
-              actor: actor
-            )
+        cycles =
+          Enum.sort_by(
+            updated_member.membership_fee_cycles || [],
+            & &1.cycle_start,
+            {:desc, Date}
+          )
 
-          cycles =
-            Enum.sort_by(
-              updated_member.membership_fee_cycles || [],
-              & &1.cycle_start,
-              {:desc, Date}
-            )
+        send(self(), {:member_updated, updated_member})
 
-          send(self(), {:member_updated, updated_member})
+        {:noreply,
+         socket
+         |> assign(:member, updated_member)
+         |> assign(:cycles, cycles)
+         |> assign(:regenerating, false)
+         |> put_flash(:info, gettext("Cycles regenerated successfully"))}
 
-          {:noreply,
-           socket
-           |> assign(:member, updated_member)
-           |> assign(:cycles, cycles)
-           |> assign(:regenerating, false)
-           |> put_flash(:info, gettext("Cycles regenerated successfully"))}
-
-        {:error, error} ->
-          {:noreply,
-           socket
-           |> assign(:regenerating, false)
-           |> put_flash(:error, format_error(error))}
-      end
-    else
-      {:noreply,
-       socket
-       |> put_flash(:error, gettext("Only administrators can regenerate cycles"))}
+      {:error, error} ->
+        {:noreply,
+         socket
+         |> assign(:regenerating, false)
+         |> put_flash(:error, format_error(error))}
     end
   end
 
@@ -940,6 +948,10 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do
     Enum.map_join(error.errors, ", ", fn e -> e.message end)
   end
 
+  defp format_error(%Ash.Error.Forbidden{}) do
+    gettext("You are not allowed to perform this action.")
+  end
+
   defp format_error(error) when is_binary(error), do: error
   defp format_error(_error), do: gettext("An error occurred")
 
diff --git a/test/mv_web/member_live/show_membership_fees_test.exs b/test/mv_web/member_live/show_membership_fees_test.exs
index 20bf46d..5636d2b 100644
--- a/test/mv_web/member_live/show_membership_fees_test.exs
+++ b/test/mv_web/member_live/show_membership_fees_test.exs
@@ -274,4 +274,65 @@ defmodule MvWeb.MemberLive.ShowMembershipFeesTest do
       assert html =~ member.first_name
     end
   end
+
+  describe "read_only user (Vorstand/Buchhaltung) - no cycle action buttons" do
+    @tag role: :read_only
+    test "read_only does not see Regenerate Cycles, Delete All Cycles, or Create Cycle buttons",
+         %{
+           conn: conn
+         } do
+      fee_type = create_fee_type(%{interval: :yearly})
+      member = create_member(%{membership_fee_type_id: fee_type.id})
+      _cycle = create_cycle(member, fee_type, %{cycle_start: ~D[2023-01-01], status: :unpaid})
+
+      {:ok, view, _html} = live(conn, "/members/#{member.id}")
+
+      view
+      |> element("button[phx-click='switch_tab'][phx-value-tab='membership_fees']")
+      |> render_click()
+
+      refute has_element?(view, "button[phx-click='regenerate_cycles']")
+      refute has_element?(view, "button[phx-click='delete_all_cycles']")
+      refute has_element?(view, "button[phx-click='open_create_cycle_modal']")
+    end
+
+    @tag role: :read_only
+    test "read_only does not see Paid, Unpaid, Suspended, or Delete buttons in cycles table", %{
+      conn: conn
+    } do
+      fee_type = create_fee_type(%{interval: :yearly})
+      member = create_member(%{membership_fee_type_id: fee_type.id})
+      cycle = create_cycle(member, fee_type, %{cycle_start: ~D[2023-01-01], status: :unpaid})
+
+      {:ok, view, _html} = live(conn, "/members/#{member.id}")
+
+      view
+      |> element("button[phx-click='switch_tab'][phx-value-tab='membership_fees']")
+      |> render_click()
+
+      # Row action buttons must not be present for read_only
+      refute has_element?(view, "button[phx-click='mark_cycle_status']")
+      refute has_element?(view, "button[phx-click='delete_cycle']")
+      # Sanity: cycle row is present (read is allowed)
+      assert has_element?(view, "tr[id='cycle-#{cycle.id}']")
+    end
+  end
+
+  describe "read_only cannot delete all cycles (policy enforced via Ash.destroy)" do
+    @tag role: :read_only
+    test "confirm_delete_all_cycles returns error for read_only user", %{
+      current_user: read_only_user
+    } do
+      # Backend policy test: read_only cannot destroy any cycle.
+      # The UI hides the Delete All button for read_only; this test ensures
+      # that if the handler were triggered (e.g. via dev tools), the server
+      # would enforce policy and return Forbidden.
+      fee_type = create_fee_type(%{interval: :yearly})
+      member = create_member(%{membership_fee_type_id: fee_type.id})
+      cycle = create_cycle(member, fee_type, %{cycle_start: ~D[2023-01-01], status: :unpaid})
+
+      assert {:error, %Ash.Error.Forbidden{}} =
+               Ash.destroy(cycle, domain: Mv.MembershipFees, actor: read_only_user)
+    end
+  end
 end