Fix remaining review issues from PR #335 and #338

lib/modules/entities/web/data_navigator.ex

@@ -284,67 +284,79 @@ defmodule PhoenixKit.Modules.Entities.Web.DataNavigator do
   end
 
   def handle_event("archive_data", %{"uuid" => uuid}, socket) do
-    data_record = EntityData.get!(uuid)
+    if Scope.admin?(socket.assigns.phoenix_kit_current_scope) do
+      data_record = EntityData.get!(uuid)
 
-    case EntityData.update_data(data_record, %{status: "archived"}) do
-      {:ok, _data} ->
-        socket =
-          socket
-          |> apply_filters()
-          |> put_flash(:info, gettext("Data record archived successfully"))
+      case EntityData.update_data(data_record, %{status: "archived"}) do
+        {:ok, _data} ->
+          socket =
+            socket
+            |> apply_filters()
+            |> put_flash(:info, gettext("Data record archived successfully"))
 
-        {:noreply, socket}
+          {:noreply, socket}
 
-      {:error, _changeset} ->
-        socket = put_flash(socket, :error, gettext("Failed to archive data record"))
-        {:noreply, socket}
+        {:error, _changeset} ->
+          socket = put_flash(socket, :error, gettext("Failed to archive data record"))
+          {:noreply, socket}
+      end
+    else
+      {:noreply, put_flash(socket, :error, gettext("Not authorized"))}
     end
   end
 
   def handle_event("restore_data", %{"uuid" => uuid}, socket) do
-    data_record = EntityData.get!(uuid)
+    if Scope.admin?(socket.assigns.phoenix_kit_current_scope) do
+      data_record = EntityData.get!(uuid)
 
-    case EntityData.update_data(data_record, %{status: "published"}) do
-      {:ok, _data} ->
-        socket =
-          socket
-          |> apply_filters()
-          |> put_flash(:info, gettext("Data record restored successfully"))
+      case EntityData.update_data(data_record, %{status: "published"}) do
+        {:ok, _data} ->
+          socket =
+            socket
+            |> apply_filters()
+            |> put_flash(:info, gettext("Data record restored successfully"))
 
-        {:noreply, socket}
+          {:noreply, socket}
 
-      {:error, _changeset} ->
-        socket = put_flash(socket, :error, gettext("Failed to restore data record"))
-        {:noreply, socket}
+        {:error, _changeset} ->
+          socket = put_flash(socket, :error, gettext("Failed to restore data record"))
+          {:noreply, socket}
+      end
+    else
+      {:noreply, put_flash(socket, :error, gettext("Not authorized"))}
     end
   end
 
   def handle_event("toggle_status", %{"uuid" => uuid}, socket) do
-    data_record = EntityData.get!(uuid)
-
-    new_status =
-      case data_record.status do
-        "draft" -> "published"
-        "published" -> "archived"
-        "archived" -> "draft"
-      end
-
-    case EntityData.update_data(data_record, %{status: new_status}) do
-      {:ok, _updated_data} ->
-        socket =
-          socket
-          |> refresh_data_stats()
-          |> apply_filters()
-          |> put_flash(
-            :info,
-            gettext("Status updated to %{status}", status: status_label(new_status))
-          )
+    if Scope.admin?(socket.assigns.phoenix_kit_current_scope) do
+      data_record = EntityData.get!(uuid)
 
-        {:noreply, socket}
+      new_status =
+        case data_record.status do
+          "draft" -> "published"
+          "published" -> "archived"
+          "archived" -> "draft"
+        end
 
-      {:error, _changeset} ->
-        socket = put_flash(socket, :error, gettext("Failed to update status"))
-        {:noreply, socket}
+      case EntityData.update_data(data_record, %{status: new_status}) do
+        {:ok, _updated_data} ->
+          socket =
+            socket
+            |> refresh_data_stats()
+            |> apply_filters()
+            |> put_flash(
+              :info,
+              gettext("Status updated to %{status}", status: status_label(new_status))
+            )
+
+          {:noreply, socket}
+
+        {:error, _changeset} ->
+          socket = put_flash(socket, :error, gettext("Failed to update status"))
+          {:noreply, socket}
+      end
+    else
+      {:noreply, put_flash(socket, :error, gettext("Not authorized"))}
     end
   end
 

lib/modules/shop/schemas/category.ex

@@ -101,7 +101,7 @@ defmodule PhoenixKit.Modules.Shop.Category do
     |> validate_number(:position, greater_than_or_equal_to: 0)
     |> validate_inclusion(:status, @statuses)
     |> maybe_generate_slug()
