Switch OrganizationGithubAppInstallation away from ja_resource and canary by treble37 · Pull Request #921 · code-corps/code-corps-api

treble37 · 2017-09-10T21:54:37Z

Switch OrganizationGithubAppInstallationController away from ja_resource and canary

Closes #893

What's in this PR?

Switch OrganizationGithubAppInstallationController away from ja_resource and canary

Make sure any changes to code include changes to documentation.

References

Fixes # 893

Progress on: # 864

joshsmith · 2017-09-11T04:57:28Z

lib/code_corps_web/controllers/organization_github_app_installation_controller.ex

+  def delete(%Conn{} = conn, %{} = params) do
+    with %User{} = current_user <- conn |> Guardian.Plug.current_resource,
+         {:ok, :authorized} <- current_user |> Policy.authorize(:delete, %OrganizationGithubAppInstallation{}, params),
+         {:ok, %OrganizationGithubAppInstallation{} = organization_installation} <- %OrganizationGithubAppInstallation{} |> OrganizationGithubAppInstallation.delete_changeset(params) |> Repo.insert do


Looks like the build is erroring because OrganizationGithubAppInstallation.delete_changeset/2 doesn't exist. I'm not sure this needs a changeset, just a delete operation.

Thanks @joshsmith, the tests were running locally (3 failures) so I didn't notice the build and I've updated...

treble37 · 2017-09-11T05:46:00Z

I get 2 failing tests locally now with message like:

1) test delete deletes resource (CodeCorpsWeb.OrganizationGithubAppInstallationControllerTest)
     test/lib/code_corps_web/controllers/organization_github_app_installation_controller_test.exs:75
     ** (RuntimeError) expected response with status 204, got: 403, with body:
     {"errors":[{"title":"403 Forbidden","status":403,"id":"FORBIDDEN","detail":"You are not authorized to perform this action."}]}
     stacktrace:
       (phoenix) lib/phoenix/test/conn_test.ex:362: Phoenix.ConnTest.response/2
       test/lib/code_corps_web/controllers/organization_github_app_installation_controller_test.exs:79: (test)

I wonder how I setup the authorization correctly?

begedin

Some changes to be made, but otherwise, good first draft. 👍

begedin · 2017-09-11T09:44:50Z

lib/code_corps_web/controllers/organization_github_app_installation_controller.ex

+    with %User{} = current_user <- conn |> Guardian.Plug.current_resource,
+         {:ok, :authorized} <- current_user |> Policy.authorize(:delete, %OrganizationGithubAppInstallation{}, params),
+         {:ok, %OrganizationGithubAppInstallation{} = organization_installation} <- %OrganizationGithubAppInstallation{} |> OrganizationGithubAppInstallation.changeset(params) |> Repo.insert do
+      conn |> put_status(:deleted) |> render("show.json-api", data: organization_installation)


The order of operations here should be

load organization github app installation by id

{:ok, :authorized} <- current_user |> Policy.authorize(:delete, organization_github_app_installation),

{:ok, %OrganizationGithubAppInstallation{}} <- organization_github_app_installation |> Repo.delete

