Add event creation support in wiki services

This ensures that events are created when wiki pages are
created/updated/destroyed in the app.

This does not add events for wiki pages edited off-line and then
published using git-access.

The wiki page base service is the place where WikiPageMeta objects are
constructed, leaving the logic in `EventCreateService#wiki_event` very
thin. For this reason the two related changes are contained in a
transaction.

We also refactor wiki services to avoid passing statically known
parameters. This is a more OOO conformant design, with fewer branches
and if statements. The different kinds of actions are distinguished,
with three actions:
 - external_action (used in the webhook)
 - usage_counter_action (used in the counter)
 - event_action (stored in events)

These have (potentially) different values and different types.

This interface in WikiPages::BaseService is enforced by implementing
the three methods inheriting classes must override.

We also place feature flag checks around more use sites. This ensures
that we do not create wiki page events if the flag is turned off, and
that we do not filter for them either.

Includes suggested changes from reviewer (@.luke)

Co-authored-by: @.luke

Incidental chang: Less dynamic queries

This passes class names for polymorphic types as constant strings. We do
this because these strings are persisted to the database, and changes to
the application need to keep previously stored values in mind. Any
updates to the WikiPage::Meta class name need to ensure that values
stored with `"WikiPage::Meta"` as target_type will still be around.

app/models/event.rb

@@ -36,6 +36,8 @@ class Event < ApplicationRecord
     expired:    EXPIRED
   ).freeze
 
