Commit ab24a7b2 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'jprovazn-subgroup-labels' into 'master'

Allow to find labels in ancestor groups and better group support in label service

See merge request gitlab-org/gitlab-ce!17209
parents 3d493cee 07a227a3
......@@ -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
......@@ -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
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
......@@ -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
......
......@@ -15,7 +15,10 @@ describe Labels::FindOrCreateService do
context 'when acting on behalf of a specific user' do
let(:user) { create(:user) }
context 'when finding labels on project level' do
subject(:service) { described_class.new(user, project, params) }
before do
project.add_developer(user)
end
......@@ -34,12 +37,6 @@ describe Labels::FindOrCreateService do
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)
end
end
context 'when label exists at project level' do
it 'returns the project label' do
project_label = create(:label, project: project, title: 'Security')
......@@ -49,7 +46,31 @@ describe Labels::FindOrCreateService do
end
end
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) }
it 'returns the project label' do
......@@ -58,5 +79,16 @@ describe Labels::FindOrCreateService do
expect(service.execute(skip_authorization: true)).to eq project_label
end
end
context 'when finding labels on group level' do
subject(:service) { described_class.new(nil, group, params) }
it 'returns the group label' do
group_label = create(:group_label, group: group, title: 'Security')
expect(service.execute(skip_authorization: true)).to eq group_label
end
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment