Merge branch '31138-improve-test-settings-for-services-in-empty-projects' into 'master'

improves test settings for chat notification services for empty projects

Closes #31138

See merge request !10886

app/models/project_services/chat_notification_service.rb

@@ -22,7 +22,7 @@ class ChatNotificationService < Service
   end
 
   def can_test?
-    super && valid?
+    valid?
   end
 
   def self.supported_events

app/models/service.rb

@@ -132,7 +132,7 @@ class Service < ActiveRecord::Base
   end
 
   def can_test?
-    !project.empty_repo?
+    true
   end
 
   # reason why service cannot be tested

changelogs/unreleased/31138-improve-test-settings-for-services-in-empty-projects.yml

+---
+title: Improves test settings for chat notification services for empty projects
+merge_request: 10886
+author:

lib/gitlab/data_builder/push.rb

@@ -41,7 +41,7 @@ module Gitlab
         type = Gitlab::Git.tag_ref?(ref) ? 'tag_push' : 'push'
 
         # Hash to be passed as post_receive_data
-        data = {
+        {
           object_kind: type,
           event_name: type,
           before: oldrev,
@@ -61,16 +61,15 @@ module Gitlab
           repository: project.hook_attrs.slice(:name, :url, :description, :homepage,
                                                :git_http_url, :git_ssh_url, :visibility_level)
         }
-
-        data
       end
 
       # This method provide a sample data generated with
       # existing project and commits to test webhooks
       def build_sample(project, user)
-        commits = project.repository.commits(project.default_branch, limit: 3)
         ref = "#{Gitlab::Git::BRANCH_REF_PREFIX}#{project.default_branch}"
-        build(project, user, commits.last.id, commits.first.id, ref, commits)
+        commits = project.repository.commits(project.default_branch.to_s, limit: 3) rescue []
+
+        build(project, user, commits.last&.id, commits.first&.id, ref, commits)
       end
 
       def checkout_sha(repository, newrev, ref)

spec/controllers/projects/services_controller_spec.rb

@@ -8,6 +8,7 @@ describe Projects::ServicesController do
   before do
     sign_in(user)
     project.team << [user, :master]
+
     controller.instance_variable_set(:@project, project)
     controller.instance_variable_set(:@service, service)
   end
@@ -18,20 +19,60 @@ describe Projects::ServicesController do
     end
 
     describe "#test" do
+      context 'when can_test? returns false' do
+        it 'renders 404' do
+          allow_any_instance_of(Service).to receive(:can_test?).and_return(false)
+
+          get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html
+
+          expect(response).to have_http_status(404)
+        end
+      end
+
       context 'success' do
+        context 'with empty project' do
+          let(:project) { create(:empty_project) }
+
+          context 'with chat notification service' do
+            let(:service) { project.create_microsoft_teams_service(webhook: 'http://webhook.com') }
+
+            it 'redirects and show success message' do
+              allow_any_instance_of(MicrosoftTeams::Notifier).to receive(:ping).and_return(true)
+
+              get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html
+
+              expect(response).to redirect_to(root_path)
+              expect(flash[:notice]).to eq('We sent a request to the provided URL')
+            end
+          end
+
+          it 'redirects and show success message' do
+            expect(service).to receive(:test).and_return(success: true, result: 'done')
+
+            get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html
+
+            expect(response).to redirect_to(root_path)
+            expect(flash[:notice]).to eq('We sent a request to the provided URL')
+          end
+        end
+
         it "redirects and show success message" do
-          expect(service).to receive(:test).and_return({ success: true, result: 'done' })
+          expect(service).to receive(:test).and_return(success: true, result: 'done')
+
           get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html
-          expect(response.status).to redirect_to('/')
+
+          expect(response).to redirect_to(root_path)
           expect(flash[:notice]).to eq('We sent a request to the provided URL')
         end
       end
 
       context 'failure' do
         it "redirects and show failure message" do
-          expect(service).to receive(:test).and_return({ success: false, result: 'Bad test' })
+          expect(service).to receive(:test).and_return(success: false, result: 'Bad test')
+
           get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html
-          expect(response.status).to redirect_to('/')
+
+          expect(response).to redirect_to(root_path)
           expect(flash[:alert]).to eq('We tried to send a request to the provided URL but an error occurred: Bad test')
         end
       end

spec/models/project_services/chat_notification_service_spec.rb

@@ -11,10 +11,10 @@ describe ChatNotificationService, models: true do
 
   describe '#can_test?' do
     context 'with empty repository' do
-      it 'returns false' do
+      it 'returns true' do
         subject.project = create(:empty_project, :empty_repo)
 
-        expect(subject.can_test?).to be false
+        expect(subject.can_test?).to be true
       end
     end
 

spec/models/service_spec.rb

@@ -11,43 +11,48 @@ describe Service, models: true do
   end
 
   describe "Test Button" do
-    before do
-      @service = Service.new
-    end
+    describe '#can_test?' do
+      let(:service) { create(:service, project: project) }
 
-    describe "Testable" do
-      let(:project) { create(:project, :repository) }
+      context 'when repository is not empty' do
+        let(:project) { create(:project, :repository) }
 
-      before do
-        allow(@service).to receive(:project).and_return(project)
-        @testable = @service.can_test?
+        it 'returns true' do
+          expect(service.can_test?).to be true
+        end
       end
 
-      describe '#can_test?' do
-        it { expect(@testable).to eq(true) }
+      context 'when repository is empty' do
+        let(:project) { create(:empty_project) }
+
+        it 'returns true' do
+          expect(service.can_test?).to be true
+        end
       end
+    end
 
-      describe '#test' do
-        let(:data) { 'test' }
+    describe '#test' do
+      let(:data) { 'test' }
+      let(:service) { create(:service, project: project) }
+
+      context 'when repository is not empty' do
+        let(:project) { create(:project, :repository) }
 
         it 'test runs execute' do
-          expect(@service).to receive(:execute).with(data)
+          expect(service).to receive(:execute).with(data)
 
-          @service.test(data)
+          service.test(data)
         end
       end
-    end
 
-    describe "With commits" do
-      let(:project) { create(:project, :repository) }
+      context 'when repository is empty' do
+        let(:project) { create(:empty_project) }
 
-      before do
-        allow(@service).to receive(:project).and_return(project)
-        @testable = @service.can_test?
-      end
+        it 'test runs execute' do
+          expect(service).to receive(:execute).with(data)
 
-      describe '#can_test?' do
-        it { expect(@testable).to eq(true) }
+          service.test(data)
+        end
       end
     end
   end