From 76708eefcffb6b451ce9c275db4bcaf23690f69d Mon Sep 17 00:00:00 2001
From: Matt Amos <zerebubuth@gmail.com>
Date: Sat, 7 Feb 2009 17:45:27 +0000
Subject: [PATCH] Fixed bug in changeset closing and querying where the number
 of elements exceeded the maximum. Added a fixture for this.

---
 app/controllers/changeset_controller.rb      |  8 ++++++-
 app/models/changeset.rb                      |  2 +-
 test/fixtures/changesets.yml                 | 14 ++++++++++++
 test/functional/changeset_controller_test.rb | 23 +++++++++++++++-----
 test/unit/changeset_test.rb                  |  2 +-
 5 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/app/controllers/changeset_controller.rb b/app/controllers/changeset_controller.rb
index c820aa705..b2ff42711 100644
--- a/app/controllers/changeset_controller.rb
+++ b/app/controllers/changeset_controller.rb
@@ -395,8 +395,14 @@ private
 
   ##
   # restrict changes to those which are open
+  #
+  # at the moment this code assumes we're only interested in open
+  # changesets and gives no facility to query closed changesets. this
+  # would be reasonably simple to implement if anyone actually wants
+  # it?
   def conditions_open(open)
-    return open.nil? ? nil : ['closed_at >= ?', DateTime.now]
+    return open.nil? ? nil : ['closed_at >= ? and num_changes <= ?', 
+                              DateTime.now, Changeset::MAX_ELEMENTS]
   end
 
 end
diff --git a/app/models/changeset.rb b/app/models/changeset.rb
index 7b90659d8..31d927e9a 100644
--- a/app/models/changeset.rb
+++ b/app/models/changeset.rb
@@ -46,7 +46,7 @@ class Changeset < ActiveRecord::Base
   end
 
   def set_closed_time_now
-    unless is_open?
+    if is_open?
       self.closed_at = Time.now
     end
   end
diff --git a/test/fixtures/changesets.yml b/test/fixtures/changesets.yml
index defd691d2..9fe5bc6d8 100644
--- a/test/fixtures/changesets.yml
+++ b/test/fixtures/changesets.yml
@@ -48,3 +48,17 @@ invalid_changeset:
   created_at: "2008-01-01 00:00:00"
   closed_at: "2008-01-02 00:00:00"
   num_changes: 9
+
+# changeset which still has time remaining, but has been closed
+# by containing too many elements.
+too_many_elements_changeset:
+  id: 6
+  user_id: 1
+  created_at: "2008-01-01 00:00:00"
+  closed_at: <%= DateTime.now + Rational(1,24) %>
+  min_lon: <%= 1 * SCALE %>
+  min_lat: <%= 1 * SCALE %>
+  max_lon: <%= 4 * SCALE %>
+  max_lat: <%= 4 * SCALE %>
+  num_changes: <%= Changeset::MAX_ELEMENTS + 1 %>
+
diff --git a/test/functional/changeset_controller_test.rb b/test/functional/changeset_controller_test.rb
index e8648e5c3..524fad91b 100644
--- a/test/functional/changeset_controller_test.rb
+++ b/test/functional/changeset_controller_test.rb
@@ -65,8 +65,14 @@ class ChangesetControllerTest < ActionController::TestCase
   def test_close
     basic_authorization "test@openstreetmap.org", "test"
 
-    put :close, :id => changesets(:normal_user_first_change).id
+    cs_id = changesets(:normal_user_first_change).id
+    put :close, :id => cs_id
     assert_response :success
+
+    # 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}.")
   end
 
   ##
@@ -669,7 +675,7 @@ EOF
   def test_query
     get :query, :bbox => "-10,-10, 10, 10"
     assert_response :success, "can't get changesets in bbox"
-    assert_changesets [1,4]
+    assert_changesets [1,4,6]
 
     get :query, :bbox => "4.5,4.5,4.6,4.6"
     assert_response :success, "can't get changesets in bbox"
@@ -683,7 +689,7 @@ EOF
     basic_authorization "test@openstreetmap.org", "test"
     get :query, :user => users(:normal_user).id
     assert_response :success, "can't get changesets by user"
-    assert_changesets [1,3,4]
+    assert_changesets [1,3,4,6]
 
     get :query, :user => users(:normal_user).id, :open => true
     assert_response :success, "can't get changesets by user and open"
@@ -691,15 +697,15 @@ EOF
 
     get :query, :time => '2007-12-31'
     assert_response :success, "can't get changesets by time-since"
-    assert_changesets [1,2,4,5]
+    assert_changesets [1,2,4,5,6]
 
     get :query, :time => '2008-01-01T12:34Z'
     assert_response :success, "can't get changesets by time-since with hour"
-    assert_changesets [1,2,4,5]
+    assert_changesets [1,2,4,5,6]
 
     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]
+    assert_changesets [1,4,5,6]
 
     get :query, :open => 'true'
     assert_response :success, "can't get changesets by open-ness"
@@ -840,6 +846,11 @@ EOF
 
     changeset = Changeset.find(cs_id)
     assert_equal Changeset::MAX_ELEMENTS + 1, changeset.num_changes
+
+    # check that the changeset is now closed as well
+    assert(!changeset.is_open?, 
+           "changeset should have been auto-closed by exceeding " + 
+           "element limit.")
   end
   
   #------------------------------------------------------------
diff --git a/test/unit/changeset_test.rb b/test/unit/changeset_test.rb
index ee9d0925a..4550ffba5 100644
--- a/test/unit/changeset_test.rb
+++ b/test/unit/changeset_test.rb
@@ -5,7 +5,7 @@ class ChangesetTest < Test::Unit::TestCase
   
   
   def test_changeset_count
-    assert_equal 5, Changeset.count
+    assert_equal 6, Changeset.count
   end
   
 end
-- 
2.43.2