+  WIKI_ACTIONS = [CREATED, UPDATED, DESTROYED].freeze
+
   TARGET_TYPES = HashWithIndifferentAccess.new(
     issue:          Issue,
     milestone:      Milestone,
@@ -81,8 +83,10 @@ class Event < ApplicationRecord
   scope :recent, -> { reorder(id: :desc) }
   scope :code_push, -> { where(action: PUSHED) }
   scope :merged, -> { where(action: MERGED) }
-  scope :for_wiki_page, -> { where(target_type: WikiPage::Meta.name) }
-  scope :not_wiki_page, -> { where('target_type IS NULL or target_type <> ?', WikiPage::Meta.name) }
+  scope :for_wiki_page, -> { where(target_type: 'WikiPage::Meta') }
+
+  # Needed to implement feature flag: can be removed when feature flag is removed
+  scope :not_wiki_page, -> { where('target_type IS NULL or target_type <> ?', 'WikiPage::Meta') }
 
   scope :with_associations, -> do
     # We're using preload for "push_event_payload" as otherwise the association
@@ -230,7 +234,7 @@ class Event < ApplicationRecord
   end
 
   def wiki_page?
-    target_type == WikiPage::Meta.name
+    target_type == 'WikiPage::Meta'
   end
 
   def milestone

app/services/event_create_service.rb

@@ -8,6 +8,8 @@
 #   EventCreateService.new.new_issue(issue, current_user)
 #
 class EventCreateService
+  IllegalActionError = Class.new(StandardError)
+
   def open_issue(issue, current_user)
     create_record_event(issue, current_user, Event::CREATED)
   end
@@ -80,6 +82,19 @@ class EventCreateService
     create_push_event(BulkPushEventPayloadService, project, current_user, push_data)
   end
 
+  # Create a new wiki page event
+  #
+  # @param [WikiPage::Meta] wiki_page_meta The event target
+  # @param [User] current_user The event author
+  # @param [Integer] action One of the Event::WIKI_ACTIONS
+  def wiki_event(wiki_page_meta, current_user, action)
+    return unless Feature.enabled?(:wiki_events)
+
+    raise IllegalActionError, action unless Event::WIKI_ACTIONS.include?(action)
+
+    create_record_event(wiki_page_meta, current_user, action)
+  end
+
   private
 
   def create_record_event(record, current_user, status)

app/services/wiki_pages/base_service.rb

 # frozen_string_literal: true
 
 module WikiPages
+  # There are 3 notions of 'action' that inheriting classes must implement:
+  #
+  # - external_action: the action we report to external clients with webhooks
+  # - usage_counter_action: the action that we count in out internal counters
+  # - event_action: what we record as the value of `Event#action`
   class BaseService < ::BaseService
     private
 
-    def execute_hooks(page, action = 'create')
-      page_data = Gitlab::DataBuilder::WikiPage.build(page, current_user, action)
+    def execute_hooks(page)
+      page_data = payload(page)
       @project.execute_hooks(page_data, :wiki_page_hooks)
       @project.execute_services(page_data, :wiki_page_hooks)
-      increment_usage(action)
+      increment_usage
+      create_wiki_event(page)
+    end
+
+    # Passed to web-hooks, and send to external consumers.
+    def external_action
+      raise NotImplementedError
+    end
+
+    # Passed to the WikiPageCounter to count events.
+    # Must be one of WikiPageCounter::KNOWN_EVENTS
+    def usage_counter_action
+      raise NotImplementedError
+    end
+
+    # Used to create `Event` records.
+    # Must be a valid value for `Event#action`
+    def event_action
+      raise NotImplementedError
+    end
+
+    def payload(page)
+      Gitlab::DataBuilder::WikiPage.build(page, current_user, external_action)
     end
 
     # This method throws an error if the action is an unanticipated value.
-    def increment_usage(action)
-      Gitlab::UsageDataCounters::WikiPageCounter.count(action)
+    def increment_usage
+      Gitlab::UsageDataCounters::WikiPageCounter.count(usage_counter_action)
+    end
+
+    def create_wiki_event(page)
+      return unless ::Feature.enabled?(:wiki_events)
+
+      slug = slug_for_page(page)
+
+      Event.transaction do
+        wiki_page_meta = WikiPage::Meta.find_or_create(slug, page)
+        EventCreateService.new.wiki_event(wiki_page_meta, current_user, event_action)
+      end
+    end
+
+    def slug_for_page(page)
+      page.slug
     end
   end
 end

app/services/wiki_pages/create_service.rb

@@ -7,10 +7,22 @@ module WikiPages
       page = WikiPage.new(project_wiki)
 
       if page.create(@params)
-        execute_hooks(page, 'create')
+        execute_hooks(page)
       end
 
       page
     end
+
+    def usage_counter_action
+      :create
+    end
+
+    def external_action
+      'create'
+    end
+
+    def event_action
+      Event::CREATED
+    end
   end
 end

app/services/wiki_pages/destroy_service.rb

@@ -4,10 +4,22 @@ module WikiPages
   class DestroyService < WikiPages::BaseService
     def execute(page)
       if page&.delete
-        execute_hooks(page, 'delete')
+        execute_hooks(page)
       end
 
       page
     end
+
+    def usage_counter_action
+      :delete
+    end
+
+    def external_action
+      'delete'
+    end
+
+    def event_action
+      Event::DESTROYED
+    end
   end
 end

app/services/wiki_pages/update_service.rb

@@ -3,11 +3,30 @@
 module WikiPages
   class UpdateService < WikiPages::BaseService
     def execute(page)
+      # this class is not thread safe!
+      @old_slug = page.slug
+
       if page.update(@params)
-        execute_hooks(page, 'update')
+        execute_hooks(page)
       end
 
       page
     end
+
+    def usage_counter_action
+      :update
+    end
+
+    def external_action
+      'update'
+    end
+
+    def event_action
+      Event::UPDATED
+    end
+
+    def slug_for_page(page)
+      @old_slug.presence || super
+    end
   end
 end

ee/app/services/ee/wiki_pages/base_service.rb

@@ -11,7 +11,7 @@ module EE
 
       private
 
-      def execute_hooks(page, action = 'create')
+      def execute_hooks(page)
         super
         process_wiki_repository_update
       end

spec/factories/events.rb

@@ -25,12 +25,12 @@ FactoryBot.define do
 
     factory :wiki_page_event do
       action { Event::CREATED }
+      project { @overrides[:wiki_page]&.project || create(:project, :wiki_repo) }
+      target { create(:wiki_page_meta, :for_wiki_page, wiki_page: wiki_page) }
 
       transient do
         wiki_page { create(:wiki_page, project: project) }
       end
-
-      target { create(:wiki_page_meta, :for_wiki_page, wiki_page: wiki_page) }
     end
   end
 

spec/models/event_spec.rb

@@ -470,6 +470,7 @@ describe Event do
       it 'only contains the wiki page events' do
         wiki_events = events.select(&:wiki_page?)
 
+        expect(events).not_to match_array(wiki_events)
         expect(described_class.for_wiki_page).to match_array(wiki_events)
       end
     end
@@ -478,6 +479,7 @@ describe Event do
       it 'does not contain the wiki page events' do
         non_wiki_events = events.reject(&:wiki_page?)
 
+        expect(events).not_to match_array(non_wiki_events)
         expect(described_class.not_wiki_page).to match_array(non_wiki_events)
       end
     end

spec/requests/api/events_spec.rb

@@ -115,21 +115,15 @@ describe API::Events do
       end
 
       context 'when the list of events includes wiki page events' do
-        let(:page) do
-          wiki = create(:project_wiki, project: private_project, user: user)
-          create(:wiki_page, wiki: wiki)
-        end
-
-        before do
-          [Event::CREATED, Event::UPDATED, Event::DESTROYED].each do |event|
-            EventCreateService.new.wiki_event(page, event, nil)
-          end
+        it 'returns information about the wiki event', :aggregate_failures do
+          page = create(:wiki_page, project: private_project)
+          [Event::CREATED, Event::UPDATED, Event::DESTROYED].each do |action|
+            create(:wiki_page_event, wiki_page: page, action: action, author: user)
           end
 
-        it 'returns information about the wiki event', :aggregate_failures do
           get api("/users/#{user.id}/events", user)
 
-          wiki_events = json_response.select { |e| e['target_type'] == 'WikiPageMeta' }
+          wiki_events = json_response.select { |e| e['target_type'] == 'WikiPage::Meta' }
           action_names = wiki_events.map { |e| e['action_name'] }
           titles = wiki_events.map { |e| e['target_title'] }
           slugs = wiki_events.map { |e| e.dig('wiki_page', 'slug') }

spec/services/event_create_service_spec.rb

@@ -153,6 +153,46 @@ describe EventCreateService do
     end
   end
 
+  describe '#wiki_event' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:wiki_page) { create(:wiki_page) }
+    let_it_be(:meta) { create(:wiki_page_meta, :for_wiki_page, wiki_page: wiki_page) }
+
+    Event::WIKI_ACTIONS.each do |action|
+      context "The action is #{action}" do
+        let(:event) { service.wiki_event(meta, user, action) }
+
+        it 'creates the event' do
+          expect(event).to have_attributes(
+            wiki_page?: true,
+            valid?: true,
+            persisted?: true,
+            action: action,
+            wiki_page: wiki_page
+          )
+        end
+
+        context 'the feature is disabled' do
+          before do
+            stub_feature_flags(wiki_events: false)
+          end
+
+          it 'does not create the event' do
+            expect { event }.not_to change(Event, :count)
+          end
+        end
+      end
+    end
+
+    (Event::ACTIONS.values - Event::WIKI_ACTIONS).each do |bad_action|
+      context "The action is #{bad_action}" do
+        it 'raises an error' do
+          expect { service.wiki_event(meta, user, bad_action) }.to raise_error(described_class::IllegalActionError)
+        end
+      end
+    end
+  end
+
   describe '#push', :clean_gitlab_redis_shared_state do
     let(:project) { create(:project) }
     let(:user) { create(:user) }

