Refactor Release services

CreateReleaseService and UpdateReleaseService now takes all the release
attributes as constructor parameters. This will simplify attribute
expansion

9 files changed, 90 insertions, 85 deletions

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)
-    end
-  end
+      release = project.releases.create!(create_params)
 
-  def create_tag(tag_name, ref)
-    Tags::CreateService.new(project, current_user)
-      .execute(tag_name, ref, nil)
+      success(tag: tag, release: release)
+    end
   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