Refactoring labels related services

9 files changed, 160 insertions, 63 deletions

diff --git a/app/models/label.rb b/app/models/label.rb
index bd04f1c6de5..a78fe16f762 100644
--- a/app/models/label.rb
+++ b/app/models/label.rb
@@ -36,6 +36,7 @@ class Label < ActiveRecord::Base
   default_scope { order(title: :asc) }
 
   scope :templates, -> { where(template: true) }
+  scope :with_type, ->(label_type) { where(label_type: Label.label_types[label_type]) }
 
   def self.prioritized
     where.not(priority: nil).reorder(:priority, :title)
diff --git a/app/services/labels/create_service.rb b/app/services/labels/create_service.rb
index ffc7851baab..97281b7c048 100644
--- a/app/services/labels/create_service.rb
+++ b/app/services/labels/create_service.rb
@@ -1,36 +1,34 @@
 module Labels
   class CreateService < Labels::BaseService
     def execute
-      label = subject.labels.build(params)
+      Label.transaction do
+        label = subject.labels.build(params)
+        label.label_type = subject.is_a?(Group) ? :group_label : :project_label
 
-      return label if subject.is_a?(Project) && subject.group.present? && subject.group.labels.where(title: title).exists?
+        return label if subject.is_a?(Project) && exists_at_group_level?
 
-      if label.save
-        if subject.is_a?(Group)
-          subject.projects.each do |project|
-            project.labels.find_or_create_by!(title: title) do |label|
-              label.color = color
-              label.description = description
-            end
-          end
+        if label.save && subject.is_a?(Group)
+          replicate_labels_to_projects
         end
-      end
 
-      label
+        label
+      end
     end
 
     private
 
-    def title
-      params[:title]
-    end
-
-    def color
-      params[:color]
+    def exists_at_group_level?
+      subject.group && subject.group.labels.where(title: params[:title]).exists?
     end
 
-    def description
-      params[:description]
+    def replicate_labels_to_projects
+      subject.projects.each do |project|
+        project.labels.find_or_create_by!(title: params[:title]) do |label|
+          label.color = params[:color]
+          label.description = params[:description]
+          label.label_type = :group_label
+        end
+      end
     end
   end
 end
diff --git a/app/services/labels/destroy_service.rb b/app/services/labels/destroy_service.rb
index 2bd065e13cf..b2bf42112cd 100644
--- a/app/services/labels/destroy_service.rb
+++ b/app/services/labels/destroy_service.rb
@@ -2,18 +2,27 @@ module Labels
   class DestroyService < Labels::BaseService
     def execute(label)
       Label.transaction do
-        destroy_project_labels(label.title) if subject.is_a?(Group)
         label.destroy
+
+        return unless label.group_label?
+
+        if subject.is_a?(Group)
+          destroy_labels(subject.projects, label.title)
+        end
+
+        if subject.is_a?(Project)
+          destroy_labels(subject.group, label.title)
+          destroy_labels(subject.group.projects - [subject], label.title)
+        end
       end
     end
 
     private
 
-    def destroy_project_labels(title)
-      subject.projects.each do |project|
-        label = project.labels.find_by(title: title)
-        label.destroy if label.present?
-      end
+    def destroy_labels(subject, title)
+      Label.with_type(:group_label)
+           .where(subject: subject, title: title)
+           .each(&:destroy)
     end
   end
 end
diff --git a/app/services/labels/replicate_service.rb b/app/services/labels/replicate_service.rb
index bf7db9469e8..707f3bcdfa9 100644
--- a/app/services/labels/replicate_service.rb
+++ b/app/services/labels/replicate_service.rb
@@ -1,22 +1,25 @@
 module Labels
   class ReplicateService < Labels::BaseService
     def execute
-      labels.each { |label| replicate(label) }
+      global_labels.each { |label| replicate_label(label, :global_label) }
+      group_labels.each  { |label| replicate_label(label, :group_label) }
     end
 
     private
 
-    def labels
-      global_labels = Label.templates
-      group_labels  = subject.group.present? ? subject.group.labels : []
+    def global_labels
+      Label.templates
+    end
 
-      global_labels + group_labels
+    def group_labels
+      subject.group.present? ? subject.group.labels : []
     end
 
-    def replicate(label)
+    def replicate_label(label, label_type)
       subject.labels.find_or_create_by!(title: label.title) do |replicated|
-        replicated.color = label.color
+        replicated.color       = label.color
         replicated.description = label.description
