From: Shaun McDonald Date: Wed, 10 Jun 2009 17:17:03 +0000 (+0000) Subject: Cleanup the Relation.from_xml to come in line with the Way and Node versions. Include... X-Git-Tag: live~7079 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/37df03a0432c0fcbc1c1b91af741a5cc135bea34?hp=3b063cfa19eca6207f87deb7dc10547894724714 Cleanup the Relation.from_xml to come in line with the Way and Node versions. Includes tests. Bug fix for the previous tests. --- diff --git a/app/models/relation.rb b/app/models/relation.rb index fda5e5677..28fa326ba 100644 --- a/app/models/relation.rb +++ b/app/models/relation.rb @@ -41,25 +41,23 @@ class Relation < ActiveRecord::Base def self.from_xml_node(pt, create=false) relation = Relation.new - if !create and pt['id'] != '0' - relation.id = pt['id'].to_i - end - - raise OSM::APIBadXMLError.new("relation", pt, "You are missing the required changeset in the relation") if pt['changeset'].nil? + raise OSM::APIBadXMLError.new("relation", pt, "Version is required when updating") unless create or not pt['version'].nil? + relation.version = pt['version'] + raise OSM::APIBadXMLError.new("relation", pt, "Changeset id is missing") if pt['changeset'].nil? relation.changeset_id = pt['changeset'] - - # The follow block does not need to be executed because they are dealt with - # in create_with_history, update_from and delete_with_history - if create - relation.timestamp = Time.now.getutc - relation.visible = true - relation.version = 0 - else - if pt['timestamp'] - relation.timestamp = Time.parse(pt['timestamp']) - end - relation.version = pt['version'] + + unless create + raise OSM::APIBadXMLError.new("relation", pt, "ID is required when updating") if pt['id'].nil? + relation.id = pt['id'].to_i + # .to_i will return 0 if there is no number that can be parsed. + # We want to make sure that there is no id with zero anyway + raise OSM::APIBadUserInput.new("ID of relation cannot be zero when updating.") if relation.id == 0 end + + # We don't care about the timestamp nor the visibility as these are either + # set explicitly or implicit in the action. The visibility is set to true, + # and manually set to false before the actual delete. + relation.visible = true pt.find('tag').each do |tag| relation.add_tag_keyval(tag['k'], tag['v']) diff --git a/test/functional/relation_controller_test.rb b/test/functional/relation_controller_test.rb index 10762f48d..19b3617fc 100644 --- a/test/functional/relation_controller_test.rb +++ b/test/functional/relation_controller_test.rb @@ -433,10 +433,10 @@ class RelationControllerTest < ActionController::TestCase assert_response :bad_request # try to delete without specifying a changeset - content "" + content "" delete :delete, :id => current_relations(:visible_relation).id assert_response :bad_request - assert_match(/You are missing the required changeset in the relation/, @response.body) + assert_match(/Changeset id is missing/, @response.body) # try to delete with an invalid (closed) changeset content update_changeset(current_relations(:visible_relation).to_xml, diff --git a/test/unit/node_test.rb b/test/unit/node_test.rb index 11b745f5e..8e71233e6 100644 --- a/test/unit/node_test.rb +++ b/test/unit/node_test.rb @@ -267,6 +267,6 @@ class NodeTest < ActiveSupport::TestCase message_update = assert_raise(OSM::APIBadXMLError) { Node.from_xml(no_text, false) } - assert_match /Must specify a string with one or more characters/, message_create.message + assert_match /Must specify a string with one or more characters/, message_update.message end end diff --git a/test/unit/relation_test.rb b/test/unit/relation_test.rb index 6b2363025..1c46c7012 100644 --- a/test/unit/relation_test.rb +++ b/test/unit/relation_test.rb @@ -7,4 +7,63 @@ class RelationTest < ActiveSupport::TestCase assert_equal 6, Relation.count end + def test_from_xml_no_id + noid = "" + assert_nothing_raised(OSM::APIBadXMLError) { + Relation.from_xml(noid, true) + } + message = assert_raise(OSM::APIBadXMLError) { + Relation.from_xml(noid, false) + } + assert_match /ID is required when updating/, message.message + end + + def test_from_xml_no_changeset_id + nocs = "" + message_create = assert_raise(OSM::APIBadXMLError) { + Relation.from_xml(nocs, true) + } + assert_match /Changeset id is missing/, message_create.message + message_update = assert_raise(OSM::APIBadXMLError) { + Relation.from_xml(nocs, false) + } + assert_match /Changeset id is missing/, message_update.message + end + + def test_from_xml_no_version + no_version = "" + assert_nothing_raised(OSM::APIBadXMLError) { + Relation.from_xml(no_version, true) + } + message_update = assert_raise(OSM::APIBadXMLError) { + Relation.from_xml(no_version, false) + } + assert_match /Version is required when updating/, message_update.message + end + + def test_from_xml_id_zero + id_list = ["", "0", "00", "0.0", "a"] + id_list.each do |id| + zero_id = "" + assert_nothing_raised(OSM::APIBadUserInput) { + Relation.from_xml(zero_id, true) + } + message_update = assert_raise(OSM::APIBadUserInput) { + Relation.from_xml(zero_id, false) + } + assert_match /ID of relation cannot be zero when updating/, message_update.message + end + end + + def test_from_xml_no_text + no_text = "" + message_create = assert_raise(OSM::APIBadXMLError) { + Relation.from_xml(no_text, true) + } + assert_match /Must specify a string with one or more characters/, message_create.message + message_update = assert_raise(OSM::APIBadXMLError) { + Relation.from_xml(no_text, false) + } + assert_match /Must specify a string with one or more characters/, message_update.message + end end diff --git a/test/unit/way_test.rb b/test/unit/way_test.rb index afbe36252..bdf4ab365 100644 --- a/test/unit/way_test.rb +++ b/test/unit/way_test.rb @@ -94,6 +94,6 @@ class WayTest < ActiveSupport::TestCase message_update = assert_raise(OSM::APIBadXMLError) { Way.from_xml(no_text, false) } - assert_match /Must specify a string with one or more characters/, message_create.message + assert_match /Must specify a string with one or more characters/, message_update.message end end