Address MR feedback

Signed-off-by: Rémy Coutable <remy@rymai.me>

CHANGELOG

@@ -19,6 +19,7 @@ v 8.7.0 (unreleased)
   - Load award emoji images separately unless opening the full picker. Saves several hundred KBs of data for most pages. (Connor Shea)
   - Do not include award_emojis in issue and merge_request comment_count !3610 (Lucas Charles)
   - Restrict user profiles when public visibility level is restricted.
+  - Add ability set due date to issues, sort and filter issues by due date (Mehmet Beydogan)
   - All images in discussions and wikis now link to their source files !3464 (Connor Shea).
   - Return status code 303 after a branch DELETE operation to avoid project deletion (Stan Hu)
   - Add setting for customizing the list of trusted proxies !3524
@@ -188,12 +189,6 @@ v 8.6.1
   - Fixes issue with assign milestone not loading milestone list. !3346
   - Fix an issue causing the Dashboard/Milestones page to be blank. !3348
 
-v 8.6.0
-  - Add ability to move issue to another project
-  - Prevent tokens in the import URL to be showed by the UI
-v 8.7.0(unreleased)
-  - Add ability set due date to issues, sort and filter issues by due date(Mehmet Beydogan)
-
 v 8.6.0 (unreleased)
   - Fix bug where wrong commit ID was being used in a merge request diff to show old image (Stan Hu)
   - Add confidential issues
@@ -271,7 +266,6 @@ v 8.5.7
 
 v 8.5.6
   - Obtain a lease before querying LDAP
-  - Add ability set due date to issues, sort and filter issues by due date
 
 v 8.5.5
   - Ensure removing a project removes associated Todo entries

app/finders/issuable_finder.rb

@@ -122,7 +122,7 @@ class IssuableFinder
   end
 
   def filter_by_overdue?
-    due_date? && params[:due_date] == Issue::OverDue.name
+    due_date? && params[:due_date] == Issue::Overdue.name
   end
 
   def filter_by_due_this_week?
@@ -305,17 +305,20 @@ class IssuableFinder
   end
 
   def by_due_date(items)
+    return items unless klass.column_names.include?('due_date')
+
     if due_date?
       if filter_by_no_due_date?
         items = items.without_due_date
       elsif filter_by_overdue?
-        items = items.overdue
+        items = items.due_before(Date.today)
       elsif filter_by_due_this_week?
-        items = items.due_between(Date.today.beginning_of_week, Date.today.end_of_week + 1.day)
+        items = items.due_between(Date.today.beginning_of_week, Date.today.end_of_week)
       elsif filter_by_due_this_month?
-        items = items.due_between(Date.today.beginning_of_month, Date.today.end_of_month + 1.day)
+        items = items.due_between(Date.today.beginning_of_month, Date.today.end_of_month)
       end
     end
+
     items
   end
 

app/helpers/issues_helper.rb

@@ -178,13 +178,12 @@ module IssuesHelper
       Issue::NoDueDate,
       Issue::DueThisWeek,
       Issue::DueThisMonth,
-        Issue::OverDue
+      Issue::Overdue
     ]
 
     options_from_collection_for_select(options, 'name', 'title', params[:due_date])
   end
 
-
   # Required for Banzai::Filter::IssueReferenceFilter
   module_function :url_for_issue
 end

app/models/concerns/issuable.rb

@@ -39,10 +39,10 @@ module Issuable
     scope :order_milestone_due_asc, -> { joins(:milestone).reorder('milestones.due_date ASC, milestones.id ASC') }
     scope :with_label, ->(title) { joins(:labels).where(labels: { title: title }) }
     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_due_date, ->{ where("issues.due_date IS NULL")}
-    scope :due_before, ->(date){ where("issues.due_date IS NOT NULL AND issues.due_date < ?", date)}
-    scope :due_between, ->(from_date, to_date){ where("issues.due_date >= ?", from_date).due_before(to_date) }
-    scope :overdue, ->{ where("issues.due_date < ?", Date.today)}
+
+    scope :without_due_date, -> { where('issues.due_date IS NULL') }
+    scope :due_before, ->(date) { where('issues.due_date < ?', date) }
+    scope :due_between, ->(from_date, to_date) { where('issues.due_date >= ?', from_date).where('issues.due_date <= ?', to_date) }
 
     scope :join_project, -> { joins(:project) }
     scope :references_project, -> { references(:project) }

