This project is archived and is in readonly mode.

#308 ✓resolved
Gabe da Silveira

Fix assert_redirected_to for nested controllers

Reported by Gabe da Silveira | June 3rd, 2008 @ 12:08 AM

Hey Koz,

This is a forward port of the fix you applied for me on on the 2.0-stable branch. I applied it to current HEAD and tests are passing. Have you made any more progress on your refactoring of assert_redirected_to? Let me know and I'll help you QA it.

Comments and changes to this ticket

  • Repository

    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

    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...

  • Andrew Vit

    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...

  • Andrew Vit

    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.

  • Gabe da Silveira

    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.

  • Andrew Vit

    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.

  • Michael Koziarski

    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 :)

  • Andrew Vit

    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!

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages