Merge branch 'issue_30126_be' of gitlab.com:gitlab-org/gitlab-ce into issue_30126_be

16 files changed, 131 insertions, 176 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 4f94b091598..00000000000
--- a/app/models/concerns/shared_milestone_properties.rb
+++ /dev/null
@@ -1,72 +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
-
-  # Milestone title must be unique across project milestones and group milestones
-  def uniqueness_of_title
-    title_exists = group.milestones.find_by_title(title).present? if is_group_milestone?
-
-    if is_project_milestone?
-      title_exists = project.milestones.find_by_title(title)
-      title_exists ||= project.group.milestones.find_by_title(title)
-    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 c61dee9f652..86d38e5468b 100644
--- a/app/models/group_milestone.rb
+++ b/app/models/group_milestone.rb
@@ -1,31 +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
+  def self.build(group, projects, title)
+    super(projects, title).tap do |milestone|
+      milestone&.group = group
+    end
   end
 
-  private
-
-  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 f6277fe532c..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,8 +174,30 @@ 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
+    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)
     raise ArgumentError, 'Unknown format' unless [:iid, :name].include?(format)
 
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"
 
diff --git a/db/migrate/20170619183807_add_group_id_to_milestones.rb b/db/migrate/20170619183807_add_group_id_to_milestones.rb
new file mode 100644
index 00000000000..feb45ac240e
--- /dev/null
+++ b/db/migrate/20170619183807_add_group_id_to_milestones.rb
@@ -0,0 +1,10 @@
+class AddGroupIdToMilestones < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def change
+    change_column_null :milestones, :project_id, true
+    add_column :milestones, :group_id, :integer
+    add_column :issues, :group_milestone_id, :integer
+    add_column :merge_requests, :group_milestone_id, :integer
+  end
+end
diff --git a/db/migrate/20170619183807_create_group_milestones.rb b/db/migrate/20170619183807_create_group_milestones.rb
deleted file mode 100644
index 9ce287bb306..00000000000
--- a/db/migrate/20170619183807_create_group_milestones.rb
+++ /dev/null
@@ -1,23 +0,0 @@
-class CreateGroupMilestones < ActiveRecord::Migration
-  DOWNTIME = false
-
-  def change
-    create_table :group_milestones do |t|
-      t.integer :group_id
-      t.string :title
-      t.text :description
-      t.date :start_date
-      t.date :due_date
-      t.string :state
-      t.string :title_html
-      t.string :description_html
-      t.integer :cached_markdown_version, limit: 4
-    end
-
-    add_column :issues, :group_milestone_id, :integer
-    add_column :merge_requests, :group_milestone_id, :integer
-
-    add_foreign_key :group_milestones, :namespaces, column: :group_id, null: false # rubocop: disable Migration/AddConcurrentForeignKey
-    add_index :group_milestones, :group_id
-  end
-end
diff --git a/db/migrate/20170619184243_add_group_milestone_id_indexes.rb b/db/migrate/20170619184243_add_group_milestone_id_indexes.rb
index 836d39e6e6d..4b52e2c5535 100644
--- a/db/migrate/20170619184243_add_group_milestone_id_indexes.rb
+++ b/db/migrate/20170619184243_add_group_milestone_id_indexes.rb
@@ -6,17 +6,21 @@ class AddGroupMilestoneIdIndexes < ActiveRecord::Migration
   DOWNTIME = false
 
   def up
+    add_foreign_key :milestones, :namespaces, column: :group_id
     add_foreign_key :issues, :namespaces, column: :group_milestone_id
     add_foreign_key :merge_requests, :namespaces, column: :group_milestone_id
 
+    add_concurrent_index :milestones, :group_id
     add_concurrent_index :issues, :group_milestone_id
     add_concurrent_index :merge_requests, :group_milestone_id
   end
 
   def down
