diff options
author | Etienne BaquƩ <ebaque@gitlab.com> | 2019-09-09 04:54:12 +0300 |
---|---|---|
committer | Etienne BaquƩ <ebaque@gitlab.com> | 2019-09-12 00:37:15 +0300 |
commit | 64a22784c674b638d5d4f01cd815e492ed2881be (patch) | |
tree | fd3859cad2e04fe26bee9da06fdd324d84472ace | |
parent | 1af4a04658b793216970fb4c1202d410e89b4330 (diff) |
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.
-rw-r--r-- | app/models/milestone.rb | 3 | ||||
-rw-r--r-- | app/models/release.rb | 3 | ||||
-rw-r--r-- | app/services/releases/concerns.rb | 8 | ||||
-rw-r--r-- | app/services/releases/create_service.rb | 2 | ||||
-rw-r--r-- | app/services/releases/update_service.rb | 2 | ||||
-rw-r--r-- | db/post_migrate/20190904205212_remove_id_column_from_intermediate_release_milestones.rb | 2 | ||||
-rw-r--r-- | lib/api/releases.rb | 2 | ||||
-rw-r--r-- | spec/models/milestone_release_spec.rb | 29 | ||||
-rw-r--r-- | spec/models/milestone_spec.rb | 1 | ||||
-rw-r--r-- | spec/models/release_spec.rb | 1 | ||||
-rw-r--r-- | spec/services/releases/create_service_spec.rb | 28 | ||||
-rw-r--r-- | spec/services/releases/update_service_spec.rb | 23 |
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 |