Skip to content
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 route matcher accepts a format as a symbol or string #693

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/shoulda/matchers/action_controller/route_matcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ module ActionController
#
# route(:get, '/posts/1').to('posts#show', id: 1)
#
# You may also specify special parameters such as `:format`:
#
# route(:get, '/posts').to('posts#index', format: :json)
#
# @return [RouteMatcher]
#
def route(method, path)
Expand Down
21 changes: 15 additions & 6 deletions lib/shoulda/matchers/action_controller/route_params.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ module Matchers
module ActionController
# @private
class RouteParams
PARAMS_TO_SYMBOLIZE = %i{ format }

def initialize(args)
@args = args
end
Expand All @@ -26,17 +28,24 @@ def controller_and_action_given_as_string?
def extract_params_from_string
controller, action = args[0].split('#')
params = (args[1] || {}).merge(controller: controller, action: action)
stringify_values(params)
normalize_values(params)
end

def stringify_params
stringify_values(args[0])
normalize_values(args[0])
end

def stringify_values(hash)
hash.inject({}) do |hash_copy, (key, value)|
hash_copy[key] = stringify(value)
hash_copy
def normalize_values(hash)
hash.each_with_object({}) do |(key, value), hash_copy|
hash_copy[key] = symbolize_or_stringify(key, value)
end
end

def symbolize_or_stringify(key, value)
if key.in?(PARAMS_TO_SYMBOLIZE)
value.to_sym
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works obviously, although I wonder if it makes a little more sense to say "leave this value alone if the key is :format". In other words change this line to just value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the value.to_sym if we want accept the format as string or symbol.

Since we accept controller and action as string and symbols, I think is a good idea we accept the format as string and symbols too. What you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could go either way. This is fine.

I wish there were a better way to say this actually. The only reason we stringify is for the params I believe, the other "special" params like controller, action, format, etc. don't need to be touched. Ah well.

else
stringify(value)
end
end

Expand Down
37 changes: 37 additions & 0 deletions spec/unit/shoulda/matchers/action_controller/route_matcher_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,26 @@
to(action: 'show', some: 'other', params: 'here')
end
end

context 'when route has a default format' do
it 'accepts' do
expect(controller_with_defined_routes).
to route(:post, "/#{controller_path}").
to(action: 'create', format: 'json')
end

it 'accepts when format is specified as a symbol' do
expect(controller_with_defined_routes).
to route(:post, "/#{controller_path}").
to(action: 'create', format: :json)
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you leave the format out? Seems we need a test for that too.

expect(controller_with_defined_routes).
  to route(:post, "/#{controller_path}").
  to(action: 'create')


it 'rejects when format is unspecified' do
expect(controller_with_defined_routes).
not_to route(:post, "/#{controller_path}").
to(action: 'create')
end
end
end

context 'when controller and action are specified as a joined string' do
Expand All @@ -64,6 +84,20 @@
to("#{controller_path}#show", id: 1)
end
end

context 'when route has the format' do
it 'accepts' do
expect(controller_with_defined_routes).
to route(:post, "/#{controller_path}").
to("#{controller_path}#create", format: 'json')
end

it 'rejects when format is unspecified' do
expect(controller_with_defined_routes).
not_to route(:post, "/#{controller_path}").
to(action: 'create')
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here, seems we need to test that it rejects if format is not specified.

end

def controller_with_defined_routes
Expand All @@ -76,6 +110,9 @@ def controller_with_defined_routes
define_routes do
get "/#{_controller_path}", to: "#{_controller_path}#index"
get "/#{_controller_path}/:id", to: "#{_controller_path}#show"
post "/#{_controller_path}",
to: "#{_controller_path}#create",
defaults: { format: :json }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align the elements of a hash literal if they span more than one line.

end

controller
Expand Down