Standardize errors in enrollment controller (#183)

lib/bokken/events.ex

@@ -609,28 +609,26 @@ 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 \\ :guardian, 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()
+        insert_enrollment(role, attrs)
       end
     else
-      {:error, "Enrollments are closed"}
+      {:error, :enrollments_closed}
     end
   end
 
@@ -640,6 +638,44 @@ 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
+
+  defp insert_enrollment(_, _attrs) do
+    {:error, :invalid_role}
+  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(event, guardian_id, ninja_id, %{field: bad_value})
+      {:error, %Ecto.Changeset{}}
+  """
+  def guardian_create_enrollment(event, guardian_id, ninja_id, attrs \\ %{}) do
+    ninja = Accounts.get_ninja!(ninja_id, [: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 """
   Updates an enrollment.
 
@@ -681,6 +717,28 @@ defmodule Bokken.Events do
     end
   end
 
+  @doc """
+  Deletes 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{}}
+  """
+  def guardian_delete_enrollment(enrollment, guardian_id, ninja_id) do
+    ninja = Accounts.get_ninja!(ninja_id, [: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 """
   Returns an `%Ecto.Changeset{}` for tracking enrollment changes.
 

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
 
@@ -28,4 +30,15 @@ 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: gettext("Guardian cannot submit an accepted enrollment")
+    )
+    |> assoc_constraint(:event)
+    |> assoc_constraint(:ninja)
+  end
 end

lib/bokken/exceptions/user_has_not_permission_error.ex

@@ -0,0 +1,3 @@
+defmodule Bokken.UserHasNotPermissionError do
+  defexception [:message, plug_status: 403]
+end

lib/bokken/guards.ex

@@ -29,5 +29,12 @@ 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
+                ]
 end

lib/bokken_web/controllers/enrollment_controller.ex

@@ -1,7 +1,6 @@
 defmodule BokkenWeb.EnrollmentController do
   use BokkenWeb, controller: "1.6"
 
-  alias Bokken.Accounts
   alias Bokken.Events
   alias Bokken.Events.Enrollment
 
@@ -40,59 +39,36 @@ 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
     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 +83,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

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

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

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

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,32 @@ 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
+      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
+
+    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_raise Bokken.UserHasNotPermissionError, fn ->
+        Events.guardian_create_enrollment(
+          event,
+          Ecto.UUID.generate(),
+          valid_attrs.ninja_id,
+          valid_attrs
+        )
+      end
+    end
+
     test "update_enrollment/2 updates existing enrollment" do
       enrollment = insert(:enrollment)