app/models/concerns/sortable.rb

@@ -33,8 +33,8 @@ module Sortable
       when 'created_desc' then order_created_desc
       when 'id_desc' then order_id_desc
       when 'id_asc' then order_id_asc
-      when 'due_date_asc' then order_due_date_asc
-      when 'due_date_desc' then order_due_date_desc
+      when 'due_date_asc', 'due_date_desc'
+        column_names.include?('due_date') ? send("order_#{method}") : all
       else
         all
       end

app/models/issue.rb

@@ -28,12 +28,12 @@ class Issue < ActiveRecord::Base
   include Sortable
   include Taskable
 
-  DueDateStruct = Struct.new(:title, :name)
-  NoDueDate = DueDateStruct.new('No Due Date', '0')
-  AnyDueDate = DueDateStruct.new('Any Due Date', '')
-  OverDue = DueDateStruct.new('Overdue', 'overdue')
-  DueThisWeek = DueDateStruct.new('Due This Week', 'week')
-  DueThisMonth = DueDateStruct.new('Due This Month', 'month')
+  DueDateStruct = Struct.new(:title, :name).freeze
+  NoDueDate     = DueDateStruct.new('No Due Date', '0').freeze
+  AnyDueDate    = DueDateStruct.new('Any Due Date', '').freeze
+  Overdue       = DueDateStruct.new('Overdue', 'overdue').freeze
+  DueThisWeek   = DueDateStruct.new('Due This Week', 'week').freeze
+  DueThisMonth  = DueDateStruct.new('Due This Month', 'month').freeze
 
   ActsAsTaggableOn.strict_case_match = true
 
@@ -178,10 +178,6 @@ class Issue < ActiveRecord::Base
   end
 
   def overdue?
-    if due_date
-      due_date.past?
-    else
-      false
-    end
+    due_date.try(:past?) || false
   end
 end

app/views/shared/_sort_dropdown.html.haml

@@ -20,10 +20,11 @@
         = sort_title_milestone_soon
       = link_to page_filter_path(sort: sort_value_milestone_later) do
         = sort_title_milestone_later
+      - if controller.controller_name == 'issues' || controller.action_name == 'issues'
         = link_to page_filter_path(sort: sort_value_due_date_soon) do
-        = sort_title_due_date_soon if @issues
+          = sort_title_due_date_soon
         = link_to page_filter_path(sort: sort_value_due_date_later) do
-        = sort_title_due_date_later if @issues
+          = sort_title_due_date_later
       = link_to page_filter_path(sort: sort_value_upvotes) do
         = sort_title_upvotes
       = link_to page_filter_path(sort: sort_value_downvotes) do

app/views/shared/issuable/_sidebar.html.haml

@@ -58,7 +58,7 @@
             - if issuable.milestone
               = issuable.milestone.title
             - else
-              No
+              None
         .title.hide-collapsed
           Milestone
           = icon('spinner spin', class: 'block-loading')
@@ -74,17 +74,15 @@
         .selectbox.hide-collapsed
           = f.hidden_field 'milestone_id', value: issuable.milestone_id, id: nil
           = dropdown_tag('Milestone', options: { title: 'Assign milestone', toggle_class: 'js-milestone-select js-extra-options', filter: true, dropdown_class: 'dropdown-menu-selectable', placeholder: 'Search milestones', data: { show_no: true, field_name: "#{issuable.to_ability_name}[milestone_id]", project_id: @project.id, issuable_id: issuable.id, milestones: namespace_project_milestones_path(@project.namespace, @project, :json), ability_name: issuable.to_ability_name, issue_update: issuable_json_path(issuable), use_id: true }})
-      - if issuable.has_attribute? :due_date
+
+      - if issuable.has_attribute?(:due_date)
         .block.due_date
           .sidebar-collapsed-icon
             = icon('calendar')
             %span.js-due-date-sidebar-value
-              - if issuable.due_date
-                = issuable.due_date.to_s(:medium)
-              - else
-                None
+              = issuable.due_date.try(:to_s, :medium) || 'None'
           .title.hide-collapsed
-            Due Date
+            Due date
             = icon('spinner spin', class: 'block-loading')
             - if can?(current_user, :"admin_#{issuable.to_ability_name}", @project)
               = link_to 'Edit', '#', class: 'edit-link pull-right'
@@ -96,13 +94,13 @@
           - if can?(current_user, :"admin_#{issuable.to_ability_name}", @project)
             .selectbox.hide-collapsed
               = hidden_field_tag "#{issuable.to_ability_name}[due_date]", issuable.due_date
+              = f.hidden_field :due_date, value: issuable.due_date
               .dropdown
-                %button.dropdown-menu-toggle.js-due-date-select{ type: "button", data: { toggle: "dropdown", field_name: "#{issuable.to_ability_name}[due_date]", ability_name: issuable.to_ability_name, issue_update: issuable_json_path(issuable) } }
-                  %span.dropdown-toggle-text
-                    Due date
-                  = icon("chevron-down")
+                %button.dropdown-menu-toggle.js-due-date-select{ type: 'button', data: { toggle: 'dropdown', field_name: "#{issuable.to_ability_name}[due_date]", ability_name: issuable.to_ability_name, issue_update: issuable_json_path(issuable) } }
+                  %span.dropdown-toggle-text Due date
+                  = icon('chevron-down')
                 .dropdown-menu.dropdown-menu-due-date
-                  = dropdown_title("Due date")
+                  = dropdown_title('Due date')
                   = dropdown_content do
                     .js-due-date-calendar
 

db/migrate/20160310124959_add_due_date_to_issues.rb

 class AddDueDateToIssues < ActiveRecord::Migration
   def change
     add_column :issues, :due_date, :date
+    add_index :issues, :due_date
   end
 end

db/migrate/20160320123111_add_index_to_issues_due_date.rb

-class AddIndexToIssuesDueDate < ActiveRecord::Migration
-  def change
-    add_index :issues, :due_date
-  end
-end

db/schema.rb

@@ -366,19 +366,6 @@ ActiveRecord::Schema.define(version: 20160419120017) do
   add_index "emails", ["email"], name: "index_emails_on_email", unique: true, using: :btree
   add_index "emails", ["user_id"], name: "index_emails_on_user_id", using: :btree
 
-  create_table "emoji_awards", force: :cascade do |t|
-    t.string   "name"
-    t.integer  "user_id"
-    t.integer  "awardable_id"
-    t.string   "awardable_type"
-    t.datetime "created_at"
-    t.datetime "updated_at"
-  end
-
-  add_index "emoji_awards", ["awardable_id"], name: "index_emoji_awards_on_awardable_id", using: :btree
-  add_index "emoji_awards", ["awardable_type"], name: "index_emoji_awards_on_awardable_type", using: :btree
-  add_index "emoji_awards", ["user_id"], name: "index_emoji_awards_on_user_id", using: :btree
-
   create_table "events", force: :cascade do |t|
     t.string   "target_type"
     t.integer  "target_id"

spec/features/issues_spec.rb

@@ -112,7 +112,7 @@ describe 'Issues', feature: true do
   end
 
   describe 'filter issue' do
