diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index bb32bc502e6..27f1e91d865 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/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! diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index 5daf4311cc8..b2ff36f6538 100644 --- a/app/controllers/projects/application_controller.rb +++ b/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 diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 1558426e1a4..9f18c8c03df 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -47,15 +47,11 @@ class Projects::IssuesController < Projects::ApplicationController assignee_id: "" ) - @issue = @noteable = @project.issues.new(issue_params) - @labels = project_labels - + @issue = @noteable = @project.issues.new(issue_params) respond_with(@issue) end def edit - @labels = project_labels - respond_with(@issue) end diff --git a/app/controllers/projects/labels_controller.rb b/app/controllers/projects/labels_controller.rb index 3db3c091da6..33c3b7f79c2 100644 --- a/app/controllers/projects/labels_controller.rb +++ b/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 diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 291c3f64914..9171b47cda1 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/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 diff --git a/app/views/projects/labels/destroy.js.haml b/app/views/projects/labels/destroy.js.haml index 0b9937c4808..8d09e2bda11 100644 --- a/app/views/projects/labels/destroy.js.haml +++ b/app/views/projects/labels/destroy.js.haml @@ -1,2 +1,2 @@ -- if @project_labels.none? +- if @labels.empty? $('.labels').load(document.URL + ' .nothing-here-block').hide().fadeIn(1000) diff --git a/app/views/projects/labels/index.html.haml b/app/views/projects/labels/index.html.haml index 286b58f57c2..05282338493 100644 --- a/app/views/projects/labels/index.html.haml +++ b/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) diff --git a/spec/controllers/projects/labels_controller_spec.rb b/spec/controllers/projects/labels_controller_spec.rb index 3492b6ffbbb..2b39f9cf0d1 100644 --- a/spec/controllers/projects/labels_controller_spec.rb +++ b/spec/controllers/projects/labels_controller_spec.rb @@ -1,52 +1,92 @@ 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 + 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') } - before do - 15.times { |i| create_label(priority: (i % 3) + 1, title: "label #{15 - i}") } - 5.times { |i| create_label(title: "label #{100 - i}") } - - 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(titles.sort).to eq(titles) + 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(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