return data only for updated emoji

2020-02-06 18:01:12 +03:00 · 2020-02-06 18:01:12 +03:00 · 5839e67eb8
parent 4c4344b7b1
commit 5839e67eb8
5 changed files with 149 additions and 115 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -125,13 +125,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 <details>
  <summary>API Changes</summary>

- **Breaking** EmojiReactions: Change endpoints and responses to align with Mastodon
- **Breaking** Admin API: `PATCH /api/pleroma/admin/users/:nickname/force_password_reset` is now `PATCH /api/pleroma/admin/users/force_password_reset` (accepts `nicknames` array in the request body)
+- **Breaking:** EmojiReactions: Change endpoints and responses to align with Mastodon
+- **Breaking:** Admin API: `PATCH /api/pleroma/admin/users/:nickname/force_password_reset` is now `PATCH /api/pleroma/admin/users/force_password_reset` (accepts `nicknames` array in the request body)
 - **Breaking:** Admin API: Return link alongside with token on password reset
 - **Breaking:** Admin API: `PUT /api/pleroma/admin/reports/:id` is now `PATCH /api/pleroma/admin/reports`, see admin_api.md for details
 - **Breaking:** `/api/pleroma/admin/users/invite_token` now uses `POST`, changed accepted params and returns full invite in json instead of only token string.
- **Breaking** replying to reports is now "report notes", enpoint changed from `POST /api/pleroma/admin/reports/:id/respond` to `POST /api/pleroma/admin/reports/:id/notes`
+- **Breaking:** replying to reports is now "report notes", endpoint changed from `POST /api/pleroma/admin/reports/:id/respond` to `POST /api/pleroma/admin/reports/:id/notes`
 - Mastodon API: stopped sanitizing display names, field names and subject fields since they are supposed to be treated as plaintext
+- **Breaking:** Pleroma API: `/api/pleroma/emoji/packs/:name/update_file` endpoint returns only updated emoji data.
 - Admin API: Return `total` when querying for reports
 - Mastodon API: Return `pleroma.direct_conversation_id` when creating a direct message (`POST /api/v1/statuses`)
 - Admin API: Return link alongside with token on password reset
--- a/docs/API/pleroma_api.md
+++ b/docs/API/pleroma_api.md
@ -357,7 +357,7 @@ The status posting endpoint takes an additional parameter, `in_reply_to_conversa
    * if the `action` is `update`, changes emoji shortcode
      (from `shortcode` to `new_shortcode` or moves the file (from the current filename to `new_filename`)
    * if the `action` is `remove`, removes the emoji named `shortcode` and it's associated file
-* Response: JSON, updated "files" section of the pack and 200 status, 409 if the trying to use a shortcode
+* Response: JSON, emoji shortcode with filename which was added/updated/deleted and 200 status, 409 if the trying to use a shortcode
  that is already taken, 400 if there was an error with the shortcode, filename or file (additional info
  in the "error" part of the response JSON)

--- a/lib/pleroma/web/pleroma_api/controllers/emoji_api_controller.ex
+++ b/lib/pleroma/web/pleroma_api/controllers/emoji_api_controller.ex
@ -385,23 +385,35 @@ defp update_metadata_and_send(conn, full_pack, new_data, pack_file_p) do
    json(conn, new_data)
  end

-  defp get_filename(%{"filename" => filename}), do: filename
-
-  defp get_filename(%{"file" => file}) do
-    case file do
-      %Plug.Upload{filename: filename} -> filename
-      url when is_binary(url) -> Path.basename(url)
-    end
-  end
+  defp get_filename(%Plug.Upload{filename: filename}), do: filename
+  defp get_filename(url) when is_binary(url), do: Path.basename(url)

  defp empty?(str), do: String.trim(str) == ""

-  defp update_file_and_send(conn, updated_full_pack, pack_file_p) do
-    # Write the emoji pack file
-    File.write!(pack_file_p, Jason.encode!(updated_full_pack, pretty: true))
+  defp update_pack_file(updated_full_pack, pack_file_p) do
+    content = Jason.encode!(updated_full_pack, pretty: true)

-    # Return the modified file list
-    json(conn, updated_full_pack["files"])
+    File.write!(pack_file_p, content)
+  end
+
+  defp create_subdirs(file_path) do
+    if String.contains?(file_path, "/") do
+      file_path
+      |> Path.dirname()
+      |> File.mkdir_p!()
+    end
+  end
+
+  defp pack_info(pack_name) do
+    dir = Path.join(emoji_dir_path(), pack_name)
+    json_path = Path.join(dir, "pack.json")
+
+    json =
+      json_path
+      |> File.read!()
+      |> Jason.decode!()
+
+    {dir, json_path, json}
  end

  @doc """
@ -422,23 +434,25 @@ defp update_file_and_send(conn, updated_full_pack, pack_file_p) do
  # Add
  def update_file(
        conn,
-        %{"pack_name" => pack_name, "action" => "add", "shortcode" => shortcode} = params
+        %{"pack_name" => pack_name, "action" => "add"} = params
      ) do
-    pack_dir = Path.join(emoji_dir_path(), pack_name)
-    pack_file_p = Path.join(pack_dir, "pack.json")
+    shortcode =
+      if params["shortcode"] do
+        params["shortcode"]
+      else
+        filename = get_filename(params["file"])
+        Path.basename(filename, Path.extname(filename))
+      end

-    full_pack = Jason.decode!(File.read!(pack_file_p))
+    {pack_dir, pack_file_p, full_pack} = pack_info(pack_name)

    with {_, false} <- {:has_shortcode, Map.has_key?(full_pack["files"], shortcode)},
-         filename <- get_filename(params),
+         filename <- params["filename"] || get_filename(params["file"]),
         false <- empty?(shortcode),
-         false <- empty?(filename) do
-      file_path = Path.join(pack_dir, filename)
-
+         false <- empty?(filename),
+         file_path <- Path.join(pack_dir, filename) do
      # If the name contains directories, create them
-      if String.contains?(file_path, "/") do
-        File.mkdir_p!(Path.dirname(file_path))
-      end
+      create_subdirs(file_path)

      case params["file"] do
        %Plug.Upload{path: upload_path} ->
@ -451,8 +465,11 @@ def update_file(
          File.write!(file_path, file_contents)
      end

-      updated_full_pack = put_in(full_pack, ["files", shortcode], filename)
-      update_file_and_send(conn, updated_full_pack, pack_file_p)
+      full_pack
+      |> put_in(["files", shortcode], filename)
+      |> update_pack_file(pack_file_p)
+
+      json(conn, %{shortcode => filename})
    else
      {:has_shortcode, _} ->
        conn
@ -472,10 +489,7 @@ def update_file(conn, %{
        "action" => "remove",
        "shortcode" => shortcode
      }) do
-    pack_dir = Path.join(emoji_dir_path(), pack_name)
-    pack_file_p = Path.join(pack_dir, "pack.json")
-
-    full_pack = Jason.decode!(File.read!(pack_file_p))
+    {pack_dir, pack_file_p, full_pack} = pack_info(pack_name)

    if Map.has_key?(full_pack["files"], shortcode) do
      {emoji_file_path, updated_full_pack} = pop_in(full_pack, ["files", shortcode])
@ -494,7 +508,8 @@ def update_file(conn, %{
        end
      end

-      update_file_and_send(conn, updated_full_pack, pack_file_p)
+      update_pack_file(updated_full_pack, pack_file_p)
+      json(conn, %{shortcode => full_pack["files"][shortcode]})
    else
      conn
      |> put_status(:bad_request)
@ -507,10 +522,7 @@ def update_file(
        conn,
        %{"pack_name" => pack_name, "action" => "update", "shortcode" => shortcode} = params
      ) do
-    pack_dir = Path.join(emoji_dir_path(), pack_name)
-    pack_file_p = Path.join(pack_dir, "pack.json")
-
-    full_pack = Jason.decode!(File.read!(pack_file_p))
+    {pack_dir, pack_file_p, full_pack} = pack_info(pack_name)

    with {_, true} <- {:has_shortcode, Map.has_key?(full_pack["files"], shortcode)},
         %{"new_shortcode" => new_shortcode, "new_filename" => new_filename} <- params,
@ -522,9 +534,7 @@ def update_file(
      new_emoji_file_path = Path.join(pack_dir, new_filename)

      # If the name contains directories, create them
-      if String.contains?(new_emoji_file_path, "/") do
-        File.mkdir_p!(Path.dirname(new_emoji_file_path))
-      end
+      create_subdirs(new_emoji_file_path)

      # Move/Rename the old filename to a new filename
      # These are probably on the same filesystem, so just rename should work
@ -540,8 +550,11 @@ def update_file(
      end

      # Then, put in the new shortcode with the new path
-      updated_full_pack = put_in(updated_full_pack, ["files", new_shortcode], new_filename)
-      update_file_and_send(conn, updated_full_pack, pack_file_p)
+      updated_full_pack
+      |> put_in(["files", new_shortcode], new_filename)
+      |> update_pack_file(pack_file_p)
+
+      json(conn, %{new_shortcode => new_filename})
    else
      {:has_shortcode, _} ->
        conn
--- a/test/instance_static/add/shortcode.png
+++ b/test/instance_static/add/shortcode.png
--- a/test/web/pleroma_api/controllers/emoji_api_controller_test.exs
+++ b/test/web/pleroma_api/controllers/emoji_api_controller_test.exs
@ -295,96 +295,116 @@ test "when the fallback source doesn't have all the files", ctx do
    end
  end

-  test "updating pack files" do
-    pack_file = "#{@emoji_dir_path}/test_pack/pack.json"
-    original_content = File.read!(pack_file)
+  describe "update_file/2" do
+    setup do
+      pack_file = "#{@emoji_dir_path}/test_pack/pack.json"
+      original_content = File.read!(pack_file)

-    on_exit(fn ->
-      File.write!(pack_file, original_content)
+      on_exit(fn ->
+        File.write!(pack_file, original_content)
+      end)

-      File.rm_rf!("#{@emoji_dir_path}/test_pack/blank_url.png")
-      File.rm_rf!("#{@emoji_dir_path}/test_pack/dir")
-      File.rm_rf!("#{@emoji_dir_path}/test_pack/dir_2")
-    end)
+      admin = insert(:user, is_admin: true)
+      %{conn: conn} = oauth_access(["admin:write"], user: admin)
+      {:ok, conn: conn}
+    end

-    admin = insert(:user, is_admin: true)
-    %{conn: conn} = oauth_access(["admin:write"], user: admin)
+    test "update file without shortcode", %{conn: conn} do
+      on_exit(fn -> File.rm_rf!("#{@emoji_dir_path}/test_pack/shortcode.png") end)

-    same_name = %{
-      "action" => "add",
-      "shortcode" => "blank",
-      "filename" => "dir/blank.png",
-      "file" => %Plug.Upload{
-        filename: "blank.png",
-        path: "#{@emoji_dir_path}/test_pack/blank.png"
+      assert conn
+             |> post("/api/pleroma/emoji/packs/test_pack/update_file", %{
+               "action" => "add",
+               "file" => %Plug.Upload{
+                 filename: "shortcode.png",
+                 path: "#{Pleroma.Config.get([:instance, :static_dir])}/add/shortcode.png"
+               }
+             })
+             |> json_response(200) == %{"shortcode" => "shortcode.png"}
+    end
+
+    test "updating pack files", %{conn: conn} do
+      on_exit(fn ->
+        File.rm_rf!("#{@emoji_dir_path}/test_pack/blank_url.png")
+        File.rm_rf!("#{@emoji_dir_path}/test_pack/dir")
+        File.rm_rf!("#{@emoji_dir_path}/test_pack/dir_2")
+      end)
+
+      same_name = %{
+        "action" => "add",
+        "shortcode" => "blank",
+        "filename" => "dir/blank.png",
+        "file" => %Plug.Upload{
+          filename: "blank.png",
+          path: "#{@emoji_dir_path}/test_pack/blank.png"
+        }
      }
-    }

-    different_name = %{same_name | "shortcode" => "blank_2"}
+      different_name = %{same_name | "shortcode" => "blank_2"}

-    assert (conn
-            |> post(emoji_api_path(conn, :update_file, "test_pack"), same_name)
-            |> json_response(:conflict))["error"] =~ "already exists"
+      assert (conn
+              |> post(emoji_api_path(conn, :update_file, "test_pack"), same_name)
+              |> json_response(:conflict))["error"] =~ "already exists"

-    assert conn
-           |> post(emoji_api_path(conn, :update_file, "test_pack"), different_name)
-           |> json_response(200) == %{"blank" => "blank.png", "blank_2" => "dir/blank.png"}
+      assert conn
+             |> post(emoji_api_path(conn, :update_file, "test_pack"), different_name)
+             |> json_response(200) == %{"blank_2" => "dir/blank.png"}

-    assert File.exists?("#{@emoji_dir_path}/test_pack/dir/blank.png")
+      assert File.exists?("#{@emoji_dir_path}/test_pack/dir/blank.png")

-    assert conn
-           |> post(emoji_api_path(conn, :update_file, "test_pack"), %{
-             "action" => "update",
-             "shortcode" => "blank_2",
-             "new_shortcode" => "blank_3",
-             "new_filename" => "dir_2/blank_3.png"
-           })
-           |> json_response(200) == %{"blank" => "blank.png", "blank_3" => "dir_2/blank_3.png"}
+      assert conn
+             |> post(emoji_api_path(conn, :update_file, "test_pack"), %{
+               "action" => "update",
+               "shortcode" => "blank_2",
+               "new_shortcode" => "blank_3",
+               "new_filename" => "dir_2/blank_3.png"
+             })
+             |> json_response(200) == %{"blank_3" => "dir_2/blank_3.png"}

-    refute File.exists?("#{@emoji_dir_path}/test_pack/dir/")
-    assert File.exists?("#{@emoji_dir_path}/test_pack/dir_2/blank_3.png")
+      refute File.exists?("#{@emoji_dir_path}/test_pack/dir/")
+      assert File.exists?("#{@emoji_dir_path}/test_pack/dir_2/blank_3.png")

-    assert conn
-           |> post(emoji_api_path(conn, :update_file, "test_pack"), %{
-             "action" => "remove",
-             "shortcode" => "blank_3"
-           })
-           |> json_response(200) == %{"blank" => "blank.png"}
+      assert conn
+             |> post(emoji_api_path(conn, :update_file, "test_pack"), %{
+               "action" => "remove",
+               "shortcode" => "blank_3"
+             })
+             |> json_response(200) == %{"blank_3" => "dir_2/blank_3.png"}

-    refute File.exists?("#{@emoji_dir_path}/test_pack/dir_2/")
+      refute File.exists?("#{@emoji_dir_path}/test_pack/dir_2/")

-    mock(fn
-      %{
-        method: :get,
-        url: "https://test-blank/blank_url.png"
-      } ->
-        text(File.read!("#{@emoji_dir_path}/test_pack/blank.png"))
-    end)
+      mock(fn
+        %{
+          method: :get,
+          url: "https://test-blank/blank_url.png"
+        } ->
+          text(File.read!("#{@emoji_dir_path}/test_pack/blank.png"))
+      end)

-    # The name should be inferred from the URL ending
-    from_url = %{
-      "action" => "add",
-      "shortcode" => "blank_url",
-      "file" => "https://test-blank/blank_url.png"
-    }
+      # The name should be inferred from the URL ending
+      from_url = %{
+        "action" => "add",
+        "shortcode" => "blank_url",
+        "file" => "https://test-blank/blank_url.png"
+      }

-    assert conn
-           |> post(emoji_api_path(conn, :update_file, "test_pack"), from_url)
-           |> json_response(200) == %{
-             "blank" => "blank.png",
-             "blank_url" => "blank_url.png"
-           }
+      assert conn
+             |> post(emoji_api_path(conn, :update_file, "test_pack"), from_url)
+             |> json_response(200) == %{
+               "blank_url" => "blank_url.png"
+             }

-    assert File.exists?("#{@emoji_dir_path}/test_pack/blank_url.png")
+      assert File.exists?("#{@emoji_dir_path}/test_pack/blank_url.png")

-    assert conn
-           |> post(emoji_api_path(conn, :update_file, "test_pack"), %{
-             "action" => "remove",
-             "shortcode" => "blank_url"
-           })
-           |> json_response(200) == %{"blank" => "blank.png"}
+      assert conn
+             |> post(emoji_api_path(conn, :update_file, "test_pack"), %{
+               "action" => "remove",
+               "shortcode" => "blank_url"
+             })
+             |> json_response(200) == %{"blank_url" => "blank_url.png"}

-    refute File.exists?("#{@emoji_dir_path}/test_pack/blank_url.png")
+      refute File.exists?("#{@emoji_dir_path}/test_pack/blank_url.png")
+    end
  end

  test "creating and deleting a pack" do