From 37bf35f0bcba28e271789542fb8c81a6c77236b6 Mon Sep 17 00:00:00 2001
From: Felipe Artur <felipefac@gmail.com>
Date: Tue, 26 Jul 2016 18:21:20 -0300
Subject: [PATCH] Todos sorting dropdown
---
CHANGELOG | 1 +
app/controllers/dashboard/todos_controller.rb | 1 +
app/finders/todos_finder.rb | 6 +-
app/models/concerns/issuable.rb | 19 ++---
app/models/concerns/sortable.rb | 14 ++++
app/models/todo.rb | 19 +++++
app/views/dashboard/todos/index.html.haml | 19 +++++
spec/features/todos/todos_sorting_spec.rb | 67 ++++++++++++++++++
spec/finders/todos_finder_spec.rb | 70 +++++++++++++++++++
9 files changed, 200 insertions(+), 16 deletions(-)
create mode 100644 spec/features/todos/todos_sorting_spec.rb
create mode 100644 spec/finders/todos_finder_spec.rb
diff --git a/CHANGELOG b/CHANGELOG
index a92c85873c477..df2301ed3263a 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -29,6 +29,7 @@ v 8.11.0 (unreleased)
- Show member roles to all users on members page
- Project.visible_to_user is instrumented again
- Fix awardable button mutuality loading spinners (ClemMakesApps)
+ - Sort todos by date and priority
- Add support for using RequestStore within Sidekiq tasks via SIDEKIQ_REQUEST_STORE env variable
- Optimize maximum user access level lookup in loading of notes
- Add "No one can push" as an option for protected branches. !5081
diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index 1243bb96d4d45..9828d39869b37 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -2,6 +2,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
before_action :find_todos, only: [:index, :destroy_all]
def index
+ @sort = params[:sort]
@todos = @todos.page(params[:page])
end
diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb
index 4fe0070552efd..37e919e6cf7e3 100644
--- a/app/finders/todos_finder.rb
+++ b/app/finders/todos_finder.rb
@@ -33,7 +33,7 @@ def execute
# the project IDs yielded by the todos query thus far
items = by_project(items)
- items.reorder(id: :desc)
+ sort(items)
end
private
@@ -106,6 +106,10 @@ def type
params[:type]
end
+ def sort(items)
+ params[:sort] ? items.sort(params[:sort]) : items.reorder(id: :desc)
+ end
+
def by_action(items)
if action?
items = items.where(action: to_action_id)
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index cbae1cd439bc3..afb5ce37c06f6 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -131,7 +131,10 @@ def sort(method, excluded_labels: [])
end
def order_labels_priority(excluded_labels: [])
- select("#{table_name}.*, (#{highest_label_priority(excluded_labels).to_sql}) AS highest_priority").
+ condition_field = "#{table_name}.id"
+ highest_priority = highest_label_priority(name, condition_field, excluded_labels: excluded_labels).to_sql
+
+ select("#{table_name}.*, (#{highest_priority}) AS highest_priority").
group(arel_table[:id]).
reorder(Gitlab::Database.nulls_last_order('highest_priority', 'ASC'))
end
@@ -159,20 +162,6 @@ def grouping_columns(sort)
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?
diff --git a/app/models/concerns/sortable.rb b/app/models/concerns/sortable.rb
index 8b47b9e0abd28..1ebecd86af9b4 100644
--- a/app/models/concerns/sortable.rb
+++ b/app/models/concerns/sortable.rb
@@ -35,5 +35,19 @@ def order_by(method)
all
end
end
+
+ private
+
+ def highest_label_priority(object_types, condition_field, excluded_labels: [])
+ query = Label.select(Label.arel_table[:priority].minimum).
+ joins(:label_links).
+ where(label_links: { target_type: object_types }).
+ where("label_links.target_id = #{condition_field}").
+ reorder(nil)
+
+ query.where.not(title: excluded_labels) if excluded_labels.present?
+
+ query
+ end
end
end
diff --git a/app/models/todo.rb b/app/models/todo.rb
index 8d7a5965aa15f..6ae9956ade5f6 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -1,4 +1,6 @@
class Todo < ActiveRecord::Base
+ include Sortable
+
ASSIGNED = 1
MENTIONED = 2
BUILD_FAILED = 3
@@ -41,6 +43,23 @@ class Todo < ActiveRecord::Base
after_save :keep_around_commit
+ class << self
+ def sort(method)
+ method == "priority" ? order_by_labels_priority : order_by(method)
+ end
+
+ # Order by priority depending on which issue/merge request the Todo belongs to
+ # Todos with highest priority first then oldest todos
+ # Need to order by created_at last because of differences on Mysql and Postgres when joining by type "Merge_request/Issue"
+ def order_by_labels_priority
+ highest_priority = highest_label_priority(["Issue", "MergeRequest"], "todos.target_id").to_sql
+
+ select("#{table_name}.*, (#{highest_priority}) AS highest_priority").
+ order(Gitlab::Database.nulls_last_order('highest_priority', 'ASC')).
+ order('todos.created_at')
+ end
+ end
+
def build_failed?
action == BUILD_FAILED
end
diff --git a/app/views/dashboard/todos/index.html.haml b/app/views/dashboard/todos/index.html.haml
index 4e340b6ec16f3..d320d3bcc1e31 100644
--- a/app/views/dashboard/todos/index.html.haml
+++ b/app/views/dashboard/todos/index.html.haml
@@ -43,6 +43,25 @@
class: 'select2 trigger-submit', include_blank: true,
data: {placeholder: 'Action'})
+ .pull-right
+ .dropdown.inline.prepend-left-10
+ %button.dropdown-toggle.btn{type: 'button', 'data-toggle' => 'dropdown'}
+ %span.light
+ - if @sort.present?
+ = sort_options_hash[@sort]
+ - else
+ = sort_title_recently_created
+ %b.caret
+ %ul.dropdown-menu.dropdown-menu-align-right.dropdown-menu-sort
+ %li
+ = link_to todos_filter_path(sort: sort_value_priority) do
+ = sort_title_priority
+ = link_to todos_filter_path(sort: sort_value_recently_created) do
+ = sort_title_recently_created
+ = link_to todos_filter_path(sort: sort_value_oldest_created) do
+ = sort_title_oldest_created
+
+
.prepend-top-default
- if @todos.any?
.js-todos-options{ data: {per_page: @todos.limit_value, current_page: @todos.current_page, total_pages: @todos.total_pages} }
diff --git a/spec/features/todos/todos_sorting_spec.rb b/spec/features/todos/todos_sorting_spec.rb
new file mode 100644
index 0000000000000..e74a51acede8b
--- /dev/null
+++ b/spec/features/todos/todos_sorting_spec.rb
@@ -0,0 +1,67 @@
+require 'spec_helper'
+
+describe "Dashboard > User sorts todos", feature: true do
+ let(:user) { create(:user) }
+ let(:project) { create(:empty_project) }
+
+ 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(:issue_1) { create(:issue, title: 'issue_1', project: project) }
+ let(:issue_2) { create(:issue, title: 'issue_2', project: project) }
+ let(:issue_3) { create(:issue, title: 'issue_3', project: project) }
+ let(:issue_4) { create(:issue, title: 'issue_4', project: project) }
+
+ let!(:merge_request_1) { create(:merge_request, source_project: project, title: "merge_request_1") }
+
+ before do
+ create(:todo, user: user, project: project, target: issue_4, created_at: 5.hours.ago)
+ create(:todo, user: user, project: project, target: issue_2, created_at: 4.hours.ago)
+ create(:todo, user: user, project: project, target: issue_3, created_at: 3.hours.ago)
+ create(:todo, user: user, project: project, target: issue_1, created_at: 2.hours.ago)
+ create(:todo, user: user, project: project, target: merge_request_1, created_at: 1.hour.ago)
+
+ merge_request_1.labels << label_1
+ issue_3.labels << label_1
+ issue_2.labels << label_3
+ issue_1.labels << label_2
+
+ project.team << [user, :developer]
+ login_as(user)
+ visit dashboard_todos_path
+ end
+
+ it "sorts with oldest created todos first" do
+ click_link "Last created"
+
+ results_list = page.find('.todos-list')
+ expect(results_list.all('p')[0]).to have_content("merge_request_1")
+ expect(results_list.all('p')[1]).to have_content("issue_1")
+ expect(results_list.all('p')[2]).to have_content("issue_3")
+ expect(results_list.all('p')[3]).to have_content("issue_2")
+ expect(results_list.all('p')[4]).to have_content("issue_4")
+ end
+
+ it "sorts with newest created todos first" do
+ click_link "Oldest created"
+
+ results_list = page.find('.todos-list')
+ expect(results_list.all('p')[0]).to have_content("issue_4")
+ expect(results_list.all('p')[1]).to have_content("issue_2")
+ expect(results_list.all('p')[2]).to have_content("issue_3")
+ expect(results_list.all('p')[3]).to have_content("issue_1")
+ expect(results_list.all('p')[4]).to have_content("merge_request_1")
+ end
+
+ it "sorts by priority" do
+ click_link "Priority"
+
+ results_list = page.find('.todos-list')
+ expect(results_list.all('p')[0]).to have_content("issue_3")
+ expect(results_list.all('p')[1]).to have_content("merge_request_1")
+ expect(results_list.all('p')[2]).to have_content("issue_1")
+ expect(results_list.all('p')[3]).to have_content("issue_2")
+ expect(results_list.all('p')[4]).to have_content("issue_4")
+ end
+end
diff --git a/spec/finders/todos_finder_spec.rb b/spec/finders/todos_finder_spec.rb
new file mode 100644
index 0000000000000..f7e7e733cf71f
--- /dev/null
+++ b/spec/finders/todos_finder_spec.rb
@@ -0,0 +1,70 @@
+require 'spec_helper'
+
+describe TodosFinder do
+ describe '#execute' do
+ let(:user) { create(:user) }
+ let(:project) { create(:empty_project) }
+ let(:finder) { described_class }
+
+ before { project.team << [user, :developer] }
+
+ describe '#sort' do
+ context 'by date' do
+ let!(:todo1) { create(:todo, user: user, project: project) }
+ let!(:todo2) { create(:todo, user: user, project: project) }
+ let!(:todo3) { create(:todo, user: user, project: project) }
+
+ it 'sorts with oldest created first' do
+ todos = finder.new(user, { sort: 'id_asc' }).execute
+
+ expect(todos.first).to eq(todo1)
+ expect(todos.second).to eq(todo2)
+ expect(todos.third).to eq(todo3)
+ end
+
+ it 'sorts with newest created first' do
+ todos = finder.new(user, { sort: 'id_desc' }).execute
+
+ expect(todos.first).to eq(todo3)
+ expect(todos.second).to eq(todo2)
+ expect(todos.third).to eq(todo1)
+ end
+ end
+
+ it "sorts by priority" do
+ label_1 = create(:label, title: 'label_1', project: project, priority: 1)
+ label_2 = create(:label, title: 'label_2', project: project, priority: 2)
+ label_3 = create(:label, title: 'label_3', project: project, priority: 3)
+
+ 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)
+ merge_request_1 = create(:merge_request, source_project: project)
+
+ merge_request_1.labels << label_1
+
+ # Covers the case where Todo has more than one label
+ issue_3.labels << label_1
+ issue_3.labels << label_3
+
+ issue_2.labels << label_3
+ issue_1.labels << label_2
+
+ todo_1 = create(:todo, user: user, project: project, target: issue_4)
+ todo_2 = create(:todo, user: user, project: project, target: issue_2)
+ todo_3 = create(:todo, user: user, project: project, target: issue_3, created_at: 2.hours.ago)
+ todo_4 = create(:todo, user: user, project: project, target: issue_1)
+ todo_5 = create(:todo, user: user, project: project, target: merge_request_1, created_at: 1.hour.ago)
+
+ todos = finder.new(user, { sort: 'priority' }).execute
+
+ expect(todos.first).to eq(todo_3)
+ expect(todos.second).to eq(todo_5)
+ expect(todos.third).to eq(todo_4)
+ expect(todos.fourth).to eq(todo_2)
+ expect(todos.fifth).to eq(todo_1)
+ end
+ end
+ end
+end
--
GitLab