Remove "default" image description #493

Merged
floatingghost merged 1 commit from ilja/akkoma:remove_default_image_description into develop 2023-04-14 16:27:42 +00:00
7 changed files with 5 additions and 51 deletions

View file

@ -17,6 +17,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
### Removed ### Removed
- Possibility of using the `style` parameter on `span` elements. This will break certain MFM parameters. - Possibility of using the `style` parameter on `span` elements. This will break certain MFM parameters.
- Option for "default" image description.
## 2023.02 ## 2023.02

View file

@ -65,7 +65,6 @@
link_name: false, link_name: false,
proxy_remote: false, proxy_remote: false,
filename_display_max_length: 30, filename_display_max_length: 30,
default_description: nil,
base_url: nil base_url: nil
config :pleroma, Pleroma.Uploaders.Local, uploads: "uploads" config :pleroma, Pleroma.Uploaders.Local, uploads: "uploads"

View file

@ -23,8 +23,7 @@
config :pleroma, Pleroma.Upload, config :pleroma, Pleroma.Upload,
filters: [], filters: [],
link_name: false, link_name: false
default_description: :filename
config :pleroma, Pleroma.Uploaders.Local, uploads: "test/uploads" config :pleroma, Pleroma.Uploaders.Local, uploads: "test/uploads"

View file

@ -562,7 +562,6 @@ the source code is here: [kocaptcha](https://github.com/koto-bank/kocaptcha). Th
* `proxy_remote`: If you're using a remote uploader, Akkoma will proxy media requests instead of redirecting to it. * `proxy_remote`: If you're using a remote uploader, Akkoma will proxy media requests instead of redirecting to it.
* `proxy_opts`: Proxy options, see `Pleroma.ReverseProxy` documentation. * `proxy_opts`: Proxy options, see `Pleroma.ReverseProxy` documentation.
* `filename_display_max_length`: Set max length of a filename to display. 0 = no limit. Default: 30. * `filename_display_max_length`: Set max length of a filename to display. 0 = no limit. Default: 30.
* `default_description`: Sets which default description an image has if none is set explicitly. Options: nil (default) - Don't set a default, :filename - use the filename of the file, a string (e.g. "attachment") - Use this string
!!! warning !!! warning
`strip_exif` has been replaced by `Pleroma.Upload.Filter.Mogrify`. `strip_exif` has been replaced by `Pleroma.Upload.Filter.Mogrify`.

View file

@ -65,15 +65,6 @@ defmodule Pleroma.Upload do
} }
defstruct [:id, :name, :tempfile, :content_type, :width, :height, :blurhash, :path] defstruct [:id, :name, :tempfile, :content_type, :width, :height, :blurhash, :path]
defp get_description(opts, upload) do
case {opts[:description], Pleroma.Config.get([Pleroma.Upload, :default_description])} do
{description, _} when is_binary(description) -> description
{_, :filename} -> upload.name
{_, str} when is_binary(str) -> str
_ -> ""
end
end
@spec store(source, options :: [option()]) :: {:ok, Map.t()} | {:error, any()} @spec store(source, options :: [option()]) :: {:ok, Map.t()} | {:error, any()}
@doc "Store a file. If using a `Plug.Upload{}` as the source, be sure to use `Majic.Plug` to ensure its content_type and filename is correct." @doc "Store a file. If using a `Plug.Upload{}` as the source, be sure to use `Majic.Plug` to ensure its content_type and filename is correct."
def store(upload, opts \\ []) do def store(upload, opts \\ []) do
@ -82,7 +73,7 @@ def store(upload, opts \\ []) do
with {:ok, upload} <- prepare_upload(upload, opts), with {:ok, upload} <- prepare_upload(upload, opts),
upload = %__MODULE__{upload | path: upload.path || "#{upload.id}/#{upload.name}"}, upload = %__MODULE__{upload | path: upload.path || "#{upload.id}/#{upload.name}"},
{:ok, upload} <- Pleroma.Upload.Filter.filter(opts.filters, upload), {:ok, upload} <- Pleroma.Upload.Filter.filter(opts.filters, upload),
description = get_description(opts, upload), description = Map.get(opts, :description) || "",
floatingghost marked this conversation as resolved
Review

might be cleaner is use Map.get/3 here instead

might be cleaner is use `Map.get/3` here instead
Review

Map.get/3 doesn't work in this case. The key can contain nil. So in that case the description would be nil instead of an empty string (and the rest of the code expects a string).

iex(1)> Map.get(%{description: nil}, :description, "")
nil
iex(2)> Map.get(%{description: nil}, :description) || ""
""
`Map.get/3` doesn't work in this case. The key can contain `nil`. So in that case the description would be `nil` instead of an empty string (and the rest of the code expects a string). ```iex iex(1)> Map.get(%{description: nil}, :description, "") nil iex(2)> Map.get(%{description: nil}, :description) || "" "" ```
Review

ah yeah , forgot that it could be explicitly nil , I'm stupid

ah yeah , forgot that it could be explicitly nil , I'm stupid
{_, true} <- {_, true} <-
{:description_limit, {:description_limit,
String.length(description) <= Pleroma.Config.get([:instance, :description_limit])}, String.length(description) <= Pleroma.Config.get([:instance, :description_limit])},

View file

@ -54,7 +54,7 @@ test "it returns file" do
assert result == assert result ==
%{ %{
"id" => result["id"], "id" => result["id"],
"name" => "image.jpg", "name" => "",
"type" => "Document", "type" => "Document",
"mediaType" => "image/jpeg", "mediaType" => "image/jpeg",
"url" => [ "url" => [
@ -154,19 +154,6 @@ test "copies the file to the configured folder with deduping" do
"e30397b58d226d6583ab5b8b3c5defb0c682bda5c31ef07a9f57c1c4986e3781.jpg" "e30397b58d226d6583ab5b8b3c5defb0c682bda5c31ef07a9f57c1c4986e3781.jpg"
end end
test "copies the file to the configured folder without deduping" do
File.cp!("test/fixtures/image.jpg", "test/fixtures/image_tmp.jpg")
file = %Plug.Upload{
content_type: "image/jpeg",
path: Path.absname("test/fixtures/image_tmp.jpg"),
filename: "an [image.jpg"
}
{:ok, data} = Upload.store(file)
assert data["name"] == "an [image.jpg"
end
Outdated
Review

Note: This test broke with removing the filename from this field. But this is not what it tests, and as afaict it doesn't even actually tests what it's supposed to do. Therefore I decided to just remove it instead of leaving it in what seems to be a broken state.

Note: This test broke with removing the filename from this field. But this is not what it tests, and as afaict it doesn't even actually tests what it's supposed to do. Therefore I decided to just remove it instead of leaving it in what seems to be a broken state.
test "fixes incorrect content type when base64 is given" do test "fixes incorrect content type when base64 is given" do
params = %{ params = %{
img: "data:image/png;base64,#{Base.encode64(File.read!("test/fixtures/image.jpg"))}" img: "data:image/png;base64,#{Base.encode64(File.read!("test/fixtures/image.jpg"))}"
@ -184,7 +171,7 @@ test "adds extension when base64 is given" do
} }
{:ok, data} = Upload.store(params) {:ok, data} = Upload.store(params)
assert String.ends_with?(data["name"], ".jpg") assert String.ends_with?(List.first(data["url"])["href"], ".jpg")
end end
test "copies the file to the configured folder with anonymizing filename" do test "copies the file to the configured folder with anonymizing filename" do

View file

@ -1308,28 +1308,6 @@ test "sets a description if given", %{test_file: file} do
assert object.data["name"] == "a cool file" assert object.data["name"] == "a cool file"
end end
test "it sets the default description depending on the configuration", %{test_file: file} do
clear_config([Pleroma.Upload, :default_description])
clear_config([Pleroma.Upload, :default_description], nil)
{:ok, %Object{} = object} = ActivityPub.upload(file)
assert object.data["name"] == ""
clear_config([Pleroma.Upload, :default_description], :filename)
{:ok, %Object{} = object} = ActivityPub.upload(file)
assert object.data["name"] == "an_image.jpg"
clear_config([Pleroma.Upload, :default_description], "unnamed attachment")
{:ok, %Object{} = object} = ActivityPub.upload(file)
assert object.data["name"] == "unnamed attachment"
end
test "copies the file to the configured folder", %{test_file: file} do
clear_config([Pleroma.Upload, :default_description], :filename)
{:ok, %Object{} = object} = ActivityPub.upload(file)
assert object.data["name"] == "an_image.jpg"
end
test "works with base64 encoded images" do test "works with base64 encoded images" do
file = %{ file = %{
img: data_uri() img: data_uri()