Add ja_resource

[joshsmith]

Closes #331.

Will need documentation to explain to new contributors what's happening, mostly by linking to the ja_resource repo.

• Removes controller helpers
• Removes some model helpers (would like to remove all)
• Introduces query, slug, and string helpers
• Refactors most controllers to use the JaResource plug

Lots of little issues listed below in comments.

[joshsmith]

One issue that's unclear to me right now is how to do the analytics.track that we were doing previously inside controller actions. Maybe some sort of plug instead?

[joshsmith]

Don't yet understand how to use conn.assigns.changeset the way we had, even with handle_ functions.

[joshsmith]

Also is escaping me how to do show with slugs like in the project controller.

[joshsmith]

Some filters like limit, task_type, task_status, number_as_id, project, role, and title are not really filters in the sense that JaSerializer means.

We'll need to find a way to rework these, either on the Ember side or in the way we're handling the request in the controller.

[joshsmith]

Also not sure how to do the slugged route controller.

[joshsmith]

Have not done the task controller yet.

[joshsmith]

Decent indication of what still needs a good amount of work:

web/controllers/organization_membership_controller.ex:  plug JaResource, except: [:create, :update]
web/controllers/project_category_controller.ex:  plug JaResource, except: [:create]
web/controllers/project_controller.ex:  plug JaResource, except: [:show, :create]
web/controllers/project_skill_controller.ex:  plug JaResource, except: [:create]
web/controllers/role_skill_controller.ex:  plug JaResource, except: [:create]
web/controllers/skill_controller.ex:  plug JaResource, except: [:index]
web/controllers/user_category_controller.ex:  plug JaResource, except: [:create]
web/controllers/user_controller.ex:  plug JaResource, except: [:update]
web/controllers/user_role_controller.ex:  plug JaResource, except: [:create]
web/controllers/user_skill_controller.ex:  plug JaResource, except: [:create]

[joshsmith]

I have no idea if changesets are being handled here correctly at all via the load_and_authorize_changeset plug.

[joshsmith]

Also these will help some:

web/controllers/role_skill_controller.ex:  alias JaSerializer.Params
web/controllers/task_controller.ex:  alias JaSerializer.Params
web/controllers/user_controller.ex:  alias JaSerializer.Params

[joshsmith]

I think we could probably cut out between 150-200 LOC more if we figure out some of the above issues.

[sbatson5]

👍 Good catch. Didn't notice we weren't using response.

[begedin]

Looked into the ja_resource code and this is what a create callback looks like:

def call(controller, conn) do
  merged = JaResource.Attributes.from_params(conn.params) 
  attributes = controller.permitted_attributes(conn, merged, :create)

  conn
  |> controller.handle_create(attributes)
  |> JaResource.Create.insert(controller)
  |> JaResource.Create.respond(conn)
end

handle_create, handle_update and handle_delete need to be overriden to hook into ja_resource in any meaningful way.

For the first two of these, if they return a changeset, then ja_resource performs the create/update automatically. If they return a model, then ja_resource does nothing and just assumes a resource has been created/updated already. If they return a conn, then ja_resource also skips handling the response, assuming it's been handled.

For handle_delete, the function itself is what receives a model passed in and performs deletion.

For analytics, we might be able to create a plug that goes at the end of the line, after ja_resource does it's thing. On success, ja_resource will render the model, meaning it will call Phoenix.Controller.render(:show, data: model). A consequence of this is that conn.assigns.data will now be set to the model, so our plug could figure out the action the same way our load_and_authorize_changeset plug does and then call track with that action and the model data.

For authorization, however, I'm not seeing a way to create a plug. Instead, our handle_ function overrides would have to call authorize on the model/changeset manually. That would sadly mean that neither canary, nor bodyguard are compatible with ja_resource in what I would consider a nice way. We can make it work, but to a degree where we will have slightly less boilerplate in our controllers than we have without this, but it would be written in a way that's not really often seen in a semi-restful app, so it would be harder to understand for newcomers.

