Improve error rendering of validations

lib/code_corps/accounts/changesets.ex

@@ -3,6 +3,8 @@ defmodule CodeCorps.Accounts.Changesets do
   Changesets for Code Corps accounts.
   """
 
+  import CodeCorpsWeb.Gettext
+
   alias CodeCorps.GitHub.Adapters
   alias CodeCorps.Helpers.RandomIconColor
   alias Ecto.Changeset
@@ -18,7 +20,7 @@ defmodule CodeCorps.Accounts.Changesets do
     |> Changeset.validate_inclusion(:type, ["bot", "user"])
     |> RandomIconColor.generate_icon_color(:default_color)
     |> Changeset.unique_constraint(:email)
-    |> Changeset.unique_constraint(:github_id)
+    |> unique_github_constraint()
   end
 
   @doc ~S"""
@@ -31,7 +33,7 @@ defmodule CodeCorps.Accounts.Changesets do
     |> ensure_email_without_overwriting(params)
     |> Changeset.validate_required([:github_auth_token, :github_avatar_url, :github_id, :github_username, :type])
     |> Changeset.unique_constraint(:email)
-    |> Changeset.unique_constraint(:github_id)
+    |> unique_github_constraint()
   end
 
   @spec ensure_email_without_overwriting(Changeset.t, map) :: Changeset.t
@@ -42,4 +44,9 @@ defmodule CodeCorps.Accounts.Changesets do
     end
   end
   defp ensure_email_without_overwriting(%Changeset{} = changeset, _params), do: changeset
+
+  defp unique_github_constraint(struct) do
+    struct
+    |> Changeset.unique_constraint(:github_id, message: dgettext("errors", "account is already connected to someone else"))
+  end
 end

lib/code_corps/github/user.ex

@@ -33,6 +33,7 @@ defmodule CodeCorps.GitHub.User do
     do
        user |> do_connect(user_payload, access_token)
     else
+      {:ok, %{"error" => _} = error} -> handle_oauth_error(error)
       {:error, error} -> {:error, error}
     end
   end
@@ -54,4 +55,8 @@ defmodule CodeCorps.GitHub.User do
       {:error, error} -> {:error, error}
     end
   end
+
+  defp handle_oauth_error(%{"error_description" => message, "error_uri" => documentation_url}) do
+    {:error, GitHub.APIError.new({401, %{"message" => message, "documentation_url" => documentation_url}})}
+  end
 end

lib/code_corps_web/controllers/fallback_controller.ex

@@ -18,7 +18,7 @@ defmodule CodeCorpsWeb.FallbackController do
   def call(%Conn{} = conn, {:error, %Changeset{} = changeset}) do
     conn
     |> put_status(:unprocessable_entity)
-    |> render(:errors, data: changeset)
+    |> render(CodeCorpsWeb.ChangesetView, "422.json", changeset: changeset)
   end
   def call(%Conn{} = conn, {:error, :not_authorized}) do
     conn
@@ -41,4 +41,10 @@ defmodule CodeCorpsWeb.FallbackController do
     |> put_status(500)
     |> render(CodeCorpsWeb.ErrorView, "500.json", message: "An unknown error occurred with Stripe's API.")
   end
+  def call(%Conn{} = conn, {:error, %CodeCorps.GitHub.APIError{message: message}}) do
+    Logger.info message
+    conn
+    |> put_status(500)
+    |> render(CodeCorpsWeb.ErrorView, "github-error.json", message: message)
+  end
 end

lib/code_corps_web/controllers/user_controller.ex

@@ -49,11 +49,6 @@ defmodule CodeCorpsWeb.UserController do
     with {:ok, user} <- GitHub.User.connect(current_user, code, state)
     do
       conn |> render("show.json-api", data: user)
-    else
-      {:error, _error} ->
-        conn
-        |> put_status(:internal_server_error)
-        |> render(CodeCorpsWeb.ErrorView, "500.json-api")
     end
   end
 

lib/code_corps_web/views/changeset_view.ex

@@ -1,19 +1,23 @@
 defmodule CodeCorpsWeb.ChangesetView do
   use CodeCorpsWeb, :view
+  use JaSerializer.PhoenixView
+
+  import CodeCorpsWeb.Gettext
 
   alias Ecto.Changeset
