From 1ceb4ab9ba10d97333414bf66ae2c9d553668903 Mon Sep 17 00:00:00 2001 From: Matt Amos Date: Tue, 12 May 2009 13:54:37 +0000 Subject: [PATCH] Fixed bug #1816 - the timeout updating logic should have been in a before_save handler, not in save_with_tags. --- app/controllers/amf_controller.rb | 8 +++---- app/models/changeset.rb | 24 ++++++++++++-------- test/fixtures/changesets.yml | 4 ++-- test/functional/amf_controller_test.rb | 13 ++++++----- test/functional/changeset_controller_test.rb | 4 ++-- 5 files changed, 28 insertions(+), 25 deletions(-) diff --git a/app/controllers/amf_controller.rb b/app/controllers/amf_controller.rb index b6d8cbe53..0f61b7a47 100644 --- 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 fa2d556b1..a1162b323 100644 --- 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 b201bed4f..9f6b201a5 100644 --- 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 0ca506f36..06aa3ad5c 100644 --- 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 458a5adf1..f734c8c0f 100644 --- 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" -- 2.39.5