moving the conistency checks for updates and deletes to library, hopefully got the...
authorShaun McDonald <shaun@shaunmcdonald.me.uk>
Thu, 9 Oct 2008 16:22:05 +0000 (16:22 +0000)
committerShaun McDonald <shaun@shaunmcdonald.me.uk>
Thu, 9 Oct 2008 16:22:05 +0000 (16:22 +0000)
app/controllers/node_controller.rb
app/controllers/relation_controller.rb
app/controllers/way_controller.rb
app/models/node.rb
app/models/relation.rb
app/models/way.rb
lib/geo_record.rb

index 8e8c844..29bf672 100644 (file)
@@ -76,7 +76,7 @@ class NodeController < ApplicationController
       end
     rescue OSM::APIVersionMismatchError => ex
       render :text => "Version mismatch: Provided " + ex.provided.to_s +
-       ", server had: " + ex.latest.to_s, :status => :bad_request
+        ", server had: " + ex.latest.to_s, :status => :bad_request
     rescue ActiveRecord::RecordNotFound
       render :nothing => true, :status => :not_found
     end
@@ -87,14 +87,19 @@ class NodeController < ApplicationController
   def delete
     begin
       node = Node.find(params[:id])
+      new_node = Node.from_xml(request.raw_post)
       # FIXME we no longer care about the user, (or maybe we want to check
       # that the user of the changeset is the same user as is making this
       # little change?) we really care about the 
       # changeset which must be open, and that the version that we have been
       # given is the one that is currently stored in the database
-      node.delete_with_history(@user)
-
-      render :nothing => true
+      
+      if new_node and new_node.id == node.id
+        node.delete_with_history(new_node, @user)
+        render :nothing => true, :status => :success
+      else
+        render :nothing => true, :status => :bad_request
+      end
     rescue ActiveRecord::RecordNotFound
       render :nothing => true, :status => :not_found
     rescue OSM::APIError => ex
index d879050..b77d41e 100644 (file)
@@ -68,7 +68,13 @@ class RelationController < ApplicationController
 #XXX check if member somewhere!
     begin
       relation = Relation.find(params[:id])
-      relation.delete_with_history(@user)
+      new_relation = Relation.from_xml(request.raw_post)
+      if new_relation and new_relation.id == relation.id
+        relation.delete_with_history(new_relation, @user)
+        render :nothing => true, :status => :success
+      else
+        render :nothing => true, :status => :bad_request
+      end
     rescue OSM::APIError => ex
       render ex.render_opts
     rescue ActiveRecord::RecordNotFound
index 17f166d..6f4704c 100644 (file)
@@ -68,10 +68,15 @@ class WayController < ApplicationController
   def delete
     begin
       way = Way.find(params[:id])
-      way.delete_with_history(@user)
+      new_way = Way.from_xml(request.raw_post)
+      if new_way and new_way.id == way.id
+        way.delete_with_history(@user)
 
-      # if we get here, all is fine, otherwise something will catch below.  
-      render :nothing => true
+        # if we get here, all is fine, otherwise something will catch below.  
+        render :nothing => true
+      else
+        render :nothing => true, :status => :bad_request
+      end
     rescue OSM::APIError => ex
       render ex.render_opts
     rescue ActiveRecord::RecordNotFound
index a25a19f..39e1228 100644 (file)
@@ -131,14 +131,16 @@ class Node < ActiveRecord::Base
     end
   end
 
-  def delete_with_history(user)
+  # Should probably be renamed delete_from to come in line with update
+  def delete_with_history(new_node, user)
     if self.visible
+      check_consistency(self, new_node, user)
       if WayNode.find(:first, :joins => "INNER JOIN current_ways ON current_ways.id = current_way_nodes.id", :conditions => [ "current_ways.visible = 1 AND current_way_nodes.node_id = ?", self.id ])
         raise OSM::APIPreconditionFailedError.new
       elsif RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = 1 AND member_type='node' and member_id=?", self.id])
         raise OSM::APIPreconditionFailedError.new
       else
-        self.user_id = user.id
+        self.changeset_id = new_node.changeset_id
         self.visible = 0
         save_with_history!
       end
@@ -148,15 +150,9 @@ class Node < ActiveRecord::Base
   end
 
   def update_from(new_node, user)
-    if new_node.version != version
-      raise OSM::APIVersionMismatchError.new(new_node.version, version)
-    elsif new_node.changeset.user_id != user.id
-      raise OSM::APIUserChangesetMismatchError.new
-    elsif not new_node.changeset.open?
-      raise OSM::APIChangesetAlreadyClosedError.new
-    end
+    check_consistency(self, new_node, user)
 
