From: Andy Allan <git@gravitystorm.co.uk>
Date: Wed, 20 Mar 2024 14:42:20 +0000 (+0000)
Subject: Merge pull request #4593 from AntonKhorev/simplify-link_to-on-issue-pages
X-Git-Tag: live~1238
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/c7cc05c078b540641b6c8bac51f218807a83a914?hp=6cf9f5dc9cba1b73d7f275560798d7dcc6a2e3ea

Merge pull request #4593 from AntonKhorev/simplify-link_to-on-issue-pages

Simplify link_to user calls on issue pages
---

diff --git a/app/views/messages/_message_summary.html.erb b/app/views/messages/_message_summary.html.erb
index 4a552e83e..8ac09251c 100644
--- a/app/views/messages/_message_summary.html.erb
+++ b/app/views/messages/_message_summary.html.erb
@@ -1,6 +1,6 @@
 <tr id="inbox-<%= message_summary.id %>" class="inbox-row<%= "-unread" unless message_summary.message_read? %>">
-  <td><%= link_to message_summary.sender.display_name, user_path(message_summary.sender) %></td>
-  <td><%= link_to message_summary.title, message_path(message_summary) %></td>
+  <td><%= link_to message_summary.sender.display_name, message_summary.sender %></td>
+  <td><%= link_to message_summary.title, message_summary %></td>
   <td class="text-nowrap"><%= l message_summary.sent_on, :format => :friendly %></td>
   <td class="text-nowrap d-flex justify-content-end gap-1">
     <%= button_to t(".unread_button"), message_mark_path(message_summary, :mark => "unread"), :remote => true, :class => "btn btn-sm btn-primary", :form => { :class => "inbox-mark-unread", :hidden => !message_summary.message_read? } %>
diff --git a/app/views/messages/_sent_message_summary.html.erb b/app/views/messages/_sent_message_summary.html.erb
index 24caba5ab..d869fd3cb 100644
--- a/app/views/messages/_sent_message_summary.html.erb
+++ b/app/views/messages/_sent_message_summary.html.erb
@@ -1,6 +1,6 @@
 <tr class="inbox-row">
-  <td><%= link_to sent_message_summary.recipient.display_name, user_path(sent_message_summary.recipient) %></td>
-  <td><%= link_to sent_message_summary.title, message_path(sent_message_summary) %></td>
+  <td><%= link_to sent_message_summary.recipient.display_name, sent_message_summary.recipient %></td>
+  <td><%= link_to sent_message_summary.title, sent_message_summary %></td>
   <td class="text-nowrap"><%= l sent_message_summary.sent_on, :format => :friendly %></td>
   <td class="text-nowrap d-flex justify-content-end gap-1">
     <%= button_to t(".destroy_button"), message_path(sent_message_summary, :referer => request.fullpath), :method => :delete, :remote => true, :class => "btn btn-sm btn-danger", :form_class => "inbox-destroy" %>
diff --git a/app/views/messages/new.html.erb b/app/views/messages/new.html.erb
index 7c91722ff..2838a5dab 100644
--- a/app/views/messages/new.html.erb
+++ b/app/views/messages/new.html.erb
@@ -1,5 +1,5 @@
 <% content_for :heading do %>
-  <h1><%= t(".send_message_to_html", :name => link_to(@message.recipient.display_name, user_path(@message.recipient))) %></h1>
+  <h1><%= t(".send_message_to_html", :name => link_to(@message.recipient.display_name, @message.recipient)) %></h1>
 <% end %>
 
 <%= bootstrap_form_for @message do |f| %>
diff --git a/app/views/messages/show.html.erb b/app/views/messages/show.html.erb
index f0742280f..29bf02cd8 100644
--- a/app/views/messages/show.html.erb
+++ b/app/views/messages/show.html.erb
@@ -5,10 +5,10 @@
 <div class='mb-3 border-bottom border-grey py-1 d-flex gap-1 flex-wrap'>
   <% if current_user == @message.recipient %>
     <%= user_thumbnail_tiny @message.sender %>
