Respond to reviewer comments

Most important of which is removing 'setup' lazy sequencing binding.

Also some re-ordering has moved some actions to the top level as they
are of general importance.

spec/services/git/wiki_push_service_spec.rb

@@ -5,180 +5,127 @@ require 'spec_helper'
 describe Git::WikiPushService, services: true do
   include RepoHelpers
 
-  let(:gl_repository) { "wiki-#{project.id}" }
-  let(:key) { create(:key, user: current_user) }
-  let(:key_id) { key.shell_id }
+  let_it_be(:key_id) { create(:key, user: current_user).shell_id }
   let_it_be(:project) { create(:project, :wiki_repo) }
   let_it_be(:current_user) { create(:user) }
   let_it_be(:git_wiki) { project.wiki.wiki }
   let_it_be(:repository) { git_wiki.repository }
-  let(:commit_details) { create(:git_wiki_commit_details, author: current_user) }
-
-  # Any actions that need to happen before we capture the base_sha
-  let(:setup) { nil }
-
-  # We need to know the SHA before each example runs, hence the use of let-bang
-  let!(:base_sha) do
-    setup
-    current_sha || Gitlab::Git::BLANK_SHA
-  end
 
   describe '#execute' do
     context 'the push contains more than the permitted number of changes' do
-      before do
-        described_class::MAX_CHANGES.times { write_new_page }
-        write_new_page
+      def run_service
+        process_changes { described_class::MAX_CHANGES.succ.times { write_new_page } }
       end
 
       it 'creates only MAX_CHANGES events' do
-        expect { execute_service }.to change(Event, :count).by(described_class::MAX_CHANGES)
+        expect { run_service }.to change(Event, :count).by(described_class::MAX_CHANGES)
       end
     end
 
     context 'default_branch collides with a tag' do
-      before do
+      it 'creates only one event' do
+        base_sha = current_sha
         write_new_page
-      end
 
-      it 'creates only one event' do
-        changes = post_received(base_sha, ['refs/heads/master', 'refs/tags/master']).changes
-        service = described_class.new(project, current_user, changes: changes)
+        service = create_service(base_sha, ['refs/heads/master', 'refs/tags/master'])
 
         expect { service.execute }.to change(Event, :count).by(1)
       end
     end
 
-    context 'one creation, one update, one deletion' do
-      let(:wiki_page_a) { create(:wiki_page, project: project) }
-      let(:wiki_page_b) { create(:wiki_page, project: project) }
-      let(:count) { Event::WIKI_ACTIONS.count }
-      let(:setup) { wiki_page_a && wiki_page_b }
+    describe 'successfully creating events' do
+      let(:count) { Event::WIKI_ACTIONS.size }
 
-      before do
+      def run_service
+        wiki_page_a = create(:wiki_page, project: project)
+        wiki_page_b = create(:wiki_page, project: project)
+
+        process_changes do
           write_new_page
           update_page(wiki_page_a.title)
           delete_page(wiki_page_b.page.path)
         end
+      end
 
-      it 'creates two events' do
-        expect { execute_service }.to change(Event, :count).by(count)
+      it 'creates one event for every wiki action' do
+        expect { run_service }.to change(Event, :count).by(count)
       end
 
       it 'handles all known actions' do
-        execute_service
+        run_service
 
         expect(Event.last(count).pluck(:action)).to match_array(Event::WIKI_ACTIONS)
       end
-
-      shared_examples 'a no-op push' do
-        it 'does not create any events' do
-          expect { execute_service }.not_to change(Event, :count)
-        end
-
-        it 'does not even look for events to process' do
-          service = described_class.new(project, current_user, changes: post_received.changes)
-
-          expect(service).not_to receive(:changed_files)
-
-          service.execute
-        end
-      end
-
-      context 'the wiki_events feature is disabled' do
-        before do
-          stub_feature_flags(wiki_events: false)
-        end
-
-        it_behaves_like 'a no-op push'
-      end
-
-      context 'the wiki_events_on_git_push feature is disabled' do
-        before do
-          stub_feature_flags(wiki_events_on_git_push: false)
-        end
-
-        it_behaves_like 'a no-op push'
-
-        context 'but is enabled for a given project' do
-          before do
-            stub_feature_flags(wiki_events_on_git_push: { enabled: true, thing: project })
-          end
-
-          it 'creates events' do
-            expect { execute_service }.to change(Event, :count).by(count)
-          end
-        end
-      end
     end
 
     context 'two pages have been created' do
-      before do
+      def run_service
+        process_changes do
           write_new_page
           write_new_page
         end
+      end
 
       it 'creates two events' do
-        expect { execute_service }.to change(Event, :count).by(2)
+        expect { run_service }.to change(Event, :count).by(2)
       end
 
       it 'creates two metadata records' do
-        expect { execute_service }.to change(WikiPage::Meta, :count).by(2)
+        expect { run_service }.to change(WikiPage::Meta, :count).by(2)
       end
 
       it 'creates appropriate events' do