spec/services/wiki_pages/base_service_spec.rb

@@ -6,22 +6,24 @@ describe WikiPages::BaseService do
   let(:project) { double('project') }
   let(:user) { double('user') }
 
-  subject(:service) { described_class.new(project, user, {}) }
-
   describe '#increment_usage' do
     counter = Gitlab::UsageDataCounters::WikiPageCounter
     error = counter::UnknownEvent
 
-    it 'raises an error on unknown events' do
-      expect { subject.send(:increment_usage, :bad_event) }.to raise_error error
-    end
+    let(:subject) { bad_service_class.new(project, user, {}) }
 
-    context 'the event is valid' do
-      counter::KNOWN_EVENTS.each do |e|
-        it "updates the #{e} counter" do
-          expect { subject.send(:increment_usage, e) }.to change { counter.read(e) }
+    context 'the class implements usage_counter_action incorrectly' do
+      let(:bad_service_class) do
+        Class.new(described_class) do
+          def usage_counter_action
+            :bad_event
+          end
         end
       end
+
+      it 'raises an error on unknown events' do
+        expect { subject.send(:increment_usage) }.to raise_error(error)
+      end
     end
   end
 end

spec/services/wiki_pages/create_service_spec.rb

@@ -5,19 +5,16 @@ require 'spec_helper'
 describe WikiPages::CreateService do
   let(:project) { create(:project, :wiki_repo) }
   let(:user) { create(:user) }
+  let(:page_title) { 'Title' }
 
   let(:opts) do
     {
-      title: 'Title',
+      title: page_title,
       content: 'Content for wiki page',
       format: 'markdown'
     }
   end
 
-  let(:bad_opts) do
-    { title: '' }
-  end
-
   subject(:service) { described_class.new(project, user, opts) }
 
   before do
@@ -35,8 +32,7 @@ describe WikiPages::CreateService do
     end
 
     it 'executes webhooks' do
-      expect(service).to receive(:execute_hooks).once
-        .with(instance_of(WikiPage), 'create')
+      expect(service).to receive(:execute_hooks).once.with(WikiPage)
 
       service.execute
     end
@@ -47,8 +43,41 @@ describe WikiPages::CreateService do
       expect { service.execute }.to change { counter.read(:create) }.by 1
     end
 
+    shared_examples 'correct event created' do
+      it 'creates appropriate events' do
+        expect { service.execute }.to change { Event.count }.by 1
+
+        expect(Event.recent.first).to have_attributes(
+          action: Event::CREATED,
+          target: have_attributes(canonical_slug: page_title)
+        )
+      end
+    end
+
+    context 'the new page is at the top level' do
+      let(:page_title) { 'root-level-page' }
+
+      include_examples 'correct event created'
+    end
+
+    context 'the new page is in a subsection' do
+      let(:page_title) { 'subsection/page' }
+
+      include_examples 'correct event created'
+    end
+
+    context 'the feature is disabled' do
+      before do
+        stub_feature_flags(wiki_events: false)
+      end
+
+      it 'does not record the activity' do
+        expect { service.execute }.not_to change(Event, :count)
+      end
+    end
+
     context 'when the options are bad' do
-      subject(:service) { described_class.new(project, user, bad_opts) }
+      let(:page_title) { '' }
 
       it 'does not count a creation event' do
         counter = Gitlab::UsageDataCounters::WikiPageCounter
@@ -56,6 +85,10 @@ describe WikiPages::CreateService do
         expect { service.execute }.not_to change { counter.read(:create) }
       end
 
+      it 'does not record the activity' do
+        expect { service.execute }.not_to change(Event, :count)
+      end
+
       it 'reports the error' do
         expect(service.execute).to be_invalid
           .and have_attributes(errors: be_present)

spec/services/wiki_pages/destroy_service_spec.rb

@@ -15,8 +15,7 @@ describe WikiPages::DestroyService do
 
   describe '#execute' do
     it 'executes webhooks' do
-      expect(service).to receive(:execute_hooks).once
-        .with(instance_of(WikiPage), 'delete')
+      expect(service).to receive(:execute_hooks).once.with(page)
 
       service.execute(page)
     end
@@ -27,10 +26,29 @@ describe WikiPages::DestroyService do
       expect { service.execute(page) }.to change { counter.read(:delete) }.by 1
     end
 
+    it 'creates a new wiki page deletion event' do
+      expect { service.execute(page) }.to change { Event.count }.by 1
+
+      expect(Event.recent.first).to have_attributes(
+        action: Event::DESTROYED,
+        target: have_attributes(canonical_slug: page.slug)
+      )
+    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) }
     end
   end
+
+  context 'the feature is disabled' do
+    before do
+      stub_feature_flags(wiki_events: false)
+    end
+
+    it 'does not record the activity' do
+      expect { service.execute(page) }.not_to change(Event, :count)
+    end
+  end
 end

spec/services/wiki_pages/update_service_spec.rb

@@ -6,20 +6,17 @@ describe WikiPages::UpdateService do
   let(:project) { create(:project) }
   let(:user) { create(:user) }
   let(:page) { create(:wiki_page) }
+  let(:page_title) { 'New Title' }
 
   let(:opts) do
     {
       content: 'New content for wiki page',
       format: 'markdown',
       message: 'New wiki message',
-      title: 'New Title'
+      title: page_title
     }
   end
 
-  let(:bad_opts) do
-    { title: '' }
-  end
-
   subject(:service) { described_class.new(project, user, opts) }
 
   before do
@@ -34,12 +31,11 @@ describe WikiPages::UpdateService do
       expect(updated_page.message).to eq(opts[:message])
       expect(updated_page.content).to eq(opts[:content])
       expect(updated_page.format).to eq(opts[:format].to_sym)
-      expect(updated_page.title).to eq(opts[:title])
+      expect(updated_page.title).to eq(page_title)
     end
 
     it 'executes webhooks' do
-      expect(service).to receive(:execute_hooks).once
-        .with(instance_of(WikiPage), 'update')
+      expect(service).to receive(:execute_hooks).once.with(WikiPage)
 
       service.execute(page)
     end
@@ -50,8 +46,42 @@ describe WikiPages::UpdateService do
       expect { service.execute page }.to change { counter.read(:update) }.by 1
     end
 
+    shared_examples 'adds activity event' do
+      it 'adds a new wiki page activity event' do
+        expect { service.execute(page) }.to change { Event.count }.by 1
+
+        expect(Event.recent.first).to have_attributes(
+          action: Event::UPDATED,
+          wiki_page: page,
+          target_title: page.title
+        )
+      end
+    end
+
+    context 'the page is at the top level' do
+      let(:page_title) { 'Top level page' }
+
+      include_examples 'adds activity event'
+    end
+
+    context 'the page is in a subsection' do
+      let(:page_title) { 'Subsection / secondary page' }
+
+      include_examples 'adds activity event'
+    end
+
+    context 'the feature is disabled' do
+      before do
+        stub_feature_flags(wiki_events: false)
+      end
+
+      it 'does not record the activity' do
+        expect { service.execute(page) }.not_to change(Event, :count)
+      end
+    end
+
     context 'when the options are bad' do
-      subject(:service) { described_class.new(project, user, bad_opts) }
+      let(:page_title) { '' }
 
       it 'does not count an edit event' do
         counter = Gitlab::UsageDataCounters::WikiPageCounter
@@ -59,6 +89,10 @@ describe WikiPages::UpdateService do
         expect { service.execute page }.not_to change { counter.read(:update) }
       end
 
+      it 'does not record the activity' do
+        expect { service.execute page }.not_to change(Event, :count)
+      end
+
       it 'reports the error' do
         expect(service.execute page).to be_invalid
           .and have_attributes(errors: be_present)