Merge branch '50414-rubocop-rule-to-enforce-class-methods-over-module' into 'master'

CE Port 50414 rubocop rule to enforce class methods over module

See merge request gitlab-org/gitlab-ee!7047

app/models/concerns/atomic_internal_id.rb

@@ -26,7 +26,7 @@
 module AtomicInternalId
   extend ActiveSupport::Concern
 
-  module ClassMethods
+  class_methods do
     def has_internal_id(column, scope:, init:, presence: true) # rubocop:disable Naming/PredicateName
       # We require init here to retain the ability to recalculate in the absence of a
       # InternaLId record (we may delete records in `internal_ids` for example).

app/models/concerns/awardable.rb

@@ -12,7 +12,7 @@ module Awardable
     end
   end
 
-  module ClassMethods
+  class_methods do
     def awarded(user, name)
       sql = <<~EOL
         EXISTS (

app/models/concerns/case_sensitivity.rb

@@ -4,7 +4,7 @@
 module CaseSensitivity
   extend ActiveSupport::Concern
 
-  module ClassMethods
+  class_methods do
     # Queries the given columns regardless of the casing used.
     #
     # Unlike other ActiveRecord methods this method only operates on a Hash.

app/models/concerns/each_batch.rb

@@ -3,7 +3,7 @@
 module EachBatch
   extend ActiveSupport::Concern
 
-  module ClassMethods
+  class_methods do
     # Iterates over the rows in a relation in batches, similar to Rails'
     # `in_batches` but in a more efficient way.
     #

app/models/concerns/ignorable_column.rb

@@ -14,7 +14,7 @@
 module IgnorableColumn
   extend ActiveSupport::Concern
 
-  module ClassMethods
+  class_methods do
     def columns
       super.reject { |column| ignored_columns.include?(column.name) }
     end

app/models/concerns/issuable.rb

@@ -117,7 +117,7 @@ module Issuable
     end
   end
 
-  module ClassMethods
+  class_methods do
     # Searches for records with a matching title.
     #
     # This method uses ILIKE on PostgreSQL and LIKE on MySQL.

app/models/concerns/loaded_in_group_list.rb

@@ -3,7 +3,7 @@
 module LoadedInGroupList
   extend ActiveSupport::Concern
 
-  module ClassMethods
+  class_methods do
     def with_counts(archived:)
       selects_including_counts = [
         'namespaces.*',

app/models/concerns/manual_inverse_association.rb

@@ -3,7 +3,7 @@
 module ManualInverseAssociation
   extend ActiveSupport::Concern
 
-  module ClassMethods
+  class_methods do
     def manual_inverse_association(association, inverse)
       define_method(association) do |*args|
         super(*args).tap do |value|

app/models/concerns/mentionable.rb

@@ -10,7 +10,7 @@
 module Mentionable
   extend ActiveSupport::Concern
 
-  module ClassMethods
+  class_methods do
     # Indicate which attributes of the Mentionable to search for GFM references.
     def attr_mentionable(attr, options = {})
       attr = attr.to_s

app/models/concerns/participable.rb

@@ -27,7 +27,7 @@ module Participable
   extend ActiveSupport::Concern
   prepend EE::Participable
 
-  module ClassMethods
+  class_methods do
     # Adds a list of participant attributes. Attributes can either be symbols or
     # Procs.
     #

app/models/concerns/referable.rb

@@ -40,7 +40,7 @@ module Referable
     end
   end
 
-  module ClassMethods
+  class_methods do
     # The character that prefixes the actual reference identifier
     #
     # This should be overridden by the including class.

app/models/concerns/resolvable_note.rb

@@ -20,7 +20,7 @@ module ResolvableNote
     scope :unresolved, -> { resolvable.where(resolved_at: nil) }
   end
 
-  module ClassMethods
+  class_methods do
     # This method must be kept in sync with `#resolve!`
     def resolve!(current_user)
       unresolved.update_all(resolved_at: Time.now, resolved_by_id: current_user.id)

app/models/concerns/select_for_project_authorization.rb

@@ -3,7 +3,7 @@
 module SelectForProjectAuthorization
   extend ActiveSupport::Concern
 
-  module ClassMethods
+  class_methods do
     def select_for_project_authorization
       select("projects.id AS project_id, members.access_level")
     end

app/models/concerns/sha_attribute.rb

@@ -3,7 +3,7 @@
 module ShaAttribute
   extend ActiveSupport::Concern
 
-  module ClassMethods
+  class_methods do
     def sha_attribute(name)
       return if ENV['STATIC_VERIFICATION']
 

app/models/concerns/sortable.rb

@@ -19,7 +19,7 @@ module Sortable
     scope :order_name_desc, -> { reorder(Arel::Nodes::Descending.new(arel_table[:name].lower)) }
   end
 
-  module ClassMethods
+  class_methods do
     def order_by(method)
       case method.to_s
       when 'created_asc'  then order_created_asc

app/models/concerns/spammable.rb

@@ -3,7 +3,7 @@
 module Spammable
   extend ActiveSupport::Concern
 
-  module ClassMethods
+  class_methods do
     def attr_spammable(attr, options = {})
       spammable_attrs << [attr.to_s, options]
     end

app/models/concerns/strip_attribute.rb

@@ -14,7 +14,7 @@
 module StripAttribute
   extend ActiveSupport::Concern
 
-  module ClassMethods
+  class_methods do
     def strip_attributes(*attrs)
       strip_attrs.concat(attrs)
     end

app/workers/concerns/application_worker.rb

@@ -11,7 +11,7 @@ module ApplicationWorker
     set_queue
   end
 
-  module ClassMethods
+  class_methods do
     def inherited(subclass)
       subclass.set_queue
     end

app/workers/concerns/waitable_worker.rb

@@ -3,7 +3,7 @@
 module WaitableWorker
   extend ActiveSupport::Concern
 
-  module ClassMethods
+  class_methods do
     # Schedules multiple jobs and waits for them to be completed.
     def bulk_perform_and_wait(args_list, timeout: 10)
       # Short-circuit: it's more efficient to do small numbers of jobs inline

ee/app/finders/ee/issues_finder.rb

@@ -4,7 +4,7 @@ module EE
     extend ::Gitlab::Utils::Override
     include ::Gitlab::Utils::StrongMemoize
 
-    module ClassMethods
+    class_methods do
       extend ::Gitlab::Utils::Override
 
       override :scalar_params

ee/app/models/concerns/custom_model_naming.rb

@@ -7,7 +7,7 @@ module CustomModelNaming
     self.class_attribute :singular_route_key, :route_key, :param_key
   end
 
-  module ClassMethods
+  class_methods do
     def model_name
       @_model_name ||= begin
         namespace = self.parents.detect do |n|

ee/app/models/concerns/elastic/application_search.rb

@@ -85,7 +85,7 @@ module Elastic
       end
     end
 
-    module ClassMethods
+    class_methods do
       # Should be overridden for all nested models
       def nested?
         false

ee/app/models/ee/application_setting.rb

@@ -87,7 +87,7 @@ module EE
                      encode: true
     end
 
-    module ClassMethods
+    class_methods do
       extend ::Gitlab::Utils::Override
 
       override :defaults

ee/app/models/ee/broadcast_message.rb

@@ -6,7 +6,7 @@ module EE
   module BroadcastMessage
     extend ActiveSupport::Concern
 
-    module ClassMethods
+    class_methods do
       extend ::Gitlab::Utils::Override
 
       override :cache_expires_in

ee/app/models/ee/epic.rb

@@ -31,7 +31,7 @@ module EE
       end
     end
 
-    module ClassMethods
+    class_methods do
       # We support internal references (&epic_id) and cross-references (group.full_path&epic_id)
       #
       # Escaped versions with `&` will be extracted too

ee/app/models/ee/identity.rb

@@ -12,7 +12,7 @@ module EE
       end
     end
 
-    module ClassMethods
+    class_methods do
       def uniqueness_scope
         [*super, :saml_provider_id]
       end

ee/app/models/ee/issue.rb

@@ -70,7 +70,7 @@ module EE
       project&.feature_available?(:issue_weights)
     end
 
-    module ClassMethods
+    class_methods do
       # override
       def sort_by_attribute(method, excluded_labels: [])
         case method.to_s

ee/app/models/ee/namespace.rb

@@ -41,7 +41,7 @@ module EE
       before_create :sync_membership_lock_with_parent
     end
 
-    module ClassMethods
+    class_methods do
       def plans_with_feature(feature)
         LICENSE_PLANS_TO_NAMESPACE_PLANS.values_at(*License.plans_with_feature(feature))
       end

ee/app/models/ee/project.rb

@@ -86,7 +86,7 @@ module EE
       default_value_for :packages_enabled, true
     end
 
-    module ClassMethods
+    class_methods do
       def search_by_visibility(level)
         where(visibility_level: ::Gitlab::VisibilityLevel.string_options[level])
       end

ee/app/models/ee/project_authorization.rb

@@ -2,7 +2,7 @@ module EE
   module ProjectAuthorization
     extend ActiveSupport::Concern
 
-    module ClassMethods
+    class_methods do
       # Get amout of users with highest role they have.
       # If John is developer in one project but maintainer in another he will be
       # counted once as maintainer. This is needed to count users who don't use

ee/app/models/ee/service.rb

@@ -2,7 +2,7 @@ module EE
   module Service
     extend ActiveSupport::Concern
 
-    module ClassMethods
+    class_methods do
       extend ::Gitlab::Utils::Override
 
       override :available_services_names

ee/app/models/ee/user.rb

@@ -45,7 +45,7 @@ module EE
       enum roadmap_layout: { weeks: 1, months: 4, quarters: 12 }
     end
 
-    module ClassMethods
+    class_methods do
       def support_bot
         email_pattern = "support%s@#{Settings.gitlab.host}"
 

ee/changelogs/unreleased/ce-port-50414-rubocop-rule-to-enforce-class-methods-over-module.yml

+---
+title: Adds Rubocop rule to enforce class_methods over module ClassMethods
+merge_request: 7044
+author: Jacopo Beschi @jacopo-beschi
+type: added

ee/lib/ee/banzai/filter/epic_reference_filter.rb

@@ -8,7 +8,7 @@ module EE
       module EpicReferenceFilter
         extend ActiveSupport::Concern
 
-        module ClassMethods
+        class_methods do
           def references_in(text, pattern = object_class.reference_pattern)
             text.gsub(pattern) do |match|
               symbol = $~[object_sym]

ee/lib/elasticsearch/git/repository.rb

@@ -400,7 +400,7 @@ module Elasticsearch
         end
       end
 
-      module ClassMethods
+      class_methods do
         def search(query, type: :all, page: 1, per: 20, options: {})
           results = { blobs: [], commits: [] }
 

lib/api/api_guard.rb

@@ -85,7 +85,7 @@ module API
       end
     end
 
-    module ClassMethods
+    class_methods do
       private
 
       def install_error_responders(base)

lib/api/projects_relation_builder.rb

@@ -2,7 +2,7 @@ module API
   module ProjectsRelationBuilder
     extend ActiveSupport::Concern
 
-    module ClassMethods
+    class_methods do
       def prepare_relation(projects_relation, options = {})
         projects_relation = preload_relation(projects_relation, options)
         execute_batch_counting(projects_relation)

lib/gitlab/github_import/representation/expose_attribute.rb

@@ -6,7 +6,7 @@ module Gitlab
       module ExposeAttribute
         extend ActiveSupport::Concern
 
-        module ClassMethods
+        class_methods do
           # Defines getter methods for the given attribute names.
           #
           # Example:

lib/gitlab/graphql/mount_mutation.rb

@@ -5,7 +5,7 @@ module Gitlab
     module MountMutation
       extend ActiveSupport::Concern
 
-      module ClassMethods
+      class_methods do
         def mount_mutation(mutation_class)
           # Using an underscored field name symbol will make `graphql-ruby`
           # standardize the field name

lib/static_model.rb

@@ -2,7 +2,7 @@
 module StaticModel
   extend ActiveSupport::Concern
 
-  module ClassMethods
+  class_methods do
     # Used by ActiveRecord's polymorphic association to set object_id
     def primary_key
       'id'

rubocop/cop/prefer_class_methods_over_module.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    # Enforces the use of 'class_methods' instead of 'module ClassMethods' for activesupport concerns.
+    # For more information see: https://gitlab.com/gitlab-org/gitlab-ce/issues/50414
+    #
+    # @example
+    #   # bad
+    #   module Foo
+    #     extend ActiveSupport::Concern
+    #
+    #     module ClassMethods
+    #       def a_class_method
+    #       end
+    #     end
+    #   end
+    #
+    #   # good
+    #   module Foo
+    #     extend ActiveSupport::Concern
+    #
+    #     class_methods do
+    #       def a_class_method
+    #       end
+    #     end
+    #   end
+    #
+    class PreferClassMethodsOverModule < RuboCop::Cop::Cop
+      include RangeHelp
+
+      MSG = 'Do not use module ClassMethods, use class_methods block instead.'
+
+      def_node_matcher :extend_activesupport_concern?, <<~PATTERN
+        (:send nil? :extend (:const (:const nil? :ActiveSupport) :Concern))
+      PATTERN
+
+      def on_module(node)
+        add_offense(node) if node.defined_module_name == 'ClassMethods' && module_extends_activesupport_concern?(node)
+      end
+
+      def autocorrect(node)
+        lambda do |corrector|
+          corrector.replace(module_range(node), 'class_methods do')
+        end
+      end
+
+      private
+
+      def module_extends_activesupport_concern?(node)
+        container_module = container_module_of(node)
+        return false unless container_module
+
+        container_module.descendants.any? do |descendant|
+          extend_activesupport_concern?(descendant)
+        end
+      end
+
+      def container_module_of(node)
+        while node = node.parent
+          break if node.type == :module
+        end
+
+        node
+      end
+
+      def module_range(node)
+        module_node, _ = *node
+        range_between(node.loc.keyword.begin_pos, module_node.source_range.end_pos)
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -7,6 +7,7 @@ require_relative 'cop/include_sidekiq_worker'
 require_relative 'cop/avoid_return_from_blocks'
 require_relative 'cop/avoid_break_from_strong_memoize'
 require_relative 'cop/line_break_around_conditional_block'
+require_relative 'cop/prefer_class_methods_over_module'
 require_relative 'cop/migration/add_column'
 require_relative 'cop/migration/add_concurrent_foreign_key'
 require_relative 'cop/migration/add_concurrent_index'

spec/rubocop/cop/prefer_class_methods_over_module_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../rubocop/cop/prefer_class_methods_over_module'
+
+describe RuboCop::Cop::PreferClassMethodsOverModule do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  it 'flags violation when using module ClassMethods' do
+    expect_offense(<<~RUBY)
+      module Foo
+        extend ActiveSupport::Concern
+
+        module ClassMethods
+        ^^^^^^^^^^^^^^^^^^^ Do not use module ClassMethods, use class_methods block instead.
+          def a_class_method
+          end
+        end
+      end
+    RUBY
+  end
+
+  it "doesn't flag violation when using class_methods" do
+    expect_no_offenses(<<~RUBY)
+      module Foo
+        extend ActiveSupport::Concern
+
+        class_methods do
+          def a_class_method
+          end
+        end
+      end
+    RUBY
+  end
+
+  it "doesn't flag violation when module is not extending ActiveSupport::Concern" do
+    expect_no_offenses(<<~RUBY)
+      module Foo
+        module ClassMethods
+          def a_class_method
+          end
+        end
+      end
+    RUBY
+  end
+
+  it "doesn't flag violation when ClassMethods is used inside a class" do
+    expect_no_offenses(<<~RUBY)
+      class Foo
+        module ClassMethods
+          def a_class_method
+          end
+        end
+      end
+    RUBY
+  end
+
+  it "doesn't flag violation when not using either class_methods or ClassMethods" do
+    expect_no_offenses(<<~RUBY)
+      module Foo
+        extend ActiveSupport::Concern
+
+        def a_method
+        end
+      end
+    RUBY
+  end
+
+  it 'autocorrects ClassMethods into class_methods' do
+    source = <<~RUBY
+      module Foo
+        extend ActiveSupport::Concern
+
+        module ClassMethods
+          def a_class_method
+          end
+        end
+      end
+    RUBY
+    autocorrected = autocorrect_source(source)
+
+    expected_source = <<~RUBY
+      module Foo
+        extend ActiveSupport::Concern
+
+        class_methods do
+          def a_class_method
+          end
+        end
+      end
+    RUBY
+    expect(autocorrected).to eq(expected_source)
+  end
+end