From: Tom Hughes <tom@compton.nu>
Date: Wed, 28 Oct 2020 15:56:59 +0000 (+0000)
Subject: Merge remote-tracking branch 'upstream/pull/2920' into master
X-Git-Tag: live~1904
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/2479319b7859d9b271dc8900e5d530c77976025a?hp=10d88e8a6fb5d01ff1e990a4bbbd1aeaf4cc26b7

Merge remote-tracking branch 'upstream/pull/2920' into master
---

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index e983c51c6..cfff8c43e 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -84,21 +84,6 @@ Metrics/PerceivedComplexity:
 Minitest/MultipleAssertions:
   Max: 81
 
-# Offense count: 26
-# Cop supports --auto-correct.
-Minitest/TestMethodName:
-  Exclude:
-    - 'test/abilities/api_capability_test.rb'
-    - 'test/controllers/api/nodes_controller_test.rb'
-    - 'test/controllers/api/old_nodes_controller_test.rb'
-    - 'test/controllers/api/relations_controller_test.rb'
-    - 'test/controllers/api/ways_controller_test.rb'
-    - 'test/helpers/browse_helper_test.rb'
-    - 'test/integration/client_applications_test.rb'
-    - 'test/integration/short_links_test.rb'
-    - 'test/integration/user_blocks_test.rb'
-    - 'test/integration/user_creation_test.rb'
-
 # Offense count: 6
 Naming/AccessorMethodName:
   Exclude:
@@ -165,9 +150,6 @@ Rails/OutputSafety:
   Exclude:
     - 'app/controllers/users_controller.rb'
     - 'app/helpers/application_helper.rb'
-    - 'app/helpers/changesets_helper.rb'
-    - 'app/helpers/geocoder_helper.rb'
-    - 'app/helpers/user_blocks_helper.rb'
     - 'lib/rich_text.rb'
     - 'test/helpers/application_helper_test.rb'
 
diff --git a/app/helpers/changesets_helper.rb b/app/helpers/changesets_helper.rb
index 97a70f789..b91810e95 100644
--- a/app/helpers/changesets_helper.rb
+++ b/app/helpers/changesets_helper.rb
@@ -17,7 +17,7 @@ module ChangesetsHelper
     else
       action = :closed
       time = time_ago_in_words(changeset.closed_at, :scope => :'datetime.distance_in_words_ago')
-      title = "#{t('browse.created')}: #{l(changeset.created_at)}&#10;#{t('browse.closed')}: #{l(changeset.closed_at)}".html_safe
+      title = safe_join([t("browse.created"), ": ", l(changeset.created_at), "&#10;".html_safe, t("browse.closed"), ": ", l(changeset.closed_at)])
     end
 
     if params.key?(:display_name)
diff --git a/app/helpers/geocoder_helper.rb b/app/helpers/geocoder_helper.rb
index 161bb2d6d..1826b08a4 100644
--- a/app/helpers/geocoder_helper.rb
+++ b/app/helpers/geocoder_helper.rb
@@ -14,13 +14,13 @@ module GeocoderHelper
       html_options[:data][key.to_s.tr("_", "-")] = value
     end
 
-    html = ""
+    html = []
     html << result[:prefix] if result[:prefix]
     html << " " if result[:prefix] && result[:name]
     html << link_to(result[:name], url, html_options) if result[:name]
     html << " " if result[:suffix] && result[:name]
     html << result[:suffix] if result[:suffix]
-    html.html_safe
+    safe_join(html)
   end
 
   def describe_location(lat, lon, zoom = nil, language = nil)
diff --git a/app/helpers/user_blocks_helper.rb b/app/helpers/user_blocks_helper.rb
index 9f0c4a304..73425edec 100644
--- a/app/helpers/user_blocks_helper.rb
+++ b/app/helpers/user_blocks_helper.rb
@@ -1,4 +1,6 @@
 module UserBlocksHelper
+  include ActionView::Helpers::TranslationHelper
+
   ##
   # returns a translated string representing the status of the
   # user block (i.e: whether it's active, what the expiry time is)
@@ -7,34 +9,34 @@ module UserBlocksHelper
       # if the block hasn't expired yet show the date, if the user just needs to login show that
       if block.needs_view?
         if block.ends_at > Time.now.getutc
-          I18n.t("user_blocks.helper.time_future_and_until_login", :time => friendly_date(block.ends_at)).html_safe
+          t("user_blocks.helper.time_future_and_until_login_html", :time => friendly_date(block.ends_at))
         else
-          I18n.t("user_blocks.helper.until_login")
+          t("user_blocks.helper.until_login")
         end
       else
-        I18n.t("user_blocks.helper.time_future", :time => friendly_date(block.ends_at)).html_safe
+        t("user_blocks.helper.time_future_html", :time => friendly_date(block.ends_at))
       end
     else
       # the max of the last update time or the ends_at time is when this block finished
       # either because the user viewed the block (updated_at) or it expired or was
       # revoked (ends_at)
       last_time = [block.ends_at, block.updated_at].max
-      I18n.t("user_blocks.helper.time_past", :time => friendly_date_ago(last_time)).html_safe
+      t("user_blocks.helper.time_past_html", :time => friendly_date_ago(last_time))
     end
   end
 
   def block_duration_in_words(duration)
     parts = ActiveSupport::Duration.build(duration).parts
     if duration < 1.day
-      I18n.t("user_blocks.helper.block_duration.hours", :count => parts[:hours])
+      t("user_blocks.helper.block_duration.hours", :count => parts[:hours])
     elsif duration < 1.week
-      I18n.t("user_blocks.helper.block_duration.days", :count => parts[:days])
+      t("user_blocks.helper.block_duration.days", :count => parts[:days])
     elsif duration < 1.month
-      I18n.t("user_blocks.helper.block_duration.weeks", :count => parts[:weeks])
+      t("user_blocks.helper.block_duration.weeks", :count => parts[:weeks])
     elsif duration < 1.year
-      I18n.t("user_blocks.helper.block_duration.months", :count => parts[:months])
+      t("user_blocks.helper.block_duration.months", :count => parts[:months])
     else
-      I18n.t("user_blocks.helper.block_duration.years", :count => parts[:years])
+      t("user_blocks.helper.block_duration.years", :count => parts[:years])
     end
   end
 end
diff --git a/config/locales/en.yml b/config/locales/en.yml
index cc1fa7316..37d6f99e9 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -2572,10 +2572,10 @@ en:
       revoke: "Revoke!"
       flash: "This block has been revoked."
     helper:
-      time_future: "Ends in %{time}."
+      time_future_html: "Ends in %{time}."
       until_login: "Active until the user logs in."
-      time_future_and_until_login: "Ends in %{time} and after the user has logged in."
-      time_past: "Ended %{time}."
+      time_future_and_until_login_html: "Ends in %{time} and after the user has logged in."
+      time_past_html: "Ended %{time}."
       block_duration:
         hours:
           one: "1 hour"
diff --git a/test/abilities/api_capability_test.rb b/test/abilities/api_capability_test.rb
index 8a98f29e0..dccde5758 100644
--- a/test/abilities/api_capability_test.rb
+++ b/test/abilities/api_capability_test.rb
@@ -3,6 +3,8 @@
 require "test_helper"
 
 class ApiCapabilityTest < ActiveSupport::TestCase
+  private
+
   def tokens(*toks)
     AccessToken.new do |token|
       toks.each do |t|
diff --git a/test/controllers/api/nodes_controller_test.rb b/test/controllers/api/nodes_controller_test.rb
index 645408dd4..339dd2af8 100644
--- a/test/controllers/api/nodes_controller_test.rb
+++ b/test/controllers/api/nodes_controller_test.rb
@@ -559,6 +559,8 @@ module Api
       assert_includes apinode.tags, "\#{@user.inspect}"
     end
 
+    private
+
     ##
     # update the changeset_id of a node element
     def update_changeset(xml, changeset_id)
