Rails Ruby on Rails

Comments and changes to this ticket

• 

  Rizwan Reza May 21st, 2010 @ 12:46 AM

  • no changes were found...
• 

  Jeff Kreeftmeijer May 22nd, 2010 @ 10:49 AM

  Verified on master.

  EventsController#index only returns true for the first one:

  current_page?(:controller => 'events', :action => 'index')       # => true
  current_page?(:controller => 'admin/events', :action => 'index') # => false
  

  Admin::EventsController#index returns true for both:

  current_page?(:controller => 'events', :action => 'index')       # => true
  current_page?(:controller => 'admin/events', :action => 'index') # => true
  

  This definitely shouldn't be the case if you ask me.

• You flagged this item as spam.
  

  Jonathan Greenberg May 23rd, 2010 @ 05:56 AM

  I was able to reproduce this as well.

  I started digging into the code and tests a bit but got stuck when I found that the current_page? tests stub out the controller#url_for method making it hard to write an accurate test.

  I am somehow guessing that this might be a tricky bug to solve in the end without rewriting the helper method because the helper currently is calling that controller method which probably is supposed to accommodate namespaced routes in this manner.

  This is my first attempt at cracking a rails bug so if anyone has any suggestions about how to approach solving this bug let me know.

• You flagged this item as spam.
  

  Jonathan Greenberg May 24th, 2010 @ 05:26 AM

  So I have a possible simple fix but I am unsure of my implementation and the correct way to adapt the tests if my approach is acceptable. On master it could look like this:

    def current_page?(options)
      unless request
        raise "You cannot use helpers that need to determine the current " \
              "page unless your view context provides a Request object " \
              "in a #request method"
      end 
  
      return false if options[:controller] && controller.controller_path != options[:controller]
  
      url_string = CGI.unescapeHTML(url_for(options)) 
      ....
  

  This seems to return correct results using the circumstances described above when run with a server; however, the tests blow up. I get a little lost with how the current url_helper_test.rb mocks requests and what would be the best way to keep those test passing.

  I had hoped to submit this as a patch but did not feel comfortable without passing tests.

  I would appreciate the insight of anyone with more depth of experience with this part of rails and its tests.

• You flagged this item as spam.
  

  Curtis Edmond May 27th, 2010 @ 07:01 PM

  The problem seems to stem from the "use_relative_controller!" method in the RouteSet class. It basically takes into account the url segments separated by "/" and appends them to the passed :controller if the passed :controller doesn't have them. Allowing us to say url_for(:controller => 'events', :action => 'index') to get "/admin/events" if the current controller we're in is "admin/events".

  I like Jonathan's proposed fix since it stays out of that use_relative_controller! method. I attached a patch for it which includes tests.

  • 4649_update_current_page_for_namespaced_controllers.diff 3.2 KB

• You flagged this item as spam.
  

  Lake September 16th, 2010 @ 09:17 PM

  Hey Curtis,

  Your patch doesn't apply cleanly to master.

  I changed the patch to make it apply cleanly. I would post it here, but two tests are failing once the patch has been applied.

  I'm still looking into the failing tests.

  Thanks for your help.

• You flagged this item as spam.
  

  Curtis Edmond September 16th, 2010 @ 09:32 PM

  Ok, thanks for the heads up. I'll try this against trunk and see what I can find.

• 

  Santiago Pastorino February 2nd, 2011 @ 04:31 PM

  • State changed from “new” to “open”

  This issue has been automatically marked as stale because it has not been commented on for at least three months.

  The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

  Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

• 

  Santiago Pastorino February 2nd, 2011 @ 04:31 PM

  • State changed from “open” to “stale”

• You flagged this item as spam.
  

  bingbing March 26th, 2011 @ 02:07 AM

  louis vuitton watches