+        replicated.label_type  = label_type
       end
     end
   end
diff --git a/app/services/labels/update_service.rb b/app/services/labels/update_service.rb
index 20246ad9a1b..c7d58266ea9 100644
--- a/app/services/labels/update_service.rb
+++ b/app/services/labels/update_service.rb
@@ -2,15 +2,30 @@ module Labels
   class UpdateService < Labels::BaseService
     def execute(label)
       Label.transaction do
+        previous_title = label.title.dup
+        label.update(params)
+
+        return label unless label.valid? && label.group_label?
+
         if subject.is_a?(Group)
-          subject.projects.each do |project|
-            project_label = project.labels.find_by(title: label.title)
-            project_label.update_attributes(params) if project_label.present?
-          end
+          update_labels(subject.projects, previous_title)
+        end
+
+        if subject.is_a?(Project)
+          update_labels(subject.group, previous_title)
+          update_labels(subject.group.projects - [subject], previous_title)
         end
 
-        label.update_attributes(params)
+        label
       end
     end
+
+    private
+
+    def update_labels(subject, title)
+      Label.with_type(:group_label)
+           .where(subject: subject, title: title)
+           .update_all(params)
+    end
   end
 end
diff --git a/spec/factories/labels.rb b/spec/factories/labels.rb
index 942080b8644..34a56865697 100644
--- a/spec/factories/labels.rb
+++ b/spec/factories/labels.rb
@@ -4,4 +4,12 @@ FactoryGirl.define do
     color "#990000"
     subject factory: :project
   end
+
+  factory :group_label, parent: :label do
+    label_type Label.label_types[:group_label]
+  end
+
+  factory :project_label, parent: :label do
+    label_type Label.label_types[:project_label]
+  end
 end
diff --git a/spec/services/labels/create_service_spec.rb b/spec/services/labels/create_service_spec.rb
index b06424bcde4..8dfa6f70a6f 100644
--- a/spec/services/labels/create_service_spec.rb
+++ b/spec/services/labels/create_service_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Labels::CreateService, services: true do
   describe '#execute' do
-    let(:group) { create(:group) }
+    let!(:group) { create(:group) }
     let!(:project) { create(:empty_project, group: group) }
 
     let(:params) do
@@ -20,10 +20,16 @@ describe Labels::CreateService, services: true do
         expect { service.execute }.to change(group.labels, :count).by(1)
       end
 
+      it 'sets label_type to group_label' do
+        service.execute
+
+        expect(Label.last).to have_attributes(label_type: 'group_label')
+      end
+
       it 'becames available to all already existing projects of the group' do
         service.execute
 
-        expect(project.labels.where(params)).not_to be_empty
+        expect(project.labels.where(params.merge(label_type: Label.label_types[:group_label]))).not_to be_empty
       end
 
       it 'does not overwrite label that already exists in the project' do
@@ -43,8 +49,14 @@ describe Labels::CreateService, services: true do
         expect { service.execute }.to change(project.labels, :count).by(1)
       end
 
+      it 'sets label_type to project_label' do
+        service.execute
+
+        expect(Label.last).to have_attributes(label_type: 'project_label')
+      end
+
       it 'does not create a label that already exists on the group level' do
-        group.labels.create(params)
+        group.labels.create(params.merge(label_type: Label.label_types[:group_label]))
 
         expect { service.execute }.not_to change(project.labels, :count)
       end
diff --git a/spec/services/labels/destroy_service_spec.rb b/spec/services/labels/destroy_service_spec.rb
index cb6b048862e..6d3810154e6 100644
--- a/spec/services/labels/destroy_service_spec.rb
+++ b/spec/services/labels/destroy_service_spec.rb
@@ -2,22 +2,22 @@ require 'spec_helper'
 
 describe Labels::DestroyService, services: true do
   describe '#execute' do
-    let(:group) { create(:group) }
+    let!(:group) { create(:group) }
     let!(:project1) { create(:empty_project, group: group) }
+    let!(:project2) { create(:empty_project, group: group) }
 
-    context 'with a group as subject' do
-      let!(:label) { create(:label, subject: group, title: 'Bug') }
+    context 'with a group label' do
+      let!(:label) { create(:group_label, subject: group, title: 'Bug') }
 
       subject(:service) { described_class.new(group, double) }
 
-      it 'removes the label' do
+      it 'removes the group label' do
         expect { service.execute(label) }.to change(group.labels, :count).by(-1)
       end
 
