Some improvements for the XML parsing, as there are somethings that we don't need...

diff --git a/app/controllers/amf_controller.rb b/app/controllers/amf_controller.rb
index 9c9f85e1dda3e47efda55c553e0b1674971b74d3..39de7e671650a5192235034d012b8e22079f73d6 100644 (file)
--- a/app/controllers/amf_controller.rb
+++ b/app/controllers/amf_controller.rb
@@ -446,7 +446,10 @@ class AmfController < ApplicationController
   rescue OSM::APIChangesetAlreadyClosedError => ex
     return [-1, "The changeset #{ex.changeset.id} was closed at #{ex.changeset.closed_at}"]
   rescue OSM::APIVersionMismatchError => ex
-    return [-1, "You have taken too long to edit, please reload the area"]
+    # Really need to check to see whether this is a server load issue, and the 
+    # last version was in the same changeset, or belongs to the same user, then
+    # we can return something different
+    return [-3, "You have taken too long to edit, please reload the area"]
   rescue OSM::APIAlreadyDeletedError => ex
     return [-1, "The object has already been deleted"]
   rescue OSM::APIError => ex
@@ -570,6 +573,18 @@ class AmfController < ApplicationController
     end
 
     [0, originalway, way.id, renumberednodes, way.version]
+  rescue OSM::APIChangesetAlreadyClosedError => ex
+    return [-1, "The changeset #{ex.changeset.id} was closed at #{ex.changeset.closed_at}"]
+  rescue OSM::APIVersionMismatchError => ex
+    # Really need to check to see whether this is a server load issue, and the 
+    # last version was in the same changeset, or belongs to the same user, then
+    # we can return something different
+    return [-3, "You have taken too long to edit, please reload the area"]
+  rescue OSM::APIAlreadyDeletedError => ex
+    return [-1, "The object has already been deleted"]
+  rescue OSM::APIError => ex
+    # Some error that we don't specifically catch
+    return [-2, "Something really bad happened :-()"]
   end
 
   # Save POI to the database.
@@ -621,6 +636,18 @@ class AmfController < ApplicationController
     else
       return [0, id, node.id, node.version]
     end
+  rescue OSM::APIChangesetAlreadyClosedError => ex
+    return [-1, "The changeset #{ex.changeset.id} was closed at #{ex.changeset.closed_at}"]
+  rescue OSM::APIVersionMismatchError => ex
+        # Really need to check to see whether this is a server load issue, and the 
+    # last version was in the same changeset, or belongs to the same user, then
+    # we can return something different
+    return [-3, "You have taken too long to edit, please reload the area"]
+  rescue OSM::APIAlreadyDeletedError => ex
+    return [-1, "The object has already been deleted"]
+  rescue OSM::APIError => ex
+    # Some error that we don't specifically catch
+    return [-2, "Something really bad happened :-()"]
   end
 
   # Read POI from database
@@ -645,21 +672,23 @@ class AmfController < ApplicationController
   # Delete way and all constituent nodes. Also removes from any relations.
   # Returns 0 (success), unchanged way id.
 
-  def deleteway(usertoken, changeset_id, way_id) #:doc:
-    if !getuserid(usertoken) then return -1,"You are not logged in, so the way could not be deleted." end
-
-    way_id = way_id.to_i
+  def deleteway(usertoken, changeset_id, way_id, version_id) #:doc:
+    user = getuser(usertoken)
+    if user then return -1,"You are not logged in, so the way could not be deleted." end
+    # Need a transaction so that if one item fails to delete, the whole delete fails.
+    Way.transaction do
+      way_id = way_id.to_i
+
+      # FIXME: would be good not to make two history entries when removing
+      #                 two nodes from the same relation
+      old_way = Way.find(way_id)
+      old_way.unshared_node_ids.each do |n|
+        deleteitemrelations(n, 'node')
+      end
+      deleteitemrelations(way_id, 'way')
 
-    # FIXME: would be good not to make two history entries when removing
-    #           two nodes from the same relation
-    way = Way.find(way_id)
-    way.unshared_node_ids.each do |n|
-      deleteitemrelations(n, 'node')
+      way.delete_with_relations_and_nodes_and_history(changeset_id.to_i)
     end
-    deleteitemrelations(way_id, 'way')
-
-    way.delete_with_relations_and_nodes_and_history(changeset_id.to_i)
-
     [0, way_id]
   end
 
@@ -668,7 +697,7 @@ class AmfController < ApplicationController
   # Support functions
 
   # Remove a node or way from all relations
-
+  # FIXME needs version, changeset, and user
   def deleteitemrelations(objid, type) #:doc:
     relations = RelationMember.find(:all, 
                                                                        :conditions => ['member_type = ? and member_id = ?', type, objid], 
@@ -676,7 +705,12 @@ class AmfController < ApplicationController
 
     relations.each do |rel|
       rel.members.delete_if { |x| x[0] == type and x[1] == objid }
-      rel.save_with_history!
+      # FIXME need to create the new node/way based on the type.
+      new_rel = Relation.new
+      new_rel.version = version
+      new_rel.members = members
+      new_rel.changeset = changeset
+      rel.delete_with_history(new_rel, user)
     end
   end
 

diff --git a/app/models/node.rb b/app/models/node.rb
index ce2bd5c74aa4eb6d45fdfabab0e094333db1c64d..f2ad3a78add2196f117214816489d3c7bdc686cb 100644 (file)
--- a/app/models/node.rb
+++ b/app/models/node.rb
@@ -83,6 +83,7 @@ class Node < ActiveRecord::Base
     raise OSM::APIBadXMLError.new("node", pt, "lon missing") if pt['lon'].nil?
     node.lat = pt['lat'].to_f
     node.lon = pt['lon'].to_f
+    raise OSM::APIBadXMLError.new("node", pt, "changeset id missing") if pt['changeset'].nil?
     node.changeset_id = pt['changeset'].to_i
 
     raise OSM::APIBadUserInput.new("The node is outside this world") unless node.in_world?
@@ -99,15 +100,10 @@ class Node < ActiveRecord::Base
 
     # visible if it says it is, or as the default if the attribute
     # is missing.
-    node.visible = pt['visible'].nil? or pt['visible'] == 'true'
+    # Don't need to set the visibility, when it is set explicitly in the create/update/delete
+    #node.visible = pt['visible'].nil? or pt['visible'] == 'true'
 
-    if create
-      node.timestamp = Time.now
-    else
-      if pt['timestamp']
-        node.timestamp = Time.parse(pt['timestamp'])
-      end
-    end
+    # We don't care about the time, as it is explicitly set on create/update/delete
 
     tags = []
 

diff --git a/app/models/relation.rb b/app/models/relation.rb
index 94ef9d42a8f39552ffe59804b305b86c69e630d7..6be1061591dda7b665b20ceed5b5b81ae79e96d3 100644 (file)
--- a/app/models/relation.rb
+++ b/app/models/relation.rb
@@ -49,6 +49,8 @@ class Relation < ActiveRecord::Base
     raise OSM::APIBadXMLError.new("relation", pt, "You are missing the required changeset in the relation") 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
       relation.visible = true

diff --git a/app/models/way.rb b/app/models/way.rb
index ac4461f8e123efce46a265d193d99c8f91069a46..92b69ed7dc15c472c6779c5d5673736cdc65c89d 100644 (file)
--- a/app/models/way.rb
+++ b/app/models/way.rb
@@ -47,8 +47,10 @@ class Way < ActiveRecord::Base
     end
     
     way.version = pt['version']
+    raise OSM::APIBadXMLError.new("node", pt, "Changeset is required") if pt['changeset'].nil?
     way.changeset_id = pt['changeset']
 
+    # This next section isn't required for the create, update, or delete of ways
     if create
       way.timestamp = Time.now
       way.visible = true
@@ -244,7 +246,7 @@ class Way < ActiveRecord::Base
       check_consistency(self, new_way, user)
       if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id",
                              :conditions => [ "visible = ? AND member_type='way' and member_id=? ", true, self.id])
-        raise OSM::APIPreconditionFailedError
+        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.tags = []
@@ -259,12 +261,15 @@ class Way < ActiveRecord::Base
 
   # FIXME: merge the potlatch code to delete the relations
   #        and refactor to use delete_with_history!
+  # This really needs the ids and versions of the nodes/relations to be passed in too
+  # so that we can do the version checking before the delete
   def delete_with_relations_and_nodes_and_history(changeset_id)
     # delete the nodes not used by other ways
     self.unshared_node_ids.each do |node_id|
       n = Node.find(node_id)
       n.changeset_id = changeset_id
       n.visible = false
+      # FIXME next line is bad
       n.save_with_history!
     end
     
@@ -272,6 +277,7 @@ class Way < ActiveRecord::Base
     self.tags = []
     self.nds = []
     self.visible = false
+    # FIXME next line is bad
     self.save_with_history!
   end