Merge branch '241990-show-inherited-labels' into 'master'

Show inherited labels in a subgroup and project labels page

Closes #241990

See merge request gitlab-org/gitlab!42960

app/controllers/concerns/show_inherited_labels_checker.rb

+# frozen_string_literal: true
+
+module ShowInheritedLabelsChecker
+  extend ActiveSupport::Concern
+
+  private
+
+  def show_inherited_labels?(include_ancestor_groups)
+    Feature.enabled?(:show_inherited_labels, @project || @group) || include_ancestor_groups # rubocop:disable Gitlab/ModuleWithInstanceVariables
+  end
+end

app/controllers/groups/labels_controller.rb

@@ -2,6 +2,7 @@
 
 class Groups::LabelsController < Groups::ApplicationController
   include ToggleSubscriptionAction
+  include ShowInheritedLabelsChecker
 
   before_action :label, only: [:edit, :update, :destroy]
   before_action :authorize_admin_labels!, only: [:new, :create, :edit, :update, :destroy]
@@ -12,8 +13,9 @@ class Groups::LabelsController < Groups::ApplicationController
   def index
     respond_to do |format|
       format.html do
-        @labels = GroupLabelsFinder
-          .new(current_user, @group, params.merge(sort: sort)).execute
+        # at group level we do not want to list project labels,
+        # we only want `only_group_labels = false` when pulling labels for label filter dropdowns, fetched through json
+        @labels = available_labels(params.merge(only_group_labels: true)).page(params[:page])
       end
       format.json do
         render json: LabelSerializer.new.represent_appearance(available_labels)
@@ -74,7 +76,7 @@ class Groups::LabelsController < Groups::ApplicationController
   end
 
   def label
-    @label ||= @group.labels.find(params[:id])
+    @label ||= available_labels(params.merge(only_group_labels: true)).find(params[:id])
   end
   alias_method :subscribable_resource, :label
 
@@ -102,15 +104,17 @@ class Groups::LabelsController < Groups::ApplicationController
     session[:previous_labels_path] = URI(request.referer || '').path
   end
 
-  def available_labels
+  def available_labels(options = params)
     @available_labels ||=
       LabelsFinder.new(
         current_user,
         group_id: @group.id,
-        only_group_labels: params[:only_group_labels],
-        include_ancestor_groups: params[:include_ancestor_groups],
-        include_descendant_groups: params[:include_descendant_groups],
-        search: params[:search]).execute
+        only_group_labels: options[:only_group_labels],
+        include_ancestor_groups: show_inherited_labels?(params[:include_ancestor_groups]),
+        sort: sort,
+        subscribed: options[:subscribed],
+        include_descendant_groups: options[:include_descendant_groups],
+        search: options[:search]).execute
   end
 
   def sort

app/controllers/projects/labels_controller.rb

@@ -2,6 +2,7 @@
 
 class Projects::LabelsController < Projects::ApplicationController
   include ToggleSubscriptionAction
+  include ShowInheritedLabelsChecker
 
   before_action :check_issuables_available!
   before_action :label, only: [:edit, :update, :destroy, :promote]
@@ -161,7 +162,7 @@ class Projects::LabelsController < Projects::ApplicationController
     @available_labels ||=
       LabelsFinder.new(current_user,
                        project_id: @project.id,
-                       include_ancestor_groups: params[:include_ancestor_groups],
+                       include_ancestor_groups: show_inherited_labels?(params[:include_ancestor_groups]),
                        search: params[:search],
                        subscribed: params[:subscribed],
                        sort: sort).execute

app/finders/group_labels_finder.rb

-# frozen_string_literal: true
-
-class GroupLabelsFinder
-  attr_reader :current_user, :group, :params
-
-  def initialize(current_user, group, params = {})
-    @current_user = current_user
-    @group = group
-    @params = params
-  end
-
-  def execute
-    group.labels
-      .optionally_subscribed_by(subscriber_id)
-      .optionally_search(params[:search])
-      .order_by(params[:sort])
-      .page(params[:page])
-  end
-
-  private
-
-  def subscriber_id
-    current_user&.id if subscribed?
-  end
-
-  def subscribed?
-    params[:subscribed] == 'true'
-  end
-end

config/feature_flags/development/show_inherited_labels.yml

+---
+name: show_inherited_labels
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/42960
+rollout_issue_url:
+group: group::project management
+type: development
+default_enabled: false

spec/controllers/groups/labels_controller_spec.rb

@@ -9,6 +9,8 @@ RSpec.describe Groups::LabelsController do
 
   before do
     group.add_owner(user)
+    # by default FFs are enabled in specs so we turn it off
+    stub_feature_flags(show_inherited_labels: false)
 
     sign_in(user)
   end
@@ -32,11 +34,41 @@ RSpec.describe Groups::LabelsController do
         subgroup.add_owner(user)
       end
 
