Rails Ruby on Rails

Comments and changes to this ticket

• 

  Repository June 4th, 2008 @ 12:10 AM

  • State changed from “new” to “resolved”

  (from [55e5219f9912860abdb28636749c0cb2d8b6a809]) Fix assert_redirected_to for nested controllers and named routes

  [#308 state:resolved]

  Signed-off-by: Michael Koziarski

  http://github.com/rails/rails/co...

• 

  Repository June 4th, 2008 @ 12:10 AM

  (from [025515b234d8380f195e3de3818d076302605b12]) Fix assert_redirected_to for nested controllers and named routes

  [#308 state:resolved]

  Signed-off-by: Michael Koziarski

  http://github.com/rails/rails/co...

• You flagged this item as spam.
  

  Andrew Vit July 9th, 2008 @ 05:08 AM

  • Tag set to “actionpack, patch”

  This patch looks like it breaks existing documented functionality of assert_redirected_to. It's now too fussy about getting exact parameters in order to match a route. The method should just match the route segments that are provided, and ignore other params that are omitted.

  It looks like this patch makes some basic tests impossible:

  def test_create_should_redirect_to_edit
    post :create, valid_form_input
    assert_redirected_to :action => 'edit'
  end
  
  ## Result:
  # No route matches {:action=>"edit"}
  

  The route requires an id in order to match, but we can't guess the id of the new record...

• You flagged this item as spam.
  

  Andrew Vit July 9th, 2008 @ 05:15 AM

  It looks like this patch modified existing tests to specify the controller wherever it was omitted. For the record, I did try adding the controller in my own tests, but that doesn't fix the test failures where other params are needed.

• You flagged this item as spam.
  

  Gabe da Silveira July 9th, 2008 @ 05:20 AM

  Looking at this right now. Hit me on #rails-contrib if you want to talk about this.

• You flagged this item as spam.
  

  Andrew Vit July 9th, 2008 @ 06:52 AM

  Gabe, it doesn't look like your patch attached here is the culprit, because the whole thing has since been rewritten... See ticket #c3aaba01

  A comment on the rescue block in the previous code said:

  rescue ActionController::RoutingError # routing failed us, so match the strings only.
  

  But the new code that Koz wrote normalizes into strings straightaway, instead of diffing the requested route segments like before.

  I'm out of my league with this. Not sure if this should be a new ticket or if it's still related to this fix... I've attached a test which shows the problem I described above.

  • test_assert_redirected_to.diff 964 Bytes
• 

  Michael Koziarski July 9th, 2008 @ 08:32 AM

  Hey there,

  definitely open a new ticket with some examples that failed.

  The test cases I changed in the previous ticket were passing not only due to the partial matching, but due to regular route expiry rules. i.e. url_for :action=>'index' implicitly defaults to this controller.

  I'm not sure that the previous behaviour makes sense to me, but if enough people feel strongly about it we can check it out.

  So yeah, another ticket with a failing example which doesn't involve components and we can take a look :)

• You flagged this item as spam.
  

  Andrew Vit July 9th, 2008 @ 07:13 PM

  The follow-up ticket is #584. The test I attached to that ticket is the same as here, I stayed out of the nested modules ("components?").

  Thanks!