Looking at generated controller specs we tend to get something like
describe PostsController do
describe "handling GET /posts" do
before(:each) do
@post = mock_model(Post)
Post.stub!(:find).and_return([@post])
end
def do_get
get :index
end
...
it "should find all posts" do
Post.should_receive(:find).with(:all).and_return([@post])
do_get
end
Now this last spec "should find all posts" is nosy is far as
I''m concerned
in that its specifying how the model should get all the posts (i.e. white
box testing) rather than checking the result i.e. that its got @post back.
So now if I change the model so that "all posts" is for example
"all posts
in last year" because there is a new business rule that we don''t
show posts
over a year old then my controller spec fails. Now it seems to me that I
should only have to change my model specs when I make this sort of change,
this implementation details is none of the controllers business
So a couple of things
1) Am I right about this?
2) If so is there a better way to still use the mock for speed but not be
nosy
3) Should the default controller generators be re-written
TIA
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/rspec-users/attachments/20081211/a03cc007/attachment.html>
Looking at this a bit further, it seems in Rails controllers decide how things are got from Models i.e. by calling a class Method. I sort of forgot this for a bit. So with this pattern the testing is correct in reflecting the design of Rails. Still, with my OO purist hat on, it feels like this responsibility should be in the model not in the controller, and that Rails has got it wrong here (fat controllers). Do other ruby frameworks take a different approach? 2008/12/11 Andrew Premdas <apremdas at gmail.com>> Looking at generated controller specs we tend to get something like > > describe PostsController do > describe "handling GET /posts" do > > before(:each) do > @post = mock_model(Post) > Post.stub!(:find).and_return([@post]) > end > > def do_get > get :index > end > ... > > it "should find all posts" do > Post.should_receive(:find).with(:all).and_return([@post]) > do_get > end > > Now this last spec "should find all posts" is nosy is far as I''m concerned > in that its specifying how the model should get all the posts (i.e. white > box testing) rather than checking the result i.e. that its got @post back. > So now if I change the model so that "all posts" is for example "all posts > in last year" because there is a new business rule that we don''t show posts > over a year old then my controller spec fails. Now it seems to me that I > should only have to change my model specs when I make this sort of change, > this implementation details is none of the controllers business > > So a couple of things > > 1) Am I right about this? > 2) If so is there a better way to still use the mock for speed but not be > nosy > 3) Should the default controller generators be re-written > > TIA >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://rubyforge.org/pipermail/rspec-users/attachments/20081211/0639a993/attachment.html>
On Thu, Dec 11, 2008 at 4:33 AM, Andrew Premdas <apremdas at gmail.com> wrote:> >> it "should find all posts" do >> Post.should_receive(:find).with(:all).and_return([@post]) >> do_get >> end >> >> Now this last spec "should find all posts" is nosy is far as I''m concerned >> in that its specifying how the model should get all the posts (i.e. white >> box testing) rather than checking the result i.e. that its got @post back. >> So now if I change the model so that "all posts" is for example "all posts >> in last year" because there is a new business rule that we don''t show posts >> over a year old then my controller spec fails. > >I think this is probably correct as is. When specing a controller, it''s correct BDD to specify how it interacts with other objects. If the controller itself wants all posts in the last year, it''s OK to test that. The fact that only this year''s posts are of interest is a presentation issue - it''s not an essential characteristic of the data. It''s not the model''s job to decide that all clients should get back posts in the last year, and hence that''s what its find method should return. All business rules don''t reside in the model. As soon as you say "we don''t SHOW posts over a year," you''re talking about a presentation rule, not a model rule. Controllers mediate between data and screen - they''re responsible for getting the data to be shown. That code should exist in the controller and be tested there. On the other hand, the controller should not necessarily be responsible for defining what constitutes "all posts in the last year." That should very likely be a named scope. This is all just my humble opinion, of course, and might be utter rubbish in any particular real world situation. ///ark -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://rubyforge.org/pipermail/rspec-users/attachments/20081211/c4b63fb6/attachment.html>
"Mark Wilden" <mark at mwilden.com> writes:> On Thu, Dec 11, 2008 at 4:33 AM, Andrew Premdas <apremdas at gmail.com> wrote: > > it "should find all posts" do > Post.should_receive(:find).with(:all).and_return([@post]) > do_get > end > > Now this last spec "should find all posts" is nosy is far as I''m concerned in that its specifying how the model should get all the > posts (i.e. white box testing) rather than checking the result i.e. that its got @post back. So now if I change the model so that > "all posts" is for example "all posts in last year" because there is a new business rule that we don''t show posts over a year old > then my controller spec fails. > > I think this is probably correct as is. > > When specing a controller, it''s correct BDD to specify how it interacts with other objects. If the controller itself wants all posts in the > last year, it''s OK to test that. The fact that only this year''s posts are of interest is a presentation issue - it''s not an essential > characteristic of the data. It''s not the model''s job to decide that all clients should get back posts in the last year, and hence that''s > what its find method should return. > > All business rules don''t reside in the model. As soon as you say "we don''t SHOW posts over a year," you''re talking about a presentation > rule, not a model rule. Controllers mediate between data and screen - they''re responsible for getting the data to be shown. That code should > exist in the controller and be tested there. > > On the other hand, the controller should not necessarily be responsible for defining what constitutes "all posts in the last year." That > should very likely be a named scope. > > This is all just my humble opinion, of course, and might be utter rubbish in any particular real world situation. > > ///ark > > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-usersI agree with this. You generally don''t want to screw with the meaning of AR::Base.find. That''s really only for situations where you want some uniform, transparent behavior (acts_as_versioned, acts_as_paranoid, etc). The view shouldn''t really care about this stuff either...it should just take a list of posts and display them. It''s up to the controller to ask the model for what it wants to push to the view. So your finder can be something like Post.since(1.year.ago) or whatever. Andrew, I''ve noticed a theme in your posts that suggests you feel you needn''t ever change controller specs. That''s just not true. Models are a distilled representation of the domain, and controllers give meaning to it in the context of the application you''re working on. Models are what your app *is* and controllers are what your app *does*. When you change what your app does, you''re going to have to change some specs and production code along with it. Keep in mind that if you went with a state-based test, you would *still* have to modify the controller spec. It wouldn''t be good enough to expect a post to be displayed. You''d have to create a post with a created_at of over a year ago, and one within the last year, and expect that the former is not shown while the latter is. Also, I''d like to point out that if you *really* wanted Post.find(:all) to only return posts in the last year, you wouldn''t actually have to change the controller spec :) You would just have to write a model spec for that behavior. Pat
Mark, Thanks for your reply. As you say controllers should not be responsible for defining things, but in Rails this is exactly what they do, they formulate queries using class methods like find_by_xxx. Personally I think Rails is somewhat confused or perhaps lax in defining Controller responsibilities compared to many MVC frameworks and particularly to the underlying concept of MVC. As for this years posts being a presentation issue that depends. If the request came from a view i.e. I''d like to see post for 2001 then the controller should pass parameters to the model so it can return the correct things. On the other hand if last years posts represent a specific business concept something perphaps like AuditablePosts, then the model could/should represent this as a seperate resource. In Rails all business rules don''t generally reside in the model, but maybe they should! IMO of course :) 2008/12/11 Mark Wilden <mark at mwilden.com>> On Thu, Dec 11, 2008 at 4:33 AM, Andrew Premdas <apremdas at gmail.com>wrote: > >> >>> it "should find all posts" do >>> Post.should_receive(:find).with(:all).and_return([@post]) >>> do_get >>> end >>> >>> Now this last spec "should find all posts" is nosy is far as I''m >>> concerned in that its specifying how the model should get all the posts >>> (i.e. white box testing) rather than checking the result i.e. that its got >>> @post back. So now if I change the model so that "all posts" is for example >>> "all posts in last year" because there is a new business rule that we don''t >>> show posts over a year old then my controller spec fails. >> >> > I think this is probably correct as is. > > When specing a controller, it''s correct BDD to specify how it interacts > with other objects. If the controller itself wants all posts in the last > year, it''s OK to test that. The fact that only this year''s posts are of > interest is a presentation issue - it''s not an essential characteristic of > the data. It''s not the model''s job to decide that all clients should get > back posts in the last year, and hence that''s what its find method should > return. > > All business rules don''t reside in the model. As soon as you say "we don''t > SHOW posts over a year," you''re talking about a presentation rule, not a > model rule. Controllers mediate between data and screen - they''re > responsible for getting the data to be shown. That code should exist in the > controller and be tested there. > > On the other hand, the controller should not necessarily be responsible for > defining what constitutes "all posts in the last year." That should very > likely be a named scope. > > This is all just my humble opinion, of course, and might be utter rubbish > in any particular real world situation. > > ///ark > > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://rubyforge.org/pipermail/rspec-users/attachments/20081211/5630921a/attachment-0001.html>
On Thu, Dec 11, 2008 at 11:28 AM, Andrew Premdas <apremdas at gmail.com> wrote:> As you say controllers should not be responsible for defining things, but > in Rails this is exactly what they do, they formulate queries using class > methods like find_by_xxx. Personally I think Rails is somewhat confused or > perhaps lax in defining Controller responsibilities compared to many MVC > frameworks and particularly to the underlying concept of MVC.When I was talking about defining things, I meant that the controller should request "this year''s posts," but shouldn''t define what that means (calendar year? fiscal year? which calendar? which timezone?). Put it another way - if there was a button on the view labeled "View this year''s posts", it would be appropriate for the controller to call the this_years_posts method on the model, right? I was just extending that thinking to what the controller displays by default. But it''s not cut and dried. Pat mentioned things like acts_as_paranoid which controllers certainly should not be concerned with.> As for this years posts being a presentation issue that depends. If the > request came from a view i.e. I''d like to see post for 2001 then the > controller should pass parameters to the model so it can return the correct > things.Don''t most requests come from views? I mean, you got to this page via a user gesture of some kind. (Parenthetically, I consider views the most important part of a web app - if the right thing is displayed to the right user at the right time, it doesn''t matter if it''s generated by a herd of monkeys.)> On the other hand if last years posts represent a specific business concept > something perphaps like AuditablePosts, then the model could/should > represent this as a seperate resource.That sounds good. But the controller would still have to decide to display AuditablePosts on that page (and might display UnauditablePosts on another page). Again, it would know what result set - in domain terms - it wants to display. It shouldn''t care about how the model defines that result set. I appreciate the opportunity to grope my way through my opinions on this subject. :) ///ark -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://rubyforge.org/pipermail/rspec-users/attachments/20081211/0d77e5e6/attachment.html>
Pat,
Thanks for reply. The one year ago thing was a really bad example. Using
your terminilogy it looks like a rule that changes what my app does. But I
was talking about a change in what the app is; i.e. a business rule.
I definitely don''t want to have business rules in controllers. From my
viewpoint in MVC the controller is a conduit that allows the business domain
encapsulated by the model to be presented in views and in reverse allows
interaction with views to communicate with the model. In Rails though its
very easy for the business domain to creep into the controller. I''m
beggining to think that Rails got this wrong. Perhaps it should have
specified a restrictive api between the model and the controller, I''ve
been
thinking of something like
get_collection
get_object
get_new_object
By giving completely open access to anything in the model including all
classes its real easy for controllers to get real messy. I think the
emergence of Presenters as well as skinny controllers is symptomatic of that
problem
With a resource based approach the controller really should only be
requesting resources or more accurately representations of resources. It
shouldn''t be creating/specifying representations of resources. That is
outside the responsibility of a controller. Again using your terminology
REST restricts what an application does by using a standard set of verbs to
do things. Now in Rails we can pollute this by putting all sorts of stuff in
our restful methods, but really we shouldn''t. Instead we should use the
expressivness of what ''is'' to get our functionality. If we do
this then our
controllers can be ultra skinny and our controller specs should be much less
brittle. To change what our app ''does'' we then add new routes,
controllers
and views to show a different representation from our model but still using
our standard verbs.
If controllers only use the restrictive api I''ve suggested and we
combine
this with a ''presenter'' layer (when required) on top of our
model. then
controller specs really can be very straightforward and focus on their key
responsibilities which are getting something and controlling i.e. routes.
I''ll try and give a better example. In my business domain I have
products
and some of these products have refills which are also products. Generally
when I want to look at all products I don''t want to see the refills.
This is
because the refills are bought and specified through their product. So using
tags when I want to find ''all'' products I''ll actually
do the following
bizzarre thing
Product.find_tagged_with(''refill'', :exclude =>
''refill'')
Now generally in Rails this will be done in my controllers index method, and
with rspecs scaffold code we''d mock Product and test that this method
was
called the correct way. So as my call has changed from Product.find(:all) to
this new call I have to change my controller spec. Now changing what all
products means has nothing to do with controllers and everything to do with
models. What my controller should have done is called
Product.get_collection. Then when I change my definition of what
get_collection does then yes I have to update my model spec, but that makes
sense, and my controller and its specs remain the same.
I don''t see controller specs as being fixed, but I do think they should
only
need to change when something a controller is responsible for changes.
Generally this is routing - controlling where we go - which is fundamentally
how web applications do things.
Anyhow hope that makes sense and once again thanks for the input much
appreciated
Andrew
2008/12/11 Pat Maddox <pergesu at gmail.com>
> "Mark Wilden" <mark at mwilden.com> writes:
>
> > On Thu, Dec 11, 2008 at 4:33 AM, Andrew Premdas <apremdas at
gmail.com>
> wrote:
> >
> > it "should find all posts" do
> >
Post.should_receive(:find).with(:all).and_return([@post])
> > do_get
> > end
> >
> > Now this last spec "should find all posts" is nosy
is far as I''m
> concerned in that its specifying how the model should get all the
> > posts (i.e. white box testing) rather than checking the result
> i.e. that its got @post back. So now if I change the model so that
> > "all posts" is for example "all posts in last
year" because there
> is a new business rule that we don''t show posts over a year old
> > then my controller spec fails.
> >
> > I think this is probably correct as is.
> >
> > When specing a controller, it''s correct BDD to specify how it
interacts
> with other objects. If the controller itself wants all posts in the
> > last year, it''s OK to test that. The fact that only this
year''s posts are
> of interest is a presentation issue - it''s not an essential
> > characteristic of the data. It''s not the model''s job
to decide that all
> clients should get back posts in the last year, and hence that''s
> > what its find method should return.
> >
> > All business rules don''t reside in the model. As soon as you
say "we
> don''t SHOW posts over a year," you''re talking about
a presentation
> > rule, not a model rule. Controllers mediate between data and screen -
> they''re responsible for getting the data to be shown. That code
should
> > exist in the controller and be tested there.
> >
> > On the other hand, the controller should not necessarily be
responsible
> for defining what constitutes "all posts in the last year." That
> > should very likely be a named scope.
> >
> > This is all just my humble opinion, of course, and might be utter
rubbish
> in any particular real world situation.
> >
> > ///ark
> >
> > _______________________________________________
> > rspec-users mailing list
> > rspec-users at rubyforge.org
> > http://rubyforge.org/mailman/listinfo/rspec-users
>
> I agree with this. You generally don''t want to screw with the
meaning
> of AR::Base.find. That''s really only for situations where you
want some
> uniform, transparent behavior (acts_as_versioned, acts_as_paranoid,
> etc).
>
> The view shouldn''t really care about this stuff either...it should
just
> take a list of posts and display them.
>
> It''s up to the controller to ask the model for what it wants to
push to
> the view. So your finder can be something like Post.since(1.year.ago)
> or whatever.
>
> Andrew, I''ve noticed a theme in your posts that suggests you feel
you
> needn''t ever change controller specs. That''s just not
true. Models
> are a distilled representation of the domain, and controllers give
> meaning to it in the context of the application you''re working on.
> Models are what your app *is* and controllers are what your app *does*.
> When you change what your app does, you''re going to have to change
some
> specs and production code along with it.
>
> Keep in mind that if you went with a state-based test, you would *still*
> have to modify the controller spec. It wouldn''t be good enough to
> expect a post to be displayed. You''d have to create a post with a
> created_at of over a year ago, and one within the last year, and expect
> that the former is not shown while the latter is.
>
> Also, I''d like to point out that if you *really* wanted
Post.find(:all)
> to only return posts in the last year, you wouldn''t actually have
to
> change the controller spec :) You would just have to write a model spec
> for that behavior.
>
> Pat
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/rspec-users/attachments/20081211/cfec0f77/attachment.html>
We''re actually getting pretty close to saying the same thing here,
though my
use of language is causing some confusion. The two sorts of requests I was
talking about where a view request and a request by the business for a
specific rule to be implemented.
If there were AuditablePosts the controller wouldn''t decide what
display it
could decide where to route (well actually the Dispatcher but you get my
drift) because this new resource would have its own controller as its a
seperate business entity.
A controller really shouldn''t be choosing between model entities, you
wouldn''t do this would you?
def index
if params[:auditable]
@posts = AuditablePosts.find(:all)
else
@posts = Posts.find(:all)
end
end
but we often end up with
def index
if params[:auditable]
@posts = Posts.find_auditable
else
@posts = Posts.find(:all)
end
end
which really isn''t much different. You mentioned something like
def index
if params[:auditable]
@posts = Posts.get_auditable_posts
else
@posts = Posts.find(:all)
end
end
which I think is better. However I kind of think we should have
def index
@posts = Posts.get_collection(params)
or have two resources and two controllers, and not have anything inbetween.
Andrew
2008/12/11 Mark Wilden <mark at mwilden.com>
> On Thu, Dec 11, 2008 at 11:28 AM, Andrew Premdas <apremdas at
gmail.com>wrote:
>
>
>> As you say controllers should not be responsible for defining things,
but
>> in Rails this is exactly what they do, they formulate queries using
class
>> methods like find_by_xxx. Personally I think Rails is somewhat confused
or
>> perhaps lax in defining Controller responsibilities compared to many
MVC
>> frameworks and particularly to the underlying concept of MVC.
>
>
> When I was talking about defining things, I meant that the controller
> should request "this year''s posts," but
shouldn''t define what that means
> (calendar year? fiscal year? which calendar? which timezone?). Put it
> another way - if there was a button on the view labeled "View this
year''s
> posts", it would be appropriate for the controller to call the
> this_years_posts method on the model, right? I was just extending that
> thinking to what the controller displays by default. But it''s not
cut and
> dried. Pat mentioned things like acts_as_paranoid which controllers
> certainly should not be concerned with.
>
>
>> As for this years posts being a presentation issue that depends. If the
>> request came from a view i.e. I''d like to see post for 2001
then the
>> controller should pass parameters to the model so it can return the
correct
>> things.
>
>
> Don''t most requests come from views? I mean, you got to this page
via a
> user gesture of some kind. (Parenthetically, I consider views the most
> important part of a web app - if the right thing is displayed to the right
> user at the right time, it doesn''t matter if it''s
generated by a herd of
> monkeys.)
>
>
>> On the other hand if last years posts represent a specific business
>> concept something perphaps like AuditablePosts, then the model
could/should
>> represent this as a seperate resource.
>
>
> That sounds good. But the controller would still have to decide to display
> AuditablePosts on that page (and might display UnauditablePosts on another
> page). Again, it would know what result set - in domain terms - it wants to
> display. It shouldn''t care about how the model defines that result
set.
>
> I appreciate the opportunity to grope my way through my opinions on this
> subject. :)
>
> ///ark
>
>
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/rspec-users/attachments/20081211/43bcf5f6/attachment.html>
"Andrew Premdas" <apremdas at gmail.com> writes:> We''re actually getting pretty close to saying the same thing here, though my use of language is causing some confusion. The two sorts of > requests I was talking about where a view request and a request by the business for a specific rule to be implemented. > > If there were AuditablePosts the controller wouldn''t decide what display it could decide where to route (well actually the Dispatcher but > you get my drift) because this new resource would have its own controller as its a seperate business entity. > > A controller really shouldn''t be choosing between model entities, you wouldn''t do this would you? > > def index > if params[:auditable] > @posts = AuditablePosts.find(:all) > else > @posts = Posts.find(:all) > end > endI don''t see why not. In fact, something I like to do is def index @posts = repository.find :all end def repository params[:auditable] ? Post.auditable : Post end :O Makes a lot of sense to me. The controller''s basic job is to get some stuff from the model and give it to the view. If the "get some stuff" requires parameterization, fine by me. Of course, there are plenty of ways you can structure your code. You can use query params (and take advantage of routes to make it look like a clean URL...hey, you''re still RESTful!) or a new action (again, still RESTful), or create a whole new controller for it. That''s another reason why REST is sweet from web app development perspective - stick to a consistent URL scheme, and you can refactor the app beneath it however you want. Pat
"Andrew Premdas" <apremdas at gmail.com> writes:> Pat, > > Thanks for reply. The one year ago thing was a really bad > example. Using your terminilogy it looks like a rule that changes what > my app does. But I was talking about a change in what the app is; > i.e. a business rule. > > I definitely don''t want to have business rules in controllers. From my > viewpoint in MVC the controller is a conduit that allows the business > domain encapsulated by the model to be presented in views and in > reverse allows interaction with views to communicate with the > model. In Rails though its very easy for the business domain to creep > into the controller. I''m beggining to think that Rails got this > wrong. Perhaps it should have specified a restrictive api between the > model and the controller, I''ve been thinking of something like > > get_collection > get_object > get_new_objectYou do.... find(:all), find(id) and new(attributes)> By giving completely open access to anything in the model including > all classes its real easy for controllers to get real messy. I think > the emergence of Presenters as well as skinny controllers is > symptomatic of that problemOh. I''m a software libertarian. Arguments for locking stuff down are NEVER going to fly with me. Look, Ruby is a very powerful language. As Spiderman teaches us, with great power comes great responsibility. Just because stuff can get messy doesn''t mean we need to lock down the language or framework. It means we need to be good developers, inventing useful conventions and abstractions. Do you know the secret to having a clean controller? Don''t write one! Use something like make_resourceful.> I''ll try and give a better example. In my business domain I have > products and some of these products have refills which are also > products. Generally when I want to look at all products I don''t want > to see the refills. This is because the refills are bought and > specified through their product. So using tags when I want to find > ''all'' products I''ll actually do the following bizzarre thing > > Product.find_tagged_with(''refill'', :exclude => ''refill'') > > Now generally in Rails this will be done in my controllers index > method, and with rspecs scaffold code we''d mock Product and test that > this method was called the correct way. So as my call has changed from > Product.find(:all) to this new call I have to change my controller > spec.Well, yeah. You''re asking for a different thing now. You need to verify that somehow. Either you use mock objects to expect a different method call, or you write an expectation that you should see one product and not another.> Now changing what all products means has nothing to do with > controllers and everything to do with models. What my controller > should have done is called Product.get_collection. Then when I change > my definition of what get_collection does then yes I have to update my > model spec, but that makes sense, and my controller and its specs > remain the same.If you really, really want that, why not just change Product.find(:all) to return something different? Same thing. I don''t like it, but same thing :) I think you''re overlooking the fact that all of this stuff must occur in context. As in "show me all the products" is a straightforward statement, yes, but is missing some info. As you yourself have stated, "all" can mean something different than "every single record which exists in our system." The clearest way to represent these contextual differences, in my experience, is to create an intention-revealing method and use that. Pat
2008/12/11 Pat Maddox <pergesu at gmail.com> writes> "Andrew Premdas" <apremdas at gmail.com> writes: > > > Pat, > > > > Thanks for reply. The one year ago thing was a really bad > > example. Using your terminilogy it looks like a rule that changes what > > my app does. But I was talking about a change in what the app is; > > i.e. a business rule. > > > > I definitely don''t want to have business rules in controllers. From my > > viewpoint in MVC the controller is a conduit that allows the business > > domain encapsulated by the model to be presented in views and in > > reverse allows interaction with views to communicate with the > > model. In Rails though its very easy for the business domain to creep > > into the controller. I''m beggining to think that Rails got this > > wrong. Perhaps it should have specified a restrictive api between the > > model and the controller, I''ve been thinking of something like > > > > get_collection > > get_object > > get_new_object > > You do.... find(:all), find(id) and new(attributes) > > > > By giving completely open access to anything in the model including > > all classes its real easy for controllers to get real messy. I think > > the emergence of Presenters as well as skinny controllers is > > symptomatic of that problem > > Oh. I''m a software libertarian. Arguments for locking stuff down are > NEVER going to fly with me. > > Look, Ruby is a very powerful language. As Spiderman teaches us, with > great power comes great responsibility. Just because stuff can get > messy doesn''t mean we need to lock down the language or framework. It > means we need to be good developers, inventing useful conventions and > abstractions. >I''m looking to find better conventions (for me) to use the power of rails responsibly. But probably I''m betraying my Java/C++ heritage by using the word rules instead of convention In the design of any web framework and in Rails lots of rules/conventions exist about what you can do and where you can do things. For example Rails is quite happy to restrict what you can access in views by default.> > Do you know the secret to having a clean controller? Don''t write one! > Use something like make_resourceful. > > Thats exactly what I''m looking at. The trouble with these is that the howof object retrieval ends up buried in ''magic'' code somewhere, so when you need to change things you end up having to overload something somewhat obscure in the controller, and then having to test this overloading occurs in a controller spec. For example using resource_controller and the example below I''d end up with something like class ProductsController < ResourceController::Base private def collection @collection ||end_of_association_chain.find_tagged_with(''refill'', :exclude => ''refill'') end end end This sort of code isn''t very intuitive and (for me) it feels as though its in the wrong place> > > I''ll try and give a better example. In my business domain I have > > products and some of these products have refills which are also > > products. Generally when I want to look at all products I don''t want > > to see the refills. This is because the refills are bought and > > specified through their product. So using tags when I want to find > > ''all'' products I''ll actually do the following bizzarre thing > > > > Product.find_tagged_with(''refill'', :exclude => ''refill'') > > > > Now generally in Rails this will be done in my controllers index > > method, and with rspecs scaffold code we''d mock Product and test that > > this method was called the correct way. So as my call has changed from > > Product.find(:all) to this new call I have to change my controller > > spec. > > Well, yeah. You''re asking for a different thing now. You need to > verify that somehow. Either you use mock objects to expect a different > method call, or you write an expectation that you should see one product > and not another. > > > > Now changing what all products means has nothing to do with > > controllers and everything to do with models. What my controller > > should have done is called Product.get_collection. Then when I change > > my definition of what get_collection does then yes I have to update my > > model spec, but that makes sense, and my controller and its specs > > remain the same. > > If you really, really want that, why not just change Product.find(:all) > to return something different? Same thing. I don''t like it, but same > thing :)Because I don''t like it either its really ugly, and its far to clever for me> > I think you''re overlooking the fact that all of this stuff must occur in > context. As in "show me all the products" is a straightforward > statement, yes, but is missing some info. As you yourself have stated, > "all" can mean something different than "every single record which > exists in our system." The clearest way to represent these contextual > differences, in my experience, is to create an intention-revealing > method and use that. >The point I''m making is that this intention should be expressed clearly in the model, whereas currently its expressed in the controller.> Pat > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users >Thanks for providing lots of input, its been most interesting Andrew -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://rubyforge.org/pipermail/rspec-users/attachments/20081212/2ff439ad/attachment-0001.html>
yeah, instead of using Post.find(:all) in your controller, use a method like Post.all_posts, or some names like that. Then in the controller spec, you go: Post.should_receive(:all_posts) and leave the implementation detail of this method to the Post model. Technically speaking, this all_posts more belongs to Service layer though. Yi On Thu, Dec 11, 2008 at 5:48 AM, Andrew Premdas <apremdas at gmail.com> wrote:> Looking at generated controller specs we tend to get something like > > describe PostsController do > describe "handling GET /posts" do > > before(:each) do > @post = mock_model(Post) > Post.stub!(:find).and_return([@post]) > end > > def do_get > get :index > end > ... > > it "should find all posts" do > Post.should_receive(:find).with(:all).and_return([@post]) > do_get > end > > Now this last spec "should find all posts" is nosy is far as I''m concerned > in that its specifying how the model should get all the posts (i.e. white > box testing) rather than checking the result i.e. that its got @post back. > So now if I change the model so that "all posts" is for example "all posts > in last year" because there is a new business rule that we don''t show posts > over a year old then my controller spec fails. Now it seems to me that I > should only have to change my model specs when I make this sort of change, > this implementation details is none of the controllers business > > So a couple of things > > 1) Am I right about this? > 2) If so is there a better way to still use the mock for speed but not be > nosy > 3) Should the default controller generators be re-written > > TIA > > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://rubyforge.org/pipermail/rspec-users/attachments/20081211/1ab9ce27/attachment.html>
"Andrew Premdas" <apremdas at gmail.com> writes:> 2008/12/11 Pat Maddox <pergesu at gmail.com> writes >> The clearest way to represent these contextual differences, in my >> experience, is to create an intention-revealing method and use that. > > The point I''m making is that this intention should be expressed > clearly in the model, whereas currently its expressed in the > controller.What? No. I said create methods with intention-revealing names. That means stuff like Post.since or Account.audited or Product.non_refillable. That absolutely is expressing the intent in the model, and calling it from the controller, which is what you want. Pat
I was agreeing! I wasn''t saying that what you were saying there wasn''t expressing the intent in the model, I was saying that my long ramble was saying the same thing roughly, and that this differs from the standard approach in rails of using ActiveRecord:Base class methods directly. Apologies for not making that clear - I find email such a challenging form of communication at times :) Andrew 2008/12/12 Pat Maddox <pergesu at gmail.com>> "Andrew Premdas" <apremdas at gmail.com> writes: > > 2008/12/11 Pat Maddox <pergesu at gmail.com> writes > >> The clearest way to represent these contextual differences, in my > >> experience, is to create an intention-revealing method and use that. > > > > The point I''m making is that this intention should be expressed > > clearly in the model, whereas currently its expressed in the > > controller. > > What? No. I said create methods with intention-revealing names. That > means stuff like Post.since or Account.audited or > Product.non_refillable. That absolutely is expressing the intent in the > model, and calling it from the controller, which is what you want. > > Pat > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://rubyforge.org/pipermail/rspec-users/attachments/20081212/34efd61f/attachment-0001.html>