-    titles = ['foo','bar','baz']
+    titles = %w[foo bar baz]
     titles.each_with_index do |title, index|
       let!(title.to_sym) do
         create(:issue, title: title,
@@ -154,86 +154,93 @@ describe 'Issues', feature: true do
     end
 
     describe 'sorting by due date' do
-      before :each do
-        foo.due_date = 1.day.from_now
-        foo.save
-        bar.due_date = 6.days.from_now
-        bar.save
+       before do
+        foo.update(due_date: 1.day.from_now)
+        bar.update(due_date: 6.days.from_now)
       end
 
       it 'sorts by recently due date' do
         visit namespace_project_issues_path(project.namespace, project, sort: sort_value_due_date_soon)
+
         expect(first_issue).to include('foo')
       end
 
       it 'sorts by least recently due date' do
         visit namespace_project_issues_path(project.namespace, project, sort: sort_value_due_date_later)
+
         expect(first_issue).to include('bar')
       end
 
       it 'sorts by least recently due date by excluding nil due dates' do
         bar.update(due_date: nil)
+
         visit namespace_project_issues_path(project.namespace, project, sort: sort_value_due_date_later)
+
         expect(first_issue).to include('foo')
       end
     end
 
     describe 'filtering by due date' do
-      before :each do
-        foo.due_date = 1.day.from_now
-        foo.save
-        bar.due_date = 6.days.from_now
-        bar.save
+      before do
+        foo.update(due_date: 1.day.from_now)
+        bar.update(due_date: 6.days.from_now)
       end
 
       it 'filters by none' do
         visit namespace_project_issues_path(project.namespace, project, due_date: Issue::NoDueDate.name)
-        expect(page).not_to have_content("foo")
-        expect(page).not_to have_content("bar")
-        expect(page).to have_content("baz")
+
+        expect(page).not_to have_content('foo')
+        expect(page).not_to have_content('bar')
+        expect(page).to have_content('baz')
       end
 
       it 'filters by any' do
         visit namespace_project_issues_path(project.namespace, project, due_date: Issue::AnyDueDate.name)
-        expect(page).to have_content("foo")
-        expect(page).to have_content("bar")
-        expect(page).to have_content("baz")
+
+        expect(page).to have_content('foo')
+        expect(page).to have_content('bar')
+        expect(page).to have_content('baz')
       end
 
       it 'filters by due this week' do
         foo.update(due_date: Date.today.beginning_of_week + 2.days)
         bar.update(due_date: Date.today.end_of_week)
         baz.update(due_date: Date.today - 8.days)
+
         visit namespace_project_issues_path(project.namespace, project, due_date: Issue::DueThisWeek.name)
-        expect(page).to have_content("foo")
-        expect(page).to have_content("bar")
-        expect(page).not_to have_content("baz")
+
+        expect(page).to have_content('foo')
+        expect(page).to have_content('bar')
+        expect(page).not_to have_content('baz')
       end
 
       it 'filters by due this month' do
         foo.update(due_date: Date.today.beginning_of_month + 2.days)
         bar.update(due_date: Date.today.end_of_month)
         baz.update(due_date: Date.today - 50.days)
+
         visit namespace_project_issues_path(project.namespace, project, due_date: Issue::DueThisMonth.name)
-        expect(page).to have_content("foo")
-        expect(page).to have_content("bar")
-        expect(page).not_to have_content("baz")
+
+        expect(page).to have_content('foo')
+        expect(page).to have_content('bar')
+        expect(page).not_to have_content('baz')
       end
 
       it 'filters by overdue' do
         foo.update(due_date: Date.today + 2.days)
         bar.update(due_date: Date.today + 20.days)
         baz.update(due_date: Date.yesterday)
-        visit namespace_project_issues_path(project.namespace, project, due_date: Issue::OverDue.name)
-        expect(page).not_to have_content("foo")
-        expect(page).not_to have_content("bar")
-        expect(page).to have_content("baz")
-      end
 
+        visit namespace_project_issues_path(project.namespace, project, due_date: Issue::Overdue.name)
+
+        expect(page).not_to have_content('foo')
+        expect(page).not_to have_content('bar')
+        expect(page).to have_content('baz')
+      end
     end
 
     describe 'sorting by milestone' do
-      before :each do
+      before do
         foo.milestone = newer_due_milestone
         foo.save
         bar.milestone = later_due_milestone
@@ -256,7 +263,7 @@ describe 'Issues', feature: true do
     describe 'combine filter and sort' do
       let(:user2) { create(:user) }
 
-      before :each do
+      before do
         foo.assignee = user2
         foo.save
         bar.assignee = user2
@@ -303,7 +310,7 @@ describe 'Issues', feature: true do
 
       let(:guest) { create(:user) }
 
-      before :each do
+      before do
         project.team << [[guest], :guest]
       end
 
@@ -346,7 +353,7 @@ describe 'Issues', feature: true do
     context 'by unauthorized user' do
       let(:guest) { create(:user) }
 
-      before :each do
+      before do
         project.team << [guest, :guest]
         issue.milestone = milestone
         issue.save
@@ -364,7 +371,7 @@ describe 'Issues', feature: true do
     describe 'removing assignee' do
       let(:user2) { create(:user) }
 
-      before :each do
+      before do
         issue.assignee = user2
         issue.save
       end