diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index 081abbd11a..e1a62c8713 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,5 +1,11 @@ *SVN* +* Modernize scaffolding to match the generator: use the new render method and change style from the warty @params["id"] to the sleek params[:id]. #1367 + +* Include :id in the action generated by the form helper method. Then, for example, the controller can do Model.find(params[:id]) for both edit and update actions. Updated scaffolding to take advantage. #1367 + +* Add assertions with friendly messages to TestCase#process to ensure that @controller, @request, and @response are set. #1367 + * Arrays sent via multipart posts are converted to strings #1032 [dj@omelia.org] * render(:layout => true) is a synonym for render(:layout => nil) diff --git a/actionpack/lib/action_controller/scaffolding.rb b/actionpack/lib/action_controller/scaffolding.rb index 4faa23cdf3..3435ce5108 100644 --- a/actionpack/lib/action_controller/scaffolding.rb +++ b/actionpack/lib/action_controller/scaffolding.rb @@ -27,12 +27,12 @@ module ActionController # end # # def show - # @entry = Entry.find(@params["id"]) + # @entry = Entry.find(params[:id]) # render_scaffold # end # # def destroy - # Entry.find(@params["id"]).destroy + # Entry.find(params[:id]).destroy # redirect_to :action => "list" # end # @@ -42,9 +42,9 @@ module ActionController # end # # def create - # @entry = Entry.new(@params["entry"]) + # @entry = Entry.new(params[:entry]) # if @entry.save - # flash["notice"] = "Entry was successfully created" + # flash[:notice] = "Entry was successfully created" # redirect_to :action => "list" # else # render_scaffold('new') @@ -52,17 +52,17 @@ module ActionController # end # # def edit - # @entry = Entry.find(@params["id"]) + # @entry = Entry.find(params[:id]) # render_scaffold # end # # def update - # @entry = Entry.find(@params["entry"]["id"]) - # @entry.attributes = @params["entry"] + # @entry = Entry.find(params[:id]) + # @entry.attributes = params[:entry] # # if @entry.save - # flash["notice"] = "Entry was successfully updated" - # redirect_to :action => "show/" + @entry.id.to_s + # flash[:notice] = "Entry was successfully updated" + # redirect_to :action => "show", :id => @entry # else # render_scaffold('edit') # end @@ -84,9 +84,9 @@ module ActionController def scaffold(model_id, options = {}) validate_options([ :class_name, :suffix ], options.keys) - singular_name = model_id.id2name - class_name = options[:class_name] || Inflector.camelize(singular_name) - plural_name = Inflector.pluralize(singular_name) + singular_name = model_id.to_s + class_name = options[:class_name] || singular_name.camelize + plural_name = singular_name.pluralize suffix = options[:suffix] ? "_#{singular_name}" : "" unless options[:suffix] @@ -104,12 +104,12 @@ module ActionController end def show#{suffix} - @#{singular_name} = #{class_name}.find(@params["id"]) + @#{singular_name} = #{class_name}.find(params[:id]) render#{suffix}_scaffold end def destroy#{suffix} - #{class_name}.find(@params["id"]).destroy + #{class_name}.find(params[:id]).destroy redirect_to :action => "list#{suffix}" end @@ -119,9 +119,9 @@ module ActionController end def create#{suffix} - @#{singular_name} = #{class_name}.new(@params["#{singular_name}"]) + @#{singular_name} = #{class_name}.new(params[:#{singular_name}]) if @#{singular_name}.save - flash["notice"] = "#{class_name} was successfully created" + flash[:notice] = "#{class_name} was successfully created" redirect_to :action => "list#{suffix}" else render#{suffix}_scaffold('new') @@ -129,17 +129,17 @@ module ActionController end def edit#{suffix} - @#{singular_name} = #{class_name}.find(@params["id"]) + @#{singular_name} = #{class_name}.find(params[:id]) render#{suffix}_scaffold end def update#{suffix} - @#{singular_name} = #{class_name}.find(@params["#{singular_name}"]["id"]) - @#{singular_name}.attributes = @params["#{singular_name}"] + @#{singular_name} = #{class_name}.find(params[:id]) + @#{singular_name}.attributes = params[:#{singular_name}] if @#{singular_name}.save - flash["notice"] = "#{class_name} was successfully updated" - redirect_to :action => "show#{suffix}", :id => @#{singular_name}.id.to_s + flash[:notice] = "#{class_name} was successfully updated" + redirect_to :action => "show#{suffix}", :id => @#{singular_name} else render#{suffix}_scaffold('edit') end @@ -148,7 +148,7 @@ module ActionController private def render#{suffix}_scaffold(action = caller_method_name(caller)) if template_exists?("\#{self.class.controller_path}/\#{action}") - render_action(action) + render(:action => action) else @scaffold_class = #{class_name} @scaffold_singular_name, @scaffold_plural_name = "#{singular_name}", "#{plural_name}" @@ -156,10 +156,15 @@ module ActionController add_instance_variables_to_assigns @content_for_layout = @template.render_file(scaffold_path(action.sub(/#{suffix}$/, "")), false) - self.active_layout ? render_file(self.active_layout, "200 OK", true) : render_file(scaffold_path("layout")) + + if active_layout? + render :file => active_layout, :use_full_path => true + else + render :file => scaffold_path("layout") + end end end - + def scaffold_path(template_name) File.dirname(__FILE__) + "/templates/scaffolds/" + template_name + ".rhtml" end diff --git a/actionpack/lib/action_controller/templates/scaffolds/edit.rhtml b/actionpack/lib/action_controller/templates/scaffolds/edit.rhtml index 31e15f7501..63dff602a1 100644 --- a/actionpack/lib/action_controller/templates/scaffolds/edit.rhtml +++ b/actionpack/lib/action_controller/templates/scaffolds/edit.rhtml @@ -1,7 +1,7 @@

Editing <%= @scaffold_singular_name %>

<%= error_messages_for(@scaffold_singular_name) %> -<%= form(@scaffold_singular_name, :action => "update" + @scaffold_suffix) %> +<%= form(@scaffold_singular_name, :action => "update#{@scaffold_suffix}") %> -<%= link_to "Show", :action => "show#{@scaffold_suffix}", :id => instance_variable_get("@#{@scaffold_singular_name}").id %> | +<%= link_to "Show", :action => "show#{@scaffold_suffix}", :id => instance_variable_get("@#{@scaffold_singular_name}") %> | <%= link_to "Back", :action => "list#{@scaffold_suffix}" %> diff --git a/actionpack/lib/action_controller/templates/scaffolds/layout.rhtml b/actionpack/lib/action_controller/templates/scaffolds/layout.rhtml index 300aa1d77a..80996985d2 100644 --- a/actionpack/lib/action_controller/templates/scaffolds/layout.rhtml +++ b/actionpack/lib/action_controller/templates/scaffolds/layout.rhtml @@ -28,40 +28,40 @@ #ErrorExplanation { width: 400px; - border: 2px solid 'red'; - padding: 7px; - padding-bottom: 12px; - margin-bottom: 20px; - background-color: #f0f0f0; + border: 2px solid 'red'; + padding: 7px; + padding-bottom: 12px; + margin-bottom: 20px; + background-color: #f0f0f0; } #ErrorExplanation h2 { - text-align: left; - font-weight: bold; - padding: 5px 5px 5px 15px; - font-size: 12px; - margin: -7px; - background-color: #c00; - color: #fff; + text-align: left; + font-weight: bold; + padding: 5px 5px 5px 15px; + font-size: 12px; + margin: -7px; + background-color: #c00; + color: #fff; } #ErrorExplanation p { - color: #333; - margin-bottom: 0; - padding: 5px; + color: #333; + margin-bottom: 0; + padding: 5px; } #ErrorExplanation ul li { - font-size: 12px; - list-style: square; + font-size: 12px; + list-style: square; } -

<%= flash['notice'] %>

+

<%= flash[:notice] %>

<%= @content_for_layout %> - \ No newline at end of file + diff --git a/actionpack/lib/action_controller/templates/scaffolds/list.rhtml b/actionpack/lib/action_controller/templates/scaffolds/list.rhtml index 233e9da87d..d1196a1599 100644 --- a/actionpack/lib/action_controller/templates/scaffolds/list.rhtml +++ b/actionpack/lib/action_controller/templates/scaffolds/list.rhtml @@ -12,9 +12,9 @@ <% for column in @scaffold_class.content_columns %> <%= entry.send(column.name) %> <% end %> - <%= link_to "Show", :action => "show#{@scaffold_suffix}", :id => entry.id %> - <%= link_to "Edit", :action => "edit#{@scaffold_suffix}", :id => entry.id %> - <%= link_to "Destroy", :action => "destroy#{@scaffold_suffix}", :id => entry.id, :confirm => "Are you sure?" %> + <%= link_to "Show", :action => "show#{@scaffold_suffix}", :id => entry %> + <%= link_to "Edit", :action => "edit#{@scaffold_suffix}", :id => entry %> + <%= link_to "Destroy", :action => "destroy#{@scaffold_suffix}", :id => entry, :confirm => "Are you sure?" %> <% end %> diff --git a/actionpack/lib/action_controller/templates/scaffolds/new.rhtml b/actionpack/lib/action_controller/templates/scaffolds/new.rhtml index 74db8468bd..66f6626f4e 100644 --- a/actionpack/lib/action_controller/templates/scaffolds/new.rhtml +++ b/actionpack/lib/action_controller/templates/scaffolds/new.rhtml @@ -1,6 +1,6 @@

New <%= @scaffold_singular_name %>

<%= error_messages_for(@scaffold_singular_name) %> -<%= form(@scaffold_singular_name, :action => "create" + @scaffold_suffix) %> +<%= form(@scaffold_singular_name, :action => "create#{@scaffold_suffix}") %> -<%= link_to "Back", :action => "list#{@scaffold_suffix}" %> \ No newline at end of file +<%= link_to "Back", :action => "list#{@scaffold_suffix}" %> diff --git a/actionpack/lib/action_controller/templates/scaffolds/show.rhtml b/actionpack/lib/action_controller/templates/scaffolds/show.rhtml index 10c46342fd..46cdfdb493 100644 --- a/actionpack/lib/action_controller/templates/scaffolds/show.rhtml +++ b/actionpack/lib/action_controller/templates/scaffolds/show.rhtml @@ -5,5 +5,5 @@

<% end %> -<%= link_to "Edit", :action => "edit#{@scaffold_suffix}", :id => instance_variable_get("@#{@scaffold_singular_name}").id %> | +<%= link_to "Edit", :action => "edit#{@scaffold_suffix}", :id => instance_variable_get("@#{@scaffold_singular_name}") %> | <%= link_to "Back", :action => "list#{@scaffold_suffix}" %> diff --git a/actionpack/lib/action_controller/test_process.rb b/actionpack/lib/action_controller/test_process.rb index 25ad540468..20022aab91 100644 --- a/actionpack/lib/action_controller/test_process.rb +++ b/actionpack/lib/action_controller/test_process.rb @@ -247,6 +247,11 @@ module Test private # execute the request and set/volley the response def process(action, parameters = nil, session = nil, flash = nil) + # Sanity check for required instance variables so we can give an understandable error message. + %w(controller request response).each do |iv_name| + assert_not_nil instance_variable_get("@#{iv_name}"), "@#{iv_name} is nil: make sure you set it in your test's setup method." + end + @html_document = nil @request.env['REQUEST_METHOD'] ||= "GET" @request.action = action.to_s diff --git a/actionpack/lib/action_view/helpers/active_record_helper.rb b/actionpack/lib/action_view/helpers/active_record_helper.rb index f8a003e948..deb3effb1c 100644 --- a/actionpack/lib/action_view/helpers/active_record_helper.rb +++ b/actionpack/lib/action_view/helpers/active_record_helper.rb @@ -56,20 +56,22 @@ module ActionView # form << content_tag("b", "Department") # form << collection_select("department", "id", @departments, "id", "name") # end - def form(record_name, options = nil) - options = (options || {}).symbolize_keys - record = instance_eval("@#{record_name}") + def form(record_name, options = {}) + record = instance_variable_get("@#{record_name}") + options = options.symbolize_keys options[:action] ||= record.new_record? ? "create" : "update" - action = url_for(:action => options[:action]) + action = url_for(:action => options[:action], :id => record) submit_value = options[:submit_value] || options[:action].gsub(/[^\w]/, '').capitalize - id_field = record.new_record? ? "" : InstanceTag.new(record_name, "id", self).to_input_field_tag("hidden") + contents = '' + contents << hidden_field(record_name, :id) unless record.new_record? + contents << all_input_tags(record, record_name, options) + yield contents if block_given? + contents << submit_tag(submit_value) - formtag = %(
#{id_field}) + all_input_tags(record, record_name, options) - yield formtag if block_given? - formtag + %(
) + content_tag('form', contents, :action => action, :method => 'post') end # Returns a string containing the error message attached to the +method+ on the +object+, if one exists. @@ -83,8 +85,8 @@ module ActionView # <%= error_message_on "post", "title", "Title simply ", " (or it won't work)", "inputError" %> => #
Title simply can't be empty (or it won't work)
def error_message_on(object, method, prepend_text = "", append_text = "", css_class = "formError") - if errors = instance_eval("@#{object}").errors.on(method) - "
#{prepend_text + (errors.is_a?(Array) ? errors.first : errors) + append_text}
" + if errors = instance_variable_get("@#{object}").errors.on(method) + content_tag("div", "#{prepend_text}#{errors.is_a?(Array) ? errors.first : errors}#{append_text}", :class => css_class) end end @@ -96,7 +98,7 @@ module ActionView # * class - The class of the error div (default: errorExplanation) def error_messages_for(object_name, options = {}) options = options.symbolize_keys - object = instance_eval "@#{object_name}" + object = instance_variable_get("@#{object_name}") unless object.errors.empty? content_tag("div", content_tag( @@ -117,7 +119,7 @@ module ActionView end def default_input_block - Proc.new { |record, column| "


#{input(record, column.name)}

" } + Proc.new { |record, column| %(


#{input(record, column.name)}

) } end end diff --git a/actionpack/test/template/active_record_helper_test.rb b/actionpack/test/template/active_record_helper_test.rb index 5ee7221913..8a4e45b65f 100644 --- a/actionpack/test/template/active_record_helper_test.rb +++ b/actionpack/test/template/active_record_helper_test.rb @@ -4,6 +4,7 @@ require File.dirname(__FILE__) + '/../../lib/action_view/helpers/form_helper' require File.dirname(__FILE__) + '/../../lib/action_view/helpers/text_helper' require File.dirname(__FILE__) + '/../../lib/action_view/helpers/tag_helper' require File.dirname(__FILE__) + '/../../lib/action_view/helpers/url_helper' +require File.dirname(__FILE__) + '/../../lib/action_view/helpers/form_tag_helper' # require File.dirname(__FILE__) + '/../../lib/action_view/helpers/active_record_helper' class ActiveRecordHelperTest < Test::Unit::TestCase @@ -12,6 +13,7 @@ class ActiveRecordHelperTest < Test::Unit::TestCase include ActionView::Helpers::TextHelper include ActionView::Helpers::TagHelper include ActionView::Helpers::UrlHelper + include ActionView::Helpers::FormTagHelper Post = Struct.new("Post", :title, :author_name, :body, :secret, :written_on) Post.class_eval do @@ -33,6 +35,7 @@ class ActiveRecordHelperTest < Test::Unit::TestCase end def @post.new_record?() true end + def @post.to_param() nil end def @post.column_for_attribute(attr_name) Post.content_columns.select { |column| column.name == attr_name }.first @@ -46,12 +49,12 @@ class ActiveRecordHelperTest < Test::Unit::TestCase @post.secret = 1 @post.written_on = Date.new(2004, 6, 15) - @controller = Class.new do - def url_for(options, *parameters_for_method_reference) - options[:action] || options["action"] - end + @controller = Object.new + def @controller.url_for(options, *parameters_for_method_reference) + options = options.symbolize_keys + + [options[:action], options[:id].to_param].compact.join('/') end - @controller = @controller.new end def test_generic_input_tag @@ -76,7 +79,17 @@ class ActiveRecordHelperTest < Test::Unit::TestCase def test_form_with_string assert_equal( - %(


\n


), + %(


\n


), + form("post") + ) + + class << @post + def new_record?() false end + def to_param() id end + def id() 1 end + end + assert_equal( + %(


\n


), form("post") ) end @@ -85,7 +98,7 @@ class ActiveRecordHelperTest < Test::Unit::TestCase def Post.content_columns() [ Column.new(:date, "written_on", "Written on") ] end assert_equal( - %(


\n\n\n

), + %(


\n\n\n

), form("post") ) end @@ -95,7 +108,7 @@ class ActiveRecordHelperTest < Test::Unit::TestCase @post.written_on = Time.gm(2004, 6, 15, 16, 30) assert_equal( - %(


\n\n\n — \n : \n

), + %(


\n\n\n — \n : \n

), form("post") ) end diff --git a/railties/CHANGELOG b/railties/CHANGELOG index 5e797be8b8..8f6de2d135 100644 --- a/railties/CHANGELOG +++ b/railties/CHANGELOG @@ -1,5 +1,7 @@ *SVN* +* Modernize the scaffold generator to use the simplified render and test methods and to change style from @params["id"] to params[:id]. #1367 + * Added graceful exit from pressing CTRL-C during the run of the rails command #1150 [Caleb Tennis] * Allow graceful exits for dispatch.fcgi processes by sending a SIGUSR1. If the process is currently handling a request, the request will be allowed to complete and then will terminate itself. If a request is not being handled, the process is terminated immediately (via #exit). This basically works like restart graceful on Apache. [Jamis Buck] diff --git a/railties/lib/rails_generator/generators/components/scaffold/templates/functional_test.rb b/railties/lib/rails_generator/generators/components/scaffold/templates/functional_test.rb index c1b934a5d3..3441b56670 100644 --- a/railties/lib/rails_generator/generators/components/scaffold/templates/functional_test.rb +++ b/railties/lib/rails_generator/generators/components/scaffold/templates/functional_test.rb @@ -16,65 +16,83 @@ class <%= controller_class_name %>ControllerTest < Test::Unit::TestCase <% for action in unscaffolded_actions -%> def test_<%= action %> get :<%= action %> - assert_rendered_file '<%= action %>' + assert_response :success + assert_template '<%= action %>' end <% end -%> <% unless suffix -%> def test_index get :index - assert_rendered_file 'list' + assert_response :success + assert_template 'list' end <% end -%> def test_list<%= suffix %> get :list<%= suffix %> - assert_rendered_file 'list<%= suffix %>' - assert_template_has '<%= plural_name %>' + + assert_response :success + assert_template 'list<%= suffix %>' + + assert_not_nil assigns(:<%= plural_name %>) end def test_show<%= suffix %> - get :show<%= suffix %>, 'id' => 1 - assert_rendered_file 'show' - assert_template_has '<%= singular_name %>' - assert_valid_record '<%= singular_name %>' + get :show<%= suffix %>, :id => 1 + + assert_response :success + assert_template 'show' + + assert_not_nil assigns(:<%= singular_name %>) + assert assigns(:<%= singular_name %>).valid? end def test_new<%= suffix %> get :new<%= suffix %> - assert_rendered_file 'new<%= suffix %>' - assert_template_has '<%= singular_name %>' + + assert_response :success + assert_template 'new<%= suffix %>' + + assert_not_nil assigns(:<%= singular_name %>) end def test_create - num_<%= plural_name %> = <%= model_name %>.find_all.size + num_<%= plural_name %> = <%= model_name %>.count - post :create<%= suffix %>, '<%= singular_name %>' => { } + post :create<%= suffix %>, :<%= singular_name %> => {} + + assert_response :redirect assert_redirected_to :action => 'list<%= suffix %>' - assert_equal num_<%= plural_name %> + 1, <%= model_name %>.find_all.size + assert_equal num_<%= plural_name %> + 1, <%= model_name %>.count end def test_edit<%= suffix %> - get :edit<%= suffix %>, 'id' => 1 - assert_rendered_file 'edit<%= suffix %>' - assert_template_has '<%= singular_name %>' - assert_valid_record '<%= singular_name %>' + get :edit<%= suffix %>, :id => 1 + + assert_response :success + assert_template 'edit<%= suffix %>' + + assert_not_nil assigns(:<%= singular_name %>) + assert assigns(:<%= singular_name %>).valid? end def test_update<%= suffix %> - post :update<%= suffix %>, 'id' => 1 + post :update<%= suffix %>, :id => 1 + assert_response :redirect assert_redirected_to :action => 'show<%= suffix %>', :id => 1 end def test_destroy<%= suffix %> assert_not_nil <%= model_name %>.find(1) - post :destroy, 'id' => 1 + post :destroy, :id => 1 + assert_response :redirect assert_redirected_to :action => 'list<%= suffix %>' assert_raise(ActiveRecord::RecordNotFound) { - <%= singular_name %> = <%= model_name %>.find(1) + <%= model_name %>.find(1) } end end