Skip to content

Refactoring Project Category Controller Tests #420

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 3, 2016
Merged

Refactoring Project Category Controller Tests #420

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 35 additions & 101 deletions test/controllers/project_category_controller_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,47 +1,25 @@
defmodule CodeCorps.ProjectCategoryControllerTest do
use CodeCorps.ApiCase

alias CodeCorps.Category
alias CodeCorps.Project
alias CodeCorps.ProjectCategory
alias CodeCorps.Repo

@attrs %{}

defp build_payload, do: %{ "data" => %{"type" => "project-category", "attributes" => %{}}}
use CodeCorps.ApiCase, resource_name: :project_category

describe "index" do
test "lists all entries on index", %{conn: conn} do
conn = get conn, project_category_path(conn, :index)
assert json_response(conn, 200)["data"] == []
[project_category_1, project_category_2] = insert_pair(:project_category)

conn
|> request_index
|> json_response(200)
|> assert_ids_from_response([project_category_1.id, project_category_2.id])
end

test "filters resources on index", %{conn: conn} do
arts = insert(:category, name: "Arts")
society = insert(:category, name: "Society")
technology = insert(:category, name: "Technology")
[project_category_1, project_category_2 | _] = insert_list(3, :project_category)

project = insert(:project)
project_category_1 = insert(:project_category, project: project, category: arts)
project_category_2 = insert(:project_category, project: project, category: society)
insert(:project_category, project: project, category: technology)

response =
conn
|> get("project-categories/?filter[id]=#{project_category_1.id},#{project_category_2.id}")
|> json_response(200)

[first_result, second_result] = response |> Map.get("data")

first_result
|> assert_result_id(project_category_1.id)
|> assert_jsonapi_relationship("project", project.id)
|> assert_jsonapi_relationship("category", arts.id)

second_result
|> assert_result_id(project_category_2.id)
|> assert_jsonapi_relationship("project", project.id)
|> assert_jsonapi_relationship("category", society.id)
path = "project-categories/?filter[id]=#{project_category_1.id},#{project_category_2.id}"

conn
|> get(path)
|> json_response(200)
|> assert_ids_from_response([project_category_1.id, project_category_2.id])
end
end

Expand All @@ -52,106 +30,62 @@ defmodule CodeCorps.ProjectCategoryControllerTest do
project_category = insert(:project_category, project: project, category: category)

conn
|> get(project_category_path(conn, :show, project_category))
|> request_show(project_category)
|> json_response(200)
|> Map.get("data")
|> assert_result_id(project_category.id)
|> assert_jsonapi_relationship("project", project.id)
|> assert_jsonapi_relationship("category", category.id)
end

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we no longer want to test the relationships are valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think since they are already tested in the model test, we decided to take them out of the controller tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you agree with that?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me. Thanks for clarifying!

test "does not show resource and instead throw error when id is nonexistent", %{conn: conn} do
path = conn |> project_category_path(:show, -1)
assert conn |> get(path) |> json_response(:not_found)
test "renders 404 when id is nonexistent", %{conn: conn} do
assert conn |> request_show(:not_found) |> json_response(404)
end
end

describe "create" do
@tag :authenticated
test "creates and renders resource when data is valid", %{conn: conn, current_user: current_user} do
@tag authenticated: :admin
test "creates and renders resource when data is valid", %{conn: conn} do
organization = insert(:organization)
category = insert(:category)
project = insert(:project, organization: organization)
insert(:organization_membership, role: "admin", member: current_user, organization: organization)


payload = build_payload |> put_relationships(project, category)

path = conn |> project_category_path(:create)
response = conn |> post(path, payload) |> json_response(201)
data = response |> Map.get("data")

data
|> assert_jsonapi_relationship("project", project.id)
|> assert_jsonapi_relationship("category", category.id)

project_category = ProjectCategory |> Repo.get(data["id"])
assert project_category
assert project_category.project_id == project.id
assert project_category.category_id == category.id
attrs = %{category: category, project: project}
assert conn |> request_create(attrs) |> json_response(201)
end

@tag authenticated: :admin
test "does not create resource and renders 422 when data is invalid", %{conn: conn} do
payload = build_payload()

