From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 19 Feb 2024 01:34:41 +0000 (+0300)
Subject: Link to current and old element versions from changeset pages
X-Git-Tag: live~714^2
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/7c522a4e024a8763125251f6df6bbcbcc0f0f1a1?hp=ba2c75641d3de82fc4cda78dc864dc690b860b6d

Link to current and old element versions from changeset pages
---

diff --git a/app/helpers/browse_helper.rb b/app/helpers/browse_helper.rb
index 3c9d4d092..7aa6e4754 100644
--- a/app/helpers/browse_helper.rb
+++ b/app/helpers/browse_helper.rb
@@ -2,7 +2,7 @@ module BrowseHelper
   def element_single_current_link(type, object, url)
     link_to url, { :class => element_class(type, object), :title => element_title(object), :rel => (link_follow(object) if type == "node") } do
       element_strikethrough object do
-        printable_name object
+        printable_element_name object
       end
     end
   end
@@ -13,14 +13,13 @@ module BrowseHelper
     end
   end
 
-  def printable_name(object, version: false)
+  def printable_element_name(object)
     id = if object.id.is_a?(Array)
            object.id[0]
          else
            object.id
          end
-    name = t "printable_name.with_id", :id => id.to_s
-    name = t "printable_name.with_version", :id => name, :version => object.version.to_s if version
+    name = id.to_s
 
     # don't look at object tags if redacted, so as to avoid giving
     # away redacted version tag information.
@@ -41,6 +40,10 @@ module BrowseHelper
     name
   end
 
+  def printable_element_version(object)
+    t "printable_name.version", :version => object.version
+  end
+
   def element_strikethrough(object, &block)
     if object.redacted? || !object.visible?
       tag.s(&block)
diff --git a/app/views/browse/_containing_relation.html.erb b/app/views/browse/_containing_relation.html.erb
index 06220a97d..2547ff7aa 100644
--- a/app/views/browse/_containing_relation.html.erb
+++ b/app/views/browse/_containing_relation.html.erb
@@ -1,4 +1,4 @@
-<li><%= linked_name = link_to printable_name(containing_relation.relation), :action => "relation", :id => containing_relation.relation.id.to_s
+<li><%= linked_name = link_to printable_element_name(containing_relation.relation), :action => "relation", :id => containing_relation.relation.id.to_s
         if containing_relation.member_role.blank?
           t ".entry_html", :relation_name => linked_name
         else
diff --git a/app/views/browse/_relation_member.html.erb b/app/views/browse/_relation_member.html.erb
index 56b8f154f..16842f569 100644
--- a/app/views/browse/_relation_member.html.erb
+++ b/app/views/browse/_relation_member.html.erb
@@ -1,4 +1,4 @@
-<% linked_name = link_to printable_name(relation_member.member), { :controller => :browse, :action => relation_member.member_type.downcase, :id => relation_member.member_id.to_s }, { :rel => link_follow(relation_member.member) }
+<% linked_name = link_to printable_element_name(relation_member.member), { :controller => :browse, :action => relation_member.member_type.downcase, :id => relation_member.member_id.to_s }, { :rel => link_follow(relation_member.member) }
    type_str = t ".type.#{relation_member.member_type.downcase}" %>
 <%= element_list_item relation_member.member_type.downcase, relation_member.member do %>
   <%= if relation_member.member_role.blank?
diff --git a/app/views/browse/changeset.html.erb b/app/views/browse/changeset.html.erb
index d2ccff44d..501712717 100644
--- a/app/views/browse/changeset.html.erb
+++ b/app/views/browse/changeset.html.erb
@@ -94,7 +94,9 @@
     <ul class="list-unstyled">
       <% @ways.each do |way| %>
         <%= element_list_item "way", way do %>
-          <%= link_to printable_name(way, :version => true), { :action => "way", :id => way.way_id.to_s } %>
+          <%= t "printable_name.current_and_old_links_html",
+                :current_link => link_to(printable_element_name(way), way_path(way.way_id)),
+                :old_link => link_to(printable_element_version(way), old_way_path(way.way_id, way.version)) %>
         <% end %>
       <% end %>
     </ul>
@@ -105,7 +107,9 @@
     <ul class="list-unstyled">
       <% @relations.each do |relation| %>
         <%= element_list_item "relation", relation do %>