-    |> validate_not_self_parent()
+    |> validate_no_circular_parent()
     |> unique_constraint(:slug, name: "idx_shop_categories_slug_primary")
   end
 
@@ -306,23 +306,38 @@ defmodule PhoenixKit.Modules.Shop.Category do
     end
   end
 
-  # Prevent category from being its own parent
-  defp validate_not_self_parent(changeset) do
+  # Prevent category from being its own parent or creating circular references
+  defp validate_no_circular_parent(changeset) do
     parent_uuid = get_change(changeset, :parent_uuid)
     category_uuid = changeset.data.uuid
 
-    parent_id = get_change(changeset, :parent_id)
-    category_id = changeset.data.id
-
     cond do
-      parent_uuid && parent_uuid == category_uuid ->
-        add_error(changeset, :parent_uuid, "cannot be self")
+      is_nil(parent_uuid) ->
+        changeset
 
-      parent_id && parent_id == category_id ->
-        add_error(changeset, :parent_id, "cannot be self")
+      parent_uuid == category_uuid ->
+        add_error(changeset, :parent_uuid, "cannot be self")
 
       true ->
+        check_ancestor_cycle(changeset, category_uuid, parent_uuid)
+    end
+  end
+
+  defp check_ancestor_cycle(changeset, target_uuid, current_uuid) do
+    repo = PhoenixKit.RepoHelper.repo()
+
+    case repo.get_by(__MODULE__, uuid: current_uuid) do
+      nil ->
+        changeset
+
+      %{parent_uuid: nil} ->
         changeset
+
+      %{parent_uuid: ^target_uuid} ->
+        add_error(changeset, :parent_uuid, "would create a circular reference")
+
+      %{parent_uuid: next_uuid} ->
+        check_ancestor_cycle(changeset, target_uuid, next_uuid)
     end
   end
 

lib/modules/shop/shop.ex

@@ -29,6 +29,7 @@ defmodule PhoenixKit.Modules.Shop do
   """
 
   import Ecto.Query, warn: false
+  require Logger
 
   alias PhoenixKit.Modules.Billing
   alias PhoenixKit.Modules.Billing.Currency
@@ -750,7 +751,6 @@ defmodule PhoenixKit.Modules.Shop do
     |> Map.new()
   rescue
     e ->
-      require Logger
       Logger.warning("Failed to load product counts by category: #{inspect(e)}")
       %{}
   end
@@ -978,8 +978,15 @@ defmodule PhoenixKit.Modules.Shop do
   to prevent self-reference. Uses a single UPDATE with subquery to resolve parent_id.
   """
   def bulk_update_category_parent(ids, parent_uuid) when is_list(ids) do
-    # Exclude the target parent from update set to prevent self-reference
-    ids_to_update = if parent_uuid, do: Enum.reject(ids, &(&1 == parent_uuid)), else: ids
+    # Exclude the target parent and its ancestors from update set to prevent cycles
+    ids_to_update =
+      if parent_uuid do
+        ancestors = collect_ancestor_uuids(parent_uuid, %{})
+
+        Enum.reject(ids, &(&1 == parent_uuid or Map.has_key?(ancestors, &1)))
+      else
+        ids
+      end
 
     if ids_to_update == [] do
       0
@@ -1019,6 +1026,19 @@ defmodule PhoenixKit.Modules.Shop do
     end
   end
 
+  defp collect_ancestor_uuids(nil, acc), do: acc
+
+  defp collect_ancestor_uuids(uuid, acc) do
+    if Map.has_key?(acc, uuid) do
+      acc
+    else
+      case repo().get_by(Category, uuid: uuid) do
+        nil -> acc
+        %{parent_uuid: parent} -> collect_ancestor_uuids(parent, Map.put(acc, uuid, true))
+      end
+    end
+  end
+
   @doc """
   Bulk delete categories.
   Returns count of deleted categories. Nullifies category references on orphaned products.
@@ -1140,6 +1160,7 @@ defmodule PhoenixKit.Modules.Shop do
   defp category_product_options_query(category_id) when is_integer(category_id) do
     from(p in Product,
       where: p.category_id == ^category_id,
+      where: p.status == "active",
       where:
         not is_nil(p.featured_image_id) or
           (not is_nil(p.featured_image) and p.featured_image != ""),
@@ -1152,6 +1173,7 @@ defmodule PhoenixKit.Modules.Shop do
     if match?({:ok, _}, Ecto.UUID.cast(category_id)) do
       from(p in Product,
         where: p.category_uuid == ^category_id,
+        where: p.status == "active",
         where:
           not is_nil(p.featured_image_id) or
             (not is_nil(p.featured_image) and p.featured_image != ""),