Address MR feedback

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

7 files changed, 41 insertions, 44 deletions

diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index 7613283443a..e21cfbd0d47 100644
--- a/app/finders/issuable_finder.rb
+++ b/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
 
diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb
index d3779eda91f..afe1e11a0da 100644
--- a/app/helpers/issues_helper.rb
+++ b/app/helpers/issues_helper.rb
@@ -131,7 +131,7 @@ module IssuesHelper
         class: "icon emoji-icon emoji-#{unicode}",
         title: name,
         data: data
-    else 
+    else
       # Emoji icons displayed separately, used for the awards already given
       # to an issue or merge request.
       content_tag :img, "",
@@ -174,17 +174,16 @@ module IssuesHelper
 
   def due_date_options
     options = [
-        Issue::AnyDueDate,
-        Issue::NoDueDate,
-        Issue::DueThisWeek,
-        Issue::DueThisMonth,
-        Issue::OverDue
+      Issue::AnyDueDate,
+      Issue::NoDueDate,
+      Issue::DueThisWeek,
+      Issue::DueThisMonth,
+      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
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 03d8a573a4d..7c2a92fa86a 100644
--- a/app/models/concerns/issuable.rb
+++ b/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) }
diff --git a/app/models/concerns/sortable.rb b/app/models/concerns/sortable.rb
index f1f09011c1b..42617ecbb96 100644
--- a/app/models/concerns/sortable.rb
+++ b/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
diff --git a/app/models/issue.rb b/app/models/issue.rb
index db0208ae3ad..d72233006ee 100644
--- a/app/models/issue.rb
+++ b/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
diff --git a/app/views/shared/_sort_dropdown.html.haml b/app/views/shared/_sort_dropdown.html.haml
index 922b02f9f3e..d327bd0a96f 100644
--- a/app/views/shared/_sort_dropdown.html.haml
+++ b/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
-      = link_to page_filter_path(sort: sort_value_due_date_soon) do
-        = sort_title_due_date_soon if @issues
-      = link_to page_filter_path(sort: sort_value_due_date_later) do
-        = sort_title_due_date_later if @issues
+      - 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
+        = link_to page_filter_path(sort: sort_value_due_date_later) do
+          = 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
diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml
index 61a655a40df..a4dbb5d23fc 100644
--- a/app/views/shared/issuable/_sidebar.html.haml
+++ b/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