Skip to content
Browse files

Require persisted? in ActiveModel::Lint and remove new_record? and de…

…stroyed? methods. ActionPack does not care if the resource is new or if it was destroyed, it cares only if it's persisted somewhere or not.
  • Loading branch information...
1 parent 9dd67fc commit 250c8092461f5e6bf62751b313f6605a37fd1b2b @josevalim josevalim committed
View
3 actionpack/lib/action_controller/polymorphic_routes.rb
@@ -92,8 +92,7 @@ def polymorphic_url(record_or_hash_or_array, options = {})
inflection = if options[:action].to_s == "new"
args.pop
:singular
- elsif (record.respond_to?(:new_record?) && record.new_record?) ||
- (record.respond_to?(:destroyed?) && record.destroyed?)
+ elsif (record.respond_to?(:persisted?) && !record.persisted?)
args.pop
:plural
elsif record.is_a?(Class)
View
4 actionpack/lib/action_view/helpers/active_model_helper.rb
@@ -80,13 +80,13 @@ def form(record_name, options = {})
record = convert_to_model(record)
options = options.symbolize_keys
- options[:action] ||= record.new_record? ? "create" : "update"
+ options[:action] ||= record.persisted? ? "update" : "create"
action = url_for(:action => options[:action], :id => record)
submit_value = options[:submit_value] || options[:action].gsub(/[^\w]/, '').capitalize
contents = form_tag({:action => action}, :method =>(options[:method] || 'post'), :enctype => options[:multipart] ? 'multipart/form-data': nil)
- contents.safe_concat hidden_field(record_name, :id) unless record.new_record?
+ contents.safe_concat hidden_field(record_name, :id) if record.persisted?
contents.safe_concat all_input_tags(record, record_name, options)
yield contents if block_given?
contents.safe_concat submit_tag(submit_value)
View
17 actionpack/lib/action_view/helpers/form_helper.rb
@@ -316,14 +316,13 @@ def form_for(record_or_name_or_array, *args, &proc)
def apply_form_for_options!(object_or_array, options) #:nodoc:
object = object_or_array.is_a?(Array) ? object_or_array.last : object_or_array
-
object = convert_to_model(object)
html_options =
- if object.respond_to?(:new_record?) && object.new_record?
- { :class => dom_class(object, :new), :id => dom_id(object), :method => :post }
- else
+ if object.respond_to?(:persisted?) && object.persisted?
{ :class => dom_class(object, :edit), :id => dom_id(object, :edit), :method => :put }
+ else
+ { :class => dom_class(object, :new), :id => dom_id(object), :method => :post }
end
options[:html] ||= {}
@@ -1150,7 +1149,7 @@ def objectify_options(options)
def submit_default_value
object = @object.respond_to?(:to_model) ? @object.to_model : @object
- key = object ? (object.new_record? ? :create : :update) : :submit
+ key = object ? (object.persisted? ? :update : :create) : :submit
model = if object.class.respond_to?(:model_name)
object.class.model_name.human
@@ -1176,7 +1175,7 @@ def fields_for_with_nested_attributes(association_name, args, block)
association = args.shift
association = association.to_model if association.respond_to?(:to_model)
- if association.respond_to?(:new_record?)
+ if association.respond_to?(:persisted?)
association = [association] if @object.send(association_name).is_a?(Array)
elsif !association.is_a?(Array)
association = @object.send(association_name)
@@ -1195,13 +1194,13 @@ def fields_for_with_nested_attributes(association_name, args, block)
def fields_for_nested_model(name, object, options, block)
object = object.to_model if object.respond_to?(:to_model)
- if object.new_record?
- @template.fields_for(name, object, options, &block)
- else
+ if object.persisted?
@template.fields_for(name, object, options) do |builder|
block.call(builder)
@template.concat builder.hidden_field(:id) unless builder.emitted_hidden_id?
end
+ else
+ @template.fields_for(name, object, options, &block)
end
end
View
2 actionpack/lib/action_view/helpers/url_helper.rb
@@ -63,7 +63,7 @@ def default_url_options(*args) #:nodoc:
# # => /testing/jump/#tax&ship
#
# <%= url_for(Workshop.new) %>
- # # relies on Workshop answering a new_record? call (and in this case returning true)
+ # # relies on Workshop answering a persisted? call (and in this case returning false)
# # => /workshops
#
# <%= url_for(@workshop) %>
View
4 actionpack/test/controller/mime_responds_test.rb
@@ -513,7 +513,7 @@ def respond; @controller.render :text => "respond #{format}"; end
protected
def resource
- Customer.new("david", 13)
+ Customer.new("david", request.delete? ? nil : 13)
end
def _render_js(js, options)
@@ -717,7 +717,7 @@ def test_using_resource_for_delete_with_html_redirects_on_failure
delete :using_resource
assert_equal "text/html", @response.content_type
assert_equal 302, @response.status
- assert_equal "http://www.example.com/customers/13", @response.location
+ assert_equal "http://www.example.com/customers", @response.location
end
end
View
18 actionpack/test/controller/redirect_test.rb
@@ -6,14 +6,14 @@ class WorkshopsController < ActionController::Base
class Workshop
extend ActiveModel::Naming
include ActiveModel::Conversion
- attr_accessor :id, :new_record
+ attr_accessor :id
- def initialize(id, new_record)
- @id, @new_record = id, new_record
+ def initialize(id)
+ @id = id
end
- def new_record?
- @new_record
+ def persisted?
+ id.present?
end
def to_s
@@ -88,11 +88,11 @@ def redirect_to_back
end
def redirect_to_existing_record
- redirect_to Workshop.new(5, false)
+ redirect_to Workshop.new(5)
end
def redirect_to_new_record
- redirect_to Workshop.new(5, true)
+ redirect_to Workshop.new(nil)
end
def redirect_to_nil
@@ -239,11 +239,11 @@ def test_redirect_to_record
get :redirect_to_existing_record
assert_equal "http://test.host/workshops/5", redirect_to_url
- assert_redirected_to Workshop.new(5, false)
+ assert_redirected_to Workshop.new(5)
get :redirect_to_new_record
assert_equal "http://test.host/workshops", redirect_to_url
- assert_redirected_to Workshop.new(5, true)
+ assert_redirected_to Workshop.new(nil)
end
end
View
40 actionpack/test/lib/controller/fake_models.rb
@@ -6,14 +6,6 @@ class Customer < Struct.new(:name, :id)
undef_method :to_json
- def to_key
- id ? [id] : nil
- end
-
- def to_param
- id.to_s
- end
-
def to_xml(options={})
if options[:builder]
options[:builder].name name
@@ -31,8 +23,8 @@ def errors
[]
end
- def destroyed?
- false
+ def persisted?
+ id.present?
end
end
@@ -47,12 +39,8 @@ class Question < Struct.new(:name, :id)
extend ActiveModel::Naming
include ActiveModel::Conversion
- def to_key
- id ? [id] : nil
- end
-
- def to_param
- id.to_s
+ def persisted?
+ id.present?
end
end
@@ -67,16 +55,12 @@ class Post < Struct.new(:title, :author_name, :body, :secret, :written_on, :cost
alias_method :secret?, :secret
- def to_key
- id ? [id] : nil
- end
-
- def new_record=(boolean)
- @new_record = boolean
+ def persisted=(boolean)
+ @persisted = boolean
end
- def new_record?
- @new_record
+ def persisted?
+ @persisted
end
attr_accessor :author
@@ -98,7 +82,7 @@ class Comment
def initialize(id = nil, post_id = nil); @id, @post_id = id, post_id end
def to_key; id ? [id] : nil end
def save; @id = 1; @post_id = 1 end
- def new_record?; @id.nil? end
+ def persisted?; @id.present? end
def to_param; @id; end
def name
@id.nil? ? "new #{self.class.name.downcase}" : "#{self.class.name.downcase} ##{@id}"
@@ -118,7 +102,7 @@ class Tag
def initialize(id = nil, post_id = nil); @id, @post_id = id, post_id end
def to_key; id ? [id] : nil end
def save; @id = 1; @post_id = 1 end
- def new_record?; @id.nil? end
+ def persisted?; @id.present? end
def to_param; @id; end
def value
@id.nil? ? "new #{self.class.name.downcase}" : "#{self.class.name.downcase} ##{@id}"
@@ -138,7 +122,7 @@ class CommentRelevance
def initialize(id = nil, comment_id = nil); @id, @comment_id = id, comment_id end
def to_key; id ? [id] : nil end
def save; @id = 1; @comment_id = 1 end
- def new_record?; @id.nil? end
+ def persisted?; @id.present? end
def to_param; @id; end
def value
@id.nil? ? "new #{self.class.name.downcase}" : "#{self.class.name.downcase} ##{@id}"
@@ -154,7 +138,7 @@ class TagRelevance
def initialize(id = nil, tag_id = nil); @id, @tag_id = id, tag_id end
def to_key; id ? [id] : nil end
def save; @id = 1; @tag_id = 1 end
- def new_record?; @id.nil? end
+ def persisted?; @id.present? end
def to_param; @id; end
def value
@id.nil? ? "new #{self.class.name.downcase}" : "#{self.class.name.downcase} ##{@id}"
View
4 actionpack/test/template/active_model_helper_test.rb
@@ -64,7 +64,7 @@ def full_messages() [ "Author name can't be empty" ] end
}.new
end
- def @post.new_record?() true end
+ def @post.persisted?() false end
def @post.to_param() nil end
def @post.column_for_attribute(attr_name)
@@ -155,7 +155,7 @@ def test_form_with_string
silence_warnings do
class << @post
- def new_record?() false end
+ def persisted?() true end
def to_param() id end
def id() 1 end
end
View
8 actionpack/test/template/atom_feed_helper_test.rb
@@ -4,8 +4,8 @@ class Scroll < Struct.new(:id, :to_param, :title, :body, :updated_at, :created_a
extend ActiveModel::Naming
include ActiveModel::Conversion
- def new_record?
- true
+ def persisted?
+ false
end
end
@@ -34,7 +34,7 @@ class ScrollsController < ActionController::Base
feed.updated((@scrolls.first.created_at))
for scroll in @scrolls
- feed.entry(scroll, :url => "/otherstuff/" + scroll.to_param, :updated => Time.utc(2007, 1, scroll.id)) do |entry|
+ feed.entry(scroll, :url => "/otherstuff/" + scroll.to_param.to_s, :updated => Time.utc(2007, 1, scroll.id)) do |entry|
entry.title(scroll.title)
entry.content(scroll.body, :type => 'html')
@@ -55,7 +55,7 @@ class ScrollsController < ActionController::Base
end
for scroll in @scrolls
- feed.entry(scroll, :url => "/otherstuff/" + scroll.to_param, :updated => Time.utc(2007, 1, scroll.id)) do |entry|
+ feed.entry(scroll, :url => "/otherstuff/" + scroll.to_param.to_s, :updated => Time.utc(2007, 1, scroll.id)) do |entry|
entry.title(scroll.title)
entry.content(scroll.body, :type => 'html')
end
View
13 actionpack/test/template/form_helper_test.rb
@@ -53,6 +53,7 @@ def @post.id; 123; end
def @post.id_before_type_cast; 123; end
def @post.to_param; '123'; end
+ @post.persisted = true
@post.title = "Hello World"
@post.author_name = ""
@post.body = "Back to the hill and over it again!"
@@ -529,7 +530,7 @@ def test_form_for_with_nil_index_option_override
def test_submit_with_object_as_new_record_and_locale_strings
old_locale, I18n.locale = I18n.locale, :submit
- def @post.new_record?() true; end
+ @post.persisted = false
form_for(:post, @post) do |f|
concat f.submit
end
@@ -1363,7 +1364,7 @@ def test_form_for_with_existing_object
def test_form_for_with_new_object
post = Post.new
- post.new_record = true
+ post.persisted = false
def post.id() nil end
form_for(post) do |f| end
@@ -1373,9 +1374,7 @@ def post.id() nil end
end
def test_form_for_with_existing_object_in_list
- @post.new_record = false
@comment.save
-
form_for([@post, @comment]) {}
expected = %(<form action="#{comment_path(@post, @comment)}" class="edit_comment" id="edit_comment_1" method="post"><div style="margin:0;padding:0;display:inline"><input name="_method" type="hidden" value="put" /></div></form>)
@@ -1383,8 +1382,6 @@ def test_form_for_with_existing_object_in_list
end
def test_form_for_with_new_object_in_list
- @post.new_record = false
-
form_for([@post, @comment]) {}
expected = %(<form action="#{comments_path(@post)}" class="new_comment" id="new_comment" method="post"></form>)
@@ -1392,9 +1389,7 @@ def test_form_for_with_new_object_in_list
end
def test_form_for_with_existing_object_and_namespace_in_list
- @post.new_record = false
@comment.save
-
form_for([:admin, @post, @comment]) {}
expected = %(<form action="#{admin_comment_path(@post, @comment)}" class="edit_comment" id="edit_comment_1" method="post"><div style="margin:0;padding:0;display:inline"><input name="_method" type="hidden" value="put" /></div></form>)
@@ -1402,8 +1397,6 @@ def test_form_for_with_existing_object_and_namespace_in_list
end
def test_form_for_with_new_object_and_namespace_in_list
- @post.new_record = false
-
form_for([:admin, @post, @comment]) {}
expected = %(<form action="#{admin_comments_path(@post)}" class="new_comment" id="new_comment" method="post"></form>)
View
1 actionpack/test/template/record_tag_helper_test.rb
@@ -18,6 +18,7 @@ class RecordTagHelperTest < ActionView::TestCase
def setup
super
@post = Post.new
+ @post.persisted = true
end
def test_content_tag_for
View
36 actionpack/test/template/url_helper_test.rb
@@ -499,14 +499,14 @@ def with_restful_routing
class Workshop
extend ActiveModel::Naming
include ActiveModel::Conversion
- attr_accessor :id, :new_record
+ attr_accessor :id
- def initialize(id, new_record)
- @id, @new_record = id, new_record
+ def initialize(id)
+ @id = id
end
- def new_record?
- @new_record
+ def persisted?
+ id.present?
end
def to_s
@@ -517,14 +517,14 @@ def to_s
class Session
extend ActiveModel::Naming
include ActiveModel::Conversion
- attr_accessor :id, :workshop_id, :new_record
+ attr_accessor :id, :workshop_id
- def initialize(id, new_record)
- @id, @new_record = id, new_record
+ def initialize(id)
+ @id = id
end
- def new_record?
- @new_record
+ def persisted?
+ id.present?
end
def to_s
@@ -534,12 +534,12 @@ def to_s
class WorkshopsController < ActionController::Base
def index
- @workshop = Workshop.new(1, true)
+ @workshop = Workshop.new(nil)
render :inline => "<%= url_for(@workshop) %>\n<%= link_to('Workshop', @workshop) %>"
end
def show
- @workshop = Workshop.new(params[:id], false)
+ @workshop = Workshop.new(params[:id])
render :inline => "<%= url_for(@workshop) %>\n<%= link_to('Workshop', @workshop) %>"
end
@@ -548,14 +548,14 @@ def rescue_action(e) raise e end
class SessionsController < ActionController::Base
def index
- @workshop = Workshop.new(params[:workshop_id], false)
- @session = Session.new(1, true)
+ @workshop = Workshop.new(params[:workshop_id])
+ @session = Session.new(nil)
render :inline => "<%= url_for([@workshop, @session]) %>\n<%= link_to('Session', [@workshop, @session]) %>"
end
def show
- @workshop = Workshop.new(params[:workshop_id], false)
- @session = Session.new(params[:id], false)
+ @workshop = Workshop.new(params[:workshop_id])
+ @session = Session.new(params[:id])
render :inline => "<%= url_for([@workshop, @session]) %>\n<%= link_to('Session', [@workshop, @session]) %>"
end
@@ -565,8 +565,8 @@ def rescue_action(e) raise e end
class PolymorphicControllerTest < ActionController::TestCase
def setup
super
- @request = ActionController::TestRequest.new
- @response = ActionController::TestResponse.new
+ @request = ActionController::TestRequest.new
+ @response = ActionController::TestResponse.new
end
def test_new_resource
View
2 activemodel/examples/validations.rb
@@ -16,7 +16,7 @@ def persist
@persisted = true
end
- def new_record?
+ def persisted?
@persisted
end
end
View
12 activemodel/lib/active_model/conversion.rb
@@ -8,9 +8,9 @@ module ActiveModel
# class ContactMessage
# include ActiveModel::Conversion
#
- # # Always a new record, since it's not persisted in the DB.
- # def new_record?
- # true
+ # # ContactMessage are never persisted in the DB
+ # def persisted?
+ # false
# end
# end
#
@@ -30,13 +30,13 @@ def to_model
self
end
- # Returns an Enumerable of all (primary) key attributes or nil if new_record? is true
+ # Returns an Enumerable of all (primary) key attributes or nil if persisted? is fakse
def to_key
- new_record? ? nil : [id]
+ persisted? ? [id] : nil
end
# Returns a string representing the object's key suitable for use in URLs,
- # or nil if new_record? is true
+ # or nil if persisted? is false
def to_param
to_key ? to_key.join('-') : nil
end
View
25 activemodel/lib/active_model/lint.rb
@@ -17,28 +17,28 @@ module Tests
# == Responds to <tt>to_key</tt>
#
# Returns an Enumerable of all (primary) key attributes
- # or nil if model.new_record? is true
+ # or nil if model.persisted? is false
def test_to_key
assert model.respond_to?(:to_key), "The model should respond to to_key"
- def model.new_record?() true end
+ def model.persisted?() false end
assert model.to_key.nil?
- def model.new_record?() false end
+ def model.persisted?() true end
assert model.to_key.respond_to?(:each)
end
# == Responds to <tt>to_param</tt>
#
# Returns a string representing the object's key suitable for use in URLs
- # or nil if model.new_record? is true.
+ # or nil if model.persisted? is false.
#
# Implementers can decide to either raise an exception or provide a default
# in case the record uses a composite primary key. There are no tests for this
# behavior in lint because it doesn't make sense to force any of the possible
# implementation strategies on the implementer. However, if the resource is
- # a new_record?, then to_param should always return nil.
+ # not persisted?, then to_param should always return nil.
def test_to_param
assert model.respond_to?(:to_param), "The model should respond to to_param"
- def model.new_record?() true end
+ def model.persisted?() false end
assert model.to_param.nil?
end
@@ -51,21 +51,16 @@ def test_valid?
assert_boolean model.valid?, "valid?"
end
- # == Responds to <tt>new_record?</tt>
+ # == Responds to <tt>persisted?</tt>
#
# Returns a boolean that specifies whether the object has been persisted yet.
# This is used when calculating the URL for an object. If the object is
# not persisted, a form for that object, for instance, will be POSTed to the
# collection. If it is persisted, a form for the object will put PUTed to the
# URL for the object.
- def test_new_record?
- assert model.respond_to?(:new_record?), "The model should respond to new_record?"
- assert_boolean model.new_record?, "new_record?"
- end
-
- def test_destroyed?
- assert model.respond_to?(:destroyed?), "The model should respond to destroyed?"
- assert_boolean model.destroyed?, "destroyed?"
+ def test_persisted?
+ assert model.respond_to?(:persisted?), "The model should respond to persisted?"
+ assert_boolean model.persisted?, "persisted?"
end
# == Naming
View
4 activemodel/test/cases/conversion_test.rb
@@ -12,7 +12,7 @@ class ConversionTest < ActiveModel::TestCase
end
test "to_key default implementation returns the id in an array for persisted records" do
- assert_equal [1], Contact.new(:new_record => false, :id => 1).to_key
+ assert_equal [1], Contact.new(:id => 1).to_key
end
test "to_param default implementation returns nil for new records" do
@@ -20,6 +20,6 @@ class ConversionTest < ActiveModel::TestCase
end
test "to_param default implementation returns a string of ids for persisted records" do
- assert_equal "1", Contact.new(:new_record => false, :id => 1).to_param
+ assert_equal "1", Contact.new(:id => 1).to_param
end
end
View
3 activemodel/test/cases/lint_test.rb
@@ -8,8 +8,7 @@ class CompliantModel
include ActiveModel::Conversion
def valid?() true end
- def new_record?() true end
- def destroyed?() true end
+ def persisted?() false end
def errors
obj = Object.new
View
6 activemodel/test/models/contact.rb
@@ -1,13 +1,13 @@
class Contact
include ActiveModel::Conversion
- attr_accessor :id, :name, :age, :created_at, :awesome, :preferences, :new_record
+ attr_accessor :id, :name, :age, :created_at, :awesome, :preferences
def initialize(options = {})
options.each { |name, value| send("#{name}=", value) }
end
- def new_record?
- defined?(@new_record) ? @new_record : true
+ def persisted?
+ id.present?
end
end
View
13 activerecord/lib/active_record/base.rb
@@ -1767,6 +1767,11 @@ def destroyed?
@destroyed || false
end
+ # Returns if the record is persisted, i.e. it's not a new record and it was not destroyed.
+ def persisted?
+ !(new_record? || destroyed?)
+ end
+
# :call-seq:
# save(options)
#
@@ -1816,7 +1821,7 @@ def save!
# callbacks, Observer methods, or any <tt>:dependent</tt> association
# options, use <tt>#destroy</tt>.
def delete
- self.class.delete(id) unless new_record?
+ self.class.delete(id) if persisted?
@destroyed = true
freeze
end
@@ -1824,7 +1829,7 @@ def delete
# Deletes the record in the database and freezes this instance to reflect that no changes should
# be made (since they can't be persisted).
def destroy
- unless new_record?
+ if persisted?
self.class.unscoped.where(self.class.arel_table[self.class.primary_key].eq(id)).delete_all
end
@@ -1844,6 +1849,7 @@ def becomes(klass)
became.instance_variable_set("@attributes", @attributes)
became.instance_variable_set("@attributes_cache", @attributes_cache)
became.instance_variable_set("@new_record", new_record?)
+ became.instance_variable_set("@destroyed", destroyed?)
became
end
@@ -2042,8 +2048,7 @@ def column_for_attribute(name)
def ==(comparison_object)
comparison_object.equal?(self) ||
(comparison_object.instance_of?(self.class) &&
- comparison_object.id == id &&
- !comparison_object.new_record?)
+ comparison_object.id == id && !comparison_object.new_record?)
end
# Delegates to ==
View
22 activerecord/test/cases/base_test.rb
@@ -1330,11 +1330,6 @@ def test_new_record_returns_boolean
end
def test_destroyed_returns_boolean
- developer = Developer.new
- assert_equal developer.destroyed?, false
- developer.destroy
- assert_equal developer.destroyed?, true
-
developer = Developer.first
assert_equal developer.destroyed?, false
developer.destroy
@@ -1346,6 +1341,23 @@ def test_destroyed_returns_boolean
assert_equal developer.destroyed?, true
end
+ def test_persisted_returns_boolean
+ developer = Developer.new(:name => "Jose")
+ assert_equal developer.persisted?, false
+ developer.save!
+ assert_equal developer.persisted?, true
+
+ developer = Developer.first
+ assert_equal developer.persisted?, true
+ developer.destroy
+ assert_equal developer.persisted?, false
+
+ developer = Developer.last
+ assert_equal developer.persisted?, true
+ developer.delete
+ assert_equal developer.persisted?, false
+ end
+
def test_clone
topic = Topic.find(1)
cloned_topic = nil

1 comment on commit 250c809

@jpartogi

What is the to_key method in ActiveModel::Conversion is for?

Please sign in to comment.
Something went wrong with that request. Please try again.