-    <%= link_to @message.sender.display_name, user_path(@message.sender) %>
+    <%= link_to @message.sender.display_name, @message.sender %>
   <% else %>
     <%= user_thumbnail_tiny @message.recipient %>
-    <%= link_to @message.recipient.display_name, user_path(@message.recipient) %>
+    <%= link_to @message.recipient.display_name, @message.recipient %>
   <% end %>
   <span class="ms-auto">
     <%= l @message.sent_on, :format => :friendly %>
diff --git a/app/views/traces/_description.html.erb b/app/views/traces/_description.html.erb
index f4edc0e21..5fea819da 100644
--- a/app/views/traces/_description.html.erb
+++ b/app/views/traces/_description.html.erb
@@ -1,4 +1,4 @@
-<%= image_tag(url_for(:controller => :traces, :action => :icon, :id => description.id, :display_name => description.user.display_name)) %>
+<%= image_tag trace_icon_path(description.user, description.id) %>
 <% if description.size -%>
 <%= t ".description_with_count", :count => description.size, :user => description.user.display_name %>
 <% else -%>
diff --git a/app/views/traces/_trace.html.erb b/app/views/traces/_trace.html.erb
index 9ee58cf22..f72671e37 100644
--- a/app/views/traces/_trace.html.erb
+++ b/app/views/traces/_trace.html.erb
@@ -2,7 +2,7 @@
   <td>
     <% if Settings.status != "gpx_offline" %>
       <% if trace.inserted %>
-        <a href="<%= url_for :controller => "traces", :action => "show", :id => trace.id, :display_name => trace.user.display_name %>"><img src="<%= url_for :controller => "traces", :action => "icon", :id => trace.id, :display_name => trace.user.display_name %>" border="0" alt="" /></a>
+        <%= link_to image_tag(trace_icon_path(trace.user, trace), :alt => ""), show_trace_path(trace.user, trace) %>
       <% else %>
         <span class="text-danger"><%= t ".pending" %></span>
       <% end %>
@@ -11,7 +11,7 @@
   <td>
     <ul class="list-inline mb-0">
       <li class="list-inline-item">
-        <%= link_to trace.name, :controller => "traces", :action => "show", :display_name => trace.user.display_name, :id => trace.id %>
+        <%= link_to trace.name, show_trace_path(trace.user, trace) %>
       </li>
 
       <% if trace.inserted? %>
@@ -30,7 +30,7 @@
     </ul>
     <p class="text-muted mb-0">
       <%= friendly_date_ago(trace.timestamp) %>
-      <%= t ".by" %> <%= link_to trace.user.display_name, user_path(trace.user) %>
+      <%= t ".by" %> <%= link_to trace.user.display_name, trace.user %>
       <% if !trace.tags.empty? %>
         <%= t ".in" %>
         <%= safe_join(trace.tags.collect { |tag| link_to_tag tag.tag }, ", ") %>
@@ -45,10 +45,10 @@
       <nav class="secondary-actions">
         <ul>
           <li>
-            <%= link_to t(".view_map"), { :controller => "site", :action => "index", :mlat => trace.latitude, :mlon => trace.longitude, :anchor => "map=14/#{trace.latitude}/#{trace.longitude}" } %>
+            <%= link_to t(".view_map"), root_path(:mlat => trace.latitude, :mlon => trace.longitude, :anchor => "map=14/#{trace.latitude}/#{trace.longitude}") %>
           </li>
           <li>
-            <%= link_to t(".edit_map"), { :controller => "site", :action => "edit", :gpx => trace.id } %>
+            <%= link_to t(".edit_map"), edit_path(:gpx => trace.id) %>
           </li>
         </ul>
       </nav>
