From: Tom Hughes Date: Thu, 30 Mar 2017 14:45:37 +0000 (+0100) Subject: Merge remote-tracking branch 'openstreetmap/pull/1508' X-Git-Tag: live~3441 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/3d6a3a6de33aa0c1bc436bce9ea9da33e00460ff?hp=4afedb07a26f8e9da05697b4e26ff52ef1fe55fb Merge remote-tracking branch 'openstreetmap/pull/1508' --- diff --git a/app/models/node.rb b/app/models/node.rb index f8559dca7..6346907f1 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -185,8 +185,8 @@ class Node < ActiveRecord::Base add_metadata_to_xml_node(el, self, changeset_cache, user_display_name_cache) if visible? - el["lat"] = lat.to_s - el["lon"] = lon.to_s + el["lat"] = format("%.7f", lat) + el["lon"] = format("%.7f", lon) end add_tags_to_xml_node(el, node_tags) diff --git a/test/controllers/api_controller_test.rb b/test/controllers/api_controller_test.rb index 5aac44a4f..7d249acf3 100644 --- a/test/controllers/api_controller_test.rb +++ b/test/controllers/api_controller_test.rb @@ -69,7 +69,7 @@ class ApiControllerTest < ActionController::TestCase assert_response :success, "Expected scucess with the map call" assert_select "osm[version='#{API_VERSION}'][generator='#{GENERATOR}']", :count => 1 do assert_select "bounds[minlon='#{minlon}'][minlat='#{minlat}'][maxlon='#{maxlon}'][maxlat='#{maxlat}']", :count => 1 - assert_select "node[id='#{node.id}'][lat='#{node.lat}'][lon='#{node.lon}'][version='#{node.version}'][changeset='#{node.changeset_id}'][visible='#{node.visible}'][timestamp='#{node.timestamp.xmlschema}']", :count => 1 do + assert_select "node[id='#{node.id}'][lat='#{format('%.7f', node.lat)}'][lon='#{format('%.7f', node.lon)}'][version='#{node.version}'][changeset='#{node.changeset_id}'][visible='#{node.visible}'][timestamp='#{node.timestamp.xmlschema}']", :count => 1 do # This should really be more generic assert_select "tag[k='#{tag.k}'][v='#{tag.v}']" end @@ -91,7 +91,7 @@ class ApiControllerTest < ActionController::TestCase assert_response :success, "The map call should have succeeded" assert_select "osm[version='#{API_VERSION}'][generator='#{GENERATOR}']", :count => 1 do assert_select "bounds[minlon='#{node.lon}'][minlat='#{node.lat}'][maxlon='#{node.lon}'][maxlat='#{node.lat}']", :count => 1 - assert_select "node[id='#{node.id}'][lat='#{node.lat}'][lon='#{node.lon}'][version='#{node.version}'][changeset='#{node.changeset_id}'][visible='#{node.visible}'][timestamp='#{node.timestamp.xmlschema}']", :count => 1 do + assert_select "node[id='#{node.id}'][lat='#{format('%.7f', node.lat)}'][lon='#{format('%.7f', node.lon)}'][version='#{node.version}'][changeset='#{node.changeset_id}'][visible='#{node.visible}'][timestamp='#{node.timestamp.xmlschema}']", :count => 1 do # This should really be more generic assert_select "tag[k='#{tag.k}'][v='#{tag.v}']" end diff --git a/test/controllers/browse_controller_test.rb b/test/controllers/browse_controller_test.rb index 567fc0576..12262b224 100644 --- a/test/controllers/browse_controller_test.rb +++ b/test/controllers/browse_controller_test.rb @@ -62,11 +62,11 @@ class BrowseControllerTest < ActionController::TestCase end def test_read_node - browse_check "node", nodes(:visible_node).node_id, "browse/feature" + browse_check "node", create(:node).id, "browse/feature" end def test_read_node_history - browse_check "node_history", nodes(:visible_node).node_id, "browse/history" + browse_check "node_history", create(:node, :with_history).id, "browse/history" end def test_read_changeset @@ -135,7 +135,11 @@ class BrowseControllerTest < ActionController::TestCase # then please make it more easily (and robustly) testable! ## def test_redacted_node - get :node, :id => current_nodes(:redacted_node).id + node = create(:node, :with_history, :deleted, :version => 2) + node_v1 = node.old_nodes.find_by(:version => 1) + node_v1.redact!(create(:redaction)) + + get :node, :id => node.id assert_response :success assert_template "feature" @@ -147,7 +151,11 @@ class BrowseControllerTest < ActionController::TestCase end def test_redacted_node_history - get :node_history, :id => nodes(:redacted_node_redacted_version).node_id + node = create(:node, :with_history, :deleted, :version => 2) + node_v1 = node.old_nodes.find_by(:version => 1) + node_v1.redact!(create(:redaction)) + + get :node_history, :id => node.id assert_response :success assert_template "browse/history" diff --git a/test/controllers/way_controller_test.rb b/test/controllers/way_controller_test.rb index 650977078..ae52c036f 100644 --- a/test/controllers/way_controller_test.rb +++ b/test/controllers/way_controller_test.rb @@ -74,7 +74,7 @@ class WayControllerTest < ActionController::TestCase way.nodes.each do |n| count = (way.nodes - (way.nodes - [n])).length assert_select "osm way nd[ref='#{n.id}']", count - assert_select "osm node[id='#{n.id}'][version='#{n.version}'][lat='#{n.lat}'][lon='#{n.lon}']", 1 + assert_select "osm node[id='#{n.id}'][version='#{n.version}'][lat='#{format('%.7f', n.lat)}'][lon='#{format('%.7f', n.lon)}']", 1 end end end diff --git a/test/factories/node.rb b/test/factories/node.rb index 85eded405..cbebbb009 100644 --- a/test/factories/node.rb +++ b/test/factories/node.rb @@ -8,5 +8,24 @@ FactoryGirl.define do visible true timestamp Time.now version 1 + + trait :deleted do + visible false + end + + trait :with_history do + after(:create) do |node, _evaluator| + (1..node.version).each do |n| + create(:old_node, :node_id => node.id, :version => n) + end + + # For deleted nodes, make sure the most recent old_node is also deleted. + if node.visible == false + latest = node.old_nodes.find_by(:version => node.version) + latest.visible = false + latest.save + end + end + end end end diff --git a/test/helpers/browse_helper_test.rb b/test/helpers/browse_helper_test.rb index 32cf96c6b..62aa73f94 100644 --- a/test/helpers/browse_helper_test.rb +++ b/test/helpers/browse_helper_test.rb @@ -17,77 +17,93 @@ class BrowseHelperTest < ActionView::TestCase end def test_printable_name - add_tags_selection(current_nodes(:node_with_name)) - create(:node_tag, :node => current_nodes(:node_with_ref_without_name), :k => "ref", :v => "3.1415926") - add_old_tags_selection(nodes(:node_with_name_current_version)) - add_old_tags_selection(nodes(:node_with_name_redacted_version)) - - # current_nodes(:redacted_node) is deleted, so has no tags. - assert_dom_equal "17", printable_name(current_nodes(:redacted_node)) - assert_dom_equal "Test Node (18)", printable_name(current_nodes(:node_with_name)) - assert_dom_equal "Test Node (18)", printable_name(nodes(:node_with_name_current_version)) - assert_dom_equal "18", printable_name(nodes(:node_with_name_redacted_version)) - assert_dom_equal "Test Node (18, v2)", printable_name(nodes(:node_with_name_current_version), true) - assert_dom_equal "18, v1", printable_name(nodes(:node_with_name_redacted_version), true) - assert_dom_equal "3.1415926 (19)", printable_name(current_nodes(:node_with_ref_without_name)) + node = create(:node, :with_history, :version => 2) + node_v1 = node.old_nodes.find_by(:version => 1) + node_v2 = node.old_nodes.find_by(:version => 2) + node_v1.redact!(create(:redaction)) + + add_tags_selection(node) + add_old_tags_selection(node_v2) + add_old_tags_selection(node_v1) + + node_with_ref_without_name = create(:node) + create(:node_tag, :node => node_with_ref_without_name, :k => "ref", :v => "3.1415926") + + deleted_node = create(:node, :deleted) + + assert_dom_equal deleted_node.id.to_s, printable_name(deleted_node) + assert_dom_equal "Test Node (#{node.id})", printable_name(node) + assert_dom_equal "Test Node (#{node.id})", printable_name(node_v2) + assert_dom_equal node.id.to_s, printable_name(node_v1) + assert_dom_equal "Test Node (#{node.id}, v2)", printable_name(node_v2, true) + assert_dom_equal "#{node.id}, v1", printable_name(node_v1, true) + assert_dom_equal "3.1415926 (#{node_with_ref_without_name.id})", printable_name(node_with_ref_without_name) I18n.locale = "pt" - assert_dom_equal "17", printable_name(current_nodes(:redacted_node)) - assert_dom_equal "Nó teste (18)", printable_name(current_nodes(:node_with_name)) - assert_dom_equal "Nó teste (18)", printable_name(nodes(:node_with_name_current_version)) - assert_dom_equal "18", printable_name(nodes(:node_with_name_redacted_version)) - assert_dom_equal "Nó teste (18, v2)", printable_name(nodes(:node_with_name_current_version), true) - assert_dom_equal "18, v1", printable_name(nodes(:node_with_name_redacted_version), true) - assert_dom_equal "3.1415926 (19)", printable_name(current_nodes(:node_with_ref_without_name)) + assert_dom_equal deleted_node.id.to_s, printable_name(deleted_node) + assert_dom_equal "Nó teste (#{node.id})", printable_name(node) + assert_dom_equal "Nó teste (#{node.id})", printable_name(node_v2) + assert_dom_equal node.id.to_s, printable_name(node_v1) + assert_dom_equal "Nó teste (#{node.id}, v2)", printable_name(node_v2, true) + assert_dom_equal "#{node.id}, v1", printable_name(node_v1, true) + assert_dom_equal "3.1415926 (#{node_with_ref_without_name.id})", printable_name(node_with_ref_without_name) I18n.locale = "pt-BR" - assert_dom_equal "17", printable_name(current_nodes(:redacted_node)) - assert_dom_equal "Nó teste (18)", printable_name(current_nodes(:node_with_name)) - assert_dom_equal "Nó teste (18)", printable_name(nodes(:node_with_name_current_version)) - assert_dom_equal "18", printable_name(nodes(:node_with_name_redacted_version)) - assert_dom_equal "Nó teste (18, v2)", printable_name(nodes(:node_with_name_current_version), true) - assert_dom_equal "18, v1", printable_name(nodes(:node_with_name_redacted_version), true) - assert_dom_equal "3.1415926 (19)", printable_name(current_nodes(:node_with_ref_without_name)) + assert_dom_equal deleted_node.id.to_s, printable_name(deleted_node) + assert_dom_equal "Nó teste (#{node.id})", printable_name(node) + assert_dom_equal "Nó teste (#{node.id})", printable_name(node_v2) + assert_dom_equal node.id.to_s, printable_name(node_v1) + assert_dom_equal "Nó teste (#{node.id}, v2)", printable_name(node_v2, true) + assert_dom_equal "#{node.id}, v1", printable_name(node_v1, true) + assert_dom_equal "3.1415926 (#{node_with_ref_without_name.id})", printable_name(node_with_ref_without_name) I18n.locale = "de" - assert_dom_equal "17", printable_name(current_nodes(:redacted_node)) - assert_dom_equal "Test Node (18)", printable_name(current_nodes(:node_with_name)) - assert_dom_equal "Test Node (18)", printable_name(nodes(:node_with_name_current_version)) - assert_dom_equal "18", printable_name(nodes(:node_with_name_redacted_version)) - assert_dom_equal "Test Node (18, v2)", printable_name(nodes(:node_with_name_current_version), true) - assert_dom_equal "18, v1", printable_name(nodes(:node_with_name_redacted_version), true) - assert_dom_equal "3.1415926 (19)", printable_name(current_nodes(:node_with_ref_without_name)) + assert_dom_equal deleted_node.id.to_s, printable_name(deleted_node) + assert_dom_equal "Test Node (#{node.id})", printable_name(node) + assert_dom_equal "Test Node (#{node.id})", printable_name(node_v2) + assert_dom_equal node.id.to_s, printable_name(node_v1) + assert_dom_equal "Test Node (#{node.id}, v2)", printable_name(node_v2, true) + assert_dom_equal "#{node.id}, v1", printable_name(node_v1, true) + assert_dom_equal "3.1415926 (#{node_with_ref_without_name.id})", printable_name(node_with_ref_without_name) end def test_link_class - add_tags_selection(current_nodes(:node_with_name)) + node = create(:node, :with_history, :version => 2) + node_v1 = node.old_nodes.find_by(:version => 1) + node_v2 = node.old_nodes.find_by(:version => 2) + node_v1.redact!(create(:redaction)) - assert_equal "node", link_class("node", current_nodes(:visible_node)) - assert_equal "node deleted", link_class("node", current_nodes(:invisible_node)) - assert_equal "node deleted", link_class("node", current_nodes(:redacted_node)) - assert_equal "node building yes shop gift tourism museum", link_class("node", current_nodes(:node_with_name)) + add_tags_selection(node) + add_old_tags_selection(node_v2) + add_old_tags_selection(node_v1) - add_old_tags_selection(nodes(:node_with_name_current_version)) - add_old_tags_selection(nodes(:node_with_name_redacted_version)) - assert_equal "node building yes shop gift tourism museum", link_class("node", nodes(:node_with_name_current_version)) - assert_equal "node deleted", link_class("node", nodes(:node_with_name_redacted_version)) + assert_equal "node", link_class("node", create(:node)) + assert_equal "node deleted", link_class("node", create(:node, :deleted)) + + assert_equal "node building yes shop gift tourism museum", link_class("node", node) + assert_equal "node building yes shop gift tourism museum", link_class("node", node_v2) + assert_equal "node deleted", link_class("node", node_v1) end def test_link_title - add_tags_selection(current_nodes(:node_with_name)) + node = create(:node, :with_history, :version => 2) + node_v1 = node.old_nodes.find_by(:version => 1) + node_v2 = node.old_nodes.find_by(:version => 2) + node_v1.redact!(create(:redaction)) + + add_tags_selection(node) + add_old_tags_selection(node_v2) + add_old_tags_selection(node_v1) - assert_equal "", link_title(current_nodes(:visible_node)) - assert_equal "", link_title(current_nodes(:invisible_node)) - assert_equal "", link_title(current_nodes(:redacted_node)) - assert_equal "building=yes, shop=gift, and tourism=museum", link_title(current_nodes(:node_with_name)) + assert_equal "", link_title(create(:node)) + assert_equal "", link_title(create(:node, :deleted)) - add_old_tags_selection(nodes(:node_with_name_current_version)) - add_old_tags_selection(nodes(:node_with_name_redacted_version)) - assert_equal "building=yes, shop=gift, and tourism=museum", link_title(nodes(:node_with_name_current_version)) - assert_equal "", link_title(nodes(:node_with_name_redacted_version)) + assert_equal "building=yes, shop=gift, and tourism=museum", link_title(node) + assert_equal "building=yes, shop=gift, and tourism=museum", link_title(node_v2) + assert_equal "", link_title(node_v1) end def test_format_key @@ -122,24 +138,29 @@ class BrowseHelperTest < ActionView::TestCase end def test_icon_tags - add_tags_selection(current_nodes(:node_with_name)) + node = create(:node, :with_history, :version => 2) + node_v1 = node.old_nodes.find_by(:version => 1) + node_v2 = node.old_nodes.find_by(:version => 2) + node_v1.redact!(create(:redaction)) + + add_tags_selection(node) - tags = icon_tags(current_nodes(:node_with_name)) + tags = icon_tags(node) assert_equal 3, tags.count assert tags.include?(%w(building yes)) assert tags.include?(%w(tourism museum)) assert tags.include?(%w(shop gift)) - add_old_tags_selection(nodes(:node_with_name_current_version)) - add_old_tags_selection(nodes(:node_with_name_redacted_version)) + add_old_tags_selection(node_v2) + add_old_tags_selection(node_v1) - tags = icon_tags(nodes(:node_with_name_current_version)) + tags = icon_tags(node_v2) assert_equal 3, tags.count assert tags.include?(%w(building yes)) assert tags.include?(%w(tourism museum)) assert tags.include?(%w(shop gift)) - tags = icon_tags(nodes(:node_with_name_redacted_version)) + tags = icon_tags(node_v1) assert_equal 3, tags.count assert tags.include?(%w(building yes)) assert tags.include?(%w(tourism museum)) diff --git a/test/models/node_test.rb b/test/models/node_test.rb index 4384c6287..bd664cfca 100644 --- a/test/models/node_test.rb +++ b/test/models/node_test.rb @@ -61,6 +61,14 @@ class NodeTest < ActiveSupport::TestCase assert_in_delta 76.543 * OldNode::SCALE, node.longitude, 0.000001 end + # Ensure the lat/lon is formatted as a decimal e.g. not 4.0e-05 + def test_lat_lon_xml_format + node = build(:node, :latitude => 0.00004 * OldNode::SCALE, :longitude => 0.00008 * OldNode::SCALE) + + assert_match /lat="0.0000400"/, node.to_xml.to_s + assert_match /lon="0.0000800"/, node.to_xml.to_s + end + # Check that you can create a node and store it def test_create changeset = create(:changeset) diff --git a/test/models/redaction_test.rb b/test/models/redaction_test.rb index 7ea3e458d..2e629867b 100644 --- a/test/models/redaction_test.rb +++ b/test/models/redaction_test.rb @@ -12,8 +12,8 @@ class RedactionTest < ActiveSupport::TestCase end def test_cannot_redact_current_via_old - node = create(:node) - node_v1 = create(:old_node, :node_id => node.id) + node = create(:node, :with_history) + node_v1 = node.old_nodes.find_by(:version => 1) r = create(:redaction) assert_equal(false, node_v1.redacted?, "Expected node to not be redacted already.") assert_raise(OSM::APICannotRedactError) do @@ -22,9 +22,9 @@ class RedactionTest < ActiveSupport::TestCase end def test_can_redact_old - node = create(:node, :version => 2) - node_v1 = create(:old_node, :node_id => node.id) - node_v2 = create(:old_node, :node_id => node.id, :version => 2) + node = create(:node, :with_history, :version => 2) + node_v1 = node.old_nodes.find_by(:version => 1) + node_v2 = node.old_nodes.find_by(:version => 2) r = create(:redaction) assert_equal(false, node_v1.redacted?, "Expected node to not be redacted already.")