Rendering form errors for non-database-backed changesets

introduction.md

Introduction

Disclaimer: I'm a one-day-old Elixir/Phoenix developer.

In Sonny's training yesterday, we used an Ecto.Changeset to handle user registration (a database-backed operation). This worked perfectly for rendering the intial form and re-rendering the form with validation errors.

This is because our Workshop.RegistrationController is calling Repo.Insert which sets the changeset's :action, whether or not the insertion succeeds. Then Phoenix.HTML's form_for function appropriately sets errors on the form so they can be rendered on the page.

The Problem

In contrast, the process of session creation does not trigger a database insert or update, so using the same pattern does not work out of the box as a developer might expect.

Below are the model, view, and controller for session creation, adapted from Sonny's example application. See my fork.

You'll see that the registration and session controllers are very similar, except for the Repo.Insert. However, if you spin up the application, you'll see that the registration process renders inline errors properly while the login process renders none of the inline error tags.

This is because form_for only sets errors on the form if the changeset's :action has been set, which is not the case for our non-database-backed login process.

The Solution?

You may also notice that the new and create actions in my controllers are rendering different changesets. The new action uses the new_changeset function and the create action uses the validated_changeset function. I think this distinction is important for a couple of reasons:

1. There's no need to run validations on a changeset when you're only rendering a form.
2. The fact that the changeset has been validated and has errors should be the only condition controlling whether the form renders those errors.

I think it would benefit the Phoenix community to adopt the pattern of using different changesets between non-validated and validated actions. It's more performant, plus there would no longer be any need to switch form_for's error handling behavior on the presence of the changeset's :action (introduced by this commit).

This would be a breaking change but I think it's an important one. Simply making the change to Phoenix.HTML's form/error handling (to not switch on :action) would encourage developers to do The Right Thing™ in terms of using validated or non-validated changesets in their controllers.

I'd love to get feedback on this idea! Thank you!

login.ex

defmodule Workshop.Login do
  use Workshop.Web, :model

  embedded_schema do
    field :email, :string
    field :password, :string
  end

  @allowed_fields ~w(email password)a
  @required_fields @allowed_fields

  def new_changeset(login, params \\ %{}) do
    login
    |> cast(params, @allowed_fields)
  end

  def validated_changeset(login, params) do
    new_changeset(login, params)
    |> validate_required(@required_fields)
    |> update_change(:email, &String.downcase/1)
    |> validate_format(:email, ~r/@/)
    |> check_password()
  end

  defp check_password(%{valid?: false} = changeset), do: changeset
  defp check_password(%{changes: %{email: email, password: password}} = changeset) do
    user = Workshop.Repo.get_by(Workshop.User, email: email)

    if user do
      if Comeonin.Bcrypt.checkpw(password, user.hashed_password) do
        put_change changeset, :user_id, user.id
      else
        add_error changeset, :password, "is incorrect"
      end
    else
      add_error changeset, :email, "cannot be found"
    end
  end
end

new.html.eex

<h2>Login</h2>

<%= form_for @changeset, session_path(@conn, :create), fn form -> %>

  <div class="form-group">
    <%= label form, :email %>
    <%= email_input form, :email, class: "form-control" %>
    <%= error_tag form, :email %>
  </div>
  <div class="form-group">
    <%= label form, :password %>
    <%= password_input form, :password, class: "form-control" %>
    <%= error_tag form, :password %>
  </div>

  <%= submit "Submit", class: "btn btn-primary" %>
<% end %>

session_controller.ex

defmodule Workshop.SessionController do
  use Workshop.Web, :controller

  alias Workshop.Login

  def new(conn, _params) do
    changeset = Login.new_changeset(%Login{})

    render conn, "new.html", changeset: changeset
  end

  def create(conn, %{"login" => login_params}) do
    changeset = Login.validated_changeset(%Login{}, login_params)

    if changeset.valid? do
      conn
      |> put_flash(:info, "Welcome back!")
      |> put_session(:current_user_id, changeset.changes.user_id)
      |> redirect(to: page_path(conn, :index))
    else
      conn
      |> put_flash(:error, "Sorry, try again.")
      |> render("new.html", changeset: changeset)
    end
  end

  def delete(conn, _params) do
    conn
    |> put_session(:current_user_id, nil)
    |> redirect(to: page_path(conn, :index))
  end
end

I completely agree with you. Try to open an Issue with this point of view, I will support you.