Switched comment controller away from ja_resource #867
Conversation
|
@begedin this looks great and definitely like the clarity this brings! Where was the double querying happening? |
Basically hidden from plain sight
|
|
@begedin I'm sure we are waiting on @joshsmith but I can help once this is merged! |
lib/code_corps/policy/policy.ex
Outdated
| end | ||
|
|
||
| def can?(%User{} = user, :update, %Comment{} = comment), do: Policy.Comment.update?(user, comment) | ||
| def can?(%User{} = user, :create, %Changeset{data: %Comment{}} = changeset), do: Policy.Comment.create?(user, changeset) |
There was a problem hiding this comment.
We do not rely on canary here, so we do not scope it within defimpl
begedin
force-pushed
the
864-switch-comment-controller-off-ja-resource
branch
from
6050448 to
b264709
Compare
| query |> id_filter(id_csv) | ||
| end | ||
| def id_filter(query, %{}), do: query | ||
| def id_filter(query, id_list) when is_binary(id_list) do |
There was a problem hiding this comment.
This whole thing has potential to be rewritten in a recursive fashion, where we accept a map, then pass the queryable through a pipe of clauses, each supporting a single key, applying a filter from that key, then calling the next clause with a map where that key was removed. We could even split our queries apport into context-specific modules, for example, Common.Query, Comment.Query, etc.
|
|
||
| @spec can?(User.t, atom, map | struct) :: boolean | ||
| defp can?(%User{} = user, :update, %Comment{} = comment), do: Policy.Comment.update?(user, comment) | ||
| defp can?(%User{} = user, :create, %{} = params), do: Policy.Comment.create?(user, params) |
There was a problem hiding this comment.
As we switch controllers away from ja_resource, we also switch policies away from Canary. They also should no longer accept changesets and instead simply accept params maps.
| @@ -41,6 +40,9 @@ defmodule CodeCorpsWeb do | |||
|
|
|||
| import Canary.Plugs | |||
| import CodeCorpsWeb.AuthenticationHelpers, only: [load_and_authorize_changeset: 2] | |||
|
|
|||
| alias CodeCorps.{Repo, Policy} | |||
| alias Plug.Conn | |||
There was a problem hiding this comment.
These will presumably be used in ally controllers, once we make the full switch so no reason not to include them here.
|
|
||
| alias CodeCorps.Comment | ||
| action_fallback CodeCorpsWeb.FallbackController | ||
| plug CodeCorpsWeb.Plug.DataToAttributes |
There was a problem hiding this comment.
We can move this one into the router :api pipeline once everything is fully switched.
|
|
||
| alias CodeCorps.Comment | ||
| action_fallback CodeCorpsWeb.FallbackController |
There was a problem hiding this comment.
This could go into the code_corps_web.ex using block once we make the full switch.
Any additional fallbacks can be put into the FallbackController module if they are used across controllers, or into the current controller module as a function, if specific to this controller, since this is just a plug anyway.
|
I feel this has been sufficiently reviewed and well documented here as well as in #864, so I'm merging this. |
|
@snewcomer just bringing to your attention that this is merged! |
| @@ -0,0 +1,21 @@ | |||
| defmodule CodeCorpsWeb.Plug.DataToAttributes do | |||
| @moduledoc """ | |||
| Puts authenticated Guardian user into conn.assigns[:current_user] | |||
There was a problem hiding this comment.
@begedin Is this moduledoc correct?
There was a problem hiding this comment.
@snewcomer It was absolutely not. I'll do a quick correction PR
Progress on #864
What's in this PR?
This PR contains a mergeable example of a complete switch away from
ja_resourcetowards using the facilities provided byaction_fallbackavailable in Phoenix 1.3The general philosophy is explicitness. In each controller action we
Note that in some cases, we instead authorize an action on a changeset, so the changeset is initialized first. This was troublesome for us before as well, but seems less so this time around.
The changes here remove all reliance on
ja_resourcefrom the comment controller, as well as reliance on the canary library for authorization, since all that library really provides is it's own set ofload_resource,authorize_resource, etc. plugs. It still keeps policies around, but they are moved outside the scope of aCanada.Canimplementation.A big advantage is that, since
ja_resourceisn't there to do it's own loading anymore, we effectively almost halved the amount of querying the do.Sadly, the
action_fallbackdoes have a limitation - it does not fallback out of plugs, only from the controller action itself. That means that, while full reliance on plugs would reduce controller boilerplate further, it would also effectively put us in the same rigid system we have now.I feel the changes here, while increasing the number of lines of code slightly
Some progressive enhancements we can do
JaSerializer.Params.to_attributes(data)step. We could call it ajson_apiplug201status to the conn