Merge branch '217354-service-response-destroy' into 'master'

Use ServiceResponse in wiki destroy service

Closes #217354

See merge request gitlab-org/gitlab!39140

app/controllers/concerns/wiki_actions.rb

@@ -103,7 +103,7 @@ module WikiActions
     else
       render 'shared/wikis/edit'
     end
-  rescue WikiPage::PageChangedError, WikiPage::PageRenameError, Gitlab::Git::Wiki::OperationError => e
+  rescue WikiPage::PageChangedError, WikiPage::PageRenameError => e
     @error = e
     render 'shared/wikis/edit'
   end
@@ -120,13 +120,8 @@ module WikiActions
         notice: _('Wiki was successfully updated.')
       )
     else
-      flash[:alert] = response.message
       render 'shared/wikis/edit'
     end
-  rescue Gitlab::Git::Wiki::OperationError => e
-    @page = build_page(wiki_params)
-    @error = e
-    render 'shared/wikis/edit'
   end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables
 
@@ -162,14 +157,18 @@ module WikiActions
 
   # rubocop:disable Gitlab/ModuleWithInstanceVariables
   def destroy
-    WikiPages::DestroyService.new(container: container, current_user: current_user).execute(page)
+    return render_404 unless page
 
-    redirect_to wiki_path(wiki),
-                status: :found,
-                notice: _("Page was successfully deleted")
-  rescue Gitlab::Git::Wiki::OperationError => e
-    @error = e
-    render 'shared/wikis/edit'
+    response = WikiPages::DestroyService.new(container: container, current_user: current_user).execute(page)
+
+    if response.success?
+      redirect_to wiki_path(wiki),
+      status: :found,
+      notice: _("Page was successfully deleted")
+    else
+      @error = response
+      render 'shared/wikis/edit'
+    end
   end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables
 

app/services/wiki_pages/destroy_service.rb

@@ -3,11 +3,14 @@
 module WikiPages
   class DestroyService < WikiPages::BaseService
     def execute(page)
-      if page&.delete
+      if page.delete
         execute_hooks(page)
+        ServiceResponse.success(payload: { page: page })
+      else
+        ServiceResponse.error(
+          message: _('Could not delete wiki page'), payload: { page: page }
+        )
       end
-
-      page
     end
 
     def usage_counter_action

lib/api/wikis.rb

@@ -101,11 +101,15 @@ module API
         delete ':id/wikis/:slug' do
           authorize! :admin_wiki, container
 
-          WikiPages::DestroyService
+          response = WikiPages::DestroyService
             .new(container: container, current_user: current_user)
             .execute(wiki_page)
 
-          no_content!
+          if response.success?
+            no_content!
+          else
+            render_api_error!(reponse.message)
+          end
         end
 
         desc 'Upload an attachment to the wiki repository' do

lib/gitlab/git/wiki.rb

@@ -6,7 +6,6 @@ module Gitlab
       include Gitlab::Git::WrapsGitalyErrors
 
       DuplicatePageError = Class.new(StandardError)
-      OperationError = Class.new(StandardError)
 
       DEFAULT_PAGINATION = Kaminari.config.default_per_page
 

locale/gitlab.pot

@@ -6982,6 +6982,9 @@ msgstr ""
 msgid "Could not delete chat nickname %{chat_name}."
 msgstr ""
 
+msgid "Could not delete wiki page"
+msgstr ""
+
 msgid "Could not find design."
 msgstr ""
 

spec/support/shared_examples/controllers/wiki_actions_shared_examples.rb

@@ -388,7 +388,54 @@ RSpec.shared_examples 'wiki controller actions' do
         end.not_to change { wiki.list_pages.size }
 
         expect(response).to render_template('shared/wikis/edit')
-        expect(flash[:alert]).to eq('Could not create wiki page')
+      end
+    end
+  end
+
+  describe 'DELETE #destroy' do
+    let(:id_param) { wiki_title }
+
+    subject do
+      delete(:destroy,
+            params: routing_params.merge(
+              id: id_param
+            ))
+    end
+
+    context 'when page exists' do
+      it 'deletes the page' do
+        expect do
+          subject
+        end.to change { wiki.list_pages.size }.by(-1)
+      end
+
+      context 'but page cannot be deleted' do
+        before do
+          allow_next_instance_of(WikiPage) do |page|
+            allow(page).to receive(:delete).and_return(false)
+          end
+        end
+
+        it 'renders the edit state' do
+          expect do
+            subject
+          end.not_to change { wiki.list_pages.size }
+
+          expect(response).to render_template('shared/wikis/edit')
+          expect(assigns(:error).message).to eq('Could not delete wiki page')
+        end
+      end
+    end
+
+    context 'when page does not exist' do
+      let(:id_param) { 'nil' }
+
+      it 'renders 404' do
+        expect do
+          subject
+        end.not_to change { wiki.list_pages.size }
+
+        expect(response).to have_gitlab_http_status(:not_found)
       end
     end
   end

spec/support/shared_examples/services/wiki_pages/destroy_service_shared_examples.rb

@@ -32,9 +32,19 @@ RSpec.shared_examples 'WikiPages::DestroyService#execute' do |container_type|
     )
   end
 
-  it 'does not increment the delete count if the deletion failed' do
-    counter = Gitlab::UsageDataCounters::WikiPageCounter
-
-    expect { service.execute(nil) }.not_to change { counter.read(:delete) }
+  context 'when the deletion fails' do
+    before do
+      expect(page).to receive(:delete).and_return(false)
+    end
+
+    it 'returns an error response' do
+      response = service.execute(page)
+      expect(response).to be_error
+    end
+
+    it 'does not increment the delete count if the deletion failed' do
+      counter = Gitlab::UsageDataCounters::WikiPageCounter
+      expect { service.execute(page) }.not_to change { counter.read(:delete) }
+    end
   end
 end