From 860c9ac321707ffd8795a54fa351c5e4482aba9b Mon Sep 17 00:00:00 2001 From: daniel_sp Date: Thu, 29 Jun 2023 18:06:46 +0100 Subject: [PATCH 1/5] Enhance enrollment controller --- lib/bokken/events.ex | 82 +++++++++++++++++-- lib/bokken/events/enrollment.ex | 9 ++ .../controllers/enrollment_controller.ex | 48 +++-------- .../controllers/fallback_controller.ex | 21 +++++ test/bokken/events_test.exs | 18 +++- .../enrollment_controller_test.exs | 56 +++++++++++-- 6 files changed, 182 insertions(+), 52 deletions(-) diff --git a/lib/bokken/events.ex b/lib/bokken/events.ex index f413fa99..0b7f2764 100644 --- a/lib/bokken/events.ex +++ b/lib/bokken/events.ex @@ -609,28 +609,34 @@ defmodule Bokken.Events do ## Examples - iex> create_enrollment(%{field: value}) + iex> create_enrollment(event, %{field: value}) {:ok, %Enrollment{}} - iex> create_enrollment(%{field: bad_value}) + iex> create_enrollment(event, %{field: bad_value}) {:error, %Ecto.Changeset{}} """ - def create_enrollment(event, attrs \\ %{}) do + def create_enrollment(event, role \\ :admin, attrs \\ %{}) do cur_time = DateTime.utc_now() ninja_id = Map.get(attrs, :ninja_id) || Map.get(attrs, "ninja_id") if DateTime.compare(event.enrollments_open, cur_time) == :lt and DateTime.compare(event.enrollments_close, cur_time) == :gt do if is_ninja_enrolled?(ninja_id, event.id) do - {:error, "Ninja already enrolled"} + {:error, :ninja_already_enrolled} else - %Enrollment{} - |> Enrollment.changeset(attrs) - |> Repo.insert() + case role do + :admin -> + insert_enrollment(:admin, attrs) + + :guardian -> + insert_enrollment(:guardian, attrs) + _ -> + {:error, :invalid_role} + end end else - {:error, "Enrollments are closed"} + {:error, :enrollments_closed} end end @@ -640,6 +646,42 @@ defmodule Bokken.Events do |> Repo.exists?() end + defp insert_enrollment(:admin, attrs) do + %Enrollment{} + |> Enrollment.changeset(attrs) + |> Repo.insert() + end + + defp insert_enrollment(:guardian, attrs) do + %Enrollment{} + |> Enrollment.guardian_changeset(attrs) + |> Repo.insert() + end + + @doc """ + Creates an enrollment as a guardian. + + ## Examples + + iex> guardian_create_enrollment(event, guardian_id, ninja_id, %{field: value}) + {:ok, %Enrollment{}} + + iex> guardian_create_enrollment(%{field: bad_value}) + {:error, %Ecto.Changeset{}} + + iex> guardian_create_enrollment(event, guardian_id, ninja_id, %{field: value}) + {:error, :not_ninja_guardian} + + """ + def guardian_create_enrollment(event, guardian_id, ninja_id, attrs \\ %{}) do + ninja = Accounts.get_ninja!(ninja_id, [:guardian]) + if ninja.guardian.id == guardian_id do + create_enrollment(event, :guardian, attrs) + else + {:error, :not_ninja_guardian} + end + end + @doc """ Updates an enrollment. @@ -681,6 +723,30 @@ defmodule Bokken.Events do end end + @doc """ + Creates an enrollment as a guardian. + + ## Examples + + iex> guardian_delete_enrollment(event, guardian_id, ninja_id, %{field: value}) + {:ok, %Enrollment{}} + + iex> guardian_delete_enrollment(%{field: bad_value}) + {:error, %Ecto.Changeset{}} + + iex> guardian_delete_enrollment(event, guardian_id, ninja_id, %{field: value}) + {:error, :not_ninja_guardian} + + """ + def guardian_delete_enrollment(enrollment, guardian_id, ninja_id, attrs \\ %{}) do + ninja = Accounts.get_ninja!(ninja_id, [:guardian]) + if ninja.guardian.id == guardian_id do + delete_enrollment(enrollment) + else + {:error, :not_ninja_guardian} + end + end + @doc """ Returns an `%Ecto.Changeset{}` for tracking enrollment changes. diff --git a/lib/bokken/events/enrollment.ex b/lib/bokken/events/enrollment.ex index 350ed208..6560325f 100644 --- a/lib/bokken/events/enrollment.ex +++ b/lib/bokken/events/enrollment.ex @@ -28,4 +28,13 @@ defmodule Bokken.Events.Enrollment do |> assoc_constraint(:event) |> assoc_constraint(:ninja) end + + def guardian_changeset(enrollment, attrs) do + enrollment + |> cast(attrs, @required_fields ++ @optional_fields) + |> validate_required(@required_fields) + |> validate_inclusion(:accepted, [false], message: "Guardian cannot submit an accepted enrollment") + |> assoc_constraint(:event) + |> assoc_constraint(:ninja) + end end diff --git a/lib/bokken_web/controllers/enrollment_controller.ex b/lib/bokken_web/controllers/enrollment_controller.ex index e184d8da..c3dfa757 100644 --- a/lib/bokken_web/controllers/enrollment_controller.ex +++ b/lib/bokken_web/controllers/enrollment_controller.ex @@ -1,7 +1,6 @@ defmodule BokkenWeb.EnrollmentController do use BokkenWeb, :controller - alias Bokken.Accounts alias Bokken.Events alias Bokken.Events.Enrollment @@ -41,7 +40,7 @@ defmodule BokkenWeb.EnrollmentController do conn, %{ "enrollment" => - %{"ninja_id" => ninja_id, "accepted" => accepted, "event_id" => event_id} = + %{"ninja_id" => ninja_id, "accepted" => _accepted, "event_id" => event_id} = enrollment_params } ) @@ -49,50 +48,28 @@ defmodule BokkenWeb.EnrollmentController do guardian = conn.assigns.current_user.guardian event = Events.get_event!(event_id) - cond do - # credo:disable-for-next-line Credo.Check.Design.TagTODO - # TODO verify guardian inside the Events.create_enrollment function - !is_guardian_of_ninja(guardian, ninja_id) -> - conn - |> put_status(:forbidden) - |> render("error.json", reason: "Not the ninja's guardian") - - # credo:disable-for-next-line Credo.Check.Design.TagTODO - # TODO create a create_enrollement_changeset for guardians and one for admins - accepted -> - conn - |> put_status(:forbidden) - |> render("error.json", reason: "Guardian cannot submit an accepted enrollment") - - true -> - with {:ok, %Enrollment{} = enrollment} <- - Events.create_enrollment(event, enrollment_params) do - conn - |> put_status(:created) - |> render("show.json", enrollment: enrollment) - end + with {:ok, enrollment} <- + Events.guardian_create_enrollment(event, guardian.id, ninja_id, enrollment_params) do + conn + |> put_status(:created) + |> render("show.json", enrollment: enrollment) end end def delete(conn, %{"id" => enrollment_id}) when is_guardian(conn) do enrollment = Events.get_enrollment(enrollment_id, [:ninja]) guardian = conn.assigns.current_user.guardian + ninja = enrollment.ninja if is_nil(enrollment) do conn |> put_status(:not_found) |> render("error.json", reason: "No such enrollment") else - if is_guardian_of_ninja(guardian, enrollment.ninja.id) do - with {:ok, %Enrollment{}} <- Events.delete_enrollment(enrollment) do - conn - |> put_status(:ok) - |> render("success.json", message: "Enrollment deleted successfully") - end - else + with {:ok, %Enrollment{}} <- Events.guardian_delete_enrollment(enrollment, guardian.id, ninja.id) do conn - |> put_status(:unauthorized) - |> render("error.json", reason: "Not the ninja's guardian") + |> put_status(:ok) + |> render("success.json", message: "Enrollment deleted successfully") end end end @@ -107,9 +84,4 @@ defmodule BokkenWeb.EnrollmentController do |> render("show.json", enrollment: new_enrollment) end end - - defp is_guardian_of_ninja(guardian, ninja_id) do - ninja = Accounts.get_ninja!(ninja_id, [:guardian]) - ninja.guardian.id == guardian.id - end end diff --git a/lib/bokken_web/controllers/fallback_controller.ex b/lib/bokken_web/controllers/fallback_controller.ex index 209e69b4..c3f0cc9b 100644 --- a/lib/bokken_web/controllers/fallback_controller.ex +++ b/lib/bokken_web/controllers/fallback_controller.ex @@ -34,4 +34,25 @@ defmodule BokkenWeb.FallbackController do |> put_view(BokkenWeb.ErrorView) |> render(:"401") end + + def call(conn, {:error, :ninja_already_enrolled}) do + conn + |> put_status(:forbidden) + |> put_view(BokkenWeb.ErrorView) + |> render(:"403") + end + + def call(conn, {:error, :enrollments_closed}) do + conn + |> put_status(:forbidden) + |> put_view(BokkenWeb.ErrorView) + |> render(:"403") + end + + def call(conn, {:error, :not_ninja_guardian}) do + conn + |> put_status(:forbidden) + |> put_view(BokkenWeb.ErrorView) + |> render(:"403") + end end diff --git a/test/bokken/events_test.exs b/test/bokken/events_test.exs index 219e2cfb..9be0523a 100644 --- a/test/bokken/events_test.exs +++ b/test/bokken/events_test.exs @@ -37,7 +37,7 @@ defmodule Bokken.EventsTest do assert is_nil(Events.get_enrollment(Ecto.UUID.generate(), [:ninja, :event])) end - test "create_enrollment/1 returns error if the enrollments are closed" do + test "create_enrollment/2 returns error if the enrollments are closed" do valid_attrs = insert(:enrollment) event = Events.get_event!(valid_attrs.event_id) @@ -52,6 +52,22 @@ defmodule Bokken.EventsTest do assert elem(Events.create_enrollment(event, valid_attrs), 0) == :error end + # test "guardian_create_enrollment/4 creates a new enrollment" do + # guardian = insert(:guardian) + # ninja = insert(:ninja) + # valid_attrs = insert(:enrollment, %{ninja_id: ninja.id}) + # event = Events.get_event!(valid_attrs.event_id) + + # assert elem(Events.guardian_create_enrollment(event, guardian.id, ninja.id, valid_attrs), 0) == :ok + # end + + test "guardian_create_enrollment/4 returns error if it's not the ninja guaridan" do + valid_attrs = insert(:enrollment) + event = Events.get_event!(valid_attrs.event_id) + + assert elem(Events.guardian_create_enrollment(event, Ecto.UUID.generate(), valid_attrs.ninja_id, valid_attrs), 0) == :error + end + test "update_enrollment/2 updates existing enrollment" do enrollment = insert(:enrollment) diff --git a/test/bokken_web/controllers/enrollment_controller_test.exs b/test/bokken_web/controllers/enrollment_controller_test.exs index e1fec19f..2f509b91 100644 --- a/test/bokken_web/controllers/enrollment_controller_test.exs +++ b/test/bokken_web/controllers/enrollment_controller_test.exs @@ -14,6 +14,37 @@ defmodule BokkenWeb.EnrollmentControllerTest do {:ok, conn: log_in_user(conn, guardian_user), ninja: ninja, event: event} end + describe "show enrollment" do + test "renders enrollment when data is valid", %{ + conn: conn, + ninja: ninja, + event: event + } do + enrollment_attrs = %{enrollment: %{event_id: event.id, ninja_id: ninja.id, accepted: false}} + + conn = post(conn, Routes.event_enrollment_path(conn, :create, event.id), enrollment_attrs) + assert %{"id" => enrollment_id} = json_response(conn, 201)["data"] + + + conn = get(conn, Routes.event_enrollment_path(conn, :show, event.id, enrollment_id)) + assert json_response(conn, 200) + end + + test "fails when enrollment_id is invalid", %{ + conn: conn, + ninja: ninja, + event: event + } do + enrollment_attrs = %{enrollment: %{event_id: event.id, ninja_id: ninja.id, accepted: false}} + + conn = post(conn, Routes.event_enrollment_path(conn, :create, event.id), enrollment_attrs) + assert %{"id" => _enrollment_id} = json_response(conn, 201)["data"] + + conn = get(conn, Routes.event_enrollment_path(conn, :show, event.id, Ecto.UUID.generate())) + assert json_response(conn, 404) + end + end + describe "create enrollment" do test "renders enrollment when data is valid", %{ conn: conn, @@ -43,7 +74,7 @@ defmodule BokkenWeb.EnrollmentControllerTest do enrollment_attrs = %{enrollment: %{event_id: event.id, ninja_id: ninja.id, accepted: true}} conn = post(conn, Routes.event_enrollment_path(conn, :create, event.id), enrollment_attrs) - assert not is_nil(json_response(conn, 403)["reason"]) + assert json_response(conn, 422) end test "fails when user is not the ninja's guardian", %{ @@ -59,7 +90,7 @@ defmodule BokkenWeb.EnrollmentControllerTest do } conn = post(conn, Routes.event_enrollment_path(conn, :create, event.id), enrollment_attrs) - assert not is_nil(json_response(conn, 403)["reason"]) + assert json_response(conn, 403) end end @@ -94,7 +125,22 @@ defmodule BokkenWeb.EnrollmentControllerTest do assert json_response(conn, 200)["message"] conn = get(conn, Routes.event_enrollment_path(conn, :show, event.id, enrollment_id)) - assert not is_nil(json_response(conn, 404)["reason"]) + assert json_response(conn, 404) + end + + test "fails to delete enrollment when is not ninja's guardian", %{ + conn: conn, + ninja: _ninja, + event: event + } do + new_ninja = insert(:ninja) + + enrollment_attrs = %{event_id: event.id, ninja_id: new_ninja.id, accepted: false} + + {:ok, enrollment} = Events.create_enrollment(event, :admin, enrollment_attrs) + + conn = delete(conn, Routes.event_enrollment_path(conn, :delete, event.id, enrollment.id)) + assert json_response(conn, 403) end end @@ -136,7 +182,7 @@ defmodule BokkenWeb.EnrollmentControllerTest do event: event } do enrollment_attrs = %{event_id: event.id, ninja_id: ninja.id, accepted: false} - {:ok, enrollment} = Events.create_enrollment(event, enrollment_attrs) + {:ok, enrollment} = Events.create_enrollment(event, :admin, enrollment_attrs) new_enrollment_attrs = %{ enrollment: %{event_id: event.id, ninja_id: ninja.id, accepted: true, id: enrollment.id} @@ -149,7 +195,7 @@ defmodule BokkenWeb.EnrollmentControllerTest do new_enrollment_attrs ) - assert json_response(conn, 200)["data"] + assert json_response(conn, 200) end end end From d422afc63ffc066575e6a3bd3eef3900b88b303a Mon Sep 17 00:00:00 2001 From: daniel_sp Date: Sun, 2 Jul 2023 11:27:19 +0100 Subject: [PATCH 2/5] Aplly suggested changes --- lib/bokken/events.ex | 22 +++++++-------- lib/bokken/events/enrollment.ex | 6 ++++- lib/bokken/guards.ex | 10 ++++++- .../controllers/enrollment_controller.ex | 7 +++-- .../controllers/fallback_controller.ex | 21 --------------- priv/gettext/default.pot | 22 ++++++++++++++- priv/gettext/en/LC_MESSAGES/default.po | 22 ++++++++++++++- priv/gettext/pt/LC_MESSAGES/default.po | 22 ++++++++++++++- test/bokken/events_test.exs | 27 +++++++++++++------ .../enrollment_controller_test.exs | 1 - 10 files changed, 109 insertions(+), 51 deletions(-) diff --git a/lib/bokken/events.ex b/lib/bokken/events.ex index 0b7f2764..fe2c3756 100644 --- a/lib/bokken/events.ex +++ b/lib/bokken/events.ex @@ -625,15 +625,7 @@ defmodule Bokken.Events do if is_ninja_enrolled?(ninja_id, event.id) do {:error, :ninja_already_enrolled} else - case role do - :admin -> - insert_enrollment(:admin, attrs) - - :guardian -> - insert_enrollment(:guardian, attrs) - _ -> - {:error, :invalid_role} - end + insert_enrollment(role, attrs) end else {:error, :enrollments_closed} @@ -658,6 +650,10 @@ defmodule Bokken.Events do |> Repo.insert() end + defp insert_enrollment(_, _attrs) do + {:error, :invalid_role} + end + @doc """ Creates an enrollment as a guardian. @@ -666,7 +662,7 @@ defmodule Bokken.Events do iex> guardian_create_enrollment(event, guardian_id, ninja_id, %{field: value}) {:ok, %Enrollment{}} - iex> guardian_create_enrollment(%{field: bad_value}) + iex> guardian_create_enrollment(event, guardian_id, ninja_id, %{field: bad_value}) {:error, %Ecto.Changeset{}} iex> guardian_create_enrollment(event, guardian_id, ninja_id, %{field: value}) @@ -675,6 +671,7 @@ defmodule Bokken.Events do """ def guardian_create_enrollment(event, guardian_id, ninja_id, attrs \\ %{}) do ninja = Accounts.get_ninja!(ninja_id, [:guardian]) + if ninja.guardian.id == guardian_id do create_enrollment(event, :guardian, attrs) else @@ -724,7 +721,7 @@ defmodule Bokken.Events do end @doc """ - Creates an enrollment as a guardian. + Deletes an enrollment as a guardian. ## Examples @@ -738,8 +735,9 @@ defmodule Bokken.Events do {:error, :not_ninja_guardian} """ - def guardian_delete_enrollment(enrollment, guardian_id, ninja_id, attrs \\ %{}) do + def guardian_delete_enrollment(enrollment, guardian_id, ninja_id) do ninja = Accounts.get_ninja!(ninja_id, [:guardian]) + if ninja.guardian.id == guardian_id do delete_enrollment(enrollment) else diff --git a/lib/bokken/events/enrollment.ex b/lib/bokken/events/enrollment.ex index 6560325f..066f382d 100644 --- a/lib/bokken/events/enrollment.ex +++ b/lib/bokken/events/enrollment.ex @@ -4,6 +4,8 @@ defmodule Bokken.Events.Enrollment do """ use Bokken.Schema + import BokkenWeb.Gettext + alias Bokken.Accounts.Ninja alias Bokken.Events.Event @@ -33,7 +35,9 @@ defmodule Bokken.Events.Enrollment do enrollment |> cast(attrs, @required_fields ++ @optional_fields) |> validate_required(@required_fields) - |> validate_inclusion(:accepted, [false], message: "Guardian cannot submit an accepted enrollment") + |> validate_inclusion(:accepted, [false], + message: gettext("Guardian cannot submit an accepted enrollment") + ) |> assoc_constraint(:event) |> assoc_constraint(:ninja) end diff --git a/lib/bokken/guards.ex b/lib/bokken/guards.ex index 40c3faaf..7d5a060d 100644 --- a/lib/bokken/guards.ex +++ b/lib/bokken/guards.ex @@ -29,5 +29,13 @@ defmodule Bokken.Guards do """ defguard is_404(reason) when reason in [:not_found, :not_registered, :invalid_credentials] defguard is_401(reason) when reason in [:token_expired] - defguard is_403(reason) when reason in [:not_authorized, :not_allowed] + + defguard is_403(reason) + when reason in [ + :not_authorized, + :not_allowed, + :ninja_already_enrolled, + :enrollments_closed, + :not_ninja_guardian + ] end diff --git a/lib/bokken_web/controllers/enrollment_controller.ex b/lib/bokken_web/controllers/enrollment_controller.ex index c3dfa757..bb1d121a 100644 --- a/lib/bokken_web/controllers/enrollment_controller.ex +++ b/lib/bokken_web/controllers/enrollment_controller.ex @@ -39,9 +39,7 @@ defmodule BokkenWeb.EnrollmentController do def create( conn, %{ - "enrollment" => - %{"ninja_id" => ninja_id, "accepted" => _accepted, "event_id" => event_id} = - enrollment_params + "enrollment" => %{"ninja_id" => ninja_id, "event_id" => event_id} = enrollment_params } ) when is_guardian(conn) do @@ -66,7 +64,8 @@ defmodule BokkenWeb.EnrollmentController do |> put_status(:not_found) |> render("error.json", reason: "No such enrollment") else - with {:ok, %Enrollment{}} <- Events.guardian_delete_enrollment(enrollment, guardian.id, ninja.id) do + with {:ok, %Enrollment{}} <- + Events.guardian_delete_enrollment(enrollment, guardian.id, ninja.id) do conn |> put_status(:ok) |> render("success.json", message: "Enrollment deleted successfully") diff --git a/lib/bokken_web/controllers/fallback_controller.ex b/lib/bokken_web/controllers/fallback_controller.ex index c3f0cc9b..209e69b4 100644 --- a/lib/bokken_web/controllers/fallback_controller.ex +++ b/lib/bokken_web/controllers/fallback_controller.ex @@ -34,25 +34,4 @@ defmodule BokkenWeb.FallbackController do |> put_view(BokkenWeb.ErrorView) |> render(:"401") end - - def call(conn, {:error, :ninja_already_enrolled}) do - conn - |> put_status(:forbidden) - |> put_view(BokkenWeb.ErrorView) - |> render(:"403") - end - - def call(conn, {:error, :enrollments_closed}) do - conn - |> put_status(:forbidden) - |> put_view(BokkenWeb.ErrorView) - |> render(:"403") - end - - def call(conn, {:error, :not_ninja_guardian}) do - conn - |> put_status(:forbidden) - |> put_view(BokkenWeb.ErrorView) - |> render(:"403") - end end diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index cbd8f4c6..107bb67c 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -11,7 +11,27 @@ msgid "" msgstr "" -#: lib/bokken/ecto/pt_mobile.ex:60 +#: lib/bokken/ecto/pt_mobile.ex:61 #, elixir-autogen, elixir-format msgid "invalid PT phone number" msgstr "" + +#: lib/bokken/events/enrollment.ex:39 +#, elixir-autogen, elixir-format +msgid "Guardian cannot submit an accepted enrollment" +msgstr "" + +#: lib/bokken/events/event.ex:60 +#, elixir-autogen, elixir-format +msgid "must be greater than enrollments_open" +msgstr "" + +#: lib/bokken/events/event.ex:56 +#, elixir-autogen, elixir-format +msgid "must be greater than start_time" +msgstr "" + +#: lib/bokken/events/event.ex:62 +#, elixir-autogen, elixir-format +msgid "must be smaller than start_time" +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index f096499c..c8deeeb5 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -11,7 +11,27 @@ msgstr "" "Language: en\n" "Plural-Forms: nplurals=2; plural=(n != 1);\n" -#: lib/bokken/ecto/pt_mobile.ex:60 +#: lib/bokken/ecto/pt_mobile.ex:61 #, elixir-autogen, elixir-format msgid "invalid PT phone number" msgstr "" + +#: lib/bokken/events/enrollment.ex:39 +#, elixir-autogen, elixir-format +msgid "Guardian cannot submit an accepted enrollment" +msgstr "" + +#: lib/bokken/events/event.ex:60 +#, elixir-autogen, elixir-format +msgid "must be greater than enrollments_open" +msgstr "" + +#: lib/bokken/events/event.ex:56 +#, elixir-autogen, elixir-format +msgid "must be greater than start_time" +msgstr "" + +#: lib/bokken/events/event.ex:62 +#, elixir-autogen, elixir-format +msgid "must be smaller than start_time" +msgstr "" diff --git a/priv/gettext/pt/LC_MESSAGES/default.po b/priv/gettext/pt/LC_MESSAGES/default.po index 28af117c..2fdcaf1e 100644 --- a/priv/gettext/pt/LC_MESSAGES/default.po +++ b/priv/gettext/pt/LC_MESSAGES/default.po @@ -11,7 +11,27 @@ msgstr "" "Language: pt\n" "Plural-Forms: nplurals=2; plural=(n != 1);\n" -#: lib/bokken/ecto/pt_mobile.ex:60 +#: lib/bokken/ecto/pt_mobile.ex:61 #, elixir-autogen, elixir-format msgid "invalid PT phone number" msgstr "número de telemóvel PT inválido" + +#: lib/bokken/events/enrollment.ex:39 +#, elixir-autogen, elixir-format +msgid "Guardian cannot submit an accepted enrollment" +msgstr "" + +#: lib/bokken/events/event.ex:60 +#, elixir-autogen, elixir-format +msgid "must be greater than enrollments_open" +msgstr "" + +#: lib/bokken/events/event.ex:56 +#, elixir-autogen, elixir-format +msgid "must be greater than start_time" +msgstr "" + +#: lib/bokken/events/event.ex:62 +#, elixir-autogen, elixir-format +msgid "must be smaller than start_time" +msgstr "" diff --git a/test/bokken/events_test.exs b/test/bokken/events_test.exs index 9be0523a..265c99f1 100644 --- a/test/bokken/events_test.exs +++ b/test/bokken/events_test.exs @@ -52,20 +52,31 @@ defmodule Bokken.EventsTest do assert elem(Events.create_enrollment(event, valid_attrs), 0) == :error end - # test "guardian_create_enrollment/4 creates a new enrollment" do - # guardian = insert(:guardian) - # ninja = insert(:ninja) - # valid_attrs = insert(:enrollment, %{ninja_id: ninja.id}) - # event = Events.get_event!(valid_attrs.event_id) + test "guardian_create_enrollment/4 creates a new enrollment" do + event = insert(:event) + ninja = insert(:ninja) + guardian_id = ninja.guardian_id + + valid_attrs = + params_for(:enrollment, %{event_id: event.id, ninja_id: ninja.id, accepted: false}) - # assert elem(Events.guardian_create_enrollment(event, guardian.id, ninja.id, valid_attrs), 0) == :ok - # end + assert elem(Events.guardian_create_enrollment(event, guardian_id, ninja.id, valid_attrs), 0) == + :ok + end test "guardian_create_enrollment/4 returns error if it's not the ninja guaridan" do valid_attrs = insert(:enrollment) event = Events.get_event!(valid_attrs.event_id) - assert elem(Events.guardian_create_enrollment(event, Ecto.UUID.generate(), valid_attrs.ninja_id, valid_attrs), 0) == :error + assert elem( + Events.guardian_create_enrollment( + event, + Ecto.UUID.generate(), + valid_attrs.ninja_id, + valid_attrs + ), + 0 + ) == :error end test "update_enrollment/2 updates existing enrollment" do diff --git a/test/bokken_web/controllers/enrollment_controller_test.exs b/test/bokken_web/controllers/enrollment_controller_test.exs index 2f509b91..358f1159 100644 --- a/test/bokken_web/controllers/enrollment_controller_test.exs +++ b/test/bokken_web/controllers/enrollment_controller_test.exs @@ -25,7 +25,6 @@ defmodule BokkenWeb.EnrollmentControllerTest do conn = post(conn, Routes.event_enrollment_path(conn, :create, event.id), enrollment_attrs) assert %{"id" => enrollment_id} = json_response(conn, 201)["data"] - conn = get(conn, Routes.event_enrollment_path(conn, :show, event.id, enrollment_id)) assert json_response(conn, 200) end From 5dfd7ce09438406c04f51967875d572cda72b8ec Mon Sep 17 00:00:00 2001 From: daniel_sp Date: Mon, 14 Aug 2023 19:23:00 +0100 Subject: [PATCH 3/5] Add custom exceptions --- lib/bokken/events.ex | 26 +++++++------------ .../exceptions/UserHasNotPermissionError.ex | 3 +++ lib/bokken/guards.ex | 3 +-- test/bokken/events_test.exs | 21 ++++++++------- 4 files changed, 26 insertions(+), 27 deletions(-) create mode 100644 lib/bokken/exceptions/UserHasNotPermissionError.ex diff --git a/lib/bokken/events.ex b/lib/bokken/events.ex index 5d3aad83..d6929cc1 100644 --- a/lib/bokken/events.ex +++ b/lib/bokken/events.ex @@ -664,19 +664,16 @@ defmodule Bokken.Events do iex> guardian_create_enrollment(event, guardian_id, ninja_id, %{field: bad_value}) {:error, %Ecto.Changeset{}} - - iex> guardian_create_enrollment(event, guardian_id, ninja_id, %{field: value}) - {:error, :not_ninja_guardian} - """ def guardian_create_enrollment(event, guardian_id, ninja_id, attrs \\ %{}) do ninja = Accounts.get_ninja!(ninja_id, [:guardian]) - if ninja.guardian.id == guardian_id do - create_enrollment(event, :guardian, attrs) - else - {:error, :not_ninja_guardian} + unless ninja.guardian.id == guardian_id do + raise Bokken.UserHasNotPermissionError, + "This ninja does not belong to the guardian, so enrollment cannot be modified." end + + create_enrollment(event, :guardian, attrs) end @doc """ @@ -730,19 +727,16 @@ defmodule Bokken.Events do iex> guardian_delete_enrollment(%{field: bad_value}) {:error, %Ecto.Changeset{}} - - iex> guardian_delete_enrollment(event, guardian_id, ninja_id, %{field: value}) - {:error, :not_ninja_guardian} - """ def guardian_delete_enrollment(enrollment, guardian_id, ninja_id) do ninja = Accounts.get_ninja!(ninja_id, [:guardian]) - if ninja.guardian.id == guardian_id do - delete_enrollment(enrollment) - else - {:error, :not_ninja_guardian} + unless ninja.guardian.id == guardian_id do + raise Bokken.UserHasNotPermissionError, + "This ninja does not belong to the guardian, so enrollment cannot be modified." end + + delete_enrollment(enrollment) end @doc """ diff --git a/lib/bokken/exceptions/UserHasNotPermissionError.ex b/lib/bokken/exceptions/UserHasNotPermissionError.ex new file mode 100644 index 00000000..f99c8792 --- /dev/null +++ b/lib/bokken/exceptions/UserHasNotPermissionError.ex @@ -0,0 +1,3 @@ +defmodule Bokken.UserHasNotPermissionError do + defexception [:message, plug_status: 404] +end diff --git a/lib/bokken/guards.ex b/lib/bokken/guards.ex index 7d5a060d..29bdb2b4 100644 --- a/lib/bokken/guards.ex +++ b/lib/bokken/guards.ex @@ -35,7 +35,6 @@ defmodule Bokken.Guards do :not_authorized, :not_allowed, :ninja_already_enrolled, - :enrollments_closed, - :not_ninja_guardian + :enrollments_closed ] end diff --git a/test/bokken/events_test.exs b/test/bokken/events_test.exs index 42b31e84..d1a5dff0 100644 --- a/test/bokken/events_test.exs +++ b/test/bokken/events_test.exs @@ -68,15 +68,18 @@ defmodule Bokken.EventsTest do valid_attrs = insert(:enrollment) event = Events.get_event!(valid_attrs.event_id) - assert elem( - Events.guardian_create_enrollment( - event, - Ecto.UUID.generate(), - valid_attrs.ninja_id, - valid_attrs - ), - 0 - ) == :error + try do + Events.guardian_create_enrollment( + event, + Ecto.UUID.generate(), + valid_attrs.ninja_id, + valid_attrs + ) + rescue + e in Bokken.UserHasNotPermissionError -> + assert e.message == + "This ninja does not belong to the guardian, so enrollment cannot be modified." + end end test "update_enrollment/2 updates existing enrollment" do From 7c01443b48a37a0e48b7026bed59e70c78618fe5 Mon Sep 17 00:00:00 2001 From: daniel_sp Date: Tue, 15 Aug 2023 09:34:59 +0100 Subject: [PATCH 4/5] Refactor tests --- ...issionError.ex => user_has_not_permission_error.ex} | 2 +- test/bokken/events_test.exs | 6 +----- .../controllers/enrollment_controller_test.exs | 10 ++++++---- 3 files changed, 8 insertions(+), 10 deletions(-) rename lib/bokken/exceptions/{UserHasNotPermissionError.ex => user_has_not_permission_error.ex} (53%) diff --git a/lib/bokken/exceptions/UserHasNotPermissionError.ex b/lib/bokken/exceptions/user_has_not_permission_error.ex similarity index 53% rename from lib/bokken/exceptions/UserHasNotPermissionError.ex rename to lib/bokken/exceptions/user_has_not_permission_error.ex index f99c8792..ceeaa006 100644 --- a/lib/bokken/exceptions/UserHasNotPermissionError.ex +++ b/lib/bokken/exceptions/user_has_not_permission_error.ex @@ -1,3 +1,3 @@ defmodule Bokken.UserHasNotPermissionError do - defexception [:message, plug_status: 404] + defexception [:message, plug_status: 403] end diff --git a/test/bokken/events_test.exs b/test/bokken/events_test.exs index d1a5dff0..aa9db825 100644 --- a/test/bokken/events_test.exs +++ b/test/bokken/events_test.exs @@ -68,17 +68,13 @@ defmodule Bokken.EventsTest do valid_attrs = insert(:enrollment) event = Events.get_event!(valid_attrs.event_id) - try do + assert_raise Bokken.UserHasNotPermissionError, fn -> Events.guardian_create_enrollment( event, Ecto.UUID.generate(), valid_attrs.ninja_id, valid_attrs ) - rescue - e in Bokken.UserHasNotPermissionError -> - assert e.message == - "This ninja does not belong to the guardian, so enrollment cannot be modified." end end diff --git a/test/bokken_web/controllers/enrollment_controller_test.exs b/test/bokken_web/controllers/enrollment_controller_test.exs index 358f1159..674fabd1 100644 --- a/test/bokken_web/controllers/enrollment_controller_test.exs +++ b/test/bokken_web/controllers/enrollment_controller_test.exs @@ -88,8 +88,9 @@ defmodule BokkenWeb.EnrollmentControllerTest do enrollment: %{event_id: event.id, ninja_id: new_ninja.id, accepted: true} } - conn = post(conn, Routes.event_enrollment_path(conn, :create, event.id), enrollment_attrs) - assert json_response(conn, 403) + assert_error_sent 403, fn -> + post(conn, Routes.event_enrollment_path(conn, :create, event.id), enrollment_attrs) + end end end @@ -138,8 +139,9 @@ defmodule BokkenWeb.EnrollmentControllerTest do {:ok, enrollment} = Events.create_enrollment(event, :admin, enrollment_attrs) - conn = delete(conn, Routes.event_enrollment_path(conn, :delete, event.id, enrollment.id)) - assert json_response(conn, 403) + assert_error_sent 403, fn -> + delete(conn, Routes.event_enrollment_path(conn, :delete, event.id, enrollment.id)) + end end end From d90824d810128c4ce3403bd8a7a5b6f3040a6950 Mon Sep 17 00:00:00 2001 From: daniel_sp Date: Tue, 15 Aug 2023 22:44:56 +0100 Subject: [PATCH 5/5] Add suggested changes --- lib/bokken/events.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/bokken/events.ex b/lib/bokken/events.ex index d6929cc1..cdeeaca1 100644 --- a/lib/bokken/events.ex +++ b/lib/bokken/events.ex @@ -616,7 +616,7 @@ defmodule Bokken.Events do {:error, %Ecto.Changeset{}} """ - def create_enrollment(event, role \\ :admin, attrs \\ %{}) do + def create_enrollment(event, role \\ :guardian, attrs \\ %{}) do cur_time = DateTime.utc_now() ninja_id = Map.get(attrs, :ninja_id) || Map.get(attrs, "ninja_id")