improve tagging logic

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

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

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

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

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

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

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

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) %>
 

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) %>
 

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) %>
 

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"

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"

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 ""

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 ""

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 ""

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 ""

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

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

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

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

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

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