From 631eed028bffc55f0a80b72ab3598bc73e49272b Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Mon, 5 Mar 2018 16:41:48 +0000
Subject: [PATCH] Remove default scope from todos
This was causing todo priority sorting to fail.
---
app/finders/todos_finder.rb | 4 +---
app/models/todo.rb | 14 ++++++++------
spec/finders/todos_finder_spec.rb | 27 +++++++++++++++------------
3 files changed, 24 insertions(+), 21 deletions(-)
diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb
index 47c8b9b60ed9..150f4c7688b6 100644
--- a/app/finders/todos_finder.rb
+++ b/app/finders/todos_finder.rb
@@ -150,9 +150,7 @@ def by_project(items)
if project?
items.where(project: project)
else
- projects = Project
- .public_or_visible_to_user(current_user)
- .order_id_desc
+ projects = Project.public_or_visible_to_user(current_user)
items.joins(:project).merge(projects)
end
diff --git a/app/models/todo.rb b/app/models/todo.rb
index bb5965e20eb2..8afacd188e0e 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -32,8 +32,6 @@ class Todo < ActiveRecord::Base
validates :target_id, presence: true, unless: :for_commit?
validates :commit_id, presence: true, if: :for_commit?
- default_scope { reorder(id: :desc) }
-
scope :pending, -> { with_state(:pending) }
scope :done, -> { with_state(:done) }
@@ -53,10 +51,14 @@ class << self
# milestones, but still show something if the user has a URL with that
# selected.
def sort(method)
- case method.to_s
- when 'priority', 'label_priority' then order_by_labels_priority
- else order_by(method)
- end
+ sorted =
+ case method.to_s
+ when 'priority', 'label_priority' then order_by_labels_priority
+ else order_by(method)
+ end
+
+ # Break ties with the ID column for pagination
+ sorted.order(id: :desc)
end
# Order by priority depending on which issue/merge request the Todo belongs to
diff --git a/spec/finders/todos_finder_spec.rb b/spec/finders/todos_finder_spec.rb
index 90eb0fe21e4f..9747b9402a7f 100644
--- a/spec/finders/todos_finder_spec.rb
+++ b/spec/finders/todos_finder_spec.rb
@@ -2,12 +2,13 @@
describe TodosFinder do
describe '#execute' do
- let(:user) { create(:user) }
- let(:project) { create(:project) }
- let(:finder) { described_class }
+ let(:user) { create(:user) }
+ let(:group) { create(:group) }
+ let(:project) { create(:project, namespace: group) }
+ let(:finder) { described_class }
before do
- project.add_developer(user)
+ group.add_developer(user)
end
describe '#sort' do
@@ -34,17 +35,20 @@
end
it "sorts by priority" do
+ project_2 = create(:project)
+
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)
+ label_1_2 = create(:label, title: 'label_1', project: project_2, priority: 1)
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 = create(:merge_request, source_project: project_2)
- merge_request_1.labels << label_1
+ merge_request_1.labels << label_1_2
# Covers the case where Todo has more than one label
issue_3.labels << label_1
@@ -57,15 +61,14 @@
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)
+ todo_5 = create(:todo, user: user, project: project_2, target: merge_request_1, created_at: 1.hour.ago)
+
+ project_2.add_developer(user)
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)
+ puts todos.to_sql
+ expect(todos).to eq([todo_3, todo_5, todo_4, todo_2, todo_1])
end
end
end
--
GitLab