-      it 'returns ancestor group labels' do
-        get :index, params: { group_id: subgroup, include_ancestor_groups: true, only_group_labels: true }, format: :json
+      RSpec.shared_examples 'returns ancestor group labels' do
+        it 'returns ancestor group labels' do
+          get :index, params: params, format: :json
 
-        label_ids = json_response.map {|label| label['title']}
-        expect(label_ids).to match_array([group_label_1.title, subgroup_label_1.title])
+          label_ids = json_response.map {|label| label['title']}
+          expect(label_ids).to match_array([group_label_1.title, subgroup_label_1.title])
+        end
+      end
+
+      context 'when include_ancestor_groups true' do
+        let(:params) { { group_id: subgroup, include_ancestor_groups: true, only_group_labels: true } }
+
+        it_behaves_like 'returns ancestor group labels'
+      end
+
+      context 'when include_ancestor_groups false' do
+        let(:params) { { group_id: subgroup, only_group_labels: true } }
+
+        it 'does not return ancestor group labels', :aggregate_failures do
+          get :index, params: params, format: :json
+
+          label_ids = json_response.map {|label| label['title']}
+          expect(label_ids).to match_array([subgroup_label_1.title])
+          expect(label_ids).not_to include([group_label_1.title])
+        end
+      end
+
+      context 'when show_inherited_labels enabled' do
+        let(:params) { { group_id: subgroup } }
+
+        before do
+          stub_feature_flags(show_inherited_labels: true)
+        end
+
+        it_behaves_like 'returns ancestor group labels'
       end
     end
 

spec/controllers/projects/labels_controller_spec.rb

@@ -3,9 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe Projects::LabelsController do
-  let(:group)   { create(:group) }
-  let(:project) { create(:project, namespace: group) }
-  let(:user)    { create(:user) }
+  let_it_be(:group)   { create(:group) }
+  let_it_be(:project, reload: true) { create(:project, namespace: group) }
+  let_it_be(:user)    { create(:user) }
 
   before do
     project.add_maintainer(user)
@@ -14,16 +14,21 @@ RSpec.describe Projects::LabelsController do
   end
 
   describe 'GET #index' do
-    let!(:label_1) { create(:label, project: project, priority: 1, title: 'Label 1') }
-    let!(:label_2) { create(:label, project: project, priority: 3, title: 'Label 2') }
-    let!(:label_3) { create(:label, project: project, priority: 1, title: 'Label 3') }
-    let!(:label_4) { create(:label, project: project, title: 'Label 4') }
-    let!(:label_5) { create(:label, project: project, title: 'Label 5') }
-
-    let!(:group_label_1) { create(:group_label, group: group, title: 'Group Label 1') }
-    let!(:group_label_2) { create(:group_label, group: group, title: 'Group Label 2') }
-    let!(:group_label_3) { create(:group_label, group: group, title: 'Group Label 3') }
-    let!(:group_label_4) { create(:group_label, group: group, title: 'Group Label 4') }
+    let_it_be(:label_1) { create(:label, project: project, priority: 1, title: 'Label 1') }
+    let_it_be(:label_2) { create(:label, project: project, priority: 3, title: 'Label 2') }
+    let_it_be(:label_3) { create(:label, project: project, priority: 1, title: 'Label 3') }
+    let_it_be(:label_4) { create(:label, project: project, title: 'Label 4') }
+    let_it_be(:label_5) { create(:label, project: project, title: 'Label 5') }
+
+    let_it_be(:group_label_1) { create(:group_label, group: group, title: 'Group Label 1') }
+    let_it_be(:group_label_2) { create(:group_label, group: group, title: 'Group Label 2') }
+    let_it_be(:group_label_3) { create(:group_label, group: group, title: 'Group Label 3') }
+    let_it_be(:group_label_4) { create(:group_label, group: group, title: 'Group Label 4') }
+
+    let_it_be(:group_labels) { [group_label_3, group_label_4]}
+    let_it_be(:project_labels) { [label_4, label_5]}
+    let_it_be(:group_priority_labels) { [group_label_1, group_label_2]}
+    let_it_be(:project_priority_labels) { [label_1, label_2, label_3]}
 
     before do
       create(:label_priority, project: project, label: group_label_1, priority: 3)
@@ -68,6 +73,60 @@ RSpec.describe Projects::LabelsController do
       end
     end
 
