Merge pull request #13629 from opf/rubocop-performance

Add rubocop-performance

.rubocop.yml

@@ -5,6 +5,7 @@ require:
   - ./lib_static/rubocop/cop/open_project/use_service_result_factory_methods.rb
   - rubocop-capybara
   - rubocop-factory_bot
+  - rubocop-performance
 
 <% if File.exist?('.rubocop-local.yml') %>
 inherit_from:
@@ -436,3 +437,6 @@ Style/HashTransformKeys:
 
 Style/HashTransformValues:
   Enabled: true
+
+Performance/Casecmp:
+  Enabled: false

Gemfile

@@ -308,6 +308,7 @@ group :development, :test do
   gem 'rubocop', require: false
   gem 'rubocop-rails', require: false
   gem 'rubocop-rspec', require: false
+  gem 'rubocop-performance', require: false
 
   # Brakeman scanner
   gem 'brakeman', '~> 6.0.0'

Gemfile.lock

@@ -860,6 +860,9 @@ GEM
       rubocop (~> 1.41)
     rubocop-factory_bot (2.23.1)
       rubocop (~> 1.33)
+    rubocop-performance (1.19.0)
+      rubocop (>= 1.7.0, < 2.0)
+      rubocop-ast (>= 0.4.0)
     rubocop-rails (2.20.2)
       activesupport (>= 4.2.0)
       rack (>= 1.1)
@@ -1137,6 +1140,7 @@ DEPENDENCIES
   rspec-rails (~> 6.0.0)
   rspec-retry (~> 0.6.1)
   rubocop
+  rubocop-performance
   rubocop-rails
   rubocop-rspec
   ruby-duration (~> 3.2.0)

app/contracts/base_contract.rb

@@ -101,7 +101,7 @@ def attribute_permission(attribute, permission)
     private
 
     def add_writable(attribute, writable)
-      attribute_name = attribute.to_s.gsub /_id\z/, ''
+      attribute_name = attribute.to_s.delete_suffix('_id')
 
       unless writable == false
         writable_attributes << attribute_name
@@ -244,7 +244,7 @@ def reduce_by_writable_permissions(attributes)
     attribute_permissions = collect_ancestor_attributes(:attribute_permissions)
 
     attributes.reject do |attribute|
-      canonical_attribute = attribute.gsub(/_id\z/, '')
+      canonical_attribute = attribute.delete_suffix('_id')
 
       permissions = attribute_permissions[canonical_attribute] ||
                     attribute_permissions["#{canonical_attribute}_id"] ||

app/contracts/user_preferences/base_contract.rb

@@ -81,7 +81,7 @@ def namesake_time_zone(time_zones)
         if time_zones.length == 1
           time_zones.first
         else
-          time_zones.detect { |tz| tz.tzinfo.name.include?(tz.name.gsub(' ', '_')) }
+          time_zones.detect { |tz| tz.tzinfo.name.include?(tz.name.tr(' ', '_')) }
         end
       end
     end
@@ -109,7 +109,7 @@ def user_allowed_to_access
     end
 
     def full_hour_reminder_time
-      unless model.daily_reminders[:times].all? { |time| time.match?(/00:00\+00:00\z/) }
+      unless model.daily_reminders[:times].all? { |time| time.end_with?('00:00+00:00') }
         errors.add :daily_reminders, :full_hour
       end
     end

app/controllers/application_controller.rb

@@ -377,7 +377,7 @@ def find_work_packages
                                 .order('id ASC')
     fail ActiveRecord::RecordNotFound if @work_packages.empty?
 
-    @projects = @work_packages.map(&:project).compact.uniq
+    @projects = @work_packages.filter_map(&:project).uniq
     @project = @projects.first if @projects.size == 1
   rescue ActiveRecord::RecordNotFound
     render_404
@@ -441,7 +441,7 @@ def accept_key_auth_actions
 
   # Returns a string that can be used as filename value in Content-Disposition header
   def filename_for_content_disposition(name)
-    request.env['HTTP_USER_AGENT'] =~ %r{(MSIE|Trident)} ? ERB::Util.url_encode(name) : name
+    %r{(MSIE|Trident)}.match?(request.env['HTTP_USER_AGENT']) ? ERB::Util.url_encode(name) : name
   end
 
   def api_request?

app/controllers/members_controller.rb

@@ -220,7 +220,7 @@ def user_ids_for_new_members(member_params)
   end
 
   def invite_new_users(user_ids)
-    user_ids.map do |id|
+    user_ids.filter_map do |id|
       if id.to_i == 0 && id.present? # we've got an email - invite that user
         # Only users with the create_user permission can add users.
         if current_user.allowed_to_globally?(:create_user) && enterprise_allow_new_users?
@@ -234,7 +234,7 @@ def invite_new_users(user_ids)
       else
         id
       end
-    end.compact
+    end
   end
 
   def enterprise_allow_new_users?
@@ -243,7 +243,7 @@ def enterprise_allow_new_users?
 
   def each_comma_separated(array, &block)
     array.map do |e|
-      if e.to_s.match /\d(,\d)*/
+      if e.to_s.match? /\d(,\d)*/
         block.call(e)
       else
         e

app/controllers/oauth/auth_base_controller.rb

@@ -57,7 +57,7 @@ def application_http_redirect_uris
 
     def application_native_redirect_uris
       registered_redirect_uris
-        .reject { |url| url.start_with?('http') || url.start_with?('urn') }
+        .reject { |url| url.start_with?('http', 'urn') }
         .map(&method(:parse_native_redirect_uri))
         .compact
     end

app/controllers/repositories_controller.rb

@@ -391,9 +391,7 @@ def graph_commits_per_month(repository)
 
   def graph_commits_per_author(repository)
     commits_by_author = Changeset.where(['repository_id = ?', repository.id]).group(:committer).size
-    commits_by_author.to_a.sort! do |x, y|
-      x.last <=> y.last
-    end
+    commits_by_author.to_a.sort_by!(&:last)
 
     changes_by_author = Change.includes(:changeset)
                         .where(["#{Changeset.table_name}.repository_id = ?", repository.id])

app/controllers/watchers_controller.rb

@@ -48,7 +48,7 @@ def unwatch
   def find_watched_by_object
     klass = params[:object_type].singularize.camelcase.constantize
 
-    return false unless klass.respond_to?('watched_by') and
+    return false unless klass.respond_to?(:watched_by) and
                         klass.ancestors.include? Redmine::Acts::Watchable and
                         params[:object_id].to_s =~ /\A\d+\z/
 

app/helpers/errors_helper.rb

@@ -104,9 +104,9 @@ def render_error(arg)
 
   def unset_template_magic
     if $ERROR_INFO.is_a?(ActionView::ActionViewError)
-      @template.instance_variable_set('@project', nil)
-      @template.instance_variable_set('@status', 500)
-      @template.instance_variable_set('@message', message)
+      @template.instance_variable_set(:@project, nil)
+      @template.instance_variable_set(:@status, 500)
+      @template.instance_variable_set(:@message, message)
     else
       @project = nil
     end

app/helpers/repositories_helper.rb

@@ -56,7 +56,7 @@ def render_properties(properties)
   end
 
   def render_changeset_changes
-    changes = @changeset.file_changes.limit(1000).order(Arel.sql('path')).map do |change|
+    changes = @changeset.file_changes.limit(1000).order(Arel.sql('path')).filter_map do |change|
       case change.action
       when 'A'
         # Detects moved/copied files
@@ -70,7 +70,7 @@ def render_changeset_changes
       else
         change
       end
-    end.compact
+    end
 
     tree = {}
     changes.each do |change|
@@ -178,7 +178,7 @@ def to_utf8_internal(str)
       str.force_encoding('ASCII-8BIT')
     end
     return str if str.empty?
-    return str if /\A[\r\n\t\x20-\x7e]*\Z/n.match(str) # for us-ascii
+    return str if /\A[\r\n\t\x20-\x7e]*\Z/n.match?(str) # for us-ascii
 
     if str.respond_to?(:force_encoding)
       str.force_encoding('UTF-8')

app/helpers/sort_helper.rb

@@ -123,8 +123,8 @@ def to_a
       @criteria
         .map { |c, o| [@available_criteria[c], o] }
         .reject { |c, _| c.nil? }
-        .map { |c, o| append_direction(Array(c), o) }
-        .compact
+        .filter_map { |c, o| append_direction(Array(c), o) }
+
     end
 
     def to_query_hash
@@ -187,7 +187,7 @@ def append_direction(criterion, asc = true)
 
     # Appends DESC to the sort criterion unless it has a fixed order
     def append_desc(criterion)
-      if criterion =~ / (asc|desc)\z/i
+      if / (asc|desc)\z/i.match?(criterion)
         criterion
       else
         "#{criterion} DESC"

app/helpers/watchers_helper.rb

@@ -34,7 +34,7 @@ def watcher_link(object, user, options = { replace: '.watcher_link', class: 'wat
     options = options.with_indifferent_access
     raise ArgumentError, 'Missing :replace option in options hash' if options['replace'].blank?
 
-    return '' unless user&.logged? && object.respond_to?('watched_by?')
+    return '' unless user&.logged? && object.respond_to?(:watched_by?)
 
     watched = object.watched_by?(user)
 

app/mailers/digest_mailer.rb

@@ -67,8 +67,7 @@ def work_packages(recipient_id, notification_ids)
     @mentioned_count = @aggregated_notifications
                          .values
                          .flatten
-                         .map(&:reason)
-                         .compact
+                         .filter_map(&:reason)
                          .count("mentioned")
 
     return if @aggregated_notifications.empty?

app/models/activities/fetcher.rb

@@ -49,9 +49,9 @@ def event_types
       @event_types ||=
         if @project
           OpenProject::Activity.available_event_types.select do |o|
-            permissions = constantized_providers(o).map do |activity_provider|
+            permissions = constantized_providers(o).filter_map do |activity_provider|
               activity_provider.activity_provider_options[:permission]
-            end.compact
+            end
             permissions.all? { |p| @user.allowed_to?(p, @project) }
           end
         else

app/models/colors/hex_color.rb

@@ -53,7 +53,7 @@ def brightness_yiq
     # Splits the hexcode into rbg color array
     def rgb_colors
       hexcode
-        .gsub('#', '') # Remove trailing #
+        .delete('#') # Remove trailing #
         .scan(/../) # Pair hex chars
         .map(&:hex) # to int
     end

app/models/custom_action.rb

@@ -87,9 +87,9 @@ def available_conditions
   end
 
   def conditions
-    @conditions ||= available_conditions.map do |condition_class|
+    @conditions ||= available_conditions.filter_map do |condition_class|
       condition_class.getter(self)
-    end.compact
+    end
   end
 
   def conditions=(new_conditions)

app/models/custom_actions/actions/serializer.rb

@@ -34,7 +34,7 @@ def load(value)
 
     YAML
       .safe_load(value, permitted_classes: [Symbol, Date])
-      .map do |key, values|
+      .filter_map do |key, values|
       klass = nil
 
       CustomActions::Register
@@ -46,7 +46,7 @@ def load(value)
       klass ||= CustomActions::Actions::Inexistent
 
       klass.new(values)
-    end.compact
+    end
   end
 
   def dump(actions)

app/models/mail_handler.rb

@@ -272,8 +272,8 @@ def add_attachments(container)
     email
       .attachments
       .reject { |attachment| ignored_filename?(attachment.filename) }
-      .map { |attachment| create_attachment(attachment, container) }
-      .compact
+      .filter_map { |attachment| create_attachment(attachment, container) }
+
   end
 
   def create_attachment(attachment, container)

app/models/permitted_params.rb

@@ -399,13 +399,13 @@ def custom_field_values(key = nil, required: true)
   def permitted_attributes(key, additions = {})
     merged_args = { params:, current_user: }.merge(additions)
 
-    self.class.permitted_attributes[key].map do |permission|
+    self.class.permitted_attributes[key].filter_map do |permission|
       if permission.respond_to?(:call)
         permission.call(merged_args)
       else
         permission
       end
-    end.compact
+    end
   end
 
   def self.permitted_attributes

app/models/principal.rb

@@ -165,7 +165,7 @@ class << self
     # by the #compact call.
     def type_condition(table = arel_table)
       sti_column = table[inheritance_column]
-      sti_names = ([self] + descendants).map(&:sti_name).compact
+      sti_names = ([self] + descendants).filter_map(&:sti_name)
 
       predicate_builder.build(sti_column, sti_names)
     end

app/models/principals/scopes/possible_assignee.rb

@@ -49,7 +49,7 @@ def possible_assignee(project)
               .assignable
               .of(project)
               .group('user_id')
-              .having(["COUNT(DISTINCT(project_id, user_id)) = ?", Array(project).count])
+              .having(["COUNT(DISTINCT(project_id, user_id)) = ?", Array(project).size])
               .select('user_id')
         )
       end

app/models/queries/filters/strategies/boolean_list.rb

@@ -37,7 +37,7 @@ def validate
     end
 
     def valid_values!
-      filter.values &= allowed_values.map(&:last).map(&:to_s)
+      filter.values &= allowed_values.map { |v| v.last.to_s }
     end
 
     private
@@ -51,7 +51,7 @@ def operator_map
     end
 
     def too_many_values
-      values.reject(&:blank?).length > 1
+      values.count(&:present?) > 1
     end
   end
 end

app/models/queries/filters/strategies/relation.rb

@@ -48,13 +48,13 @@ def validate
     end
 
     def valid_values!
-      filter.values &= allowed_values.map(&:last).map(&:to_s)
+      filter.values &= allowed_values.map { |v| v.last.to_s }
     end
 
     private
 
     def too_many_values
-      values.reject(&:blank?).length > 1
+      values.count(&:present?) > 1
     end
   end
 end

app/models/queries/work_packages/columns/property_column.rb

@@ -132,10 +132,10 @@ def caption
   }
 
   def self.instances(_context = nil)
-    property_columns.map do |name, options|
+    property_columns.filter_map do |name, options|
       next unless !options[:if] || options[:if].call
 
       new(name, options.except(:if))
-    end.compact
+    end
   end
 end

app/models/queries/work_packages/filter/category_filter.rb

@@ -48,8 +48,8 @@ def value_objects
     available_categories = all_project_categories.index_by(&:id)
 
     values
-      .map { |category_id| available_categories[category_id.to_i] }
-      .compact
+      .filter_map { |category_id| available_categories[category_id.to_i] }
+
   end
 
   def ar_object_filter?

app/models/queries/work_packages/filter/group_filter.rb

@@ -55,8 +55,8 @@ def value_objects
     available_groups = all_groups.index_by(&:id)
 
     values
-      .map { |group_id| available_groups[group_id.to_i] }
-      .compact
+      .filter_map { |group_id| available_groups[group_id.to_i] }
+
   end
 
   def where