Fixed a couple of bugs with changeset bounding box calculation when modifying or...

diff --git a/app/models/node.rb b/app/models/node.rb
index d3e0a7e8d392a470ed193eb6de4fff0738ee8e29..05aae0896847509a6d92c92c8011a54fe7d95808 100644 (file)
--- a/app/models/node.rb
+++ b/app/models/node.rb
@@ -150,17 +150,20 @@ class Node < ActiveRecord::Base
   def update_from(new_node, user)
     check_consistency(self, new_node, user)
 
   def update_from(new_node, user)
     check_consistency(self, new_node, user)
 

-    # update changeset with *old* position first
+    # update changeset first
+    self.changeset_id = new_node.changeset_id
+    self.changeset = new_node.changeset
+
+    # update changeset bbox with *old* position first

     changeset.update_bbox!(bbox);
 
     # FIXME logic needs to be double checked
     changeset.update_bbox!(bbox);
 
     # FIXME logic needs to be double checked

-    self.changeset_id = new_node.changeset_id

     self.latitude = new_node.latitude 
     self.longitude = new_node.longitude
     self.tags = new_node.tags
     self.visible = true
 
     self.latitude = new_node.latitude 
     self.longitude = new_node.longitude
     self.tags = new_node.tags
     self.visible = true
 

-    # update changeset with *new* position
+    # update changeset bbox with *new* position

     changeset.update_bbox!(bbox);
 
     save_with_history!
     changeset.update_bbox!(bbox);
 
     save_with_history!

diff --git a/app/models/relation.rb b/app/models/relation.rb
index c3769b14d68e2a126d2f54002e746127bc61d80d..4b5d9e32b89ac90832e674e5434e328e1e0f92bc 100644 (file)
--- a/app/models/relation.rb
+++ b/app/models/relation.rb
@@ -253,6 +253,7 @@ class Relation < ActiveRecord::Base
       raise OSM::APIPreconditionFailedError.new
     end
     self.changeset_id = new_relation.changeset_id
       raise OSM::APIPreconditionFailedError.new
     end
     self.changeset_id = new_relation.changeset_id

+    self.changeset = new_relation.changeset

     self.tags = new_relation.tags
     self.members = new_relation.members
     self.visible = true
     self.tags = new_relation.tags
     self.members = new_relation.members
     self.visible = true


@@ -372,6 +373,10 @@ class Relation < ActiveRecord::Base
         tag.id = self.id
         tag.save!
       end
         tag.id = self.id
         tag.save!
       end

+      
+      # reload, so that all of the members are accessible in their
+      # new state.
+      self.reload

 
       # same pattern as before, but this time we're collecting the
       # changed members in an array, as the bounding box updates for
 
       # same pattern as before, but this time we're collecting the
       # changed members in an array, as the bounding box updates for

diff --git a/app/models/way.rb b/app/models/way.rb
index 88bbaf6c59669229dfb221659389cf1605442d22..94a6fa7548faf23299219d5e7f9d5aed72e82847 100644 (file)
--- a/app/models/way.rb
+++ b/app/models/way.rb
@@ -202,7 +202,9 @@ class Way < ActiveRecord::Base
     if !new_way.preconditions_ok?
       raise OSM::APIPreconditionFailedError.new
     end
     if !new_way.preconditions_ok?
       raise OSM::APIPreconditionFailedError.new
     end

+

     self.changeset_id = new_way.changeset_id
     self.changeset_id = new_way.changeset_id

+    self.changeset = new_way.changeset

     self.tags = new_way.tags
     self.nds = new_way.nds
     self.visible = true
     self.tags = new_way.tags
     self.nds = new_way.nds
     self.visible = true


@@ -248,6 +250,8 @@ class Way < ActiveRecord::Base
         raise OSM::APIPreconditionFailedError.new("You need to make sure that this way is not a member of a relation.")
       else
         self.changeset_id = new_way.changeset_id
         raise OSM::APIPreconditionFailedError.new("You need to make sure that this way is not a member of a relation.")
       else
         self.changeset_id = new_way.changeset_id

+        self.changeset = new_way.changeset
+

         self.tags = []
         self.nds = []
         self.visible = false
         self.tags = []
         self.nds = []
         self.visible = false


@@ -294,11 +298,12 @@ class Way < ActiveRecord::Base
   def save_with_history!
     t = Time.now
 
   def save_with_history!
     t = Time.now
 

-    # update the bounding box, but don't save it as the controller knows the 
-    # lifetime of the change better. note that this has to be done both before 
+    # update the bounding box, note that this has to be done both before 

     # and after the save, so that nodes from both versions are included in the 
     # and after the save, so that nodes from both versions are included in the 

-    # bbox.
-    changeset.update_bbox!(bbox) unless nodes.empty?
+    # bbox. we use a copy of the changeset so that it isn't reloaded
+    # later in the save.
+    cs = self.changeset
+    cs.update_bbox!(bbox) unless nodes.empty?

 
     Way.transaction do
       self.version += 1
 
     Way.transaction do
       self.version += 1


@@ -330,14 +335,18 @@ class Way < ActiveRecord::Base
       old_way.timestamp = t
       old_way.save_with_dependencies!
 
       old_way.timestamp = t
       old_way.save_with_dependencies!
 

+      # reload the way so that the nodes array points to the correct
+      # new set of nodes.
+      self.reload
+

       # update and commit the bounding box, now that way nodes 
       # have been updated and we're in a transaction.
       # update and commit the bounding box, now that way nodes 
       # have been updated and we're in a transaction.

