New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix current_page? helper issue with engine root path #29503

Merged
merged 2 commits into from Jun 20, 2017

Conversation

Projects
None yet
6 participants
@savroff
Contributor

savroff commented Jun 19, 2017

Summary

Solving for #29401.
I just figure out that we need to clean request_uri as well as url_string.

@rails-bot

This comment has been minimized.

rails-bot commented Jun 19, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@rafaelfranca

Looks good to me. Could you add a changelog entry?

@@ -15,6 +15,10 @@ class UrlHelperTest < ActiveSupport::TestCase
get "/other" => "foo#other"
get "/article/:id" => "foo#article", :as => :article
get "/category/:category" => "foo#category"
scope :engine do
get "/" => "foo#bar"

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 19, 2017

Member

Could you explain how this is testing what happens in a real engine and why we need to clear request_uri to make it work?

This comment has been minimized.

@savroff

savroff Jun 19, 2017

Contributor

basically, with scope in routes, we will duplicate situation

# routes.rb
  mount MyEngine::Engine => '/my_engine'

Problem is that here we will get url_string == '/engine' and will try to compare it to request_uri == '/engine/'.
I jump around stack, but could not figure out where exactly we get this difference.
This is why I make fix in easy way.

This comment has been minimized.

@JanBussieck

JanBussieck Jun 20, 2017

Contributor

An even simpler way would be to ensure that we do not chomp('/') the url_string passed in by the user if the request_uri ends with '/', otherwise we'd introduce the inequality which we later on check.
So as I mentioned in #29401 I think it would be easiest to change this line
url_string.chomp!("/") if url_string.start_with?("/") && url_string != "/"
to this line:
url_string.chomp!("/") if url_string.start_with?("/") && !request_uri.end_with? "/"

This comment has been minimized.

@savroff

savroff Jun 20, 2017

Contributor

@JanBussieck I tried this, this will not solve the problem with scope, the test will still fail. We will get request_uri eq to /engine/ and in your case, we will still get false, because '/engine' != '/engine/'. Or else you need to force people to write in helper always like this:

current_page?('/engine/')

And it's easy to fail this :)
I'm usually doing something like this:

current_page?('/engine')

@savroff savroff force-pushed the savroff:fix_current_page_engine_issue branch Jun 19, 2017

actionview/CHANGELOG.md Outdated
@@ -1,3 +1,10 @@
* `chomp!` both `url_string` and `request_uri` to fix issue with scopes and engines

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 19, 2017

Member

Better to say:

Fix issues with scopes and engine on current_page? method.

This comment has been minimized.

@savroff

savroff Jun 19, 2017

Contributor

@rafaelfranca thank you! Fill fix in a second.
First pull request, a little bit nerveus 😟

@savroff savroff force-pushed the savroff:fix_current_page_engine_issue branch to 01ddd1c Jun 19, 2017

@savroff

This comment has been minimized.

Contributor

savroff commented Jun 20, 2017

@rafaelfranca

  • Added change to CHANGELOG
  • Tests are green

Maybe I rename route in tests to something more generic like scope :blog or we are good to go here?

@rafaelfranca rafaelfranca merged commit 95ee77b into rails:master Jun 20, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Jun 20, 2017

Merge pull request #29503 from savroff/fix_current_page_engine_issue
Fix current_page? helper issue with engine root path

@savroff savroff deleted the savroff:fix_current_page_engine_issue branch Jun 20, 2017

@dependabot dependabot bot referenced this pull request Mar 14, 2018

Closed

Bump rails from 4.2.9 to 5.1.5 #73

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Mar 18, 2018

Fix "Ruby on Rails 5.2 Release Notes" [ci skip]
- Remove
  ```
  *   Namespace error pages' CSS selectors to stop the styles from bleeding
      into other pages when using Turbolinks.
      ([Pull Request](rails#28814))
  ```
  since it was backported to
  `5-1-stable` by 50d5baf
  and
  `5-0-stable` by d1c4a39.

- Remove
  ```
  *   Allow irb options to be passed from `rails console` command.
      ([Pull Request](rails#29010))
  ```
  since it was backported to
  `5-1-stable` by e91b483.