diff --git a/app/views/traces/edit.html.erb b/app/views/traces/edit.html.erb
index 09fe9c510..809112d4b 100644
--- a/app/views/traces/edit.html.erb
+++ b/app/views/traces/edit.html.erb
@@ -2,7 +2,7 @@
   <h1><%= t ".heading", :name => @trace.name %></h1>
 <% end %>
 
-<img src="<%= url_for :controller => "traces", :action => "picture", :id => @trace.id, :display_name => @trace.user.display_name %>">
+<%= image_tag trace_picture_path(@trace.user, @trace) %>
 
 <%= bootstrap_form_for @trace do |f| %>
   <%= f.text_field :name, :disabled => true %>
diff --git a/app/views/traces/georss.rss.builder b/app/views/traces/georss.rss.builder
index 36641fec0..924ffb935 100644
--- a/app/views/traces/georss.rss.builder
+++ b/app/views/traces/georss.rss.builder
@@ -21,8 +21,8 @@ xml.rss("version" => "2.0",
       xml.item do
         xml.title trace.name
 
-        xml.link url_for(:controller => :traces, :action => :show, :id => trace.id, :display_name => trace.user.display_name, :only_path => false)
-        xml.guid url_for(:controller => :traces, :action => :show, :id => trace.id, :display_name => trace.user.display_name, :only_path => false)
+        xml.link show_trace_url(trace.user, trace)
+        xml.guid show_trace_url(trace.user, trace)
 
         xml.description do
           xml.cdata! render(:partial => "description", :object => trace, :formats => [:html])
diff --git a/app/views/traces/show.html.erb b/app/views/traces/show.html.erb
index 69f58b5b8..def928da5 100644
--- a/app/views/traces/show.html.erb
+++ b/app/views/traces/show.html.erb
@@ -4,7 +4,7 @@
 
 <% if Settings.status != "gpx_offline" %>
   <% if @trace.inserted %>
-    <img src="<%= url_for :controller => "traces", :action => "picture", :id => @trace.id, :display_name => @trace.user.display_name %>">
+    <%= image_tag trace_picture_path(@trace.user, @trace) %>
   <% else %>
     <span class="text-danger"><%= t ".pending" %></span>
   <% end %>
@@ -31,12 +31,12 @@
               :latitude => tag.span(number_with_delimiter(@trace.latitude), :class => "latitude"),
               :longitude => tag.span(number_with_delimiter(@trace.longitude), :class => "longitude") %>
       </div>
-      (<%= link_to t(".map"), :controller => "site", :action => "index", :mlat => @trace.latitude, :mlon => @trace.longitude, :anchor => "map=14/#{@trace.latitude}/#{@trace.longitude}" %> / <%= link_to t(".edit"), :controller => "site", :action => "edit", :gpx => @trace.id, :anchor => "map=14/#{@trace.latitude}/#{@trace.longitude}" %>)</td>
+      (<%= link_to t(".map"), root_path(:mlat => @trace.latitude, :mlon => @trace.longitude, :anchor => "map=14/#{@trace.latitude}/#{@trace.longitude}") %> / <%= link_to t(".edit"), edit_path(:gpx => @trace.id, :anchor => "map=14/#{@trace.latitude}/#{@trace.longitude}") %>)</td>
   </tr>
   <% end %>
   <tr>
     <th><%= t ".owner" %></th>
-    <td><%= link_to @trace.user.display_name, user_path(@trace.user) %></td>
+    <td><%= link_to @trace.user.display_name, @trace.user %></td>
   </tr>
   <tr>
     <th><%= t ".description" %></th>
@@ -63,6 +63,6 @@
     <% if current_user == @trace.user %>
       <%= link_to t(".edit_trace"), edit_trace_path(@trace), :class => "btn btn-outline-primary" %>
     <% end %>
-    <%= link_to t(".delete_trace"), { :controller => "traces", :action => "destroy", :id => @trace.id }, { :method => :delete, :class => "btn btn-outline-danger", :data => { :confirm => t(".confirm_delete") } } %>
+    <%= link_to t(".delete_trace"), @trace, { :method => :delete, :class => "btn btn-outline-danger", :data => { :confirm => t(".confirm_delete") } } %>
   </div>
 <% end %>
diff --git a/test/controllers/messages_controller_test.rb b/test/controllers/messages_controller_test.rb
index c50eaf562..df7146ad6 100644
--- a/test/controllers/messages_controller_test.rb
+++ b/test/controllers/messages_controller_test.rb
@@ -64,6 +64,7 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
     assert_response :success
     assert_template "new"
     assert_select "title", "Send message | OpenStreetMap"
+    assert_select "a[href='#{user_path recipient_user}']", :text => recipient_user.display_name
     assert_select "form[action='/messages']", :count => 1 do
       assert_select "input[type='hidden'][name='display_name'][value='#{recipient_user.display_name}']"
       assert_select "input#message_title", :count => 1
@@ -299,6 +300,7 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
     get message_path(:id => unread_message)
     assert_response :success
     assert_template "show"
+    assert_select "a[href='#{user_path recipient_user}']", :text => recipient_user.display_name
     assert_not Message.find(unread_message.id).message_read
 
     # Login as the message recipient
@@ -308,6 +310,7 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
     get message_path(:id => unread_message)
     assert_response :success
     assert_template "show"
+    assert_select "a[href='#{user_path user}']", :text => user.display_name
     assert Message.find(unread_message.id).message_read
 
     # Asking to read a message with no ID should fail
@@ -338,7 +341,10 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
     assert_template "inbox"
     assert_select ".content-inner > table", :count => 1 do
       assert_select "tr", :count => 2
-      assert_select "tr#inbox-#{read_message.id}.inbox-row", :count => 1
+      assert_select "tr#inbox-#{read_message.id}.inbox-row", :count => 1 do
+        assert_select "a[href='#{user_path read_message.sender}']", :text => read_message.sender.display_name
+        assert_select "a[href='#{message_path read_message}']", :text => read_message.title
+      end
     end
   end
 
@@ -346,7 +352,7 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
   # test the outbox action
   def test_outbox
     user = create(:user)
-    create(:message, :sender => user)
+    message = create(:message, :sender => user)
 
     # Check that the outbox page requires us to login
     get outbox_messages_path
@@ -361,7 +367,10 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
     assert_template "outbox"
     assert_select ".content-inner > table", :count => 1 do
       assert_select "tr", :count => 2
-      assert_select "tr.inbox-row", :count => 1
+      assert_select "tr.inbox-row", :count => 1 do
+        assert_select "a[href='#{user_path message.recipient}']", :text => message.recipient.display_name
+        assert_select "a[href='#{message_path message}']", :text => message.title
+      end
     end
   end
 
diff --git a/test/controllers/traces_controller_test.rb b/test/controllers/traces_controller_test.rb
index 1953b6378..2a6dc12dc 100644
--- a/test/controllers/traces_controller_test.rb
+++ b/test/controllers/traces_controller_test.rb
@@ -833,6 +833,7 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
             assert_select row, "li", Regexp.new(Regexp.escape("#{trace.size} points")) if trace.inserted?
             assert_select row, "td", Regexp.new(Regexp.escape(trace.description))
             assert_select row, "td", Regexp.new(Regexp.escape("by #{trace.user.display_name}"))
+            assert_select row, "a[href='#{user_path trace.user}']", :text => trace.user.display_name
           end
         end
       end
@@ -845,7 +846,7 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
 
     assert_select "table", :count => 1 do
       assert_select "td", /^#{Regexp.quote(trace.name)} /
-      assert_select "td", trace.user.display_name
+      assert_select "td a[href='#{user_path trace.user}']", :text => trace.user.display_name
       assert_select "td", trace.description
     end
   end