Merge branch 'remove-multiple-milestone-mess' into 'master'

Remove unnecessary milestone join tables

See merge request gitlab-org/gitlab!25198

app/models/concerns/milestoneable.rb

@@ -14,8 +14,6 @@ module Milestoneable
 
     validate :milestone_is_valid
 
-    after_save :write_to_new_milestone_relationship
-
     scope :of_milestones, ->(ids) { where(milestone_id: ids) }
     scope :any_milestone, -> { where('milestone_id IS NOT NULL') }
     scope :with_milestone, ->(title) { left_joins_milestones.where(milestones: { title: title }) }
@@ -41,10 +39,6 @@ module Milestoneable
     def milestone_is_valid
       errors.add(:milestone_id, message: "is invalid") if respond_to?(:milestone_id) && milestone_id.present? && !milestone_available?
     end
-
-    def write_to_new_milestone_relationship
-      self.milestones = [milestone].compact if supports_milestone? && saved_change_to_milestone_id?
-    end
   end
 
   def milestone_available?

app/models/issue.rb

@@ -33,9 +33,6 @@ class Issue < ApplicationRecord
 
   has_internal_id :iid, scope: :project, track_if: -> { !importing? }, init: ->(s) { s&.project&.issues&.maximum(:iid) }
 
-  has_many :issue_milestones
-  has_many :milestones, through: :issue_milestones
-
   has_many :events, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
   has_many :merge_requests_closing_issues,

app/models/issue_milestone.rb

-# frozen_string_literal: true
-
-class IssueMilestone < ApplicationRecord
-  belongs_to :milestone
-  belongs_to :issue
-end

app/models/merge_request.rb

@@ -38,9 +38,6 @@ class MergeRequest < ApplicationRecord
   has_many :merge_request_context_commits
   has_many :merge_request_context_commit_diff_files, through: :merge_request_context_commits, source: :diff_files
 
-  has_many :merge_request_milestones
-  has_many :milestones, through: :merge_request_milestones
-
   has_one :merge_request_diff,
     -> { order('merge_request_diffs.id DESC') }, inverse_of: :merge_request
 

app/models/merge_request_milestone.rb

-# frozen_string_literal: true
-
-class MergeRequestMilestone < ApplicationRecord
-  belongs_to :milestone
-  belongs_to :merge_request
-end

app/models/milestone.rb

@@ -39,9 +39,6 @@ class Milestone < ApplicationRecord
   has_many :merge_requests
   has_many :events, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
-  has_many :issue_milestones
-  has_many :merge_request_milestones
-
   scope :of_projects, ->(ids) { where(project_id: ids) }
   scope :of_groups, ->(ids) { where(group_id: ids) }
   scope :active, -> { with_state(:active) }

changelogs/unreleased/remove-multiple-milestone-mess.yml

+---
+title: Remove unnecessary milestone join tables
+merge_request: 25198
+author:
+type: changed

db/post_migrate/20200213204737_remove_unnecessary_milestone_join_tables.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveUnnecessaryMilestoneJoinTables < ActiveRecord::Migration[6.0]
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  def up
+    drop_table :issue_milestones
+    drop_table :merge_request_milestones
+  end
+
+  def down
+    create_table :issue_milestones, id: false do |t|
+      t.references :issue, foreign_key: { on_delete: :cascade }, index: { unique: true }, null: false
+      t.references :milestone, foreign_key: { on_delete: :cascade }, index: true, null: false
+    end
+
+    add_index :issue_milestones, [:issue_id, :milestone_id], unique: true
+
+    create_table :merge_request_milestones, id: false do |t|
+      t.references :merge_request, foreign_key: { on_delete: :cascade }, index: { unique: true }, null: false
+      t.references :milestone, foreign_key: { on_delete: :cascade }, index: true, null: false
+    end
+
+    add_index :merge_request_milestones, [:merge_request_id, :milestone_id], name: 'index_mrs_milestones_on_mr_id_and_milestone_id', unique: true
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_02_12_052620) do
+ActiveRecord::Schema.define(version: 2020_02_13_204737) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -2125,14 +2125,6 @@ ActiveRecord::Schema.define(version: 2020_02_12_052620) do
     t.index ["issue_id"], name: "index_issue_metrics"
   end
 
-  create_table "issue_milestones", id: false, force: :cascade do |t|
-    t.bigint "issue_id", null: false
-    t.bigint "milestone_id", null: false
-    t.index ["issue_id", "milestone_id"], name: "index_issue_milestones_on_issue_id_and_milestone_id", unique: true
-    t.index ["issue_id"], name: "index_issue_milestones_on_issue_id", unique: true
-    t.index ["milestone_id"], name: "index_issue_milestones_on_milestone_id"
-  end
-
   create_table "issue_tracker_data", force: :cascade do |t|
     t.integer "service_id", null: false
     t.datetime_with_timezone "created_at", null: false
@@ -2552,14 +2544,6 @@ ActiveRecord::Schema.define(version: 2020_02_12_052620) do
     t.index ["pipeline_id"], name: "index_merge_request_metrics_on_pipeline_id"
   end
 
-  create_table "merge_request_milestones", id: false, force: :cascade do |t|
-    t.bigint "merge_request_id", null: false
-    t.bigint "milestone_id", null: false
-    t.index ["merge_request_id", "milestone_id"], name: "index_mrs_milestones_on_mr_id_and_milestone_id", unique: true
-    t.index ["merge_request_id"], name: "index_merge_request_milestones_on_merge_request_id", unique: true
-    t.index ["milestone_id"], name: "index_merge_request_milestones_on_milestone_id"
-  end
-
   create_table "merge_request_user_mentions", force: :cascade do |t|
     t.integer "merge_request_id", null: false
     t.integer "note_id"
@@ -4799,8 +4783,6 @@ ActiveRecord::Schema.define(version: 2020_02_12_052620) do
   add_foreign_key "issue_links", "issues", column: "source_id", name: "fk_c900194ff2", on_delete: :cascade
   add_foreign_key "issue_links", "issues", column: "target_id", name: "fk_e71bb44f1f", on_delete: :cascade
   add_foreign_key "issue_metrics", "issues", on_delete: :cascade
-  add_foreign_key "issue_milestones", "issues", on_delete: :cascade
-  add_foreign_key "issue_milestones", "milestones", on_delete: :cascade
   add_foreign_key "issue_tracker_data", "services", on_delete: :cascade
   add_foreign_key "issue_user_mentions", "issues", on_delete: :cascade
   add_foreign_key "issue_user_mentions", "notes", on_delete: :cascade
@@ -4846,8 +4828,6 @@ ActiveRecord::Schema.define(version: 2020_02_12_052620) do
   add_foreign_key "merge_request_metrics", "merge_requests", on_delete: :cascade
   add_foreign_key "merge_request_metrics", "users", column: "latest_closed_by_id", name: "fk_ae440388cc", on_delete: :nullify
   add_foreign_key "merge_request_metrics", "users", column: "merged_by_id", name: "fk_7f28d925f3", on_delete: :nullify
-  add_foreign_key "merge_request_milestones", "merge_requests", on_delete: :cascade
-  add_foreign_key "merge_request_milestones", "milestones", on_delete: :cascade
   add_foreign_key "merge_request_user_mentions", "merge_requests", on_delete: :cascade
   add_foreign_key "merge_request_user_mentions", "notes", on_delete: :cascade
   add_foreign_key "merge_requests", "ci_pipelines", column: "head_pipeline_id", name: "fk_fd82eae0b9", on_delete: :nullify

lib/gitlab/import_export/import_export.yml

@@ -25,8 +25,6 @@ tree:
       - milestone:
         - events:
           - :push_event_payload
-      - issue_milestones:
-          - :milestone
       - resource_label_events:
         - label:
           - :priorities
@@ -64,8 +62,6 @@ tree:
       - milestone:
         - events:
           - :push_event_payload
-      - merge_request_milestones:
-          - :milestone
       - resource_label_events:
         - label:
           - :priorities
@@ -212,12 +208,6 @@ excluded_attributes:
     - :latest_merge_request_diff_id
     - :head_pipeline_id
     - :state_id
-  issue_milestones:
-    - :milestone_id
-    - :issue_id
-  merge_request_milestones:
-    - :milestone_id
-    - :merge_request_id
   award_emoji:
     - :awardable_id
   statuses:

spec/lib/gitlab/import_export/all_models.yml

@@ -6,8 +6,6 @@ issues:
 - assignees
 - updated_by
 - milestone
-- issue_milestones
-- milestones
 - notes
 - resource_label_events
 - resource_weight_events
@@ -82,8 +80,6 @@ milestone:
 - boards
 - milestone_releases
 - releases
-- issue_milestones
-- merge_request_milestones
 snippets:
 - author
 - project
@@ -113,8 +109,6 @@ merge_requests:
 - assignee
 - updated_by
 - milestone
-- merge_request_milestones
-- milestones
 - notes
 - resource_label_events
 - label_links
@@ -157,12 +151,6 @@ merge_requests:
 - deployment_merge_requests
 - deployments
 - user_mentions
-issue_milestones:
-- milestone
-- issue
-merge_request_milestones:
-- milestone
-- merge_request
 external_pull_requests:
 - project
 merge_request_diff:

spec/models/concerns/milestoneable_spec.rb

@@ -35,41 +35,6 @@ describe Milestoneable do
 
           it { is_expected.to be_invalid }
         end
-
-        context 'when valid and saving' do
-          it 'copies the value to the new milestones relationship' do
-            subject.save!
-
-            expect(subject.milestones).to match_array([milestone])
-          end
-
-          context 'with old values in milestones relationship' do
-            let(:old_milestone) { create(:milestone, project: project) }
-
-            before do
-              subject.milestone = old_milestone
-              subject.save!
-            end
-
-            it 'replaces old values' do
-              expect(subject.milestones).to match_array([old_milestone])
-
-              subject.milestone = milestone
-              subject.save!
-
-              expect(subject.milestones).to match_array([milestone])
-            end
-
-            it 'can nullify the milestone' do
-              expect(subject.milestones).to match_array([old_milestone])
-
-              subject.milestone = nil
-              subject.save!
-
-              expect(subject.milestones).to match_array([])
-            end
-          end
-        end
       end
     end