Skip to content

Conversation

@npendery
Copy link
Contributor

Closes #132

|> Repo.all

render(conn, "index.json-api", data: memberships)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the problem with this approach is that the two could be combined: you may be filtering either ids or roles.

I'm not sure what the actual requests look like that are generating the filtering, and if they're both used, but this does look like a bit of an explosion of endpoints and some duplication, as well as possibly missing combinations of request types.

@joshsmith
Copy link
Contributor

You'll need to rebase off develop now.

|> where([om], om.role in ^roles)
_ ->
OrganizationMembership
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I would personally simplify this into a couple different methods:

def index(conn, params) do
  memberships = OrganizationMembership
  |> with_filters(params)
  |> preload([:organization, :member])
  |> Repo.all

  render(conn, "index.json-api", data: memberships)
end

defp with_filters(query, params) do
  query
  |> with_organization_filter(params)
  |> with_role_filter(params)
  |> with_member_filter(params)
end

defp with_organization_filter(query, %{"organization_id" => organization_id}) do
  query |> where([om], om.organization_id == ^organization_id)
end
defp with_organization_filter(query, _), do: query

defp with_role_filter(query, %{"roles" => roles}) do
  roles = roles |> coalesce_string
  query |> where([om], om.role in ^roles)
end
defp with_role_filter(query, _), do: query

defp with_member_filter(query, %{"filter" => id_list}) do
  ids = id_list |> coalesce_id_string
  query |> where([om], om.member_id in ^ids)
end
defp with_member_filter(query, _), do: query

@joshsmith
Copy link
Contributor

Looks like you have just one failure:

  1) test filters resources on index (CodeCorps.OrganizationMembershipControllerTest)
     test/controllers/organization_memberships_controller_test.exs:26
     ** (FunctionClauseError) no function clause matching in String.split/3

@npendery
Copy link
Contributor Author

Yeah, still need to edit/add tests

membership_2 = insert(:organization_membership, member: member_2)
insert(:organization_membership, member: member_3)


Copy link
Contributor

Choose a reason for hiding this comment

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

Bit nitpicky, but mind ditching the extra empty line here? One is enough.

@begedin
Copy link
Contributor

begedin commented Aug 26, 2016

Other than that extra empty line, looks good to go. I would like to raise the question of code organization, though. I think we might have gotten to the point where model_helpers and controller_helpers aren't the correct names anymore.

Basically, everything in our controller helpers and most of model helpers fits together, maybe into something like a CodeCorps.Filtering module?.

@joshsmith Do we want to create an issue for that?

I think this is good to merge otherwise. If we want to reorganize, it should go with the separate issue.

@npendery npendery merged commit 3e113d4 into develop Aug 26, 2016
@npendery npendery deleted the np-filter-by-role branch August 26, 2016 19:03
@joshsmith
Copy link
Contributor

@begedin yes let's please create a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants