Backend refactoring

12 files changed, 110 insertions, 144 deletions

diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb
index a1109ab6c40..2a182b5fdde 100644
--- a/app/controllers/groups/milestones_controller.rb
+++ b/app/controllers/groups/milestones_controller.rb
@@ -15,7 +15,7 @@ class Groups::MilestonesController < Groups::ApplicationController
   end
 
   def new
-    @milestone = GroupMilestone.new
+    @milestone = Milestone.new
   end
 
   def create
@@ -47,7 +47,7 @@ class Groups::MilestonesController < Groups::ApplicationController
   end
 
   def milestone_params
-    params.require(:group_milestone).permit(:title, :description, :start_date, :due_date, :state_event)
+    params.require(:milestone).permit(:title, :description, :start_date, :due_date, :state_event)
   end
 
   def milestone_path(title)
@@ -55,10 +55,7 @@ class Groups::MilestonesController < Groups::ApplicationController
   end
 
   def milestones
-    @group_milestones = GroupMilestonesFinder.new(group, params).execute
-    @project_milestones = ProjectMilestonesFinder.new(@projects, params).execute
-
-    @group_milestones + @project_milestones
+    @milestones = MilestonesFinder.new(@projects, group, params).execute
   end
 
   def milestone
diff --git a/app/finders/group_milestones_finder.rb b/app/finders/group_milestones_finder.rb
deleted file mode 100644
index d5bf810c41b..00000000000
--- a/app/finders/group_milestones_finder.rb
+++ /dev/null
@@ -1,12 +0,0 @@
-class GroupMilestonesFinder
-  attr_reader :group, :params
-
-  def initialize(group, params)
-    @group = group
-    @params = params
-  end
-
-  def execute
-    GroupMilestone.filter_by_state(group.milestones, params[:state])
-  end
-end
diff --git a/app/finders/milestones_finder.rb b/app/finders/milestones_finder.rb
new file mode 100644
index 00000000000..6e2090ef781
--- /dev/null
+++ b/app/finders/milestones_finder.rb
@@ -0,0 +1,22 @@
+class MilestonesFinder
+  attr_reader :projects, :group, :params
+
+  def initialize(projects = nil, group = nil, params = {})
+    @projects = projects
+    @group = group
+    @params = params
+  end
+
+  def execute
+    table = Milestone.arel_table
+    project_ids = projects&.map(&:id)
+    group_id = group&.id
+
+    milestones =
+      Milestone.where(table[:project_id].in(project_ids)
+        .or(table[:group_id].eq(group_id))
+      )
+
+    Milestone.filter_by_state(milestones, params[:state])
+  end
+end
diff --git a/app/finders/project_milestones_finder.rb b/app/finders/project_milestones_finder.rb
deleted file mode 100644
index a9ffbc9de55..00000000000
--- a/app/finders/project_milestones_finder.rb
+++ /dev/null
@@ -1,14 +0,0 @@
-class ProjectMilestonesFinder
-  attr_reader :projects, :params
-
-  def initialize(projects, params)
-    @projects = projects
-    @params = params
-  end
-
-  def execute
-    milestones = Milestone.of_projects(projects)
-
-    Milestone.filter_by_state(milestones, params[:state])
-  end
-end
diff --git a/app/models/concerns/internal_id.rb b/app/models/concerns/internal_id.rb
index 5382dde6765..67a0adfcd56 100644
--- a/app/models/concerns/internal_id.rb
+++ b/app/models/concerns/internal_id.rb
@@ -8,7 +8,8 @@ module InternalId
 
   def set_iid
     if iid.blank?
-      records = project.send(self.class.name.tableize)
+      parent = project || group
+      records = parent.send(self.class.name.tableize)
       records = records.with_deleted if self.paranoid?
       max_iid = records.maximum(:iid)
 
diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb
index 60aeab38118..5a6a408ea3a 100644
--- a/app/models/concerns/milestoneish.rb
+++ b/app/models/concerns/milestoneish.rb
@@ -71,11 +71,11 @@ module Milestoneish
   end
 
   def is_group_milestone?
-    respond_to?(:group_id)
+    group_id.present?
   end
 
   def is_project_milestone?
-    respond_to?(:project_id)
+    project_id.present?
   end
 
   def title=(value)
