Merge branch 'issue-58418-release-notes' into 'master'

Set Release Name When Adding Release Notes to an Existing Tag

Closes #58418

See merge request gitlab-org/gitlab-ce!26807

10 files changed, 106 insertions, 21 deletions

diff --git a/app/controllers/projects/tags/releases_controller.rb b/app/controllers/projects/tags/releases_controller.rb
index 334e1847cc8..5e4c601a693 100644
--- a/app/controllers/projects/tags/releases_controller.rb
+++ b/app/controllers/projects/tags/releases_controller.rb
@@ -12,16 +12,13 @@ class Projects::Tags::ReleasesController < Projects::ApplicationController
   end
 
   def update
-    # Release belongs to Tag which is not active record object,
-    # it exists only to save a description to each Tag.
-    # If description is empty we should destroy the existing record.
     if release_params[:description].present?
       release.update(release_params)
     else
       release.destroy
     end
 
-    redirect_to project_tag_path(@project, @tag.name)
+    redirect_to project_tag_path(@project, tag.name)
   end
 
   private
@@ -30,11 +27,10 @@ class Projects::Tags::ReleasesController < Projects::ApplicationController
     @tag ||= @repository.find_tag(params[:tag_id])
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def release
-    @release ||= @project.releases.find_or_initialize_by(tag: @tag.name)
+    @release ||= Releases::CreateService.new(project, current_user, tag: @tag.name)
+                                        .find_or_build_release
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   def release_params
     params.require(:release).permit(:description)
diff --git a/app/models/release.rb b/app/models/release.rb
index 746fc31a038..0f9e94373c7 100644
--- a/app/models/release.rb
+++ b/app/models/release.rb
@@ -15,6 +15,7 @@ class Release < ApplicationRecord
   accepts_nested_attributes_for :links, allow_destroy: true
 
   validates :description, :project, :tag, presence: true
+  validates :name, presence: true, on: :create
 
   scope :sorted, -> { order(created_at: :desc) }
 
diff --git a/app/services/releases/concerns.rb b/app/services/releases/concerns.rb
index a04bb8f9e14..ff6b696ca96 100644
--- a/app/services/releases/concerns.rb
+++ b/app/services/releases/concerns.rb
@@ -15,7 +15,7 @@ module Releases
       end
 
       def name
-        params[:name]
+        params[:name] || tag_name
       end
 
       def description
diff --git a/app/services/releases/create_service.rb b/app/services/releases/create_service.rb
index c6e143d440d..a271a7e5e49 100644
--- a/app/services/releases/create_service.rb
+++ b/app/services/releases/create_service.rb
@@ -15,6 +15,10 @@ module Releases
       create_release(tag)
     end
 
+    def find_or_build_release
+      release || build_release(existing_tag)
+    end
+
     private
 
     def ensure_tag
@@ -38,7 +42,17 @@ module Releases
     end
 
     def create_release(tag)
-      release = project.releases.create!(
+      release = build_release(tag)
+
+      release.save!
+
+      success(tag: tag, release: release)
+    rescue => e
+      error(e.message, 400)
+    end
+
+    def build_release(tag)
+      project.releases.build(
         name: name,
         description: description,
         author: current_user,
@@ -46,10 +60,6 @@ module Releases
         sha: tag.dereferenced_target.sha,
         links_attributes: params.dig(:assets, 'links') || []
       )
-
-      success(tag: tag, release: release)
-    rescue => e
-      error(e.message, 400)
     end
   end
 end
diff --git a/changelogs/unreleased/issue-58418-release-notes.yml b/changelogs/unreleased/issue-58418-release-notes.yml
new file mode 100644
index 00000000000..80e6529eb12
--- /dev/null
+++ b/changelogs/unreleased/issue-58418-release-notes.yml
@@ -0,0 +1,5 @@
+---
+title: Set release name when adding release notes to an existing tag
+merge_request: 26807
+author:
+type: fixed
diff --git a/lib/gitlab/legacy_github_import/release_formatter.rb b/lib/gitlab/legacy_github_import/release_formatter.rb
index 8c0c17780ca..746786b5a66 100644
--- a/lib/gitlab/legacy_github_import/release_formatter.rb
+++ b/lib/gitlab/legacy_github_import/release_formatter.rb
@@ -7,6 +7,7 @@ module Gitlab
         {
           project: project,
           tag: raw_data.tag_name,
+          name: raw_data.name,
           description: raw_data.body,
           created_at: raw_data.created_at,
           updated_at: raw_data.created_at
diff --git a/spec/controllers/projects/tags/releases_controller_spec.rb b/spec/controllers/projects/tags/releases_controller_spec.rb
index 29f206c574b..66eff4844c2 100644
--- a/spec/controllers/projects/tags/releases_controller_spec.rb
+++ b/spec/controllers/projects/tags/releases_controller_spec.rb
@@ -18,40 +18,85 @@ describe Projects::Tags::ReleasesController do
       tag_id = release.tag
       project.releases.destroy_all # rubocop: disable DestroyAll
 
-      get :edit, params: { namespace_id: project.namespace, project_id: project, tag_id: tag_id }
+      response = get :edit, params: { namespace_id: project.namespace, project_id: project, tag_id: tag_id }
 
       release = assigns(:release)
       expect(release).not_to be_nil
       expect(release).not_to be_persisted
+      expect(response).to have_http_status(:ok)
     end
 
     it 'retrieves an existing release' do
-      get :edit, params: { namespace_id: project.namespace, project_id: project, tag_id: release.tag }
+      response = get :edit, params: { namespace_id: project.namespace, project_id: project, tag_id: release.tag }
 
       release = assigns(:release)
       expect(release).not_to be_nil
       expect(release).to be_persisted
+      expect(response).to have_http_status(:ok)
     end
   end
 
   describe 'PUT #update' do
     it 'updates release note description' do
-      update_release('description updated')
+      response = update_release(release.tag, "description updated")
 
-      release = project.releases.find_by_tag(tag)
+      release = project.releases.find_by(tag: tag)
       expect(release.description).to eq("description updated")
+      expect(response).to have_http_status(:found)
     end
 
-    it 'deletes release note when description is null' do
-      expect { update_release('') }.to change(project.releases, :count).by(-1)
+    it 'creates a release if one does not exist' do
+      tag_without_release = create_new_tag
+
+      expect do
+        update_release(tag_without_release.name, "a new release")
+      end.to change { project.releases.count }.by(1)
+
+      expect(response).to have_http_status(:found)
+    end
+
+    it 'sets the release name, sha, and author for a new release' do
+      tag_without_release = create_new_tag
+
+      response = update_release(tag_without_release.name, "a new release")
+
+      release = project.releases.find_by(tag: tag_without_release.name)
+      expect(release.name).to eq(tag_without_release.name)
+      expect(release.sha).to eq(tag_without_release.target_commit.sha)
+      expect(release.author.id).to eq(user.id)
+      expect(response).to have_http_status(:found)
+    end
+
+    it 'deletes release when description is empty' do
+      initial_releases_count = project.releases.count
+
+      response = update_release(release.tag, "")
+
+      expect(initial_releases_count).to eq(1)
+      expect(project.releases.count).to eq(0)
+      expect(response).to have_http_status(:found)
+    end
+
+    it 'does nothing when description is empty and the tag does not have a release' do
+      tag_without_release = create_new_tag
+
+      expect do
+        update_release(tag_without_release.name, "")
+      end.not_to change { project.releases.count }
+
+      expect(response).to have_http_status(:found)
     end
   end
 
-  def update_release(description)
+  def create_new_tag
+    project.repository.add_tag(user, 'mytag', 'master')
+  end
+
+  def update_release(tag_id, description)
     put :update, params: {
       namespace_id: project.namespace.to_param,
       project_id: project,
-      tag_id: release.tag,
+      tag_id: tag_id,
       release: { description: description }
     }
   end
diff --git a/spec/lib/gitlab/legacy_github_import/release_formatter_spec.rb b/spec/lib/gitlab/legacy_github_import/release_formatter_spec.rb
index 082e3b36dd0..c57b96fb00d 100644
--- a/spec/lib/gitlab/legacy_github_import/release_formatter_spec.rb
+++ b/spec/lib/gitlab/legacy_github_import/release_formatter_spec.rb
@@ -25,6 +25,7 @@ describe Gitlab::LegacyGithubImport::ReleaseFormatter do
       expected = {
         project: project,
         tag: 'v1.0.0',
+        name: 'First release',
         description: 'Release v1.0.0',
         created_at: created_at,
         updated_at: created_at
diff --git a/spec/models/release_spec.rb b/spec/models/release_spec.rb
index b4b32c95dee..0b19a4f8efc 100644
--- a/spec/models/release_spec.rb
+++ b/spec/models/release_spec.rb
@@ -18,6 +18,22 @@ RSpec.describe Release do
   describe 'validation' do
     it { is_expected.to validate_presence_of(:project) }
     it { is_expected.to validate_presence_of(:description) }
+    it { is_expected.to validate_presence_of(:name) }
+
+    context 'when a release exists in the database without a name' do
+      it 'does not require name' do
+        existing_release_without_name = build(:release, project: project, author: user, name: nil)
+        existing_release_without_name.save(validate: false)
+
+        existing_release_without_name.description = "change"
+        existing_release_without_name.save
+        existing_release_without_name.reload
+
+        expect(existing_release_without_name).to be_valid
+        expect(existing_release_without_name.description).to eq("change")
+        expect(existing_release_without_name.name).to be_nil
+      end
+    end
   end
 
   describe '#assets_count' do
diff --git a/spec/services/releases/create_service_spec.rb b/spec/services/releases/create_service_spec.rb
index 612e9f152e7..0efe37f1167 100644
--- a/spec/services/releases/create_service_spec.rb
+++ b/spec/services/releases/create_service_spec.rb
@@ -19,6 +19,8 @@ describe Releases::CreateService do
     shared_examples 'a successful release creation' do
       it 'creates a new release' do
         result = service.execute
+
+        expect(project.releases.count).to eq(1)
         expect(result[:status]).to eq(:success)
         expect(result[:tag]).not_to be_nil
         expect(result[:release]).not_to be_nil
@@ -69,4 +71,12 @@ describe Releases::CreateService do
       end
     end
   end
+
+  describe '#find_or_build_release' do
+    it 'does not save the built release' do
+      service.find_or_build_release
+
+      expect(project.releases.count).to eq(0)
+    end
+  end
 end