With some amount of work, we could make this work to a point where a lot of our controllers would have some or all of these:

def handle_create(conn, attributes) do
  changeset = %Task{} |> Task.create_changeset(attributes)
  case conn |> authorize(changeset) do
    true -> changeset # ja_resource will continue
    false -> conn |> handle_not_authorized # conn will halt
  end
end

def handle_update(conn, model, attributes) do
  changeset = model |> Task.update_changeset(attributes)
  case conn |> authorize(model/changeset) do
    true -> changeset # ja_resource will continue
    false ->  conn |> handle_not_authorized # conn will halt
  end
end

def handle_delete(conn, model) do
  case conn |> authorize(model) do
    true -> model # ja_resource will continue
    false ->  conn |> handle_not_authorized # conn will halt
  end
end

For slugs, we would override the record callback.

def record(conn, _id) do
  params = conn |> get_params_we_need_to_find_record
  Task |> repo.get_by(params)
end

For custom index filters, we override records

def records(conn) do
  params = conn |> get_index_filter_params_we_need
  Task.index_filters(params) |> repo.all
end

As I said, for analytics, we might be able to create a plug, but i'm worried that, since Phoenix.Controller.render does a send_resp, the conn might halt before our plug can do what it needs. If that's the case, then the track call would have to go into handle_create/_update_delete, on the true branch.

I also see you've created an issue in ja_resource, but I'm not sure your requests will be enough. They will allow us to handle analytics slightly better, but they will not help with authorization.

[begedin]

I created an issue of my own and the author replied with basically the same suggestion I have: vt-elixir/ja_resource#32 (comment)

[joshsmith]

@begedin should we temporarily bring back the six or so places where @analytics.track is used and get this merged? Then we can circle back to implement bodyguard along with any changes from ja_resource later?

[begedin]

@joshsmith I did a spike for analytics that reintroduces the tracking we removed, while keeping ja_resource. I also implemented a method to significantly trim down the amount of code in our Segment module once we completely switch.

While doing this, I realised where not really testing tracking in any proper way. Now that controller actions pass through segment, I really think we need some sort of testing for that, but I'm not sure what the best approach is. Ideally, we test the code we have by abstracting the part out of our control (direct segment calls) as much as possible and then somehow injecting our own layer for testing. I'll look into how to do that.

[begedin]

@joshsmith I went ahead and implemented the new tracking in all controllers we touched on in one commit. Then I moved on to add ja_resource and tracking to all those controllers that have tracking, but we haven't touched them yet, in separate commits.

I also, due to reasons above, increased the abstraction of our segment layer, by basically pulling two tiny functions (identify and track) to an outside module. Now our segment module is the one that handles the injection of the newly abstracted api layer, instead of our controllers.

Once I did that, several tests started failing due to newly discovered bugs, so I believe it was well worth the trouble.

Also, this will allow us to write more tests for our segment layer.

What's really left here is just adding ja_resource to the controllers that don't have it yet. These also don't have tracking, so they do not prevent us from merging, and can be done separately.

We can wrap up the remaining endpoints in a separate PR, then switch to bodyguard in a whole new PR. We can also make a PR to test our segment layer.

[joshsmith]

Some quick notes here.

Also would request that we spend some time doing inline documentation of this module and their functions. I don't think we should be approving any PRs with new functionality – even on our own – that don't have inline docs.

[joshsmith]

Is there any way to pattern match on the record's type at all for extra type safety?

[begedin]

Outside of having one function signature for every single record type, I'm not sure there is.

[joshsmith]

So we can just pass a record along to ja_resource and it won't try to do anything else with it, is that right?

[begedin]

Exactly. That is, it won't try insert it again. It will, however, render it.

[coveralls]

Changes Unknown when pulling 3e91d5c on 331-add-ja_resource into * on develop*.

[coveralls]

Changes Unknown when pulling f6775e0 on 331-add-ja_resource into * on develop*.

[joshsmith]

• Create or update bodyguard issue
• Create task to add ja_resource to controllers that lack it
• Create task to test segment analytics tracking