cst1/openstreetmap-website
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Actually the changeset doesn't need an id attribute in the xml. It is simply fetched as a parameter in the url. Thanks for pointing it out Frederik. We really need more tests so that things like this are thought about more before committing potentially significant changes.

Browse source
This commit is contained in:
Shaun McDonald 2009-12-15 23:53:19 +00:00
parent 1fb6325630
commit 094855be22
2 changed files with 41 additions and 50 deletions
Download patch file Download diff file Expand all files Collapse all files

48
app/models/changeset.rb
View file

@ -4,14 +4,14 @@ class Changeset < ActiveRecord::Base
belongs_to :user belongs_to :user
has_many :changeset_tags, :foreign_key => 'id' has_many :changeset_tags, :foreign_key => 'id'
has_many :nodes has_many :nodes
has_many :ways has_many :ways
has_many :relations has_many :relations
has_many :old_nodes has_many :old_nodes
has_many :old_ways has_many :old_ways
has_many :old_relations has_many :old_relations
validates_presence_of :id, :on => :update validates_presence_of :id, :on => :update
validates_presence_of :user_id, :created_at, :closed_at, :num_changes validates_presence_of :user_id, :created_at, :closed_at, :num_changes
validates_uniqueness_of :id validates_uniqueness_of :id
@ -34,13 +34,13 @@ class Changeset < ActiveRecord::Base
IDLE_TIMEOUT = 1.hour IDLE_TIMEOUT = 1.hour
# Use a method like this, so that we can easily change how we # Use a method like this, so that we can easily change how we
# determine whether a changeset is open, without breaking code in at # determine whether a changeset is open, without breaking code in at
# least 6 controllers # least 6 controllers
def is_open? def is_open?
# a changeset is open (that is, it will accept further changes) when # a changeset is open (that is, it will accept further changes) when
# it has not yet run out of time and its capacity is small enough. # it has not yet run out of time and its capacity is small enough.
# note that this may not be a hard limit - due to timing changes and # note that this may not be a hard limit - due to timing changes and
# concurrency it is possible that some changesets may be slightly # concurrency it is possible that some changesets may be slightly
# longer than strictly allowed or have slightly more changes in them. # longer than strictly allowed or have slightly more changes in them.
return ((closed_at > Time.now.getutc) and (num_changes <= MAX_ELEMENTS)) return ((closed_at > Time.now.getutc) and (num_changes <= MAX_ELEMENTS))
end end
@ -50,7 +50,7 @@ class Changeset < ActiveRecord::Base
self.closed_at = Time.now.getutc self.closed_at = Time.now.getutc
end end
end end
def self.from_xml(xml, create=false) def self.from_xml(xml, create=false)
begin begin
p = XML::Parser.string(xml) p = XML::Parser.string(xml)
@ -64,7 +64,7 @@ class Changeset < ActiveRecord::Base
raise OSM::APIBadXMLError.new("changeset", xml, ex.message) raise OSM::APIBadXMLError.new("changeset", xml, ex.message)
end end
end end
def self.from_xml_node(pt, create=false) def self.from_xml_node(pt, create=false)
cs = Changeset.new cs = Changeset.new
if create if create
@ -74,12 +74,6 @@ class Changeset < ActiveRecord::Base
cs.closed_at = cs.created_at + IDLE_TIMEOUT cs.closed_at = cs.created_at + IDLE_TIMEOUT
# initially we have no changes in a changeset # initially we have no changes in a changeset
cs.num_changes = 0 cs.num_changes = 0
else
raise OSM::APIBadXMLError.new("changeset", pt, "ID is required when updating.") if pt['id'].nil?
cs.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 changeset cannot be zero when updating.") if cs.id == 0
end end
pt.find('tag').each do |tag| pt.find('tag').each do |tag|
@ -87,7 +81,7 @@ class Changeset < ActiveRecord::Base
raise OSM::APIBadXMLError.new("changeset", pt, "tag is missing value") if tag['v'].nil? raise OSM::APIBadXMLError.new("changeset", pt, "tag is missing value") if tag['v'].nil?
cs.add_tag_keyval(tag['k'], tag['v']) cs.add_tag_keyval(tag['k'], tag['v'])
end end
return cs return cs
end end
@ -97,33 +91,33 @@ class Changeset < ActiveRecord::Base
def bbox def bbox
@bbox ||= [ min_lon, min_lat, max_lon, max_lat ] @bbox ||= [ min_lon, min_lat, max_lon, max_lat ]
end end
def has_valid_bbox? def has_valid_bbox?
not bbox.include? nil not bbox.include? nil
end end
## ##
# returns area of the changset bbox as a rough comparitive quantity for use of changset displays # returns area of the changset bbox as a rough comparitive quantity for use of changset displays
def area def area
if has_valid_bbox? if has_valid_bbox?
(max_lon - min_lon) * (max_lat - min_lat) (max_lon - min_lon) * (max_lat - min_lat)
else else
0 0
end end
end end
## ##
# expand the bounding box to include the given bounding box. also, # expand the bounding box to include the given bounding box. also,
# expand a little bit more in the direction of the expansion, so that # expand a little bit more in the direction of the expansion, so that
# further expansions may be unnecessary. this is an optimisation # further expansions may be unnecessary. this is an optimisation
# suggested on the wiki page by kleptog. # suggested on the wiki page by kleptog.
def update_bbox!(array) def update_bbox!(array)
# ensure that bbox is cached and has no nils in it. if there are any # ensure that bbox is cached and has no nils in it. if there are any
# nils, just use the bounding box update to write over them. # nils, just use the bounding box update to write over them.
@bbox = bbox.zip(array).collect { |a, b| a.nil? ? b : a } @bbox = bbox.zip(array).collect { |a, b| a.nil? ? b : a }
# FIXME - this looks nasty and violates DRY... is there any prettier # FIXME - this looks nasty and violates DRY... is there any prettier
# way to do this? # way to do this?
@bbox[0] = [-180 * GeoRecord::SCALE, array[0] + EXPAND * (@bbox[0] - @bbox[2])].max if array[0] < @bbox[0] @bbox[0] = [-180 * GeoRecord::SCALE, array[0] + EXPAND * (@bbox[0] - @bbox[2])].max if array[0] < @bbox[0]
@bbox[1] = [ -90 * GeoRecord::SCALE, array[1] + EXPAND * (@bbox[1] - @bbox[3])].max if array[1] < @bbox[1] @bbox[1] = [ -90 * GeoRecord::SCALE, array[1] + EXPAND * (@bbox[1] - @bbox[3])].max if array[1] < @bbox[1]
@bbox[2] = [ 180 * GeoRecord::SCALE, array[2] + EXPAND * (@bbox[2] - @bbox[0])].min if array[2] > @bbox[2] @bbox[2] = [ 180 * GeoRecord::SCALE, array[2] + EXPAND * (@bbox[2] - @bbox[0])].min if array[2] > @bbox[2]
@ -202,13 +196,13 @@ class Changeset < ActiveRecord::Base
end end
end end
end end
def to_xml def to_xml
doc = OSM::API.new.get_xml_doc doc = OSM::API.new.get_xml_doc
doc.root << to_xml_node() doc.root << to_xml_node()
return doc return doc
end end
def to_xml_node(user_display_name_cache = nil) def to_xml_node(user_display_name_cache = nil)
el1 = XML::Node.new 'changeset' el1 = XML::Node.new 'changeset'
el1['id'] = self.id.to_s el1['id'] = self.id.to_s
@ -232,7 +226,7 @@ class Changeset < ActiveRecord::Base
el2['v'] = v.to_s el2['v'] = v.to_s
el1 << el2 el1 << el2
end end
el1['created_at'] = self.created_at.xmlschema el1['created_at'] = self.created_at.xmlschema
el1['closed_at'] = self.closed_at.xmlschema unless is_open? el1['closed_at'] = self.closed_at.xmlschema unless is_open?
el1['open'] = is_open?.to_s el1['open'] = is_open?.to_s
@ -241,7 +235,7 @@ class Changeset < ActiveRecord::Base
el1['min_lat'] = (bbox[1].to_f / GeoRecord::SCALE).to_s unless bbox[1].nil? el1['min_lat'] = (bbox[1].to_f / GeoRecord::SCALE).to_s unless bbox[1].nil?
el1['max_lon'] = (bbox[2].to_f / GeoRecord::SCALE).to_s unless bbox[2].nil? el1['max_lon'] = (bbox[2].to_f / GeoRecord::SCALE).to_s unless bbox[2].nil?
el1['max_lat'] = (bbox[3].to_f / GeoRecord::SCALE).to_s unless bbox[3].nil? el1['max_lat'] = (bbox[3].to_f / GeoRecord::SCALE).to_s unless bbox[3].nil?
# NOTE: changesets don't include the XML of the changes within them, # NOTE: changesets don't include the XML of the changes within them,
# they are just structures for tagging. to get the osmChange of a # they are just structures for tagging. to get the osmChange of a
# changeset, see the download method of the controller. # changeset, see the download method of the controller.
@ -255,10 +249,10 @@ class Changeset < ActiveRecord::Base
# bounding box, only the tags of the changeset. # bounding box, only the tags of the changeset.
def update_from(other, user) def update_from(other, user)
# ensure that only the user who opened the changeset may modify it. # ensure that only the user who opened the changeset may modify it.
unless user.id == self.user_id unless user.id == self.user_id
raise OSM::APIUserChangesetMismatchError.new raise OSM::APIUserChangesetMismatchError.new
end end
# can't change a closed changeset # can't change a closed changeset
unless is_open? unless is_open?
raise OSM::APIChangesetAlreadyClosedError.new(self) raise OSM::APIChangesetAlreadyClosedError.new(self)

