Merge branch '239356-fix-Style/FormatString' into 'master'

Fix Style/FormatString offenses

See merge request !178810



Merged-by: Peter Leitzen <pleitzen@gitlab.com>
Approved-by: Peter Leitzen <pleitzen@gitlab.com>
Approved-by: Andrew Evans <aevans@gitlab.com>
Reviewed-by: Peter Leitzen <pleitzen@gitlab.com>
Reviewed-by: Nao Hashizume <nhashizume@gitlab.com>
Co-authored-by: Jonston Chan <jonstonchan1@gmail.com>

.rubocop_todo/style/format_string.yml

@@ -2,21 +2,6 @@
 # Cop supports --autocorrect.
 Style/FormatString:
   Exclude:
-    - 'app/components/diffs/overflow_warning_component.rb'
-    - 'app/controllers/import/bitbucket_server_controller.rb'
-    - 'app/controllers/import/bulk_imports_controller.rb'
-    - 'app/controllers/import/fogbugz_controller.rb'
-    - 'app/controllers/import/gitea_controller.rb'
-    - 'app/controllers/import/github_controller.rb'
-    - 'app/controllers/import/gitlab_groups_controller.rb'
-    - 'app/controllers/import/gitlab_projects_controller.rb'
-    - 'app/controllers/invites_controller.rb'
-    - 'app/controllers/omniauth_callbacks_controller.rb'
-    - 'app/controllers/profiles/chat_names_controller.rb'
-    - 'app/controllers/profiles/emails_controller.rb'
-    - 'app/controllers/profiles/preferences_controller.rb'
-    - 'app/controllers/profiles/two_factor_auths_controller.rb'
-    - 'app/controllers/profiles_controller.rb'
     - 'app/controllers/projects/google_cloud/deployments_controller.rb'
     - 'app/controllers/projects/google_cloud/service_accounts_controller.rb'
     - 'app/controllers/projects/issues_controller.rb'

app/components/diffs/overflow_warning_component.rb

@@ -15,12 +15,12 @@ def before_render
     end
 
     def message
-      html_escape(message_text) % {
+      safe_format(
+        message_text,
         display_size: @diff_files.size,
         real_size: @diffs.real_size,
-        strong_open: '<strong>'.html_safe,
-        strong_close: '</strong>'.html_safe
-      }
+        **tag_pair(tag.strong, :strong_open, :strong_close)
+      )
     end
 
     def diff_link

app/controllers/import/bitbucket_server_controller.rb

@@ -173,14 +173,14 @@ def limit_per_page
   end
 
   def bitbucket_connection_error(error)
-    flash[:alert] = _("Unable to connect to server: %{error}") % { error: error }
+    flash[:alert] = safe_format(_("Unable to connect to server: %{error}"), error: error)
     clear_session_data
 
     respond_to do |format|
       format.json do
         render json: {
           error: {
-            message: _("Unable to connect to server: %{error}") % { error: error },
+            message: safe_format(_("Unable to connect to server: %{error}"), error: error),
             redirect: new_import_bitbucket_server_path
           }
         }, status: :unprocessable_entity

app/controllers/import/bulk_imports_controller.rb

@@ -2,6 +2,7 @@
 
 class Import::BulkImportsController < ApplicationController
   include ActionView::Helpers::SanitizeHelper
+  include SafeFormatHelper
 
   before_action :ensure_bulk_import_enabled
   before_action :verify_blocked_uri, only: :status
@@ -181,7 +182,7 @@ def verify_blocked_uri
     clear_session_data
 
     redirect_to new_group_path(anchor: 'import-group-pane'),
-      alert: _('Specified URL cannot be used: "%{reason}"') % { reason: e.message }
+      alert: safe_format(_('Specified URL cannot be used: "%{reason}"'), reason: e.message)
   end
 
   def allow_local_requests?
@@ -191,7 +192,7 @@ def allow_local_requests?
   def bulk_import_connection_error(error)
     clear_session_data
 
-    error_message = _("Unable to connect to server: %{error}") % { error: error }
+    error_message = safe_format(_("Unable to connect to server: %{error}"), error: error)
     flash[:alert] = error_message
 
     respond_to do |format|

app/controllers/import/fogbugz_controller.rb

@@ -3,6 +3,8 @@
 class Import::FogbugzController < Import::BaseController
   extend ::Gitlab::Utils::Override
 
+  include SafeFormatHelper
+
   before_action :verify_fogbugz_import_enabled
   before_action -> { check_rate_limit!(:fogbugz_import, scope: current_user, redirect_back: true) }, only: :callback
 
@@ -133,7 +135,9 @@ def verify_blocked_uri
       outbound_local_requests_allowlist: Gitlab::CurrentSettings.outbound_local_requests_whitelist # rubocop:disable Naming/InclusiveLanguage -- existing setting
     )
   rescue Gitlab::HTTP_V2::UrlBlocker::BlockedUrlError => e
-    redirect_to new_import_fogbugz_url, alert: _('Specified URL cannot be used: "%{reason}"') % { reason: e.message }
+    redirect_to new_import_fogbugz_url, alert: safe_format(
+      _('Specified URL cannot be used: "%{reason}"'), reason: e.message
+    )
   end
 
   def allow_local_requests?

app/controllers/import/gitea_controller.rb

@@ -105,7 +105,7 @@ def verify_blocked_uri
   rescue Gitlab::HTTP_V2::UrlBlocker::BlockedUrlError => e
     session[access_token_key] = nil
 
-    redirect_to new_import_url, alert: _('Specified URL cannot be used: "%{reason}"') % { reason: e.message }
+    redirect_to new_import_url, alert: safe_format(_('Specified URL cannot be used: "%{reason}"'), reason: e.message)
   end
 
   def allow_local_requests?

app/controllers/import/github_controller.rb

@@ -245,7 +245,7 @@ def provider_rate_limit(exception)
     reset_time = Time.zone.at(exception.response_headers['x-ratelimit-reset'].to_i)
     session[access_token_key] = nil
     redirect_to new_import_url,
-      alert: _("GitHub API rate limit exceeded. Try again after %{reset_time}") % { reset_time: reset_time }
+      alert: safe_format(_("GitHub API rate limit exceeded. Try again after %{reset_time}"), reset_time: reset_time)
   end
 
   def provider_forbidden

app/controllers/import/gitlab_groups_controller.rb

 # frozen_string_literal: true
 
 class Import::GitlabGroupsController < ApplicationController
+  include SafeFormatHelper
   include WorkhorseAuthorization
 
   before_action :check_import_rate_limit!, only: %i[create]
@@ -30,16 +31,15 @@ def create
       if Groups::ImportExport::ImportService.new(group: group, user: current_user).async_execute
         redirect_to(
           group_path(group),
-          notice: s_("GroupImport|Group '%{group_name}' is being imported.") % { group_name: group.name }
+          notice: safe_format(s_("GroupImport|Group '%{group_name}' is being imported."), group_name: group.name)
         )
       else
         redirect_to group_path(group), alert: _("Group import could not be scheduled")
       end
     else
       redirect_to new_group_path(anchor: 'import-group-pane'),
-        alert: s_("GroupImport|Group could not be imported: %{errors}") % {
-          errors: group.errors.full_messages.to_sentence
-        }
+        alert: safe_format(s_("GroupImport|Group could not be imported: %{errors}"),
+          errors: group.errors.full_messages.to_sentence)
     end
   end
 

app/controllers/import/gitlab_projects_controller.rb

 # frozen_string_literal: true
 
 class Import::GitlabProjectsController < Import::BaseController
+  include SafeFormatHelper
   include WorkhorseAuthorization
 
   before_action :disable_query_limiting, only: [:create]
@@ -27,7 +28,7 @@ def create
     if @project.saved?
       redirect_to(
         project_path(@project),
-        notice: _("Project '%{project_name}' is being imported.") % { project_name: @project.name }
+        notice: safe_format(_("Project '%{project_name}' is being imported."), project_name: @project.name)
       )
     else
       redirect_back_or_default(

app/controllers/invites_controller.rb

@@ -2,6 +2,7 @@
 
 class InvitesController < ApplicationController
   include Gitlab::Utils::StrongMemoize
+  include SafeFormatHelper
 
   prepend_before_action :authenticate_user!, :track_invite_join_click, only: :show
   before_action :member
@@ -38,8 +39,10 @@ def decline
           new_user_session_path
         end
 
-      redirect_to path, notice: _("You have declined the invitation to join %{title} %{name}.") %
-        { title: invite_details[:title], name: invite_details[:name] }
+      redirect_to path,
+        notice: safe_format(_("You have declined the invitation to join %{title} %{name}."),
+          title: invite_details[:title],
+          name: invite_details[:name])
     else
       redirect_back_or_default(options: { alert: _("The invitation could not be declined.") })
     end

app/controllers/omniauth_callbacks_controller.rb

@@ -203,7 +203,7 @@ def redirect_identity_exists
 
   def redirect_identity_link_failed(error_message)
     redirect_to profile_account_path,
-      notice: _("Authentication failed: %{error_message}") % { error_message: error_message }
+      notice: safe_format(_("Authentication failed: %{error_message}"), error_message: error_message)
   end
 
   def redirect_identity_linked
@@ -288,10 +288,8 @@ def handle_signup_error
     label = Gitlab::Auth::OAuth::Provider.label_for(oauth['provider'])
     simple_url = Settings.gitlab.url.sub(%r{^https?://(www\.)?}i, '')
     message = [
-      _('Signing in using your %{label} account without a pre-existing ' \
-        'account in %{simple_url} is not allowed.') % {
-          label: label, simple_url: simple_url
-        }
+      safe_format(_('Signing in using your %{label} account without a pre-existing ' \
+        'account in %{simple_url} is not allowed.'), label: label, simple_url: simple_url)
     ]
 
     if Gitlab::CurrentSettings.allow_signup?
@@ -349,7 +347,7 @@ def redirect_unverified_saml_initiation
 
   def handle_identity_with_untrusted_extern_uid
     label = Gitlab::Auth::OAuth::Provider.label_for(oauth['provider'])
-    flash[:alert] = format(
+    flash[:alert] = safe_format(
       _('Signing in using your %{label} account has been disabled for security reasons. ' \
         'Please sign in to your GitLab account using another authentication method and ' \
         'reconnect to your %{label} account.'
@@ -362,7 +360,7 @@ def handle_identity_with_untrusted_extern_uid
 
   def handle_disabled_provider
     label = Gitlab::Auth::OAuth::Provider.label_for(oauth['provider'])
-    flash[:alert] = _("Signing in using %{label} has been disabled") % { label: label }
+    flash[:alert] = safe_format(_("Signing in using %{label} has been disabled"), label: label)
 
     redirect_to new_user_session_path
   end

app/controllers/profiles/chat_names_controller.rb

 # frozen_string_literal: true
 
 class Profiles::ChatNamesController < Profiles::ApplicationController
+  include SafeFormatHelper
+
   before_action :chat_name_token, only: [:new]
   before_action :chat_name_params, only: [:new, :create, :deny]
 
@@ -18,7 +20,7 @@ def create
     new_chat_name = current_user.chat_names.new(chat_name_params)
 
     if new_chat_name.save
-      flash[:notice] = _("Authorized %{new_chat_name}") % { new_chat_name: new_chat_name.chat_name }
+      flash[:notice] = safe_format(_("Authorized %{new_chat_name}"), new_chat_name: new_chat_name.chat_name)
     else
       flash[:alert] = _("Could not authorize chat nickname. Try again!")
     end
@@ -31,7 +33,7 @@ def deny
     delete_chat_name_token
 
     flash[:notice] =
-      _("Denied authorization of chat nickname %{user_name}.") % { user_name: chat_name_params[:user_name] }
+      safe_format(_("Denied authorization of chat nickname %{user_name}."), user_name: chat_name_params[:user_name])
 
     redirect_to profile_chat_names_path
   end
@@ -40,9 +42,9 @@ def destroy
     @chat_name = chat_names.find(params[:id])
 
     if @chat_name.destroy
-      flash[:notice] = _("Deleted chat nickname: %{chat_name}!") % { chat_name: @chat_name.chat_name }
+      flash[:notice] = safe_format(_("Deleted chat nickname: %{chat_name}!"), chat_name: @chat_name.chat_name)
     else
-      flash[:alert] = _("Could not delete chat nickname %{chat_name}.") % { chat_name: @chat_name.chat_name }
+      flash[:alert] = safe_format(_("Could not delete chat nickname %{chat_name}."), chat_name: @chat_name.chat_name)
     end
 
     redirect_to profile_chat_names_path, status: :found

app/controllers/profiles/emails_controller.rb

 # frozen_string_literal: true
 
 class Profiles::EmailsController < Profiles::ApplicationController
+  include SafeFormatHelper
+
   before_action :find_email, only: [:destroy, :resend_confirmation_instructions]
   before_action -> { check_rate_limit!(:profile_add_new_email, scope: current_user, redirect_back: true) },
     only: [:create]
@@ -33,7 +35,7 @@ def destroy
 
   def resend_confirmation_instructions
     if Emails::ConfirmService.new(current_user, user: current_user).execute(@email)
-      flash[:notice] = _("Confirmation email sent to %{email}") % { email: @email.email }
+      flash[:notice] = safe_format(_("Confirmation email sent to %{email}"), email: @email.email)
     else
       flash[:alert] = _("There was a problem sending the confirmation email")
     end

app/controllers/profiles/preferences_controller.rb

 # frozen_string_literal: true
 
 class Profiles::PreferencesController < Profiles::ApplicationController
+  include SafeFormatHelper
+
   before_action :user
 
   feature_category :user_profile
@@ -21,7 +23,7 @@ def update
     end
   rescue ArgumentError => e
     # Raised when `dashboard` is given an invalid value.
-    message = _("Failed to save preferences (%{error_message}).") % { error_message: e.message }
+    message = safe_format(_("Failed to save preferences (%{error_message})."), error_message: e.message)
     render status: :bad_request, json: { type: :alert, message: message }
   end
 

app/controllers/profiles/two_factor_auths_controller.rb

@@ -222,7 +222,7 @@ def webauthn_options
   def groups_notification(groups)
     group_links = groups.map { |group| view_context.link_to group.full_name, group_path(group) }.to_sentence
     leave_group_links = groups.map do |group|
-      view_context.link_to (s_("leave %{group_name}") % { group_name: group.full_name }),
+      view_context.link_to safe_format(s_("leave %{group_name}"), group_name: group.full_name),
         leave_group_members_path(group),
         remote: false, method: :delete
     end.to_sentence

app/controllers/profiles_controller.rb

@@ -3,6 +3,7 @@
 class ProfilesController < Profiles::ApplicationController
   include ActionView::Helpers::SanitizeHelper
   include Gitlab::Tracking
+  include SafeFormatHelper
 
   before_action :user
   before_action :authorize_change_username!, only: :update_username
@@ -54,7 +55,7 @@ def update_username
         format.html { redirect_back_or_default(default: user_settings_profile_path, options: { notice: message }) }
         format.json { render json: { message: message }, status: :ok }
       else
-        message = s_("Profiles|Username change failed - %{message}") % { message: result[:message] }
+        message = safe_format(s_("Profiles|Username change failed - %{message}"), message: result[:message])
 
         format.html { redirect_back_or_default(default: user_settings_profile_path, options: { alert: message }) }
         format.json { render json: { message: message }, status: :unprocessable_entity }

spec/components/diffs/overflow_warning_component_spec.rb

@@ -114,6 +114,10 @@ def parse_link(html)
   describe "#message" do
     subject { component.message }
 
+    before do
+      render_inline component
+    end
+
     it { is_expected.to be_a(String) }
 
     it "is HTML-safe" do

spec/controllers/import/bulk_imports_controller_spec.rb

@@ -224,7 +224,7 @@ def get_status(params_override = {}, format = :json)
           it 'denies network request' do
             get :status
             expect(controller).to redirect_to(new_group_path(anchor: 'import-group-pane'))
-            expect(flash[:alert]).to eq("Specified URL cannot be used: \"#{expected_error}\"")
+            expect(flash[:alert]).to eq("Specified URL cannot be used: "#{expected_error}"")
           end
         end
 

spec/controllers/import/fogbugz_controller_spec.rb

@@ -64,7 +64,7 @@
           post :callback, params: { uri: uri, email: 'test@example.com', password: 'mypassword' }
 
           expect(response).to redirect_to(new_import_fogbugz_url)
-          expect(flash[:alert]).to eq("Specified URL cannot be used: \"#{reason}\"")
+          expect(flash[:alert]).to eq("Specified URL cannot be used: "#{reason}"")
         end
       end
 

spec/controllers/import/gitea_controller_spec.rb

@@ -53,7 +53,7 @@ def assign_host_url
           get :status, format: :json
 
           expect(controller).to redirect_to(new_import_url)
-          expect(flash[:alert]).to eq("Specified URL cannot be used: \"#{expected_error}\"")
+          expect(flash[:alert]).to eq("Specified URL cannot be used: "#{expected_error}"")
         end
       end