Merge branch 'tancnle/refactor-audit-log-finder' into 'master'

Refactor AuditLogFinder public interface

See merge request gitlab-org/gitlab!27209

app/models/audit_event.rb

@@ -13,11 +13,18 @@ class AuditEvent < ApplicationRecord
 
   scope :by_entity_type, -> (entity_type) { where(entity_type: entity_type) }
   scope :by_entity_id, -> (entity_id) { where(entity_id: entity_id) }
-  scope :order_by_id_desc, -> { order(id: :desc) }
-  scope :order_by_id_asc, -> { order(id: :asc) }
 
   after_initialize :initialize_details
 
+  def self.order_by(method)
+    case method.to_s
+    when 'created_asc'
+      order(id: :asc)
+    else
+      order(id: :desc)
+    end
+  end
+
   def initialize_details
     self.details = {} if details.nil?
   end

ee/app/finders/audit_log_finder.rb

@@ -2,20 +2,33 @@
 
 class AuditLogFinder
   include CreatedAtFilter
+  include FinderMethods
+
   VALID_ENTITY_TYPES = %w[Project User Group].freeze
 
-  def initialize(params)
+  # Instantiates a new finder
+  #
+  # @param [Hash] params
+  # @option params [String] :entity_type
+  # @option params [Integer] :entity_id
+  # @option params [DateTime] :created_after from created_at date
+  # @option params [DateTime] :created_before to created_at date
+  # @option params [String] :sort order by field_direction (e.g. created_asc)
+  #
+  # @return [AuditLogFinder]
+  def initialize(params = {})
     @params = params
   end
 
+  # Filters and sorts records according to `params`
+  #
+  # @return [AuditEvent::ActiveRecord_Relation]
   def execute
     audit_events = AuditEvent.all
     audit_events = by_entity(audit_events)
     audit_events = by_created_at(audit_events)
-    audit_events = sort(audit_events)
-    audit_events = by_id(audit_events)
 
-    audit_events
+    sort(audit_events)
   end
 
   private
@@ -27,26 +40,22 @@ class AuditLogFinder
 
     audit_events = audit_events.by_entity_type(params[:entity_type])
 
-    if params[:entity_id].present? && params[:entity_id] != '0'
+    if valid_entity_id?
       audit_events = audit_events.by_entity_id(params[:entity_id])
     end
 
     audit_events
   end
 
-  def by_id(audit_events)
-    return audit_events unless params[:id].present?
-
-    audit_events.find_by_id(params[:id])
-  end
-
   def sort(audit_events)
-    return audit_events.order_by_id_asc if params[:sort] == 'created_asc'
-
-    audit_events.order_by_id_desc
+    audit_events.order_by(params[:sort])
   end
 
   def valid_entity_type?
     VALID_ENTITY_TYPES.include? params[:entity_type]
   end
+
+  def valid_entity_id?
+    params[:entity_id].to_i.nonzero?
+  end
 end

ee/lib/api/audit_events.rb

@@ -37,8 +37,10 @@ module API
         requires :id, type: Integer, desc: 'The ID of audit event'
       end
       get ':id' do
-        audit_event = AuditEvent.find_by_id(params[:id])
-        not_found!('Audit Event') unless audit_event
+        # rubocop: disable CodeReuse/ActiveRecord
+        # This is not `find_by!` from ActiveRecord
+        audit_event = AuditLogFinder.new.find_by!(id: params[:id])
+        # rubocop: enable CodeReuse/ActiveRecord
 
         present audit_event, with: EE::API::Entities::AuditEvent
       end

ee/lib/ee/api/groups.rb

@@ -114,10 +114,11 @@ module EE
               requires :audit_event_id, type: Integer, desc: 'The ID of the audit event'
             end
             get '/:audit_event_id' do
-              audit_log_finder_params = audit_log_finder_params(user_group)
-              audit_event = AuditLogFinder.new(audit_log_finder_params.merge(id: params[:audit_event_id])).execute
-
-              not_found!('Audit Event') unless audit_event
+              # rubocop: disable CodeReuse/ActiveRecord
+              # This is not `find_by!` from ActiveRecord
+              audit_event = AuditLogFinder.new(audit_log_finder_params(user_group))
+                .find_by!(id: params[:audit_event_id])
+              # rubocop: enable CodeReuse/ActiveRecord
 
               present audit_event, with: EE::API::Entities::AuditEvent
             end

ee/spec/finders/audit_log_finder_spec.rb

@@ -3,11 +3,11 @@
 require 'spec_helper'
 
 describe AuditLogFinder do
-  describe '#execute' do
-    let_it_be(:user_audit_event) { create(:user_audit_event, created_at: 3.days.ago) }
-    let_it_be(:project_audit_event) { create(:project_audit_event, created_at: 2.days.ago) }
-    let_it_be(:group_audit_event) { create(:group_audit_event, created_at: 1.day.ago) }
+  let_it_be(:user_audit_event) { create(:user_audit_event, created_at: 3.days.ago) }
+  let_it_be(:project_audit_event) { create(:project_audit_event, created_at: 2.days.ago) }
+  let_it_be(:group_audit_event) { create(:group_audit_event, created_at: 1.day.ago) }
 
+  describe '#execute' do
     subject { described_class.new(params).execute }
 
     context 'no filtering' do
@@ -27,6 +27,14 @@ describe AuditLogFinder do
         end
       end
 
+      context 'invalid entity_id' do
+        let(:params) { { entity_type: 'User', entity_id: '0' } }
+
+        it 'ignores entity_id and returns all events for given entity_type' do
+          expect(subject.count).to eq(1)
+        end
+      end
+
       shared_examples 'finds the right event' do
         it 'finds the right event' do
           expect(subject.count).to eq(1)
@@ -137,22 +145,21 @@ describe AuditLogFinder do
         end
       end
     end
+  end
 
-    context 'filtering by id' do
-      context 'non-existent id provided' do
-        let(:params) { { id: 'non-existent-id' } }
+  describe '#find_by!' do
+    let(:params) { {} }
+    let(:id) { user_audit_event.id }
 
-        it 'returns nil' do
-          expect(subject).to be_nil
-        end
-      end
+    subject { described_class.new(params).find_by!(id: id) }
 
-      context 'existent id provided' do
-        let(:params) { { id: user_audit_event.id } }
+    it { is_expected.to eq(user_audit_event) }
 
-        it 'returns the specific audit events with the id' do
-          expect(subject).to eql(user_audit_event)
-        end
+    context 'non-existent id provided' do
+      let(:id) { 'non-existent-id' }
+
+      it 'raises exception' do
+        expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
       end
     end
   end

ee/spec/models/audit_event_spec.rb

@@ -13,6 +13,30 @@ RSpec.describe AuditEvent, type: :model do
     it { is_expected.to validate_presence_of(:entity_type) }
   end
 
+  describe '.order_by' do
+    let_it_be(:event_1) { create(:audit_event) }
+    let_it_be(:event_2) { create(:audit_event) }
+    let_it_be(:event_3) { create(:audit_event) }
+
+    subject(:event) { described_class.order_by(method) }
+
+    context 'when sort by created_at in ascending order' do
+      let(:method) { 'created_asc' }
+
+      it 'sorts results by id in ascending order' do
+        expect(event).to eq([event_1, event_2, event_3])
+      end
+    end
+
+    context 'when it is default' do
+      let(:method) { nil }
+
+      it 'sorts results by id in descending order' do
+        expect(event).to eq([event_3, event_2, event_1])
+      end
+    end
+  end
+
   describe '#author_name' do
     context 'when user exists' do
       let(:user) { create(:user, name: 'John Doe') }