Merge branch '62402-milestone-release-be' into 'master'

"Allow milestones to be associated with releases" BE

See merge request gitlab-org/gitlab-ce!30816

app/models/milestone.rb

@@ -24,6 +24,12 @@ 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_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) }
 
@@ -59,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
 

app/models/milestone_release.rb

+# frozen_string_literal: true
+
+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

app/models/release.rb

@@ -12,6 +12,12 @@ 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
+
   default_value_for :released_at, allows_nil: false do
     Time.zone.now
   end
@@ -20,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) }
 

app/services/releases/concerns.rb

@@ -47,6 +47,27 @@ module Releases
           project.repository
         end
       end
+
+      def milestone
+        return unless params[:milestone]
+
+        strong_memoize(:milestone) do
+          MilestonesFinder.new(
+            project: project,
+            current_user: current_user,
+            project_ids: Array(project.id),
+            title: params[:milestone]
+          ).execute.first
+        end
+      end
+
+      def inexistent_milestone?
+        params[:milestone] && !params[:milestone].empty? && !milestone
+      end
+
+      def param_for_milestone_title_provided?
+        params[:milestone].present? || params[:milestone]&.empty?
+      end
     end
   end
 end

app/services/releases/create_service.rb

@@ -7,6 +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?
 
       tag = ensure_tag
 
@@ -59,7 +60,8 @@ module Releases
         tag: tag.name,
         sha: tag.dereferenced_target.sha,
         released_at: released_at,
-        links_attributes: params.dig(:assets, 'links') || []
+        links_attributes: params.dig(:assets, 'links') || [],
+        milestone: milestone
       )
     end
   end

app/services/releases/update_service.rb

@@ -9,6 +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?
+
+      params[:milestone] = milestone if param_for_milestone_title_provided?
 
       if release.update(params)
         success(tag: existing_tag, release: release)

changelogs/unreleased/62402-milestone-release-be.yml

+---
+title: Allow milestones to be associated with a release (backend)
+merge_request: 30816
+author:
+type: added

db/migrate/20190722144316_create_milestone_releases_table.rb

+# frozen_string_literal: true
+
+class CreateMilestoneReleasesTable < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    create_table :milestone_releases do |t|
+      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
+    drop_table :milestone_releases
+  end
+end

db/schema.rb

@@ -2158,6 +2158,13 @@ ActiveRecord::Schema.define(version: 2019_09_02_131045) do
     t.index ["user_id"], name: "index_merge_trains_on_user_id"
   end
 
+  create_table "milestone_releases", 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
+    t.index ["release_id"], name: "index_milestone_releases_on_release_id"
+  end
+
   create_table "milestones", id: :serial, force: :cascade do |t|
     t.string "title", null: false
     t.integer "project_id"
@@ -3932,6 +3939,8 @@ ActiveRecord::Schema.define(version: 2019_09_02_131045) do
   add_foreign_key "merge_trains", "merge_requests", on_delete: :cascade
   add_foreign_key "merge_trains", "projects", column: "target_project_id", on_delete: :cascade
   add_foreign_key "merge_trains", "users", on_delete: :cascade
+  add_foreign_key "milestone_releases", "milestones", on_delete: :cascade
+  add_foreign_key "milestone_releases", "releases", on_delete: :cascade
   add_foreign_key "milestones", "namespaces", column: "group_id", name: "fk_95650a40d4", on_delete: :cascade
   add_foreign_key "milestones", "projects", name: "fk_9bd0a0c791", on_delete: :cascade
   add_foreign_key "namespace_aggregation_schedules", "namespaces", on_delete: :cascade

lib/api/entities.rb

@@ -1229,6 +1229,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 :assets do
         expose :assets_count, as: :count do |release, _|

lib/api/releases.rb

@@ -54,6 +54,7 @@ module API
             requires :url, type: String
           end
         end
+        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
@@ -79,6 +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'
       end
       put ':id/releases/:tag_name', requirements: RELEASE_ENDPOINT_REQUIREMETS do
         authorize_update_release!

spec/factories/milestone_releases.rb

+# 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

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

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

spec/lib/gitlab/import_export/all_models.yml

@@ -62,6 +62,8 @@ milestone:
 - participants
 - events
 - boards
+- milestone_release
+- release
 snippets:
 - author
 - project
@@ -72,6 +74,8 @@ releases:
 - author
 - project
 - links
+- milestone_release
+- milestone
 links:
 - release
 project_members:
@@ -484,3 +488,6 @@ lists:
 - board
 - label
 - list_user_preferences
+milestone_releases:
+- milestone
+- release

spec/models/milestone_release_spec.rb

+# 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

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) }

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

spec/services/milestones/destroy_service_spec.rb

@@ -65,5 +65,19 @@ describe Milestones::DestroyService do
         expect { service.execute(group_milestone) }.not_to change { Event.count }
       end
     end
+
+    context 'when a release is tied to a milestone' do
+      it 'destroys the milestone but not the associated release' do
+        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
 end

spec/services/releases/create_service_spec.rb

@@ -72,6 +72,15 @@ describe Releases::CreateService do
         expect(project.releases.find_by(tag: tag_name).description).to eq(description)
       end
     end
+
+    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' }))
+        result = service.execute
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq('Milestone does not exist')
+      end
+    end
   end
 
   describe '#find_or_build_release' do
@@ -80,5 +89,58 @@ describe Releases::CreateService do
 
       expect(project.releases.count).to eq(0)
     end
+
+    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 }) }
+
+      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)
+        expect(result[:status]).to eq(:success)
+
+        release = project.releases.last
+
+        expect(release.milestone).to eq(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)
+          service.execute
+          release = project.releases.last
+
+          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
+
+    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
+        service.execute
+        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
+      it 'creates a release without a milestone tied to it' do
+        service = described_class.new(project, user, params.merge!({ milestone: '' }))
+        service.execute
+        release = project.releases.last
+        expect(release.milestone).to be_nil
+      end
+    end
   end
 end

spec/services/releases/destroy_service_spec.rb

@@ -57,5 +57,15 @@ describe Releases::DestroyService do
                                http_status: 403)
       end
     end
+
+    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) }
+
+      it 'destroys the release but leave the milestone intact' do
+        expect { subject }.not_to change { Milestone.count }
+        expect(milestone.reload).to be_persisted
+      end
+    end
   end
 end

spec/services/releases/update_service_spec.rb

@@ -48,5 +48,42 @@ describe Releases::UpdateService do
 
       it_behaves_like 'a failed update'
     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(:new_milestone) { create(:milestone, project: project, title: new_title) }
+      let(:params_with_milestone) { params.merge!({ milestone: new_title }) }
+
+      before do
+        release.milestone = milestone
+        release.save!
+
+        described_class.new(new_milestone.project, user, params_with_milestone).execute
+        release.reload
+      end
+
+      it 'updates the related milestone accordingly' do
+        expect(release.milestone.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: '' }) }
+
+      before do
+        release.milestone = milestone
+        release.save!
+
+        described_class.new(milestone.project, user, params_with_empty_milestone).execute
+        release.reload
+      end
+
+      it 'removes the old milestone and does not associate any new milestone' do
+        expect(release.milestone).to be_nil
+      end
+    end
   end
 end