Goals:

Up until now, I have not touched upon testing of Pharchive, today that changes! We will shore up Pharchive with tests, and be ready to to build new features using TDD.

An aside on TDD

TDD has never truely resonated with me. I know that there are many many arguments as to how writing tests first can help guide the design of your application; and I think that those arguments are correct! For me, it has always been a balance between throwing code at something to see if an idea is viable(as quickly as possible), then using TDD to guide refactoring. In the case of Pharchive, because so much of the current functionality is provided by Phoenix generated code, I decided that my viable stopping point was “Can I create collections and their associated parent records in sane way”.

This is 100% a personal preference, and I can say freely that I am probably wrong in that I don’t “TDD all the things all the time”.

Requirements

• An hour of your time
• Having read/worked through parts one and two

Content

Since we have ignored testing up to this point, this is a great point to take a step back and ensure that all of our auto generated tests are passing.

Starting with the page_controller we change the test string to reflect the changes to our index (we moved from the default index to using CollectionController#index).

file: test/controllers/page_controller_test.exs

defmodule Pharchive.PageControllerTest do
 
   test "GET /", %{conn: conn} do
     conn = get conn, "/"
     assert html_response(conn, 200) =~ "New collection"
   end
 end

Our next error comes from the use of Integer.parse. There exist cases where params["film_id"] wont exist so we are trying to parse a nil value, this of course does not work. I decided to fix this in the bluntest way possible, guarding with a conditional, because I know we are going to have to do this same process in our other two controllers, and this is about the time that I start looking for shared behavior and think about extraction. Notice that we move our previous conditional into our else clause. This is not ideal, but lets keep running with it until all our tests are passing.

file: web/controllers/collection_controller.ex

def create(conn, %{"collection" => params}) do
  if !params["film_id"] do
    changeset = Collection.changeset(%Collection{}, Dict.delete(params, "film_id"))
  else
   {film_id, _} = Integer.parse(params["film_id"])

   if film_id == -1 do
     changeset = Collection.changeset(%Collection{}, Dict.delete(params, "film_id"))
   else
     changeset = Repo.get!(Film, params["film_id"])
      |> Ecto.build_assoc(:collections)
      |> Collection.changeset(params)
   end
  end
end

The next error is a little esoteric. We have a failing truthy assertion. I figured that this is likely because we looking up a single record in too specific a manner. The autogenerated code says: "give me the collection whos attributes look exactly like the @valid_attrs map. Loosening this constraint to ensure that we are doing a lookup only based on the physical_id solves this quickly.

file: test/controllers/collection_controller_test.exs

defmodule Pharchive.CollectionControllerTest do
   use Pharchive.ConnCase
 
   alias Pharchive.Collection
   @valid_attrs %{description: "some content",
                  frame_count: 42,
                  frame_size: "some content",
                  location: "some content",
                  physical_id: 42,
                  taken_at: "2010-04-17 14:00:00",
                  uuid: "7488a646-e31f-11e4-aace-600308960662"
                 }
   @invalid_attrs %{}
 
   test "creates resource and redirects when data is valid", %{conn: conn} do
     conn = post conn, collection_path(conn, :create), collection: @valid_attrs
     assert redirected_to(conn) == collection_path(conn, :index)
     assert Repo.get_by(Collection, %{physical_id: @valid_attrs[:physical_id]})
   end
 
   test "updates chosen resource and redirects when data is valid", %{conn: conn} do
     collection = Repo.insert! %Collection{}
     conn = put conn, collection_path(conn, :update, collection), collection: @valid_attrs
     assert redirected_to(conn) == collection_path(conn, :show, collection)
     assert Repo.get_by(Collection, %{physical_id: @valid_attrs[:physical_id]})
   end

Next we fix the integer parse error in our FilmController in the same way as we just did in the CollectionController. Hey, this is starting to get repetitive!

file: web/controllers/film_controller.ex

def create(conn, %{"film" => film_params}) do
  if !film_params["manufacturer_id"] do
    changeset = Film.changeset(%Film{}, Dict.delete(film_params, "manufacturer_id"))
  else
    {manufacturer_id, _} = Integer.parse(film_params["manufacturer_id"])

    if manufacturer_id == -1 do
      changeset = Film.changeset(%Film{}, Dict.delete(film_params, "manufacturer_id"))
    else
      changeset = Repo.get!(Manufacturer, film_params["manufacturer_id"])
      |> Ecto.build_assoc(:films)
      |> Film.changeset(film_params)
    end
  end
  ...
end

Next we need to make sure that the :collection_id is always present in a form for Film. So I decided to put a conn = assign(conn, :collection_id, "") in the three places that need to have it assigned: the case of validation errors on create, the case of editing a film, and the case of validation errors on update.

file: web/controllers/film_controller.ex

# case of error in create
    {:error, changeset} ->
      conn = assign(conn, :collection_id, "")
      render(conn, "new.html", changeset: changeset)

...
 
# case of edit
    def edit(conn, %{"id" => id}) do
      film = Repo.get!(Film, id)
      conn = assign(conn, :collection_id, "")
      changeset = Film.changeset(film)
      render(conn, "edit.html", film: film, changeset: changeset)
    end

...

# case of error in update
    {:error, changeset} ->
      conn = assign(conn, :collection_id, "")
      render(conn, "edit.html", film: film, changeset: changeset)

...

Next we have to ensure that the :collection_id is available in the film edit form. Somehow I missed this last time!

file: web/templates/film/edit.html.eex

 <%= render "form.html", changeset: @changeset,
                         manufacturers: @manufacturers,
                         collection_id: @collection_id,
                         action: film_path(@conn, :update, @film) %>
 
 <%= link "Back", to: film_path(@conn, :index) %>

Now we can move on to the ManufacturerController which is a bit simpler. We only need to ensure that film_id is always present. We have the same cases as in the FilmController

film: web/controllers/manufacturer_controller.ex

# case of error in create
{:error, changeset} ->
  conn = assign(conn, :film_id, "")
  render(conn, "new.html", changeset: changeset)

# case of error in edit
 
def edit(conn, %{"id" => id}) do
  manufacturer = Repo.get!(Manufacturer, id)
  conn = assign(conn, :film_id, "")
  changeset = Manufacturer.changeset(manufacturer)
  render(conn, "edit.html", manufacturer: manufacturer, changeset: changeset)
end

# case of error in update
{:error, changeset} ->
  conn = assign(conn, :film_id, "")
  render(conn, "edit.html", manufacturer: manufacturer, changeset: changeset)

Now we make sure that :film_id is available in the edit form.

film: web/templates/manufacturer/edit.html.eex

<h2>Edit manufacturer</h2>
 
<%= render "form.html", changeset: @changeset,
                        film_id: @film_id,
                        action: manufacturer_path(@conn, :update, @manufacturer)
%>
 
<%= link "Back", to: manufacturer_path(@conn, :index) %>

As a small refactor, I would like to see get parameter names be homogeneous. Let’s change the get parameter on the “film created successfully” branch to be called film_id rather than film.

file: web/controllers/film_controller.ex

...
  if manufacturer_id == -1 do
    conn
    |> put_flash(:info, "Film created successfully. Please build the manufacturer")
    |> redirect(to: manufacturer_path(conn, :new, film_id: film.id))
  else
...     

Lets apply the same change to the CollectionController as in the Manufacturer redirect.

file: web/controllers/collection_controller.ex

...
  if film_id == -1 do
    conn
    |> put_flash(:info, "Collection created successfully. Please build the Film")
    |> redirect(to: film_path(conn, :new, collection_id: collection.id))
  else
...

It seems rather silly that we have all of these assigns everywhere doesnt it? Lets use a controller level plug and set a default where we need to. In lovely fashion, Map#get takes an optional third parameter which acts as a default! This means that in the case where film_id is present its value is preserved, and in the case where film_id is not present, it is initialized to a default. Im going to display the full patch diff here because I think its easier to follow. In a quick pre summary, we add a plug to handle the assignment, then we remove all of the conn = assign(conn, :film_id, film_id) that we added in our last step.

file: web/controllers/manufacturer_controller.ex

defmodule Pharchive.ManufacturerController do
   alias Pharchive.Film
 
   plug :scrub_params, "manufacturer" when action in [:create, :update]
+  plug :default_creation_connection_variables when action in [:new, :edit, :create, :update]
+
+  def default_creation_connection_variables(conn, _) do
+    assign(conn, :film_id, Map.get(conn.params, "film_id", ""))
+  end
 
   def index(conn, _params) do
     manufacturers = Repo.all(Manufacturer)
     render(conn, "index.html", manufacturers: manufacturers)
   end
 
-  def new(conn, %{"film" => film_id}) do
-    changeset = Manufacturer.changeset(%Manufacturer{})
-    conn = assign(conn, :film_id, film_id)
-    render(conn, "new.html", changeset: changeset)
-  end
-
   def new(conn, _params) do
     changeset = Manufacturer.changeset(%Manufacturer{})
-    conn = assign(conn, :film_id, "")
     render(conn, "new.html", changeset: changeset)
   end
 
@@ -37,7 +35,6 @@ defmodule Pharchive.ManufacturerController do
         |> put_flash(:info, "Manufacturer created successfully.")
         |> redirect(to: manufacturer_path(conn, :index))
       {:error, changeset} ->
-        conn = assign(conn, :film_id, "")
         render(conn, "new.html", changeset: changeset)
     end
   end
@@ -49,7 +46,6 @@ defmodule Pharchive.ManufacturerController do
 
   def edit(conn, %{"id" => id}) do
     manufacturer = Repo.get!(Manufacturer, id)
-    conn = assign(conn, :film_id, "")
     changeset = Manufacturer.changeset(manufacturer)
     render(conn, "edit.html", manufacturer: manufacturer, changeset: changeset)
   end
@@ -64,7 +60,6 @@ defmodule Pharchive.ManufacturerController do
         |> put_flash(:info, "Manufacturer updated successfully.")
         |> redirect(to: manufacturer_path(conn, :show, manufacturer))
       {:error, changeset} ->
-        conn = assign(conn, :film_id, "")
         render(conn, "edit.html", manufacturer: manufacturer, changeset: changeset)
     end
   end

Since the plug strategy worked so well for the ManufacturerController, lets do the same thing to the FilmController.

file: /web/controllers/film_controller.ex

defmodule Pharchive.FilmController do
 
   plug :scrub_params, "film" when action in [:create, :update]
   plug :load_manufacturers when action in [:new, :create, :edit, :update]
+  plug :default_creation_connection_variables when action in [:new, :edit, :create, :update]
+
+  def default_creation_connection_variables(conn, _) do
+    assign(conn, :collection_id, Map.get(conn.params, "collection_id", ""))
+  end
 
   def load_manufacturers(conn, _) do
     manufacturers = Repo.all from(m in Pharchive.Manufacturer,
@@ -26,13 +31,11 @@ defmodule Pharchive.FilmController do
 
   def new(conn, %{"collection" => collection_id}) do
     changeset = Film.changeset(%Film{})
-    conn = assign(conn, :collection_id, collection_id)
     render(conn, "new.html", changeset: changeset)
   end
 
   def new(conn, _params) do
     changeset = Film.changeset(%Film{})
-    conn = assign(conn, :collection_id, "")
     render(conn, "new.html", changeset: changeset)
   end
 
@@ -69,7 +72,6 @@ defmodule Pharchive.FilmController do
           |> redirect(to: film_path(conn, :index))
         end
       {:error, changeset} ->
-        conn = assign(conn, :collection_id, "")
         render(conn, "new.html", changeset: changeset)
     end
   end
@@ -81,7 +83,6 @@ defmodule Pharchive.FilmController do
 
   def edit(conn, %{"id" => id}) do
     film = Repo.get!(Film, id)
-    conn = assign(conn, :collection_id, "")
     changeset = Film.changeset(film)
     render(conn, "edit.html", film: film, changeset: changeset)
   end
@@ -96,7 +97,6 @@ defmodule Pharchive.FilmController do
         |> put_flash(:info, "Film updated successfully.")
         |> redirect(to: film_path(conn, :show, film))
       {:error, changeset} ->
-        conn = assign(conn, :collection_id, "")
         render(conn, "edit.html", film: film, changeset: changeset)
     end
   end

The test suite is giving me some flak for unsafe variables because we are defining film_id and changeset within conditional statements (thanks Elixir, good catch!) in all three controllers. So we should rewrite these

file: web/controllers/collection_controller.ex

def create(conn, %{"collection" => params}) do
  film_id = 0
  changeset = if !params["film_id"] do
    Collection.changeset(%Collection{}, Dict.delete(params, "film_id"))
  else
    {film_id, _} = Integer.parse(params["film_id"])

    if film_id == -1 do
     Collection.changeset(%Collection{}, Dict.delete(params, "film_id"))
    else
      Repo.get!(Film, params["film_id"])
      |> Ecto.build_assoc(:collections)
      |> Collection.changeset(params)
    end
  ...
  end
...
end

file: web/controllers/film_controller.ex

def create(conn, %{"film" => film_params}) do
  manufacturer_id = 0
  changeset = if !film_params["manufacturer_id"] do
    Film.changeset(%Film{}, Map.delete(film_params, "manufacturer_id"))
  else
    {manufacturer_id, _} = Integer.parse(film_params["manufacturer_id"])

    if manufacturer_id == -1 do
     Film.changeset(%Film{}, Map.delete(film_params, "manufacturer_id"))
    else
      Repo.get!(Manufacturer, film_params["manufacturer_id"])
      |> Ecto.build_assoc(:films)
      |> Film.changeset(film_params)
    end
  ...
  end
...
end

Dict was recently depricated with Elixir 1.2, so we should stop using it (sorry about the churn!). We will just replace occurences of Dict with Map.

I was not pleased with the creation logic related to our “create parent” flag. This is a change which makes things easier to follow, and avoids uneccessary integer parsing. I remove a level of conditionals by setting an initial value, and changing that value if certain conditions are met.

file: web/controllers/collection_controller.ex

def create(conn, %{"collection" => params}) do
  build_parent? = Map.get(params, "film_id", false) == "-1"
  changeset = Collection.changeset(%Collection{}, Map.delete(params, "film_id"))
  flash_message = "Collection created successfully."
  redirect = collection_path(conn, :index)

  if Map.has_key?(params, "film_id") do
    changeset = Repo.get!(Film, params["film_id"])
    |> Ecto.build_assoc(:collections)
    |> Collection.changeset(params)
  end

  case Repo.insert(changeset) do
    {:ok, collection} ->
      if build_parent? do
        flash_message = "Collection created successfully. Please build the Film"
        redirect = film_path(conn, :new, collection_id: collection.id)
      end
      conn
      |> put_flash(:info, flash_message)
      |> redirect(to: redirect)

    ...
  end
...
end

Since our refactor went so well for collections, we should apply it to the FilmController

film: web/controllers/film_controller.ex

def create(conn, %{"film" => film_params}) do
  build_parent? = Map.get(film_params, "manufacturer_id", false) == "-1"
  changeset = Film.changeset(%Film{}, Map.delete(film_params, "manufacturer_id"))

  flash_message = "Film created successfully."
  redirect = film_path(conn, :index)

  if Map.has_key?(film_params, "manufacturer_id") do
    changeset = Repo.get!(Manufacturer, film_params["manufacturer_id"])
    |> Ecto.build_assoc(:films)
    |> Film.changeset(film_params)
  end

  case Repo.insert(changeset) do
    ...
      if build_parent? do
        flash_message = "Film created successfully. Please build the manufacturer"
        redirect = manufacturer_path(conn, :new, film_id: film.id)
      else
        conn
        |> put_flash(:info, flash_message)
        |> redirect(to: redirect)
      end
    ...
  end
...
end

And then finally the CollectionController

file: web/controllers/collection_controller.ex

...
     if !build_parent? && Map.has_key?(params, "film_id") do
       changeset = Repo.get!(Film, params["film_id"])
       |> Ecto.build_assoc(:collections)
       |> Collection.changeset(params)
    end
...
    if !build_parent? && Map.has_key?(film_params, "manufacturer_id") do
    ...
    end
...

Now we have fixed our failing tests and refactored some of our code to be better! Now lets use ExMachina to setup some model factories to make testing easier. Model factories are very useful for testing basic suppositions of model logic, and DRYing up repetitive tests. Following the directions from the ExMachina README, we add the :ex_machina application to our application list in mix.es as well as adding it as a dependency.

file: mix.exs

...
   def application do
     [mod: {Pharchive, []},
      applications: [:phoenix, :phoenix_html, :cowboy, :logger, :gettext,
                     :phoenix_ecto, :postgrex, :ex_machina]]
   end

...
   defp deps do
     [{:ex_machina, "~>0.6.1", only: :test},
      {:phoenix, "~> 1.1.2"},
      {:phoenix_ecto, "~> 2.0"},
      {:postgrex, ">= 0.0.0"},
      {:phoenix_html, "~> 2.3.1"},

We also need to update our test helper to make sure that ExMachina will run on test.

file: test/test_helper.exs

{:ok, _} = Application.ensure_all_started(:ex_machina)

After a quick mix deps.get, we can move on to declaring our model factories. The nice thing about ExMachina is that it supports relations and generated attributes (sequences of unique emails for instance)

file: test/support/factory.ex

defmodule Pharchive.Factory do
  use ExMachina.Ecto, repo: Pharchive.Repo

  def factory(:manufacturer) do
    %Pharchive.Manufacturer{
      name: "Kodak",
    }
  end

  def factory(:film) do
    %Pharchive.Film{
      speed: trunc(Float.floor(:random.uniform * 1000)),
      name: "FOBAZ",
      short_name: "FBZ",
      manufacturer: build(:manufacturer),
    }
  end

  def factory(:collection) do
    {:ok, date} = Ecto.Date.load({2015, 1, 1})
    %Pharchive.Collection{
      physical_id: 1,
      uuid: Ecto.UUID.generate(),
      location: "home",
      frame_count: 36,
      description: "The dude abides",
      taken_at: date,
      frame_size: "645",
      film: build(:film)
    }
  end
end

Now that the factories are declared, we are going to add a test to the CollectionController for the case of “creates Collection and associates Film when asked, redirects when data is valid”.

File: test/controllers/collection_controller_test.exs

 defmodule Pharchive.CollectionControllerTest do
   use Pharchive.ConnCase
   import Pharchive.Factory
 
   alias Pharchive.Film
   alias Pharchive.Collection
... 

test "creates Collection and associates Film when asked, redirects when data is valid", %{conn: conn} do
  film = create(:film)
  with_collection_params = %{description: "some content",
                             frame_count: 42,
                             frame_size: "some content",
                             location: "some content",
                             physical_id: 42,
                             taken_at: "2010-04-17 14:00:00",
                             uuid: "7488a646-e31f-11e4-aace-600308960662",
                             film_id: "#{film.id}"}
  conn = post conn, collection_path(conn, :create), collection: with_collection_params
  assert redirected_to(conn) == collection_path(conn, :index)
  f = Repo.get_by(Film, %{id: film.id})
  assert Repo.get_by(Collection, %{film_id: f.id})
end

We will add a similar test for the FilmController which tests two cases:

• “creates Film and associates Manufacturer when asked, redirects when data is valid”
• “creates Film and associates Collection when asked, redirects when data is valid”

file: test/controllers/film_controller_test.exs

 defmodule Pharchive.FilmControllerTest do
   use Pharchive.ConnCase
   import Pharchive.Factory
 
   alias Pharchive.Film
   alias Pharchive.Collection
   alias Pharchive.Manufacturer

...

test "creates Film and associates Manufacturer when asked, redirects when data is valid", %{conn: conn} do
  manufacturer = create(:manufacturer)
  with_film_params = %{name: "foobar", short_name: "some content", speed: 42, manufacturer_id: "#{manufacturer.id}"}
  conn = post conn, film_path(conn, :create), film: with_film_params
  assert redirected_to(conn) == film_path(conn, :index)
  m = Repo.get_by(Manufacturer, %{id: manufacturer.id})
  assert Repo.get_by(Film, %{manufacturer_id: m.id})
end

test "creates Film and associates Collection when asked, redirects when data is valid", %{conn: conn} do
  collection = create(:collection)
  with_film_params = %{name: "foobar", short_name: "some content", speed: 42, collection_id: "#{collection.id}"}
  conn = post conn, film_path(conn, :create), film: with_film_params
  assert redirected_to(conn) == film_path(conn, :index)
  f = Repo.get_by(Film, %{name: "foobar"})
  assert Repo.get_by(Collection, %{film_id: f.id})
end

Finally we will add a test case for the ManufacturerController for “creates manufacturer and associates film when asked, redirects when data is valid”

file: test/controllers/manufacturer_controller_test.exs

 defmodule Pharchive.ManufacturerControllerTest do
   use Pharchive.ConnCase
   import Pharchive.Factory
 
   alias Pharchive.Manufacturer
   alias Pharchive.Film

...
 
test "creates manufacturer and associates film when asked, redirects when data is valid", %{conn: conn} do
  film = create(:film)
  with_film_params = %{name: "foobar", film_id: "#{film.id}"}
  conn = post conn, manufacturer_path(conn, :create), manufacturer: with_film_params
  assert redirected_to(conn) == manufacturer_path(conn, :index)
  m = Repo.get_by(Manufacturer, %{name: "foobar"})
  assert Repo.get_by(Film, %{manufacturer_id: m.id})
end

Conclusion

This has been a pretty gnarly step with an unfortunate amount of churn. Since this is an application that I am actually using day to day; I am glad to find issues like this early rather than later! I have tagged the code related to this post here

Next time we will be adding a user model and authentication!