Fixed tests and classes based on review comments66986-milestone-release-many-to-many

Updated Release services spec files.
Fixed Post-migration file.
Removed obsolete comments.
Fixed coding style in spec files.

12 files changed, 51 insertions, 53 deletions

diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index e46e087e64a..2fb4b35b2ca 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -25,9 +25,6 @@ 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_many :milestone_releases
   has_many :releases, through: :milestone_releases
 
diff --git a/app/models/release.rb b/app/models/release.rb
index 96156baa5e5..cd63b4d5fef 100644
--- a/app/models/release.rb
+++ b/app/models/release.rb
@@ -12,9 +12,6 @@ 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_many :milestone_releases
   has_many :milestones, through: :milestone_releases
 
diff --git a/app/services/releases/concerns.rb b/app/services/releases/concerns.rb
index 469e099ab07..c791b1609c0 100644
--- a/app/services/releases/concerns.rb
+++ b/app/services/releases/concerns.rb
@@ -49,7 +49,7 @@ module Releases
       end
 
       def milestones
-        return [] unless params[:milestones]
+        return [] unless param_for_milestone_titles_provided?
 
         strong_memoize(:milestones) do
           MilestonesFinder.new(
@@ -63,14 +63,14 @@ module Releases
       end
 
       def inexistent_milestones
-        return unless params[:milestones] && !params[:milestones].empty?
+        return [] unless param_for_milestone_titles_provided?
 
         existing_milestone_titles = milestones.map(&:title)
-        (params[:milestones] - existing_milestone_titles).join(', ')
+        params[:milestones] - existing_milestone_titles
       end
 
       def param_for_milestone_titles_provided?
-        params[:milestones].present? || params[:milestones]&.empty?
+        params.key?(:milestones)
       end
     end
   end
diff --git a/app/services/releases/create_service.rb b/app/services/releases/create_service.rb
index bf3444ec36e..9a0a876454f 100644
--- a/app/services/releases/create_service.rb
+++ b/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("Inexistent milestone(s): #{inexistent_milestones}", 400) if inexistent_milestones.present?
+      return error("Milestone(s) not found: #{inexistent_milestones.join(', ')}", 400) if inexistent_milestones.any?
 
       tag = ensure_tag
 
diff --git a/app/services/releases/update_service.rb b/app/services/releases/update_service.rb
index 2c5e805fcfd..7aa51c4a332 100644
--- a/app/services/releases/update_service.rb
+++ b/app/services/releases/update_service.rb
@@ -9,7 +9,7 @@ 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("Inexistent milestone(s): #{inexistent_milestones}", 400) if inexistent_milestones.present?
+      return error("Milestone(s) not found: #{inexistent_milestones.join(', ')}", 400) if inexistent_milestones.any?
 
       params[:milestones] = milestones if param_for_milestone_titles_provided?
 
diff --git a/db/post_migrate/20190904205212_remove_id_column_from_intermediate_release_milestones.rb b/db/post_migrate/20190904205212_remove_id_column_from_intermediate_release_milestones.rb
index 6a53b6721e6..e4b65d26db6 100644
--- a/db/post_migrate/20190904205212_remove_id_column_from_intermediate_release_milestones.rb
+++ b/db/post_migrate/20190904205212_remove_id_column_from_intermediate_release_milestones.rb
@@ -1,8 +1,6 @@
 # frozen_string_literal: true
 
 class RemoveIdColumnFromIntermediateReleaseMilestones < ActiveRecord::Migration[5.2]
-  include Gitlab::Database::MigrationHelpers
-
   DOWNTIME = false
 
   def change
diff --git a/lib/api/releases.rb b/lib/api/releases.rb
index a9bbd81e6d3..4238529142c 100644
--- a/lib/api/releases.rb
+++ b/lib/api/releases.rb
@@ -54,7 +54,7 @@ module API
             requires :url, type: String
           end
         end
-        optional :milestones, type: Array, desc: 'The titles of the related milestones'
+        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
diff --git a/spec/models/milestone_release_spec.rb b/spec/models/milestone_release_spec.rb
index 95ff45deaa8..28cec7bbc17 100644
--- a/spec/models/milestone_release_spec.rb
+++ b/spec/models/milestone_release_spec.rb
@@ -14,30 +14,29 @@ describe MilestoneRelease do
     it { is_expected.to belong_to(:release) }
   end
 
-  describe 'validations' do
-    context 'when trying to create the same record in milestone_releases twice' do
-      it 'is not committing on the second time' do
-        described_class.create!(milestone_id: milestone.id, release_id: release.id)
-        expect do
-          described_class.create!(milestone_id: milestone.id, release_id: release.id)
-        end.to raise_error(ActiveRecord::RecordNotUnique)
-      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
+    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
diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb
index 4a03bb46b78..0c4952eebd7 100644
--- a/spec/models/milestone_spec.rb
+++ b/spec/models/milestone_spec.rb
@@ -79,6 +79,7 @@ describe Milestone do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to have_many(:issues) }
     it { is_expected.to have_many(:releases) }
+    it { is_expected.to have_many(:milestone_releases) }
   end
 
   let(:project) { create(:project, :public) }
diff --git a/spec/models/release_spec.rb b/spec/models/release_spec.rb
index c9000f268c0..8714c67f29d 100644
--- a/spec/models/release_spec.rb
+++ b/spec/models/release_spec.rb
@@ -14,6 +14,7 @@ RSpec.describe Release do
     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_many(:milestones) }
+    it { is_expected.to have_many(:milestone_releases) }
   end
 
   describe 'validation' do
diff --git a/spec/services/releases/create_service_spec.rb b/spec/services/releases/create_service_spec.rb
index 9f04574201a..b624b9475e3 100644
--- a/spec/services/releases/create_service_spec.rb
+++ b/spec/services/releases/create_service_spec.rb
@@ -78,8 +78,9 @@ describe Releases::CreateService do
         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("Inexistent milestone(s): #{inexistent_milestone_tag}")
+        expect(result[:message]).to eq("Milestone(s) not found: #{inexistent_milestone_tag}")
       end
     end
   end
@@ -95,9 +96,9 @@ describe Releases::CreateService do
       let(:title) { 'v1.0' }
       let(:milestone) { create(:milestone, :active, project: project, title: 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)
@@ -105,18 +106,17 @@ describe Releases::CreateService do
 
         release = project.releases.last
 
-        expect(release.milestones.first).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, milestones: [milestone], project: project, tag: 'v1.0')
-          service = described_class.new(milestone.project, user, params_with_milestone)
           service.execute
           release = project.releases.last
 
-          expect(release.milestones.first).to eq(milestone)
-          expect(other_release.milestones.first).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
@@ -131,8 +131,8 @@ describe Releases::CreateService do
 
       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
@@ -142,17 +142,18 @@ describe Releases::CreateService do
       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 = described_class.new(milestone.project, user, params_with_milestones).execute
+        result = service.execute
 
         expect(result[:status]).to eq(:error)
-        expect(result[:message]).to eq("Inexistent milestone(s): #{inexistent_title}")
+        expect(result[:message]).to eq("Milestone(s) not found: #{inexistent_title}")
       end
 
       it 'does not create any release' do
         expect do
-          described_class.new(milestone.project, user, params_with_milestones).execute
+          service.execute
         end.not_to change(Release, :count)
       end
     end
@@ -160,9 +161,11 @@ describe Releases::CreateService do
     context 'when no milestone is passed in' do
       it 'creates a release without a milestone tied to it' do
         expect(params.key? :milestones).to be_falsey
+
         service.execute
         release = project.releases.last
-        expect(release.milestones).not_to be_present
+
+        expect(release.milestones).to be_empty
       end
 
       it 'does not create any new MilestoneRelease object' do
@@ -175,7 +178,8 @@ describe Releases::CreateService do
         service = described_class.new(project, user, params.merge!({ milestones: [] }))
         service.execute
         release = project.releases.last
-        expect(release.milestones).not_to be_present
+
+        expect(release.milestones).to be_empty
       end
     end
   end
diff --git a/spec/services/releases/update_service_spec.rb b/spec/services/releases/update_service_spec.rb
index 4fd8854a231..178bac3574f 100644
--- a/spec/services/releases/update_service_spec.rb
+++ b/spec/services/releases/update_service_spec.rb
@@ -50,16 +50,16 @@ 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!({ milestones: [new_title] }) }
+      let(:service) { described_class.new(new_milestone.project, user, params_with_milestone) }
 
       before do
         release.milestones << milestone
 
-        described_class.new(new_milestone.project, user, params_with_milestone).execute
+        service.execute
         release.reload
       end
 
@@ -75,7 +75,8 @@ describe Releases::UpdateService do
       before do
         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
 
@@ -85,24 +86,24 @@ describe Releases::UpdateService do
     end
 
     context "when multiple new milestones are passed in" do
-      let(:old_title) { 'v1.0' }
       let(:new_title_1) { 'v2.0' }
       let(:new_title_2) { 'v2.0-rc' }
-      let(:milestone) { create(:milestone, project: project, title: old_title) }
-      let(:new_milestone_1) { create(:milestone, project: project, title: new_title_1) }
-      let!(:new_milestone_2) { create(:milestone, project: project, title: new_title_2) }
+      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
 
-        described_class.new(new_milestone_1.project, user, params_with_milestones).execute
+        service.execute
         release.reload
       end
 
       it 'removes the old milestone and update the release with the new ones' do
-        expect(release.milestones.map(&:title)).to include(new_title_1, new_title_2)
-        expect(release.milestones.map(&:title)).not_to include(old_title)
+        milestone_titles = release.milestones.map(&:title)
+        expect(milestone_titles).to match_array([new_title_1, new_title_2])
       end
     end
   end