[openstreetmap/openstreetmap-website] Tag factories (#1367)

[Tom Hughes]

tomhughes requested changes on this pull request.

There's some kind of deep issues here about how hard we should try to keep our crazy schema consistent between the current and history tables that will only get worse when we do the object models...

> @@ -90,6 +90,9 @@ def test_version
     xml_node = xml_doc.find("//osm/node").first
     nodeid = current_nodes(:node_with_versions).id
 
+    # Ensure that the current tags are propogated to the history too
+    propogate_tags(current_nodes(:node_with_versions), nodes(:node_with_versions_v4))

You can't spell propagate ;-)

> @@ -90,6 +90,9 @@ def test_version
     xml_node = xml_doc.find("//osm/node").first
     nodeid = current_nodes(:node_with_versions).id
 
+    # Ensure that the current tags are propogated to the history too
+    propogate_tags(current_nodes(:node_with_versions), nodes(:node_with_versions_v4))

Also I don't understand why this is needed - we haven't created any tags on the current version of the node so what is there to propagate?

> @@ -161,6 +164,12 @@ def check_not_found_id_version(id, version)
   # Test that getting the current version is identical to picking
   # that version with the version URI call.
   def test_current_version
+    propogate_tags(current_nodes(:visible_node), nodes(:visible_node))
+    propogate_tags(current_nodes(:used_node_1), nodes(:used_node_1))
+    propogate_tags(current_nodes(:used_node_2), nodes(:used_node_2))
+    propogate_tags(current_nodes(:node_used_by_relationship), nodes(:node_used_by_relationship))
+    propogate_tags(current_nodes(:node_with_versions), nodes(:node_with_versions_v4))
+

Again I am obviously not understanding as I don't see that we have created anything yet that needs to be propagated?

> @@ -337,6 +337,7 @@ def test_create
   def test_update_relation_tags
     basic_authorization "test at example.com", "test"
     rel_id = current_relations(:multi_tag_relation).id
+    create_list(:relation_tag, 4, :relation => current_relations(:multi_tag_relation))

Now arguably here we should propagate as technically by creating tags on the current object that are not on the corresponding history object we have put the database into an invalid state. It probably doesn't actually affect anything though, but then I'm not sure we even need to create these tags? We'll still test whether adding the new one works...

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/1367#pullrequestreview-7892426
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20161109/83066b69/attachment.html>