- Remove
  ```
  *   Load environment file in `dbconsole` command.
      ([Pull Request](rails#29725))
  ```
  since it was backported to
  `5-1-stable` by 7f93428.

- Remove
  ```
  *   Gemfile for new apps: upgrade redis-rb from ~> 3.0 to 4.0.
      ([Pull Request](rails#30748))
  ```
  since it was backported to
  `5-1-stable` by 3789531.

- Remove
  ```
  *   Fix minitest rails plugin.
      The custom reporters are added only if needed.
      This will fix conflicts with others plugins.
      ([Commit](rails@ac99916))
  ```
  since it was backported to
  `5-1-stable` by caa7695.

- Remove
  ```
  *   Add support for compatibility with redis-rb gem for 4.0 version.
      ([Pull Request](rails#30748))
  ```
  since it was backported to
  `5-1-stable` by 3789531.

- Remove
  ```
  *   Add `action_controller_api` and `action_controller_base` load hooks to be
      called in `ActiveSupport.on_load`.
      ([Pull Request](rails#28402))
  ```
  since it was backported to
  `5-1-stable` by b9a5fd7.

- Remove
  ```
  *   `driven_by` now registers poltergeist and capybara-webkit.
      ([Pull Request](rails#29315))
  ```
  since it was backported to
  `5-1-stable` by c5dd451.

