Switch GithubAppInstallationController away from ja_resource and canary #1055
Conversation
| plug JaResource | ||
| action_fallback CodeCorpsWeb.FallbackController | ||
| plug CodeCorpsWeb.Plug.DataToAttributes | ||
| plug CodeCorpsWeb.Plug.IdsToIntegers | ||
|
|
||
| @spec model :: module | ||
| def model, do: CodeCorps.GithubAppInstallation |
There was a problem hiding this comment.
You can completely remove the model function. It's used internally by ja_resource only.
| def create?(%User{} = user, %Ecto.Changeset{} = changeset), | ||
| do: changeset |> get_project |> owned_by?(user) | ||
| def create?(%User{} = user, params), | ||
| do: params |> get_project |> owned_by?(user) | ||
|
|
||
| def update?(%User{} = user, %GithubAppInstallation{} = github_app_installation), |
There was a problem hiding this comment.
Seems like you pointed out a bug we weren't aware of. There github app installation endpoint should only support :index, :show and :create in the controller. That means we do not really need an :update policy here, so feel free to remove it as well as the associated tests.
There was a problem hiding this comment.
Ok, all changes should be live now, thanks @begedin !
| %GithubAppInstallation{} | ||
| |> GithubAppInstallation.create_changeset(attributes) | ||
| @spec delete(Plug.Conn.t, map) :: Conn.t | ||
| def delete(%Conn{} = conn, %{"id" => id} = params) do |
There was a problem hiding this comment.
Again, a "bug" here. We should not be supporting a :delete. The controller should only have :index, :show and :create.
treble37
force-pushed
the
890-githubappcontroller
branch
from
956c75a to
6ea520b
Compare
| def create?(%User{} = user, %Ecto.Changeset{} = changeset), | ||
| do: changeset |> get_project |> owned_by?(user) | ||
| def create?(%User{} = user, params), | ||
| do: params |> get_project |> owned_by?(user) | ||
|
|
There was a problem hiding this comment.
Minor, but would kill this line.
There was a problem hiding this comment.
removed newline and it's all one one line now, thx!
|
Tiny tiny change and then this is good to go. |
treble37
force-pushed
the
890-githubappcontroller
branch
from
6ea520b to
bfbb0f4
Compare
begedin
dismissed
joshsmith’s stale review
Took over, requested changes were made.
|
Great work @treble37 |
What's in this PR?
Helps support #864
Make sure any changes to code include changes to documentation.
References
Fixes #890
Progress on: #864