Merge branch '66986-milestone-release-many-to-many' into 'master'

Switched Milestone and Release to many-to-many relationships

See merge request gitlab-org/gitlab!16517

app/models/milestone.rb

@@ -27,11 +27,8 @@ class Milestone < ApplicationRecord
   belongs_to :project
   belongs_to :group
 
-  # A one-to-one relationship is set up here as part of a MVC: https://gitlab.com/gitlab-org/gitlab-ce/issues/62402
-  # However, on the long term, we will want a many-to-many relationship between Release and Milestone.
-  # The "has_one through" allows us today to set up this one-to-one relationship while setting up the architecture for the long-term (ie intermediate table).
-  has_one :milestone_release
-  has_one :release, through: :milestone_release
+  has_many :milestone_releases
+  has_many :releases, through: :milestone_releases
 
   has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.milestones&.maximum(:iid) }
   has_internal_id :iid, scope: :group, init: ->(s) { s&.group&.milestones&.maximum(:iid) }
@@ -68,7 +65,7 @@ class Milestone < ApplicationRecord
   validate :milestone_type_check
   validate :start_date_should_be_less_than_due_date, if: proc { |m| m.start_date.present? && m.due_date.present? }
   validate :dates_within_4_digits
-  validates_associated :milestone_release, message: -> (_, obj) { obj[:value].errors.full_messages.join(",") }
+  validates_associated :milestone_releases, message: -> (_, obj) { obj[:value].map(&:errors).map(&:full_messages).join(",") }
 
   strip_attributes :title
 

app/models/milestone_release.rb

@@ -4,9 +4,11 @@ class MilestoneRelease < ApplicationRecord
   belongs_to :milestone
   belongs_to :release
 
-  validates :milestone_id, uniqueness: { scope: [:release_id] }
   validate :same_project_between_milestone_and_release
 
+  # Keep until 2019-11-29
+  self.ignored_columns += %i[id]
+
   private
 
   def same_project_between_milestone_and_release

app/models/release.rb

@@ -12,11 +12,8 @@ class Release < ApplicationRecord
 
   has_many :links, class_name: 'Releases::Link'
 
-  # A one-to-one relationship is set up here as part of a MVC: https://gitlab.com/gitlab-org/gitlab-ce/issues/62402
-  # However, on the long term, we will want a many-to-many relationship between Release and Milestone.
-  # The "has_one through" allows us today to set up this one-to-one relationship while setting up the architecture for the long-term (ie intermediate table).
-  has_one :milestone_release
-  has_one :milestone, through: :milestone_release
+  has_many :milestone_releases
+  has_many :milestones, through: :milestone_releases
 
   default_value_for :released_at, allows_nil: false do
     Time.zone.now
@@ -26,7 +23,7 @@ class Release < ApplicationRecord
 
   validates :description, :project, :tag, presence: true
   validates :name, presence: true, on: :create
-  validates_associated :milestone_release, message: -> (_, obj) { obj[:value].errors.full_messages.join(",") }
+  validates_associated :milestone_releases, message: -> (_, obj) { obj[:value].map(&:errors).map(&:full_messages).join(",") }
 
   scope :sorted, -> { order(released_at: :desc) }
 

app/services/releases/concerns.rb

@@ -48,25 +48,29 @@ module Releases
         end
       end
 
-      def milestone
-        return unless params[:milestone]
+      def milestones
+        return [] unless param_for_milestone_titles_provided?
 
-        strong_memoize(:milestone) do
+        strong_memoize(:milestones) do
           MilestonesFinder.new(
             project: project,
             current_user: current_user,
             project_ids: Array(project.id),
-            title: params[:milestone]
-          ).execute.first
+            state: 'all',
+            title: params[:milestones]
+          ).execute
         end
       end
 
-      def inexistent_milestone?
-        params[:milestone] && !params[:milestone].empty? && !milestone
+      def inexistent_milestones
+        return [] unless param_for_milestone_titles_provided?
+
+        existing_milestone_titles = milestones.map(&:title)
+        Array(params[:milestones]) - existing_milestone_titles
       end
 