render a 204 (basically, conn |> send_resp(:no_content, "")

If you implement it that way, you do should

not have to make the changeset function public

implement the delete policy to accept a record, not params

Updated @begedin, thanks

begedin · 2017-09-11T09:48:37Z

lib/code_corps_web/controllers/organization_github_app_installation_controller.ex

+      {:ok, %OrganizationGithubAppInstallation{} = organization_installation} <- organization_installation |> OrganizationGithubAppInstallation.changeset(params) |> Repo.update do
+        conn |> render("show.json-api", data: organization_installation)
+    end
+  end


We don't actually support an update in this controller, so this action should be completely removed.

A good way to determine which are supported is to look into the router.ex:

# in the non-authenticated scope, we have resources "/organization-github-app-installations", OrganizationGithubAppInstallationController, only: [:index, :show] # in the authenticated scope, we have resources "/organization-github-app-installations", OrganizationGithubAppInstallationController, only: [:create, :delete]

That makes the supported actions only [:index, :show, :create, :delete]

If you think about it, it makes sense. An organization_github_app_installation is a many-to-many link between an organization and a github-app-installation, so there is nothing to change about an existing record. It either exists, or it doesn't.

begedin · 2017-09-11T09:49:50Z

lib/code_corps/policy/policy.ex

  defp can?(%User{} = user, :create, %Organization{}, %{}), do: Policy.Organization.create?(user)
  defp can?(%User{} = user, :update, %Organization{} = organization, %{}), do: Policy.Organization.update?(user, organization)  
  defp can?(%User{} = current_user, :update, %User{} = user, %{}), do: Policy.User.update?(user, current_user)
+  defp can?(%User{} = user, :delete, %OrganizationGithubAppInstallation{}, %{} = params), do: Policy.OrganizationGithubAppInstallation.delete?(user, params)


If you fix the delete action below, this will become

defp can?(%User{} = user, :delete, %OrganizationGithubAppInstallation{} = organization_github_app_installation, %{}), do: Policy.OrganizationGithubAppInstallation.delete?(user, organization_github_app_installation)

begedin · 2017-09-11T09:50:45Z

lib/code_corps/policy/organization_github_app_installation.ex

-  def delete?(%User{} = user, %OrganizationGithubAppInstallation{} = github_app_installation),
-    do: github_app_installation |> get_organization |> owned_by?(user)
+  def delete?(%User{} = user, params),
+    do: params |> atomize_params |> get_organization |> owned_by?(user)


You can completely revert this change if you fix the controller action.

joshsmith · 2017-09-12T21:35:54Z

Hey @treble37 just a heads up that lib/code_corps/policy/policy.ex has changed so you'll need to rebase on upstream develop and bring in the new changes from upstream.

Let me know if you need any help at all.

joshsmith · 2017-09-15T00:15:08Z

Hey @treble37 wanted to check in on this again and see if you need anything from me.

treble37 · 2017-09-15T05:35:03Z

Thanks @joshsmith, I just pulled down the latest dev branch changes, will rebase and get back to you...it's just been a busy week

treble37 · 2017-09-17T14:33:28Z

@joshsmith - mix test passes locally but not on circle CI:

....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

Finished in 14.5 seconds
1164 tests, 0 failures

begedin · 2017-09-18T07:50:11Z

@treble37 I restarted the build and the tests passed. It must have been some parallelism issue with how bypass uses ports or something.

We need you to squash your commits into one and rebase before we can merge.

treble37 · 2017-09-18T13:31:15Z

squashed @begedin , thanks!

begedin · 2017-09-18T14:32:56Z

lib/code_corps_web/controllers/organization_github_app_installation_controller.ex

+  plug CodeCorpsWeb.Plug.DataToAttributes

  @spec model :: module
  def model, do: CodeCorps.OrganizationGithubAppInstallation


Sorry I missed this on the first pass, but the method here is no longer necessary now, so you can remove it.

So I don't annoy you further, don't forget to squash again, as well as run a git pull develop followed by git rebase origin/develop at the end of it. Then I can merge.

ok thanks for your patience @begedin with my first contribution, I rebased on the latest origin/develop and removed def model method....thanks!

I'm the one who should be thankful @treble37

And now I have to apologize again and ask you to do yet another change. The filter method also isn't required anymore. Mind removing that one to?

Again, squash and rebase after.

I'm really sorry about this, but the "changes" view on github really sucks when it comes to seeing which code needs to be removed.

No worries @begedin, filter removed!

Switch OrganizationGithubAppInstallationController away from ja_resource and canary Closes code-corps#893

begedin · 2017-09-18T15:10:43Z

@treble37 Excellent work! 👍

joshsmith · 2017-09-18T19:25:16Z

🙌

joshsmith reviewed Sep 11, 2017

View reviewed changes

begedin requested changes Sep 11, 2017

View reviewed changes

treble37 force-pushed the org_github_controller_893 branch from aaf9f12 to 0930663 Compare September 17, 2017 06:34

treble37 force-pushed the org_github_controller_893 branch from 0930663 to f710533 Compare September 18, 2017 13:30

begedin approved these changes Sep 18, 2017

View reviewed changes

begedin requested changes Sep 18, 2017

View reviewed changes

treble37 force-pushed the org_github_controller_893 branch 3 times, most recently from 7abfa03 to a3d36b6 Compare September 18, 2017 14:43

Switch OrganizationGithubAppInstallation...

55ca3eb

Switch OrganizationGithubAppInstallationController away from ja_resource and canary Closes code-corps#893

treble37 force-pushed the org_github_controller_893 branch from a3d36b6 to 55ca3eb Compare September 18, 2017 14:51

begedin approved these changes Sep 18, 2017

View reviewed changes

begedin changed the title ~~Switch OrganizationGithubAppInstallation...~~ Switch OrganizationGithubAppInstallation away from ja_resource and canary Sep 18, 2017

begedin merged commit 8973e5f into code-corps:develop Sep 18, 2017

treble37 deleted the org_github_controller_893 branch September 18, 2017 20:53

Conversation

treble37 commented Sep 10, 2017

What's in this PR?

References

Uh oh!

Choose a reason for hiding this comment

Uh oh!

treble37 Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

treble37 commented Sep 11, 2017

Uh oh!

begedin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshsmith commented Sep 12, 2017

Uh oh!

joshsmith commented Sep 15, 2017

Uh oh!

treble37 commented Sep 15, 2017

Uh oh!

treble37 commented Sep 17, 2017

Uh oh!

begedin commented Sep 18, 2017

Uh oh!

treble37 commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begedin commented Sep 18, 2017

Uh oh!

joshsmith commented Sep 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

treble37 Sep 11, 2017 •

edited

Loading