-          <%= link_to printable_name(relation, :version => true), { :action => "relation", :id => relation.relation_id.to_s } %>
+          <%= t "printable_name.current_and_old_links_html",
+                :current_link => link_to(printable_element_name(relation), relation_path(relation.relation_id)),
+                :old_link => link_to(printable_element_version(relation), old_relation_path(relation.relation_id, relation.version)) %>
         <% end %>
       <% end %>
     </ul>
@@ -116,7 +120,9 @@
     <ul class="list-unstyled">
       <% @nodes.each do |node| %>
         <%= element_list_item "node", node do %>
-          <%= link_to printable_name(node, :version => true), { :action => "node", :id => node.node_id.to_s }, { :rel => link_follow(node) } %>
+          <%= t "printable_name.current_and_old_links_html",
+                :current_link => link_to(printable_element_name(node), node_path(node.node_id), { :rel => link_follow(node) }),
+                :old_link => link_to(printable_element_version(node), old_node_path(node.node_id, node.version), { :rel => link_follow(node) }) %>
         <% end %>
       <% end %>
     </ul>
diff --git a/app/views/browse/feature.html.erb b/app/views/browse/feature.html.erb
index 37be312b1..cd333d3bf 100644
--- a/app/views/browse/feature.html.erb
+++ b/app/views/browse/feature.html.erb
@@ -1,6 +1,6 @@
-<% set_title(t("browse.#{@type}.title_html", :name => printable_name(@feature))) %>
+<% set_title(t("browse.#{@type}.title_html", :name => printable_element_name(@feature))) %>
 
-<%= render "sidebar_header", :title => t("browse.#{@type}.title_html", :name => printable_name(@feature)) %>
+<%= render "sidebar_header", :title => t("browse.#{@type}.title_html", :name => printable_element_name(@feature)) %>
 
 <%= render :partial => @type, :object => @feature %>
 
diff --git a/app/views/browse/history.html.erb b/app/views/browse/history.html.erb
index fd1f5ffc5..bf11e8f1e 100644
--- a/app/views/browse/history.html.erb
+++ b/app/views/browse/history.html.erb
@@ -1,6 +1,6 @@
-<% set_title(t("browse.#{@type}.history_title_html", :name => printable_name(@feature))) %>
+<% set_title(t("browse.#{@type}.history_title_html", :name => printable_element_name(@feature))) %>
 
-<%= render "sidebar_header", :title => t("browse.#{@type}.history_title_html", :name => printable_name(@feature)) %>
+<%= render "sidebar_header", :title => t("browse.#{@type}.history_title_html", :name => printable_element_name(@feature)) %>
 
 <%= render :partial => @type, :collection => @feature.send(:"old_#{@type}s").reverse %>
 
diff --git a/app/views/old_nodes/show.html.erb b/app/views/old_nodes/show.html.erb
index 9220b2cc2..90be6b7c9 100644
--- a/app/views/old_nodes/show.html.erb
+++ b/app/views/old_nodes/show.html.erb
@@ -1,6 +1,6 @@
-<% set_title t("browse.node.title_html", :name => printable_name(@feature)) %>
+<% set_title t("browse.node.title_html", :name => printable_element_name(@feature)) %>
 
-<%= render "sidebar_header", :title => t("browse.node.title_html", :name => printable_name(@feature)) %>
+<%= render "sidebar_header", :title => t("browse.node.title_html", :name => printable_element_name(@feature)) %>
 
 <%= render :partial => "browse/node", :object => @feature %>
 
diff --git a/app/views/old_relations/show.html.erb b/app/views/old_relations/show.html.erb
index b049a4cf0..42e80e651 100644
--- a/app/views/old_relations/show.html.erb
+++ b/app/views/old_relations/show.html.erb
@@ -1,6 +1,6 @@
-<% set_title t("browse.relation.title_html", :name => printable_name(@feature)) %>
+<% set_title t("browse.relation.title_html", :name => printable_element_name(@feature)) %>
 
-<%= render "sidebar_header", :title => t("browse.relation.title_html", :name => printable_name(@feature)) %>
+<%= render "sidebar_header", :title => t("browse.relation.title_html", :name => printable_element_name(@feature)) %>
 
 <%= render :partial => "browse/relation", :object => @feature %>
 
diff --git a/app/views/old_ways/show.html.erb b/app/views/old_ways/show.html.erb
index d128cd2ad..b16c6b291 100644
--- a/app/views/old_ways/show.html.erb
+++ b/app/views/old_ways/show.html.erb
@@ -1,6 +1,6 @@
-<% set_title t("browse.way.title_html", :name => printable_name(@feature)) %>
+<% set_title t("browse.way.title_html", :name => printable_element_name(@feature)) %>
 
-<%= render "sidebar_header", :title => t("browse.way.title_html", :name => printable_name(@feature)) %>
+<%= render "sidebar_header", :title => t("browse.way.title_html", :name => printable_element_name(@feature)) %>
 
 <%= render :partial => "browse/way", :object => @feature %>
 
diff --git a/config/locales/en.yml b/config/locales/en.yml
index d66482639..633e8b1ed 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -204,9 +204,9 @@ en:
         one: "%{count} year ago"
         other: "%{count} years ago"
   printable_name:
-    with_id: "%{id}"
-    with_version: "%{id}, v%{version}"
+    version: "v%{version}"
     with_name_html: "%{name} (%{id})"
+    current_and_old_links_html: "%{current_link}, %{old_link}"
   editor:
     default: "Default (currently %{name})"
     id:
diff --git a/test/controllers/browse_controller_test.rb b/test/controllers/browse_controller_test.rb
index 7df246c57..1023d76ae 100644
--- a/test/controllers/browse_controller_test.rb
+++ b/test/controllers/browse_controller_test.rb
@@ -156,6 +156,15 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
     assert_select "div.changeset-comments ul li", :count => 4
   end
 
+  def test_read_changeset_element_links
+    changeset = create(:changeset)
+    node = create(:node, :with_history, :changeset => changeset)
+
+    browse_check :changeset_path, changeset.id, "browse/changeset"
+    assert_dom "a[href='#{node_path node}']", :count => 1
+    assert_dom "a[href='#{old_node_path node, 1}']", :count => 1
+  end
+
   ##
   #  Methods to check redaction.
   #
diff --git a/test/helpers/browse_helper_test.rb b/test/helpers/browse_helper_test.rb
index 06d5f3d53..4cf0ab69a 100644
--- a/test/helpers/browse_helper_test.rb
+++ b/test/helpers/browse_helper_test.rb
@@ -4,7 +4,7 @@ class BrowseHelperTest < ActionView::TestCase
   include ERB::Util
   include ApplicationHelper
 
-  def test_printable_name
+  def test_printable_element_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)
@@ -19,42 +19,34 @@ class BrowseHelperTest < ActionView::TestCase
 
     deleted_node = create(:node, :deleted)
 
-    assert_dom_equal deleted_node.id.to_s, printable_name(deleted_node)
-    assert_dom_equal "<bdi>Test Node</bdi> (<bdi>#{node.id}</bdi>)", printable_name(node)
-    assert_dom_equal "<bdi>Test Node</bdi> (<bdi>#{node.id}</bdi>)", printable_name(node_v2)
-    assert_dom_equal node.id.to_s, printable_name(node_v1)
-    assert_dom_equal "<bdi>Test Node</bdi> (<bdi>#{node.id}, v2</bdi>)", printable_name(node_v2, :version => true)
-    assert_dom_equal "#{node.id}, v1", printable_name(node_v1, :version => true)
-    assert_dom_equal "<bdi>3.1415926</bdi> (<bdi>#{node_with_ref_without_name.id}</bdi>)", printable_name(node_with_ref_without_name)
+    assert_dom_equal deleted_node.id.to_s, printable_element_name(deleted_node)
+    assert_dom_equal "<bdi>Test Node</bdi> (<bdi>#{node.id}</bdi>)", printable_element_name(node)
+    assert_dom_equal "<bdi>Test Node</bdi> (<bdi>#{node.id}</bdi>)", printable_element_name(node_v2)
+    assert_dom_equal node.id.to_s, printable_element_name(node_v1)
+    assert_dom_equal "<bdi>3.1415926</bdi> (<bdi>#{node_with_ref_without_name.id}</bdi>)", printable_element_name(node_with_ref_without_name)
 
     I18n.with_locale "pt" do
