Merge branch 'web-hooks-log-pagination' into 'master'

Fixed pagination of web hook logs

See merge request gitlab-org/gitlab-ce!20247

app/controllers/admin/hooks_controller.rb

@@ -52,8 +52,7 @@ class Admin::HooksController < Admin::ApplicationController
   end
 
   def hook_logs
-    @hook_logs ||=
-      Kaminari.paginate_array(hook.web_hook_logs.order(created_at: :desc)).page(params[:page])
+    @hook_logs ||= hook.web_hook_logs.recent.page(params[:page])
   end
 
   def hook_params

app/controllers/projects/hooks_controller.rb

@@ -58,8 +58,7 @@ class Projects::HooksController < Projects::ApplicationController
   end
 
   def hook_logs
-    @hook_logs ||=
-      Kaminari.paginate_array(hook.web_hook_logs.order(created_at: :desc)).page(params[:page])
+    @hook_logs ||= hook.web_hook_logs.recent.page(params[:page])
   end
 
   def hook_params

app/models/hooks/web_hook_log.rb

@@ -7,6 +7,11 @@ class WebHookLog < ActiveRecord::Base
 
   validates :web_hook, presence: true
 
+  def self.recent
+    where('created_at >= ?', 2.days.ago.beginning_of_day)
+      .order(created_at: :desc)
+  end
+
   def success?
     response_status =~ /^2/
   end

changelogs/unreleased/web-hooks-log-pagination.yml

+---
+title: Fixed pagination of web hook logs
+merge_request:
+author:
+type: performance

db/migrate/20180628124813_alter_web_hook_logs_indexes.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AlterWebHookLogsIndexes < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  # "created_at" comes first so the Sidekiq worker pruning old webhook logs can
+  # use a composite index index.
+  #
+  # We leave the old standalone index on "web_hook_id" in place so future code
+  # that doesn't care about "created_at" can still use that index.
+  COLUMNS_TO_INDEX = %i[created_at web_hook_id]
+
+  def up
+    add_concurrent_index(:web_hook_logs, COLUMNS_TO_INDEX)
+  end
+
+  def down
+    remove_concurrent_index(:web_hook_logs, COLUMNS_TO_INDEX)
+  end
+end

db/schema.rb

@@ -2145,6 +2145,7 @@ ActiveRecord::Schema.define(version: 20180629191052) do
     t.datetime "updated_at", null: false
   end
 
+  add_index "web_hook_logs", ["created_at", "web_hook_id"], name: "index_web_hook_logs_on_created_at_and_web_hook_id", using: :btree
   add_index "web_hook_logs", ["web_hook_id"], name: "index_web_hook_logs_on_web_hook_id", using: :btree
 
   create_table "web_hooks", force: :cascade do |t|

spec/models/hooks/web_hook_log_spec.rb

@@ -9,6 +9,24 @@ describe WebHookLog do
 
   it { is_expected.to validate_presence_of(:web_hook) }
 
+  describe '.recent' do
+    let(:hook) { create(:project_hook) }
+
+    it 'does not return web hook logs that are too old' do
+      create(:web_hook_log, web_hook: hook, created_at: 91.days.ago)
+
+      expect(described_class.recent.size).to be_zero
+    end
+
+    it 'returns the web hook logs in descending order' do
+      hook1 = create(:web_hook_log, web_hook: hook, created_at: 2.hours.ago)
+      hook2 = create(:web_hook_log, web_hook: hook, created_at: 1.hour.ago)
+      hooks = described_class.recent.to_a
+
+      expect(hooks).to eq([hook2, hook1])
+    end
+  end
+
   describe '#success?' do
     let(:web_hook_log) { build(:web_hook_log, response_status: status) }