feat handling on the test-groups page (#2288)

* fix: Improve error handling on the test-groups page

* feat: Add ability to delete a test group

* feat: Trim leading and trailing whitespace from test group names

lib/skate/settings/db/test_group.ex

@@ -19,5 +19,6 @@ defmodule Skate.Settings.Db.TestGroup do
     |> cast(attrs, [:name])
     |> cast_assoc(:test_group_users)
     |> validate_required([:name])
+    |> unique_constraint(:name, name: :test_groups_unique_index)
   end
 end

lib/skate/settings/test_group.ex

@@ -12,12 +12,21 @@ defmodule Skate.Settings.TestGroup do
 
   defstruct [:id, :name, users: []]
 
-  @spec create(String.t()) :: __MODULE__.t()
+  @spec create(String.t()) :: {:ok, t()} | {:error, Ecto.Changeset.t()}
   def create(name) do
-    %DbTestGroup{name: name}
-    |> Skate.Repo.insert!()
-    |> Skate.Repo.preload(:users)
-    |> convert_from_db_test_group()
+    %DbTestGroup{}
+    |> DbTestGroup.changeset(%{name: String.trim(name)})
+    |> Skate.Repo.insert()
+    |> case do
+      {:ok, new_test_group} ->
+        {:ok,
+         new_test_group
+         |> Skate.Repo.preload(:users)
+         |> convert_from_db_test_group()}
+
+      {:error, errored_changeset} ->
+        {:error, errored_changeset}
+    end
   end
 
   @spec get(integer()) :: t() | nil
@@ -72,4 +81,12 @@ defmodule Skate.Settings.TestGroup do
       users: db_test_group.users
     }
   end
+
+  @doc """
+  Deletes a test group with the given ID
+  """
+  @spec delete(integer()) :: nil
+  def delete(id) do
+    Skate.Repo.delete(%DbTestGroup{id: id})
+  end
 end

lib/skate_web/controllers/test_group_controller.ex

@@ -11,19 +11,31 @@ defmodule SkateWeb.TestGroupController do
 
   @spec index(Plug.Conn.t(), map()) :: Plug.Conn.t()
   def index(conn, _params) do
-    test_groups = TestGroup.get_all() |> Enum.sort_by(& &1.name)
+    test_groups = Enum.sort_by(TestGroup.get_all(), & &1.name)
 
     conn
     |> assign(:test_groups, test_groups)
+    |> assign(:changeset, Skate.Settings.Db.TestGroup.changeset(%Skate.Settings.Db.TestGroup{}))
     |> put_layout()
     |> render("index.html")
   end
 
   @spec post(Plug.Conn.t(), map()) :: Plug.Conn.t()
   def post(conn, params) do
-    TestGroup.create(params["name"])
-
-    redirect(conn, to: SkateWeb.Router.Helpers.test_group_path(conn, :index))
+    case TestGroup.create(params["test_group"]["name"]) do
+      {:ok, _} ->
+        redirect(conn, to: SkateWeb.Router.Helpers.test_group_path(conn, :index))
+
+      {:error, changeset} ->
+        test_groups = Enum.sort_by(TestGroup.get_all(), & &1.name)
+
+        conn
+        |> assign(:test_groups, test_groups)
+        |> assign(:changeset, changeset)
+        |> put_layout()
+        |> put_status(:bad_request)
+        |> render("index.html")
+    end
   end
 
   @spec show(Plug.Conn.t(), map()) :: Plug.Conn.t()
@@ -116,6 +128,13 @@ defmodule SkateWeb.TestGroupController do
     end
   end
 
+  @spec delete(Plug.Conn.t(), map()) :: Plug.Conn.t()
+  def delete(conn, %{"id" => id}) do
+    {id, _} = Integer.parse(id)
+    TestGroup.delete(id)
+    redirect(conn, to: SkateWeb.Router.Helpers.test_group_path(conn, :index))
+  end
+
   defp put_layout(conn) do
     conn
     |> put_layout({SkateWeb.LayoutView, "barebones.html"})

lib/skate_web/router.ex

@@ -109,6 +109,7 @@ defmodule SkateWeb.Router do
     get "/test_groups", TestGroupController, :index
     post "/test_groups/create", TestGroupController, :post
     get "/test_groups/:id", TestGroupController, :show
+    delete "/test_groups/:id", TestGroupController, :delete
     get "/test_groups/:id/add_user", TestGroupController, :add_user_form
     post "/test_groups/:id/add_user", TestGroupController, :add_user
     post "/test_groups/:id/remove_user", TestGroupController, :remove_user

lib/skate_web/templates/layout/barebones.html.eex

@@ -17,6 +17,13 @@
         <link rel="apple-touch-icon" href="<%= static_href.(@conn, "/images/mbta-logo-t-180.png") %>" type="image/png">
         <link rel="icon" href="<%= static_href.(@conn, "/images/mbta-logo-t-favicon.png") %>" sizes="32x32" type="image/png">
         <link rel="icon" href="<%= static_href.(@conn, "/favicon.ico") %>" sizes="16x16" type="image/vnd.microsoft.icon">
+
+        <style>
+            .error-tag {
+                color: red;
+                font-style: italic;
+            }
+        </style>
     </head>
 
     <body>

lib/skate_web/templates/test_group/index.html.eex

@@ -4,9 +4,14 @@
         <li><%= link test_group.name, to: Routes.test_group_path(@conn, :show, test_group.id) %></li>
     <% end %>
 </ul>
-<%= form_for @conn, Routes.test_group_path(@conn, :post), fn f -> %>
-    <%= label do %>
-        Name: <%= text_input f, :name %>
-    <% end %>
-    <%= submit "Create" %>
+<%= form_for @changeset, Routes.test_group_path(@conn, :post), fn f -> %>
+    <div>
+        <%= label do %>
+            Name: <%= text_input f, :name %>
+        <% end %>
+        <%= submit "Create" %>
+    </div>
+    <div>
+        <%= error_tag f, :name %>
+    </div>
 <% end %>

lib/skate_web/templates/test_group/test_group.html.eex

@@ -12,3 +12,6 @@
 </ul>
 <%= link "Add user", to: Routes.test_group_path(@conn, :add_user_form, @test_group_id) %>
 <%= link "Back to all test groups", to: Routes.test_group_path(@conn, :index) %>
+<%= form_for @conn, Routes.test_group_path(@conn, :delete, @test_group_id), [method: "DELETE"], fn _f -> %>
+    <%= submit "Delete group", [onclick: "return confirm('Are you sure? This cannot be undone.')"] %>
+<% end %>

lib/skate_web/views/error_helpers.ex

