From 571f6fffdb3e87e11f0ada05f3d6a102596886d3 Mon Sep 17 00:00:00 2001 From: shibao Date: Mon, 19 Dec 2022 22:29:26 -0500 Subject: [PATCH] improve tagging logic --- changelog.md | 1 + lib/memex/contexts.ex | 15 ------- lib/memex/contexts/context.ex | 44 ++++++++++++++----- lib/memex/notes.ex | 15 ------- lib/memex/notes/note.ex | 44 ++++++++++++++----- lib/memex/pipelines.ex | 15 ------- lib/memex/pipelines/pipeline.ex | 44 ++++++++++++++----- .../context_live/form_component.html.heex | 4 +- .../live/note_live/form_component.html.heex | 4 +- .../pipeline_live/form_component.html.heex | 4 +- priv/gettext/actions.pot | 6 +-- priv/gettext/de/LC_MESSAGES/actions.po | 6 +-- priv/gettext/de/LC_MESSAGES/default.po | 12 ++--- priv/gettext/de/LC_MESSAGES/errors.po | 7 +++ priv/gettext/default.pot | 12 ++--- priv/gettext/errors.pot | 7 +++ test/memex_web/live/context_live_test.exs | 12 +++-- test/memex_web/live/note_live_test.exs | 12 +++-- test/memex_web/live/pipeline_live_test.exs | 12 +++-- test/support/fixtures/contexts_fixtures.ex | 2 +- test/support/fixtures/notes_fixtures.ex | 2 +- test/support/fixtures/pipelines_fixtures.ex | 2 +- 22 files changed, 162 insertions(+), 120 deletions(-) diff --git a/changelog.md b/changelog.md index 5cccc24..2542a62 100644 --- a/changelog.md +++ b/changelog.md @@ -2,6 +2,7 @@ - fix docker-compose - fix newlines in notes/context/step contents - fix user invite page +- improve tagging logic # v0.1.3 - backlink to other notes in notes diff --git a/lib/memex/contexts.ex b/lib/memex/contexts.ex index 8543b25..943be61 100644 --- a/lib/memex/contexts.ex +++ b/lib/memex/contexts.ex @@ -4,7 +4,6 @@ defmodule Memex.Contexts do """ import Ecto.Query, warn: false - alias Ecto.Changeset alias Memex.{Accounts.User, Contexts.Context, Repo} @doc """ @@ -229,18 +228,4 @@ defmodule Memex.Contexts do def change_context(%Context{} = context, attrs \\ %{}, user) do context |> Context.update_changeset(attrs, user) end - - @doc """ - Gets a canonical string representation of the `:tags` field for a Note - """ - @spec get_tags_string(Context.t() | Context.changeset() | [String.t()] | nil) :: String.t() - def get_tags_string(nil), do: "" - def get_tags_string(tags) when tags |> is_list(), do: tags |> Enum.join(",") - def get_tags_string(%Context{tags: tags}), do: tags |> get_tags_string() - - def get_tags_string(%Changeset{} = changeset) do - changeset - |> Changeset.get_field(:tags) - |> get_tags_string() - end end diff --git a/lib/memex/contexts/context.ex b/lib/memex/contexts/context.ex index f52917e..9279adc 100644 --- a/lib/memex/contexts/context.ex +++ b/lib/memex/contexts/context.ex @@ -27,7 +27,7 @@ defmodule Memex.Contexts.Context do slug: slug(), content: String.t(), tags: [String.t()] | nil, - tags_string: String.t(), + tags_string: String.t() | nil, visibility: :public | :private | :unlisted, user: User.t() | Ecto.Association.NotLoaded.t(), user_id: User.id(), @@ -66,16 +66,38 @@ defmodule Memex.Contexts.Context do |> unsafe_validate_unique(:slug, Repo) end - defp cast_tags_string(changeset, %{"tags_string" => tags_string}) - when tags_string |> is_binary() do - tags = - tags_string - |> String.split(",", trim: true) - |> Enum.map(fn str -> str |> String.trim() end) - |> Enum.sort() - - changeset |> change(tags: tags) + defp cast_tags_string(changeset, attrs) do + changeset + |> put_change(:tags_string, changeset |> get_field(:tags) |> get_tags_string()) + |> cast(attrs, [:tags_string]) + |> validate_format(:tags_string, ~r/^[\p{L}\p{N}\-\,]+$/, + message: + dgettext( + "errors", + "invalid format: only numbers, letters and hyphen are accepted. tags must be comma-delimited" + ) + ) + |> cast_tags() end - defp cast_tags_string(changeset, _attrs), do: changeset + defp cast_tags(%{valid?: false} = changeset), do: changeset + + defp cast_tags(%{valid?: true} = changeset) do + tags = changeset |> get_field(:tags_string) |> process_tags() + changeset |> put_change(:tags, tags) + end + + defp process_tags(tags_string) when tags_string |> is_binary() do + tags_string + |> String.split(",", trim: true) + |> Enum.map(fn str -> str |> String.trim() end) + |> Enum.reject(fn str -> str |> is_nil() end) + |> Enum.sort() + end + + defp process_tags(_other_tags_string), do: [] + + @spec get_tags_string([String.t()] | nil) :: String.t() + def get_tags_string(nil), do: "" + def get_tags_string(tags) when tags |> is_list(), do: tags |> Enum.join(",") end diff --git a/lib/memex/notes.ex b/lib/memex/notes.ex index b86aca8..d1e8567 100644 --- a/lib/memex/notes.ex +++ b/lib/memex/notes.ex @@ -4,7 +4,6 @@ defmodule Memex.Notes do """ import Ecto.Query, warn: false - alias Ecto.Changeset alias Memex.{Accounts.User, Notes.Note, Repo} @doc """ @@ -229,18 +228,4 @@ defmodule Memex.Notes do def change_note(%Note{} = note, attrs \\ %{}, user) do note |> Note.update_changeset(attrs, user) end - - @doc """ - Gets a canonical string representation of the `:tags` field for a Note - """ - @spec get_tags_string(Note.t() | Note.changeset() | [String.t()] | nil) :: String.t() - def get_tags_string(nil), do: "" - def get_tags_string(tags) when tags |> is_list(), do: tags |> Enum.join(",") - def get_tags_string(%Note{tags: tags}), do: tags |> get_tags_string() - - def get_tags_string(%Changeset{} = changeset) do - changeset - |> Changeset.get_field(:tags) - |> get_tags_string() - end end diff --git a/lib/memex/notes/note.ex b/lib/memex/notes/note.ex index 9276384..89ee8ce 100644 --- a/lib/memex/notes/note.ex +++ b/lib/memex/notes/note.ex @@ -26,7 +26,7 @@ defmodule Memex.Notes.Note do slug: slug(), content: String.t(), tags: [String.t()] | nil, - tags_string: String.t(), + tags_string: String.t() | nil, visibility: :public | :private | :unlisted, user: User.t() | Ecto.Association.NotLoaded.t(), user_id: User.id(), @@ -65,16 +65,38 @@ defmodule Memex.Notes.Note do |> unsafe_validate_unique(:slug, Repo) end - defp cast_tags_string(changeset, %{"tags_string" => tags_string}) - when tags_string |> is_binary() do - tags = - tags_string - |> String.split(",", trim: true) - |> Enum.map(fn str -> str |> String.trim() end) - |> Enum.sort() - - changeset |> change(tags: tags) + defp cast_tags_string(changeset, attrs) do + changeset + |> put_change(:tags_string, changeset |> get_field(:tags) |> get_tags_string()) + |> cast(attrs, [:tags_string]) + |> validate_format(:tags_string, ~r/^[\p{L}\p{N}\-\,]+$/, + message: + dgettext( + "errors", + "invalid format: only numbers, letters and hyphen are accepted. tags must be comma-delimited" + ) + ) + |> cast_tags() end - defp cast_tags_string(changeset, _attrs), do: changeset + defp cast_tags(%{valid?: false} = changeset), do: changeset + + defp cast_tags(%{valid?: true} = changeset) do + tags = changeset |> get_field(:tags_string) |> process_tags() + changeset |> put_change(:tags, tags) + end + + defp process_tags(tags_string) when tags_string |> is_binary() do + tags_string + |> String.split(",", trim: true) + |> Enum.map(fn str -> str |> String.trim() end) + |> Enum.reject(fn str -> str |> is_nil() end) + |> Enum.sort() + end + + defp process_tags(_other_tags_string), do: [] + + @spec get_tags_string([String.t()] | nil) :: String.t() + def get_tags_string(nil), do: "" + def get_tags_string(tags) when tags |> is_list(), do: tags |> Enum.join(",") end diff --git a/lib/memex/pipelines.ex b/lib/memex/pipelines.ex index f72a1a1..6e6f541 100644 --- a/lib/memex/pipelines.ex +++ b/lib/memex/pipelines.ex @@ -4,7 +4,6 @@ defmodule Memex.Pipelines do """ import Ecto.Query, warn: false - alias Ecto.Changeset alias Memex.{Accounts.User, Pipelines.Pipeline, Repo} @doc """ @@ -231,18 +230,4 @@ defmodule Memex.Pipelines do def change_pipeline(%Pipeline{} = pipeline, attrs \\ %{}, user) do pipeline |> Pipeline.update_changeset(attrs, user) end - - @doc """ - Gets a canonical string representation of the `:tags` field for a Pipeline - """ - @spec get_tags_string(Pipeline.t() | Pipeline.changeset() | [String.t()] | nil) :: String.t() - def get_tags_string(nil), do: "" - def get_tags_string(tags) when tags |> is_list(), do: tags |> Enum.join(",") - def get_tags_string(%Pipeline{tags: tags}), do: tags |> get_tags_string() - - def get_tags_string(%Changeset{} = changeset) do - changeset - |> Changeset.get_field(:tags) - |> get_tags_string() - end end diff --git a/lib/memex/pipelines/pipeline.ex b/lib/memex/pipelines/pipeline.ex index 07aca87..adca293 100644 --- a/lib/memex/pipelines/pipeline.ex +++ b/lib/memex/pipelines/pipeline.ex @@ -28,7 +28,7 @@ defmodule Memex.Pipelines.Pipeline do slug: slug(), description: String.t(), tags: [String.t()] | nil, - tags_string: String.t(), + tags_string: String.t() | nil, visibility: :public | :private | :unlisted, user: User.t() | Ecto.Association.NotLoaded.t(), user_id: User.id(), @@ -67,16 +67,38 @@ defmodule Memex.Pipelines.Pipeline do |> unsafe_validate_unique(:slug, Repo) end - defp cast_tags_string(changeset, %{"tags_string" => tags_string}) - when tags_string |> is_binary() do - tags = - tags_string - |> String.split(",", trim: true) - |> Enum.map(fn str -> str |> String.trim() end) - |> Enum.sort() - - changeset |> change(tags: tags) + defp cast_tags_string(changeset, attrs) do + changeset + |> put_change(:tags_string, changeset |> get_field(:tags) |> get_tags_string()) + |> cast(attrs, [:tags_string]) + |> validate_format(:tags_string, ~r/^[\p{L}\p{N}\-\,]+$/, + message: + dgettext( + "errors", + "invalid format: only numbers, letters and hyphen are accepted. tags must be comma-delimited" + ) + ) + |> cast_tags() end - defp cast_tags_string(changeset, _attrs), do: changeset + defp cast_tags(%{valid?: false} = changeset), do: changeset + + defp cast_tags(%{valid?: true} = changeset) do + tags = changeset |> get_field(:tags_string) |> process_tags() + changeset |> put_change(:tags, tags) + end + + defp process_tags(tags_string) when tags_string |> is_binary() do + tags_string + |> String.split(",", trim: true) + |> Enum.map(fn str -> str |> String.trim() end) + |> Enum.reject(fn str -> str |> is_nil() end) + |> Enum.sort() + end + + defp process_tags(_other_tags_string), do: [] + + @spec get_tags_string([String.t()] | nil) :: String.t() + def get_tags_string(nil), do: "" + def get_tags_string(tags) when tags |> is_list(), do: tags |> Enum.join(",") end diff --git a/lib/memex_web/live/context_live/form_component.html.heex b/lib/memex_web/live/context_live/form_component.html.heex index 9c7aa91..c5073d1 100644 --- a/lib/memex_web/live/context_live/form_component.html.heex +++ b/lib/memex_web/live/context_live/form_component.html.heex @@ -27,9 +27,7 @@ <%= text_input(f, :tags_string, id: "tags-input", class: "input input-primary", - placeholder: gettext("tag1,tag2"), - phx_update: "ignore", - value: Contexts.get_tags_string(@changeset) + placeholder: gettext("tag1,tag2") ) %> <%= error_tag(f, :tags_string) %> diff --git a/lib/memex_web/live/note_live/form_component.html.heex b/lib/memex_web/live/note_live/form_component.html.heex index e2cc50d..9928eea 100644 --- a/lib/memex_web/live/note_live/form_component.html.heex +++ b/lib/memex_web/live/note_live/form_component.html.heex @@ -27,9 +27,7 @@ <%= text_input(f, :tags_string, id: "tags-input", class: "input input-primary", - placeholder: gettext("tag1,tag2"), - phx_update: "ignore", - value: Notes.get_tags_string(@changeset) + placeholder: gettext("tag1,tag2") ) %> <%= error_tag(f, :tags_string) %> diff --git a/lib/memex_web/live/pipeline_live/form_component.html.heex b/lib/memex_web/live/pipeline_live/form_component.html.heex index b4b3a65..8a39f8d 100644 --- a/lib/memex_web/live/pipeline_live/form_component.html.heex +++ b/lib/memex_web/live/pipeline_live/form_component.html.heex @@ -27,9 +27,7 @@ <%= text_input(f, :tags_string, id: "tags-input", class: "input input-primary", - placeholder: gettext("tag1,tag2"), - phx_update: "ignore", - value: Pipelines.get_tags_string(@changeset) + placeholder: gettext("tag1,tag2") ) %> <%= error_tag(f, :tags_string) %> diff --git a/priv/gettext/actions.pot b/priv/gettext/actions.pot index 4a05b2e..ac3fdf3 100644 --- a/priv/gettext/actions.pot +++ b/priv/gettext/actions.pot @@ -124,9 +124,9 @@ msgstr "" msgid "register" msgstr "" -#: lib/memex_web/live/context_live/form_component.html.heex:42 -#: lib/memex_web/live/note_live/form_component.html.heex:42 -#: lib/memex_web/live/pipeline_live/form_component.html.heex:42 +#: lib/memex_web/live/context_live/form_component.html.heex:40 +#: lib/memex_web/live/note_live/form_component.html.heex:40 +#: lib/memex_web/live/pipeline_live/form_component.html.heex:40 #: lib/memex_web/live/step_live/form_component.html.heex:28 #, elixir-autogen, elixir-format msgid "save" diff --git a/priv/gettext/de/LC_MESSAGES/actions.po b/priv/gettext/de/LC_MESSAGES/actions.po index b840f70..3b2c9e8 100644 --- a/priv/gettext/de/LC_MESSAGES/actions.po +++ b/priv/gettext/de/LC_MESSAGES/actions.po @@ -125,9 +125,9 @@ msgstr "" msgid "register" msgstr "" -#: lib/memex_web/live/context_live/form_component.html.heex:42 -#: lib/memex_web/live/note_live/form_component.html.heex:42 -#: lib/memex_web/live/pipeline_live/form_component.html.heex:42 +#: lib/memex_web/live/context_live/form_component.html.heex:40 +#: lib/memex_web/live/note_live/form_component.html.heex:40 +#: lib/memex_web/live/pipeline_live/form_component.html.heex:40 #: lib/memex_web/live/step_live/form_component.html.heex:28 #, elixir-autogen, elixir-format msgid "save" diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 8ea9934..e577c01 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -301,17 +301,17 @@ msgstr "" msgid "report bugs or request features" msgstr "" -#: lib/memex_web/live/context_live/form_component.html.heex:43 -#: lib/memex_web/live/note_live/form_component.html.heex:43 -#: lib/memex_web/live/pipeline_live/form_component.html.heex:43 +#: lib/memex_web/live/context_live/form_component.html.heex:41 +#: lib/memex_web/live/note_live/form_component.html.heex:41 +#: lib/memex_web/live/pipeline_live/form_component.html.heex:41 #: lib/memex_web/live/step_live/form_component.html.heex:29 #, elixir-autogen, elixir-format msgid "saving..." msgstr "" -#: lib/memex_web/live/context_live/form_component.html.heex:39 -#: lib/memex_web/live/note_live/form_component.html.heex:39 -#: lib/memex_web/live/pipeline_live/form_component.html.heex:39 +#: lib/memex_web/live/context_live/form_component.html.heex:37 +#: lib/memex_web/live/note_live/form_component.html.heex:37 +#: lib/memex_web/live/pipeline_live/form_component.html.heex:37 #, elixir-autogen, elixir-format msgid "select privacy" msgstr "" diff --git a/priv/gettext/de/LC_MESSAGES/errors.po b/priv/gettext/de/LC_MESSAGES/errors.po index ef40ad0..b16e330 100644 --- a/priv/gettext/de/LC_MESSAGES/errors.po +++ b/priv/gettext/de/LC_MESSAGES/errors.po @@ -131,3 +131,10 @@ msgstr "" #, elixir-autogen, elixir-format msgid "unauthorized" msgstr "" + +#: lib/memex/contexts/context.ex:75 +#: lib/memex/notes/note.ex:74 +#: lib/memex/pipelines/pipeline.ex:76 +#, elixir-autogen, elixir-format, fuzzy +msgid "invalid format: only numbers, letters and hyphen are accepted. tags must be comma-delimited" +msgstr "" diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 82169a1..0c0f7ae 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -290,17 +290,17 @@ msgstr "" msgid "report bugs or request features" msgstr "" -#: lib/memex_web/live/context_live/form_component.html.heex:43 -#: lib/memex_web/live/note_live/form_component.html.heex:43 -#: lib/memex_web/live/pipeline_live/form_component.html.heex:43 +#: lib/memex_web/live/context_live/form_component.html.heex:41 +#: lib/memex_web/live/note_live/form_component.html.heex:41 +#: lib/memex_web/live/pipeline_live/form_component.html.heex:41 #: lib/memex_web/live/step_live/form_component.html.heex:29 #, elixir-autogen, elixir-format msgid "saving..." msgstr "" -#: lib/memex_web/live/context_live/form_component.html.heex:39 -#: lib/memex_web/live/note_live/form_component.html.heex:39 -#: lib/memex_web/live/pipeline_live/form_component.html.heex:39 +#: lib/memex_web/live/context_live/form_component.html.heex:37 +#: lib/memex_web/live/note_live/form_component.html.heex:37 +#: lib/memex_web/live/pipeline_live/form_component.html.heex:37 #, elixir-autogen, elixir-format msgid "select privacy" msgstr "" diff --git a/priv/gettext/errors.pot b/priv/gettext/errors.pot index d25880c..308c766 100644 --- a/priv/gettext/errors.pot +++ b/priv/gettext/errors.pot @@ -130,3 +130,10 @@ msgstr "" #, elixir-autogen, elixir-format msgid "unauthorized" msgstr "" + +#: lib/memex/contexts/context.ex:75 +#: lib/memex/notes/note.ex:74 +#: lib/memex/pipelines/pipeline.ex:76 +#, elixir-autogen, elixir-format +msgid "invalid format: only numbers, letters and hyphen are accepted. tags must be comma-delimited" +msgstr "" diff --git a/test/memex_web/live/context_live_test.exs b/test/memex_web/live/context_live_test.exs index 9fbe061..8a824f1 100644 --- a/test/memex_web/live/context_live_test.exs +++ b/test/memex_web/live/context_live_test.exs @@ -18,7 +18,7 @@ defmodule MemexWeb.ContextLiveTest do } @invalid_attrs %{ "content" => nil, - "tags_string" => "", + "tags_string" => " ", "slug" => nil, "visibility" => nil } @@ -114,9 +114,13 @@ defmodule MemexWeb.ContextLiveTest do assert_patch(show_live, Routes.context_show_path(conn, :edit, context.slug)) - assert show_live - |> form("#context-form", context: @invalid_attrs) - |> render_change() =~ "can't be blank" + html = + show_live + |> form("#context-form", context: @invalid_attrs) + |> render_change() + + assert html =~ "can't be blank" + assert html =~ "tags must be comma-delimited" {:ok, _, html} = show_live diff --git a/test/memex_web/live/note_live_test.exs b/test/memex_web/live/note_live_test.exs index d8d709b..91144e1 100644 --- a/test/memex_web/live/note_live_test.exs +++ b/test/memex_web/live/note_live_test.exs @@ -19,7 +19,7 @@ defmodule MemexWeb.NoteLiveTest do } @invalid_attrs %{ "content" => nil, - "tags_string" => "", + "tags_string" => " ", "slug" => nil, "visibility" => nil } @@ -54,9 +54,13 @@ defmodule MemexWeb.NoteLiveTest do assert_patch(index_live, Routes.note_index_path(conn, :new)) - assert index_live - |> form("#note-form", note: @invalid_attrs) - |> render_change() =~ "can't be blank" + html = + index_live + |> form("#note-form", note: @invalid_attrs) + |> render_change() + + assert html =~ "can't be blank" + assert html =~ "tags must be comma-delimited" {:ok, _, html} = index_live diff --git a/test/memex_web/live/pipeline_live_test.exs b/test/memex_web/live/pipeline_live_test.exs index c7778d1..a6e8f5c 100644 --- a/test/memex_web/live/pipeline_live_test.exs +++ b/test/memex_web/live/pipeline_live_test.exs @@ -17,7 +17,7 @@ defmodule MemexWeb.PipelineLiveTest do } @invalid_attrs %{ "description" => nil, - "tags_string" => "", + "tags_string" => " ", "slug" => nil, "visibility" => nil } @@ -128,9 +128,13 @@ defmodule MemexWeb.PipelineLiveTest do assert_patch(show_live, Routes.pipeline_show_path(conn, :edit, pipeline.slug)) - assert show_live - |> form("#pipeline-form", pipeline: @invalid_attrs) - |> render_change() =~ "can't be blank" + html = + show_live + |> form("#pipeline-form", pipeline: @invalid_attrs) + |> render_change() + + assert html =~ "can't be blank" + assert html =~ "tags must be comma-delimited" {:ok, _, html} = show_live diff --git a/test/support/fixtures/contexts_fixtures.ex b/test/support/fixtures/contexts_fixtures.ex index a75a5cb..ec66478 100644 --- a/test/support/fixtures/contexts_fixtures.ex +++ b/test/support/fixtures/contexts_fixtures.ex @@ -22,6 +22,6 @@ defmodule Memex.ContextsFixtures do }) |> Contexts.create_context(user) - context + %{context | tags_string: nil} end end diff --git a/test/support/fixtures/notes_fixtures.ex b/test/support/fixtures/notes_fixtures.ex index 7d15667..8312f0e 100644 --- a/test/support/fixtures/notes_fixtures.ex +++ b/test/support/fixtures/notes_fixtures.ex @@ -22,6 +22,6 @@ defmodule Memex.NotesFixtures do }) |> Notes.create_note(user) - note + %{note | tags_string: nil} end end diff --git a/test/support/fixtures/pipelines_fixtures.ex b/test/support/fixtures/pipelines_fixtures.ex index 07d8435..b4e46ed 100644 --- a/test/support/fixtures/pipelines_fixtures.ex +++ b/test/support/fixtures/pipelines_fixtures.ex @@ -22,6 +22,6 @@ defmodule Memex.PipelinesFixtures do }) |> Pipelines.create_pipeline(user) - pipeline + %{pipeline | tags_string: nil} end end