+  alias JaSerializer.Formatter.Utils
 
   @doc """
   Traverses and translates changeset errors.
 
   See `Ecto.Changeset.traverse_errors/2` and
   `CodeCorpsWeb.ErrorHelpers.translate_error/1` for more details.
   """
-  def translate_errors(changeset) do
+  def translate_errors(%Ecto.Changeset{} = changeset) do
     errors =
       changeset
       |> Changeset.traverse_errors(&translate_error/1)
-      |> format_errors
+      |> format_errors()
     errors
   end
 
@@ -32,18 +36,25 @@ defmodule CodeCorpsWeb.ChangesetView do
 
   def create_error(attribute, message) do
     %{
-      id: "VALIDATION_ERROR",
+      detail: format_detail(attribute, message),
+      title: message,
       source: %{
         pointer: "data/attributes/#{attribute}"
       },
-      detail: message,
-      status: 422
+      status: "422"
     }
   end
 
-  def render("error.json-api", %{changeset: changeset}) do
+  def render("422.json", %{changeset: changeset}) do
     # When encoded, the changeset returns its errors
     # as a JSON object. So we just pass it forward.
     %{errors: translate_errors(changeset)}
   end
+
+  defp format_detail(attribute, message) do
+    "#{attribute |> Utils.humanize |> translate_attribute} #{message}"
+  end
+
+  defp translate_attribute("Github"), do: dgettext("errors", "Github")
+  defp translate_attribute(attribute), do: attribute
 end

lib/code_corps_web/views/error_view.ex

@@ -4,27 +4,36 @@ defmodule CodeCorpsWeb.ErrorView do
 
   def render("stripe-400.json-api", _assigns) do
     %{
-      id: "INVALID_GRANT",
       title: "This authorization code has already been used. All tokens issued with this code have been revoked.",
-      status: 400
+      detail: "This authorization code has already been used. All tokens issued with this code have been revoked.",
+      status: "400"
     }
     |> JaSerializer.ErrorSerializer.format
   end
 
   def render("404.json-api", _assigns) do
     %{
-      id: "NOT_FOUND",
       title: "404 Resource not found",
-      status: 404
+      detail: "404 Resource not found",
+      status: "404"
     }
     |> JaSerializer.ErrorSerializer.format
   end
 
   def render("500.json-api", _assigns) do
     %{
-      id: "INTERNAL_SERVER_ERROR",
       title: "500 Internal server error",
-      status: 500
+      detail: "500 Internal server error",
+      status: "500"
+    }
+    |> JaSerializer.ErrorSerializer.format
+  end
+
+  def render("github-error.json", %{message: message}) do
+    %{
+      title: "GitHub API error",
+      detail: message,
+      status: "500"
     }
     |> JaSerializer.ErrorSerializer.format
   end

priv/gettext/default.pot

@@ -0,0 +1,15 @@
+## This file is a PO Template file.
+##
+## `msgid`s here are often extracted from source code.
+## Add new translations manually only if they're dynamic
+## translations that can't be statically extracted.
+##
+## Run `mix gettext.extract` to bring this file up to
+## date. Leave `msgstr`s empty as changing them here as no
+## effect: edit them in PO (`.po`) files instead.
+msgid ""
+msgstr ""
+
+#: lib/code_corps_web/templates/page/index.html.eex:2
+msgid "Welcome to %{name}"
+msgstr ""

priv/gettext/en/LC_MESSAGES/default.po

@@ -0,0 +1,15 @@
+## `msgid`s in this file come from POT (.pot) files.
+##
+## Do not add, change, or remove `msgid`s manually here as
+## they're tied to the ones in the corresponding POT file
+## (with the same domain).
+##
+## Use `mix gettext.extract --merge` or `mix gettext.merge`
+## to merge POT files into PO files.
+msgid ""
+msgstr ""
+"Language: en\n"
+
+#: lib/code_corps_web/templates/page/index.html.eex:2
+msgid "Welcome to %{name}"
+msgstr ""

priv/gettext/en/LC_MESSAGES/errors.po

@@ -22,6 +22,10 @@ msgstr ""
 msgid "is invalid"
 msgstr ""
 
+## From Ecto.Changeset.validate_acceptance/3
+msgid "must be accepted"
+msgstr ""
+
 ## From Ecto.Changeset.validate_format/3
 msgid "has invalid format"
 msgstr ""