diff --git a/test/controllers/api/old_nodes_controller_test.rb b/test/controllers/api/old_nodes_controller_test.rb
index cc7fae416..e85bc86f8 100644
--- a/test/controllers/api/old_nodes_controller_test.rb
+++ b/test/controllers/api/old_nodes_controller_test.rb
@@ -161,13 +161,6 @@ module Api
       check_not_found_id_version(24356, create(:node).version)
     end
 
-    def check_not_found_id_version(id, version)
-      get node_version_path(:id => id, :version => version)
-      assert_response :not_found
-    rescue ActionController::UrlGenerationError => e
-      assert_match(/No route matches/, e.to_s)
-    end
-
     ##
     # Test that getting the current version is identical to picking
     # that version with the version URI call.
@@ -419,6 +412,13 @@ module Api
       assert_nodes_are_equal current_node, old_node
     end
 
+    def check_not_found_id_version(id, version)
+      get node_version_path(:id => id, :version => version)
+      assert_response :not_found
+    rescue ActionController::UrlGenerationError => e
+      assert_match(/No route matches/, e.to_s)
+    end
+
     ##
     # returns a 16 character long string with some nasty characters in it.
     # this ought to stress-test the tag handling as well as the versioning.
diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index 559f19bd6..de24c087d 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -151,25 +151,6 @@ module Api
                                   [relation_with_relation, second_relation])
     end
 
-    def check_relations_for_element(path, type, id, expected_relations)
-      # check the "relations for relation" mode
-      get path
-      assert_response :success
-
-      # count one osm element
-      assert_select "osm[version='#{Settings.api_version}'][generator='OpenStreetMap server']", 1
-
-      # we should have only the expected number of relations
-      assert_select "osm>relation", expected_relations.size
-
-      # and each of them should contain the element we originally searched for
-      expected_relations.each do |relation|
-        # The relation should appear once, but the element could appear multiple times
-        assert_select "osm>relation[id='#{relation.id}']", 1
-        assert_select "osm>relation[id='#{relation.id}']>member[type='#{type}'][ref='#{id}']"
-      end
-    end
-
     def test_full
       # check the "full" mode
       get relation_full_path(:id => 999999)
@@ -926,9 +907,26 @@ module Api
       end
     end
 
-    # ============================================================
-    # utility functions
-    # ============================================================
+    private
+
+    def check_relations_for_element(path, type, id, expected_relations)
+      # check the "relations for relation" mode
+      get path
+      assert_response :success
+
+      # count one osm element
+      assert_select "osm[version='#{Settings.api_version}'][generator='OpenStreetMap server']", 1
+
+      # we should have only the expected number of relations
+      assert_select "osm>relation", expected_relations.size
+
+      # and each of them should contain the element we originally searched for
+      expected_relations.each do |relation|
+        # The relation should appear once, but the element could appear multiple times
+        assert_select "osm>relation[id='#{relation.id}']", 1
+        assert_select "osm>relation[id='#{relation.id}']>member[type='#{type}'][ref='#{id}']"
+      end
+    end
 
     ##
     # checks that the XML document and the string arguments have
diff --git a/test/controllers/api/ways_controller_test.rb b/test/controllers/api/ways_controller_test.rb
index 74ef9f524..0cf30e4ff 100644
--- a/test/controllers/api/ways_controller_test.rb
+++ b/test/controllers/api/ways_controller_test.rb
@@ -754,6 +754,8 @@ module Api
       end
     end
 
+    private
+
     ##
     # update the changeset_id of a way element
     def update_changeset(xml, changeset_id)
diff --git a/test/helpers/browse_helper_test.rb b/test/helpers/browse_helper_test.rb
index 8e46f6ff3..bc597596f 100644
--- a/test/helpers/browse_helper_test.rb
+++ b/test/helpers/browse_helper_test.rb
@@ -132,6 +132,8 @@ class BrowseHelperTest < ActionView::TestCase
     assert_includes tags, %w[shop gift]
   end
 