@@ -4,4 +4,17 @@ defmodule SkateWeb.ErrorHelpers do
   """
 
   use Phoenix.HTML
+
+  @doc """
+  Generates a tag for inlined form input errors.
+  """
+  def error_tag(form, field) do
+    case form.errors[field] do
+      {error, _} ->
+        content_tag(:span, error, class: "error-tag")
+
+      _ ->
+        nil
+    end
+  end
 end

test/skate/settings/test_group_test.exs

@@ -6,13 +6,44 @@ defmodule Skate.Settings.TestGroupTest do
 
   describe "create/1" do
     test "creates the test group" do
-      assert %TestGroup{name: "group name"} = TestGroup.create("group name")
+      assert {:ok, %TestGroup{name: "group name"}} = TestGroup.create("group name")
+
+      assert [%TestGroup{name: "group name"}] = TestGroup.get_all()
+    end
+
+    test "disallows blank group names" do
+      assert {:error, changeset} = TestGroup.create("")
+
+      assert %{errors: [name: {"can't be blank", _}]} = changeset
+      assert Enum.empty?(TestGroup.get_all())
+    end
+
+    test "returns an error if the test group has a duplicate name" do
+      TestGroup.create("duplicate name")
+      assert {:error, changeset} = TestGroup.create("duplicate name")
+
+      assert %{errors: [name: {"has already been taken", _}]} = changeset
+      assert Enum.count(TestGroup.get_all()) == 1
+    end
+
+    test "strips leading and trailing spaces" do
+      assert {:ok, %TestGroup{name: "lost in space"}} = TestGroup.create("  lost in space   ")
+
+      assert [%TestGroup{name: "lost in space"}] = TestGroup.get_all()
+    end
+
+    test "treats names as duplicates if they differ by leading and trailing spaces" do
+      TestGroup.create("lost in space   ")
+      assert {:error, changeset} = TestGroup.create("     lost in space")
+
+      assert %{errors: [name: {"has already been taken", _}]} = changeset
+      assert Enum.count(TestGroup.get_all()) == 1
     end
   end
 
   describe "get/1" do
     test "retrieves the test group" do
-      test_group = TestGroup.create("group name")
+      {:ok, test_group} = TestGroup.create("group name")
       assert %TestGroup{name: "group name"} = TestGroup.get(test_group.id)
     end
 
@@ -23,8 +54,8 @@ defmodule Skate.Settings.TestGroupTest do
 
   describe "get_all/1" do
     test "gets all test groups" do
-      group1 = TestGroup.create("group 1")
-      group2 = TestGroup.create("group 2")
+      {:ok, group1} = TestGroup.create("group 1")
+      {:ok, group2} = TestGroup.create("group 2")
 
       all_groups = TestGroup.get_all()
 
@@ -37,15 +68,15 @@ defmodule Skate.Settings.TestGroupTest do
 
   describe "update/1" do
     test "updates name" do
-      test_group = TestGroup.create("name 1")
+      {:ok, test_group} = TestGroup.create("name 1")
 
       new_test_group = TestGroup.update(%{test_group | name: "name 2"})
 
       assert new_test_group.name == "name 2"
     end
 
     test "updates users" do
-      test_group = TestGroup.create("name")
+      {:ok, test_group} = TestGroup.create("name")
       user1 = User.upsert("user1", "[email protected]")
       user2 = User.upsert("user2", "[email protected]")
       user3 = User.upsert("user3", "[email protected]")
@@ -67,4 +98,14 @@ defmodule Skate.Settings.TestGroupTest do
       assert user3 in users_update_2
     end
   end
+
+  describe "delete/1" do
+    test "deletes a test group" do
+      {:ok, test_group} = TestGroup.create("group to delete")
+
+      TestGroup.delete(test_group.id)
+
+      assert Enum.empty?(TestGroup.get_all())
+    end
+  end
 end

test/skate/settings/user_test.exs

@@ -123,8 +123,8 @@ defmodule Skate.Settings.UserTest do
     test "returns true only if given is in test group" do
       user_1 = User.upsert(@username, @email)
       user_2 = User.upsert("otheruser", "[email protected]")
-      target_test_group = TestGroup.create("target_test_group")
-      other_test_group = TestGroup.create("other_test_group")
+      {:ok, target_test_group} = TestGroup.create("target_test_group")
+      {:ok, other_test_group} = TestGroup.create("other_test_group")
 
       target_test_group = TestGroup.update(%{target_test_group | users: [user_1, user_2]})
       other_test_group = TestGroup.update(%{other_test_group | users: [user_2]})

test/skate_web/channels/vehicle_channel_test.exs

@@ -117,7 +117,7 @@ defmodule SkateWeb.VehicleChannelTest do
       socket: socket,
       user: user
     } do
-      test_group = Skate.Settings.TestGroup.create("map-beta")
+      {:ok, test_group} = Skate.Settings.TestGroup.create("map-beta")
       Skate.Settings.TestGroup.update(%{test_group | users: [user]})
 
       logged_out_vehicle = build(:vehicle, id: "y1234", label: "1234", route_id: nil, run_id: nil)

test/skate_web/channels/vehicles_channel_test.exs

@@ -196,7 +196,7 @@ defmodule SkateWeb.VehiclesChannelTest do
            socket: socket,
            user: user
          } do
-      test_group = Skate.Settings.TestGroup.create("map-beta")
+      {:ok, test_group} = Skate.Settings.TestGroup.create("map-beta")
       Skate.Settings.TestGroup.update(%{test_group | users: [user]})
 
       logged_in_vehicle =

test/skate_web/controllers/page_controller_test.exs

@@ -74,8 +74,10 @@ defmodule SkateWeb.PageControllerTest do
 
     @tag :authenticated
     test "includes user test groups in HTML", %{conn: conn, user: user} do
+      {:ok, test_group} = Skate.Settings.TestGroup.create("html-test-group")
+
       Skate.Settings.TestGroup.update(%{
-        Skate.Settings.TestGroup.create("html-test-group")
+        test_group
         | users: [user]
       })