Remove project_labels from Projects::ApplicationController

app/controllers/concerns/issuable_actions.rb

@@ -2,6 +2,7 @@ module IssuableActions
   extend ActiveSupport::Concern
 
   included do
+    before_action :labels, only: [:new, :edit]
     before_action :authorize_destroy_issuable!, only: :destroy
     before_action :authorize_admin_issuable!, only: :bulk_update
   end
@@ -25,6 +26,10 @@ module IssuableActions
 
   private
 
+  def labels
+    @labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute
+  end
+
   def authorize_destroy_issuable!
     unless can?(current_user, :"destroy_#{issuable.to_ability_name}", issuable)
       return access_denied!

app/controllers/projects/application_controller.rb

@@ -44,10 +44,6 @@ class Projects::ApplicationController < ApplicationController
     @project
   end
 
-  def project_labels
-    @project_labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute
-  end
-
   def repository
     @repository ||= project.repository
   end

app/controllers/projects/issues_controller.rb

@@ -48,14 +48,10 @@ class Projects::IssuesController < Projects::ApplicationController
     )
 
     @issue = @noteable = @project.issues.new(issue_params)
-    @labels = project_labels
-
     respond_with(@issue)
   end
 
   def edit
-    @labels = project_labels
-
     respond_with(@issue)
   end
 

app/controllers/projects/labels_controller.rb

@@ -3,23 +3,23 @@ class Projects::LabelsController < Projects::ApplicationController
 
   before_action :module_enabled
   before_action :label, only: [:edit, :update, :destroy]
+  before_action :labels, only: [:index]
   before_action :authorize_read_label!
-  before_action :authorize_admin_labels!, only: [
-    :new, :create, :edit, :update, :generate, :destroy, :remove_priority, :set_priorities
-  ]
+  before_action :authorize_admin_labels!, only: [:new, :create, :edit, :update,
+                                                 :generate, :destroy, :remove_priority,
+                                                 :set_priorities]
 
   respond_to :js, :html
 
   def index
-    @project_labels = project_labels
-    @prioritized_labels = project_labels.prioritized
+    @prioritized_labels = @labels.prioritized
     @group_labels = @project.group.labels.unprioritized if @project.group.present?
-    @labels = @project.labels.unprioritized.page(params[:page])
+    @project_labels = @project.labels.unprioritized.page(params[:page])
 
     respond_to do |format|
       format.html
       format.json do
-        render json: @project_labels
+        render json: @labels
       end
     end
   end
@@ -38,7 +38,7 @@ class Projects::LabelsController < Projects::ApplicationController
       end
     else
       respond_to do |format|
-        format.html { render 'new' }
+        format.html { render :new }
         format.json { render json: { message: @label.errors.messages }, status: 400 }
       end
     end
@@ -51,7 +51,7 @@ class Projects::LabelsController < Projects::ApplicationController
     if @label.update_attributes(label_params)
       redirect_to namespace_project_labels_path(@project.namespace, @project)
     else
-      render 'edit'
+      render :edit
     end
   end
 
@@ -70,7 +70,7 @@ class Projects::LabelsController < Projects::ApplicationController
 
   def destroy
     @label.destroy
-    @project_labels = project_labels
+    @labels = labels
 
     respond_to do |format|
       format.html do
@@ -83,7 +83,7 @@ class Projects::LabelsController < Projects::ApplicationController
 
   def remove_priority
     respond_to do |format|
-      label = project_labels.find(params[:id])
+      label = labels.find(params[:id])
 
       if label.update_attribute(:priority, nil)
         format.json { render json: label }
@@ -97,7 +97,7 @@ class Projects::LabelsController < Projects::ApplicationController
   def set_priorities
     Label.transaction do
       params[:label_ids].each_with_index do |label_id, index|
-        label = project_labels.find_by_id(label_id)
+        label = labels.find_by_id(label_id)
         label.update_attribute(:priority, index) if label
       end
     end
@@ -124,6 +124,10 @@ class Projects::LabelsController < Projects::ApplicationController
   end
   alias_method :subscribable_resource, :label
 
+  def labels
+    @labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute
+  end
+
   def authorize_admin_labels!
     return render_404 unless can?(current_user, :admin_label, @project)
   end

app/controllers/projects/merge_requests_controller.rb

@@ -263,7 +263,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @source_project = @merge_request.source_project
     @target_project = @merge_request.target_project
     @target_branches = @merge_request.target_project.repository.branch_names
-    @labels = project_labels
   end
 
   def update

app/views/projects/labels/destroy.js.haml

-- if @project_labels.none?
+- if @labels.empty?
   $('.labels').load(document.URL + ' .nothing-here-block').hide().fadeIn(1000)

app/views/projects/labels/index.html.haml

@@ -14,7 +14,7 @@
           New label
 
   .labels