-      def param_for_milestone_title_provided?
-        params[:milestone].present? || params[:milestone]&.empty?
+      def param_for_milestone_titles_provided?
+        params.key?(:milestones)
       end
     end
   end

app/services/releases/create_service.rb

@@ -7,7 +7,7 @@ module Releases
     def execute
       return error('Access Denied', 403) unless allowed?
       return error('Release already exists', 409) if release
-      return error('Milestone does not exist', 400) if inexistent_milestone?
+      return error("Milestone(s) not found: #{inexistent_milestones.join(', ')}", 400) if inexistent_milestones.any?
 
       tag = ensure_tag
 
@@ -61,7 +61,7 @@ module Releases
         sha: tag.dereferenced_target.sha,
         released_at: released_at,
         links_attributes: params.dig(:assets, 'links') || [],
-        milestone: milestone
+        milestones: milestones
       )
     end
   end

app/services/releases/update_service.rb

@@ -9,9 +9,9 @@ module Releases
       return error('Release does not exist', 404) unless release
       return error('Access Denied', 403) unless allowed?
       return error('params is empty', 400) if empty_params?
-      return error('Milestone does not exist', 400) if inexistent_milestone?
+      return error("Milestone(s) not found: #{inexistent_milestones.join(', ')}", 400) if inexistent_milestones.any?
 
-      params[:milestone] = milestone if param_for_milestone_title_provided?
+      params[:milestones] = milestones if param_for_milestone_titles_provided?
 
       if release.update(params)
         success(tag: existing_tag, release: release)

changelogs/unreleased/66986-milestone-release-many-to-many.yml

+---
+title: Switch Milestone and Release to a many-to-many relationship
+merge_request: 16517
+author:
+type: changed

db/post_migrate/20190904205212_remove_id_column_from_intermediate_release_milestones.rb

+# frozen_string_literal: true
+
+class RemoveIdColumnFromIntermediateReleaseMilestones < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    remove_column :milestone_releases, :id, :bigint
+  end
+end

db/schema.rb

@@ -2208,7 +2208,7 @@ ActiveRecord::Schema.define(version: 2019_09_12_061145) do
     t.index ["user_id"], name: "index_merge_trains_on_user_id"
   end
 
-  create_table "milestone_releases", force: :cascade do |t|
+  create_table "milestone_releases", id: false, force: :cascade do |t|
     t.bigint "milestone_id", null: false
     t.bigint "release_id", null: false
     t.index ["milestone_id", "release_id"], name: "index_miletone_releases_on_milestone_and_release", unique: true

lib/api/entities.rb

@@ -1280,7 +1280,7 @@ module API
       expose :author, using: Entities::UserBasic, if: -> (release, _) { release.author.present? }
       expose :commit, using: Entities::Commit, if: lambda { |_, _| can_download_code? }
       expose :upcoming_release?, as: :upcoming_release
-      expose :milestone, using: Entities::Milestone, if: -> (release, _) { release.milestone.present? }
+      expose :milestones, using: Entities::Milestone, if: -> (release, _) { release.milestones.present? }
 
       expose :assets do
         expose :assets_count, as: :count do |release, _|

lib/api/releases.rb

@@ -54,7 +54,7 @@ module API
             requires :url, type: String
           end
         end
-        optional :milestone, type: String, desc: 'The title of the related milestone'
+        optional :milestones, type: Array, desc: 'The titles of the related milestones', default: []
         optional :released_at, type: DateTime, desc: 'The date when the release will be/was ready. Defaults to the current time.'
       end
       post ':id/releases' do
@@ -80,7 +80,7 @@ module API
         optional :name,        type: String, desc: 'The name of the release'
         optional :description, type: String, desc: 'Release notes with markdown support'
         optional :released_at, type: DateTime, desc: 'The date when the release will be/was ready.'
-        optional :milestone,   type: String, desc: 'The title of the related milestone'
+        optional :milestones,  type: Array, desc: 'The titles of the related milestones'
       end
       put ':id/releases/:tag_name', requirements: RELEASE_ENDPOINT_REQUIREMENTS do
         authorize_update_release!

spec/fixtures/api/schemas/public_api/v4/release.json

