From: Shaun McDonald <shaun@shaunmcdonald.me.uk>
Date: Mon, 7 Jul 2008 14:20:27 +0000 (+0000)
Subject: Fixing the resync that I had done wrong at the end of last week on the nodes. All... 
X-Git-Tag: live~8121^2~323
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/64a5e21f53be132cd5fe3265230c6cdc00ccabaa

Fixing the resync that I had done wrong at the end of last week on the nodes. All unit tests now do work. More assertions added. Using a scaling factor as a constant, so that the nodes are stored in the db correctly. Using the same scaling factor in the georecord library, to make code more readable, and reduce errors. Removing duplicate code that is in the GeoRecord include library.
---

diff --git a/app/models/node.rb b/app/models/node.rb
index abfa44d67..7f9b939db 100644
--- a/app/models/node.rb
+++ b/app/models/node.rb
@@ -1,6 +1,5 @@
 class Node < ActiveRecord::Base
   require 'xml/libxml'
-
   include GeoRecord
 
   set_table_name 'current_nodes'
@@ -10,17 +9,14 @@ class Node < ActiveRecord::Base
   validates_numericality_of :latitude, :longitude
   validate :validate_position
 
-  belongs_to :user
-
   has_many :old_nodes, :foreign_key => :id
-
   has_many :way_nodes
-  has_many :ways, :through => :way_nodes
-
+  has_many :node_tags, :foreign_key => :id
+  belongs_to :user
+  
   has_many :containing_relation_members, :class_name => "RelationMember", :as => :member
   has_many :containing_relations, :class_name => "Relation", :through => :containing_relation_members, :source => :relation, :extend => ObjectFinder
-
-  # Sanity check the latitude and longitude and add an error if it's broken
+ 
   def validate_position
     errors.add_to_base("Node is not in the world") unless in_world?
   end
@@ -57,64 +53,109 @@ class Node < ActiveRecord::Base
       p = XML::Parser.new
       p.string = xml
       doc = p.parse
-  
-      node = Node.new
 
       doc.find('//osm/node').each do |pt|
-        node.lat = pt['lat'].to_f
-        node.lon = pt['lon'].to_f
+        return Node.from_xml_node(pt, create)
+      end
+    rescue
+      return nil
+    end
+  end
 
-        return nil unless node.in_world?
+  def self.from_xml_node(pt, create=false)
+    node = Node.new
+    
+    node.version = pt['version']
+    node.lat = pt['lat'].to_f
+    node.lon = pt['lon'].to_f
 
-        unless create
-          if pt['id'] != '0'
-            node.id = pt['id'].to_i
-          end
-        end
+    return nil unless node.in_world?
 
-        node.visible = pt['visible'] and pt['visible'] == 'true'
+    unless create
+      if pt['id'] != '0'
+        node.id = pt['id'].to_i
+      end
+    end
 
-        if create
-          node.timestamp = Time.now
-        else
-          if pt['timestamp']
-            node.timestamp = Time.parse(pt['timestamp'])
-          end
-        end
+    node.visible = pt['visible'] and pt['visible'] == 'true'
 
-        tags = []
+    if create
+      node.timestamp = Time.now
+    else
+      if pt['timestamp']
+        node.timestamp = Time.parse(pt['timestamp'])
+      end
+    end
 
-        pt.find('tag').each do |tag|
-          tags << [tag['k'],tag['v']]
-        end
+    tags = []
 
-        node.tags = Tags.join(tags)
-      end
-    rescue
-      node = nil
+    pt.find('tag').each do |tag|
+      node.add_tag_key_val(tag['k'],tag['v'])
     end
 
     return node
   end
 
-  # Save this node with the appropriate OldNode object to represent it's history.
   def save_with_history!
+    t = Time.now
     Node.transaction do
-      self.timestamp = Time.now
+      self.version += 1
+      self.timestamp = t
       self.save!
+
+      # Create a NodeTag
+      tags = self.tags
+      NodeTag.delete_all(['id = ?', self.id])
+      tags.each do |k,v|
+        tag = NodeTag.new
+        tag.k = k 
+        tag.v = v 
+        tag.id = self.id
+        tag.save!
+      end 
+
+      # Create an OldNode
       old_node = OldNode.from_node(self)
-      old_node.save!
+      old_node.timestamp = t
+      old_node.save_with_dependencies!
+    end
+  end
+
+  def delete_with_history(user)
+    if self.visible
+      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.visible = 0
+	save_with_history!
+      end
+    else
+      raise OSM::APIAlreadyDeletedError.new
+    end
+  end
+
+  def update_from(new_node, user)
+    if new_node.version != version
+      raise OSM::APIVersionMismatchError.new(new_node.version, version)
     end
+
+    self.user_id = user.id
+    self.latitude = new_node.latitude 
+    self.longitude = new_node.longitude
+    self.tags = new_node.tags
+    self.visible = true
+    save_with_history!
   end
 
-  # Turn this Node in to a complete OSM XML object with <osm> wrapper
   def to_xml
     doc = OSM::API.new.get_xml_doc
     doc.root << to_xml_node()
     return doc
   end
 
-  # Turn this Node in to an XML Node without the <osm> wrapper.
   def to_xml_node(user_display_name_cache = nil)
     el1 = XML::Node.new 'node'
     el1['id'] = self.id.to_s
@@ -133,7 +174,7 @@ class Node < ActiveRecord::Base
 
     el1['user'] = user_display_name_cache[self.user_id] unless user_display_name_cache[self.user_id].nil?
 
-    Tags.split(self.tags) do |k,v|
+    self.tags.each do |k,v|
       el2 = XML::Node.new('tag')
       el2['k'] = k.to_s
       el2['v'] = v.to_s
@@ -142,15 +183,33 @@ class Node < ActiveRecord::Base
 
     el1['visible'] = self.visible.to_s
     el1['timestamp'] = self.timestamp.xmlschema
+    el1['version'] = self.version.to_s
     return el1
   end
 
-  # Return the node's tags as a Hash of keys and their values
   def tags_as_hash
-    hash = {}
-    Tags.split(self.tags) do |k,v|
-      hash[k] = v
+    return tags
+  end
+
+  def tags
+    unless @tags
+      @tags = {}
+      self.node_tags.each do |tag|
+        @tags[tag.k] = tag.v
+      end
     end
-    hash
+    @tags
+  end
+
+  def tags=(t)
+    @tags = t 
+  end 
+
+  def add_tag_key_val(k,v)
+    @tags = Hash.new unless @tags
+    @tags[k] = v
   end
+
+
+
 end
diff --git a/app/models/old_node.rb b/app/models/old_node.rb
index 76eab8427..018284528 100644
--- a/app/models/old_node.rb
+++ b/app/models/old_node.rb
@@ -1,6 +1,5 @@
-class OldNode < ActiveRecord::Base
-  include GeoRecord
-
+class OldNode < ActiveRecord::Base 
+ 	include GeoRecord
   set_table_name 'nodes'
   
   validates_presence_of :user_id, :timestamp
@@ -29,8 +28,15 @@ class OldNode < ActiveRecord::Base
     old_node.timestamp = node.timestamp
     old_node.user_id = node.user_id
     old_node.id = node.id
+    old_node.version = node.version
     return old_node
   end
+  
+  def to_xml
+    doc = OSM::API.new.get_xml_doc
+    doc.root << to_xml_node()
+    return doc
+  end
 
   def to_xml_node
     el1 = XML::Node.new 'node'
@@ -39,7 +45,7 @@ class OldNode < ActiveRecord::Base
     el1['lon'] = self.lon.to_s
     el1['user'] = self.user.display_name if self.user.data_public?
 
-    Tags.split(self.tags) do |k,v|
+    self.tags.each do |k,v|
       el2 = XML::Node.new('tag')
       el2['k'] = k.to_s
       el2['v'] = v.to_s
@@ -48,24 +54,57 @@ class OldNode < ActiveRecord::Base
 
     el1['visible'] = self.visible.to_s
     el1['timestamp'] = self.timestamp.xmlschema
+    el1['version'] = self.version.to_s
     return el1
   end
-  
-  def tags_as_hash
-    hash = {}
-    Tags.split(self.tags) do |k,v|
-      hash[k] = v
+
+  def save_with_dependencies!
+    save!
+    #not sure whats going on here
+    clear_aggregation_cache
+    clear_association_cache
+    #ok from here
+    @attributes.update(OldNode.find(:first, :conditions => ['id = ? AND timestamp = ?', self.id, self.timestamp]).instance_variable_get('@attributes'))
+   
+    self.tags.each do |k,v|
+      tag = OldNodeTag.new
+      tag.k = k
+      tag.v = v
+      tag.id = self.id
+      tag.version = self.version
+      tag.save!
     end
