Commit 07a227a3 authored by Jan Provaznik's avatar Jan Provaznik Committed by Sean McGivern

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

parent 3d493cee
...@@ -39,7 +39,7 @@ class LabelsFinder < UnionFinder ...@@ -39,7 +39,7 @@ class LabelsFinder < UnionFinder
end end
end end
elsif only_group_labels? elsif only_group_labels?
label_ids << Label.where(group_id: group.id) label_ids << Label.where(group_id: group_ids)
else else
label_ids << Label.where(group_id: projects.group_ids) label_ids << Label.where(group_id: projects.group_ids)
label_ids << Label.where(project_id: projects.select(:id)) label_ids << Label.where(project_id: projects.select(:id))
...@@ -59,10 +59,11 @@ class LabelsFinder < UnionFinder ...@@ -59,10 +59,11 @@ class LabelsFinder < UnionFinder
items.where(title: title) items.where(title: title)
end end
def group def group_ids
strong_memoize(:group) do strong_memoize(:group_ids) do
group = Group.find(params[:group_id]) 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
end end
...@@ -120,4 +121,10 @@ class LabelsFinder < UnionFinder ...@@ -120,4 +121,10 @@ class LabelsFinder < UnionFinder
Ability.allowed?(current_user, :read_label, label_parent) Ability.allowed?(current_user, :read_label, label_parent)
end end
def groups_user_can_read_labels(groups)
DeclarativePolicy.user_scope do
groups.select { |group| authorized_to_read_labels?(group) }
end
end
end end
...@@ -77,8 +77,12 @@ class IssuableBaseService < BaseService ...@@ -77,8 +77,12 @@ class IssuableBaseService < BaseService
return unless labels return unless labels
params[:label_ids] = labels.split(",").map do |label_name| params[:label_ids] = labels.split(",").map do |label_name|
service = Labels::FindOrCreateService.new(current_user, project, title: label_name.strip) label = Labels::FindOrCreateService.new(
label = service.execute current_user,
parent,
title: label_name.strip,
available_labels: available_labels
).execute
label.try(:id) label.try(:id)
end.compact end.compact
...@@ -102,7 +106,7 @@ class IssuableBaseService < BaseService ...@@ -102,7 +106,7 @@ class IssuableBaseService < BaseService
end end
def available_labels def available_labels
LabelsFinder.new(current_user, project_id: @project.id).execute @available_labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute
end end
def merge_quick_actions_into_params!(issuable) def merge_quick_actions_into_params!(issuable)
...@@ -303,4 +307,8 @@ class IssuableBaseService < BaseService ...@@ -303,4 +307,8 @@ class IssuableBaseService < BaseService
def update_project_counter_caches?(issuable) def update_project_counter_caches?(issuable)
issuable.state_changed? issuable.state_changed?
end end
def parent
project
end
end end
module Labels module Labels
class FindOrCreateService class FindOrCreateService
def initialize(current_user, project, params = {}) def initialize(current_user, parent, params = {})
@current_user = current_user @current_user = current_user
@project = project @parent = parent
@available_labels = params.delete(:available_labels)
@params = params.dup.with_indifferent_access @params = params.dup.with_indifferent_access
end end
...@@ -13,12 +14,13 @@ module Labels ...@@ -13,12 +14,13 @@ module Labels
private private
attr_reader :current_user, :project, :params, :skip_authorization attr_reader :current_user, :parent, :params, :skip_authorization
def available_labels def available_labels
@available_labels ||= LabelsFinder.new( @available_labels ||= LabelsFinder.new(
current_user, current_user,
project_id: project.id "#{parent_type}_id".to_sym => parent.id,
only_group_labels: parent_is_group?
).execute(skip_authorization: skip_authorization) ).execute(skip_authorization: skip_authorization)
end end
...@@ -27,8 +29,8 @@ module Labels ...@@ -27,8 +29,8 @@ module Labels
def find_or_create_label def find_or_create_label
new_label = available_labels.find_by(title: title) new_label = available_labels.find_by(title: title)
if new_label.nil? && (skip_authorization || Ability.allowed?(current_user, :admin_label, project)) if new_label.nil? && (skip_authorization || Ability.allowed?(current_user, :admin_label, parent))
new_label = Labels::CreateService.new(params).execute(project: project) new_label = Labels::CreateService.new(params).execute(parent_type.to_sym => parent)
end end
new_label new_label
...@@ -37,5 +39,13 @@ module Labels ...@@ -37,5 +39,13 @@ module Labels
def title def title
params[:title] || params[:name] params[:title] || params[:name]
end end
def parent_type
parent.model_name.param_key
end
def parent_is_group?
parent_type == "group"
end
end end
end end
...@@ -5,6 +5,8 @@ describe LabelsFinder do ...@@ -5,6 +5,8 @@ describe LabelsFinder do
let(:group_1) { create(:group) } let(:group_1) { create(:group) }
let(:group_2) { create(:group) } let(:group_2) { create(:group) }
let(:group_3) { 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_1) { create(:project, namespace: group_1) }
let(:project_2) { create(:project, namespace: group_2) } let(:project_2) { create(:project, namespace: group_2) }
...@@ -20,6 +22,8 @@ describe LabelsFinder do ...@@ -20,6 +22,8 @@ describe LabelsFinder do
let!(:group_label_1) { create(:group_label, group: group_1, title: 'Label 1 (group)') } 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_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!(: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) } let(:user) { create(:user) }
...@@ -66,6 +70,25 @@ describe LabelsFinder do ...@@ -66,6 +70,25 @@ describe LabelsFinder do
expect(finder.execute).to eq [group_label_2, group_label_1] expect(finder.execute).to eq [group_label_2, group_label_1]
end end
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 end
context 'filtering by project_id' do context 'filtering by project_id' do
......
...@@ -15,47 +15,79 @@ describe Labels::FindOrCreateService do ...@@ -15,47 +15,79 @@ describe Labels::FindOrCreateService do
context 'when acting on behalf of a specific user' do context 'when acting on behalf of a specific user' do
let(:user) { create(:user) } 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 context 'when finding labels on project level' do
it 'creates a new label at project level' do subject(:service) { described_class.new(user, project, params) }
expect { service.execute }.to change(project.labels, :count).by(1)
before do
project.add_developer(user)
end end
end
context 'when label exists at group level' do context 'when label does not exist at group level' do
it 'returns the group label' do it 'creates a new label at project level' do
group_label = create(:group_label, group: group, title: 'Security') 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
end end
context 'when label does not exist at group level' do context 'when finding labels on group level' do
it 'creates a new label at project leve' do subject(:service) { described_class.new(user, group, params) }
expect { service.execute }.to change(project.labels, :count).by(1)
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 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 it 'returns the project label' do
project_label = create(:label, project: project, title: 'Security') 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 end
end
context 'when authorization is not required' do context 'when finding labels on group level' do
subject(:service) { described_class.new(nil, project, params) } subject(:service) { described_class.new(nil, group, params) }
it 'returns the project label' do it 'returns the group label' do
project_label = create(:label, project: project, title: 'Security') 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 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