Merge branch 'fj-refactor-snippet-create-update-service' into 'master'

Refactor snippet create and update service

See merge request gitlab-org/gitlab!32514

app/services/snippets/base_service.rb

@@ -2,8 +2,32 @@
 
 module Snippets
   class BaseService < ::BaseService
+    include SpamCheckMethods
+
+    CreateRepositoryError = Class.new(StandardError)
+
+    attr_reader :uploaded_files
+
+    def initialize(project, user = nil, params = {})
+      super
+
+      @uploaded_files = Array(@params.delete(:files).presence)
+
+      filter_spam_check_params
+    end
+
     private
 
+    def visibility_allowed?(snippet, visibility_level)
+      Gitlab::VisibilityLevel.allowed_for?(current_user, visibility_level)
+    end
+
+    def error_forbidden_visibility(snippet)
+      deny_visibility_level(snippet)
+
+      snippet_error_response(snippet, 403)
+    end
+
     def snippet_error_response(snippet, http_status)
       ServiceResponse.error(
         message: snippet.errors.full_messages.to_sentence,

app/services/snippets/create_service.rb

@@ -2,25 +2,11 @@
 
 module Snippets
   class CreateService < Snippets::BaseService
-    include SpamCheckMethods
-
-    CreateRepositoryError = Class.new(StandardError)
-
     def execute
-      filter_spam_check_params
+      @snippet = build_from_params
 
-      @files = Array(params.delete(:files).presence)
-
-      @snippet = if project
-                   project.snippets.build(params)
-                 else
-                   PersonalSnippet.new(params)
-                 end
-
-      unless Gitlab::VisibilityLevel.allowed_for?(current_user, @snippet.visibility_level)
-        deny_visibility_level(@snippet)
-
-        return snippet_error_response(@snippet, 403)
+      unless visibility_allowed?(@snippet, @snippet.visibility_level)
+        return error_forbidden_visibility(@snippet)
       end
 
       @snippet.author = current_user
@@ -41,6 +27,14 @@ module Snippets
 
     private
 
+    def build_from_params
+      if project
+        project.snippets.build(params)
+      else
+        PersonalSnippet.new(params)
+      end
+    end
+
     def save_and_commit
       snippet_saved = @snippet.save
 
@@ -91,7 +85,7 @@ module Snippets
     def move_temporary_files
       return unless @snippet.is_a?(PersonalSnippet)
 
-      @files.each do |file|
+      uploaded_files.each do |file|
         FileMover.new(file, from_model: current_user, to_model: @snippet).execute
       end
     end

app/services/snippets/update_service.rb

@@ -2,26 +2,15 @@
 
 module Snippets
   class UpdateService < Snippets::BaseService
-    include SpamCheckMethods
-
     COMMITTABLE_ATTRIBUTES = %w(file_name content).freeze
 
     UpdateError = Class.new(StandardError)
-    CreateRepositoryError = Class.new(StandardError)
 
     def execute(snippet)
-      # check that user is allowed to set specified visibility_level
-      new_visibility = visibility_level
-
-      if new_visibility && new_visibility.to_i != snippet.visibility_level
-        unless Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
-          deny_visibility_level(snippet, new_visibility)
-
-          return snippet_error_response(snippet, 403)
-        end
+      if visibility_changed?(snippet) && !visibility_allowed?(snippet, visibility_level)
+        return error_forbidden_visibility(snippet)
       end
 
-      filter_spam_check_params
       snippet.assign_attributes(params)
       spam_check(snippet, current_user)
 
@@ -36,6 +25,10 @@ module Snippets
 
     private
 
+    def visibility_changed?(snippet)
+      visibility_level && visibility_level.to_i != snippet.visibility_level
+    end
+
     def save_and_commit(snippet)
       return false unless snippet.save
 

spec/services/snippets/create_service_spec.rb

@@ -74,47 +74,6 @@ describe Snippets::CreateService do
       end
     end
 
-    shared_examples 'spam check is performed' do
-      shared_examples 'marked as spam' do
-        it 'marks a snippet as spam' do
-          expect(snippet).to be_spam
-        end
-
-        it 'invalidates the snippet' do
-          expect(snippet).to be_invalid
-        end
-
-        it 'creates a new spam_log' do
-          expect { snippet }
-            .to have_spam_log(title: snippet.title, noteable_type: snippet.class.name)
-        end
-
-        it 'assigns a spam_log to an issue' do
-          expect(snippet.spam_log).to eq(SpamLog.last)
-        end
-      end
-
-      let(:extra_opts) do
-        { visibility_level: Gitlab::VisibilityLevel::PUBLIC, request: double(:request, env: {}) }
-      end
-
-      before do
-        expect_next_instance_of(Spam::AkismetService) do |akismet_service|
-          expect(akismet_service).to receive_messages(spam?: true)
-        end
-      end
-
-      [true, false, nil].each do |allow_possible_spam|
-        context "when recaptcha_disabled flag is #{allow_possible_spam.inspect}" do
-          before do
-            stub_feature_flags(allow_possible_spam: allow_possible_spam) unless allow_possible_spam.nil?
-          end
-
-          it_behaves_like 'marked as spam'
-        end
-      end
-    end
-
     shared_examples 'snippet create data is tracked' do
       let(:counter) { Gitlab::UsageDataCounters::SnippetCounter }
 
@@ -280,7 +239,7 @@ describe Snippets::CreateService do
 
       it_behaves_like 'a service that creates a snippet'
       it_behaves_like 'public visibility level restrictions apply'
-      it_behaves_like 'spam check is performed'
+      it_behaves_like 'snippets spam check is performed'
       it_behaves_like 'snippet create data is tracked'
       it_behaves_like 'an error service response when save fails'
       it_behaves_like 'creates repository and files'
@@ -306,7 +265,7 @@ describe Snippets::CreateService do
 
       it_behaves_like 'a service that creates a snippet'
       it_behaves_like 'public visibility level restrictions apply'
-      it_behaves_like 'spam check is performed'
+      it_behaves_like 'snippets spam check is performed'
       it_behaves_like 'snippet create data is tracked'
       it_behaves_like 'an error service response when save fails'
       it_behaves_like 'creates repository and files'

spec/services/snippets/update_service_spec.rb

@@ -7,7 +7,7 @@ describe Snippets::UpdateService do
     let_it_be(:user) { create(:user) }
     let_it_be(:admin) { create :user, admin: true }
     let(:visibility_level) { Gitlab::VisibilityLevel::PRIVATE }
-    let(:options) do
+    let(:base_opts) do
       {
         title: 'Test snippet',
         file_name: 'snippet.rb',
@@ -15,6 +15,8 @@ describe Snippets::UpdateService do
         visibility_level: visibility_level
       }
     end
+    let(:extra_opts) { {} }
+    let(:options) { base_opts.merge(extra_opts) }
     let(:updater) { user }
     let(:service) { Snippets::UpdateService.new(project, updater, options) }
 
@@ -85,7 +87,7 @@ describe Snippets::UpdateService do
       end
 
       context 'when update fails' do
-        let(:options) { { title: '' } }
+        let(:extra_opts) { { title: '' } }
 
         it 'does not increment count' do
           expect { subject }.not_to change { counter.read(:update) }
@@ -273,7 +275,7 @@ describe Snippets::UpdateService do
 
     shared_examples 'committable attributes' do
       context 'when file_name is updated' do
-        let(:options) { { file_name: 'snippet.rb' } }
+        let(:extra_opts) { { file_name: 'snippet.rb' } }
 
         it 'commits to repository' do
           expect(service).to receive(:create_commit)
@@ -282,7 +284,7 @@ describe Snippets::UpdateService do
       end
 
       context 'when content is updated' do
-        let(:options) { { content: 'puts "hello world"' } }
+        let(:extra_opts) { { content: 'puts "hello world"' } }
 
         it 'commits to repository' do
           expect(service).to receive(:create_commit)
@@ -314,6 +316,11 @@ describe Snippets::UpdateService do
       it_behaves_like 'updates repository content'
       it_behaves_like 'commit operation fails'
       it_behaves_like 'committable attributes'
+      it_behaves_like 'snippets spam check is performed' do
+        before do
+          subject
+        end
+      end
 
       context 'when snippet does not have a repository' do
         let!(:snippet) { create(:project_snippet, author: user, project: project) }
@@ -333,6 +340,11 @@ describe Snippets::UpdateService do
       it_behaves_like 'updates repository content'
       it_behaves_like 'commit operation fails'
       it_behaves_like 'committable attributes'
+      it_behaves_like 'snippets spam check is performed' do
+        before do
+          subject
+        end
+      end
 
       context 'when snippet does not have a repository' do
         let!(:snippet) { create(:personal_snippet, author: user, project: project) }

spec/support/shared_examples/services/snippets_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'snippets spam check is performed' do
+  shared_examples 'marked as spam' do
+    it 'marks a snippet as spam' do
+      expect(snippet).to be_spam
+    end
+
+    it 'invalidates the snippet' do
+      expect(snippet).to be_invalid
+    end
+
+    it 'creates a new spam_log' do
+      expect { snippet }
+        .to have_spam_log(title: snippet.title, noteable_type: snippet.class.name)
+    end
+
+    it 'assigns a spam_log to an issue' do
+      expect(snippet.spam_log).to eq(SpamLog.last)
+    end
+  end
+
+  let(:extra_opts) do
+    { visibility_level: Gitlab::VisibilityLevel::PUBLIC, request: double(:request, env: {}) }
+  end
+
+  before do
+    expect_next_instance_of(Spam::AkismetService) do |akismet_service|
+      expect(akismet_service).to receive_messages(spam?: true)
+    end
+  end
+
+  [true, false, nil].each do |allow_possible_spam|
+    context "when allow_possible_spam flag is #{allow_possible_spam.inspect}" do
+      before do
+        stub_feature_flags(allow_possible_spam: allow_possible_spam) unless allow_possible_spam.nil?
+      end
+
+      it_behaves_like 'marked as spam'
+    end
+  end
+end