43
test/unit/changeset_test.rb
View file

@ -2,25 +2,11 @@ require File.dirname(__FILE__) + '/../test_helper'
class ChangesetTest < ActiveSupport::TestCase class ChangesetTest < ActiveSupport::TestCase
api_fixtures api_fixtures
def test_changeset_count def test_changeset_count
assert_equal 7, Changeset.count assert_equal 7, Changeset.count
end end
def test_xml_from_id_zero
id_list = ["", "0", "00", "0.0", "a"]
id_list.each do |id|
zero_id = "<osm><changeset id='#{id}' /></osm>"
assert_nothing_raised(OSM::APIBadUserInput) {
Changeset.from_xml(zero_id, true)
}
message_update = assert_raise(OSM::APIBadUserInput) {
Changeset.from_xml(zero_id, false)
}
assert_match /ID of changeset cannot be zero when updating/, message_update.message
end
end
def test_from_xml_no_text def test_from_xml_no_text
no_text = "" no_text = ""
message_create = assert_raise(OSM::APIBadXMLError) { message_create = assert_raise(OSM::APIBadXMLError) {
@ -32,7 +18,7 @@ class ChangesetTest < ActiveSupport::TestCase
} }
assert_match /Must specify a string with one or more characters/, message_update.message assert_match /Must specify a string with one or more characters/, message_update.message
end end
def test_from_xml_no_changeset def test_from_xml_no_changeset
nocs = "<osm></osm>" nocs = "<osm></osm>"
message_create = assert_raise(OSM::APIBadXMLError) { message_create = assert_raise(OSM::APIBadXMLError) {
@ -44,7 +30,7 @@ class ChangesetTest < ActiveSupport::TestCase
} }
assert_match /XML doesn't contain an osm\/changeset element/, message_update.message assert_match /XML doesn't contain an osm\/changeset element/, message_update.message
end end
def test_from_xml_no_k_v def test_from_xml_no_k_v
nokv = "<osm><changeset><tag /></changeset></osm>" nokv = "<osm><changeset><tag /></changeset></osm>"
message_create = assert_raise(OSM::APIBadXMLError) { message_create = assert_raise(OSM::APIBadXMLError) {
@ -56,9 +42,9 @@ class ChangesetTest < ActiveSupport::TestCase
} }
assert_match /tag is missing key/, message_create.message assert_match /tag is missing key/, message_create.message
end end
def test_from_xml_no_v def test_from_xml_no_v
no_v = "<osm><changeset id='1'><tag k='key' /></changeset></osm>" no_v = "<osm><changeset><tag k='key' /></changeset></osm>"
message_create = assert_raise(OSM::APIBadXMLError) { message_create = assert_raise(OSM::APIBadXMLError) {
Changeset.from_xml(no_v, true) Changeset.from_xml(no_v, true)
} }
@ -68,9 +54,9 @@ class ChangesetTest < ActiveSupport::TestCase
} }
assert_match /tag is missing value/, message_update.message assert_match /tag is missing value/, message_update.message
end end
def test_from_xml_duplicate_k def test_from_xml_duplicate_k
dupk = "<osm><changeset id='1'><tag k='dup' v='test' /><tag k='dup' v='value' /></changeset></osm>" dupk = "<osm><changeset><tag k='dup' v='test' /><tag k='dup' v='value' /></changeset></osm>"
message_create = assert_raise(OSM::APIDuplicateTagsError) { message_create = assert_raise(OSM::APIDuplicateTagsError) {
Changeset.from_xml(dupk, true) Changeset.from_xml(dupk, true)
} }
@ -78,6 +64,17 @@ class ChangesetTest < ActiveSupport::TestCase
message_update = assert_raise(OSM::APIDuplicateTagsError) { message_update = assert_raise(OSM::APIDuplicateTagsError) {
Changeset.from_xml(dupk, false) Changeset.from_xml(dupk, false)
} }
assert_equal "Element changeset/1 has duplicate tags with key dup", message_update.message assert_equal "Element changeset/ has duplicate tags with key dup", message_update.message
end
def test_from_xml_valid
# Example taken from the Update section on the API_v0.6 docs on the wiki
xml = "<osm><changeset><tag k=\"comment\" v=\"Just adding some streetnames and a restaurant\"/></changeset></osm>"
assert_nothing_raised(OSM::APIBadXMLError) {
Changeset.from_xml(xml, false)
}
assert_nothing_raised(OSM::APIBadXMLError) {
Changeset.from_xml(xml, true)
}
end end
end end