summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/finders/labels_finder.rb15
-rw-r--r--app/services/issuable_base_service.rb14
-rw-r--r--app/services/labels/find_or_create_service.rb22
-rw-r--r--spec/finders/labels_finder_spec.rb23
-rw-r--r--spec/services/labels/find_or_create_service_spec.rb78
5 files changed, 116 insertions, 36 deletions
diff --git a/app/finders/labels_finder.rb b/app/finders/labels_finder.rb
index f013e177c5b..5c9fce211ec 100644
--- a/app/finders/labels_finder.rb
+++ b/app/finders/labels_finder.rb
@@ -39,7 +39,7 @@ class LabelsFinder < UnionFinder
end
end
elsif only_group_labels?
- label_ids << Label.where(group_id: group.id)
+ label_ids << Label.where(group_id: group_ids)
else
label_ids << Label.where(group_id: projects.group_ids)
label_ids << Label.where(project_id: projects.select(:id))
@@ -59,10 +59,11 @@ class LabelsFinder < UnionFinder
items.where(title: title)
end
- def group
- strong_memoize(:group) do
+ def group_ids
+ strong_memoize(:group_ids) do
group = Group.find(params[:group_id])
- authorized_to_read_labels?(group) && group
+ groups = params[:include_ancestor_groups].present? ? group.self_and_ancestors : [group]
+ groups_user_can_read_labels(groups).map(&:id)
end
end
@@ -120,4 +121,10 @@ class LabelsFinder < UnionFinder
Ability.allowed?(current_user, :read_label, label_parent)
end
+
+ def groups_user_can_read_labels(groups)
+ DeclarativePolicy.user_scope do
+ groups.select { |group| authorized_to_read_labels?(group) }
+ end
+ end
end
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 66a9b1f82e0..e87fd49d193 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -77,8 +77,12 @@ class IssuableBaseService < BaseService
return unless labels
params[:label_ids] = labels.split(",").map do |label_name|
- service = Labels::FindOrCreateService.new(current_user, project, title: label_name.strip)
- label = service.execute
+ label = Labels::FindOrCreateService.new(
+ current_user,
+ parent,
+ title: label_name.strip,
+ available_labels: available_labels
+ ).execute
label.try(:id)
end.compact
@@ -102,7 +106,7 @@ class IssuableBaseService < BaseService
end
def available_labels
- LabelsFinder.new(current_user, project_id: @project.id).execute
+ @available_labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute
end
def merge_quick_actions_into_params!(issuable)
@@ -303,4 +307,8 @@ class IssuableBaseService < BaseService
def update_project_counter_caches?(issuable)
issuable.state_changed?
end
+
+ def parent
+ project
+ end
end
diff --git a/app/services/labels/find_or_create_service.rb b/app/services/labels/find_or_create_service.rb
index 940c8b333d3..079f611b3f3 100644
--- a/app/services/labels/find_or_create_service.rb
+++ b/app/services/labels/find_or_create_service.rb
@@ -1,8 +1,9 @@
module Labels
class FindOrCreateService
- def initialize(current_user, project, params = {})
+ def initialize(current_user, parent, params = {})
@current_user = current_user
- @project = project
+ @parent = parent
+ @available_labels = params.delete(:available_labels)
@params = params.dup.with_indifferent_access
end
@@ -13,12 +14,13 @@ module Labels
private
- attr_reader :current_user, :project, :params, :skip_authorization
+ attr_reader :current_user, :parent, :params, :skip_authorization
def available_labels
@available_labels ||= LabelsFinder.new(
current_user,
- project_id: project.id
+ "#{parent_type}_id".to_sym => parent.id,
+ only_group_labels: parent_is_group?
).execute(skip_authorization: skip_authorization)
end
@@ -27,8 +29,8 @@ module Labels
def find_or_create_label
new_label = available_labels.find_by(title: title)
- if new_label.nil? && (skip_authorization || Ability.allowed?(current_user, :admin_label, project))
- new_label = Labels::CreateService.new(params).execute(project: project)
+ if new_label.nil? && (skip_authorization || Ability.allowed?(current_user, :admin_label, parent))
+ new_label = Labels::CreateService.new(params).execute(parent_type.to_sym => parent)
end
new_label
@@ -37,5 +39,13 @@ module Labels
def title
params[:title] || params[:name]
end
+
+ def parent_type
+ parent.model_name.param_key
+ end
+
+ def parent_is_group?
+ parent_type == "group"
+ end
end
end
diff --git a/spec/finders/labels_finder_spec.rb b/spec/finders/labels_finder_spec.rb
index 06031aee217..dc76efea35b 100644
--- a/spec/finders/labels_finder_spec.rb
+++ b/spec/finders/labels_finder_spec.rb
@@ -5,6 +5,8 @@ describe LabelsFinder do
let(:group_1) { create(:group) }
let(:group_2) { create(:group) }
let(:group_3) { create(:group) }
+ let(:private_group_1) { create(:group, :private) }
+ let(:private_subgroup_1) { create(:group, :private, parent: private_group_1) }
let(:project_1) { create(:project, namespace: group_1) }
let(:project_2) { create(:project, namespace: group_2) }
@@ -20,6 +22,8 @@ describe LabelsFinder do
let!(:group_label_1) { create(:group_label, group: group_1, title: 'Label 1 (group)') }
let!(:group_label_2) { create(:group_label, group: group_1, title: 'Group Label 2') }
let!(:group_label_3) { create(:group_label, group: group_2, title: 'Group Label 3') }
+ let!(:private_group_label_1) { create(:group_label, group: private_group_1, title: 'Private Group Label 1') }
+ let!(:private_subgroup_label_1) { create(:group_label, group: private_subgroup_1, title: 'Private Sub Group Label 1') }
let(:user) { create(:user) }
@@ -66,6 +70,25 @@ describe LabelsFinder do
expect(finder.execute).to eq [group_label_2, group_label_1]
end
end
+
+ context 'when including labels from group ancestors', :nested_groups do
+ it 'returns labels from group and its ancestors' do
+ private_group_1.add_developer(user)
+ private_subgroup_1.add_developer(user)
+
+ finder = described_class.new(user, group_id: private_subgroup_1.id, only_group_labels: true, include_ancestor_groups: true)
+
+ expect(finder.execute).to eq [private_group_label_1, private_subgroup_label_1]
+ end
+
+ it 'ignores labels from groups which user can not read' do
+ private_subgroup_1.add_developer(user)
+
+ finder = described_class.new(user, group_id: private_subgroup_1.id, only_group_labels: true, include_ancestor_groups: true)
+
+ expect(finder.execute).to eq [private_subgroup_label_1]
+ end
+ end
end
context 'filtering by project_id' do
diff --git a/spec/services/labels/find_or_create_service_spec.rb b/spec/services/labels/find_or_create_service_spec.rb
index 78aa5d442e7..68d5660445a 100644
--- a/spec/services/labels/find_or_create_service_spec.rb
+++ b/spec/services/labels/find_or_create_service_spec.rb
@@ -15,47 +15,79 @@ describe Labels::FindOrCreateService do
context 'when acting on behalf of a specific user' do
let(:user) { create(:user) }
- subject(:service) { described_class.new(user, project, params) }
- before do
- project.add_developer(user)
- end
- context 'when label does not exist at group level' do
- it 'creates a new label at project level' do
- expect { service.execute }.to change(project.labels, :count).by(1)
+ context 'when finding labels on project level' do
+ subject(:service) { described_class.new(user, project, params) }
+
+ before do
+ project.add_developer(user)
end
- end
- context 'when label exists at group level' do
- it 'returns the group label' do
- group_label = create(:group_label, group: group, title: 'Security')
+ context 'when label does not exist at group level' do
+ it 'creates a new label at project level' do
+ expect { service.execute }.to change(project.labels, :count).by(1)
+ end
+ end
- expect(service.execute).to eq group_label
+ context 'when label exists at group level' do
+ it 'returns the group label' do
+ group_label = create(:group_label, group: group, title: 'Security')
+
+ expect(service.execute).to eq group_label
+ end
+ end
+
+ context 'when label exists at project level' do
+ it 'returns the project label' do
+ project_label = create(:label, project: project, title: 'Security')
+
+ expect(service.execute).to eq project_label
+ end
end
end
- context 'when label does not exist at group level' do
- it 'creates a new label at project leve' do
- expect { service.execute }.to change(project.labels, :count).by(1)
+ context 'when finding labels on group level' do
+ subject(:service) { described_class.new(user, group, params) }
+
+ before do
+ group.add_developer(user)
+ end
+
+ context 'when label does not exist at group level' do
+ it 'creates a new label at group level' do
+ expect { service.execute }.to change(group.labels, :count).by(1)
+ end
+ end
+
+ context 'when label exists at group level' do
+ it 'returns the group label' do
+ group_label = create(:group_label, group: group, title: 'Security')
+
+ expect(service.execute).to eq group_label
+ end
end
end
+ end
+
+ context 'when authorization is not required' do
+ context 'when finding labels on project level' do
+ subject(:service) { described_class.new(nil, project, params) }
- context 'when label exists at project level' do
it 'returns the project label' do
project_label = create(:label, project: project, title: 'Security')
- expect(service.execute).to eq project_label
+ expect(service.execute(skip_authorization: true)).to eq project_label
end
end
- end
- context 'when authorization is not required' do
- subject(:service) { described_class.new(nil, project, params) }
+ context 'when finding labels on group level' do
+ subject(:service) { described_class.new(nil, group, params) }
- it 'returns the project label' do
- project_label = create(:label, project: project, title: 'Security')
+ it 'returns the group label' do
+ group_label = create(:group_label, group: group, title: 'Security')
- expect(service.execute(skip_authorization: true)).to eq project_label
+ expect(service.execute(skip_authorization: true)).to eq group_label
+ end
end
end
end