+    remove_foreign_key :milestones, column: :group_id
     remove_foreign_key :issues, column: :group_milestone_id
     remove_foreign_key :merge_requests, column: :group_milestone_id
 
+    remove_concurrent_index :milestones, :group_id
     remove_concurrent_index :issues, :group_milestone_id
     remove_concurrent_index :merge_requests, :group_milestone_id
   end
diff --git a/db/schema.rb b/db/schema.rb
index 4b32887da2d..688e5199a7d 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -511,20 +511,6 @@ ActiveRecord::Schema.define(version: 20170619184243) do
 
   add_index "forked_project_links", ["forked_to_project_id"], name: "index_forked_project_links_on_forked_to_project_id", unique: true, using: :btree
 
-  create_table "group_milestones", force: :cascade do |t|
-    t.integer "group_id"
-    t.string "title"
-    t.text "description"
-    t.date "start_date"
-    t.date "due_date"
-    t.string "state"
-    t.string "title_html"
-    t.string "description_html"
-    t.integer "cached_markdown_version"
-  end
-
-  add_index "group_milestones", ["group_id"], name: "index_group_milestones_on_group_id", using: :btree
-
   create_table "identities", force: :cascade do |t|
     t.string "extern_uid"
     t.string "provider"
@@ -815,7 +801,7 @@ ActiveRecord::Schema.define(version: 20170619184243) do
 
   create_table "milestones", force: :cascade do |t|
     t.string "title", null: false
-    t.integer "project_id", null: false
+    t.integer "project_id"
     t.text "description"
     t.date "due_date"
     t.datetime "created_at"
@@ -826,10 +812,12 @@ ActiveRecord::Schema.define(version: 20170619184243) do
     t.text "description_html"
     t.date "start_date"
     t.integer "cached_markdown_version"
+    t.integer "group_id"
   end
 
   add_index "milestones", ["description"], name: "index_milestones_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"}
   add_index "milestones", ["due_date"], name: "index_milestones_on_due_date", using: :btree
+  add_index "milestones", ["group_id"], name: "index_milestones_on_group_id", using: :btree
   add_index "milestones", ["project_id", "iid"], name: "index_milestones_on_project_id_and_iid", unique: true, using: :btree
   add_index "milestones", ["title"], name: "index_milestones_on_title", using: :btree
   add_index "milestones", ["title"], name: "index_milestones_on_title_trigram", using: :gin, opclasses: {"title"=>"gin_trgm_ops"}
@@ -1556,7 +1544,6 @@ ActiveRecord::Schema.define(version: 20170619184243) do
   add_foreign_key "ci_triggers", "users", column: "owner_id", name: "fk_e8e10d1964", on_delete: :cascade
   add_foreign_key "ci_variables", "projects", name: "fk_ada5eb64b3", on_delete: :cascade
   add_foreign_key "container_repositories", "projects"
-  add_foreign_key "group_milestones", "namespaces", column: "group_id"
   add_foreign_key "issue_assignees", "issues", name: "fk_b7d881734a", on_delete: :cascade
   add_foreign_key "issue_assignees", "users", name: "fk_5e0c8d9154", on_delete: :cascade
   add_foreign_key "issue_metrics", "issues", on_delete: :cascade
@@ -1572,6 +1559,7 @@ ActiveRecord::Schema.define(version: 20170619184243) do
   add_foreign_key "merge_requests", "namespaces", column: "group_milestone_id"
   add_foreign_key "merge_requests_closing_issues", "issues", on_delete: :cascade
   add_foreign_key "merge_requests_closing_issues", "merge_requests", on_delete: :cascade
+  add_foreign_key "milestones", "namespaces", column: "group_id"
   add_foreign_key "oauth_openid_requests", "oauth_access_grants", column: "access_grant_id", name: "fk_oauth_openid_requests_oauth_access_grants_access_grant_id"
   add_foreign_key "personal_access_tokens", "users"
   add_foreign_key "project_authorizations", "projects", on_delete: :cascade