-      assert_dom_equal deleted_node.id.to_s, printable_name(deleted_node)
-      assert_dom_equal "<bdi>Nó teste</bdi> (<bdi>#{node.id}</bdi>)", printable_name(node)
-      assert_dom_equal "<bdi>Nó teste</bdi> (<bdi>#{node.id}</bdi>)", printable_name(node_v2)
-      assert_dom_equal node.id.to_s, printable_name(node_v1)
-      assert_dom_equal "<bdi>Nó teste</bdi> (<bdi>#{node.id}, v2</bdi>)", printable_name(node_v2, :version => true)
-      assert_dom_equal "#{node.id}, v1", printable_name(node_v1, :version => true)
-      assert_dom_equal "<bdi>3.1415926</bdi> (<bdi>#{node_with_ref_without_name.id}</bdi>)", printable_name(node_with_ref_without_name)
+      assert_dom_equal deleted_node.id.to_s, printable_element_name(deleted_node)
+      assert_dom_equal "<bdi>Nó teste</bdi> (<bdi>#{node.id}</bdi>)", printable_element_name(node)
+      assert_dom_equal "<bdi>Nó teste</bdi> (<bdi>#{node.id}</bdi>)", printable_element_name(node_v2)
+      assert_dom_equal node.id.to_s, printable_element_name(node_v1)
+      assert_dom_equal "<bdi>3.1415926</bdi> (<bdi>#{node_with_ref_without_name.id}</bdi>)", printable_element_name(node_with_ref_without_name)
     end
 
     I18n.with_locale "pt-BR" do
-      assert_dom_equal deleted_node.id.to_s, printable_name(deleted_node)
-      assert_dom_equal "<bdi>Nó teste</bdi> (<bdi>#{node.id}</bdi>)", printable_name(node)
-      assert_dom_equal "<bdi>Nó teste</bdi> (<bdi>#{node.id}</bdi>)", printable_name(node_v2)
-      assert_dom_equal node.id.to_s, printable_name(node_v1)
-      assert_dom_equal "<bdi>Nó teste</bdi> (<bdi>#{node.id}, v2</bdi>)", printable_name(node_v2, :version => true)
-      assert_dom_equal "#{node.id}, v1", printable_name(node_v1, :version => true)
-      assert_dom_equal "<bdi>3.1415926</bdi> (<bdi>#{node_with_ref_without_name.id}</bdi>)", printable_name(node_with_ref_without_name)
+      assert_dom_equal deleted_node.id.to_s, printable_element_name(deleted_node)
+      assert_dom_equal "<bdi>Nó teste</bdi> (<bdi>#{node.id}</bdi>)", printable_element_name(node)
+      assert_dom_equal "<bdi>Nó teste</bdi> (<bdi>#{node.id}</bdi>)", printable_element_name(node_v2)
+      assert_dom_equal node.id.to_s, printable_element_name(node_v1)
+      assert_dom_equal "<bdi>3.1415926</bdi> (<bdi>#{node_with_ref_without_name.id}</bdi>)", printable_element_name(node_with_ref_without_name)
     end
 
     I18n.with_locale "de" do
-      assert_dom_equal deleted_node.id.to_s, printable_name(deleted_node)
-      assert_dom_equal "<bdi>Test Node</bdi> (<bdi>#{node.id}</bdi>)", printable_name(node)
-      assert_dom_equal "<bdi>Test Node</bdi> (<bdi>#{node.id}</bdi>)", printable_name(node_v2)
-      assert_dom_equal node.id.to_s, printable_name(node_v1)
-      assert_dom_equal "<bdi>Test Node</bdi> (<bdi>#{node.id}, v2</bdi>)", printable_name(node_v2, :version => true)
-      assert_dom_equal "#{node.id}, v1", printable_name(node_v1, :version => true)
-      assert_dom_equal "<bdi>3.1415926</bdi> (<bdi>#{node_with_ref_without_name.id}</bdi>)", printable_name(node_with_ref_without_name)
+      assert_dom_equal deleted_node.id.to_s, printable_element_name(deleted_node)
+      assert_dom_equal "<bdi>Test Node</bdi> (<bdi>#{node.id}</bdi>)", printable_element_name(node)
+      assert_dom_equal "<bdi>Test Node</bdi> (<bdi>#{node.id}</bdi>)", printable_element_name(node_v2)
+      assert_dom_equal node.id.to_s, printable_element_name(node_v1)
+      assert_dom_equal "<bdi>3.1415926</bdi> (<bdi>#{node_with_ref_without_name.id}</bdi>)", printable_element_name(node_with_ref_without_name)
     end
   end