+  private
+
   def add_old_tags_selection(old_node)
     { "building" => "yes",
       "shop" => "gift",
@@ -152,8 +154,6 @@ class BrowseHelperTest < ActionView::TestCase
     end
   end
 
-  private
-
   def preferred_languages
     Locale.list(I18n.locale)
   end
diff --git a/test/integration/client_applications_test.rb b/test/integration/client_applications_test.rb
index 4c3e9df47..5a00a5631 100644
--- a/test/integration/client_applications_test.rb
+++ b/test/integration/client_applications_test.rb
@@ -76,6 +76,8 @@ class ClientApplicationsTest < ActionDispatch::IntegrationTest
     # tests, as its too tied into the HTTP headers and stuff that it signs.
   end
 
+  private
+
   ##
   # utility method to make the HTML screening easier to read.
   def assert_in_heading(&block)
diff --git a/test/integration/short_links_test.rb b/test/integration/short_links_test.rb
index c03ebedab..8f949fdae 100644
--- a/test/integration/short_links_test.rb
+++ b/test/integration/short_links_test.rb
@@ -9,6 +9,8 @@ class ShortLinksTest < ActionDispatch::IntegrationTest
     assert_short_link_redirect(ShortLink.encode(-0.107846, 51.50771, 18))
   end
 
+  private
+
   ##
   # utility method to test short links
   def assert_short_link_redirect(short_link)
diff --git a/test/integration/user_blocks_test.rb b/test/integration/user_blocks_test.rb
index ce67e74ba..ecadaad53 100644
--- a/test/integration/user_blocks_test.rb
+++ b/test/integration/user_blocks_test.rb
@@ -1,17 +1,13 @@
 require "test_helper"
 
 class UserBlocksTest < ActionDispatch::IntegrationTest
-  def auth_header(user, pass)
-    { "HTTP_AUTHORIZATION" => format("Basic %<auth>s", :auth => Base64.encode64("#{user}:#{pass}")) }
-  end
-
   def test_api_blocked
     blocked_user = create(:user)
 
     get "/api/#{Settings.api_version}/user/details"
     assert_response :unauthorized
 
-    get "/api/#{Settings.api_version}/user/details", :headers => auth_header(blocked_user.display_name, "test")
+    get "/api/#{Settings.api_version}/user/details", :headers => basic_authorization_header(blocked_user.display_name, "test")
     assert_response :success
 
     # now block the user
@@ -21,7 +17,7 @@ class UserBlocksTest < ActionDispatch::IntegrationTest
       :reason => "testing",
       :ends_at => Time.now.getutc + 5.minutes
     )
-    get "/api/#{Settings.api_version}/user/details", :headers => auth_header(blocked_user.display_name, "test")
+    get "/api/#{Settings.api_version}/user/details", :headers => basic_authorization_header(blocked_user.display_name, "test")
     assert_response :forbidden
   end
 
@@ -35,7 +31,7 @@ class UserBlocksTest < ActionDispatch::IntegrationTest
       :reason => "testing",
       :ends_at => Time.now.getutc + 5.minutes
     )
-    get "/api/#{Settings.api_version}/user/details", :headers => auth_header(blocked_user.display_name, "test")
+    get "/api/#{Settings.api_version}/user/details", :headers => basic_authorization_header(blocked_user.display_name, "test")
     assert_response :forbidden
 
     # revoke the ban
@@ -54,7 +50,7 @@ class UserBlocksTest < ActionDispatch::IntegrationTest
     reset!
 
     # access the API again. this time it should work
-    get "/api/#{Settings.api_version}/user/details", :headers => auth_header(blocked_user.display_name, "test")
+    get "/api/#{Settings.api_version}/user/details", :headers => basic_authorization_header(blocked_user.display_name, "test")
     assert_response :success
   end
 end
diff --git a/test/integration/user_creation_test.rb b/test/integration/user_creation_test.rb
index 9c934dc07..8b6b3ef3c 100644
--- a/test/integration/user_creation_test.rb
+++ b/test/integration/user_creation_test.rb
@@ -157,7 +157,7 @@ class UserCreationTest < ActionDispatch::IntegrationTest
   end
 
   # Check that the user can successfully recover their password
-  def lost_password_recovery_success
+  def test_lost_password_recovery_success
     # Open the lost password form
     # Submit the lost password form
     # Check the e-mail