Updates from last code review.issue_13621_2

7 files changed, 39 insertions, 78 deletions

diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index a3c4a3d2776..27b97944e38 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -36,6 +36,8 @@ module Issuable
     scope :closed, -> { with_state(:closed) }
     scope :order_milestone_due_desc, -> { joins(:milestone).reorder('milestones.due_date DESC, milestones.id DESC') }
     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 :join_project, -> { joins(:project) }
     scope :references_project, -> { references(:project) }
diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb
new file mode 100644
index 00000000000..d67df7c1d9c
--- /dev/null
+++ b/app/models/concerns/milestoneish.rb
@@ -0,0 +1,25 @@
+module Milestoneish
+  def closed_items_count
+    issues.closed.size + merge_requests.closed_and_merged.size
+  end
+
+  def total_items_count
+    issues.size + merge_requests.size
+  end
+
+  def complete?
+    total_items_count == closed_items_count
+  end
+
+  def percent_complete
+    ((closed_items_count * 100) / total_items_count).abs
+  rescue ZeroDivisionError
+    0
+  end
+
+  def remaining_days
+    return 0 if !due_date || expired?
+
+    (due_date - Date.today).to_i
+  end
+end
diff --git a/app/models/global_label.rb b/app/models/global_label.rb
index 0171f7d54b7..ddd4bad5c21 100644
--- a/app/models/global_label.rb
+++ b/app/models/global_label.rb
@@ -2,16 +2,19 @@ class GlobalLabel
   attr_accessor :title, :labels
   alias_attribute :name, :title
 
+  delegate :color, :description, to: :@first_label
+
   def self.build_collection(labels)
     labels = labels.group_by(&:title)
 
-    labels.map do |title, label|
-      new(title, label)
+    labels.map do |title, labels|
+      new(title, labels)
     end
   end
 
   def initialize(title, labels)
     @title = title
     @labels = labels
+    @first_label = labels.find { |lbl| lbl.description.present? } || labels.first
   end
 end
diff --git a/app/models/global_milestone.rb b/app/models/global_milestone.rb
index e13aaf16732..97bd79af083 100644
--- a/app/models/global_milestone.rb
+++ b/app/models/global_milestone.rb
@@ -1,4 +1,6 @@
 class GlobalMilestone
+  include Milestoneish
+
   attr_accessor :title, :milestones
   alias_attribute :name, :title
 
@@ -31,32 +33,6 @@ class GlobalMilestone
     @projects ||= Project.for_milestones(milestones.map(&:id))
   end
 
-  def issues_count
-    issues.count
-  end
-
-  def merge_requests_count
-    merge_requests.count
-  end
-
-  def open_items_count
-    opened_issues.count + opened_merge_requests.count
-  end
-
-  def closed_items_count
-    closed_issues.count + closed_merge_requests.count
-  end
-
-  def total_items_count
-    issues_count + merge_requests_count
-  end
-
-  def percent_complete
-    ((closed_items_count * 100) / total_items_count).abs
-  rescue ZeroDivisionError
-    0
-  end
-
   def state
     state = milestones.map { |milestone| milestone.state }
 
@@ -88,29 +64,8 @@ class GlobalMilestone
   end
 
   def labels
-    @labels ||= milestones.map do |ms|
-                  ms.labels.map { |label| LabelWithMilestone.new(label, ms) }
-                end.flatten.sort_by!(&:title)
-  end
-
-  def opened_issues
-    issues.opened
-  end
-
-  def closed_issues
-    issues.closed
-  end
-
-  def opened_merge_requests
-    merge_requests.opened
-  end
-
-  def closed_merge_requests
-    merge_requests.with_states(:closed, :merged, :locked)
-  end
-
-  def complete?
-    total_items_count == closed_items_count
+    @labels ||= GlobalLabel.build_collection(milestones.map(&:labels).flatten)
+                           .sort_by!(&:title)
   end
 
   def due_date
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index f575494e2bf..0c1a47b3f6a 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -137,7 +137,6 @@ class MergeRequest < ActiveRecord::Base
   scope :by_milestone, ->(milestone) { where(milestone_id: milestone) }
   scope :in_projects, ->(project_ids) { where("source_project_id in (:project_ids) OR target_project_id in (:project_ids)", project_ids: project_ids) }
   scope :of_projects, ->(ids) { where(target_project_id: ids) }
-  scope :opened, -> { with_states(:opened, :reopened) }
   scope :merged, -> { with_state(:merged) }
   scope :closed_and_merged, -> { with_states(:closed, :merged) }
 
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index 7dc2f909b2f..e3969f32dd6 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -24,12 +24,13 @@ class Milestone < ActiveRecord::Base
   include Sortable
   include Referable
   include StripAttribute
+  include Milestoneish
 
   belongs_to :project
   has_many :issues
   has_many :labels, -> { distinct.reorder('labels.title') },  through: :issues
   has_many :merge_requests
-  has_many :participants, through: :issues, source: :assignee
+  has_many :participants, -> { distinct.reorder('users.name') }, through: :issues, source: :assignee
 
   scope :active, -> { with_state(:active) }
   scope :closed, -> { with_state(:closed) }
@@ -92,30 +93,6 @@ class Milestone < ActiveRecord::Base
     end
   end
 
-  def open_items_count
-    self.issues.opened.count + self.merge_requests.opened.count
-  end
-
-  def closed_items_count
-    self.issues.closed.count + self.merge_requests.closed_and_merged.count
-  end
-
-  def total_items_count
-    self.issues.count + self.merge_requests.count
-  end
-
-  def percent_complete
-    ((closed_items_count * 100) / total_items_count).abs
-  rescue ZeroDivisionError
-    0
-  end
-
-  def remaining_days
-    return 0 if !due_date || expired?
-
-    (due_date - Date.today).to_i
-  end
-
   def expires_at
     if due_date
       if due_date.past?
diff --git a/app/models/project.rb b/app/models/project.rb
index 3a28d5d7fee..3235a1cee50 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -215,7 +215,7 @@ class Project < ActiveRecord::Base
   scope :public_only, -> { where(visibility_level: Project::PUBLIC) }
   scope :public_and_internal_only, -> { where(visibility_level: Project.public_and_internal_levels) }
   scope :non_archived, -> { where(archived: false) }
-  scope :for_milestones, ->(ids) { joins(:milestones).where('milestones.id' => ids) }
+  scope :for_milestones, ->(ids) { joins(:milestones).where('milestones.id' => ids).distinct }
 
   state_machine :import_status, initial: :none do
     event :import_start do