diff --git a/lib/skate/settings/db/test_group.ex b/lib/skate/settings/db/test_group.ex index 2231d2461..f5b8430c1 100644 --- a/lib/skate/settings/db/test_group.ex +++ b/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 diff --git a/lib/skate/settings/test_group.ex b/lib/skate/settings/test_group.ex index 7eed4a7f9..843e873de 100644 --- a/lib/skate/settings/test_group.ex +++ b/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 diff --git a/lib/skate_web/controllers/test_group_controller.ex b/lib/skate_web/controllers/test_group_controller.ex index a43ea7ea3..d2ecfe6df 100644 --- a/lib/skate_web/controllers/test_group_controller.ex +++ b/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"}) diff --git a/lib/skate_web/router.ex b/lib/skate_web/router.ex index 4324aebf4..6b64d9415 100644 --- a/lib/skate_web/router.ex +++ b/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 diff --git a/lib/skate_web/templates/layout/barebones.html.eex b/lib/skate_web/templates/layout/barebones.html.eex index 6ac292a86..4a53d1061 100644 --- a/lib/skate_web/templates/layout/barebones.html.eex +++ b/lib/skate_web/templates/layout/barebones.html.eex @@ -17,6 +17,13 @@ " type="image/png"> " sizes="32x32" type="image/png"> " sizes="16x16" type="image/vnd.microsoft.icon"> + + diff --git a/lib/skate_web/templates/test_group/index.html.eex b/lib/skate_web/templates/test_group/index.html.eex index 2ef776a41..0fc3613a3 100644 --- a/lib/skate_web/templates/test_group/index.html.eex +++ b/lib/skate_web/templates/test_group/index.html.eex @@ -4,9 +4,14 @@
  • <%= link test_group.name, to: Routes.test_group_path(@conn, :show, test_group.id) %>
  • <% end %> -<%= 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 -> %> +
    + <%= label do %> + Name: <%= text_input f, :name %> + <% end %> + <%= submit "Create" %> +
    +
    + <%= error_tag f, :name %> +
    <% end %> diff --git a/lib/skate_web/templates/test_group/test_group.html.eex b/lib/skate_web/templates/test_group/test_group.html.eex index ae09026b1..593b36f32 100644 --- a/lib/skate_web/templates/test_group/test_group.html.eex +++ b/lib/skate_web/templates/test_group/test_group.html.eex @@ -12,3 +12,6 @@ <%= 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 %> diff --git a/lib/skate_web/views/error_helpers.ex b/lib/skate_web/views/error_helpers.ex index b3bca2aa4..1ee7157e5 100644 --- a/lib/skate_web/views/error_helpers.ex +++ b/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 diff --git a/test/skate/settings/test_group_test.exs b/test/skate/settings/test_group_test.exs index 5aa7d52a4..2a3c1156d 100644 --- a/test/skate/settings/test_group_test.exs +++ b/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,7 +68,7 @@ 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"}) @@ -45,7 +76,7 @@ defmodule Skate.Settings.TestGroupTest do end test "updates users" do - test_group = TestGroup.create("name") + {:ok, test_group} = TestGroup.create("name") user1 = User.upsert("user1", "user1@test.com") user2 = User.upsert("user2", "user2@test.com") user3 = User.upsert("user3", "user3@test.com") @@ -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 diff --git a/test/skate/settings/user_test.exs b/test/skate/settings/user_test.exs index e1751f910..d66e5bb04 100644 --- a/test/skate/settings/user_test.exs +++ b/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", "otheruser@test.com") - 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]}) diff --git a/test/skate_web/channels/vehicle_channel_test.exs b/test/skate_web/channels/vehicle_channel_test.exs index 784482e39..cda980fde 100644 --- a/test/skate_web/channels/vehicle_channel_test.exs +++ b/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) diff --git a/test/skate_web/channels/vehicles_channel_test.exs b/test/skate_web/channels/vehicles_channel_test.exs index 1bbf5f8b8..21fc304c6 100644 --- a/test/skate_web/channels/vehicles_channel_test.exs +++ b/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 = diff --git a/test/skate_web/controllers/page_controller_test.exs b/test/skate_web/controllers/page_controller_test.exs index 13272c9ab..f758c84ff 100644 --- a/test/skate_web/controllers/page_controller_test.exs +++ b/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] }) diff --git a/test/skate_web/controllers/test_group_controller_test.exs b/test/skate_web/controllers/test_group_controller_test.exs index 232052786..621839969 100644 --- a/test/skate_web/controllers/test_group_controller_test.exs +++ b/test/skate_web/controllers/test_group_controller_test.exs @@ -14,7 +14,7 @@ defmodule SkateWeb.TestGroupControllerTest do @tag :authenticated_admin test "returns page with test groups listed", %{conn: conn, user: user} do - test_group = TestGroup.create("test group name") + {:ok, test_group} = TestGroup.create("test group name") TestGroup.update(%{test_group | users: [user]}) @@ -36,13 +36,50 @@ defmodule SkateWeb.TestGroupControllerTest do test "creates test group on submit", %{conn: conn} do conn = post(conn, SkateWeb.Router.Helpers.test_group_path(conn, :post), %{ - "name" => "group to create" + "test_group" => %{ + "name" => "group to create" + } }) assert redirected_to(conn) == SkateWeb.Router.Helpers.test_group_path(conn, :index) assert [%TestGroup{name: "group to create"}] = TestGroup.get_all() end + + @tag :authenticated_admin + test "gives a reasonable error message when trying to create a group with no name", %{ + conn: conn + } do + conn = + post(conn, SkateWeb.Router.Helpers.test_group_path(conn, :post), %{ + "test_group" => %{ + "name" => "" + } + }) + + html_response(conn, 400) =~ "Test group name is required" + + assert Enum.empty?(TestGroup.get_all()) + end + + @tag :authenticated_admin + test "gives a reasonable error message when trying to create a group with a duplicate name", + %{ + conn: conn + } do + TestGroup.create("duplicate group") + + conn = + post(conn, SkateWeb.Router.Helpers.test_group_path(conn, :post), %{ + "test_group" => %{ + "name" => "duplicate group" + } + }) + + html_response(conn, 400) =~ "Test group name has been taken" + + assert Enum.count(TestGroup.get_all()) == 1 + end end describe "show/2" do @@ -55,7 +92,7 @@ defmodule SkateWeb.TestGroupControllerTest do @tag :authenticated_admin test "renders a test group", %{conn: conn, user: user} do - test_group = TestGroup.create("group to show") + {:ok, test_group} = TestGroup.create("group to show") test_group_with_user = TestGroup.update(%TestGroup{test_group | users: [user]}) html = @@ -90,7 +127,7 @@ defmodule SkateWeb.TestGroupControllerTest do user1 = User.upsert("user1", "user1@test.com") user2 = User.upsert("user2", "user2@test.com") - test_group = TestGroup.create("group to add users to") + {:ok, test_group} = TestGroup.create("group to add users to") test_group_with_user = TestGroup.update(%TestGroup{test_group | users: [user1]}) html = @@ -124,7 +161,7 @@ defmodule SkateWeb.TestGroupControllerTest do test "adds user to test group", %{conn: conn} do user = User.upsert("user", "user@test.com") - test_group = TestGroup.create("group to add user to") + {:ok, test_group} = TestGroup.create("group to add user to") conn = post(conn, SkateWeb.Router.Helpers.test_group_path(conn, :add_user, test_group.id), %{ @@ -151,7 +188,7 @@ defmodule SkateWeb.TestGroupControllerTest do @tag :authenticated_admin test "handles case where no user is found", %{conn: conn} do - test_group = TestGroup.create("group to add user to") + {:ok, test_group} = TestGroup.create("group to add user to") conn = post(conn, SkateWeb.Router.Helpers.test_group_path(conn, :add_user, test_group.id), %{ @@ -175,7 +212,7 @@ defmodule SkateWeb.TestGroupControllerTest do user1 = User.upsert("user1", "user1@test.com") user2 = User.upsert("user2", "user2@test.com") - test_group = TestGroup.create("test group") + {:ok, test_group} = TestGroup.create("test group") test_group_with_users = TestGroup.update(%TestGroup{test_group | users: [user1, user2]}) conn = @@ -194,7 +231,7 @@ defmodule SkateWeb.TestGroupControllerTest do user1 = User.upsert("user1", "user1@test.com") user2 = User.upsert("user2", "user2@test.com") - test_group = TestGroup.create("test group") + {:ok, test_group} = TestGroup.create("test group") TestGroup.update(%TestGroup{test_group | users: [user1]}) conn = @@ -215,4 +252,29 @@ defmodule SkateWeb.TestGroupControllerTest do assert response(conn, 404) =~ "no test group found" end end + + describe "delete/2" do + @tag :authenticated + test "when not an admin, redirects to unauthorized page", %{conn: conn} do + conn = get(conn, SkateWeb.Router.Helpers.test_group_path(conn, :index)) + + assert redirected_to(conn) == SkateWeb.Router.Helpers.unauthorized_path(conn, :index) + end + + @tag :authenticated_admin + test "redirects to test groups index page", %{conn: conn} do + {:ok, test_group} = TestGroup.create("test group") + conn = delete(conn, SkateWeb.Router.Helpers.test_group_path(conn, :delete, test_group.id)) + + assert redirected_to(conn) == SkateWeb.Router.Helpers.test_group_path(conn, :index) + end + + @tag :authenticated_admin + test "deletes the test group", %{conn: conn} do + {:ok, test_group} = TestGroup.create("test group") + delete(conn, SkateWeb.Router.Helpers.test_group_path(conn, :delete, test_group.id)) + + assert Enum.empty?(TestGroup.get_all()) + end + end end