yev2000/myflix — pull request #13 by yev2000 (2014-12-04 06:21:47+00:00)
Title: Beyond mvc

Please review the following code, which
* implements a decorator for the Video model to provide an average rating string
* implements a UserCreation service to take care of the logic for creating a new user that is invoked by the UsersController#create action.

I have some questions about the approach because my initial stab was different than the solution.
Here are some specific places for the code reviewer to examine and comment on whether what I'm doing makes sense:
`users_controller_spec.rb:149` -- I am explicitly stipulating here what argument types are expected by the contructor of the UserCreation class. Is that too much specificity for the test? My concern in this respect is that if I don't put such specifics in the controller test, the controller is so decoupled from the service as far as what's being tested, that it's possible for the controller spec to pass and the service spec to pass while the actual implementation is broken because (for example) it's passing the wrong type of arguments to the constructor.

`users_controller_spec.rb:157` and `users_controller_spec.rb:167` -- here I am explicitly setting the requirement that the successful? and error_message methods of the UserCreation class must be invoked as part of the logic of the UsersController. Is this too much detail or making the test too brittle?

`users_controller_spec.rb:79 -- this `it` clause looks like a "runt". But I can't write it better because the actual assertion appears in the `before` block on line 75. That's because the "should_receive" must be included before the actual post. But I am trying to factor out all of the "should" logic because it's used in multiple `it` blocks. So the block on line 79 looks like it's not actually asserting anything even though in reality it is. Is that bad form?

In my implementation of the user creation logic, I implicitly log the user in as soon as they successfully register. I was able to test for this in the original spec for UsersController. However, now that the UserCreation service is handling the user creation, and it's being mocked out in the new user_controller_spec, I'm faced with a problem - the setting of the user id in the session can only happen if the user is saved. But the save happens in the service. Since I'm using a mock, the actual @user.save can no longer happen when running the code in the user_controller_spec. So I'm forced to pull out that test and rely on feature test to confirm that the user is logged in. Is that a reasonable approach? Or is this an example of where I cannot fully mock out the UserCreation service in the controller spec?