-    hash
   end
 
-  # Pretend we're not in any ways
-  def ways
-    return []
+  def tags
+    unless @tags
+        @tags = Hash.new
+        OldNodeTag.find(:all, :conditions => ["id = ? AND version = ?", self.id, self.version]).each do |tag|
+            @tags[tag.k] = tag.v
+        end
+    end
+    @tags = Hash.new unless @tags
+    @tags
   end
 
-  # Pretend we're not in any relations
-  def containing_relation_members
-    return []
-  end
+  def tags=(t)
+    @tags = t 
+  end 
+  def tags_as_hash 
+    hash = {} 
+    Tags.split(self.tags) do |k,v| 
+      hash[k] = v 
+    end 
+    hash 
+  end 
+ 
+  # Pretend we're not in any ways 
+  def ways 
+    return [] 
+  end 
+ 
+  # Pretend we're not in any relations 
+  def containing_relation_members 
+    return [] 
+  end 
 end
diff --git a/lib/geo_record.rb b/lib/geo_record.rb
index 025bbe4a8..286ce69e1 100644
--- a/lib/geo_record.rb
+++ b/lib/geo_record.rb
@@ -1,4 +1,9 @@
 module GeoRecord
+  # This scaling factor is used to convert between the float lat/lon that is 
+  # returned by the API, and the integer lat/lon equivalent that is stored in
+  # the database.
+  SCALE = 10000000
+  
   def self.included(base)
     base.extend(ClassMethods)
   end
@@ -20,21 +25,21 @@ module GeoRecord
   end
 
   def lat=(l)
-    self.latitude = (l * 10000000).round
+    self.latitude = (l * SCALE).round
   end
 
   def lon=(l)
-    self.longitude = (l * 10000000).round
+    self.longitude = (l * SCALE).round
   end
 
   # Return WGS84 latitude
   def lat
-    return self.latitude.to_f / 10000000
+    return self.latitude.to_f / SCALE
   end
 
   # Return WGS84 longitude
   def lon
-    return self.longitude.to_f / 10000000
+    return self.longitude.to_f / SCALE
   end
 
   # Potlatch projections
diff --git a/test/fixtures/current_nodes.yml b/test/fixtures/current_nodes.yml
index 8fd3b781f..7af9bbda1 100644
--- a/test/fixtures/current_nodes.yml
+++ b/test/fixtures/current_nodes.yml
@@ -10,32 +10,32 @@ visible_node:
 
 invisible_node:
   id: 2
-  latitude: 2
-  longitude: 2
+  latitude: <%= 2*SCALE %>
+  longitude: <%= 2*SCALE %>
   user_id: 1
   visible: 0
   timestamp: 2007-01-01 00:00:00
 
 used_node_1:
   id: 3
-  latitude: 3
-  longitude: 3
+  latitude: <%= 3*SCALE %>
+  longitude: <%= 3*SCALE %>
   user_id: 1
   visible: 1
   timestamp: 2007-01-01 00:00:00
 
 used_node_2:
   id: 4
-  latitude: 4
-  longitude: 4
+  latitude: <%= 4*SCALE %>
+  longitude: <%= 4*SCALE %>
   user_id: 1
   visible: 1
   timestamp: 2007-01-01 00:00:00
 
 node_used_by_relationship:
   id: 5
-  latitude: 5
-  longitude: 5
+  latitude: <%= 5*SCALE %>
+  longitude: <%= 5*SCALE %>
   user_id: 1
   visible: 1
   timestamp: 2007-01-01 00:00:00
@@ -49,29 +49,29 @@ node_too_far_north:
   
 node_too_far_south:
   id: 7
-  latitude: -90
-  longitude: 7
+  latitude: <%= -91*SCALE %>
+  longitude: <%= 7*SCALE %>
   user_id: 1
   timestamp: 2007-01-01 00:00:00
   
 node_too_far_west:
   id: 8
-  latitude: 8
-  longitude: -181
+  latitude: <%= 8*SCALE %>
+  longitude: <%= -181*SCALE %>
   user_id: 1
   timestamp: 2007-01-01 00:00:00
   
 node_too_far_east:
   id: 9
