Commit 5b83abcc authored by Jacob Schatz's avatar Jacob Schatz

Merge branch 'issue_14189' into 'master'

Ability to prioritize labels

Closes #14189 

See merge request !4009
parents 25370fd1 ee26c3ca
......@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
v 8.9.0 (unreleased)
- Bulk assign/unassign labels to issues.
- Ability to prioritize labels !4009 / !3205 (Thijs Wouters)
- Allow enabling wiki page events from Webhook management UI
- Bump rouge to 1.11.0
- Make EmailsOnPushWorker use Sidekiq mailers queue
......
class @LabelManager
errorMessage: 'Unable to update label prioritization at this time'
constructor: (opts = {}) ->
# Defaults
{
@togglePriorityButton = $('.js-toggle-priority')
@prioritizedLabels = $('.js-prioritized-labels')
@otherLabels = $('.js-other-labels')
} = opts
@prioritizedLabels.sortable(
items: 'li'
placeholder: 'list-placeholder'
axis: 'y'
update: @onPrioritySortUpdate.bind(@)
)
@bindEvents()
bindEvents: ->
@togglePriorityButton.on 'click', @, @onTogglePriorityClick
onTogglePriorityClick: (e) ->
e.preventDefault()
_this = e.data
$btn = $(e.currentTarget)
$label = $("##{$btn.data('domId')}")
action = if $btn.parents('.js-prioritized-labels').length then 'remove' else 'add'
_this.toggleLabelPriority($label, action)
toggleLabelPriority: ($label, action, persistState = true) ->
_this = @
url = $label.find('.js-toggle-priority').data 'url'
$target = @prioritizedLabels
$from = @otherLabels
# Optimistic update
if action is 'remove'
$target = @otherLabels
$from = @prioritizedLabels
if $from.find('li').length is 1
$from.find('.empty-message').show()
if not $target.find('li').length
$target.find('.empty-message').hide()
$label.detach().appendTo($target)
# Return if we are not persisting state
return unless persistState
if action is 'remove'
xhr = $.ajax url: url, type: 'DELETE'
else
xhr = @savePrioritySort($label, action)
xhr.fail @rollbackLabelPosition.bind(@, $label, action)
onPrioritySortUpdate: ->
xhr = @savePrioritySort()
xhr.fail ->
new Flash(@errorMessage, 'alert')
savePrioritySort: () ->
$.post
url: @prioritizedLabels.data('url')
data:
label_ids: @getSortedLabelsIds()
rollbackLabelPosition: ($label, originalAction)->
action = if originalAction is 'remove' then 'add' else 'remove'
@toggleLabelPriority($label, action, false)
new Flash(@errorMessage, 'alert')
getSortedLabelsIds: ->
sortedIds = []
@prioritizedLabels.find('li').each ->
sortedIds.push $(@).data 'id'
sortedIds
......@@ -100,6 +100,8 @@ class Dispatcher
shortcut_handler = new ShortcutsNavigation()
when 'projects:labels:new', 'projects:labels:edit'
new Labels()
when 'projects:labels:index'
new LabelManager() if $('.prioritized-labels').length
when 'projects:network:show'
# Ensure we don't create a particular shortcut handler here. This is
# already created, where the network graph is created.
......
......@@ -141,6 +141,18 @@ ul.content-list {
padding: 10px 14px;
}
}
// When dragging a list item
&.ui-sortable-helper {
border-bottom: none;
}
&.list-placeholder {
background-color: $gray-light;
border: dotted 1px $gray-dark;
margin: 1px 0;
min-height: 30px;
}
}
}
......
......@@ -51,7 +51,7 @@
.label-row {
.label-name {
display: inline-block;
width: 200px;
width: 170px;
@media (max-width: $screen-xs-min) {
display: block;
......@@ -138,3 +138,34 @@
}
}
}
.prioritized-labels {
margin-bottom: 30px;
.add-priority {
display: none;
color: $gray-light;
}
}
.other-labels {
.remove-priority {
display: none;
}
}
.toggle-priority {
display: inline-block;
vertical-align: middle;
button {
border-color: transparent;
padding: 5px 8px;
vertical-align: top;
font-size: 14px;
&:hover {
border-color: transparent;
}
}
}
......@@ -5,13 +5,14 @@ class Projects::LabelsController < Projects::ApplicationController
before_action :label, only: [:edit, :update, :destroy]
before_action :authorize_read_label!
before_action :authorize_admin_labels!, only: [
:new, :create, :edit, :update, :generate, :destroy
:new, :create, :edit, :update, :generate, :destroy, :remove_priority, :set_priorities
]
respond_to :js, :html
def index
@labels = @project.labels.page(params[:page])
@labels = @project.labels.unprioritized.page(params[:page])
@prioritized_labels = @project.labels.prioritized
respond_to do |format|
format.html
......@@ -71,6 +72,30 @@ class Projects::LabelsController < Projects::ApplicationController
end
end
def remove_priority
respond_to do |format|
if label.update_attribute(:priority, nil)
format.json { render json: label }
else
message = label.errors.full_messages.uniq.join('. ')
format.json { render json: { message: message }, status: :unprocessable_entity }
end
end
end
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.update_attribute(:priority, index) if label
end
end
respond_to do |format|
format.json { render json: { message: 'success' } }
end
end
protected
def module_enabled
......
......@@ -224,7 +224,7 @@ class IssuableFinder
def sort(items)
# Ensure we always have an explicit sort order (instead of inheriting
# multiple orders when combining ActiveRecord::Relation objects).
params[:sort] ? items.sort(params[:sort]) : items.reorder(id: :desc)
params[:sort] ? items.sort(params[:sort], excluded_labels: label_names) : items.reorder(id: :desc)
end
def by_assignee(items)
......@@ -318,7 +318,11 @@ class IssuableFinder
end
def label_names
params[:label_name].is_a?(String) ? params[:label_name].split(',') : params[:label_name]
if labels?
params[:label_name].is_a?(String) ? params[:label_name].split(',') : params[:label_name]
else
[]
end
end
def current_user_related?
......
......@@ -14,7 +14,8 @@ module SortingHelper
sort_value_recently_signin => sort_title_recently_signin,
sort_value_oldest_signin => sort_title_oldest_signin,
sort_value_downvotes => sort_title_downvotes,
sort_value_upvotes => sort_title_upvotes
sort_value_upvotes => sort_title_upvotes,
sort_value_priority => sort_title_priority
}
end
......@@ -28,6 +29,10 @@ module SortingHelper
}
end
def sort_title_priority
'Priority'
end
def sort_title_oldest_updated
'Oldest updated'
end
......@@ -84,6 +89,10 @@ module SortingHelper
'Most popular'
end
def sort_value_priority
'priority'
end
def sort_value_oldest_updated
'updated_asc'
end
......
......@@ -105,17 +105,24 @@ module Issuable
where(t[:title].matches(pattern).or(t[:description].matches(pattern)))
end
def sort(method)
def sort(method, excluded_labels: [])
case method.to_s
when 'milestone_due_asc' then order_milestone_due_asc
when 'milestone_due_desc' then order_milestone_due_desc
when 'downvotes_desc' then order_downvotes_desc
when 'upvotes_desc' then order_upvotes_desc
when 'priority' then order_labels_priority(excluded_labels: excluded_labels)
else
order_by(method)
end
end
def order_labels_priority(excluded_labels: [])
select("#{table_name}.*, (#{highest_label_priority(excluded_labels).to_sql}) AS highest_priority").
group(arel_table[:id]).
reorder(Gitlab::Database.nulls_last_order('highest_priority', 'ASC'))
end
def with_label(title, sort = nil)
if title.is_a?(Array) && title.size > 1
joins(:labels).where(labels: { title: title }).group(*grouping_columns(sort)).having("COUNT(DISTINCT labels.title) = #{title.size}")
......@@ -139,6 +146,20 @@ module Issuable
grouping_columns
end
private
def highest_label_priority(excluded_labels)
query = Label.select(Label.arel_table[:priority].minimum).
joins(:label_links).
where(label_links: { target_type: name }).
where("label_links.target_id = #{table_name}.id").
reorder(nil)
query.where.not(title: excluded_labels) if excluded_labels.present?
query
end
end
def today?
......
......@@ -75,10 +75,10 @@ class Issue < ActiveRecord::Base
@link_reference_pattern ||= super("issues", /(?<issue>\d+)/)
end
def self.sort(method)
def self.sort(method, excluded_labels: [])
case method.to_s
when 'due_date_asc' then order_due_date_asc
when 'due_date_desc' then order_due_date_desc
when 'due_date_desc' then order_due_date_desc
else
super
end
......
......@@ -26,10 +26,20 @@ class Label < ActiveRecord::Base
format: { with: /\A[^&\?,]+\z/ },
uniqueness: { scope: :project_id }
before_save :nullify_priority
default_scope { order(title: :asc) }
scope :templates, -> { where(template: true) }
def self.prioritized
where.not(priority: nil).reorder(:priority, :title)
end
def self.unprioritized
where(priority: nil)
end
alias_attribute :name, :title
def self.reference_prefix
......@@ -118,4 +128,8 @@ class Label < ActiveRecord::Base
id
end
end
def nullify_priority
self.priority = nil if priority.blank?
end
end
%li{ id: dom_id(label), data: { id: label.id } }
- label_css_id = dom_id(label)
%li{id: label_css_id, data: { id: label.id } }
= render "shared/label_row", label: label
.pull-info-right
%span.append-right-20
......@@ -10,18 +11,18 @@
= pluralize label.open_issues_count(current_user), 'open issue'
- if current_user
.label-subscription{data: {url: toggle_subscription_namespace_project_label_path(@project.namespace, @project, label)}}
.subscription-status{data: {status: label_subscription_status(label)}}
.label-subscription{ data: { url: toggle_subscription_namespace_project_label_path(@project.namespace, @project, label) } }
.subscription-status{ data: { status: label_subscription_status(label) } }
%button.js-subscribe-button.label-subscribe-button.btn.action-buttons{ type: "button", data: { toggle: "tooltip" } }
%span= label_subscription_toggle_button_text(label)
- if can? current_user, :admin_label, @project
= link_to edit_namespace_project_label_path(@project.namespace, @project, label), title: "Edit", class: 'btn action-buttons', data: {toggle: "tooltip"} do
- if can?(current_user, :admin_label, @project)
= link_to edit_namespace_project_label_path(@project.namespace, @project, label), title: "Edit", class: 'btn action-buttons', data: { toggle: 'tooltip' } do
%i.fa.fa-pencil-square-o
= link_to namespace_project_label_path(@project.namespace, @project, label), title: "Delete", class: 'btn action-buttons remove-row', method: :delete, remote: true, data: {confirm: "Remove this label? Are you sure?", toggle: "tooltip"} do
= link_to namespace_project_label_path(@project.namespace, @project, label), title: "Delete", class: 'btn action-buttons remove-row', method: :delete, remote: true, data: { confirm: 'Remove this label? Are you sure?', toggle: 'tooltip' } do
%i.fa.fa-trash-o
- if current_user
:javascript
new Subscription('##{dom_id(label)} .label-subscription');
new Subscription('##{label_css_id} .label-subscription');
- page_title "Labels"
- hide_class = ''
.top-area
.nav-text
Labels can be applied to issues and merge requests.
.nav-controls
- if can? current_user, :admin_label, @project
- if can?(current_user, :admin_label, @project)
= link_to new_namespace_project_label_path(@project.namespace, @project), class: "btn btn-new" do
= icon('plus')
New label
.labels
- if @labels.present?
%ul.content-list.manage-labels-list
= render @labels
= paginate @labels, theme: 'gitlab'
- else
.nothing-here-block
- if can? current_user, :admin_label, @project
Create a label or #{link_to 'generate a default set of labels', generate_namespace_project_labels_path(@project.namespace, @project), method: :post}.
- else
No labels created
- if can?(current_user, :admin_label, @project)
-# Only show it in the first page
- hide = @project.labels.empty? || (params[:page].present? && params[:page] != '1')
.prioritized-labels{ class: ('hide' if hide) }
%h5 Prioritized Labels
%ul.content-list.manage-labels-list.js-prioritized-labels{ "data-url" => set_priorities_namespace_project_labels_path(@project.namespace, @project) }
- if @prioritized_labels.present?
= render @prioritized_labels
- else
%p.empty-message No prioritized labels yet
.other-labels
- if can?(current_user, :admin_label, @project)
%h5{ class: ('hide' if hide) } Other Labels
- if @labels.present?
%ul.content-list.manage-labels-list.js-other-labels
= render @labels
= paginate @labels, theme: 'gitlab'
- else
.nothing-here-block
- if can?(current_user, :admin_label, @project)
Create a label or #{link_to 'generate a default set of labels', generate_namespace_project_labels_path(@project.namespace, @project), method: :post}.
- else
No labels created
- if @issues.any?
- if @issues.reorder(nil).any?
- @issues.group_by(&:project).each do |group|
.panel.panel-default.panel-small
- project = group[0]
......
%span.label-row
- if can?(current_user, :admin_label, @project)
.js-toggle-priority.toggle-priority{ data: { url: remove_priority_namespace_project_label_path(@project.namespace, @project, label),
dom_id: dom_id(label) } }
%button.add-priority.btn.has-tooltip{ title: 'Prioritize', :'data-placement' => 'top' }
= icon('star-o')
%button.remove-priority.btn.has-tooltip{ title: 'Remove priority', :'data-placement' => 'top' }
= icon('star')
%span.label-name
= link_to_label(label, tooltip: false)
%span.prepend-left-10
= markdown(label.description, pipeline: :single_line)
\ No newline at end of file
= markdown(label.description, pipeline: :single_line)
......@@ -8,6 +8,8 @@
%b.caret
%ul.dropdown-menu.dropdown-menu-align-right.dropdown-menu-sort
%li
= link_to page_filter_path(sort: sort_value_priority) do
= sort_title_priority
= link_to page_filter_path(sort: sort_value_recently_created) do
= sort_title_recently_created
= link_to page_filter_path(sort: sort_value_oldest_created) do
......
......@@ -719,10 +719,12 @@ Rails.application.routes.draw do
resources :labels, constraints: { id: /\d+/ } do
collection do
post :generate
post :set_priorities
end
member do
post :toggle_subscription
delete :remove_priority
end
end
......
class AddPriorityToLabel < ActiveRecord::Migration
def change
add_column :labels, :priority, :integer
add_index :labels, :priority
end
end
......@@ -496,8 +496,10 @@ ActiveRecord::Schema.define(version: 20160530150109) do
t.datetime "updated_at"
t.boolean "template", default: false
t.string "description"
t.integer "priority"
end
add_index "labels", ["priority"], name: "index_labels_on_priority", using: :btree
add_index "labels", ["project_id"], name: "index_labels_on_project_id", using: :btree
create_table "lfs_objects", force: :cascade do |t|
......
......@@ -60,25 +60,25 @@ class Spinach::Features::ProjectIssuesLabels < Spinach::FeatureSteps
end
step 'I should see label \'feature\'' do
page.within '.manage-labels-list' do
page.within '.other-labels .manage-labels-list' do
expect(page).to have_content 'feature'
end
end
step 'I should see label \'bug\'' do
page.within '.manage-labels-list' do
page.within '.other-labels .manage-labels-list' do
expect(page).to have_content 'bug'
end
end
step 'I should not see label \'bug\'' do
page.within '.manage-labels-list' do
page.within '.other-labels .manage-labels-list' do
expect(page).not_to have_content 'bug'
end
end
step 'I should see label \'support\'' do
page.within '.manage-labels-list' do
page.within '.other-labels .manage-labels-list' do
expect(page).to have_content 'support'
end
end
......@@ -90,7 +90,7 @@ class Spinach::Features::ProjectIssuesLabels < Spinach::FeatureSteps
end
step 'I should see label \'fix\'' do
page.within '.manage-labels-list' do
page.within '.other-labels .manage-labels-list' do
expect(page).to have_content 'fix'
end
end
......
......@@ -16,6 +16,20 @@ module Gitlab
database_version.match(/\A(?:PostgreSQL |)([^\s]+).*\z/)[1]
end
def self.nulls_last_order(field, direction = 'ASC')
order = "#{field} #{direction}"
if Gitlab::Database.postgresql?
order << ' NULLS LAST'
else
# `field IS NULL` will be `0` for non-NULL columns and `1` for NULL
# columns. In the (default) ascending order, `0` comes first.
order.prepend("#{field} IS NULL, ") if direction == 'ASC'
end
order
end
def true_value
if Gitlab::Database.postgresql?
"'t'"
......
require 'spec_helper'
describe Projects::LabelsController do
let(:project) { create(:project) }
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}") }
get :index, namespace_id: project.namespace.to_param, project_id: project.to_param
end
context '@prioritized_labels' do
let(:prioritized_labels) { assigns(:prioritized_labels) }
it 'contains only prioritized labels' do
expect(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)
end
end
context '@labels' do
let(:labels) { assigns(:labels) }
it 'contains only unprioritized labels' do
expect(labels).to all(have_attributes(priority: nil))
end
it 'is sorted by label title' do
titles = labels.pluck(:title)
expect(titles.sort).to eq(titles)
end
end
end
end
require 'spec_helper'
feature 'Issue prioritization', feature: true do
let(:user) { create(:user) }
let(:project) { create(:project, name: 'test', namespace: user.namespace) }
# Labels
let(:label_1) { create(:label, title: 'label_1', project: project, priority: 1) }
let(:label_2) { create(:label, title: 'label_2', project: project, priority: 2) }
let(:label_3) { create(:label, title: 'label_3', project: project, priority: 3) }
let(:label_4) { create(:label, title: 'label_4', project: project, priority: 4) }
let(:label_5) { create(:label, title: 'label_5', project: project) } # no priority
# According to https://gitlab.com/gitlab-org/gitlab-ce/issues/14189#note_4360653
context 'when issues have one label' do
scenario 'Are sorted properly' do
# Issues
issue_1 = create(:issue, title: 'issue_1', project: project)
issue_2 = create(:issue, title: 'issue_2', project: project)
issue_3 = create(:issue, title: 'issue_3', project: project)
issue_4 = create(:issue, title: 'issue_4', project: project)
issue_5 = create(:issue, title: 'issue_5', project: project)
# Assign labels to issues disorderly
issue_4.labels << label_1
issue_3.labels << label_2
issue_5.labels << label_3
issue_2.labels << label_4
issue_1.labels << label_5
login_as user
visit namespace_project_issues_path(project.namespace, project, sort: 'priority')
# Ensure we are indicating that issues are sorted by priority
expect(page).to have_selector('.dropdown-toggle', text: 'Priority')
page.within('.issues-holder') do
issue_titles = all('.issues-list .issue-title-text').map(&:text)
expect(issue_titles).to eq(['issue_4', 'issue_3', 'issue_5', 'issue_2', 'issue_1'])
end
end
end
context 'when issues have multiple labels' do
scenario 'Are sorted properly' do
# Issues
issue_1 = create(:issue, title: 'issue_1', project: project)
issue_2 = create(:issue, title: 'issue_2', project: project)
issue_3 = create(:issue, title: 'issue_3', project: project)
issue_4 = create(:issue, title: 'issue_4', project: project)
issue_5 = create(:issue, title: 'issue_5', project: project)
issue_6 = create(:issue, title: 'issue_6', project: project)
issue_7 = create(:issue, title: 'issue_7', project: project)
issue_8 = create(:issue, title: 'issue_8', project: project)
# Assign labels to issues disorderly
issue_5.labels << label_1 # 1
issue_5.labels << label_2
issue_8.labels << label_1 # 2
issue_1.labels << label_2 # 3
issue_1.labels << label_3
issue_3.labels << label_2 # 4
issue_3.labels << label_4
issue_7.labels << label_2 # 5
issue_2.labels << label_3 # 6
issue_4.labels << label_4 # 7
issue_6.labels << label_5 # 8 - No priority
login_as user
visit namespace_project_issues_path(project.namespace, project, sort: 'priority')
expect(page).to have_selector('.dropdown-toggle', text: 'Priority')
page.within('.issues-holder') do
issue_titles = all('.issues-list .issue-title-text').map(&:text)
expect(issue_titles[0..1]).to contain_exactly('issue_5', 'issue_8')
expect(issue_titles[2..4]).to contain_exactly('issue_1', 'issue_3', 'issue_7')
expect(issue_titles[5..-1]).to eq(['issue_2', 'issue_4', 'issue_6'])
end
end
end
end
require 'spec_helper'
feature 'Prioritize labels', feature: true do
include WaitForAjax
context 'when project belongs to user' do
let(:user) { create(:user) }
let(:project) { create(:project, name: 'test', namespace: user.namespace) }
scenario 'user can prioritize a label', js: true do
bug = create(:label, title: 'bug')
wontfix = create(:label, title: 'wontfix')
project.labels << bug
project.labels << wontfix
login_as user
visit namespace_project_labels_path(project.namespace, project)
expect(page).to have_content('No prioritized labels yet')
page.within('.other-labels') do
first('.js-toggle-priority').click
wait_for_ajax
expect(page).not_to have_content('bug')
end
page.within('.prioritized-labels') do
expect(page).not_to have_content('No prioritized labels yet')
expect(page).to have_content('bug')
end
end
scenario 'user can unprioritize a label', js: true do
bug = create(:label, title: 'bug', priority: 1)
wontfix = create(:label, title: 'wontfix')
project.labels << bug
project.labels << wontfix
login_as user
visit namespace_project_labels_path(project.namespace, project)
expect(page).to have_content('bug')
page.within('.prioritized-labels') do
first('.js-toggle-priority').click
wait_for_ajax
expect(page).not_to have_content('bug')
end
page.within('.other-labels') do
expect(page).to have_content('bug')
expect(page).to have_content('wontfix')
end
end
scenario 'user can sort prioritized labels and persist across reloads', js: true do
bug = create(:label, title: 'bug', priority: 1)
wontfix = create(:label, title: 'wontfix', priority: 2)
project.labels << bug
project.labels << wontfix
login_as user
visit namespace_project_labels_path(project.namespace, project)
expect(page).to have_content 'bug'
expect(page).to have_content 'wontfix'
# Sort labels
find("#label_#{bug.id}").drag_to find("#label_#{wontfix.id}")
page.within('.prioritized-labels') do
expect(first('li')).to have_content('wontfix')
expect(page.all('li').last).to have_content('bug')
end
visit current_url
page.within('.prioritized-labels') do
expect(first('li')).to have_content('wontfix')
expect(page.all('li').last).to have_content('bug')
end
end
end
context 'as a guest' do
it 'can not prioritize labels' do
user = create(:user)
guest = create(:user)
project = create(:project, name: 'test', namespace: user.namespace)
create(:label, title: 'bug')
login_as guest
visit namespace_project_labels_path(project.namespace, project)
expect(page).not_to have_css('.prioritized-labels')
end
end
context 'as a non signed in user' do
it 'can not prioritize labels' do
user = create(:user)
project = create(:project, name: 'test', namespace: user.namespace)
create(:label, title: 'bug')
visit namespace_project_labels_path(project.namespace, project)
expect(page).not_to have_css('.prioritized-labels')
end
end
end
......@@ -39,6 +39,22 @@ describe Gitlab::Database, lib: true do
end
end
describe '.nulls_last_order' do
context 'when using PostgreSQL' do
before { expect(described_class).to receive(:postgresql?).and_return(true) }
it { expect(described_class.nulls_last_order('column', 'ASC')).to eq 'column ASC NULLS LAST'}
it { expect(described_class.nulls_last_order('column', 'DESC')).to eq 'column DESC NULLS LAST'}
end
context 'when using MySQL' do
before { expect(described_class).to receive(:postgresql?).and_return(false) }
it { expect(described_class.nulls_last_order('column', 'ASC')).to eq 'column IS NULL, column ASC'}
it { expect(described_class.nulls_last_order('column', 'DESC')).to eq 'column DESC'}
end
end
describe '#true_value' do
it 'returns correct value for PostgreSQL' do
expect(described_class).to receive(:postgresql?).and_return(true)
......
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