Refactoring Project Category Controller Tests

[amyschools]

What's in this PR?

Adding api_case.ex helpers.

References

Closes #418

Progress on: #413

[sbatson5]

Just a couple of questions, but this looks great! 👍

[sbatson5]

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

[amyschools]

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

[amyschools]

Do you agree with that?

[sbatson5]

That makes sense to me. Thanks for clarifying!

[sbatson5]

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?

[amyschools]

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.

[sbatson5]

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

[amyschools]

@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

[sbatson5]

Ah, thanks for explaining!

What are your thoughts on adding a default to request_create?

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

[joshsmith]

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

[mackenziehicks]

@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?

[joshsmith]

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

[begedin]

Minor change and then good for me.

[begedin]

Same as comment, while this is good, it could be rewritten in one line as

[project_category_1, project_category_2 | _] = insert_list(3, : project_category_1)

We don't really need to pre-insert the category, or the project. The factory should do that part for us.

[coveralls]

Coverage remained the same at 95.153% when pulling b3a3f2e on 418-projectcategory-controller-test into f9eef9d on develop.

[coveralls]

Coverage remained the same at 95.153% when pulling 3358049 on 418-projectcategory-controller-test into f9eef9d on develop.