@@ -15,7 +15,10 @@
     "author": {
       "oneOf": [{ "type": "null" }, { "$ref": "user/basic.json" }]
     },
-    "milestone": { "type": "string" },
+    "milestones": {
+      "type": "array",
+      "items": { "$ref": "milestone.json" }
+    },
     "assets": {
       "required": ["count", "links", "sources"],
       "properties": {

spec/lib/gitlab/import_export/all_models.yml

@@ -65,8 +65,8 @@ milestone:
 - participants
 - events
 - boards
-- milestone_release
-- release
+- milestone_releases
+- releases
 snippets:
 - author
 - project
@@ -77,8 +77,8 @@ releases:
 - author
 - project
 - links
-- milestone_release
-- milestone
+- milestone_releases
+- milestones
 links:
 - release
 project_members:

spec/models/milestone_release_spec.rb

@@ -14,23 +14,29 @@ describe MilestoneRelease do
     it { is_expected.to belong_to(:release) }
   end
 
+  context 'when trying to create the same record in milestone_releases twice' do
+    it 'is not committing on the second time' do
+      create(:milestone_release, milestone: milestone, release: release)
+
+      expect do
+        subject.save!
+      end.to raise_error(ActiveRecord::RecordNotUnique)
+    end
+  end
+
   describe 'validations' do
-    it { is_expected.to validate_uniqueness_of(:milestone_id).scoped_to(:release_id) }
+    subject(:milestone_release) { build(:milestone_release, milestone: milestone, release: release) }
 
     context 'when milestone and release do not have the same project' do
       it 'is not valid' do
-        other_project = create(:project)
-        release = build(:release, project: other_project)
-        milestone_release = described_class.new(milestone: milestone, release: release)
+        milestone_release.release = build(:release, project: create(:project))
+
         expect(milestone_release).not_to be_valid
       end
     end
 
     context 'when milestone and release have the same project' do
-      it 'is valid' do
-        milestone_release = described_class.new(milestone: milestone, release: release)
-        expect(milestone_release).to be_valid
-      end
+      it { is_expected.to be_valid }
     end
   end
 end

spec/models/milestone_spec.rb

@@ -55,20 +55,20 @@ describe Milestone do
       end
     end
 
-    describe 'milestone_release' do
+    describe 'milestone_releases' do
       let(:milestone) { build(:milestone, project: project) }
 
       context 'when it is tied to a release for another project' do
         it 'creates a validation error' do
           other_project = create(:project)
-          milestone.release = build(:release, project: other_project)
+          milestone.releases << build(:release, project: other_project)
           expect(milestone).not_to be_valid
         end
       end
 
       context 'when it is tied to a release for the same project' do
         it 'is valid' do
-          milestone.release = build(:release, project: project)
+          milestone.releases << build(:release, project: project)
           expect(milestone).to be_valid
         end
       end
@@ -78,7 +78,8 @@ describe Milestone do
   describe "Associations" do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to have_many(:issues) }
-    it { is_expected.to have_one(:release) }
+    it { is_expected.to have_many(:releases) }
+    it { is_expected.to have_many(:milestone_releases) }
   end
 
   let(:project) { create(:project, :public) }

spec/models/release_spec.rb

@@ -13,7 +13,8 @@ RSpec.describe Release do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:author).class_name('User') }
     it { is_expected.to have_many(:links).class_name('Releases::Link') }
-    it { is_expected.to have_one(:milestone) }
+    it { is_expected.to have_many(:milestones) }
+    it { is_expected.to have_many(:milestone_releases) }
   end
 
   describe 'validation' do
@@ -38,15 +39,15 @@ RSpec.describe Release do
 
     context 'when a release is tied to a milestone for another project' do
       it 'creates a validation error' do
-        release.milestone = build(:milestone, project: create(:project))
-        expect(release).not_to be_valid
+        milestone = build(:milestone, project: create(:project))
+        expect { release.milestones << milestone }.to raise_error
       end
     end
 
     context 'when a release is tied to a milestone linked to the same project' do
-      it 'is valid' do
-        release.milestone = build(:milestone, project: project)
-        expect(release).to be_valid
+      it 'successfully links this release to this milestone' do
+        milestone = build(:milestone, project: project)
+        expect { release.milestones << milestone }.to change { MilestoneRelease.count }.by(1)
       end
     end
   end

spec/services/milestones/destroy_service_spec.rb

@@ -72,7 +72,7 @@ describe Milestones::DestroyService do
           :release,
           tag: 'v1.0',
           project: project,
-          milestone: milestone
+          milestones: [milestone]
         )
 
         expect { service.execute(milestone) }.not_to change { Release.count }

spec/services/releases/create_service_spec.rb

@@ -75,10 +75,12 @@ describe Releases::CreateService do
 
     context 'when a passed-in milestone does not exist for this project' do
       it 'raises an error saying the milestone is inexistent' do
-        service = described_class.new(project, user, params.merge!({ milestone: 'v111.0' }))
+        inexistent_milestone_tag = 'v111.0'
+        service = described_class.new(project, user, params.merge!({ milestones: [inexistent_milestone_tag] }))
         result = service.execute
+
         expect(result[:status]).to eq(:error)
-        expect(result[:message]).to eq('Milestone does not exist')
+        expect(result[:message]).to eq("Milestone(s) not found: #{inexistent_milestone_tag}")
       end
     end
   end
@@ -93,10 +95,10 @@ describe Releases::CreateService do
     context 'when existing milestone is passed in' do
       let(:title) { 'v1.0' }
       let(:milestone) { create(:milestone, :active, project: project, title: title) }
-      let(:params_with_milestone) { params.merge!({ milestone: title }) }
+      let(:params_with_milestone) { params.merge!({ milestones: [title] }) }
+      let(:service) { described_class.new(milestone.project, user, params_with_milestone) }
 
       it 'creates a release and ties this milestone to it' do
-        service = described_class.new(milestone.project, user, params_with_milestone)
         result = service.execute
 
         expect(project.releases.count).to eq(1)
@@ -104,29 +106,66 @@ describe Releases::CreateService do
 
         release = project.releases.last
 
-        expect(release.milestone).to eq(milestone)
+        expect(release.milestones).to match_array([milestone])
       end
 
       context 'when another release was previously created with that same milestone linked' do
         it 'also creates another release tied to that same milestone' do
-          other_release = create(:release, milestone: milestone, project: project, tag: 'v1.0')
-          service = described_class.new(milestone.project, user, params_with_milestone)
+          other_release = create(:release, milestones: [milestone], project: project, tag: 'v1.0')
           service.execute
           release = project.releases.last
 
-          expect(release.milestone).to eq(milestone)
-          expect(other_release.milestone).to eq(milestone)
+          expect(release.milestones).to match_array([milestone])
+          expect(other_release.milestones).to match_array([milestone])
           expect(release.id).not_to eq(other_release.id)
         end
       end
     end
 
+    context 'when multiple existing milestone titles are passed in' do
+      let(:title_1) { 'v1.0' }
+      let(:title_2) { 'v1.0-rc' }
+      let!(:milestone_1) { create(:milestone, :active, project: project, title: title_1) }
+      let!(:milestone_2) { create(:milestone, :active, project: project, title: title_2) }
+      let!(:params_with_milestones) { params.merge!({ milestones: [title_1, title_2] }) }
+
+      it 'creates a release and ties it to these milestones' do
+        described_class.new(project, user, params_with_milestones).execute
+        release = project.releases.last
+
+        expect(release.milestones.map(&:title)).to include(title_1, title_2)
+      end
+    end
+
+    context 'when multiple miletone titles are passed in but one of them does not exist' do
+      let(:title) { 'v1.0' }
+      let(:inexistent_title) { 'v111.0' }
+      let!(:milestone) { create(:milestone, :active, project: project, title: title) }
+      let!(:params_with_milestones) { params.merge!({ milestones: [title, inexistent_title] }) }
+      let(:service) { described_class.new(milestone.project, user, params_with_milestones) }
+
+      it 'raises an error' do
+        result = service.execute
+
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq("Milestone(s) not found: #{inexistent_title}")
+      end
+
+      it 'does not create any release' do
+        expect do
+          service.execute
+        end.not_to change(Release, :count)
+      end
+    end
+
     context 'when no milestone is passed in' do
       it 'creates a release without a milestone tied to it' do
