Merge branch 'controller-feature-categories-b-f' into 'master'

Controller feature categories B-F

See merge request gitlab-org/gitlab!43873

app/controllers/boards/issues_controller.rb

@@ -21,6 +21,8 @@ module Boards
     before_action :validate_id_list, only: [:bulk_move]
     before_action :can_move_issues?, only: [:bulk_move]
 
+    feature_category :boards
+
     def index
       list_service = Boards::Issues::ListService.new(board_parent, current_user, filter_params)
       issues = issues_from(list_service)

app/controllers/boards/lists_controller.rb

@@ -8,6 +8,8 @@ module Boards
     before_action :authorize_read_list, only: [:index]
     skip_before_action :authenticate_user!, only: [:index]
 
+    feature_category :boards
+
     def index
       lists = Boards::Lists::ListService.new(board.resource_parent, current_user).execute(board)
 

app/controllers/concerns/controller_with_feature_category.rb

@@ -5,35 +5,38 @@ module ControllerWithFeatureCategory
   include Gitlab::ClassAttributes
 
   class_methods do
-    def feature_category(category, config = {})
-      validate_config!(config)
+    def feature_category(category, actions = [])
+      feature_category_configuration[category] ||= []
+      feature_category_configuration[category] += actions.map(&:to_s)
 
-      category_config = Config.new(category, config[:only], config[:except], config[:if], config[:unless])
-      # Add the config to the beginning. That way, the last defined one takes precedence.
-      feature_category_configuration.unshift(category_config)
+      validate_config!(feature_category_configuration)
     end
 
     def feature_category_for_action(action)
-      category_config = feature_category_configuration.find { |config| config.matches?(action) }
+      category_config = feature_category_configuration.find do |_, actions|
+        actions.empty? || actions.include?(action)
+      end
 
-      category_config&.category || superclass_feature_category_for_action(action)
+      category_config&.first || superclass_feature_category_for_action(action)
     end
 
     private
 
     def validate_config!(config)
-      invalid_keys = config.keys - [:only, :except, :if, :unless]
-      if invalid_keys.any?
-        raise ArgumentError, "unknown arguments: #{invalid_keys} "
+      empty = config.find { |_, actions| actions.empty? }
+      duplicate_actions = config.values.flatten.group_by(&:itself).select { |_, v| v.count > 1 }.keys
+
+      if config.length > 1 && empty
+        raise ArgumentError, "#{empty.first} is defined for all actions, but other categories are set"
       end
 
-      if config.key?(:only) && config.key?(:except)
-        raise ArgumentError, "cannot configure both `only` and `except`"
+      if duplicate_actions.any?
+        raise ArgumentError, "Actions have multiple feature categories: #{duplicate_actions.join(', ')}"
       end
     end
 
     def feature_category_configuration
-      class_attributes[:feature_category_config] ||= []
+      class_attributes[:feature_category_config] ||= {}
     end
 
     def superclass_feature_category_for_action(action)

app/controllers/concerns/controller_with_feature_category/config.rb

-# frozen_string_literal: true
-
-module ControllerWithFeatureCategory
-  class Config
-    attr_reader :category
-
-    def initialize(category, only, except, if_proc, unless_proc)
-      @category = category.to_sym
-      @only, @except = only&.map(&:to_s), except&.map(&:to_s)
-      @if_proc, @unless_proc = if_proc, unless_proc
-    end
-
-    def matches?(action)
-      included?(action) && !excluded?(action) &&
-        if_proc?(action) && !unless_proc?(action)
-    end
-
-    private
-
-    attr_reader :only, :except, :if_proc, :unless_proc
-
-    def if_proc?(action)
-      if_proc.nil? || if_proc.call(action)
-    end
-
-    def unless_proc?(action)
-      unless_proc.present? && unless_proc.call(action)
-    end
-
-    def included?(action)
-      only.nil? || only.include?(action)
-    end
-
-    def excluded?(action)
-      except.present? && except.include?(action)
-    end
-  end
-end

app/controllers/confirmations_controller.rb

@@ -3,6 +3,8 @@
 class ConfirmationsController < Devise::ConfirmationsController
   include AcceptsPendingInvitations
 
+  feature_category :users
+
   def almost_there
     flash[:notice] = nil
     render layout: "devise_empty"

app/controllers/dashboard/groups_controller.rb

@@ -5,6 +5,8 @@ class Dashboard::GroupsController < Dashboard::ApplicationController
 
   skip_cross_project_access_check :index
 
+  feature_category :subgroups
+
   def index
     groups = GroupsFinder.new(current_user, all_available: false).execute
     render_group_tree(groups)

app/controllers/dashboard/labels_controller.rb

 # frozen_string_literal: true
 
 class Dashboard::LabelsController < Dashboard::ApplicationController
+  feature_category :issue_tracking
+
   def index
     respond_to do |format|
       format.json { render json: LabelSerializer.new.represent_appearance(labels) }

app/controllers/dashboard/milestones_controller.rb

@@ -4,6 +4,8 @@ class Dashboard::MilestonesController < Dashboard::ApplicationController
   before_action :projects
   before_action :groups, only: :index
 
+  feature_category :issue_tracking
+
   def index
     respond_to do |format|
       format.html do

app/controllers/dashboard/projects_controller.rb

@@ -14,6 +14,8 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController
   before_action :projects, only: [:index]
   skip_cross_project_access_check :index, :starred
 
+  feature_category :projects
+
   def index
     respond_to do |format|
       format.html do

app/controllers/dashboard/snippets_controller.rb

@@ -7,6 +7,8 @@ class Dashboard::SnippetsController < Dashboard::ApplicationController
 
   skip_cross_project_access_check :index
 
+  feature_category :snippets
+
   def index
     @snippet_counts = Snippets::CountService
       .new(current_user, author: current_user)

app/controllers/dashboard/todos_controller.rb

@@ -9,6 +9,8 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   before_action :authorize_read_group!, only: :index
   before_action :find_todos, only: [:index, :destroy_all]
 
+  feature_category :issue_tracking
+
   def index
     @sort = params[:sort]
     @todos = @todos.page(params[:page])

app/controllers/dashboard_controller.rb

@@ -15,6 +15,10 @@ class DashboardController < Dashboard::ApplicationController
 
   respond_to :html
 
+  feature_category :audit_events, [:activity]
+  feature_category :issue_tracking, [:issues, :issues_calendar]
+  feature_category :code_review, [:merge_requests]
+
   def activity
     respond_to do |format|
       format.html

app/controllers/explore/groups_controller.rb

@@ -3,6 +3,8 @@
 class Explore::GroupsController < Explore::ApplicationController
   include GroupTree
 
+  feature_category :subgroups
+
   def index
     render_group_tree GroupsFinder.new(current_user).execute
   end

app/controllers/explore/projects_controller.rb

@@ -18,6 +18,8 @@ class Explore::ProjectsController < Explore::ApplicationController
 
   rescue_from PageOutOfBoundsError, with: :page_out_of_bounds
 
+  feature_category :projects
+
   def index
     @projects = load_projects
 

app/controllers/explore/snippets_controller.rb

@@ -3,6 +3,8 @@
 class Explore::SnippetsController < Explore::ApplicationController
   include Gitlab::NoteableMetadata
 
+  feature_category :snippets
+
   def index
     @snippets = SnippetsFinder.new(current_user, explore: true)
       .execute

app/controllers/projects/merge_requests_controller.rb

@@ -50,12 +50,17 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
 
   after_action :log_merge_request_show, only: [:show]
 
-  feature_category :source_code_management,
-                   unless: -> (action) { action.ends_with?("_reports") }
-  feature_category :code_testing,
-                   only: [:test_reports, :coverage_reports, :terraform_reports]
-  feature_category :accessibility_testing,
-                   only: [:accessibility_reports]
+  feature_category :code_review, [
+                     :assign_related_issues, :bulk_update, :cancel_auto_merge,
+                     :ci_environments_status, :commit_change_content, :commits,
+                     :context_commits, :destroy, :diff_for_path, :discussions,
+                     :edit, :exposed_artifacts, :index, :merge,
+                     :pipeline_status, :pipelines, :rebase, :remove_wip, :show,
+                     :toggle_award_emoji, :toggle_subscription, :update
+                   ]
+
+  feature_category :code_testing, [:test_reports, :coverage_reports, :terraform_reports]
+  feature_category :accessibility_testing, [:accessibility_reports]
 
   def index
     @merge_requests = @issuables

doc/development/feature_categorization/index.md

@@ -75,38 +75,23 @@ A feature category can be specified on an entire controller
 using:
 
 ```ruby
-class Projects::MergeRequestsController < ApplicationController
-  feature_category :source_code_management
+class Boards::ListsController < ApplicationController
+  feature_category :kanban_boards
 end
 ```
 
 The feature category can be limited to a list of actions using the
-`only` argument, actions can be excluded using the `except` argument.
+second argument:
 
 ```ruby
-class Projects::MergeRequestsController < ApplicationController
-  feature_category :code_testing, only: [:metrics_reports]
-  feature_category :source_code_management, except: [:test_reports, :coverage_reports]
+class DashboardController < ApplicationController
+  feature_category :issue_tracking, [:issues, :issues_calendar]
+  feature_category :code_review, [:merge_requests]
 end
 ```
 
-`except` and `only` arguments can not be combined.
-
-When specifying `except` all other actions will get the specified
-category assigned.
-
-The assignment can also be scoped using `if` and `unless` procs:
-
-```ruby
-class Projects::MergeRequestsController < ApplicationController
-  feature_category :source_code_management,
-                   unless: -> (action) { action.include?("reports") }
-                   if: -> (action) { action.include?("widget") }
-end
-```
-
-In this case, both procs need to be satisfied for the action to get
-the category assigned.
+These forms cannot be mixed: if a controller has more than one category,
+every single action must be listed.
 
 ### Excluding controller actions from feature categorization
 
@@ -125,6 +110,5 @@ The `spec/controllers/every_controller_spec.rb` will iterate over all
 defined routes, and check the controller to see if a category is
 assigned to all actions.
 
-The spec also validates if the used feature categories are known. And
-if the actions used in `only` and `except` configuration still exist
-as routes.
+The spec also validates if the used feature categories are known. And if
+the actions used in configuration still exist as routes.

ee/app/controllers/boards/milestones_controller.rb

@@ -5,6 +5,7 @@ module Boards
     include BoardsResponses
 
     before_action :authorize_read_milestone, only: [:index]
+    feature_category :boards
 
     def index
       milestones_finder = Boards::MilestonesFinder.new(board, current_user)

ee/app/controllers/boards/users_controller.rb

@@ -11,6 +11,7 @@ module Boards
     include BoardsResponses
 
     before_action :authorize_read_parent, only: [:index]
+    feature_category :boards
 
     def index
       user_ids = user_finder.execute.select(:user_id)

ee/app/controllers/ee/projects/merge_requests_controller.rb

@@ -18,13 +18,14 @@ module EE
                                                         :sast_reports, :secret_detection_reports, :dast_reports, :metrics_reports, :coverage_fuzzing_reports]
         before_action :authorize_read_licenses!, only: [:license_scanning_reports]
 
-        feature_category :container_scanning, only: [:container_scanning_reports]
-        feature_category :dependency_scanning, only: [:dependency_scanning_reports]
-        feature_category :license_compliance, only: [:license_scanning_reports]
-        feature_category :static_application_security_testing, only: [:sast_reports]
-        feature_category :secret_detection, only: [:secret_detection_reports]
-        feature_category :dynamic_application_security_testing, only: [:dast_reports]
-        feature_category :metrics, only: [:metrics_reports]
+        feature_category :code_review, [:delete_description_version, :description_diff]
+        feature_category :container_scanning, [:container_scanning_reports]
+        feature_category :dependency_scanning, [:dependency_scanning_reports]
+        feature_category :license_compliance, [:license_scanning_reports]
+        feature_category :static_application_security_testing, [:sast_reports]
+        feature_category :secret_detection, [:secret_detection_reports]
+        feature_category :dynamic_application_security_testing, [:dast_reports]
+        feature_category :metrics, [:metrics_reports]
       end
 
       def license_scanning_reports

spec/controllers/concerns/controller_with_feature_category/config_spec.rb

-# frozen_string_literal: true
-
-require "fast_spec_helper"
-require "rspec-parameterized"
-require_relative "../../../../app/controllers/concerns/controller_with_feature_category/config"
-
-RSpec.describe ControllerWithFeatureCategory::Config do
-  describe "#matches?" do
-    using RSpec::Parameterized::TableSyntax
-
-    where(:only_actions, :except_actions, :if_proc, :unless_proc, :test_action, :expected) do
-      nil         | nil         | nil   | nil   | "action"   | true
-      [:included] | nil         | nil   | nil   | "action"   | false
-      [:included] | nil         | nil   | nil   | "included" | true
-      nil         | [:excluded] | nil   | nil   | "excluded" | false
-      nil         | nil         | true  | nil   | "action"   | true
-      [:included] | nil         | true  | nil   | "action"   | false
-      [:included] | nil         | true  | nil   | "included" | true
-      nil         | [:excluded] | true  | nil   | "excluded" | false
-      nil         | nil         | false | nil   | "action"   | false
-      [:included] | nil         | false | nil   | "action"   | false
-      [:included] | nil         | false | nil   | "included" | false
-      nil         | [:excluded] | false | nil   | "excluded" | false
-      nil         | nil         | nil   | true  | "action"   | false
-      [:included] | nil         | nil   | true  | "action"   | false
-      [:included] | nil         | nil   | true  | "included" | false
-      nil         | [:excluded] | nil   | true  | "excluded" | false
-      nil         | nil         | nil   | false | "action"   | true
-      [:included] | nil         | nil   | false | "action"   | false
-      [:included] | nil         | nil   | false | "included" | true
-      nil         | [:excluded] | nil   | false | "excluded" | false
-      nil         | nil         | true  | false | "action"   | true
-      [:included] | nil         | true  | false | "action"   | false
-      [:included] | nil         | true  | false | "included" | true
-      nil         | [:excluded] | true  | false | "excluded" | false
-      nil         | nil         | false | true  | "action"   | false
-      [:included] | nil         | false | true  | "action"   | false
-      [:included] | nil         | false | true  | "included" | false
-      nil         | [:excluded] | false | true  | "excluded" | false
-    end
-
-    with_them do
-      let(:config) do
-        if_to_proc = if_proc.nil? ? nil : -> (_) { if_proc }
-        unless_to_proc = unless_proc.nil? ? nil : -> (_) { unless_proc }
-
-        described_class.new(:category, only_actions, except_actions, if_to_proc, unless_to_proc)
-      end
-
-      specify { expect(config.matches?(test_action)).to be(expected) }
-    end
-  end
-end

spec/controllers/concerns/controller_with_feature_category_spec.rb

@@ -2,7 +2,6 @@
 
 require 'fast_spec_helper'
 require_relative "../../../app/controllers/concerns/controller_with_feature_category"
-require_relative "../../../app/controllers/concerns/controller_with_feature_category/config"
 
 RSpec.describe ControllerWithFeatureCategory do
   describe ".feature_category_for_action" do
@@ -14,17 +13,15 @@ RSpec.describe ControllerWithFeatureCategory do
 
     let(:controller) do
       Class.new(base_controller) do
-        feature_category :baz
-        feature_category :foo, except: %w(update edit)
-        feature_category :bar, only: %w(index show)
-        feature_category :quux, only: %w(destroy)
-        feature_category :quuz, only: %w(destroy)
+        feature_category :foo, %w(update edit)
+        feature_category :bar, %w(index show)
+        feature_category :quux, %w(destroy)
       end
     end
 
     let(:subclass) do
       Class.new(controller) do
-        feature_category :qux, only: %w(index)
+        feature_category :baz, %w(subclass_index)
       end
     end
 
@@ -33,34 +30,31 @@ RSpec.describe ControllerWithFeatureCategory do
     end
 
     it "returns the expected category", :aggregate_failures do
-      expect(controller.feature_category_for_action("update")).to eq(:baz)
-      expect(controller.feature_category_for_action("hello")).to eq(:foo)
+      expect(controller.feature_category_for_action("update")).to eq(:foo)
       expect(controller.feature_category_for_action("index")).to eq(:bar)
+      expect(controller.feature_category_for_action("destroy")).to eq(:quux)
     end
 
-    it "returns the closest match for categories defined in subclasses" do
-      expect(subclass.feature_category_for_action("index")).to eq(:qux)
-      expect(subclass.feature_category_for_action("show")).to eq(:bar)
+    it "returns the expected category for categories defined in subclasses" do
+      expect(subclass.feature_category_for_action("subclass_index")).to eq(:baz)
     end
 
-    it "returns the last defined feature category when multiple match" do
-      expect(controller.feature_category_for_action("destroy")).to eq(:quuz)
-    end
-
-    it "raises an error when using including and excluding the same action" do
+    it "raises an error when defining for the controller and for individual actions" do
       expect do
         Class.new(base_controller) do
-          feature_category :hello, only: [:world], except: [:world]
+          feature_category :hello
+          feature_category :goodbye, [:world]
         end
-      end.to raise_error(%r(cannot configure both `only` and `except`))
+      end.to raise_error(ArgumentError, "hello is defined for all actions, but other categories are set")
     end
 
-    it "raises an error when using unknown arguments" do
+    it "raises an error when multiple calls define the same action" do
       expect do
         Class.new(base_controller) do
-          feature_category :hello, hello: :world
+          feature_category :hello, [:world]
+          feature_category :goodbye, ["world"]
         end
-      end.to raise_error(%r(unknown arguments))
+      end.to raise_error(ArgumentError, "Actions have multiple feature categories: world")
     end
   end
 end

spec/controllers/every_controller_spec.rb

@@ -17,20 +17,27 @@ RSpec.describe "Every controller" do
         .compact
         .select { |route| route[:controller].present? && route[:action].present? }
         .map { |route| [constantize_controller(route[:controller]), route[:action]] }
-        .reject { |route| route.first.nil? || !route.first.include?(ControllerWithFeatureCategory) }
+        .select { |(controller, action)| controller&.include?(ControllerWithFeatureCategory) }
+        .reject { |(controller, action)| controller == Devise::UnlocksController }
     end
 
     let_it_be(:routes_without_category) do
       controller_actions.map do |controller, action|
-        "#{controller}##{action}" unless controller.feature_category_for_action(action)
+        next if controller.feature_category_for_action(action)
+        next unless controller.to_s.start_with?('B', 'C', 'D', 'E', 'F', 'Projects::MergeRequestsController')
+
+        "#{controller}##{action}"
       end.compact
     end
 
     it "has feature categories" do
-      pending("We'll work on defining categories for all controllers: "\
-              "https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/463")
+      routes_without_category.map { |x| x.split('#') }.group_by(&:first).each do |controller, actions|
+        puts controller
+        puts actions.map { |x| ":#{x.last}" }.sort.join(', ')
+        puts ''
+      end
 
-      expect(routes_without_category).to be_empty, "#{routes_without_category.first(10)} did not have a category"
+      expect(routes_without_category).to be_empty, "#{routes_without_category} did not have a category"
     end
 
     it "completed controllers don't get new routes without categories" do
@@ -74,9 +81,9 @@ RSpec.describe "Every controller" do
   end
 
   def actions_defined_in_feature_category_config(controller)
-    feature_category_configs = controller.send(:class_attributes)[:feature_category_config]
-    feature_category_configs.map do |config|
-      Array(config.send(:only)) + Array(config.send(:except))
-    end.flatten.uniq.map(&:to_s)
+    controller.send(:class_attributes)[:feature_category_config]
+      .values
+      .flatten
+      .map(&:to_s)
   end
 end