datamapper / dm-timestamps

DataMapper plugin for magical timestamps
http://datamapper.org/
MIT License
9 stars 13 forks source link

Inconsistent timestamps behaviour when hard setting the :at/:on values #4

Open solnic opened 13 years ago

solnic commented 13 years ago

So in a model we have timestamps :at.

It creates created_at and updated_at, that's fine.

However, upon creating a new record, if I hard set both values, created_at respects my provided value, but updated_at does not. I can only assume the :on pair behaves the same too.

Is this a feature or a bug?


Created by Fred Wu - 2010-04-16 05:30:23 UTC

Original Lighthouse ticket: http://datamapper.lighthouseapp.com/projects/20609/tickets/1245

solnic commented 13 years ago

I would think that this is a bug, I added a standalone to reproduce the behavior at: http://github.com/snusnu/dm-snippets/commit/e134b6d9c1ad45e3bf80dd417783a27d4cb49c0c

by Martin Gamsjaeger (snusnu)

solnic commented 13 years ago

If you set these fields on your own, then why do you use dm-timestamps? One of the reasons why one would want to use this plugin is to not care about setting these fields manually. I’m not entirely sure if this is a valid bug.

by Piotr Solnica (solnic)

solnic commented 13 years ago

I’m not entirely sure either but it seems a bit weird that stuff behaves differently for created_at(on) and updates_at(on)

by Martin Gamsjaeger (snusnu)

solnic commented 13 years ago

Well for me, I need it in testing. :)

by Fred Wu

solnic commented 13 years ago

I would accept a patch (with specs) to dm-timestamps that makes it so those values are not set if attribute_dirty?(property_name) returns true.

While I think this is a bit of an edge case, in other parts of DM we’ve made it so that user-supplied data takes precedence over defaults set by the system, so I think it fits in with the approach we’ve taken elsewhere.

by Dan Kubb (dkubb)

solnic commented 13 years ago

I’d like to help because this is blocking me from writing tests for my own project. A little direction? I’m new to working with git & plugins. Bear with me on what’s probably a 101 question, and feel free to point me to resources on contributing

I’m pretty sure I have this fixed (with specs), but can’t get spec to work:

