diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index fbe9b7d7f16e2636d9c464062b9fc5fd40be5e0d..e3dc569152cda3af69f5719f6a94fffa0461095a 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -72,18 +72,17 @@ class IssuableBaseService < BaseService if params[:add_label_ids].present? || params[:remove_label_ids].present? params.delete(:label_ids) - filter_labels_by_name([:add_label_ids, :remove_label_ids]) + filter_labels_in_param(:add_label_ids) + filter_labels_in_param(:remove_label_ids) else - filter_labels_by_name([:label_ids]) + filter_labels_in_param(:label_ids) end end - def filter_labels_by_name(keys) - keys.each do |key| - next if params[key].to_a.empty? + def filter_labels_in_param(key) + return if params[key].to_a.empty? - params[key] = project.labels.where(id: params[key]).pluck(:id) - end + params[key] = project.labels.where(id: params[key]).pluck(:id) end def update_issuable(issuable, attributes) @@ -94,7 +93,7 @@ class IssuableBaseService < BaseService issuable.label_ids |= add_label_ids if add_label_ids issuable.label_ids -= remove_label_ids if remove_label_ids - issuable.assign_attributes(attributes) + issuable.assign_attributes(attributes.merge(updated_by: current_user)) issuable.save end @@ -105,7 +104,7 @@ class IssuableBaseService < BaseService filter_params old_labels = issuable.labels.to_a - if params.present? && update_issuable(issuable, params.merge(updated_by: current_user)) + if params.present? && update_issuable(issuable, params) issuable.reset_events_cache handle_common_system_notes(issuable, old_labels: old_labels) handle_changes(issuable, old_labels: old_labels) diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index be19be17151cbd170999a3ae63dd9f26f9862c19..dacbcd8fb46e07c5ae61a41490f2418cce8ab99a 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -1,3 +1,4 @@ +# coding: utf-8 require 'spec_helper' describe Issues::UpdateService, services: true do @@ -273,5 +274,50 @@ describe Issues::UpdateService, services: true do end end end + + context 'updating labels' do + let(:label3) { create(:label, project: project) } + let(:result) { Issues::UpdateService.new(project, user, params).execute(issue).reload } + + context 'when add_label_ids and label_ids are passed' do + let(:params) { { label_ids: [label.id], add_label_ids: [label3.id] } } + + it 'ignores the label_ids parameter' do + expect(result.label_ids).not_to include(label.id) + end + + it 'adds the passed labels' do + expect(result.label_ids).to include(label3.id) + end + end + + context 'when remove_label_ids and label_ids are passed' do + let(:params) { { label_ids: [], remove_label_ids: [label.id] } } + + before { issue.update_attributes(labels: [label, label3]) } + + it 'ignores the label_ids parameter' do + expect(result.label_ids).not_to be_empty + end + + it 'removes the passed labels' do + expect(result.label_ids).not_to include(label.id) + end + end + + context 'when add_label_ids and remove_label_ids are passed' do + let(:params) { { add_label_ids: [label3.id], remove_label_ids: [label.id] } } + + before { issue.update_attributes(labels: [label]) } + + it 'adds the passed labels' do + expect(result.label_ids).to include(label3.id) + end + + it 'removes the passed labels' do + expect(result.label_ids).not_to include(label.id) + end + end + end end end