Merge branch '26016-edit-release-be' into 'master'

Add `edit` endpoint for Release page

See merge request gitlab-org/gitlab!17858

app/controllers/projects/releases_controller.rb

@@ -3,10 +3,12 @@
 class Projects::ReleasesController < Projects::ApplicationController
   # Authorize
   before_action :require_non_empty_project, except: [:index]
+  before_action :release, only: %i[edit update]
   before_action :authorize_read_release!
   before_action do
     push_frontend_feature_flag(:release_edit_page, project)
   end
+  before_action :authorize_update_release!, only: %i[edit update]
 
   def index
     respond_to do |format|
@@ -22,4 +24,25 @@ class Projects::ReleasesController < Projects::ApplicationController
   def releases
     ReleasesFinder.new(@project, current_user).execute
   end
+
+  def edit
+    respond_to do |format|
+      format.html { render 'edit' }
+    end
+  end
+
+  private
+
+  def authorize_update_release!
+    access_denied! unless Feature.enabled?(:release_edit_page, project)
+    access_denied! unless can?(current_user, :update_release, release)
+  end
+
+  def release
+    @release ||= project.releases.find_by_tag!(sanitized_tag_name)
+  end
+
+  def sanitized_tag_name
+    CGI.unescape(params[:tag])
+  end
 end

app/finders/releases_finder.rb

@@ -6,9 +6,11 @@ class ReleasesFinder
     @current_user = current_user
   end
 
-  def execute
+  def execute(preload: true)
     return Release.none unless Ability.allowed?(@current_user, :read_release, @project)
 
-    @project.releases.sorted
+    releases = @project.releases
+    releases = releases.preloaded if preload
+    releases.sorted
   end
 end

app/models/release.rb

@@ -27,13 +27,17 @@ class Release < ApplicationRecord
   validates_associated :milestone_releases, message: -> (_, obj) { obj[:value].map(&:errors).map(&:full_messages).join(",") }
 
   scope :sorted, -> { order(released_at: :desc) }
-  scope :with_project_and_namespace, -> { includes(project: :namespace) }
+  scope :preloaded, -> { includes(project: :namespace) }
 
   delegate :repository, to: :project
 
   after_commit :create_evidence!, on: :create
   after_commit :notify_new_release, on: :create
 
+  def to_param
+    CGI.escape(tag)
+  end
+
   def commit
     strong_memoize(:commit) do
       repository.commit(actual_sha)

app/presenters/release_presenter.rb

@@ -31,6 +31,12 @@ class ReleasePresenter < Gitlab::View::Presenter::Delegated
     project_issues_url(project, params_for_issues_and_mrs)
   end
 
+  def edit_url
+    return unless release_edit_page_available?
+
+    edit_project_release_url(project, release)
+  end
+
   private
 
   def can_download_code?
@@ -44,4 +50,8 @@ class ReleasePresenter < Gitlab::View::Presenter::Delegated
   def release_mr_issue_urls_available?
     ::Feature.enabled?(:release_mr_issue_urls, project)
   end
+
+  def release_edit_page_available?
+    ::Feature.enabled?(:release_edit_page, project)
+  end
 end

app/workers/new_release_worker.rb

@@ -7,7 +7,7 @@ class NewReleaseWorker
   feature_category :release_orchestration
 
   def perform(release_id)
-    release = Release.with_project_and_namespace.find_by_id(release_id)
+    release = Release.preloaded.find_by_id(release_id)
     return unless release
 
     NotificationService.new.send_new_release_notifications(release)

config/routes/project.rb

@@ -179,7 +179,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
           end
         end
 
-        resources :releases, only: [:index]
+        resources :releases, only: [:index, :edit], param: :tag, constraints: { tag: %r{[^/]+} }
         resources :starrers, only: [:index]
         resources :forks, only: [:index, :new, :create]
         resources :group_links, only: [:index, :create, :update, :destroy], constraints: { id: /\d+/ }

lib/api/entities.rb

@@ -1324,6 +1324,7 @@ module API
       expose :_links do
         expose :merge_requests_url, expose_nil: false
         expose :issues_url, expose_nil: false
+        expose :edit_url, expose_nil: false
       end
 
       private

spec/controllers/projects/releases_controller_spec.rb

@@ -5,10 +5,33 @@ require 'spec_helper'
 describe Projects::ReleasesController do
   let!(:project)         { create(:project, :repository, :public) }
   let!(:private_project) { create(:project, :repository, :private) }
-  let!(:user)            { create(:user) }
+  let(:user)             { developer }
+  let(:developer)        { create(:user) }
+  let(:reporter)         { create(:user) }
   let!(:release_1)       { create(:release, project: project, released_at: Time.zone.parse('2018-10-18')) }
   let!(:release_2)       { create(:release, project: project, released_at: Time.zone.parse('2019-10-19')) }
 
+  before do
+    project.add_developer(developer)
+    project.add_reporter(reporter)
+  end
+
+  shared_examples_for 'successful request' do
+    it 'renders a 200' do
+      subject
+
+      expect(response).to have_gitlab_http_status(:success)
+    end
+  end
+
+  shared_examples_for 'not found' do
+    it 'renders 404' do
+      subject
+
+      expect(response).to have_gitlab_http_status(:not_found)
+    end
+  end
+
   shared_examples 'common access controls' do
     it 'renders a 200' do
       get_index
@@ -23,18 +46,28 @@ describe Projects::ReleasesController do
         sign_in(user)
       end
 
-      it 'renders a 200 for a logged in developer' do
-        project.add_developer(user)
+      context 'when user is a developer' do
+        let(:user) { developer }
+
+        it 'renders a 200 for a logged in developer' do
+          sign_in(user)
 
-        get_index
+          get_index
 
-        expect(response.status).to eq(200)
+          expect(response.status).to eq(200)
+        end
       end
 
-      it 'renders a 404 when logged in but not in the project' do
-        get_index
+      context 'when user is an external user' do
+        let(:user) { create(:user) }
+
+        it 'renders a 404 when logged in but not in the project' do
+          sign_in(user)
+
+          get_index
 
-        expect(response.status).to eq(404)
+          expect(response.status).to eq(404)
+        end
       end
     end
   end
@@ -85,6 +118,54 @@ describe Projects::ReleasesController do
     end
   end
 
+  describe 'GET #edit' do
+    subject do
+      get :edit, params: { namespace_id: project.namespace, project_id: project, tag: tag }
+    end
+
+    before do
+      sign_in(user)
+    end
+
+    let!(:release) { create(:release, project: project) }
+    let(:tag) { CGI.escape(release.tag) }
+
+    it_behaves_like 'successful request'
+
+    context 'when tag name contains slash' do
+      let!(:release) { create(:release, project: project, tag: 'awesome/v1.0') }
+      let(:tag) { CGI.escape(release.tag) }
+
+      it_behaves_like 'successful request'
+
+      it 'is accesible at a URL encoded path' do
+        expect(edit_project_release_path(project, release))
+          .to eq("/#{project.namespace.path}/#{project.name}/-/releases/awesome%252Fv1.0/edit")
+      end
+    end
+
+    context 'when feature flag `release_edit_page` is disabled' do
+      before do
+        stub_feature_flags(release_edit_page: false)
+      end
+
+      it_behaves_like 'not found'
+    end
+
+    context 'when release does not exist' do
+      let!(:release) { }
+      let(:tag) { 'non-existent-tag' }
+
+      it_behaves_like 'not found'
+    end
+
+    context 'when user is a reporter' do
+      let(:user) { reporter }
+
+      it_behaves_like 'not found'
+    end
+  end
+
   private
 
   def get_index

spec/finders/releases_finder_spec.rb

@@ -8,8 +8,7 @@ describe ReleasesFinder do
   let(:repository) { project.repository }
   let(:v1_0_0)     { create(:release, project: project, tag: 'v1.0.0') }
   let(:v1_1_0)     { create(:release, project: project, tag: 'v1.1.0') }
-
-  subject { described_class.new(project, user)}
+  let(:finder) { described_class.new(project, user) }
 
   before do
     v1_0_0.update_attribute(:released_at, 2.days.ago)
@@ -17,11 +16,13 @@ describe ReleasesFinder do
   end
 
   describe '#execute' do
+    subject { finder.execute(**args) }
+
+    let(:args) { {} }
+
     context 'when the user is not part of the project' do
       it 'returns no releases' do
-        releases = subject.execute
-
-        expect(releases).to be_empty
+        is_expected.to be_empty
       end
     end
 
@@ -31,11 +32,25 @@ describe ReleasesFinder do
       end
 
       it 'sorts by release date' do
-        releases = subject.execute
+        is_expected.to be_present
+        expect(subject.size).to eq(2)
+        expect(subject).to eq([v1_1_0, v1_0_0])
+      end
+
+      it 'preloads associations' do
+        expect(Release).to receive(:preloaded).once.and_call_original
+
+        subject
+      end
+
+      context 'when preload is false' do
+        let(:args) { { preload: false } }
+
+        it 'does not preload associations' do
+          expect(Release).not_to receive(:preloaded)
 
-        expect(releases).to be_present
-        expect(releases.size).to eq(2)
-        expect(releases).to eq([v1_1_0, v1_0_0])
+          subject
+        end
       end
     end
   end

spec/fixtures/api/schemas/public_api/v4/release.json

@@ -38,10 +38,11 @@
       "additionalProperties": false
     },
     "_links": {
-      "required": ["merge_requests_url", "issues_url"],
+      "required": ["merge_requests_url", "issues_url", "edit_url"],
       "properties": {
         "merge_requests_url": { "type": "string" },
-        "issues_url": { "type": "string" }
+        "issues_url": { "type": "string" },
+        "edit_url": { "type": "string"}
       }
     }
   },

spec/fixtures/api/schemas/public_api/v4/release/release_for_guest.json

@@ -26,10 +26,11 @@
       "additionalProperties": false
     },
     "_links": {
-      "required": ["merge_requests_url", "issues_url"],
+      "required": ["merge_requests_url", "issues_url", "edit_url"],
       "properties": {
         "merge_requests_url": { "type": "string" },
-        "issues_url": { "type": "string" }
+        "issues_url": { "type": "string" },
+        "edit_url": { "type": "string"}
       }
     }
   },

spec/presenters/release_presenter_spec.rb

@@ -82,4 +82,20 @@ describe ReleasePresenter do
       it { is_expected.to be_nil }
     end
   end
+
+  describe '#edit_url' do
+    subject { presenter.edit_url }
+
+    it 'returns release edit url' do
+      is_expected.to match /#{edit_project_release_url(project, release)}/
+    end
+
+    context 'when release_edit_page feature flag is disabled' do
+      before do
+        stub_feature_flags(release_edit_page: false)
+      end
+
+      it { is_expected.to be_nil }
+    end
+  end
 end