diff --git a/app/models/concerns/shared_milestone_properties.rb b/app/models/concerns/shared_milestone_properties.rb
deleted file mode 100644
index 77a8da9fd93..00000000000
--- a/app/models/concerns/shared_milestone_properties.rb
+++ /dev/null
@@ -1,74 +0,0 @@
-module SharedMilestoneProperties
-  extend ActiveSupport::Concern
-
-  include StripAttribute
-  include CacheMarkdownField
-
-  included do
-    has_many :issues
-    has_many :merge_requests
-    has_many :labels, -> { distinct.reorder('labels.title') },  through: :issues
-
-    validate :uniqueness_of_title, if: :title_changed?
-
-    scope :active, -> { with_state(:active) }
-    scope :closed, -> { with_state(:closed) }
-
-    validate :start_date_should_be_less_than_due_date, if: proc { |m| m.start_date.present? && m.due_date.present? }
-    strip_attributes :title
-    alias_attribute :name, :title
-
-    state_machine :state, initial: :active do
-      event :close do
-        transition active: :closed
-      end
-
-      event :activate do
-        transition closed: :active
-      end
-
-      state :closed
-
-      state :active
-    end
-
-    alias_attribute :name, :title
-
-    cache_markdown_field :title, pipeline: :single_line
-    cache_markdown_field :description
-  end
-
-  module ClassMethods
-    def filter_by_state(milestones, state)
-      case state
-      when 'closed' then milestones.closed
-      when 'all' then milestones
-      else milestones.active
-      end
-    end
-  end
-
-  def start_date_should_be_less_than_due_date
-    if due_date <= start_date
-      errors.add(:start_date, "Can't be greater than due date")
-    end
-  end
-
-  def safe_title
-    title.to_slug.normalize.to_s
-  end
-
-  private
-
-  # Milestone titles must be unique across project milestones and group milestones
-  def uniqueness_of_title(group, project = nil)
-    if group
-      title_exists = group.milestones.find_by_title(title).present?
-      title_exists ||= Milestone.where(project: group.projects).find_by_title(title).present?
-    elsif project
-      title_exists = project.milestones.find_by_title(title).present?
-    end
-
-    errors.add(:title, "Must be unique across project milestones and group milestones.") if title_exists
-  end
-end
diff --git a/app/models/global_milestone.rb b/app/models/global_milestone.rb
index 8ef4546ee24..50ff0174ff3 100644
--- a/app/models/global_milestone.rb
+++ b/app/models/global_milestone.rb
@@ -12,7 +12,7 @@ class GlobalMilestone
   end
 
   def self.build_collection(projects, params)
-    child_milestones = ProjectMilestonesFinder.new(projects, params).execute
+    child_milestones = MilestonesFinder.new(projects, nil, params).execute
 
     milestones = child_milestones.select(:id, :title).group_by(&:title).map do |title, grouped|
       milestones_relation = Milestone.where(id: grouped.map(&:id))
@@ -41,7 +41,7 @@ class GlobalMilestone
   def self.group_milestones_states_count(group)
     return STATE_COUNT_HASH unless group
 
-    relation = GroupMilestonesFinder.new(group, state: 'all').execute
+    relation = MilestonesFinder.new(nil, group, state: 'all').execute
     grouped_by_state = relation.reorder(nil).group(:state).count
 
     {
@@ -51,10 +51,11 @@ class GlobalMilestone
     }
   end
 
+  # Counts the legacy group milestones which must be grouped by title
   def self.legacy_group_milestone_states_count(projects)
     return STATE_COUNT_HASH unless projects
 
-    relation = ProjectMilestonesFinder.new(projects, state: 'all').execute
+    relation = MilestonesFinder.new(projects, nil, state: 'all').execute
     project_milestones_by_state_and_title = relation.reorder(nil).group(:state, :title).count
 
     opened = count_by_state(project_milestones_by_state_and_title, 'active')
