Refactor Task Controller Tests with helpers

[amyschools]

What's in this PR?

Add api_case.ex helpers to Task controller tests.

Stuck on a fix to make two tests pass: update: create and update: renders 422 error.

References

Closes #442

Progress on: #413

[sbatson5]

This is looking good 👍

[sbatson5]

Looks like the indentation is off here.

[sbatson5]

Looks like there is a trailing bracket here.

[begedin]

@amyschools What difficulty are you having with the tests? Would you mind fixing the extra bracket on line 142? Can't really see which tests are failing when the app doesn't compile.

[amyschools]

@begedin it should compile now. I'm getting a 403 error for three of the tests:

1) test create renders 422 when data is invalid (CodeCorps.TaskControllerTest)
2) test update renders 422 when data is invalid (CodeCorps.TaskControllerTest)
3) test create creates and renders resource when data is valid (CodeCorps.TaskControllerTest)

and a new error saying key :task is not recognized that I thought I had solved yesterday on

4) test update updates and renders chosen resource when data is valid (CodeCorps.TaskControllerTest)

[begedin]

@amyschools Looks like you didn't push up, or at least, I'm not seeing the changes. I'll try to reply best I can.

For the update test, we have the following:

@tag authenticated: :admin # an admin user is performing this test
test "updates and renders chosen resource when data is valid", %{conn: conn, current_user: current_user} do
  task = insert(:task, user: current_user)

  # you're not sending the task to `request_update`, so it doesn't do anything
  assert conn |> request_update(@valid_attrs) |> json_response(200)
end

@tag :authenticated # notice the different tag. a regular user is performing
test "renders 422 when data is invalid", %{conn: conn, current_user: current_user} do
  assert conn |> request_update(@invalid_attrs) |> json_response(422)
end

For the first test, it's tagged to authenticate as admin.

You create a task, but then create the version of the request_update function that doesn't accept a task and instead creates its own resource. The tests pass, since you're authenticated as site admin, but the correct approach would be either to

Option A

Remove the task = insert(:task, user: current_user)line (and also remove the current user from the setup hash in the test signature),

Option B

Keep the task line, tag as :authenticated(so not authenticated: :admin) and then call

assert conn |> request_update(@valid_attrs) |> json_response(200)

My vote would be option B, since as a side-effect, it's testing a more complex case of authorization.

For the second test, as you can see, you're not authenticating as admin, which is why it's failing. If you go with option B, then the solution here would be to basically copy everything from the first test and just replace @valid_attrs with @invalid_attrs. That should do the trick, so you would end up with:

@tag :authenticated
test "renders 422 when data is invalid", %{conn: conn, current_user: current_user} do
  task = insert(:task, user: current_user)
  assert conn |> request_update(task, @invalid_attrs) |> json_response(422)
end

For create, the problem is similar, but not quite the same

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

@invalid_attrs doesn't have a user_id or a project id assigned. The authorization runs, tries to see if the user is the author of the task and figures out it not (philosophical, really 😄 ) and concludes that you are not authorized.

In order to get it working, you mimic the valid request approach:

project = insert(:project)
attrs = @valid_attrs |> Map.merge(%{project: project, user: current_user})
assert conn |> request_create(attrs) |> json_response(201)]

The difference being, you replace @valid_attrs with @invalid_attrs.

project = insert(:project)
attrs = @invalid_attrs |> Map.merge(%{project: project, user: current_user})
assert conn |> request_create(attrs) |> json_response(422)

It's a bit complicated, but basically, in all our cases, we run authorization before we validate the record. That means that, to test validation error responses, you want to set up your request so authorization passes, but validation fails after. On some controllers, that isn't even possible, but in this one, it is, albeit it gets a bit complicated.

It might make many things simpler, but I'm not sure.

For your create and update valid request tests, it seems to me like they should work, from the changes here. However, it may be you altered something, so I'll take another look when you push your changes up.

@joshsmith Writing all this, maybe we should at one point look into our process being something like:

get_record_if_applicable
initialize_changeset_for_record_and_attributes
respond_with_422_if_changeset_invalid
authorize_if_changeset_valid
respond_with_403_if_unauthorized
respond_with_ok_if_authorized

[coveralls]

Coverage decreased (-0.2%) to 93.689% when pulling 0c93e62 on 442-task-controller-tests into e492da7 on develop.

[amyschools]

@begedin Thank you so much!! That explanation was super helpful. All the tests are passing now so maybe one last check if you have time?

[begedin]

Just some indentation problems that need fixing. Looks good otherwise.

[begedin]

Indentation here is bad. Which editor are you using, and does it have an .editorconfig plugin? Should prevent this from happening.

Feel free to mention it on slack. Someone can guide you through configuring an editor when we get the chance.

[amyschools]

just installed the editorconfig package for atom!

[begedin]

More indentation problems here.

[joshsmith]

@amyschools this had a review with changes that need made.

[coveralls]

Coverage decreased (-0.2%) to 93.689% when pulling 812ec03 on 442-task-controller-tests into e492da7 on develop.

[amyschools]

pushed to the wrong branch! should be good now.

The 'close pull request' button is very close to the comment button...

Reviewing

[joshsmith]

@amyschools awesome, rebase, squash, and merge.

[coveralls]

Coverage decreased (-1.6%) to 92.084% when pulling a860dad on 442-task-controller-tests into 5d0d623 on develop.