-  latitude: 9
-  longitude: 180
+  latitude: <%= 9*SCALE %>
+  longitude: <%= 181*SCALE %>
   user_id: 1
   timestamp: 2007-01-01 00:00:00
   
 node_totally_wrong:
   id: 10
-  latitude: 1000
-  longitude: 1000
+  latitude: <%= 1000*SCALE %>
+  longitude: <%= 1000*SCALE %>
   user_id: 1
   timestamp: 2007-01-01 00:00:00
   
diff --git a/test/fixtures/nodes.yml b/test/fixtures/nodes.yml
index b10ce2fe7..202a7d931 100644
--- a/test/fixtures/nodes.yml
+++ b/test/fixtures/nodes.yml
@@ -1,40 +1,40 @@
 # Read about fixtures at http://ar.rubyonrails.org/classes/Fixtures.html
 visible_node:
   id: 1
-  latitude: 1
-  longitude: 1
+  latitude: <%= 1*SCALE %>
+  longitude: <%= 1*SCALE %>
   user_id: 1
   visible: 1
   timestamp: 2007-01-01 00:00:00
 
 invisible_node:
   id: 2
-  latitude: 2
-  longitude: 2
+  latitude: <%= 2*SCALE %>
+  longitude: <%= 2*SCALE %>
   user_id: 1
   visible: 0
   timestamp: 2007-01-01 00:00:00
 
 used_node_1:
   id: 3
-  latitude: 3
-  longitude: 3
+  latitude: <%= 3*SCALE %>
+  longitude: <%= 3*SCALE %>
   user_id: 1
   visible: 1
   timestamp: 2007-01-01 00:00:00
 
 used_node_2:
   id: 4
-  latitude: 4
-  longitude: 4
+  latitude: <%= 4*SCALE %>
+  longitude: <%= 4*SCALE %>
   user_id: 1
   visible: 1
   timestamp: 2007-01-01 00:00:00
 
 node_used_by_relationship:
   id: 5
-  latitude: 5
-  longitude: 5
+  latitude: <%= 5*SCALE %>
+  longitude: <%= 5*SCALE %>
   user_id: 1
   visible: 1
   timestamp: 2007-01-01 00:00:00
diff --git a/test/unit/node_test.rb b/test/unit/node_test.rb
index d56fed50a..460c13a02 100644
--- a/test/unit/node_test.rb
+++ b/test/unit/node_test.rb
@@ -8,29 +8,38 @@ class NodeTest < Test::Unit::TestCase
   set_fixture_class :currenr_node_tags => :NodeTag
     
   def test_node_too_far_north
-	  noden = current_nodes(:node_too_far_north)
-    assert_equal noden.lat, current_nodes(:node_too_far_north).latitude/SCALE
-    assert_equal false, noden.valid?
+	  node = current_nodes(:node_too_far_north)
+    assert_equal node.lat, current_nodes(:node_too_far_north).latitude/SCALE
+    assert_equal node.lon, current_nodes(:node_too_far_north).longitude/SCALE
+    assert_equal false, node.valid?
   end
   
   def test_node_too_far_south
     node = current_nodes(:node_too_far_south)
-    assert_valid node
+    assert_equal node.lat, current_nodes(:node_too_far_south).latitude/SCALE
+    assert_equal node.lon, current_nodes(:node_too_far_south).longitude/SCALE
+    assert_equal false, node.valid?
   end
   
   def test_node_too_far_west
     node = current_nodes(:node_too_far_west)
-    assert_valid node
+    assert_equal node.lat, current_nodes(:node_too_far_west).latitude/SCALE
+    assert_equal node.lon, current_nodes(:node_too_far_west).longitude/SCALE
+    assert_equal false, node.valid?
   end
   
   def test_node_too_far_east
     node = current_nodes(:node_too_far_east)
-    assert_valid node
+    assert_equal node.lat, current_nodes(:node_too_far_east).latitude/SCALE
+    assert_equal node.lon, current_nodes(:node_too_far_east).longitude/SCALE
+    assert_equal false, node.valid?
   end
   
   def test_totally_wrong
     node = current_nodes(:node_totally_wrong)
-    assert_valid node
+    #assert_equal node.lat, current_nodes(:node_totally_wrong).latitude/SCALE
+    #assert_equal node.lon, current_nodes(:node_totally_wrong).longitude/SCALE
+    assert_equal false, node.valid?
   end
   
   def test_create