-      changeset.update_bbox!(bbox) unless nodes.empty?
+      cs.update_bbox!(bbox) unless nodes.empty?

 
       # tell the changeset we updated one element only
 
       # tell the changeset we updated one element only

-      changeset.add_changes! 1
+      cs.add_changes! 1

 
 

-      changeset.save!
+      cs.save!

     end
   end
 
     end
   end
 

diff --git a/test/functional/changeset_controller_test.rb b/test/functional/changeset_controller_test.rb
index 591241aa08a208c0ee03c047c8026dc86efa9fe6..4c98fb36d037fabb064c9aded73e2172e44fbc2e 100644 (file)
--- a/test/functional/changeset_controller_test.rb
+++ b/test/functional/changeset_controller_test.rb
@@ -553,6 +553,84 @@ EOF
       "shouldn't be able to re-use placeholder IDs"
   end
 
       "shouldn't be able to re-use placeholder IDs"
   end
 

+  ##
+  # test what happens if a diff is uploaded containing only a node
+  # move.
+  def test_upload_node_move
+    basic_authorization "test@openstreetmap.org", "test"
+
+    content "<osm><changeset>" +
+      "<tag k='created_by' v='osm test suite checking changesets'/>" + 
+      "</changeset></osm>"
+    put :create
+    assert_response :success
+    changeset_id = @response.body.to_i
+
+    old_node = current_nodes(:visible_node)
+
+    diff = XML::Document.new
+    diff.root = XML::Node.new "osmChange"
+    modify = XML::Node.new "modify"
+    xml_old_node = old_node.to_xml_node
+    xml_old_node["lat"] = (2.0).to_s
+    xml_old_node["lon"] = (2.0).to_s
+    xml_old_node["changeset"] = changeset_id.to_s
+    modify << xml_old_node
+    diff.root << modify
+
+    # upload it
+    content diff
+    post :upload, :id => changeset_id
+    assert_response :success, 
+      "diff should have uploaded OK"
+
+    # check the bbox
+    changeset = Changeset.find(changeset_id)
+    assert_equal 1*SCALE, changeset.min_lon, "min_lon should be 1 degree"
+    assert_equal 2*SCALE, changeset.max_lon, "max_lon should be 2 degrees"
+    assert_equal 1*SCALE, changeset.min_lat, "min_lat should be 1 degree"
+    assert_equal 2*SCALE, changeset.max_lat, "max_lat should be 2 degrees"
+  end
+
+  ##
+  # test what happens if a diff is uploaded adding a node to a way.
+  def test_upload_way_extend
+    basic_authorization "test@openstreetmap.org", "test"
+
+    content "<osm><changeset>" +
+      "<tag k='created_by' v='osm test suite checking changesets'/>" + 
+      "</changeset></osm>"
+    put :create
+    assert_response :success
+    changeset_id = @response.body.to_i
+
+    old_way = current_ways(:visible_way)
+
+    diff = XML::Document.new
+    diff.root = XML::Node.new "osmChange"
+    modify = XML::Node.new "modify"
+    xml_old_way = old_way.to_xml_node
+    nd_ref = XML::Node.new "nd"
+    nd_ref["ref"] = current_nodes(:visible_node).id.to_s
+    xml_old_way << nd_ref
+    xml_old_way["changeset"] = changeset_id.to_s
+    modify << xml_old_way
+    diff.root << modify
+
+    # upload it
+    content diff
+    post :upload, :id => changeset_id
+    assert_response :success, 
+      "diff should have uploaded OK"
+
+    # check the bbox
+    changeset = Changeset.find(changeset_id)
+    assert_equal 1*SCALE, changeset.min_lon, "min_lon should be 1 degree"
+    assert_equal 3*SCALE, changeset.max_lon, "max_lon should be 3 degrees"
+    assert_equal 1*SCALE, changeset.min_lat, "min_lat should be 1 degree"
+    assert_equal 3*SCALE, changeset.max_lat, "max_lat should be 3 degrees"
+  end
+

   ##
   # test for more issues in #1568
   def test_upload_empty_invalid
   ##
   # test for more issues in #1568
   def test_upload_empty_invalid


@@ -773,7 +851,7 @@ EOF
     assert_select "osm>changeset[min_lat=1.0]", 1
     assert_select "osm>changeset[max_lat=2.0]", 1
 
     assert_select "osm>changeset[min_lat=1.0]", 1
     assert_select "osm>changeset[max_lat=2.0]", 1
 

-    # add (delete) a way to it
+    # add (delete) a way to it, which contains a point at (3,3)

     with_controller(WayController.new) do
       content update_changeset(current_ways(:visible_way).to_xml,
                                changeset_id)
     with_controller(WayController.new) do
       content update_changeset(current_ways(:visible_way).to_xml,
                                changeset_id)


@@ -784,6 +862,7 @@ EOF
     # get the bounding box back from the changeset
     get :read, :id => changeset_id
     assert_response :success, "Couldn't read back changeset for the third time."
     # get the bounding box back from the changeset
     get :read, :id => changeset_id
     assert_response :success, "Couldn't read back changeset for the third time."

+    # note that the 3.1 here is because of the bbox overexpansion

     assert_select "osm>changeset[min_lon=1.0]", 1
     assert_select "osm>changeset[max_lon=3.1]", 1
     assert_select "osm>changeset[min_lat=1.0]", 1
     assert_select "osm>changeset[min_lon=1.0]", 1
     assert_select "osm>changeset[max_lon=3.1]", 1
     assert_select "osm>changeset[min_lat=1.0]", 1