-      it 'removes the label from projects of the group' do
-        project2 = create(:empty_project, group: group)
-        create(:label, subject: project1, title: 'Bug')
-        create(:label, subject: project2, title: 'Bug')
+      it 'removes the label from all projects inside the group' do
+        create(:group_label, subject: project1, title: 'Bug')
+        create(:group_label, subject: project2, title: 'Bug')
 
         service.execute(label)
 
@@ -26,14 +26,36 @@ describe Labels::DestroyService, services: true do
       end
     end
 
-    context 'with a project as subject' do
+    context 'with a project label' do
       subject(:service) { described_class.new(project1, double) }
 
-      it 'removes the label' do
-        label = create(:label, subject: project1)
+      it 'removes the project label' do
+        label = create(:project_label, subject: project1)
 
         expect { service.execute(label) }.to change(project1.labels, :count).by(-1)
       end
+
+      context 'inherited from a group' do
+        let!(:label) { create(:group_label, subject: project1, title: 'Bug') }
+
+        it 'removes the group label' do
+          create(:group_label, subject: group, title: 'Bug')
+
+          expect { service.execute(label) }.to change(group.labels, :count).by(-1)
+        end
+
+        it 'removes the label from all projects inside the group' do
+          create(:group_label, subject: project2, title: 'Bug')
+
+          service.execute(label)
+
+          expect(project2.labels.where(title: 'Bug')).to be_empty
+        end
+
+        it 'removes the project label' do
+          expect { service.execute(label) }.to change(project1.labels, :count).by(-1)
+        end
+      end
     end
   end
 end
diff --git a/spec/services/labels/update_service_spec.rb b/spec/services/labels/update_service_spec.rb
index e743bdfd0f2..24123ca22d9 100644
--- a/spec/services/labels/update_service_spec.rb
+++ b/spec/services/labels/update_service_spec.rb
@@ -2,8 +2,9 @@ require 'spec_helper'
 
 describe Labels::UpdateService, services: true do
   describe '#execute' do
-    let(:group) { create(:group) }
+    let!(:group) { create(:group) }
     let!(:project1) { create(:empty_project, group: group) }
+    let!(:project2) { create(:empty_project, group: group) }
 
     let(:params) do
       {
@@ -13,7 +14,7 @@ describe Labels::UpdateService, services: true do
       }
     end
 
-    context 'with a group as subject' do
+    context 'with a group label' do
       let(:label) { create(:label, subject: group, title: 'Bug') }
 
       subject(:service) { described_class.new(group, double, params) }
@@ -24,28 +25,56 @@ describe Labels::UpdateService, services: true do
         expect(label).to have_attributes(params)
       end
 
-      it 'updates the label from projects of the group' do
-        project2 = create(:empty_project, group: group)
-        create(:label, subject: project1, title: 'Bug')
-        create(:label, subject: project2, title: 'Bug')
+      it 'updates the label of all projects inside the group' do
+        label1 = create(:group_label, subject: project1, title: 'Bug')
+        label2 = create(:group_label, subject: project2, title: 'Bug')
 
-        service.execute(label)
+        service.execute(label1)
 
-        expect(project1.labels.where(params)).not_to be_empty
-        expect(project2.labels.where(params)).not_to be_empty
+        expect(label1.reload).to have_attributes(params)
+        expect(label2.reload).to have_attributes(params)
       end
     end
 
-    context 'with a project as subject' do
+    context 'with a project label' do
       subject(:service) { described_class.new(project1, double, params) }
 
       it 'updates the project label' do
-        label = create(:label, subject: project1)
+        label = create(:project_label, subject: project1)
 
         service.execute(label)
 
         expect(label).to have_attributes(params)
       end
+
+      context 'inherited from a group' do
+        it 'updates the group label' do
+          label1 = create(:group_label, subject: group, title: 'Bug')
+          label2 = create(:group_label, subject: project1, title: 'Bug')
+
+          service.execute(label2)
+
+          expect(label1.reload).to have_attributes(params)
+        end
+
+        it 'updates the label of all projects inside the group' do
+          label1 = create(:group_label, subject: project1, title: 'Bug')
+          label2 = create(:group_label, subject: project2, title: 'Bug')
+
+          service.execute(label1)
+
+          expect(label1.reload).to have_attributes(params)
+          expect(label2.reload).to have_attributes(params)
+        end
+
+        it 'updates the project label' do
+          label = create(:group_label, subject: project1)
+
+          service.execute(label)
+
+          expect(label).to have_attributes(params)
+        end
+      end
     end
   end
 end