Merge branch 'bvl-adjust-design-versions-relation' into 'master'

Adjust relation between desings and versions to be many to many

See merge request gitlab-org/gitlab-ee!10552

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: 20190328210840) do
+ActiveRecord::Schema.define(version: 20190403161806) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1042,14 +1042,19 @@ ActiveRecord::Schema.define(version: 20190328210840) do
     t.integer "issue_id", null: false
     t.string "filename", null: false
     t.index ["issue_id", "filename"], name: "index_design_management_designs_on_issue_id_and_filename", unique: true, using: :btree
-    t.index ["issue_id"], name: "index_design_management_designs_on_issue_id", unique: true, using: :btree
     t.index ["project_id"], name: "index_design_management_designs_on_project_id", using: :btree
   end
 
+  create_table "design_management_designs_versions", id: false, force: :cascade do |t|
+    t.bigint "design_id", null: false
+    t.bigint "version_id", null: false
+    t.index ["design_id", "version_id"], name: "design_management_designs_versions_uniqueness", unique: true, using: :btree
+    t.index ["design_id"], name: "index_design_management_designs_versions_on_design_id", using: :btree
+    t.index ["version_id"], name: "index_design_management_designs_versions_on_version_id", using: :btree
+  end
+
   create_table "design_management_versions", id: :bigserial, force: :cascade do |t|
-    t.bigint "design_management_design_id", null: false
     t.binary "sha", null: false
-    t.index ["design_management_design_id"], name: "index_design_management_versions_on_design_management_design_id", using: :btree
     t.index ["sha"], name: "index_design_management_versions_on_sha", unique: true, using: :btree
   end
 
@@ -3475,7 +3480,8 @@ ActiveRecord::Schema.define(version: 20190328210840) do
   add_foreign_key "deployments", "projects", name: "fk_b9a3851b82", on_delete: :cascade
   add_foreign_key "design_management_designs", "issues", on_delete: :cascade
   add_foreign_key "design_management_designs", "projects", on_delete: :cascade
-  add_foreign_key "design_management_versions", "design_management_designs", on_delete: :cascade
+  add_foreign_key "design_management_designs_versions", "design_management_designs", column: "design_id", on_delete: :cascade
+  add_foreign_key "design_management_designs_versions", "design_management_versions", column: "version_id", on_delete: :cascade
   add_foreign_key "draft_notes", "merge_requests", on_delete: :cascade
   add_foreign_key "draft_notes", "users", column: "author_id", on_delete: :cascade
   add_foreign_key "elasticsearch_indexed_namespaces", "namespaces", on_delete: :cascade

ee/app/models/design_management/design.rb

@@ -4,10 +4,9 @@ module DesignManagement
   class Design < ApplicationRecord
     belongs_to :project
     belongs_to :issue
-    has_many :versions, class_name: 'DesignManagement::Version', inverse_of: :design
+    has_and_belongs_to_many :versions, class_name: 'DesignManagement::Version', inverse_of: :designs
 
     validates :project, :issue, :filename, presence: true
-    validates :issue, uniqueness: true
     validates :filename, uniqueness: { scope: :issue_id }
   end
 end

ee/app/models/design_management/version.rb

@@ -4,15 +4,15 @@ module DesignManagement
   class Version < ApplicationRecord
     include ShaAttribute
 
-    belongs_to :design, class_name: "DesignManagement::Design", foreign_key: 'design_management_design_id'
-    has_one :project, through: :design
-    has_one :issue, through: :design
+    has_and_belongs_to_many :designs,
+                            class_name: "DesignManagement::Design",
+                            inverse_of: :versions
 
     # This is a polymorphic association, so we can't count on FK's to delete the
     # data
     has_many :notes, as: :noteable, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
-    validates :sha, :design, presence: true
+    validates :sha, presence: true
     validates :sha, uniqueness: { case_sensitive: false }
 
     sha_attribute :sha

ee/db/migrate/20190402112450_add_design_management_designs_versions.rb

+# frozen_string_literal: true
+
+class AddDesignManagementDesignsVersions < ActiveRecord::Migration[5.0]
+  DOWNTIME = false
+
+  def change
+    create_table(:design_management_designs_versions, id: false) do |t|
+      t.references :design,
+                   null: false,
+                   type: :bigint,
+                   foreign_key: {
+                     on_delete: :cascade,
+                     to_table: :design_management_designs
+                   }
+      t.references :version,
+                   null: false,
+                   type: :bigint,
+                   foreign_key: {
+                     on_delete: :cascade,
+                     to_table: :design_management_versions
+                   }
+    end
+
+    add_index :design_management_designs_versions,
+              [:design_id, :version_id],
+              unique: true, name: "design_management_designs_versions_uniqueness"
+  end
+end

ee/db/migrate/20190403161806_update_designs_index.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 UpdateDesignsIndex < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    remove_concurrent_index :design_management_designs, :issue_id, unique: true
+  end
+
+  def down
+    add_concurrent_index :design_management_designs, :issue_id, unique: true
+  end
+end

ee/db/post_migrate/20190402111436_remove_design_id_from_design_versions.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 RemoveDesignIdFromDesignVersions < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    remove_foreign_key :design_management_versions, :design_management_designs
+    remove_column(:design_management_versions, :design_management_design_id)
+  end
+
+  def down
+    add_column(:design_management_versions, :design_management_design_id, :bigint)
+    add_concurrent_foreign_key :design_management_versions, :design_management_designs, column: :design_management_design_id
+  end
+end

ee/spec/factories/design_management/versions.rb

@@ -2,7 +2,6 @@
 
 FactoryBot.define do
   factory :design_version, class: DesignManagement::Version do
-    design
     sequence(:sha) { |n| Digest::SHA1.hexdigest("commit-like-#{n}") }
   end
 end

ee/spec/models/design_management/design_spec.rb

@@ -6,7 +6,7 @@ describe DesignManagement::Design do
   describe 'relations' do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:issue) }
-    it { is_expected.to have_many(:versions) }
+    it { is_expected.to have_and_belong_to_many(:versions) }
   end
 
   describe 'validations' do
@@ -16,7 +16,6 @@ describe DesignManagement::Design do
     it { is_expected.to validate_presence_of(:project) }
     it { is_expected.to validate_presence_of(:issue) }
     it { is_expected.to validate_presence_of(:filename) }
-    it { is_expected.to validate_uniqueness_of(:issue) }
     it { is_expected.to validate_uniqueness_of(:filename).scoped_to(:issue_id) }
   end
 end

ee/spec/models/design_management/version_spec.rb

@@ -3,9 +3,25 @@ require 'rails_helper'
 
 describe DesignManagement::Version do
   describe 'relations' do
-    it { is_expected.to belong_to(:design) }
-    it { is_expected.to have_one(:issue) }
-    it { is_expected.to have_one(:project) }
+    it { is_expected.to have_and_belong_to_many(:designs) }
+
+    it 'constrains the designs relation correctly' do
+      design = create(:design)
+      version = create(:design_version)
+
+      version.designs << design
+
+      expect { version.designs << design }.to raise_error(ActiveRecord::RecordNotUnique)
+    end
+
+    it 'allows adding multiple versions to a single design' do
+      design = create(:design)
+      versions = create_list(:design_version, 2)
+
+      expect { versions.each { |v| design.versions << v } }
+        .not_to raise_error
+    end
+
     it { is_expected.to have_many(:notes).dependent(:delete_all) }
   end
 
@@ -13,7 +29,6 @@ describe DesignManagement::Version do
     subject(:design_version) { build(:design_version) }
 
     it { is_expected.to be_valid }
-    it { is_expected.to validate_presence_of(:design) }
     it { is_expected.to validate_presence_of(:sha) }
     it { is_expected.to validate_uniqueness_of(:sha).case_insensitive }
   end