- Remove
  ```
  *   Fallback `ActionController::Parameters#to_s` to `Hash#to_s`.
      ([Pull Request](rails#29630))
  ```
  since it was backported to
  `5-1-stable` by c1014e4
  and
  `5-0-stable` by 0e71fc3.

- Remove
  ```
  *   Make `take_failed_screenshot` work within engine.
      ([Pull Request](rails#30421))
  ```
  since it was backported to
  `5-1-stable` by 595a231.

- Remove
  ```
  *   Fix optimized url helpers when using relative url root.
      ([Pull Request](rails#31261))
  ```
  since it was backported to
  `5-1-stable` by e9b7795.

- Remove
  ```
  *   Update `distance_of_time_in_words` helper to display better error messages
      for bad input.
      ([Pull Request](rails#20701))
  ```
  since it was backported to
  `5-1-stable` by 2c97fbf.

- Remove
  ```
  *   Generate field ids in `collection_check_boxes` and
      `collection_radio_buttons`.
      ([Pull Request](rails#29412))
  ```
  since it was backported to
  `5-1-stable` by 2d8c10a.

- Remove
  ```
  *   Fix issues with scopes and engine on `current_page?` method.
      ([Pull Request](rails#29503))
  ```
  since it was backported to
  `5-1-stable` by 2135daf.

- Remove
  ```
  *   Bring back proc with arity of 1 in `ActionMailer::Base.default` proc
      since it was supported in Rails 5.0 but not deprecated.
      ([Pull Request](rails#30391))
  ```
  since it was backported to
  `5-1-stable` by b2bedb1.

- Remove
  ```
  *   Add type caster to `RuntimeReflection#alias_name`.
      ([Pull Request](rails#28961))
  ```
  since it was backported to
  `5-1-stable` by f644e7a.

- Remove
  ```
  *   Loading model schema from database is now thread-safe.
      ([Pull Request](rails#29216))
  ```
  since it was backported to
  `5-1-stable` by 02926cf.
  and
  `5-0-stable` by 84bcfe5

- Remove
  ```
  *   Fix destroying existing object does not work well when optimistic locking
      enabled and `locking_column` is null in the database.
      ([Pull Request](rails#28926))
  ```
  since it was backported to
  `5-1-stable` by e498052.

- Remove
  ```
  *   `ActiveRecord::Persistence#touch` does not work well
      when optimistic locking enabled and `locking_column`,
      without default value, is null in the database.
      ([Pull Request](rails#28914))
  ```
  since it was backported to
  `5-1-stable` by 1e2f63d.

- Remove
  ```
  *   Previously, when building records using a `has_many :through` association,
      if the child records were deleted before the parent was saved,
      they would still be persisted. Now, if child records are deleted
      before the parent is saved on a `has_many :through` association,
      the child records will not be persisted.
      ([Pull Request](rails#29593))
  ```
  since it was backported to
  `5-1-stable` by a22c39e.

- Remove
  ```
  *   Query cache was unavailable when entering the `ActiveRecord::Base.cache`
      block without being connected.
      ([Pull Request](rails#29609))
  ```
  since it was backported to
  `5-1-stable` by fd6c8cd
  and
  `5-0-stable` by 9f2532b.

- Remove
  ```
  *   `Relation#joins` is no longer affected by the target model's
      `current_scope`, with the exception of `unscoped`.
      ([Commit](rails@5c71000))
  ```
  since it was backported to
  `5-1-stable` by 3630d63.

- Remove
  ```
  *   Fix `unscoped(where: [columns])` removing the wrong bind values.
      ([Pull Request](rails#29780))
  ```
  since it was backported to
  `5-1-stable` by d378fcb.

- Remove
  ```
  *   When a `has_one` association is destroyed by `dependent: destroy`,
      `destroyed_by_association` will now be set to the reflection, matching the
      behaviour of `has_many` associations.
      ([Pull Request](rails#29855))
  ```
  since it was backported to
  `5-1-stable` by 8254a8b.

- Remove
  ```
  *   Fix `COUNT(DISTINCT ...)` with `ORDER BY` and `LIMIT`
      to keep the existing select list.
      ([Pull Request](rails#29848))
  ```
  since it was backported to
  `5-1-stable` by 0e8d4ed.

- Remove
  ```
  *   Ensure `sum` honors `distinct` on `has_many :through` associations.
      ([Commit](rails@566f1fd))
  ```
  since it was backported to
  `5-1-stable` by c0a1dc2.

- Remove
  ```
  *   Fix `COUNT(DISTINCT ...)` for `GROUP BY` with `ORDER BY` and `LIMIT`.
      ([Commit](rails@5668dc6))
  ```
  since it was backported to
  `5-1-stable` by 87ca68e.

- Remove
  ```
  *   MySQL: Don't lose `auto_increment: true` in the `db/schema.rb`.
      ([Commit](rails@9493d45))
  ```
  since it was backported to
  `5-1-stable` by 8b6e694.

- Remove
  ```
  *   Fix longer sequence name detection for serial columns.
      ([Pull Request](rails#28339))
  ```
  since it was backported to
  `5-1-stable` by af9c170
  and
  `5-0-stable` by 7025b1d.

- Remove
  ```
  *   Fix `bin/rails db:setup` and `bin/rails db:test:prepare` create wrong
      ar_internal_metadata's data for a test database.
      ([Pull Request](rails#30579))
  ```
  since it was backported to
  `5-1-stable` by bb67b5f
  and
  `5-0-stable` by 60437e6.

- Remove
  ```
  *   Fix conflicts `counter_cache` with `touch: true` by optimistic locking.
      ([Pull Request](rails#31405))
  ```
  since it was backported to
  `5-1-stable` by 5236dda.

- Remove
  ```
  *   Fix `count(:all)` to correctly work `distinct` with custom SELECT list.
      ([Commit](rails@c6cd9a5))
  ```
  since it was backported to
  `5-1-stable` by 6beb4de.

- Remove
  ```
  *   Fix to invoke callbacks when using `update_attribute`.
      ([Commit](rails@732aa34))
  ```
  since it was backported to
  `5-1-stable` by 6346683.

- Remove
  ```
  *   Use `count(:all)` in `HasManyAssociation#count_records` to prevent invalid
      SQL queries for association counting.
      ([Pull Request](rails#27561))
  ```
  since it was backported to
  `5-1-stable` by eef3c89.

- Remove
  ```
  *   Fix `count(:all)` with eager loading and having an order other than
      the driving table.
      ([Commit](rails@ebc09ed))
  ```
  since it was backported to
  `5-1-stable` by 6df9b69.

- Remove
  ```
  *   PostgreSQL: Allow pg-1.0 gem to be used with Active Record.
      ([Pull Request](rails#31671))
  ```
  since it was backported to
  `5-1-stable` by a9c06f6.

- Remove
  ```
  *   Fix that after commit callbacks on update does not triggered
      when optimistic locking is enabled.
      ([Commit](rails@7f9bd03))
  ```
  since it was backported to
  `5-1-stable` by aaee10e.

- Remove
  ```
  *   Fix regression in numericality validator when comparing Decimal and Float
      input values with more scale than the schema.
      ([Pull Request](rails#28584))
  ```
  since it was backported to
  `5-1-stable` by 5b1c3e5.
  Note that there was incorrect link to PR,
  original PR is rails#29249.

- Remove
  ```
  *   Fix to working before/after validation callbacks on multiple contexts.
      ([Pull Request](rails#31483))
  ```
  since it was backported to
  `5-1-stable` by 0f7046a.

- Remove
  ```
  *   Fix implicit coercion calculations with scalars and durations.
      ([Pull Request](rails#29163),
      [Pull Request](rails#29971))
  ```
  since it was backported to
  `5-1-stable` by 51ea27c,
                  4d82e2a.

- Remove
  ```
  *   Fix modulo operations involving durations.
      ([Commit](rails@a54e13b))
  ```
  since it was backported to
  `5-1-stable` by 233fa7e.

- Remove
  ```
  *   Return all mappings for a timezone identifier in `country_zones`.
      ([Commit](rails@cdce6a7))
  ```
  since it was backported to
  `5-1-stable` by 0222ebb.

- Remove
  ```
  *   Add support for compatibility with redis-rb gem for 4.0 version.
      ([Pull Request](rails#30748))
  ```
  since it was backported to
  `5-1-stable` by 3789531.
  Related to rails#32252.

Related to rails#32222, rails#32222 (comment).
Follow up a489cc8.
@jerome-diver

This comment has been minimized.

jerome-diver commented Jul 28, 2018

one year after, current_page?() failed again on rails-5.2.
It failed to give back false. Instead it give bacjk an error about route not match (to bad...).

@savroff

This comment has been minimized.

Contributor

savroff commented Jul 28, 2018

@jerome-diver sorry :) I tried.

Can you show me an example of a simple app?

@jerome-diver

This comment has been minimized.

jerome-diver commented Jul 29, 2018

@savroff yes sure...

rails new exemple
cd exemple
vim Gemfile
# # (i and add haml genfile, then <esc> :wq)
bundle
rails g scaffold Test title:string content:text
rails db:migrate
vim app/views/tests/_partial.haml
# # (i)
# - if current_page?(controller: :tests, action: :edit)
#   %p edit page
# - else
#   %p BAM ! error no match route instead of false when open from show page
# (and <esc> :wq)
vim app/views/test/show.html.haml # (i don't know why, but generator provide ext .html.haml instead of simply .haml... it is not really a bug, but something complicate for nothing more)
# # (i)
# Et voilà, ça va bugger ferme...
# = render partial: "test/partial"
# # ( <esc> :wq
vim config/routes.rb
# # check for presence of
# resources :tests
rails routes (for be sure)
rails s

now create a test record in your database the way you like
let's go inside tests/show/1 route...

if you want also, i can give you the link of my own web site who is a public code in bitbucket ?
tell me....

@eileencodes

This comment has been minimized.

Member

eileencodes commented Jul 30, 2018

I'm unable to reproduce this in a brand new test app using Rails 5.2.0. I didn't use haml however and used erb. If it's only reproducible using haml it's likely a problem with that gem than rails.

If you are able to reproduce it in an app without using haml please open a new issue and provide an application that reproduces the error.

@jerome-diver

This comment has been minimized.

jerome-diver commented Jul 31, 2018

@savroff sorry, i've spoken to quick and i missed that it can also be a kind of border interaction problem from an other one gem or my own code...
@eileencodes yes correct a test case simply show it works, i created a test case application:
my public test case for current_page? use
where part of this code is inside app/view/posts/_form.html.haml

but that works fine there. I also try with sassc-rails instead of sass-rails
By the time, i have some more gem to check for... maybe devise make things turn bad (i'm thinking that because of route devise own code inside routes.rb file, but i may be wrong).
here is my own rails-5.2 public app for my futur web site where problem is happening with this current_page? code:
my public web site to come

Thank you for try to help me on this. I think that if it is not my own code and if it is in relation with an other one gem, we should be able to contribute to increase compatibility/quality of other open source gem codes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment