diff --git a/app/controllers/projects/tags_controller.rb b/app/controllers/projects/tags_controller.rb index 35d798c0fda..555e066b810 100644 --- a/app/controllers/projects/tags_controller.rb +++ b/app/controllers/projects/tags_controller.rb @@ -48,8 +48,8 @@ class Projects::TagsController < Projects::ApplicationController if result[:status] == :success # Release creation with Tags was deprecated in GitLab 11.7 if params[:release_description].present? - CreateReleaseService.new(@project, current_user) - .execute(params[:tag_name], params[:release_description]) + release_params = { tag: params[:tag_name], description: params[:release_description] } + CreateReleaseService.new(@project, current_user, release_params).execute end @tag = result[:tag] diff --git a/app/models/release.rb b/app/models/release.rb index 27c9e35de56..7377af84e0b 100644 --- a/app/models/release.rb +++ b/app/models/release.rb @@ -15,6 +15,10 @@ class Release < ActiveRecord::Base delegate :repository, to: :project + def self.by_tag(project, tag) + self.find_by(project: project, tag: tag) + end + def commit git_tag = repository.find_tag(tag) repository.commit(git_tag.dereferenced_target) diff --git a/app/services/create_release_service.rb b/app/services/create_release_service.rb index 223670a935d..facf2a729ad 100644 --- a/app/services/create_release_service.rb +++ b/app/services/create_release_service.rb @@ -1,50 +1,40 @@ # frozen_string_literal: true class CreateReleaseService < BaseService - def execute(tag_name, release_description, name: nil, ref: nil) - repository = project.repository - tag = repository.find_tag(tag_name) + def execute(ref = nil) + return error('Unauthorized', 401) unless Ability.allowed?(current_user, :create_release, project) - if tag.blank? && ref.present? - result = create_tag(tag_name, ref) - return result unless result[:status] == :success + tag_result = find_or_create_tag(ref) + return tag_result if tag_result[:status] != :success - tag = result[:tag] - end - - if tag.present? - create_release(tag, name, release_description) - else - error('Tag does not exist', 404) - end - end - - def success(release) - super().merge(release: release) + create_release(tag_result[:tag]) end private - def create_release(tag, name, description) - release = project.releases.find_by(tag: tag.name) # rubocop: disable CodeReuse/ActiveRecord + def find_or_create_tag(ref) + tag = repository.find_tag(params[:tag]) + return success(tag: tag) if tag + return error('Tag does not exist', 404) if ref.blank? + + Tags::CreateService.new(project, current_user).execute(params[:tag], ref, nil) + end + + def create_release(tag) + release = Release.by_tag(project, tag.name) if release error('Release already exists', 409) else - release = project.releases.create!( - tag: tag.name, - name: name || tag.name, - sha: tag.dereferenced_target.sha, + create_params = { author: current_user, - description: description - ) + name: tag.name, + sha: tag.dereferenced_target.sha + }.merge(params) - success(release) + release = project.releases.create!(create_params) + + success(tag: tag, release: release) end end - - def create_tag(tag_name, ref) - Tags::CreateService.new(project, current_user) - .execute(tag_name, ref, nil) - end end diff --git a/app/services/update_release_service.rb b/app/services/update_release_service.rb index b51e72c7bfc..f1d5d023100 100644 --- a/app/services/update_release_service.rb +++ b/app/services/update_release_service.rb @@ -1,38 +1,18 @@ # frozen_string_literal: true class UpdateReleaseService < BaseService - attr_accessor :tag_name - - def initialize(project, user, tag_name, params) - super(project, user, params) - - @tag_name = tag_name - end - - # rubocop: disable CodeReuse/ActiveRecord def execute - repository = project.repository - existing_tag = repository.find_tag(@tag_name) + return error('Unauthorized', 401) unless Ability.allowed?(current_user, :update_release, project) - if existing_tag - release = project.releases.find_by(tag: @tag_name) + tag_name = params[:tag] + release = Release.by_tag(project, tag_name) - if release - if release.update(params) - success(release) - else - error(release.errors.messages || '400 Bad request', 400) - end - else - error('Release does not exist', 404) - end + return error('Release does not exist', 404) if release.blank? + + if release.update(params) + success(release: release) else - error('Tag does not exist', 404) + error(release.errors.messages || '400 Bad request', 400) end end - # rubocop: enable CodeReuse/ActiveRecord - - def success(release) - super().merge(release: release) - end end diff --git a/lib/api/releases.rb b/lib/api/releases.rb index 1e6867ee154..2d4a6a28998 100644 --- a/lib/api/releases.rb +++ b/lib/api/releases.rb @@ -46,15 +46,19 @@ module API end params do requires :name, type: String, desc: 'The name of the release' - requires :tag_name, type: String, desc: 'The name of the tag' + requires :tag_name, type: String, desc: 'The name of the tag', as: :tag requires :description, type: String, desc: 'The release notes' optional :ref, type: String, desc: 'The commit sha or branch name' end post ':id/releases' do authorize_create_release! - result = ::CreateReleaseService.new(user_project, current_user) - .execute(params[:tag_name], params[:description], params[:name], params[:ref]) + attributes = declared(params) + ref = attributes.delete(:ref) + attributes.delete(:id) + + result = ::CreateReleaseService.new(user_project, current_user, attributes) + .execute(ref) if result[:status] == :success present result[:release], with: Entities::Release @@ -68,7 +72,7 @@ module API success Entities::Release end params do - requires :tag_name, type: String, desc: 'The name of the tag' + requires :tag_name, type: String, desc: 'The name of the tag', as: :tag requires :name, type: String, desc: 'The name of the release' requires :description, type: String, desc: 'Release notes with markdown support' end @@ -76,8 +80,8 @@ module API authorize_update_release! attributes = declared(params) - tag = attributes.delete(:tag_name) - result = UpdateReleaseService.new(user_project, current_user, tag, attributes).execute + attributes.delete(:id) + result = UpdateReleaseService.new(user_project, current_user, attributes).execute if result[:status] == :success present result[:release], with: Entities::Release diff --git a/lib/api/tags.rb b/lib/api/tags.rb index 50ad184ba99..e1e9a7fbae5 100644 --- a/lib/api/tags.rb +++ b/lib/api/tags.rb @@ -60,8 +60,10 @@ module API if result[:status] == :success # Release creation with Tags API was deprecated in GitLab 11.7 if params[:release_description].present? - CreateReleaseService.new(user_project, current_user) - .execute(params[:tag_name], params[:release_description]) + CreateReleaseService.new( + user_project, current_user, + tag: params[:tag_name], description: params[:release_description] + ).execute end present result[:tag], @@ -99,14 +101,16 @@ module API success Entities::TagRelease end params do - requires :tag_name, type: String, desc: 'The name of the tag' + requires :tag_name, type: String, desc: 'The name of the tag', as: :tag requires :description, type: String, desc: 'Release notes with markdown support' end post ':id/repository/tags/:tag_name/release', requirements: TAG_ENDPOINT_REQUIREMENTS do authorize_create_release! - result = CreateReleaseService.new(user_project, current_user) - .execute(params[:tag_name], params[:description]) + attributes = declared(params) + attributes.delete(:id) + result = CreateReleaseService.new(user_project, current_user, attributes) + .execute if result[:status] == :success present result[:release], with: Entities::TagRelease @@ -129,7 +133,7 @@ module API result = UpdateReleaseService.new( user_project, current_user, - params[:tag_name], + tag: params[:tag_name], description: params[:description] ).execute diff --git a/spec/models/release_spec.rb b/spec/models/release_spec.rb index 92ba2d82f58..914fa4e2553 100644 --- a/spec/models/release_spec.rb +++ b/spec/models/release_spec.rb @@ -16,4 +16,18 @@ RSpec.describe Release do it { is_expected.to validate_presence_of(:project) } it { is_expected.to validate_presence_of(:description) } end + + describe '.by_tag' do + let(:tag) { release.tag } + + subject { described_class.by_tag(project, tag) } + + it { is_expected.to eq(release) } + + context 'when no releases exists' do + let(:tag) { 'not-existing' } + + it { is_expected.to be_nil } + end + end end diff --git a/spec/services/create_release_service_spec.rb b/spec/services/create_release_service_spec.rb index d7ec6d4815a..1dd74a1bcdb 100644 --- a/spec/services/create_release_service_spec.rb +++ b/spec/services/create_release_service_spec.rb @@ -6,12 +6,17 @@ describe CreateReleaseService do let(:tag_name) { project.repository.tag_names.first } let(:name) { 'Bionic Beaver'} let(:description) { 'Awesome release!' } - let(:service) { described_class.new(project, user) } + let(:params) { { tag: tag_name, name: name, description: description } } + let(:service) { described_class.new(project, user, params) } let(:ref) { nil } + before do + project.add_maintainer(user) + end + shared_examples 'a successful release creation' do it 'creates a new release' do - result = service.execute(tag_name, description, name: name, ref: ref) + result = service.execute(ref) expect(result[:status]).to eq(:success) release = project.releases.find_by(tag: tag_name) expect(release).not_to be_nil @@ -24,14 +29,16 @@ describe CreateReleaseService do it_behaves_like 'a successful release creation' it 'raises an error if the tag does not exist' do - result = service.execute("foobar", description) + service.params[:tag] = 'foobar' + + result = service.execute expect(result[:status]).to eq(:error) end it 'keeps track of the commit sha' do tag = project.repository.find_tag(tag_name) sha = tag.dereferenced_target.sha - result = service.execute(tag_name, description, name: name) + result = service.execute expect(result[:status]).to eq(:success) expect(project.releases.find_by(tag: tag_name).sha).to eq(sha) @@ -46,7 +53,7 @@ describe CreateReleaseService do it 'creates a tag if the tag does not exist' do expect(project.repository.ref_exists?("refs/tags/#{tag_name}")).to be_falsey - result = service.execute(tag_name, description, name: name, ref: ref) + result = service.execute(ref) expect(result[:status]).to eq(:success) expect(project.repository.ref_exists?("refs/tags/#{tag_name}")).to be_truthy @@ -57,11 +64,13 @@ describe CreateReleaseService do context 'there already exists a release on a tag' do before do - service.execute(tag_name, description) + service.execute end it 'raises an error and does not update the release' do - result = service.execute(tag_name, 'The best release!') + service.params[:description] = 'The best release!' + + result = service.execute expect(result[:status]).to eq(:error) expect(project.releases.find_by(tag: tag_name).description).to eq(description) end diff --git a/spec/services/update_release_service_spec.rb b/spec/services/update_release_service_spec.rb index a24dcabfc2e..4378d5d072a 100644 --- a/spec/services/update_release_service_spec.rb +++ b/spec/services/update_release_service_spec.rb @@ -7,12 +7,12 @@ describe UpdateReleaseService do let(:description) { 'Awesome release!' } let(:new_name) { 'A new name' } let(:new_description) { 'The best release!' } - let(:params) { { name: new_name, description: new_description } } - let(:service) { described_class.new(project, user, tag_name, params) } - let(:create_service) { CreateReleaseService.new(project, user) } + let(:params) { { name: new_name, description: new_description, tag: tag_name } } + let(:service) { described_class.new(project, user, params) } + let(:create_service) { CreateReleaseService.new(project, user, tag: tag_name, description: description) } before do - create_service.execute(tag_name, description) + create_service.execute end shared_examples 'a failed update' do