Add wager controller #14

sbatson5 · 2016-12-19T22:09:19Z

Add create endpoint
Add get endpoint for checking for previous bets
Add show endpoint for movie-round
Restrict wager to current user
tests tests tests

sbatson5 · 2016-12-29T20:16:00Z

Going to punt on Restrict wager to current user. Authentication on other controllers can be a separate PR

SViccari

Nice work! I like the test helpers very much 👍 Left a few comments and one larger idea around error messaging that can always be done later :)

SViccari · 2016-12-30T01:42:21Z

test/controllers/wager_controller_test.exs

+        |> get(wager_path(conn, :index, %{user_id: user.id, movie_round_id: movie_round.id}))
+        |> json_response(200)
+
+      assert length(resp["data"]) == 1


Another way to assert that the expected record, and only the expected record, is returned is collect all the record ids in the response. Ex:

Enum.map(resp["data"], fn(map) -> Map.get(map, "id") end)

I believe that will return an array of ids, so then you could give the function a name and write something like:

assert ids_from_response(resp) == [wager.id]

SViccari · 2016-12-30T01:44:31Z

test/controllers/wager_controller_test.exs

+
+  describe "POST create" do
+    test "it creates and returns a valid wager", %{conn: conn} do
+      wager_params = %{amount: 5, place: nil}


Is the inclusion of place important?

SViccari · 2016-12-30T01:47:49Z

test/controllers/wager_controller_test.exs

+      wager_params = %{amount: 1000}
+
+      wager = insert(:wager)
+


👾 Looks like an extra space that could be removed unless this is a personal style preference.

SViccari · 2016-12-30T01:52:27Z

test/controllers/wager_controller_test.exs

+
+      resp = conn
+        |> put(wager_path(conn, :update, wager.id), json)
+        |> json_response(200)


I think it's worth adding an assertion that confirms the returned wager has the updated amount. You can also explicitly set the amount when creating the wager, e.g. wager = insert(:wager, amount: 50) to avoid any false positives if factory values are altered.

SViccari · 2016-12-30T02:14:03Z

test/models/wager_test.exs

@@ -3,7 +3,7 @@ defmodule MovieWagerApi.WagerTest do

  alias MovieWagerApi.Wager

-  @valid_attrs %{amount: 42, place: 42}
+  @valid_attrs %{amount: 42, place: 42, user_id: 50, movie_round_id: 55}


In lieu of listing out the attributes yourself, there is the handy ex_machina function params_with_assocs(:wager), which is like params_for but it also sets values for the belongs_to relationships.

Then I'll need to add ExMachina to my ModelCase, for this one method. 🤔 not sure if it is overkill

I don't believe you need to make any changes to use params_with_assocs as the ModelCase file is importing the factory.ex file, and the factory.ex file calls use ExMachina.Ecto, making params_with_assocs available within your tests.

🤔 Then I must have missed something, as this wasn't working. Oh well, will get it right the next time around

hm, what error message are you seeing? Adding changeset = Wager.changeset(%Wager{}, params_with_assocs(:wager)) to the first test in the wager model doesn't return an error for me 🤔

SViccari · 2016-12-30T02:15:49Z

test/support/factory.ex

@@ -3,7 +3,7 @@ defmodule MovieWagerApi.Factory do

  def movie_round_factory do
    %MovieWagerApi.MovieRound{
-      code: "Monkey",
+      code: sequence(:code, &"Monkey #{&1}: Die Harder"),


What did monkeys ever do to you?? 😢 🐒 🙈

SViccari · 2016-12-30T02:22:18Z

test/support/test_helpers.ex

@@ -1,5 +1,16 @@
 defmodule MovieWagerApi.TestHelpers do
  import Plug.Conn, only: [put_session: 3, fetch_session: 1]
+  import ExUnit.Assertions
+
+  def assert_jsonapi_relationship(json = %{"relationships" => relationships}, relationship_name, id) do


Just curious, is this function in use? I suspect only the function below is being used.

SViccari · 2016-12-30T03:41:02Z

web/controllers/wager_controller.ex

+  end
+
+  def index(conn, %{"user_id" => user_id, "movie_round_id" => movie_round_id}) do
+    query = from(w in Wager, where: [user_id: ^user_id], where: [movie_round_id: ^movie_round_id])


This is more of a style preference on my part, but what do you think of:

wagers = Wager |> where(user_id: ^user_id) |> where(movie_round_id: ^movie_round_id) |> Repo.all |> preload() serialized_wager(conn, wagers, 200)

On second thought, perhaps you can remove the preload() here, as it looks like serialized_wager/3 will call preload().

SViccari · 2016-12-30T05:00:44Z

web/controllers/wager_controller.ex

+        serialized_wager(conn, wager, :created)
+      {:error, changeset} ->
+        if changeset.errors[:user_movie_round] do
+          send_resp(conn, :unprocessable_entity, "You have already created a bet for this round")


Since what you have works, the following is more food for thought as it can be done later, in a separate PR :)

