Working API

Author: IvanIvanoff

lib/sanbase/menu/menus.ex

@@ -46,10 +46,7 @@ defmodule Sanbase.Menus do
 
   @type update_menu_item_params :: %{
           optional(:parent_id) => menu_id,
-          optional(:position) => integer() | nil,
-          optional(:query_id) => Sanbase.Queries.Query.query_id(),
-          optional(:dashboard_id) => Sanbase.Queries.Dashboard.dashboard_id(),
-          optional(:menu_id) => menu_id
+          optional(:position) => integer() | nil
         }
 
   @doc ~s"""
@@ -77,6 +74,10 @@ defmodule Sanbase.Menus do
   """
   def menu_to_simple_map(%Menu{} = menu) do
     %{
+      # If this menu is a sub-menu, then the caller from get_menu_items/1 will
+      # additionally set the menu_item_id. If this is the top-level menu, then
+      # this is not a sub-menu and it does not have a menu_item_id
+      menu_item_id: nil,
       type: :menu,
       id: menu.id,
       name: menu.name,
@@ -95,7 +96,9 @@ defmodule Sanbase.Menus do
   """
   @spec create_menu(create_menu_params, user_id) :: {:ok, Menu.t()} | {:error, String.t()}
   def create_menu(params, user_id) do
-    params = params |> Map.merge(%{user_id: user_id})
+    params =
+      params
+      |> Map.merge(%{user_id: user_id})
 
     Ecto.Multi.new()
     |> Ecto.Multi.run(:create_menu, fn _repo, _changes ->
@@ -121,19 +124,13 @@ defmodule Sanbase.Menus do
           )
       end
     end)
-    # |> Ecto.Multi.run(
-    #   :get_menu_with_preloads,
-    #   fn _repo, %{create_menu: menu, maybe_create_menu_item: menu_or_nil} ->
-    #     # If the menu was created as a sub-menu, then the `maybe_create_menu_item` step
-    #     # has already returned the menu with preloads. Otherwise, we need to preload it here.
-    #     case menu_or_nil do
-    #       %Menu{} = m -> {:ok, m}
-    #       nil -> get_menu(menu.id, user_id)
-    #     end
-    #   end
-    # )
+    |> Ecto.Multi.run(:get_menu_with_preloads, fn _repo, %{create_menu: menu} ->
+      # There would be no menu items, but this will help to set the menu items to []
+      # instead of getting an error when trying to iterate them because they're set to <not preloaded>
+      get_menu(menu.id, user_id)
+    end)
     |> Repo.transaction()
-    |> process_transaction_result(:create_menu)
+    |> process_transaction_result(:get_menu_with_preloads)
   end
 
   @doc ~s"""
@@ -152,7 +149,7 @@ defmodule Sanbase.Menus do
       query = Menu.update(menu, params)
       Repo.update(query)
     end)