-    # FIXME logic need looked at
+    # FIXME logic needs to be double checked
     self.changeset_id = new_node.changeset_id
     self.latitude = new_node.latitude 
     self.longitude = new_node.longitude
index 93f0001..c8ee89d 100644 (file)
@@ -217,13 +217,15 @@ class Relation < ActiveRecord::Base
     end
   end
 
-  def delete_with_history(user)
+  def delete_with_history(new_relation, user)
     if self.visible
+      check_consistency(self, new_relation, user)
       if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = 1 AND member_type='relation' and member_id=?", self.id ])
         raise OSM::APIPreconditionFailedError.new
       else
         #self.user_id = user.id
         # FIXME we need to deal with changeset here, which is probably already dealt with
+        self.changeset_id = new_relation.changeset_id
         self.tags = []
         self.members = []
         self.visible = false
@@ -235,23 +237,17 @@ class Relation < ActiveRecord::Base
   end
 
   def update_from(new_relation, user)
+    check_consistency(self, new_relation, user)
     if !new_relation.preconditions_ok?
       raise OSM::APIPreconditionFailedError.new
-    elsif new_relation.version != version
-      raise OSM::APIVersionMismatchError.new(new_relation.version, version)
-    elsif new_relation.changeset.user_id != user.id
-      raise OSM::APIUserChangesetMismatchError.new
-    elsif not new_relation.changeset.open?
-      raise OSM::APIChangesetAlreadyClosedError.new
-    else
-      # FIXME need to deal with changeset etc
-      #self.user_id = user.id
-      self.changeset_id = new_relation.changeset_id
-      self.tags = new_relation.tags
-      self.members = new_relation.members
-      self.visible = true
-      save_with_history!
     end
+    # FIXME need to deal with changeset etc
+    #self.user_id = user.id
+    self.changeset_id = new_relation.changeset_id
+    self.tags = new_relation.tags
+    self.members = new_relation.members
+    self.visible = true
+    save_with_history!
   end
 
   def preconditions_ok?
index 9ddb603..05b412b 100644 (file)
@@ -206,17 +206,15 @@ class Way < ActiveRecord::Base
   end
 
   def update_from(new_way, user)
+    check_consistency(self, new_way, user)
     if !new_way.preconditions_ok?
       raise OSM::APIPreconditionFailedError.new
-    elsif new_way.version != version
-      raise OSM::APIVersionMismatchError.new(new_way.version, version)
-    else
-      self.user_id = user.id
-      self.tags = new_way.tags
-      self.nds = new_way.nds
-      self.visible = true
-      save_with_history!
     end
+    self.changeset_id = changeset_id
+    self.tags = new_way.tags
+    self.nds = new_way.nds
+    self.visible = true
+    save_with_history!
   end
 
   def preconditions_ok?
@@ -230,11 +228,13 @@ class Way < ActiveRecord::Base
     return true
   end
 
-  def delete_with_history(user)
+  def delete_with_history(new_way, user)
+    check_consistency(self, new_way, user)
     if self.visible
-         # FIXME
-         # this should actually delete the relations,
-         # not just throw a PreconditionFailed if it's a member of a relation!!
+      # FIXME
+      # this should actually delete the relations,
+      # not just throw a PreconditionFailed if it's a member of a relation!!
+      # WHY?? The editor should decide whether the node is in the relation or not!
 
       # FIXME: this should probably renamed to delete_with_history
       if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id",
@@ -242,7 +242,7 @@ class Way < ActiveRecord::Base
         raise OSM::APIPreconditionFailedError
       # end FIXME
       else
-        self.user_id = user.id
+        self.changeset_id = new_way.changeset_id
         self.tags = []
         self.nds = []
         self.visible = false
@@ -265,6 +265,7 @@ class Way < ActiveRecord::Base
       n.save_with_history!
     end
     
+    # FIXME needs more information passed in so that the changeset can be updated
     self.user_id = user.id
 
     self.delete_with_history(user)
index 2740eab..3eab72b 100644 (file)
@@ -42,6 +42,19 @@ module GeoRecord
     return self.longitude.to_f / SCALE
   end
 
+  # Generic checks that are run for the updates and deletes of
+  # node, ways and relations. This code is here to avoid duplication, 
+  # and allow the extention of the checks without having to modify the
+  # code in 6 places. This will throw an exception if there is an inconsistency
+  def check_consistency(old, new, user)
+    if new.version != old.version
+      raise OSM::APIVersionMismatchError.new(new.version, old.version)
+    elsif new.changeset.user_id != user.id
+      raise OSM::APIUserChangesetMismatchError.new
+    elsif not new.changeset.is_open?
+      raise OSM::APIChangesetAlreadyClosedError.new
+    end
+  end
 private
   
   def lat2y(a)