Fixed bug #1816 - the timeout updating logic should have been in a before_save handle...

diff --git a/app/controllers/amf_controller.rb b/app/controllers/amf_controller.rb
index b6d8cbe539fee8325f91cc2863691381953569f0..0f61b7a4774e7874443d9998bd18edc518ec01a3 100644 (file)
--- a/app/controllers/amf_controller.rb
+++ b/app/controllers/amf_controller.rb
@@ -815,12 +815,10 @@ class AmfController < ApplicationController
     return user
   end
 
-  # Update changeset timeout
-  # i.e. one hour after current edit
-  
-  def updatetimeout(changeset_id) #:doc:
+  # save the changeset identified by +changeset_id+. this 
+  # automatically sets the close timeout appropriately.
+  def updatetimeout(changeset_id) 
     cs = Changeset.find(changeset_id)
-    cs.closed_at = Time.now.getutc + Changeset::IDLE_TIMEOUT
     cs.save!
   end
 

diff --git a/app/models/changeset.rb b/app/models/changeset.rb
index fa2d556b10c00195a20d5f8670224a22475ebb7b..a1162b3234be9dc0531cc9e85aa25115b77951c9 100644 (file)
--- a/app/models/changeset.rb
+++ b/app/models/changeset.rb
@@ -144,19 +144,9 @@ class Changeset < ActiveRecord::Base
   end
 
   def save_with_tags!
-    t = Time.now.getutc
-
     # do the changeset update and the changeset tags update in the
     # same transaction to ensure consistency.
     Changeset.transaction do
-      # set the auto-close time to be one hour in the future unless
-      # that would make it more than 24h long, in which case clip to
-      # 24h, as this has been decided is a reasonable time limit.
-      if (closed_at - created_at) > (MAX_TIME_OPEN - IDLE_TIMEOUT)
-        self.closed_at = created_at + MAX_TIME_OPEN
-      else
-        self.closed_at = Time.now.getutc + IDLE_TIMEOUT
-      end
       self.save!
 
       tags = self.tags
@@ -171,6 +161,20 @@ class Changeset < ActiveRecord::Base
       end
     end
   end
+
+  ##
+  # set the auto-close time to be one hour in the future unless
+  # that would make it more than 24h long, in which case clip to
+  # 24h, as this has been decided is a reasonable time limit.
+  def before_save
+    if self.is_open?
+      if (closed_at - created_at) > (MAX_TIME_OPEN - IDLE_TIMEOUT)
+        self.closed_at = created_at + MAX_TIME_OPEN
+      else
+        self.closed_at = Time.now.getutc + IDLE_TIMEOUT
+      end
+    end
+  end
   
   def to_xml
     doc = OSM::API.new.get_xml_doc

diff --git a/test/fixtures/changesets.yml b/test/fixtures/changesets.yml
index b201bed4f984d7245f9b489736d9c90ab731ac67..9f6b201a53d19e7ab80b447d963ba51ce2187edf 100644 (file)
--- a/test/fixtures/changesets.yml
+++ b/test/fixtures/changesets.yml
@@ -17,7 +17,7 @@ normal_user_first_change:
 public_user_first_change:
   id: 2
   user_id: 2
-  created_at: "2008-05-01 01:23:45"
+  created_at: <%= Time.now.getutc %>
   closed_at: <%= DateTime.now + Rational(1,24) %>
   num_changes: 0
 
@@ -38,7 +38,7 @@ public_user_closed_change:
 public_user_version_change:
   id: 4
   user_id: 2
-  created_at: "2008-01-01 00:00:00"
+  created_at: <%= Time.now.getutc %>
   closed_at: <%= DateTime.now + Rational(1,24) %>
   min_lon: <%= 1 * SCALE %>
   min_lat: <%= 1 * SCALE %>