-    - unless @project_labels.empty?
+    - unless @labels.empty?
       -# Only show it in the first page
       - hide = params[:page].present? && params[:page] != '1'
       - if can?(current_user, :admin_label, @project)
@@ -39,10 +39,10 @@
           = icon('bookmark')
           Project Labels
         %ul.content-list.manage-labels-list.js-project-labels
-          %p.empty-message{ class: ('hidden' unless @labels.empty?) } No project labels
-          - if @labels.present?
-            = render @labels
-            = paginate @labels, theme: 'gitlab'
+          %p.empty-message{ class: ('hidden' unless @project_labels.empty?) } No project labels
+          - if @project_labels.present?
+            = render @project_labels
+            = paginate @project_labels, theme: 'gitlab'
     - else
       .nothing-here-block
         - if can?(current_user, :admin_label, @project)

spec/controllers/projects/labels_controller_spec.rb

 require 'spec_helper'
 
 describe Projects::LabelsController do
-  let(:project) { create(:project) }
+  let(:group)   { create(:group) }
+  let(:project) { create(:project, namespace: group) }
   let(:user)    { create(:user) }
 
   before do
     project.team << [user, :master]
+
     sign_in(user)
   end
 
   describe 'GET #index' do
-    def create_label(attributes)
-      create(:label, attributes.merge(project: project))
-    end
-
-    before do
-      15.times { |i| create_label(priority: (i % 3) + 1, title: "label #{15 - i}") }
-      5.times { |i| create_label(title: "label #{100 - i}") }
+    let!(:label_1) { create(:label, project: project, priority: 1, title: 'Label 1') }
+    let!(:label_2) { create(:label, project: project, priority: 3, title: 'Label 2') }
+    let!(:label_3) { create(:label, project: project, priority: 1, title: 'Label 3') }
+    let!(:label_4) { create(:label, project: project, priority: nil, title: 'Label 4') }
+    let!(:label_5) { create(:label, project: project, priority: nil, title: 'Label 5') }
 
-      get :index, namespace_id: project.namespace.to_param, project_id: project.to_param
-    end
+    let!(:group_label_1) { create(:group_label, group: group, priority: 3, title: 'Group Label 1') }
+    let!(:group_label_2) { create(:group_label, group: group, priority: 1, title: 'Group Label 2') }
+    let!(:group_label_3) { create(:group_label, group: group, priority: nil, title: 'Group Label 3') }
+    let!(:group_label_4) { create(:group_label, group: group, priority: nil, title: 'Group Label 4') }
 
     context '@prioritized_labels' do
-      let(:prioritized_labels) { assigns(:prioritized_labels) }
+      before do
+        list_labels
+      end
 
       it 'contains only prioritized labels' do
-        expect(prioritized_labels).to all(have_attributes(priority: a_value > 0))
+        expect(assigns(:prioritized_labels)).to all(have_attributes(priority: a_value > 0))
       end
 
       it 'is sorted by priority, then label title' do
-        priorities_and_titles = prioritized_labels.pluck(:priority, :title)
-
-        expect(priorities_and_titles.sort).to eq(priorities_and_titles)
+        expect(assigns(:prioritized_labels)).to match_array [group_label_2, label_1, label_3, group_label_1, label_2]
       end
     end
 
-    context '@labels' do
-      let(:labels) { assigns(:labels) }
+    context '@group_labels' do
+      it 'contains only group labels' do
+        list_labels
+
+        expect(assigns(:group_labels)).to all(have_attributes(group_id: a_value > 0))
+      end
 
       it 'contains only unprioritized labels' do
-        expect(labels).to all(have_attributes(priority: nil))
+        list_labels
+
+        expect(assigns(:group_labels)).to all(have_attributes(priority: nil))
       end
 
       it 'is sorted by label title' do
-        titles = labels.pluck(:title)
+        list_labels
+
+        expect(assigns(:group_labels)).to match_array [group_label_3, group_label_4]
+      end
+
+      it 'is nil when project does not belong to a group' do
+        project.update(namespace: create(:namespace))
+
+        list_labels
 
-        expect(titles.sort).to eq(titles)
+        expect(assigns(:group_labels)).to be_nil
       end
     end
+
+    context '@project_labels' do
+      before do
+        list_labels
+      end
+
+      it 'contains only project labels' do
+        list_labels
+
+        expect(assigns(:project_labels)).to all(have_attributes(project_id: a_value > 0))
+      end
+
+      it 'contains only unprioritized labels' do
+        expect(assigns(:project_labels)).to all(have_attributes(priority: nil))
+      end
+
+      it 'is sorted by label title' do
+        expect(assigns(:project_labels)).to match_array [label_4, label_5]
+      end
+    end
+
+    def list_labels
+      get :index, namespace_id: project.namespace.to_param, project_id: project.to_param
+    end
   end
 end