[USERSKILL]: refactor away ja_resource by snewcomer · Pull Request #926 · code-corps/code-corps-api

snewcomer · 2017-09-13T03:39:05Z

Fixes #916

snewcomer · 2017-09-13T03:52:10Z

lib/code_corps_web/controllers/user_skill_controller.ex

+    with %UserSkill{} = user_skill <- UserSkill |> Repo.get(id),
+      %User{} = current_user <- conn |> Guardian.Plug.current_resource,
+      {:ok, :authorized} <- current_user |> Policy.authorize(:delete, user_skill),
+      {:ok, %UserSkill{} = _user_skill} <- user_skill |> Repo.delete


@begedin getting a test failure here

Not really familiar with this :track code. Lmk what you think here.

You've discovered a bug we've been creating on some controllers with this switch

We have a plug, SegmentPlugTracker, which registers an action right before the conn sends a response.

This action figures out what happened (in this case, that a UserSkill was deleted) and calls tracking code for that action.

However, the "figuring out" also relied on ja_resource and canary, so in this case, it got broken and the controller test found it. Unfortunately it seems not all our controller tests have this assert_received, so we may have turned off tracking in some cases. I'll make sure to check.

We probably want to make that explicit as well, to avoid further issues, so I'll create an issue to do this.

For know, to keep tracking working here, you'll need to do a conn |> Conn.assign(:user_skill, user_skill) |> conn |> send_resp(:no_content, "")

begedin

Good work. The failed test is actually a good thing and you may have stopped us from causing bugs down the line.

begedin · 2017-09-13T10:15:24Z

lib/code_corps_web/controllers/user_skill_controller.ex

+    with %UserSkill{} = user_skill <- UserSkill |> Repo.get(id),
+      %User{} = current_user <- conn |> Guardian.Plug.current_resource,
+      {:ok, :authorized} <- current_user |> Policy.authorize(:delete, user_skill),
+      {:ok, %UserSkill{} = _user_skill} <- user_skill |> Repo.delete


You've discovered a bug we've been creating on some controllers with this switch

We have a plug, SegmentPlugTracker, which registers an action right before the conn sends a response.

This action figures out what happened (in this case, that a UserSkill was deleted) and calls tracking code for that action.

However, the "figuring out" also relied on ja_resource and canary, so in this case, it got broken and the controller test found it. Unfortunately it seems not all our controller tests have this assert_received, so we may have turned off tracking in some cases. I'll make sure to check.

We probably want to make that explicit as well, to avoid further issues, so I'll create an issue to do this.

For know, to keep tracking working here, you'll need to do a conn |> Conn.assign(:user_skill, user_skill) |> conn |> send_resp(:no_content, "")

joshsmith · 2017-09-13T18:09:44Z

@snewcomer perhaps we need some higher-level docs somewhere about the tracking?

begedin · 2017-09-14T06:18:15Z

@joshsmith Higher level docs are good, but I've also created #927 to avoid unclear cases like this one. I think we made far too many things implicit in the early days of this project.

snewcomer · 2017-09-14T14:44:56Z

@begedin is this g2g?

begedin · 2017-09-14T15:02:56Z

@snewcomer Squash the two commits together and then you can consider it merged. However, I'm hoping #805 gets merged first, since rebasing this one would be much easier, so don't worry, if the PR stays up for a bit. It's just waiting on that one.

joshsmith · 2017-09-15T03:35:50Z

#805 is possibly not going to get merged quickly, pending what @begedin says in reply to review comments from today.

begedin

We merged one of the two PRs, so rebasing the other became simple enough where we can get this merged to.

However, on a second review, I found a few things that need to be fixed.

begedin · 2017-09-18T15:14:38Z

lib/code_corps/policy/user_skill.ex

-  def create?(%User{id: id}, %Changeset{changes: %{user_id: user_id}}), do: id == user_id
-  def create?(%User{}, %Changeset{}), do: false
+  def create?(%User{admin: true}, %{}), do: true
+  def create?(%User{id: id}, %{user_id: user_id}), do: id == user_id


@snewcomer The params we send here use strings as keys. Additionally, there's an issue with id values in the params map all being strings (this is what ember sends), so to fix the problem here you need to

replace this line with def create?(%User{id: id}, %{"user_id" => user_id}), do: id == user_id

update the policy test to match

add plug CodeCorpsWeb.Plug.IdsToIntegers into the controller, right after the DataToAttributes plug.

@begedin thats a great catch. Fixed.

begedin · 2017-09-18T15:17:00Z

test/lib/code_corps/policy/user_skill_test.exs

    test "returns true if user is creating their own record" do
      user = insert(:user)
-      changeset = %UserSkill{} |> create_changeset(%{user_id: user.id})
+      user_skill = %UserSkill{user_id: user.id}


As I explained above, this needs to be a params map isntead, looking like

params = %{"user_id" => user.id} assert create?(user, params)

begedin · 2017-09-19T07:51:40Z

test/lib/code_corps/policy/user_skill_test.exs

      changeset = %UserSkill{} |> create_changeset(%{})

-      assert create?(user, changeset) 
+      assert create?(user, %{"user_id" => user.id}) 


You're not using the changeset here anymore. You can likely also remove import CodeCorps.UserSkill, only: [create_changeset: 2] from the top of the file.

Sorry I didn't catch this on my previous pass.

oops. fixed.

begedin · 2017-09-19T12:39:59Z

Thank you @snewcomer. Great work 👍

snewcomer added Difficulty: Medium needs code review labels Sep 13, 2017

snewcomer self-assigned this Sep 13, 2017

snewcomer requested a review from begedin September 13, 2017 03:39

snewcomer added in progress Size: S labels Sep 13, 2017

snewcomer force-pushed the user-skill-controller-ja-resource branch 2 times, most recently from 72a2b0f to 9ae0ae9 Compare September 13, 2017 03:50

snewcomer commented Sep 13, 2017

View reviewed changes

begedin requested changes Sep 13, 2017

View reviewed changes

snewcomer force-pushed the user-skill-controller-ja-resource branch from e7c3969 to bf6de49 Compare September 14, 2017 14:46

begedin requested changes Sep 18, 2017

View reviewed changes

snewcomer force-pushed the user-skill-controller-ja-resource branch from bf6de49 to 28db06e Compare September 19, 2017 03:22

begedin requested changes Sep 19, 2017

View reviewed changes

snewcomer added 4 commits September 19, 2017 05:28

[USERSKILL]: refactor away ja_resource

d956a51

assign user_skill to conn so segment tracking works

a1e0cba

update policy with user_id with string key

af7bf03

remove unused vars

48e4933

snewcomer force-pushed the user-skill-controller-ja-resource branch from 32f768b to 48e4933 Compare September 19, 2017 12:28

begedin approved these changes Sep 19, 2017

View reviewed changes

begedin merged commit f5a1d4f into develop Sep 19, 2017

begedin deleted the user-skill-controller-ja-resource branch September 19, 2017 12:39

Conversation

snewcomer commented Sep 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begedin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshsmith commented Sep 13, 2017

Uh oh!

begedin commented Sep 14, 2017

Uh oh!

snewcomer commented Sep 14, 2017

Uh oh!

begedin commented Sep 14, 2017

Uh oh!

joshsmith commented Sep 15, 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!

begedin commented Sep 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants