Made multiple adjustment based on code review62402-milestone-release-be

Added some validations related to milestones and releases.
Cleaned rspec files to follow guidelines.
Fixed typos and removed whitespaces.
Added rspec for MilestoneRelease.
Added some comments to explain context of release-milestone MVC.

18 files changed, 149 insertions, 46 deletions

diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index 7a7829f1d83..101e963ea29 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -24,6 +24,9 @@ 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
 
@@ -62,6 +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(",") }
 
   strip_attributes :title
 
diff --git a/app/models/milestone_release.rb b/app/models/milestone_release.rb
index 8ae4fabe5e0..c8743a8cad8 100644
--- a/app/models/milestone_release.rb
+++ b/app/models/milestone_release.rb
@@ -3,4 +3,15 @@
 class MilestoneRelease < ApplicationRecord
   belongs_to :milestone
   belongs_to :release
+
+  validates :milestone_id, uniqueness: { scope: [:release_id] }
+  validate :same_project_between_milestone_and_release
+
+  private
+
+  def same_project_between_milestone_and_release
+    return if milestone&.project_id == release&.project_id
+
+    errors.add(:base, 'does not have the same project as the milestone')
+  end
 end
diff --git a/app/models/release.rb b/app/models/release.rb
index ca6ffc0d242..b2e65974aa0 100644
--- a/app/models/release.rb
+++ b/app/models/release.rb
@@ -12,6 +12,9 @@ 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
 
@@ -23,6 +26,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(",") }
 
   scope :sorted, -> { order(released_at: :desc) }
 
diff --git a/app/services/releases/concerns.rb b/app/services/releases/concerns.rb
index 368331698dd..b5412e97284 100644
--- a/app/services/releases/concerns.rb
+++ b/app/services/releases/concerns.rb
@@ -49,25 +49,24 @@ module Releases
       end
 
       def milestone
-        return unless params[:milestone] || !empty_milestone?
+        return unless params[:milestone]
 
         strong_memoize(:milestone) do
           MilestonesFinder.new(
             project: project,
             current_user: current_user,
-            project_ids: project.id,
-            group_ids: project.group&.id,
+            project_ids: Array(project.id),
             title: params[:milestone]
           ).execute.first
         end
       end
 
-      def empty_milestone?
-        params[:milestone] && params[:milestone].empty?
+      def inexistent_milestone?
+        params[:milestone] && !params[:milestone].empty? && !milestone
       end
 
-      def inexistant_milestone?
-        params[:milestone] && !empty_milestone? && !milestone
+      def param_for_milestone_title_provided?
+        params[:milestone].present? || params[:milestone]&.empty?
       end
     end
   end
diff --git a/app/services/releases/create_service.rb b/app/services/releases/create_service.rb
index 01de29f9b0f..c91d43084d3 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('Milestone does not exist', 400) if inexistant_milestone?
+      return error('Milestone does not exist', 400) if inexistent_milestone?
 
       tag = ensure_tag
 
diff --git a/app/services/releases/update_service.rb b/app/services/releases/update_service.rb
index a1e3e60667c..70acc68f747 100644
--- a/app/services/releases/update_service.rb
+++ b/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 inexistant_milestone?
+      return error('Milestone does not exist', 400) if inexistent_milestone?
 
-      params[:milestone] = milestone if params[:milestone]
+      params[:milestone] = milestone if param_for_milestone_title_provided?
 
       if release.update(params)
         success(tag: existing_tag, release: release)
diff --git a/db/migrate/20190722144316_create_milestone_releases_table.rb b/db/migrate/20190722144316_create_milestone_releases_table.rb
index 8bf98cf97f3..55878bcec41 100644
--- a/db/migrate/20190722144316_create_milestone_releases_table.rb
+++ b/db/migrate/20190722144316_create_milestone_releases_table.rb
@@ -7,9 +7,11 @@ class CreateMilestoneReleasesTable < ActiveRecord::Migration[5.2]
 
   def up
     create_table :milestone_releases do |t|
-      t.references :milestone, foreign_key: { on_delete: :cascade }, null: false
+      t.references :milestone, foreign_key: { on_delete: :cascade }, null: false, index: false
       t.references :release, foreign_key: { on_delete: :cascade }, null: false
     end
+
+    add_index :milestone_releases, [:milestone_id, :release_id], unique: true, name: 'index_miletone_releases_on_milestone_and_release'
   end
 
   def down
diff --git a/db/schema.rb b/db/schema.rb
index 68d7ad80485..37fbebd9607 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -2161,7 +2161,7 @@ ActiveRecord::Schema.define(version: 2019_09_02_131045) do
   create_table "milestone_releases", force: :cascade do |t|
     t.bigint "milestone_id", null: false
     t.bigint "release_id", null: false
-    t.index ["milestone_id"], name: "index_milestone_releases_on_milestone_id"
+    t.index ["milestone_id", "release_id"], name: "index_miletone_releases_on_milestone_and_release", unique: true
     t.index ["release_id"], name: "index_milestone_releases_on_release_id"
   end
 
diff --git a/doc/api/releases/index.md b/doc/api/releases/index.md
index 65aa4859f9b..8d5b3a65789 100644
--- a/doc/api/releases/index.md
+++ b/doc/api/releases/index.md
@@ -374,14 +374,14 @@ Update a Release.
 PUT /projects/:id/releases/:tag_name
 ```
 
-| Attribute     | Type           | Required | Description                                                                                              |
-| ------------- | -------------- | -------- | -------------------------------------------------------------------------------------------------------- |
-| `id`          | integer/string | yes      | The ID or [URL-encoded path of the project](../README.md#namespaced-path-encoding).                      |
-| `tag_name`    | string         | yes      | The tag where the release will be created from.                                                          |
-| `name`        | string         | no       | The release name.                                                                                        |
-| `description` | string         | no       | The description of the release. You can use [markdown](../../user/markdown.md).                          |
-| `milestone`   | string         | no       | The title of the milestone to associate with the release (`""` to remove the milestone from the release) |
-| `released_at` | datetime       | no       | The date when the release will be/was ready. Expected in ISO 8601 format (`2019-03-15T08:00:00Z`). |
+| Attribute     | Type           | Required | Description                                                                                               |
+| ------------- | -------------- | -------- | --------------------------------------------------------------------------------------------------------- |
+| `id`          | integer/string | yes      | The ID or [URL-encoded path of the project](../README.md#namespaced-path-encoding).                       |
+| `tag_name`    | string         | yes      | The tag where the release will be created from.                                                           |
+| `name`        | string         | no       | The release name.                                                                                         |
+| `description` | string         | no       | The description of the release. You can use [markdown](../../user/markdown.md).                           |
+| `milestone`   | string         | no       | The title of the milestone to associate with the release (`""` to remove the milestone from the release). |
+| `released_at` | datetime       | no       | The date when the release will be/was ready. Expected in ISO 8601 format (`2019-03-15T08:00:00Z`).        |
 
 Example request:
 
diff --git a/lib/api/releases.rb b/lib/api/releases.rb
index 114b94b46ef..11a9a085068 100644
--- a/lib/api/releases.rb
+++ b/lib/api/releases.rb
@@ -54,8 +54,7 @@ module API
             requires :url, type: String
           end
         end
-        optional :milestone,   type: String, desc: 'The title of the related milestone'
-
+        optional :milestone, type: String, desc: 'The title of the related milestone'
         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/factories/milestone_releases.rb b/spec/factories/milestone_releases.rb
new file mode 100644
index 00000000000..08e109480ab
--- /dev/null
+++ b/spec/factories/milestone_releases.rb
@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :milestone_release do
+    milestone
+    release
+
+    before(:create, :build) do |mr|
+      project = create(:project)
+      mr.milestone.project = project
+      mr.release.project = project
+    end
+  end
+end
diff --git a/spec/models/milestone_release_spec.rb b/spec/models/milestone_release_spec.rb
new file mode 100644
index 00000000000..d6f73275977
--- /dev/null
+++ b/spec/models/milestone_release_spec.rb
@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MilestoneRelease do
+  let(:project) { create(:project) }
+  let(:release) { create(:release, project: project) }
+  let(:milestone) { create(:milestone, project: project) }
+
+  subject { build(:milestone_release, release: release, milestone: milestone) }
+
+  describe 'associations' do
+    it { is_expected.to belong_to(:milestone) }
+    it { is_expected.to belong_to(:release) }
+  end
+
+  describe 'validations' do
+    it { is_expected.to validate_uniqueness_of(:milestone_id).scoped_to(:release_id) }
+
+    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)
+        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
+    end
+  end
+end
diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb
index 3704a2d468d..64030f5b92a 100644
--- a/spec/models/milestone_spec.rb
+++ b/spec/models/milestone_spec.rb
@@ -54,11 +54,31 @@ describe Milestone do
         expect(milestone.errors[:due_date]).to include("date must not be after 9999-12-31")
       end
     end
+
+    describe 'milestone_release' 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)
+          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)
+          expect(milestone).to be_valid
+        end
+      end
+    end
   end
 
   describe "Associations" do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to have_many(:issues) }
+    it { is_expected.to have_one(:release) }
   end
 
   let(:project) { create(:project, :public) }
diff --git a/spec/models/release_spec.rb b/spec/models/release_spec.rb
index e9d846e7291..29ce9f762b0 100644
--- a/spec/models/release_spec.rb
+++ b/spec/models/release_spec.rb
@@ -13,6 +13,7 @@ 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) }
   end
 
   describe 'validation' do
@@ -34,6 +35,20 @@ RSpec.describe Release do
         expect(existing_release_without_name.name).to be_nil
       end
     end
+
+    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
+      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
+      end
+    end
   end
 
   describe '#assets_count' do
diff --git a/spec/services/milestones/destroy_service_spec.rb b/spec/services/milestones/destroy_service_spec.rb
index 2054e428177..ff1e1256166 100644
--- a/spec/services/milestones/destroy_service_spec.rb
+++ b/spec/services/milestones/destroy_service_spec.rb
@@ -67,11 +67,16 @@ describe Milestones::DestroyService do
     end
 
     context 'when a release is tied to a milestone' do
-      let!(:release) { create(:release, milestone: milestone, project: project, tag: 'v1.0') }
-
       it 'destroys the milestone but not the associated release' do
-        expect(milestone.release).to eq release
+        release = create(
+          :release,
+          tag: 'v1.0',
+          project: project,
+          milestone: milestone
+        )
+
         expect { service.execute(milestone) }.not_to change { Release.count }
+        expect(release.reload).to be_persisted
       end
     end
   end
diff --git a/spec/services/releases/create_service_spec.rb b/spec/services/releases/create_service_spec.rb
index 7369adc14cc..5c9d6537df1 100644
--- a/spec/services/releases/create_service_spec.rb
+++ b/spec/services/releases/create_service_spec.rb
@@ -74,15 +74,11 @@ describe Releases::CreateService do
     end
 
     context 'when a passed-in milestone does not exist for this project' do
-      let(:milestone) { create(:milestone, project: project, title: 'v1.0') }
-      let(:inexistant_milestone_title) { 'v111.0' }
-      let(:params_with_milestone) { params.merge!({ milestone: inexistant_milestone_title }) }
-
-      it 'raises an error saying the milestone is inexistant' do
-        service = described_class.new(project, user, params_with_milestone)
+      it 'raises an error saying the milestone is inexistent' do
+        service = described_class.new(project, user, params.merge!({ milestone: 'v111.0' }))
         result = service.execute
         expect(result[:status]).to eq(:error)
-        expect(result[:message]).to eq("Milestone does not exist")
+        expect(result[:message]).to eq('Milestone does not exist')
       end
     end
   end
@@ -108,19 +104,19 @@ describe Releases::CreateService do
 
         release = project.releases.last
 
-        expect(release.milestone.title).to eq title
+        expect(release.milestone).to eq(milestone)
       end
 
       context 'when another release was previously created with that same milestone linked' do
-        let(:other_release) { create(:release, milestone: milestone, project: project, tag: 'v1.0') }
-
         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)
           service.execute
           release = project.releases.last
 
-          expect(release.milestone).to eq milestone
-          expect(other_release.milestone).to eq milestone
+          expect(release.milestone).to eq(milestone)
+          expect(other_release.milestone).to eq(milestone)
+          expect(release.id).not_to eq(other_release.id)
         end
       end
     end
@@ -132,13 +128,15 @@ describe Releases::CreateService do
         release = project.releases.last
         expect(release.milestone).to be_nil
       end
+
+      it 'does not create any new MilestoneRelease object' do
+        expect { service.execute }.not_to change { MilestoneRelease.count }
+      end
     end
 
     context 'when an empty value is passed as a milestone' do
-      let(:params_with_empty_milestone) { params.merge!({ milestone: '' }) }
-
       it 'creates a release without a milestone tied to it' do
-        service = described_class.new(project, user, params_with_empty_milestone)
+        service = described_class.new(project, user, params.merge!({ milestone: '' }))
         service.execute
         release = project.releases.last
         expect(release.milestone).to be_nil
diff --git a/spec/services/releases/destroy_service_spec.rb b/spec/services/releases/destroy_service_spec.rb
index c532d13cd9e..c3172e5edbc 100644
--- a/spec/services/releases/destroy_service_spec.rb
+++ b/spec/services/releases/destroy_service_spec.rb
@@ -63,8 +63,8 @@ describe Releases::DestroyService do
       let!(:release) { create(:release, milestone: milestone, project: project, tag: tag) }
 
       it 'destroys the release but leave the milestone intact' do
-        expect(release.milestone).to eq milestone
         expect { subject }.not_to change { Milestone.count }
+        expect(milestone.reload).to be_persisted
       end
     end
   end
diff --git a/spec/services/releases/update_service_spec.rb b/spec/services/releases/update_service_spec.rb
index df958857e96..944f3d8c9ad 100644
--- a/spec/services/releases/update_service_spec.rb
+++ b/spec/services/releases/update_service_spec.rb
@@ -65,11 +65,7 @@ describe Releases::UpdateService do
       end
 
       it 'updates the related milestone accordingly' do
-        expect(release.milestone.title).to eq new_title
-      end
-
-      it 'removes the old milestone' do
-        expect(release.milestone.title).not_to eq old_title
+        expect(release.milestone.title).to eq(new_title)
       end
     end