If you would like to set custom messages for certain errors, another way to do so is to use set the message field on the model validation. For ex:

|> unique_constraint(:user_movie_round, name: :wagers_user_movie_round_idx, message: "You have already created a bet for this round")

To include these error messages in the serialized response, you'll need to add a web/views/changeset_view.ex file. Using the mix command mix phoenix.gen.json generates this file for you, but it also generates everything else a new model would need, so it's probably easiest to just copy the file.

Once that's created, the "failed to insert" case could be:

{:error, changeset} -> conn |> put_status(:unprocessable_entity) |> render(MovieWagerApi.ChangesetView, "error.json", changeset: changeset)

which is nice because then all error messages will be returned rather than the catch-all message of "invalid entry".

This does change the placement of the error message, as it will no longer be a string within "resp_body" but it will be within an errors object (ex: resp_body: "{\"errors\":{\"user_movie_round\":[\"You have already created a bet for this round\"]}}"), which means the test assertion would change to:

resp = conn |> post(wager_path(conn, :create), json) |> json_response(422) assert resp["errors"]["user_movie_round"] == ["You have already created a bet for this round"]

Just a thought for future development to lessen the need for case statements to set attribute specific error messages :) And if I explained any of this poorly, just let me know!

SViccari · 2016-12-30T05:07:28Z

web/controllers/wager_controller.ex

+    end
+  end
+
+  defp serialized_wager(conn, wager, status) do


add controller test add relationships as required fields update wager model test add helper for asserting relationship ids

add index query for wagers by user/movie add wager controller/index test address PR comments

add helpers for putting relationships in json

7a12a98

sbatson5 force-pushed the add-wager-controller branch 5 times, most recently from 30dfc39 to f25aa53 Compare December 26, 2016 17:44

sbatson5 added the Work in progress label Dec 27, 2016

sbatson5 removed the Work in progress label Dec 29, 2016

sbatson5 changed the title ~~[WIP] - Add wager controller~~ Add wager controller Dec 29, 2016

SViccari reviewed Dec 30, 2016

View reviewed changes

web/controllers/wager_controller.ex

end

end

defp serialized_wager(conn, wager, status) do

Copy link

SViccari Dec 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 👍

sbatson5 force-pushed the add-wager-controller branch from d6e5daa to 292ab61 Compare December 30, 2016 19:59

Scott Batson added 4 commits January 2, 2017 15:50

add create and update endpoint

599e2d6

add controller test add relationships as required fields update wager model test add helper for asserting relationship ids

add error for non-unique bet

16261a9

add update test for wager controller

3414f45

add a show/:id endpoint for movie_rounds

b2fb5af

add index query for wagers by user/movie add wager controller/index test address PR comments

sbatson5 force-pushed the add-wager-controller branch from 292ab61 to b2fb5af Compare January 2, 2017 20:50

sbatson5 merged commit 0da4071 into master Jan 2, 2017

sbatson5 deleted the add-wager-controller branch January 2, 2017 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wager controller #14

Add wager controller #14

sbatson5 commented Dec 19, 2016 •

edited

Loading

sbatson5 commented Dec 29, 2016

SViccari left a comment

SViccari Dec 30, 2016

SViccari Dec 30, 2016

SViccari Dec 30, 2016

SViccari Dec 30, 2016

SViccari Dec 30, 2016

sbatson5 Dec 30, 2016

SViccari Jan 2, 2017

sbatson5 Jan 2, 2017

SViccari Jan 3, 2017

SViccari Dec 30, 2016

SViccari Dec 30, 2016

SViccari Dec 30, 2016

SViccari Dec 30, 2016

SViccari Dec 30, 2016

SViccari Dec 30, 2016

Add wager controller #14

Add wager controller #14

Conversation

sbatson5 commented Dec 19, 2016 • edited Loading

sbatson5 commented Dec 29, 2016

SViccari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbatson5 commented Dec 19, 2016 •

edited

Loading