-    |> Ecto.Multi.run(:get_menu_with_preloads, fn _repo, %{create_menu: menu} ->
+    |> Ecto.Multi.run(:get_menu_with_preloads, fn _repo, %{update_menu: menu} ->
       get_menu(menu.id, user_id)
     end)
     |> Repo.transaction()
@@ -168,11 +165,19 @@ defmodule Sanbase.Menus do
     |> Ecto.Multi.run(:get_menu_for_update, fn _repo, _changes ->
       get_menu_for_update(menu_id, user_id)
     end)
+    |> Ecto.Multi.run(:get_menu_with_preloads, fn _repo, _changes ->
+      # Call this so we can return the menu with its menu items after it is
+      # successfully deleted
+      get_menu(menu_id, user_id)
+    end)
     |> Ecto.Multi.run(:delete_menu, fn _repo, %{get_menu_for_update: menu} ->
       Repo.delete(menu)
     end)
     |> Repo.transaction()
-    |> process_transaction_result(:delete_menu)
+    # Purposefully do not return the result of the last Ecto.Multi.run call,
+    # but from the get_menu_with_preloads call, so we can return the menu with
+    # its items.
+    |> process_transaction_result(:get_menu_with_preloads)
   end
 
   @doc ~s"""
@@ -234,45 +239,63 @@ defmodule Sanbase.Menus do
 
   @doc ~s"""
   Update an existing menu item.
+
+  A menu item can have the follwing fields updated:
+    - position - change the position of the item in the menu
+    - parent_id - change the parent menu of the item. On the frontend this is done
+      by dragging and dropping the item in the menu tree (this can also update the position)
+
+  The entity (query, dashboard, etc.) cannot be changed. Delete a menu item and insert a new
+  one instead.
   """
-  @spec update_menu_item(menu_id, menu_item_id, update_menu_item_params, user_id) ::
+  @spec update_menu_item(menu_item_id, update_menu_item_params, user_id) ::
           {:ok, Menu.t()} | {:error, String.t()}
-  def update_menu_item(menu_id, menu_item_id, params, user_id) do
+  def update_menu_item(menu_item_id, params, user_id) do
     Ecto.Multi.new()
-    |> Ecto.Multi.run(:get_menu_for_update, fn _repo, _changes ->
-      # Just check that the current user can update the parent menu
-      get_menu_for_update(menu_id, user_id)
-    end)
     |> Ecto.Multi.run(:get_menu_item_for_update, fn _repo, _changes ->
-      get_menu_item_for_update(menu_item_id)
+      get_menu_item_for_update(menu_item_id, user_id)
     end)
+    |> Ecto.Multi.run(
+      :maybe_update_items_positions,
+      fn _repo, %{get_menu_item_for_update: menu_item} ->
+        case Map.get(params, :position) do
+          nil ->
+            {:ok, nil}
+
+          position when is_integer(position) ->
+            # If `position` is specified, bump all the positions bigger than it by 1 in
+            # order to avoid having multiple items with the same position.
+            {:ok, {_, nil}} = inc_all_positions_after(menu_item.parent_id, position)
+            {:ok, position}
+        end
+      end
+    )
     |> Ecto.Multi.run(:update_menu_item, fn _repo, %{get_menu_item_for_update: menu_item} ->
-      # Handle change of entity
-      params = MenuItem.process_update_params(params)
       query = MenuItem.update(menu_item, params)
       Repo.update(query)
     end)
-    |> Ecto.Multi.run(:get_menu_with_preloads, fn _repo, %{get_menu_for_update: menu} ->
-      get_menu(menu.id, user_id)
+    |> Ecto.Multi.run(:get_menu_with_preloads, fn _repo, %{update_menu_item: menu_item} ->
+      get_menu(menu_item.parent_id, user_id)
     end)
     |> Repo.transaction()
     |> process_transaction_result(:get_menu_with_preloads)
   end
 
-  def delete_menu_item(menu_id, menu_item_id, user_id) do
+  @doc ~s"""
+  Delete a menu item.
+  """
+  @spec delete_menu_item(menu_item_id, user_id) ::
+          {:ok, Menu.t()} | {:error, String.t()}
+  def delete_menu_item(menu_item_id, user_id) do
     Ecto.Multi.new()
-    |> Ecto.Multi.run(:check_user_has_write_access, fn _repo, _changes ->
-      # Just check that the current user can update the parent menu
-      get_menu_for_update(menu_id, user_id)
-    end)
     |> Ecto.Multi.run(:get_menu_item, fn _repo, _changes ->
-      get_menu_item_for_update(menu_item_id)
+      get_menu_item_for_update(menu_item_id, user_id)
     end)
     |> Ecto.Multi.run(:delete_menu_item, fn _repo, %{get_menu_item: menu_item} ->
       Repo.delete(menu_item)
     end)
-    |> Ecto.Multi.run(:get_menu_with_preloads, fn _repo, %{create_menu: menu} ->
-      get_menu(menu.id, user_id)
+    |> Ecto.Multi.run(:get_menu_with_preloads, fn _repo, %{delete_menu_item: menu_item} ->
+      get_menu(menu_item.parent_id, user_id)
     end)
     |> Repo.transaction()
     |> process_transaction_result(:get_menu_with_preloads)
@@ -289,11 +312,8 @@ defmodule Sanbase.Menus do
     end
   end
 
-  defp get_menu_item_for_update(menu_item_id) do
-    # This should be called only after checking that the user
-    # has write access to the menu. This is done by calling
-    # get_menu_for_update/2 first.
-    query = MenuItem.get_for_update(menu_item_id)
+  defp get_menu_item_for_update(menu_item_id, user_id) do
+    query = MenuItem.get_for_update(menu_item_id, user_id)
 
     case Repo.one(query) do
       nil -> {:error, "Menu item does not exist"}
@@ -339,14 +359,17 @@ defmodule Sanbase.Menus do
   defp get_menu_items(%Menu{menu_items: list}) when is_list(list) do
     list
     |> Enum.map(fn
-      %{query: %{id: _} = map, position: p} ->
-        Map.take(map, [:id, :name, :description]) |> Map.merge(%{type: :query, position: p})
+      %{id: menu_item_id, query: %{id: _} = map, position: position} ->
+        Map.take(map, [:id, :name, :description])
+        |> Map.merge(%{type: :query, position: position, menu_item_id: menu_item_id})
 
-      %{dashboard: %{id: _} = map, position: p} ->
-        Map.take(map, [:id, :name, :description]) |> Map.merge(%{type: :dashboard, position: p})
+      %{id: menu_item_id, dashboard: %{id: _} = map, position: position} ->
+        Map.take(map, [:id, :name, :description])
+        |> Map.merge(%{type: :dashboard, position: position, menu_item_id: menu_item_id})
 
-      %{menu: %{id: _} = map, position: p} ->
-        menu_to_simple_map(map) |> Map.put(:position, p)
+      %{id: menu_item_id, menu: %{id: _} = map, position: position} ->
+        menu_to_simple_map(map)
+        |> Map.merge(%{type: :menu, position: position, menu_item_id: menu_item_id})
     end)
   end
 end

lib/sanbase/menu/schema/menu_item.ex

@@ -18,10 +18,6 @@ defmodule Sanbase.Menus.MenuItem do
           updated_at: DateTime.t()
         }
 
-  @enitity_fields [:query_id, :dashboard_id, :menu_id]
-  @enitity_fields_nil_map Map.new(@enitity_fields, fn k -> {k, nil} end)
-  def entity_fields(), do: @enitity_fields
-
   @timestamps_opts [type: :utc_datetime]
   schema "menu_items" do
     belongs_to(:parent, Menu)
@@ -35,9 +31,10 @@ defmodule Sanbase.Menus.MenuItem do
     timestamps()
   end
 
-  def get_for_update(id) do
+  def get_for_update(id, user_id) do
     base_query()
-    |> where([m], m.id == ^id)
+    |> join(:left, [mi], p in Menu, on: mi.parent_id == p.id, as: :parent)
+    |> where([mi, parent: p], mi.id == ^id and p.user_id == ^user_id)
     |> lock("FOR UPDATE")
   end
 
@@ -77,31 +74,10 @@ defmodule Sanbase.Menus.MenuItem do
     |> validate_required([:parent_id, :position])
   end
 
-  def process_update_params(params) do
-    case Enum.find(params, fn {k, v} -> k in @enitity_fields and not is_nil(v) end) do
-      # If a new entity is provided, we need to nullify the old entities by
-      # explicitly setting them to nil. We can set all of them to nil because
-      # when Ecto generates the update query, it will explicitly update only the
-      # existing field as it will differ from the value in the struct.
-      # Put back the provided entity id in the params after nullifying.
-      {k, v} -> params |> Map.merge(@enitity_fields_nil_map) |> Map.put(k, v)
-      nil -> params
-    end
-  end
-
   def update(menu, attrs) do
-    %__MODULE__{}
-    |> cast(attrs, [
-      # Who this item belongs to
-      :parent_id,
-      # What is the item. There's check constraint on the DB level that only one
-      # of these can be set
-      :menu_id,
-      :query_id,
-      :dashboard_id,
-      # The position of the item in the menu
-      :position
-    ])
+    menu
+    # Do not allow to change the entity. Prefer deleting and adding a new item instead.
+    |> cast(attrs, [:parent_id, :position])
   end
 
   # Private functions

lib/sanbase/run_examples.ex

@@ -784,6 +784,7 @@ defmodule Sanbase.RunExamples do
 
   defp do_run(:menus) do
     user = Sanbase.Factory.insert(:user)
+    user2 = Sanbase.Factory.insert(:user)
 
     {:ok, query} = Sanbase.Queries.create_query(%{name: "Query"}, user.id)
     {:ok, dashboard} = Sanbase.Dashboards.create_dashboard(%{name: "Dashboard"}, user.id)
@@ -803,6 +804,13 @@ defmodule Sanbase.RunExamples do
         user.id
       )
 
+    # Cannot create item on non-owner menu
+    {:error, _} =
+      Sanbase.Menus.create_menu_item(
+        %{parent_id: menu.id, dashboard_id: dashboard.id, position: 2},
+        user2.id
+      )
+
     {:ok, sub_menu} =
       Sanbase.Menus.create_menu(
         %{
@@ -814,6 +822,9 @@ defmodule Sanbase.RunExamples do
         user.id
       )
 
+    {:ok, _} = Sanbase.Menus.update_menu(sub_menu.id, %{name: "MySubMenuNewName"}, user.id)
+    # Cannot update non-owner menu
+    {:error, _} = Sanbase.Menus.update_menu(sub_menu.id, %{name: "hehe"}, user2.id)
     {:ok, fetched_menu} = Sanbase.Menus.get_menu(menu.id, user.id)
 
     menu_id = menu.id
@@ -829,7 +840,7 @@ defmodule Sanbase.RunExamples do
           description: "MySubDescription",
           id: ^sub_menu_id,
           menu_items: [],
-          name: "MySubMenu",
+          name: "MySubMenuNewName",
           position: 1,
           type: :menu
         },