diff --git a/test/functional/amf_controller_test.rb b/test/functional/amf_controller_test.rb
index 0ca506f3678bfb4a4f22376e3bea17958d985df7..06aa3ad5c2e40128dd4b51670a3cfaac1809f7d3 100644 (file)
--- a/test/functional/amf_controller_test.rb
+++ b/test/functional/amf_controller_test.rb
@@ -325,7 +325,8 @@ class AmfControllerTest < ActionController::TestCase
   # AMF Write tests
   def test_putpoi_update_valid
     nd = current_nodes(:visible_node)
-    amf_content "putpoi", "/1", ["test@openstreetmap.org:test", nd.changeset_id, nd.version, nd.id, nd.lon, nd.lat, nd.tags, nd.visible]
+    cs_id = changesets(:public_user_first_change).id
+    amf_content "putpoi", "/1", ["test@example.com:test", cs_id, nd.version, nd.id, nd.lon, nd.lat, nd.tags, nd.visible]
     post :amf_write
     assert_response :success
     amf_parse_response
@@ -339,7 +340,7 @@ class AmfControllerTest < ActionController::TestCase
     # Now try to update again, with a different lat/lon, using the updated version number
     lat = nd.lat+0.1
     lon = nd.lon-0.1
-    amf_content "putpoi", "/2", ["test@openstreetmap.org:test", nd.changeset_id, nd.version+1, nd.id, lon, lat, nd.tags, nd.visible]
+    amf_content "putpoi", "/2", ["test@example.com:test", cs_id, nd.version+1, nd.id, lon, lat, nd.tags, nd.visible]
     post :amf_write
     assert_response :success
     amf_parse_response
@@ -360,9 +361,9 @@ class AmfControllerTest < ActionController::TestCase
     lat = rand(100)-50 + rand
     lon = rand(100)-50 + rand
     # normal user has a changeset open
-    changeset = changesets(:normal_user_first_change)
+    changeset = changesets(:public_user_first_change)
     
-    amf_content "putpoi", "/1", ["test@openstreetmap.org:test", changeset.id, nil, nil, lon, lat, {}, nil]
+    amf_content "putpoi", "/1", ["test@example.com:test", changeset.id, nil, nil, lon, lat, {}, nil]
     post :amf_write
     assert_response :success
     amf_parse_response
@@ -399,9 +400,9 @@ class AmfControllerTest < ActionController::TestCase
     lat = rand(100)-50 + rand
     lon = rand(100)-50 + rand
     # normal user has a changeset open
-    changeset = changesets(:normal_user_first_change)
+    changeset = changesets(:public_user_first_change)
     
-    amf_content "putpoi", "/2", ["test@openstreetmap.org:test", changeset.id, nil, nil, lon, lat, { "key" => "value", "ping" => "pong" }, nil]
+    amf_content "putpoi", "/2", ["test@example.com:test", changeset.id, nil, nil, lon, lat, { "key" => "value", "ping" => "pong" }, nil]
     post :amf_write
     assert_response :success
     amf_parse_response

diff --git a/test/functional/changeset_controller_test.rb b/test/functional/changeset_controller_test.rb
index 458a5adf17c15182f5031d6981197c9e275c0f24..f734c8c0f398447df098dcf300f977a95f5b42c7 100644 (file)
--- a/test/functional/changeset_controller_test.rb
+++ b/test/functional/changeset_controller_test.rb
@@ -128,7 +128,7 @@ class ChangesetControllerTest < ActionController::TestCase
     # test that it really is closed now
     cs = Changeset.find(cs_id)
     assert(!cs.is_open?, 
-           "changeset should be closed now (#{cs.closed_at} > #{Time.now}.")
+           "changeset should be closed now (#{cs.closed_at} > #{Time.now.getutc}.")
   end
 
   ##
@@ -1295,7 +1295,7 @@ EOF
 
     get :query, :time => '2007-12-31T23:59Z,2008-01-01T00:01Z'
     assert_response :success, "can't get changesets by time-range"
-    assert_changesets [1,4,5,6]
+    assert_changesets [1,5,6]
 
     get :query, :open => 'true'
     assert_response :success, "can't get changesets by open-ness"