+    context 'with subgroups' do
+      let_it_be(:subgroup) { create(:group, parent: group) }
+      let_it_be(:subgroup_label_1) { create(:group_label, group: subgroup, title: 'subgroup_label_1') }
+      let_it_be(:subgroup_label_2) { create(:group_label, group: subgroup, title: 'subgroup_label_2') }
+
+      before do
+        project.update!(namespace: subgroup)
+        subgroup.add_owner(user)
+        create(:label_priority, project: project, label: subgroup_label_2, priority: 1)
+      end
+
+      RSpec.shared_examples 'returns ancestor group labels' do
+        it 'returns ancestor group labels', :aggregate_failures do
+          get :index, params: params
+
+          expect(assigns(:labels)).to match_array([subgroup_label_1] + group_labels + project_labels)
+          expect(assigns(:prioritized_labels)).to match_array([subgroup_label_2] + group_priority_labels + project_priority_labels)
+        end
+      end
+
+      context 'when show_inherited_labels disabled' do
+        before do
+          stub_feature_flags(show_inherited_labels: false)
+        end
+
+        context 'when include_ancestor_groups false' do
+          let(:params) { { namespace_id: project.namespace.to_param, project_id: project } }
+
+          it 'does not return ancestor group labels', :aggregate_failures do
+            get :index, params: params
+
+            expect(assigns(:labels)).to match_array([subgroup_label_1] + project_labels)
+            expect(assigns(:prioritized_labels)).to match_array([subgroup_label_2] + project_priority_labels)
+          end
+        end
+
+        context 'when include_ancestor_groups true' do
+          let(:params) { { namespace_id: project.namespace.to_param, project_id: project, include_ancestor_groups: true } }
+
+          it_behaves_like 'returns ancestor group labels'
+        end
+      end
+
+      context 'when show_inherited_labels enabled' do
+        let(:params) { { namespace_id: project.namespace.to_param, project_id: project } }
+
+        before do
+          stub_feature_flags(show_inherited_labels: true)
+        end
+
+        it_behaves_like 'returns ancestor group labels'
+      end
+    end
+
     def list_labels
       get :index, params: { namespace_id: project.namespace.to_param, project_id: project }
     end
@@ -75,7 +134,7 @@ RSpec.describe Projects::LabelsController do
 
   describe 'POST #generate' do
     context 'personal project' do
-      let(:personal_project) { create(:project, namespace: user.namespace) }
+      let_it_be(:personal_project) { create(:project, namespace: user.namespace) }
 
       it 'creates labels' do
         post :generate, params: { namespace_id: personal_project.namespace.to_param, project_id: personal_project }
@@ -116,8 +175,8 @@ RSpec.describe Projects::LabelsController do
   end
 
   describe 'POST #promote' do
-    let!(:promoted_label_name) { "Promoted Label" }
-    let!(:label_1) { create(:label, title: promoted_label_name, project: project) }
+    let_it_be(:promoted_label_name) { "Promoted Label" }
+    let_it_be(:label_1) { create(:label, title: promoted_label_name, project: project) }
 
     context 'not group reporters' do
       it 'denies access' do
@@ -196,7 +255,7 @@ RSpec.describe Projects::LabelsController do
       end
 
       context 'when requesting a redirected path' do
-        let!(:redirect_route) { project.redirect_routes.create(path: project.full_path + 'old') }
+        let_it_be(:redirect_route) { project.redirect_routes.create(path: project.full_path + 'old') }
 
         it 'redirects to the canonical path' do
           get :index, params: { namespace_id: project.namespace, project_id: project.to_param + 'old' }
@@ -242,7 +301,7 @@ RSpec.describe Projects::LabelsController do
     end
 
     context 'when requesting a redirected path' do
-      let!(:redirect_route) { project.redirect_routes.create(path: project.full_path + 'old') }
+      let_it_be(:redirect_route) { project.redirect_routes.create(path: project.full_path + 'old') }
 
       it 'returns not found' do
         post :generate, params: { namespace_id: project.namespace, project_id: project.to_param + 'old' }

spec/finders/group_labels_finder_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe GroupLabelsFinder, '#execute' do
-  let!(:group) { create(:group) }
-  let!(:user)  { create(:user) }
-  let!(:label1) { create(:group_label, title: 'Foo', description: 'Lorem ipsum', group: group) }
-  let!(:label2) { create(:group_label, title: 'Bar', description: 'Fusce consequat', group: group) }
-
-  it 'returns all group labels sorted by name if no params' do
-    result = described_class.new(user, group).execute
-
-    expect(result.to_a).to match_array([label2, label1])
-  end
-
-  it 'returns all group labels sorted by name desc' do
-    result = described_class.new(user, group, sort: 'name_desc').execute
-
-    expect(result.to_a).to match_array([label2, label1])
-  end
-
-  it 'returns group labels that match search' do
-    result = described_class.new(user, group, search: 'Foo').execute
-
-    expect(result.to_a).to match_array([label1])
-  end
-
-  it 'returns group labels user subscribed to' do
-    label2.subscribe(user)
-
-    result = described_class.new(user, group, subscribed: 'true').execute
-
-    expect(result.to_a).to match_array([label2])
-  end
-
-  it 'returns second page of labels' do
-    result = described_class.new(user, group, page: '2').execute
-
-    expect(result.to_a).to match_array([])
-  end
-end