@@ -39,10 +43,10 @@ msgid "does not match confirmation"
 msgstr ""
 
 ## From Ecto.Changeset.no_assoc_constraint/3
-msgid "is still associated to this entry"
+msgid "is still associated with this entry"
 msgstr ""
 
-msgid "are still associated to this entry"
+msgid "are still associated with this entry"
 msgstr ""
 
 ## From Ecto.Changeset.validate_length/3
@@ -91,3 +95,11 @@ msgstr ""
 
 msgid "must be equal to %{number}"
 msgstr ""
+
+#: lib/code_corps/accounts/changesets.ex:50
+msgid "account is already connected to someone else"
+msgstr ""
+
+#: lib/code_corps_web/views/changeset_view.ex:54
+msgid "Github"
+msgstr "GitHub"

priv/gettext/errors.pot

@@ -7,7 +7,6 @@
 ## Run `mix gettext.extract` to bring this file up to
 ## date. Leave `msgstr`s empty as changing them here as no
 ## effect: edit them in PO (`.po`) files instead.
-
 ## From Ecto.Changeset.cast/4
 msgid "can't be blank"
 msgstr ""
@@ -20,6 +19,10 @@ msgstr ""
 msgid "is invalid"
 msgstr ""
 
+## From Ecto.Changeset.validate_acceptance/3
+msgid "must be accepted"
+msgstr ""
+
 ## From Ecto.Changeset.validate_format/3
 msgid "has invalid format"
 msgstr ""
@@ -37,10 +40,10 @@ msgid "does not match confirmation"
 msgstr ""
 
 ## From Ecto.Changeset.no_assoc_constraint/3
-msgid "is still associated to this entry"
+msgid "is still associated with this entry"
 msgstr ""
 
-msgid "are still associated to this entry"
+msgid "are still associated with this entry"
 msgstr ""
 
 ## From Ecto.Changeset.validate_length/3
@@ -89,3 +92,11 @@ msgstr ""
 
 msgid "must be equal to %{number}"
 msgstr ""
+
+#: lib/code_corps/accounts/changesets.ex:50
+msgid "account is already connected to someone else"
+msgstr ""
+
+#: lib/code_corps_web/views/changeset_view.ex:54
+msgid "Github"
+msgstr ""

test/lib/code_corps/accounts/accounts_test.exs

@@ -37,6 +37,21 @@ defmodule CodeCorps.AccountsTest do
       assert changeset.errors[:email] == {"has already been taken", []}
     end
 
+    test "validates the uniqueness of the github_id" do
+      %{"id" => github_id} = payload = TestHelpers.load_endpoint_fixture("user")
+
+      # Ensure a user exists so there's a duplicate github_id
+      insert(:user, github_id: github_id)
+
+      {:error, %Changeset{} = changeset} =
+        payload
+        |> Accounts.create_from_github
+
+      wait_for_supervisor()
+
+      assert changeset.errors[:github_id] == {"account is already connected to another user on Code Corps.", []}
+    end
+
     test "uploads photo from GitHub avatar" do
       {:ok, %User{} = user} =
         "user"

test/lib/code_corps_web/views/changeset_view_test.exs

@@ -6,27 +6,28 @@ defmodule CodeCorpsWeb.ChangesetViewTest do
   test "renders all errors properly" do
     changeset = Preview.create_changeset(%Preview{}, %{})
 
-    rendered_json = render(CodeCorpsWeb.ChangesetView, "error.json-api", %{changeset: changeset})
+    rendered_json = render(CodeCorpsWeb.ChangesetView, "422.json", %{changeset: changeset})
 
     expected_json = %{
-      errors: [
+      "errors" => [
         %{
-          id: "VALIDATION_ERROR",
-          detail: "can't be blank",
+          detail: "Markdown can't be blank",
           source: %{
-            pointer: "data/attributes/markdown"
+            pointer: "/data/attributes/markdown"
           },
-          status: 422
+          status: "422",
+          title: "can't be blank"
         },
         %{
-          id: "VALIDATION_ERROR",
-          detail: "can't be blank",
+          detail: "User can't be blank",
           source: %{
-            pointer: "data/attributes/user_id"
+            pointer: "/data/relationships/user"
           },
-          status: 422
+          status: "422",
+          title: "can't be blank"
         }
-      ]
+      ],
+      "jsonapi" => %{"version" => "1.0"}
     }
 
     assert rendered_json == expected_json