-        execute_service
+        run_service
 
         expect(Event.last(2)).to all(have_attributes(wiki_page?: true, action: Event::CREATED))
       end
     end
 
     context 'a non-page file as been added' do
-      before do
-        write_non_page
-      end
-
       it 'does not create events, or WikiPage metadata' do
-        expect { execute_service }.not_to change { [Event.count, WikiPage::Meta.count] }
+        expect do
+          process_changes { write_non_page }
+        end.not_to change { [Event.count, WikiPage::Meta.count] }
       end
     end
 
     context 'one page, and one non-page have been created' do
-      before do
+      def run_service
+        process_changes do
           write_new_page
           write_non_page
         end
-
-      it 'creates one events' do
-        expect { execute_service }.to change(Event, :count).by(1)
-      end
-
-      it 'creates two metadata records' do
-        expect { execute_service }.to change(WikiPage::Meta, :count).by(1)
       end
 
-      it 'creates appropriate events' do
-        execute_service
+      it 'creates a wiki page creation event' do
+        expect { run_service }.to change(Event, :count).by(1)
 
         expect(Event.last).to have_attributes(wiki_page?: true, action: Event::CREATED)
       end
+
+      it 'creates one metadata record' do
+        expect { run_service }.to change(WikiPage::Meta, :count).by(1)
+      end
     end
 
     context 'one page has been added, and then updated' do
-      before do
+      def run_service
+        process_changes do
           title = write_new_page
           update_page(title)
         end
+      end
 
       it 'creates just a single event' do
-        expect { execute_service }.to change(Event, :count).by(1)
+        expect { run_service }.to change(Event, :count).by(1)
       end
 
       it 'creates just one metadata record' do
-        expect { execute_service }.to change(WikiPage::Meta, :count).by(1)
+        expect { run_service }.to change(WikiPage::Meta, :count).by(1)
       end
 
       it 'creates a new wiki page creation event' do
-        execute_service
+        run_service
 
         expect(Event.last).to have_attributes(
           wiki_page?: true,
@@ -189,62 +136,49 @@ describe Git::WikiPushService, services: true do
 
     context 'when a page we already know about has been updated' do
       let(:wiki_page) { create(:wiki_page, project: project) }
-      let(:setup) { create(:wiki_page_meta, :for_wiki_page, wiki_page: wiki_page) }
 
       before do
-        update_page(wiki_page.title)
+        create(:wiki_page_meta, :for_wiki_page, wiki_page: wiki_page)
+      end
+
+      def run_service
+        process_changes { update_page(wiki_page.title) }
       end
 
       it 'does not create a new meta-data record' do
-        expect { execute_service }.not_to change(WikiPage::Meta, :count)
+        expect { run_service }.not_to change(WikiPage::Meta, :count)
       end
 
       it 'creates a new event' do
-        expect { execute_service }.to change(Event, :count).by(1)
+        expect { run_service }.to change(Event, :count).by(1)
       end
 
       it 'adds an update event' do
-        execute_service
+        run_service
 
         expect(Event.last).to have_attributes(
           wiki_page?: true,
           action: Event::UPDATED
         )
       end
-
-      context 'adding the event is not successful' do
-        it 'calls log_error' do
-          event_service = double(:event_service)
-          error = ServiceResponse.error(message: 'something went very very wrong')
-          service = described_class.new(project, current_user, changes: post_received.changes)
-          allow(service).to receive(:event_service).and_return(event_service)
-          allow(event_service).to receive(:execute).with(String, WikiPage, Integer).and_return(error)
-
-          expect(service).to receive(:log_error).with(error.message)
-
-          service.execute
-        end
-      end
     end
 
     context 'when a page we do not know about has been updated' do
-      let(:wiki_page) { create(:wiki_page, project: project) }
-      let(:setup) { wiki_page }
-
-      before do
-        update_page(wiki_page.title)
+      def run_service
+        wiki_page = create(:wiki_page, project: project)
+        process_changes { update_page(wiki_page.title) }
       end
 
       it 'creates a new meta-data record' do
-        expect { execute_service }.to change(WikiPage::Meta, :count).by(1)
+        expect { run_service }.to change(WikiPage::Meta, :count).by(1)
       end
 
       it 'creates a new event' do
-        expect { execute_service(base_sha) }.to change(Event, :count).by(1)
+        expect { run_service }.to change(Event, :count).by(1)
       end
 
       it 'adds an update event' do
-        execute_service
+        run_service
 
         expect(Event.last).to have_attributes(
           wiki_page?: true,
@@ -254,23 +188,21 @@ describe Git::WikiPushService, services: true do
     end
 
     context 'when a page we do not know about has been deleted' do
-      let(:wiki_page) { create(:wiki_page, project: project) }
-      let(:setup) { wiki_page }
-
-      before do
-        delete_page(wiki_page.page.path)
+      def run_service
+        wiki_page = create(:wiki_page, project: project)
+        process_changes { delete_page(wiki_page.page.path) }
       end
 
       it 'create a new meta-data record' do
-        expect { execute_service }.to change(WikiPage::Meta, :count).by(1)
+        expect { run_service }.to change(WikiPage::Meta, :count).by(1)
       end
 
       it 'creates a new event' do
-        expect { execute_service(base_sha) }.to change(Event, :count).by(1)
+        expect { run_service }.to change(Event, :count).by(1)
       end
 
       it 'adds an update event' do
-        execute_service
+        run_service
 
         expect(Event.last).to have_attributes(
           wiki_page?: true,
@@ -278,14 +210,88 @@ describe Git::WikiPushService, services: true do
         )
       end
     end
+
+    it 'calls log_error for every event we cannot create' do
+      base_sha = current_sha
+      count = 3
+      count.times { write_new_page }
+      message = 'something went very very wrong'
+      allow_next_instance_of(WikiPages::EventCreateService, current_user) do |service|
+        allow(service).to receive(:execute)
+          .with(String, WikiPage, Integer)
+          .and_return(ServiceResponse.error(message: message))
+      end
+
+      service = create_service(base_sha)
+
+      expect(service).to receive(:log_error).exactly(count).times.with(message)
+
+      service.execute
+    end
+
+    describe 'feature flags' do
+      shared_examples 'a no-op push' do
+        it 'does not create any events' do
+          expect { process_changes { write_new_page } }.not_to change(Event, :count)
+        end
+
+        it 'does not even look for events to process' do
+          base_sha = current_sha
+          write_new_page
+
+          service = create_service(base_sha)
+
+          expect(service).not_to receive(:changed_files)
+
+          service.execute
+        end
+      end
+
+      context 'the wiki_events feature is disabled' do
+        before do
+          stub_feature_flags(wiki_events: false)
         end
 
-  def execute_service(base = base_sha)
-    changes = post_received(base).changes
-    described_class.new(project, current_user, changes: changes).tap(&:execute)
+        it_behaves_like 'a no-op push'
       end
 
-  def post_received(base = base_sha, refs = ['refs/heads/master'])
+      context 'the wiki_events_on_git_push feature is disabled' do
+        before do
+          stub_feature_flags(wiki_events_on_git_push: false)
+        end
+
+        it_behaves_like 'a no-op push'
+
+        context 'but is enabled for a given project' do
+          before do
+            stub_feature_flags(wiki_events_on_git_push: { enabled: true, thing: project })
+          end
+
+          it 'creates events' do
+            expect { process_changes { write_new_page } }.to change(Event, :count).by(1)
+          end
+        end
+      end
+    end
+  end
+
+  # In order to construct the correct GitPostReceive object that represents the
+  # changes we are applying, we need to describe the changes between old-ref and
+  # new-ref. Old ref (the base sha) we have to capture before we perform any
+  # changes. Once the changes have been applied, we can execute the service to
+  # process them.
+  def process_changes(&block)
+    base_sha = current_sha
+    yield
+    create_service(base_sha).execute
+  end
+
+  def create_service(base, refs = ['refs/heads/master'])
+    changes = post_received(base, refs).changes
+    described_class.new(project, current_user, changes: changes)
+  end
+
+  def post_received(base, refs)
     change_str = refs.map { |ref| +"#{base} #{current_sha} #{ref}" }.join("\n")
     post_received = ::Gitlab::GitPostReceive.new(project, key_id, change_str, {})
     allow(post_received).to receive(:identify).with(key_id).and_return(current_user)
@@ -294,16 +300,19 @@ describe Git::WikiPushService, services: true do
   end
 
   def current_sha
-    repository.gitaly_ref_client.find_branch('master')&.dereferenced_target&.id
+    repository.gitaly_ref_client.find_branch('master')&.dereferenced_target&.id || Gitlab::Git::BLANK_SHA
   end
 
+  # It is important not to re-use the WikiPage services here, since they create
+  # events - these helper methods below are intended to simulate actions on the repo
+  # that have not gone through our services.
+
   def write_new_page
-    commit_details = create(:git_wiki_commit_details, author: current_user)
     generate(:wiki_page_title).tap { |t| git_wiki.write_page(t, 'markdown', 'Hello', commit_details) }
   end
 
   # We write something to the wiki-repo that is not a page - as, for example, an
-  # attachment. This will appear as a raw-diff change, but wiki.find_file will
+  # attachment. This will appear as a raw-diff change, but wiki.find_page will
   # return nil.
   def write_non_page
     params = {
@@ -315,7 +324,6 @@ describe Git::WikiPushService, services: true do
   end
 
   def update_page(title)
-    commit_details = create(:git_wiki_commit_details, author: current_user)
     page = git_wiki.page(title: title)
     git_wiki.update_page(page.path, title, 'markdown', 'Hey', commit_details)
   end
@@ -323,4 +331,8 @@ describe Git::WikiPushService, services: true do
   def delete_page(path)
     git_wiki.delete_page(path, commit_details)
   end
+
+  def commit_details
+    create(:git_wiki_commit_details, author: current_user)
+  end
 end