From: Matt Amos Date: Fri, 21 Nov 2008 12:53:09 +0000 (+0000) Subject: Fixed bug in changeset idle timeout. Fixed another with a spurious require. X-Git-Tag: live~7601^2~133 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/783528830ad1b0d5f07c8e758747841a0b4e5cc6 Fixed bug in changeset idle timeout. Fixed another with a spurious require. --- diff --git a/app/controllers/changeset_controller.rb b/app/controllers/changeset_controller.rb index ca19fba30..5e538c721 100644 --- a/app/controllers/changeset_controller.rb +++ b/app/controllers/changeset_controller.rb @@ -2,7 +2,6 @@ class ChangesetController < ApplicationController require 'xml/libxml' - require 'diff_reader' before_filter :authorize, :only => [:create, :update, :delete, :upload, :include, :close] before_filter :check_write_availability, :only => [:create, :update, :delete, :upload, :include] diff --git a/app/models/changeset.rb b/app/models/changeset.rb index a65e3aebc..38cd8014f 100644 --- a/app/models/changeset.rb +++ b/app/models/changeset.rb @@ -25,7 +25,10 @@ class Changeset < ActiveRecord::Base MAX_TIME_OPEN = 1 # idle timeout increment, one hour as a rational number of days. - IDLE_TIMEOUT = 1.hour #Rational(1,24) + # NOTE: DO NOT CHANGE THIS TO 1.hour! when this was done the idle + # timeout changed to 1 second, which meant all changesets closed + # almost immediately. + IDLE_TIMEOUT = Rational(1,24) # Use a method like this, so that we can easily change how we # determine whether a changeset is open, without breaking code in at @@ -219,7 +222,7 @@ class Changeset < ActiveRecord::Base # can't change a closed changeset unless is_open? - raise OSM::APIChangesetAlreadyClosedError + raise OSM::APIChangesetAlreadyClosedError.new(self) end # copy the other's tags diff --git a/lib/consistency_validations.rb b/lib/consistency_validations.rb index 6e214f902..46fb3c06e 100644 --- a/lib/consistency_validations.rb +++ b/lib/consistency_validations.rb @@ -13,7 +13,7 @@ module ConsistencyValidations elsif new.changeset.user_id != user.id raise OSM::APIUserChangesetMismatchError.new elsif not new.changeset.is_open? - raise OSM::APIChangesetAlreadyClosedError.new + raise OSM::APIChangesetAlreadyClosedError.new(new.changeset) end end @@ -24,7 +24,7 @@ module ConsistencyValidations elsif new.changeset.user_id != user.id raise OSM::APIUserChangesetMismatchError.new elsif not new.changeset.is_open? - raise OSM::APIChangesetAlreadyClosedError.new + raise OSM::APIChangesetAlreadyClosedError.new(new.changeset) end end end diff --git a/lib/osm.rb b/lib/osm.rb index 00215c677..09ded2bd2 100644 --- a/lib/osm.rb +++ b/lib/osm.rb @@ -45,8 +45,14 @@ module OSM # Raised when the changeset provided is already closed class APIChangesetAlreadyClosedError < APIError + def initialize(changeset) + @changeset = changeset + end + + attr_reader :changeset + def render_opts - { :text => "The supplied changeset has already been closed", :status => :conflict } + { :text => "The changeset #{@changeset.id} was closed at #{@changeset.closed_at}.", :status => :conflict } end end diff --git a/test/functional/changeset_controller_test.rb b/test/functional/changeset_controller_test.rb index 3d7531cfb..47cd95c24 100644 --- a/test/functional/changeset_controller_test.rb +++ b/test/functional/changeset_controller_test.rb @@ -26,7 +26,19 @@ class ChangesetControllerTest < ActionController::TestCase put :create assert_response :success, "Creation of changeset did not return sucess status" - newid = @response.body + newid = @response.body.to_i + + # check end time, should be an hour ahead of creation time + cs = Changeset.find(newid) + duration = cs.closed_at - cs.created_at + # the difference can either be a rational, or a floating point number + # of seconds, depending on the code path taken :-( + if duration.class == Rational + assert_equal Rational(1,24), duration , "initial idle timeout should be an hour (#{cs.created_at} -> #{cs.closed_at})" + else + # must be number of seconds... + assert_equal 3600.0, duration , "initial idle timeout should be an hour (#{cs.created_at} -> #{cs.closed_at})" + end end def test_create_invalid @@ -455,6 +467,67 @@ EOF assert_select "osmChange>modify>node", 8 end + ## + # culled this from josm to ensure that nothing in the way that josm + # is formatting the request is causing it to fail. + # + # NOTE: the error turned out to be something else completely! + def test_josm_upload + basic_authorization(users(:normal_user).email, "test") + + # create a temporary changeset + content "" + + "" + + "" + put :create + assert_response :success + changeset_id = @response.body.to_i + + diff = < + + + + + + + + + + + + + + + + + + + + + + + + + +OSM + + # upload it + content diff + post :upload, :id => changeset_id + assert_response :success, + "can't upload a diff from JOSM: #{@response.body}" + + get :download, :id => changeset_id + assert_response :success + + assert_select "osmChange", 1 + assert_select "osmChange>create>node", 9 + assert_select "osmChange>create>way", 1 + assert_select "osmChange>create>way>nd", 9 + assert_select "osmChange>create>way>tag", 2 + end + ## # when we make some complex changes we get the same changes back from the # diff download.