path = conn |> project_category_path(:create)
data = conn |> post(path, payload) |> json_response(422)
assert data["errors"] != %{}
test "renders 422 when data is invalid", %{conn: conn} do
invalid_attrs = %{}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to assign invalid_attrs? On the two tests below we simply call request_create(%{}). Thoughts on removing this or possibly declaring @invalid_attrs up top and reusing it in the tests below as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the other controller tests we were declaring @invalid_attrs at the top and using it in the 422 test, but since it's the only place we end up using it @joshsmith said this might be a little cleaner.

I think this difference with the request_create for the 401 and 403 tests we don't even hit the attributes before it halts with an error, so an empty attribute map is kind of just placeholding instead of being invalid.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Do we need to even pass anything in the tests below, then? Can we just do conn |> request_create |> json_response(401) ?

Copy link
Contributor Author

@amyschools amyschools Nov 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mackenziehicks and I were asking all the same questions yesterday! In the api_case helper for request_create you have to pass in attributes, unlike for request_delete where you have an option to just use the conn.

      def request_create(conn, attrs) do
        path = conn |> path_for(:create)
        payload = json_payload(factory_name, attrs)
        conn |> post(path, payload)
      end

      def request_delete(conn), do: conn |> request_delete(default_record)
      def request_delete(conn, :not_found), do: conn |> request_delete(-1)
      def request_delete(conn, resource_or_id) do
        path = conn |> path_for(:delete, resource_or_id)
        conn |> delete(path)
      end
    end
  end

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for explaining!

What are your thoughts on adding a default to request_create?

def request_create(conn, attrs \\ %{})?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I'm dumb and hadn't considered this. Yes, this makes sense.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshsmith I think we should add it for update too.

Right now the test looks like this:
assert conn |> request_update({@valid_attrs}) |> json_response(401)
but we haven't actually added anything to update, so do we need to pass in @valid_attrs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'd add it for update, too.

assert conn |> request_create(invalid_attrs) |> json_response(422)
end

test "does not create resource and renders 401 when unauthenticated", %{conn: conn} do
payload = build_payload()

path = conn |> project_category_path(:create)
assert conn |> post(path, payload) |> json_response(401)
test "renders 401 when unauthenticated", %{conn: conn} do
assert conn |> request_create |> json_response(401)
end

@tag :authenticated
test "does not create resource and renders 403 when not authorized", %{conn: conn, current_user: current_user} do
organization = insert(:organization)
project = insert(:project, organization: organization)
category = insert(:category)
insert(:organization_membership, role: "contributor", member: current_user, organization: organization)

payload = build_payload |> put_relationships(project, category)

path = conn |> project_category_path(:create)
assert conn |> post(path, payload) |> json_response(403)
test "renders 403 when not authorized", %{conn: conn} do
assert conn |> request_create |> json_response(403)
end
end

describe "delete" do
@tag authenticated: :admin
test "deletes resource", %{conn: conn} do
project_category = insert(:project_category)

path = conn |> project_category_path(:delete, project_category)

assert conn |> delete(path) |> response(204)

refute Repo.get(ProjectCategory, project_category.id)
assert Repo.get(Project, project_category.project_id)
assert Repo.get(Category, project_category.category_id)
assert conn |> request_delete |> response(204)
end

test "does not delete resource and renders 401 when unauthenticated", %{conn: conn} do
project_category = insert(:project_category)
path = conn |> project_category_path(:delete, project_category)
assert conn |> delete(path) |> json_response(401)
test "renders 401 when unauthenticated", %{conn: conn} do
assert conn |> request_delete |> json_response(401)
end

@tag :authenticated
test "does not create resource and renders 403 when not authorized", %{conn: conn} do
project_category = insert(:project_category)
path = conn |> project_category_path(:delete, project_category)
assert conn |> delete(path) |> json_response(403)
test "renders 403 when not authorized", %{conn: conn} do
assert conn |> request_delete |> json_response(403)
end

@tag :authenticated
test "renders page not found when id is nonexistent on delete", %{conn: conn} do
path = conn |> project_category_path(:delete, -1)
assert conn |> delete(path) |> json_response(404)
test "renders 404 when id is nonexistent", %{conn: conn} do
assert conn |> request_delete(:not_found) |> json_response(404)
end
end
end