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 incorrect output from rails routes when using singular resources … #27089

Merged
merged 1 commit into from Nov 18, 2016

Conversation

Projects
None yet
6 participants
@erickueen

erickueen commented Nov 17, 2016

Related Issue #26606
Rails routes was giving an incorrect output when a single resources was defined in routes.rb An example output is the following:

     profile POST   /profile(.:format)                                 profiles#create
 new_profile GET    /profile/new(.:format)                             profiles#new
edit_profile GET    /profile/edit(.:format)                            profiles#edit
             GET    /profile(.:format)                                 profiles#show
             PATCH  /profile(.:format)                                 profiles#update
             PUT    /profile(.:format)                                 profiles#update
             DELETE /profile(.:format)                                 profiles#destroy

Now it gives the expected output:

      Prefix Verb   URI Pattern             Controller#Action
 new_profile GET    /profile/new(.:format)  profiles#new
edit_profile GET    /profile/edit(.:format) profiles#edit
     profile GET    /profile(.:format)      profiles#show
             PATCH  /profile(.:format)      profiles#update
             PUT    /profile(.:format)      profiles#update
             DELETE /profile(.:format)      profiles#destroy
             POST   /profile(.:format)      profiles#create
@rails-bot

This comment has been minimized.

rails-bot commented Nov 17, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @senny (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

Can you add a test case for this?

actionpack/CHANGELOG.md Outdated
@@ -1,11 +1,14 @@
* Fixes multiple calls to `logger.fatal` instead of a single call,
for every line in an exception backtrace, when printing trace
* Fixes Incorrect output from rails routes when using singular resources

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 17, 2016

Member

Missing . in the end of line

actionpack/CHANGELOG.md Outdated
* Fixes multiple calls to `logger.fatal` instead of a single call,
for every line in an exception backtrace, when printing trace
* Fixes Incorrect output from rails routes when using singular resources
Fixes #26606

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 17, 2016

Member

blank line before this line

actionpack/CHANGELOG.md Outdated
for every line in an exception backtrace, when printing trace
* Fixes Incorrect output from rails routes when using singular resources
Fixes #26606
*Erick Reyna*

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 17, 2016

Member

blank line before an after this line

actionpack/lib/action_dispatch/routing/mapper.rb Outdated
@@ -1266,15 +1266,16 @@ def resource(*resources, &block)
concerns(options[:concerns]) if options[:concerns]
collection do
post :create
end if parent_resource.actions.include?(:create)

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 17, 2016

Member

✂️ this blank line

@erickueen

This comment has been minimized.

erickueen commented Nov 18, 2016

@rafaelfranca done :) I added a test here:
railties/test/application/rake/single_resource_test.rb

@rafaelfranca

Sounds great. I added some comments to your changes. After you do them could you squash your commits?

railties/test/application/rake/single_resource_test.rb Outdated
@@ -0,0 +1,30 @@
require "isolation/abstract_unit"
require "active_support/core_ext/string/strip"
class RakeTest < ActiveSupport::TestCase

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 18, 2016

Member

Missing blank line before this.

railties/test/application/rake/single_resource_test.rb Outdated
require "active_support/core_ext/string/strip"
class RakeTest < ActiveSupport::TestCase
include ActiveSupport::Testing::Isolation
def setup

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 18, 2016

Member

missing blank line before this.

railties/test/application/rake/single_resource_test.rb Outdated
def teardown
teardown_app
end
def test_26606

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 18, 2016

Member

missing blank line before this.

railties/test/application/rake/single_resource_test.rb Outdated
def teardown
teardown_app
end
def test_26606

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 18, 2016

Member

Can you write a better test name for this? Maybe test_singular_resource_output_in_rake_routes

output = Dir.chdir(app_path) { `bin/rails routes -c PostController` }
assert_equal expected_output, output
end
end

This comment has been minimized.

@kaspth

kaspth Nov 18, 2016

Member

Why create a separate test file for this? Seems it would fit just fine in the application rake test file below?

This comment has been minimized.

@erickueen

erickueen Nov 18, 2016

Well I was requested to make the test, I can remove it if you want, The rake test bellow tests routes with a namespaced controller and mine just test the routes generated with a resource.
I agree that both test are the same, but I was requested to make the specific test, please let me know if I have to erase it.

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 18, 2016

Member

When I asked to add a test case the test at railties/test/application/rake_test.rb were not changed yet. Now that it was I think we can remove this new file, but I can do it since you already had many rounds of reviews 😄

This comment has been minimized.

@erickueen

erickueen Nov 18, 2016

Great sounds good for me :D thank you.

@@ -1,11 +1,17 @@
* Fixes multiple calls to `logger.fatal` instead of a single call,
for every line in an exception backtrace, when printing trace
* Fixes Incorrect output from rails routes when using singular resources.

This comment has been minimized.

@kaspth

kaspth Nov 18, 2016

Member

"Removes excessive whitespace in rails routes..." would better illustrate what the fix entailed :)

This comment has been minimized.

@erickueen

erickueen Nov 18, 2016

Actually it changes the order of the output. I made a mistake writing "fix whitespaces" in a commit, i wanted to say that I erased some blank lines of a previous commit.

@erickueen erickueen force-pushed the erickueen:erickueen_fix_26606 branch Nov 18, 2016

Erick Reyna
Fix incorrect output from rails routes when using singular resources …
…issue #26606

Rails routes (even rake routes in previous versions) output showed incorrect routes when an application use resource :controller, implying that edit_controller_path match with controller#show.
The order of the output has changed to correct this. View #26606 for more information.

Added a test case, change unit test in rake to expect the new output.
Since the output of resource :controller is changing, the string spected of the railties/test/application/rake_test.rb test_rails_routes_with_controller_environment had to be modified.

@erickueen erickueen force-pushed the erickueen:erickueen_fix_26606 branch to c79848e Nov 18, 2016

@erickueen

This comment has been minimized.

erickueen commented Nov 18, 2016

@rafaelfranca Ready :) I made the changes and squashed the commits.

rafaelfranca added a commit that referenced this pull request Nov 18, 2016

Merge pull request #27089 from erickueen/erickueen_fix_26606
Fix incorrect output from rails routes when using singular resources …
@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Nov 18, 2016

Backported in ef1e903

@rafaelfranca rafaelfranca merged commit c79848e into rails:master Nov 18, 2016

2 checks passed

codeclimate Code Climate didn't find any 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 Nov 18, 2016

Merge pull request #27089 from erickueen/erickueen_fix_26606
Fix incorrect output from rails routes when using singular resources …
@kaspth

This comment has been minimized.

Member

kaspth commented Nov 19, 2016

❤️

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Nov 19, 2016

Add missing test for singular resource output in rake routes
- This test was present in rails#27089
  but not present on master, may be removed in merge commit?
- There was discussion about moving this to `application/rake_test` so
  may be this happened in merge commit.
- rails#27089 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment