Commit 233c708d authored by Etienne Baqué's avatar Etienne Baqué

Fixed tests and classes based on review comments

Updated Release services spec files.
Fixed Post-migration file.
Removed obsolete comments.
Fixed coding style in spec files.
parent c919bd03
Pipeline #82416498 passed with stages
in 90 minutes and 42 seconds
......@@ -27,9 +27,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
......
......@@ -6,6 +6,9 @@ class MilestoneRelease < ApplicationRecord
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
......
......@@ -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
......
......@@ -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(', ')
Array(params[:milestones]) - existing_milestone_titles
end
def param_for_milestone_titles_provided?
params[:milestones].present? || params[:milestones]&.empty?
params.key?(:milestones)
end
end
end
......
......@@ -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
......
......@@ -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?
......
---
title: Switch Milestone and Release to a many-to-many relationship
merge_request: 32656
merge_request: 16517
author:
type: changed
# frozen_string_literal: true
class RemoveIdColumnFromIntermediateReleaseMilestones < ActiveRecord::Migration[5.2]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
......
......@@ -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
......
......@@ -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
......@@ -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) }
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
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