diff --git a/app/models/group.rb b/app/models/group.rb
index 9bec0ec5025..1c1fb6f41e2 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -18,7 +18,7 @@ class Group < Namespace
 
   has_many :requesters, -> { where.not(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember'
 
-  has_many :milestones, class_name: 'GroupMilestone'
+  has_many :milestones
   has_many :project_group_links, dependent: :destroy
   has_many :shared_projects, through: :project_group_links, source: :project
   has_many :notification_settings, dependent: :destroy, as: :source
diff --git a/app/models/group_milestone.rb b/app/models/group_milestone.rb
index c518fe430bd..86d38e5468b 100644
--- a/app/models/group_milestone.rb
+++ b/app/models/group_milestone.rb
@@ -1,35 +1,19 @@
-class GroupMilestone < ActiveRecord::Base
-  include SharedMilestoneProperties
-  include Milestoneish
-  include CacheMarkdownField
+class GroupMilestone < GlobalMilestone
+  attr_accessor :group
 
-  belongs_to :group
-
-  class << self
-    # Build legacy group milestone which consists on all project milestones
-    # with the same title.
-    def build(group, projects, title)
-      GlobalMilestone.build(projects, title).tap do |milestone|
-        milestone&.group = group
-      end
+  def self.build_collection(group, projects, params)
+    super(projects, params).each do |milestone|
+      milestone.group = group
     end
   end
 
-  def milestoneish_ids
-    id
-  end
-
-  def participants
-    User.joins(assigned_issues: :group_milestone).where("group_milestones.id = ?", id).uniq
-  end
-
-  private
-
-  def uniqueness_of_title
-    super(group)
+  def self.build(group, projects, title)
+    super(projects, title).tap do |milestone|
+      milestone&.group = group
+    end
   end
 
-  def issues_finder_conditions
-    { group_milestone_id: milestoneish_ids }
+  def issues_finder_params
+    { group_id: group.id }
   end
 end
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index 983253de077..8cfb69349cf 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -7,17 +7,49 @@ class Milestone < ActiveRecord::Base
   Upcoming = MilestoneStruct.new('Upcoming', '#upcoming', -2)
   Started = MilestoneStruct.new('Started', '#started', -3)
 
-  include SharedMilestoneProperties
   include InternalId
   include Sortable
   include Referable
   include Milestoneish
+  include StripAttribute
+  include CacheMarkdownField
 
   belongs_to :project
+  belongs_to :group
 
   has_many :events, as: :target, dependent: :destroy
 
+  has_many :issues
+  has_many :merge_requests
+  has_many :labels, -> { distinct.reorder('labels.title') },  through: :issues
+
+  validate :uniqueness_of_title, if: :title_changed?
+  validate :start_date_should_be_less_than_due_date, if: proc { |m| m.start_date.present? && m.due_date.present? }
+
+  strip_attributes :title
+  alias_attribute :name, :title
+
+  cache_markdown_field :title, pipeline: :single_line
+  cache_markdown_field :description
+
   scope :of_projects, ->(ids) { where(project_id: ids) }
+  scope :of_groups, ->(ids) { where(group_id: ids) }
+  scope :active, -> { with_state(:active) }
+  scope :closed, -> { with_state(:closed) }
+
+  state_machine :state, initial: :active do
+    event :close do
+      transition active: :closed
+    end
+
+    event :activate do
+      transition closed: :active
+    end
+
+    state :closed
+
+    state :active
+  end
 
   class << self
     # Searches for milestones matching the given query.
@@ -33,6 +65,14 @@ class Milestone < ActiveRecord::Base
 
       where(t[:title].matches(pattern).or(t[:description].matches(pattern)))
     end
+
+    def filter_by_state(milestones, state)
+      case state
+      when 'closed' then milestones.closed
+      when 'all' then milestones
+      else milestones.active
+      end
+    end
   end
 
   def self.reference_prefix
@@ -111,7 +151,11 @@ class Milestone < ActiveRecord::Base
     format_reference = milestone_format_reference(format)
     reference = "#{self.class.reference_prefix}#{format_reference}"
 
-    "#{project.to_reference(from_project, full: full)}#{reference}"
+    if project
+      "#{project.to_reference(from_project, full: full)}#{reference}"
+    elsif group
+      "#{group.to_reference}#{reference}"
+    end
   end
 
   def reference_link_text(from_project = nil)
@@ -130,10 +174,28 @@ class Milestone < ActiveRecord::Base
     nil
   end
 
+  def safe_title
+    title.to_slug.normalize.to_s
+  end
+
   private
 
+  def start_date_should_be_less_than_due_date
+    if due_date <= start_date
+      errors.add(:start_date, "Can't be greater than due date")
+    end
+  end
+
+  # Milestone titles must be unique across project milestones and group milestones
   def uniqueness_of_title
-    super(project.group, project)
+    if group
+      title_exists = group.milestones.find_by_title(title).present?
+      title_exists ||= Milestone.where(project: group.projects).find_by_title(title).present?
+    elsif project
+      title_exists = project.milestones.find_by_title(title).present?
+    end
+
+    errors.add(:title, "Must be unique across project milestones and group milestones.") if title_exists
   end
 
   def milestone_format_reference(format = :iid)
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 31a289daaf2..b48d73dcae7 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -16,7 +16,6 @@ class Namespace < ActiveRecord::Base
   cache_markdown_field :description, pipeline: :description
 
   has_many :projects, dependent: :destroy
-  has_many :groups, dependent: :destroy
   has_many :project_statistics
   belongs_to :owner, class_name: "User"