Switches category controller away from ja_resource #880
Conversation
Push to production
Push to production
Push to production
Push to production
Push project categories endpoints to production
Push to production
Push to production
Push to production
Push to production
Push to production
Push Stripe and tasks to production
Push fix to production
Push receipt emails to prod
Push image changes to production
Push to production
Push dialyzer changes to prod
Push analytics changes to production
Push Stripe event fix to prod
Push to production
Add project users in production
Push to production
Push to production
Push to production
Push to production
Push to production
Push to production
Push to production
Push to production
Push to production
Push to production
Push to production
lib/code_corps/policy/category.ex
Outdated
| def update?(%User{admin: true}), do: true | ||
| def update?(%User{admin: false}), do: false | ||
| def update?(%{admin: true}), do: true | ||
| def update?(%{admin: false}), do: false |
There was a problem hiding this comment.
I understand your goal here is reduction of code, but we tend to prefer using stronger type constraints unless we have to loosen them for some reason.
In fact, what we'd prefer here is to rewrite these as
@spec create?(User.t) :: boolean
def create?(%User{admin: true}), do: true
def create?(%User{}), do: false
@spec update?(User.t) :: boolean
def update?(%User{admin: true}), do: true
def update?(%User{}), do: false
lib/code_corps/policy/policy.ex
Outdated
| defp can?(%User{} = user, :create, %Category{}, %{}), do: Policy.Category.create?(user) | ||
| defp can?(%User{} = user, :update, %Category{}, %{}), do: Policy.Category.update?(user) | ||
|
|
||
| @spec can?(User.t, atom, struct, map) :: boolean |
There was a problem hiding this comment.
These all have the same "prototype" (same amount of arguments, same types of arguments in the same order), so there is no need for an extra @spec, nor for keeping a blank line between them. Elixir, and Credo prefer no blank lines between the same function's (in the context of arity) different clauses.
This is something that wasn't clear since there was only a single example in the code previously, but should be clearer once we merge your PR in.
| def model, do: CodeCorps.Category | ||
| @spec index(Conn.t, map) :: Conn.t | ||
| def index(%Conn{} = conn, %{} = params) do | ||
| with category <- Category |> Query.id_filter(params) |> Repo.all do |
There was a problem hiding this comment.
categories instead of category here. We don't want to imply that index loads a single record. I'm guessing a copy-paste lapse :)
| {:ok, %Category{} = category} <- category |> Category.changeset(params) |> Repo.update | ||
| do | ||
| conn |> render("show.json-api", data: category) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Textbook quality controller conversion here 👍
crodriguez1a
force-pushed
the
864/category_controller
branch
from
b98fb0f to
9806646
Compare
crodriguez1a
force-pushed
the
864/category_controller
branch
from
9806646 to
89316ba
Compare
|
@begedin Having a little trouble when rebasing. Is there a wiki somewhere with your git conventions? |
|
@begedin Actually, I think I may have rebased from master(instead of develop). I may need to close this PR and open another. |
What's in this PR?
Updates
Categorypolicy methods- can (
policy.ex)- update (
category/policy.ex)- create (
category/policy.ex)Updates
Categorycontroller methods- index
- show
- update
- create
Progress on: #864