Commit a43ab8d6 authored by Etienne Baqué's avatar Etienne Baqué Committed by Andreas Brandl

Added relationships between Release and Milestone

Modified schema via migrations.
Added one-to-one relationship between the two models.
Added changelog file
parent de4e2dca
......@@ -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
......
# 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
......@@ -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) }
......
......@@ -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
......@@ -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
......
......@@ -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)
......
---
title: Allow milestones to be associated with a release (backend)
merge_request: 30816
author:
type: added
# 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
......@@ -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
......
This diff is collapsed.
......@@ -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, _|
......
......@@ -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!
......
# 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
......@@ -15,6 +15,7 @@
"author": {
"oneOf": [{ "type": "null" }, { "$ref": "user/basic.json" }]
},
"milestone": { "type": "string" },
"assets": {
"required": ["count", "links", "sources"],
"properties": {
......
......@@ -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
# 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
......@@ -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) }
......
......@@ -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
......
......@@ -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
......@@ -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
......@@ -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
......@@ -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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment