From 996240a4895abddf069eb6fc195bacee5e344e16 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Wed, 27 Apr 2016 12:35:30 -0300 Subject: Fix issues/MRs filter when ordering by milestone due date --- app/finders/issuable_finder.rb | 4 ++-- app/models/concerns/issuable.rb | 4 +++- spec/models/concerns/issuable_spec.rb | 41 ++++++++++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 5849e00662b..0e3dfa74ac4 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -253,9 +253,9 @@ class IssuableFinder items = items.where(milestone_id: [-1, nil]) elsif filter_by_upcoming_milestone? upcoming_ids = Milestone.upcoming_ids_by_projects(projects) - items = items.joins(:milestone).where(milestone_id: upcoming_ids) + items = items.left_joins_milestones.where(milestone_id: upcoming_ids) else - items = items.joins(:milestone).where(milestones: { title: params[:milestone_title] }) + items = items.left_joins_milestones.where(milestones: { title: params[:milestone_title] }) if projects items = items.where(milestones: { project_id: projects }) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index c1248b53031..db73588a948 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -35,10 +35,12 @@ module Issuable scope :only_opened, -> { with_state(:opened) } scope :only_reopened, -> { with_state(:reopened) } scope :closed, -> { with_state(:closed) } + + scope :left_joins_milestones, -> { joins("LEFT OUTER JOIN milestones ON #{table_name}.milestone_id = milestones.id") } scope :order_milestone_due_desc, -> { outer_join_milestone.reorder('milestones.due_date IS NULL ASC, milestones.due_date DESC, milestones.id DESC') } scope :order_milestone_due_asc, -> { outer_join_milestone.reorder('milestones.due_date IS NULL ASC, milestones.due_date ASC, milestones.id ASC') } - scope :without_label, -> { joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{name}' AND label_links.target_id = #{table_name}.id").where(label_links: { id: nil }) } + scope :without_label, -> { joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{name}' AND label_links.target_id = #{table_name}.id").where(label_links: { id: nil }) } scope :join_project, -> { joins(:project) } scope :references_project, -> { references(:project) } scope :non_archived, -> { join_project.where(projects: { archived: false }) } diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 4a4cd093435..fa06a6f7ecf 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Issue, "Issuable" do - let(:issue) { create(:issue) } + let!(:issue) { create(:issue) } let(:user) { create(:user) } describe "Associations" do @@ -114,6 +114,45 @@ describe Issue, "Issuable" do end end + describe "#sort" do + #Correct order is: + #Issues/MRs with milestones ordered by date + #Issues/MRs with milestones without dates + #Issues/MRs without milestones + let(:project) { issue.project } + let!(:early_milestone) { create(:milestone, project: project, due_date: 10.days.from_now) } + let!(:late_milestone) { create(:milestone, project: project, due_date: 30.days.from_now) } + let!(:issue1) { create(:issue, project: project, milestone: early_milestone) } + let!(:issue2) { create(:issue, project: project, milestone: late_milestone) } + let!(:issue3) { create(:issue, project: project) } + + + context "milestone due later" do + subject { Issue.where(project_id: project.id).order_milestone_due_desc } + before { @issues = subject } + + it "puts issues with nil values at the end of collection" do + expect(@issues.first).to eq(issue2) + expect(@issues.second).to eq(issue1) + expect(@issues.third).to eq(issue) + expect(@issues.fourth).to eq(issue3) + end + end + + context "milestone due soon" do + subject { Issue.where(project_id: project.id).order_milestone_due_asc } + before { @issues = subject } + + it "puts issues with nil values at the end of collection" do + expect(@issues.first).to eq(issue1) + expect(@issues.second).to eq(issue2) + expect(@issues.third).to eq(issue) + expect(@issues.fourth).to eq(issue3) + end + end + end + + describe '#subscribed?' do context 'user is not a participant in the issue' do before { allow(issue).to receive(:participants).with(user).and_return([]) } -- cgit v1.2.1 From 673196e0a866683aecb9332f4554e72985e2ac87 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Wed, 27 Apr 2016 17:31:57 -0300 Subject: Fix ordering for Mysql --- app/models/concerns/issuable.rb | 4 ++-- spec/features/issues/filter_by_milestone_spec.rb | 8 ++++---- spec/features/merge_requests/filter_by_milestone_spec.rb | 8 ++++---- spec/models/concerns/issuable_spec.rb | 4 +++- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index db73588a948..4b21cf02919 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -37,8 +37,8 @@ module Issuable scope :closed, -> { with_state(:closed) } scope :left_joins_milestones, -> { joins("LEFT OUTER JOIN milestones ON #{table_name}.milestone_id = milestones.id") } - scope :order_milestone_due_desc, -> { outer_join_milestone.reorder('milestones.due_date IS NULL ASC, milestones.due_date DESC, milestones.id DESC') } - scope :order_milestone_due_asc, -> { outer_join_milestone.reorder('milestones.due_date IS NULL ASC, milestones.due_date ASC, milestones.id ASC') } + scope :order_milestone_due_desc, -> { left_joins_milestones.reorder('CASE WHEN milestones.due_date IS NULL then 0 ELSE 1 END DESC, CASE WHEN milestones.id IS NULL then 0 ELSE 1 END DESC, milestones.due_date DESC') } + scope :order_milestone_due_asc, -> { left_joins_milestones.reorder('CASE WHEN milestones.due_date IS NULL then 1 ELSE 0 END ASC, CASE WHEN milestones.id IS NULL then 1 ELSE 0 END ASC, milestones.due_date ASC') } scope :without_label, -> { joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{name}' AND label_links.target_id = #{table_name}.id").where(label_links: { id: nil }) } scope :join_project, -> { joins(:project) } diff --git a/spec/features/issues/filter_by_milestone_spec.rb b/spec/features/issues/filter_by_milestone_spec.rb index 99445185893..666993d19d4 100644 --- a/spec/features/issues/filter_by_milestone_spec.rb +++ b/spec/features/issues/filter_by_milestone_spec.rb @@ -15,14 +15,14 @@ feature 'Issue filtering by Milestone', feature: true do end context 'filters by upcoming milestone', js: true do - it 'should not show issues with no expiry' do + it 'should show issues with no expiry' do create(:issue, project: project) create(:issue, project: project, milestone: milestone) visit_issues(project) filter_by_milestone(Milestone::Upcoming.title) - expect(page).to have_css('.issue', count: 0) + expect(page).to have_css('.issue', count: 2) end it 'should show issues in future' do @@ -36,7 +36,7 @@ feature 'Issue filtering by Milestone', feature: true do expect(page).to have_css('.issue', count: 1) end - it 'should not show issues in past' do + it 'should show issues in past' do milestone = create(:milestone, project: project, due_date: Date.yesterday) create(:issue, project: project) create(:issue, project: project, milestone: milestone) @@ -44,7 +44,7 @@ feature 'Issue filtering by Milestone', feature: true do visit_issues(project) filter_by_milestone(Milestone::Upcoming.title) - expect(page).to have_css('.issue', count: 0) + expect(page).to have_css('.issue', count: 2) end end diff --git a/spec/features/merge_requests/filter_by_milestone_spec.rb b/spec/features/merge_requests/filter_by_milestone_spec.rb index e3ecd60a5f3..db3efca21c8 100644 --- a/spec/features/merge_requests/filter_by_milestone_spec.rb +++ b/spec/features/merge_requests/filter_by_milestone_spec.rb @@ -21,14 +21,14 @@ feature 'Merge Request filtering by Milestone', feature: true do end context 'filters by upcoming milestone', js: true do - it 'should not show issues with no expiry' do + it 'should show issues with no expiry' do create(:merge_request, :with_diffs, source_project: project) create(:merge_request, :simple, source_project: project, milestone: milestone) visit_merge_requests(project) filter_by_milestone(Milestone::Upcoming.title) - expect(page).to have_css('.merge-request', count: 0) + expect(page).to have_css('.merge-request', count: 2) end it 'should show issues in future' do @@ -42,7 +42,7 @@ feature 'Merge Request filtering by Milestone', feature: true do expect(page).to have_css('.merge-request', count: 1) end - it 'should not show issues in past' do + it 'should show issues in past' do milestone = create(:milestone, project: project, due_date: Date.yesterday) create(:merge_request, :with_diffs, source_project: project) create(:merge_request, :simple, source_project: project, milestone: milestone) @@ -50,7 +50,7 @@ feature 'Merge Request filtering by Milestone', feature: true do visit_merge_requests(project) filter_by_milestone(Milestone::Upcoming.title) - expect(page).to have_css('.merge-request', count: 0) + expect(page).to have_css('.merge-request', count: 2) end end diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index fa06a6f7ecf..2bb770df4fd 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Issue, "Issuable" do - let!(:issue) { create(:issue) } + let(:issue) { create(:issue) } let(:user) { create(:user) } describe "Associations" do @@ -119,6 +119,8 @@ describe Issue, "Issuable" do #Issues/MRs with milestones ordered by date #Issues/MRs with milestones without dates #Issues/MRs without milestones + + let!(:issue) { create(:issue) } let(:project) { issue.project } let!(:early_milestone) { create(:milestone, project: project, due_date: 10.days.from_now) } let!(:late_milestone) { create(:milestone, project: project, due_date: 30.days.from_now) } -- cgit v1.2.1 From 3b2223f11676831f9e995095de13926e1a39b462 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Thu, 5 May 2016 16:23:51 -0300 Subject: improve ordering sql for milestones --- app/models/concerns/issuable.rb | 4 +-- spec/models/concerns/issuable_spec.rb | 53 ++++++++++++++--------------------- 2 files changed, 23 insertions(+), 34 deletions(-) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 4b21cf02919..ce73fe7035e 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -37,8 +37,8 @@ module Issuable scope :closed, -> { with_state(:closed) } scope :left_joins_milestones, -> { joins("LEFT OUTER JOIN milestones ON #{table_name}.milestone_id = milestones.id") } - scope :order_milestone_due_desc, -> { left_joins_milestones.reorder('CASE WHEN milestones.due_date IS NULL then 0 ELSE 1 END DESC, CASE WHEN milestones.id IS NULL then 0 ELSE 1 END DESC, milestones.due_date DESC') } - scope :order_milestone_due_asc, -> { left_joins_milestones.reorder('CASE WHEN milestones.due_date IS NULL then 1 ELSE 0 END ASC, CASE WHEN milestones.id IS NULL then 1 ELSE 0 END ASC, milestones.due_date ASC') } + scope :order_milestone_due_desc, -> { left_joins_milestones.reorder('milestones.due_date IS NULL, milestones.id IS NULL, milestones.due_date DESC') } + scope :order_milestone_due_asc, -> { left_joins_milestones.reorder('milestones.due_date IS NULL, milestones.id IS NULL, milestones.due_date ASC') } scope :without_label, -> { joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{name}' AND label_links.target_id = #{table_name}.id").where(label_links: { id: nil }) } scope :join_project, -> { joins(:project) } diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 2bb770df4fd..56a4313e8af 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -115,41 +115,30 @@ describe Issue, "Issuable" do end describe "#sort" do - #Correct order is: - #Issues/MRs with milestones ordered by date - #Issues/MRs with milestones without dates - #Issues/MRs without milestones + let(:project) { build_stubbed(:empty_project) } - let!(:issue) { create(:issue) } - let(:project) { issue.project } - let!(:early_milestone) { create(:milestone, project: project, due_date: 10.days.from_now) } - let!(:late_milestone) { create(:milestone, project: project, due_date: 30.days.from_now) } - let!(:issue1) { create(:issue, project: project, milestone: early_milestone) } - let!(:issue2) { create(:issue, project: project, milestone: late_milestone) } - let!(:issue3) { create(:issue, project: project) } - - - context "milestone due later" do - subject { Issue.where(project_id: project.id).order_milestone_due_desc } - before { @issues = subject } - - it "puts issues with nil values at the end of collection" do - expect(@issues.first).to eq(issue2) - expect(@issues.second).to eq(issue1) - expect(@issues.third).to eq(issue) - expect(@issues.fourth).to eq(issue3) - end - end + context "by milestone due date" do + #Correct order is: + #Issues/MRs with milestones ordered by date + #Issues/MRs with milestones without dates + #Issues/MRs without milestones - context "milestone due soon" do - subject { Issue.where(project_id: project.id).order_milestone_due_asc } - before { @issues = subject } + let!(:issue) { create(:issue, project: project) } + let!(:early_milestone) { create(:milestone, project: project, due_date: 10.days.from_now) } + let!(:late_milestone) { create(:milestone, project: project, due_date: 30.days.from_now) } + let!(:issue1) { create(:issue, project: project, milestone: early_milestone) } + let!(:issue2) { create(:issue, project: project, milestone: late_milestone) } + let!(:issue3) { create(:issue, project: project) } + + + it "sorts desc" do + issues = project.issues.sort('milestone_due_desc') + expect(issues).to match_array([issue2, issue1, issue, issue3]) + end - it "puts issues with nil values at the end of collection" do - expect(@issues.first).to eq(issue1) - expect(@issues.second).to eq(issue2) - expect(@issues.third).to eq(issue) - expect(@issues.fourth).to eq(issue3) + it "sorts asc" do + issues = project.issues.sort('milestone_due_asc') + expect(issues).to match_array([issue1, issue2, issue, issue3]) end end end -- cgit v1.2.1 From 45e516b8174c174951a97438c83fd659782e2347 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Fri, 6 May 2016 12:42:29 -0300 Subject: Fix bug when ordering by milestone due date and filtering by milestone --- app/models/concerns/issuable.rb | 1 - spec/features/issues_spec.rb | 2 -- spec/models/concerns/issuable_spec.rb | 1 - 3 files changed, 4 deletions(-) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index ce73fe7035e..d34308204e5 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -44,7 +44,6 @@ module Issuable scope :join_project, -> { joins(:project) } scope :references_project, -> { references(:project) } scope :non_archived, -> { join_project.where(projects: { archived: false }) } - scope :outer_join_milestone, -> { joins("LEFT OUTER JOIN milestones ON milestones.id = #{table_name}.milestone_id") } delegate :name, :email, diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb index 749ee01890c..a4de7c004c7 100644 --- a/spec/features/issues_spec.rb +++ b/spec/features/issues_spec.rb @@ -185,14 +185,12 @@ describe 'Issues', feature: true do visit namespace_project_issues_path(project.namespace, project, sort: sort_value_recently_created) expect(first_issue).to include('baz') - expect(last_issue).to include('foo') end it 'sorts by oldest' do visit namespace_project_issues_path(project.namespace, project, sort: sort_value_oldest_created) expect(first_issue).to include('foo') - expect(last_issue).to include('baz') end it 'sorts by most recently updated' do diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 56a4313e8af..c7e98e8309b 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -130,7 +130,6 @@ describe Issue, "Issuable" do let!(:issue2) { create(:issue, project: project, milestone: late_milestone) } let!(:issue3) { create(:issue, project: project) } - it "sorts desc" do issues = project.issues.sort('milestone_due_desc') expect(issues).to match_array([issue2, issue1, issue, issue3]) -- cgit v1.2.1 From dac9b4212d700272160dff1e0a86c20cb72afedd Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Thu, 19 May 2016 18:30:38 -0400 Subject: Fix outer join when filtering milestones --- CHANGELOG | 1 + app/finders/issuable_finder.rb | 4 ++-- app/models/concerns/issuable.rb | 2 ++ spec/features/issues/filter_by_milestone_spec.rb | 8 ++++---- spec/features/issues_spec.rb | 2 ++ spec/features/merge_requests/filter_by_milestone_spec.rb | 8 ++++---- 6 files changed, 15 insertions(+), 10 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index a79c3048b79..59b14de68c7 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.9.0 (unreleased) - Redesign navigation for project pages - Use gitlab-shell v3.0.0 + - Fix issues filter when ordering by milestone v 8.8.2 (unreleased) - Fix Error 500 when accessing application settings due to nil disabled OAuth sign-in sources diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 0e3dfa74ac4..8ed3ccf1c02 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -250,12 +250,12 @@ class IssuableFinder def by_milestone(items) if milestones? if filter_by_no_milestone? - items = items.where(milestone_id: [-1, nil]) + items = items.left_joins_milestones.where(milestone_id: [-1, nil]) elsif filter_by_upcoming_milestone? upcoming_ids = Milestone.upcoming_ids_by_projects(projects) items = items.left_joins_milestones.where(milestone_id: upcoming_ids) else - items = items.left_joins_milestones.where(milestones: { title: params[:milestone_title] }) + items = items.with_milestone(params[:milestone_title]) if projects items = items.where(milestones: { project_id: projects }) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index d34308204e5..91315b3459f 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -31,6 +31,7 @@ module Issuable scope :unassigned, -> { where("assignee_id IS NULL") } scope :of_projects, ->(ids) { where(project_id: ids) } scope :of_milestones, ->(ids) { where(milestone_id: ids) } + scope :with_milestone, ->(title) { left_joins_milestones.where(milestones: { title: title }) } scope :opened, -> { with_state(:opened, :reopened) } scope :only_opened, -> { with_state(:opened) } scope :only_reopened, -> { with_state(:reopened) } @@ -45,6 +46,7 @@ module Issuable scope :references_project, -> { references(:project) } scope :non_archived, -> { join_project.where(projects: { archived: false }) } + delegate :name, :email, to: :author, diff --git a/spec/features/issues/filter_by_milestone_spec.rb b/spec/features/issues/filter_by_milestone_spec.rb index 666993d19d4..99445185893 100644 --- a/spec/features/issues/filter_by_milestone_spec.rb +++ b/spec/features/issues/filter_by_milestone_spec.rb @@ -15,14 +15,14 @@ feature 'Issue filtering by Milestone', feature: true do end context 'filters by upcoming milestone', js: true do - it 'should show issues with no expiry' do + it 'should not show issues with no expiry' do create(:issue, project: project) create(:issue, project: project, milestone: milestone) visit_issues(project) filter_by_milestone(Milestone::Upcoming.title) - expect(page).to have_css('.issue', count: 2) + expect(page).to have_css('.issue', count: 0) end it 'should show issues in future' do @@ -36,7 +36,7 @@ feature 'Issue filtering by Milestone', feature: true do expect(page).to have_css('.issue', count: 1) end - it 'should show issues in past' do + it 'should not show issues in past' do milestone = create(:milestone, project: project, due_date: Date.yesterday) create(:issue, project: project) create(:issue, project: project, milestone: milestone) @@ -44,7 +44,7 @@ feature 'Issue filtering by Milestone', feature: true do visit_issues(project) filter_by_milestone(Milestone::Upcoming.title) - expect(page).to have_css('.issue', count: 2) + expect(page).to have_css('.issue', count: 0) end end diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb index a4de7c004c7..749ee01890c 100644 --- a/spec/features/issues_spec.rb +++ b/spec/features/issues_spec.rb @@ -185,12 +185,14 @@ describe 'Issues', feature: true do visit namespace_project_issues_path(project.namespace, project, sort: sort_value_recently_created) expect(first_issue).to include('baz') + expect(last_issue).to include('foo') end it 'sorts by oldest' do visit namespace_project_issues_path(project.namespace, project, sort: sort_value_oldest_created) expect(first_issue).to include('foo') + expect(last_issue).to include('baz') end it 'sorts by most recently updated' do diff --git a/spec/features/merge_requests/filter_by_milestone_spec.rb b/spec/features/merge_requests/filter_by_milestone_spec.rb index db3efca21c8..e3ecd60a5f3 100644 --- a/spec/features/merge_requests/filter_by_milestone_spec.rb +++ b/spec/features/merge_requests/filter_by_milestone_spec.rb @@ -21,14 +21,14 @@ feature 'Merge Request filtering by Milestone', feature: true do end context 'filters by upcoming milestone', js: true do - it 'should show issues with no expiry' do + it 'should not show issues with no expiry' do create(:merge_request, :with_diffs, source_project: project) create(:merge_request, :simple, source_project: project, milestone: milestone) visit_merge_requests(project) filter_by_milestone(Milestone::Upcoming.title) - expect(page).to have_css('.merge-request', count: 2) + expect(page).to have_css('.merge-request', count: 0) end it 'should show issues in future' do @@ -42,7 +42,7 @@ feature 'Merge Request filtering by Milestone', feature: true do expect(page).to have_css('.merge-request', count: 1) end - it 'should show issues in past' do + it 'should not show issues in past' do milestone = create(:milestone, project: project, due_date: Date.yesterday) create(:merge_request, :with_diffs, source_project: project) create(:merge_request, :simple, source_project: project, milestone: milestone) @@ -50,7 +50,7 @@ feature 'Merge Request filtering by Milestone', feature: true do visit_merge_requests(project) filter_by_milestone(Milestone::Upcoming.title) - expect(page).to have_css('.merge-request', count: 2) + expect(page).to have_css('.merge-request', count: 0) end end -- cgit v1.2.1