-        expect(params.key? :milestone).to be_falsey
+        expect(params.key? :milestones).to be_falsey
+
         service.execute
         release = project.releases.last
-        expect(release.milestone).to be_nil
+
+        expect(release.milestones).to be_empty
       end
 
       it 'does not create any new MilestoneRelease object' do
@@ -136,10 +175,11 @@ describe Releases::CreateService do
 
     context 'when an empty value is passed as a milestone' do
       it 'creates a release without a milestone tied to it' do
-        service = described_class.new(project, user, params.merge!({ milestone: '' }))
+        service = described_class.new(project, user, params.merge!({ milestones: [] }))
         service.execute
         release = project.releases.last
-        expect(release.milestone).to be_nil
+
+        expect(release.milestones).to be_empty
       end
     end
   end

spec/services/releases/destroy_service_spec.rb

@@ -60,7 +60,7 @@ describe Releases::DestroyService do
 
     context 'when a milestone is tied to the release' do
       let!(:milestone) { create(:milestone, :active, project: project, title: 'v1.0') }
-      let!(:release) { create(:release, milestone: milestone, project: project, tag: tag) }
+      let!(:release) { create(:release, milestones: [milestone], project: project, tag: tag) }
 
       it 'destroys the release but leave the milestone intact' do
         expect { subject }.not_to change { Milestone.count }

spec/services/releases/update_service_spec.rb

@@ -50,39 +50,60 @@ describe Releases::UpdateService do
     end
 
     context 'when a milestone is passed in' do
-      let(:old_title) { 'v1.0' }
       let(:new_title) { 'v2.0' }
-      let(:milestone) { create(:milestone, project: project, title: old_title) }
+      let(:milestone) { create(:milestone, project: project, title: 'v1.0') }
       let(:new_milestone) { create(:milestone, project: project, title: new_title) }
-      let(:params_with_milestone) { params.merge!({ milestone: new_title }) }
+      let(:params_with_milestone) { params.merge!({ milestones: [new_title] }) }
+      let(:service) { described_class.new(new_milestone.project, user, params_with_milestone) }
 
       before do
-        release.milestone = milestone
-        release.save!
+        release.milestones << milestone
 
-        described_class.new(new_milestone.project, user, params_with_milestone).execute
+        service.execute
         release.reload
       end
 
       it 'updates the related milestone accordingly' do
-        expect(release.milestone.title).to eq(new_title)
+        expect(release.milestones.first.title).to eq(new_title)
       end
     end
 
     context "when an 'empty' milestone is passed in" do
       let(:milestone) { create(:milestone, project: project, title: 'v1.0') }
-      let(:params_with_empty_milestone) { params.merge!({ milestone: '' }) }
+      let(:params_with_empty_milestone) { params.merge!({ milestones: [] }) }
 
       before do
-        release.milestone = milestone
-        release.save!
+        release.milestones << milestone
 
-        described_class.new(milestone.project, user, params_with_empty_milestone).execute
+        service.params = params_with_empty_milestone
+        service.execute
         release.reload
       end
 
       it 'removes the old milestone and does not associate any new milestone' do
-        expect(release.milestone).to be_nil
+        expect(release.milestones).not_to be_present
+      end
+    end
+
+    context "when multiple new milestones are passed in" do
+      let(:new_title_1) { 'v2.0' }
+      let(:new_title_2) { 'v2.0-rc' }
+      let(:milestone) { create(:milestone, project: project, title: 'v1.0') }
+      let(:params_with_milestones) { params.merge!({ milestones: [new_title_1, new_title_2] }) }
+      let(:service) { described_class.new(project, user, params_with_milestones) }
+
+      before do
+        create(:milestone, project: project, title: new_title_1)
+        create(:milestone, project: project, title: new_title_2)
+        release.milestones << milestone
+
+        service.execute
+        release.reload
+      end
+
+      it 'removes the old milestone and update the release with the new ones' do
+        milestone_titles = release.milestones.map(&:title)
+        expect(milestone_titles).to match_array([new_title_1, new_title_2])
       end
     end
   end