Switch OrganizationGithubAppInstallationController away from ja_resource and canary

[begedin]

Progress on #864

Problem

We want to move away from the ja_resource and canary combo used in most of our controller modules. The central issue for this project is #864

There is a generic process of how to do it in that issue, as well as tracking for the milestone progress.

The procedure is also pasted below. If this specific controller deviates from this standard procedure, feel free to ask for help here, or by opening a PR.

The PR should indicate that it's closing this issue and is progressing #864 further along.

Procedure

• pick a controller to switch
• view actions this controller should support in lib/code_corps_web/router.ex
• add action_fallback CodeCorpsWeb.FallbackController. This makes it so in case a with clause within the controller action fails, a sort of "catch all" function will be called from FallbackController
• add plug CodeCorpsWeb.Plug.DataToAttributes. This converts the data hash within the conn params from the json api structure into a flat map convenient for usage with changesets
• for each action
  • remove that action from any load_resource, load_and_authorize_resource, etc. plugs in that controller
  • remove any def handle_#{action} functions
  • write a def #{action}(conn, params) do. For the standard create, update delete, show and index actions. See examples below on how these functions should be written, usually
  • rewrite the policy function for that action to support params instead of a changeset if necessary. See section below for pointers.

That should be the whole procedure. We already have controller tests in place, so if the process has been done correctly, the tests should pass.

Some additional steps might be necessary, however, if there are some specifics to an action. For those, we will be here to provide feedback.

How to rewrite controller actions

@spec index(Conn.t, map) :: Conn.t
def index(%Conn{} = conn, %{} = params) do
  with resources <- ResourceModule |> Query.id_filter(params) |> Repo.all do
    conn |> render("index.json-api", data: resources)
  end
end

@spec show(Conn.t, map) :: Conn.t
def show(%Conn{} = conn, %{"id" => id}) do
  with %ResourceModule{} = resource <- ResourceModule |> Repo.get(id) do
    conn |> render("show.json-api", data: resource)
  end
end

@spec create(Plug.Conn.t, map) :: Conn.t
def create(%Conn{} = conn, %{} = params) do
  with %User{} = current_user <- conn |> Guardian.Plug.current_resource,
       {:ok, :authorized} <- current_user |> Policy.authorize(:create, params),
       {:ok, %Comment{} = resource} <- %ResourceModule{} |> ResourceModule.create_changeset(params) |> Repo.insert do
    conn |> put_status(:created) |> render("show.json-api", data: resource)
  end
end

@spec update(Conn.t, map) :: Conn.t
def update(%Conn{} = conn, %{"id" => id} = params) do
  with %ResourceModule{} = resource <- ResourceModule |> Repo.get(id),
       %User{} = current_user <- conn |> Guardian.Plug.current_resource,
       {:ok, :authorized} <- current_user |> Policy.authorize(:update, resource),
       {:ok, %ResourceModule{} = resource} <- resource |> ResourceModule.changeset(params) |> Repo.update do
    conn |> render("show.json-api", data: resource)
  end
end

So to recap, the actions work like

index action

• apply filters from query params to load data
• render data

show action

• load resource
• render resource

create action

• authorize action for current user and parameters (requires rewriting that policy to use a params map instead of a changeset, expect {:ok, :authorized}
• perform insert action
• render resulting resource

update and delete actions

• load resource
• authorize action for current user on resource, expect {:ok, :authorized}
• perform update/delete
• render resource, or no content for delete

Examples in code

-CodeCorpsWeb.CommentController has examples for standard index, show, create and update actions

How to rewrite policies

This consists of several relatively simple steps.

• For each action being switch away, we need to move the def can function clause out of the defimpl block in policy.ex into the root namespace of the CodeCorps.Policy module.

• We also need to make the clause private instead of public.

• If any of the clauses expect a changeset, they should now expect a simple map of params:

defp can?(%User{} = user, :some_action, %Changeset{data: %Resource{}} = changeset), do: Policy.Resource.some_action?(user, changeset)

becomes

defp can?(%User{} = user, :some_action, %{} = params), do: Policy.Resource.some_action?(user, params)

• in this case, the policy module itself also need to be rewritten to support a params map. Since in most cases, these policies simply look at the changeset.changes map, the rewrite ought to be simpler. If there is trouble, someone will be available to provide pointers.

Examples in code

• the rewrite has been done for Policy.Comment.create?