~/Documents/workspace/dm-timestamps$ spec spec/integration/timestamps_spec.rb 
/opt/local/lib/ruby/vendor_ruby/1.8/rubygems/custom_require.rb:31:in `gem_original_require’: no such file to load -- dm-core/spec/setup (LoadError)

I’ve tried cloning the dm-core project, but those files do not exist.

FWIW, here’s what I have so far:

~/Documents/workspace/dm-timestamps$ git diff
diff --git a/lib/dm-timestamps.rb b/lib/dm-timestamps.rb
index 23ae865..054a2a5 100644
--- a/lib/dm-timestamps.rb
+++ b/lib/dm-timestamps.rb
 @@ -3,8 +3,8 @@ require ’dm-core’
 module DataMapper
   module Timestamps
     TIMESTAMP_PROPERTIES = {
-      :updated_at => [ DateTime, lambda { |r| DateTime.now                             } ],
-      :updated_on => [ Date,     lambda { |r| Date.today                               } ],
+      :updated_at => [ DateTime, lambda { |r| r.updated_at || DateTime.now             } ],
+      :updated_on => [ Date,     lambda { |r| r.updated_at || Date.today               } ],
       :created_at => [ DateTime, lambda { |r| r.created_at || (DateTime.now if r.new?) } ],
       :created_on => [ Date,     lambda { |r| r.created_on || (Date.today   if r.new?) } ],
     }.freeze
diff --git a/spec/integration/timestamps_spec.rb b/spec/integration/timestamps_spec.rb
index 393c668..7f8abff 100644
--- a/spec/integration/timestamps_spec.rb
+++ b/spec/integration/timestamps_spec.rb
 @@ -16,6 +16,18 @@ describe ’DataMapper::Timestamp’ do
         green_smoothie.created_at.should be_a_kind_of(DateTime)
         green_smoothie.created_on.should be_a_kind_of(Date)
       end
+      
+      it "should not set the updated_at/on fields if they’re already set" do
+        green_smoothie = GreenSmoothie.new(:name => ’Banana’)
+        time = (DateTime.now - 100)
+        green_smoothie.updated_at = time
+        green_smoothie.updated_on = time
+        green_smoothie.save
+        green_smoothie.updated_at.should == time
+        green_smoothie.updated_on.should == time
+        green_smoothie.updated_at.should be_a_kind_of(DateTime)
+        green_smoothie.updated_on.should be_a_kind_of(Date)
+      end

       it "should set the created_at/on fields on creation" do
         green_smoothie = GreenSmoothie.new(:name => ’Banana’)
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb

by orbiteleven

solnic commented 13 years ago

Er, scratch that last diff for this one :)

diff --git a/lib/dm-timestamps.rb b/lib/dm-timestamps.rb
index 23ae865..ba2b0a8 100644
--- a/lib/dm-timestamps.rb
+++ b/lib/dm-timestamps.rb
 @@ -3,8 +3,8 @@ require ’dm-core’
 module DataMapper
   module Timestamps
     TIMESTAMP_PROPERTIES = {
-      :updated_at => [ DateTime, lambda { |r| DateTime.now                             } ],
-      :updated_on => [ Date,     lambda { |r| Date.today                               } ],
-      :updated_at => [ DateTime, lambda { |r| r.updated_at || DateTime.now             } ],
-      :updated_on => [ Date,     lambda { |r| r.updated_on || Date.today               } ],
     :created_at => [ DateTime, lambda { |r| r.created_at || (DateTime.now if r.new?) } ],
     :created_on => [ Date,     lambda { |r| r.created_on || (Date.today   if r.new?) } ],
   }.freeze
  diff --git a/spec/integration/timestamps_spec.rb b/spec/integration/timestamps_spec.rb
  index 393c668..7f8abff 100644
  --- a/spec/integration/timestamps_spec.rb
  +++ b/spec/integration/timestamps_spec.rb
  @@ -16,6 +16,18 @@ describe ’DataMapper::Timestamp’ do
         green_smoothie.created_at.should be_a_kind_of(DateTime)
         green_smoothie.created_on.should be_a_kind_of(Date)
       end
-   
-      it "should not set the updated_at/on fields if they’re already set" do
-        green_smoothie = GreenSmoothie.new(:name => ’Banana’)
-        time = (DateTime.now - 100)
-        green_smoothie.updated_at = time
-        green_smoothie.updated_on = time
-        green_smoothie.save
-        green_smoothie.updated_at.should == time
-        green_smoothie.updated_on.should == time
-        green_smoothie.updated_at.should be_a_kind_of(DateTime)
-        green_smoothie.updated_on.should be_a_kind_of(Date)
- ```
   end
 it "should set the created_at/on fields on creation" do
   green_smoothie = GreenSmoothie.new(:name => ’Banana’)

diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb


  by orbiteleven
solnic commented 13 years ago

Hey orbiteleven,

To get up and running just clone dm-timestamps and do:

cd dm-timestamps
export ADAPTER=sqlite
bundle install
bundle exec spec spec/integration

This needs bundler to be installed, if you don’t have just do gem i bundler.

Regarding your patch - you need to check if attribute_dirty?(:updated_at/:updated_on) returns true and only if it does you don’t set the values. Thanks for you help :)

by Piotr Solnica (solnic)

solnic commented 13 years ago

Much better, thanks for the help.

I’ve attached my patch for this ticket. I hope it helps! If not, constructive criticism welcome.

by orbiteleven

ghost commented 13 years ago

I'm seeing this issue to. I have created_at and updated_at properties. updated_at sets properly but in order to get created_at to work I have to declare timestamps :created_at, :updated_at in the model.