Merge branch '220324-add-entity-path-to-audit-events' into 'master'

Refactor entity_path from AuditEvent details

See merge request gitlab-org/gitlab!37041

app/models/audit_event.rb

@@ -5,6 +5,8 @@ class AuditEvent < ApplicationRecord
   include IgnorableColumns
   include BulkInsertSafe
 
+  PARALLEL_PERSISTENCE_COLUMNS = [:author_name, :entity_path].freeze
+
   ignore_column :updated_at, remove_with: '13.4', remove_after: '2020-09-22'
 
   serialize :details, Hash # rubocop:disable Cop/ActiveRecordSerialize
@@ -19,8 +21,6 @@ class AuditEvent < ApplicationRecord
   scope :by_entity_id, -> (entity_id) { where(entity_id: entity_id) }
   scope :by_author_id, -> (author_id) { where(author_id: author_id) }
 
-  PARALLEL_PERSISTENCE_COLUMNS = [:author_name].freeze
-
   after_initialize :initialize_details
   # Note: The intention is to remove this once refactoring of AuditEvent
   # has proceeded further.

changelogs/unreleased/220324-add-entity-path-to-audit-events.yml

+---
+title: Add entity_path column to audit_events table
+merge_request: 37041
+author:
+type: changed

db/migrate/20200716044023_add_entity_path_to_audit_events.rb

+# frozen_string_literal: true
+
+class AddEntityPathToAuditEvents < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      # rubocop:disable Migration/AddLimitToTextColumns
+      add_column(:audit_events, :entity_path, :text)
+      # rubocop:enable Migration/AddLimitToTextColumns
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column(:audit_events, :entity_path)
+    end
+  end
+end

db/migrate/20200716120419_add_text_limit_on_entity_path_to_audit_events.rb

+# frozen_string_literal: true
+
+class AddTextLimitOnEntityPathToAuditEvents < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_text_limit :audit_events, :entity_path, 5_500
+  end
+
+  def down
+    remove_text_limit :audit_events, :entity_path
+  end
+end

db/structure.sql

@@ -9396,6 +9396,8 @@ CREATE TABLE public.audit_events (
     updated_at timestamp without time zone,
     ip_address inet,
     author_name text,
+    entity_path text,
+    CONSTRAINT check_492aaa021d CHECK ((char_length(entity_path) <= 5500)),
     CONSTRAINT check_83ff8406e2 CHECK ((char_length(author_name) <= 255))
 );
 
@@ -23859,5 +23861,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200712235622
 20200713071042
 20200713152443
+20200716044023
+20200716120419
 \.
 

ee/app/models/ee/audit_event.rb

@@ -13,6 +13,10 @@ module EE
       lazy_entity
     end
 
+    def entity_path
+      super || details[:entity_path]
+    end
+
     def present
       AuditEventPresenter.new(self)
     end

ee/app/presenters/audit_event_presenter.rb

@@ -28,7 +28,7 @@ class AuditEventPresenter < Gitlab::View::Presenter::Simple
   def object
     return if entity.is_a?(Gitlab::Audit::NullEntity)
 
-    details[:entity_path] || entity.name
+    audit_event.entity_path || entity.name
   end
 
   def object_url

ee/spec/factories/audit_events.rb

@@ -8,6 +8,7 @@ FactoryBot.define do
 
     entity_type { 'User' }
     entity_id   { target_user.id }
+    entity_path { target_user.full_path }
     ip_address { IPAddr.new '127.0.0.1' }
     author_name { 'Jane Doe' }
     details do
@@ -20,7 +21,7 @@ FactoryBot.define do
         target_type: 'User',
         target_details: target_user.name,
         ip_address: '127.0.0.1',
-        entity_path: target_user.username
+        entity_path: target_user.full_path
       }
     end
 
@@ -29,6 +30,7 @@ FactoryBot.define do
 
       entity_type { 'Project' }
       entity_id   { target_project.id }
+      entity_path { target_project.full_path }
       ip_address { IPAddr.new '127.0.0.1' }
       details do
         {
@@ -40,7 +42,7 @@ FactoryBot.define do
           target_type: 'Project',
           target_details: target_project.name,
           ip_address: '127.0.0.1',
-          entity_path: 'gitlab.org/gitlab-ce'
+          entity_path: target_project.full_path
         }
       end
     end
@@ -50,6 +52,7 @@ FactoryBot.define do
 
       entity_type { 'Group' }
       entity_id   { target_group.id }
+      entity_path { target_group.full_path }
       ip_address { IPAddr.new '127.0.0.1' }
       details do
         {
@@ -61,7 +64,7 @@ FactoryBot.define do
           target_type: 'Group',
           target_details: target_group.name,
           ip_address: '127.0.0.1',
-          entity_path: "gitlab-org"
+          entity_path: target_group.full_path
         }
       end
     end

ee/spec/models/audit_event_spec.rb

@@ -13,6 +13,19 @@ RSpec.describe AuditEvent, type: :model do
     it { is_expected.to validate_presence_of(:entity_type) }
   end
 
+  describe 'callbacks' do
+    let_it_be(:details) { { author_name: 'Kungfu Panda', entity_path: 'gitlab-org/gitlab' } }
+    let_it_be(:event) { create(:project_audit_event, details: details) }
+
+    it 'sets author_name' do
+      expect(event[:author_name]).to eq('Kungfu Panda')
+    end
+
+    it 'sets entity_path' do
+      expect(event[:entity_path]).to eq('gitlab-org/gitlab')
+    end
+  end
+
   describe '.by_entity' do
     let_it_be(:project_event_1) { create(:project_audit_event) }
     let_it_be(:project_event_2) { create(:project_audit_event) }
@@ -28,14 +41,6 @@ RSpec.describe AuditEvent, type: :model do
     end
   end
 
-  describe 'callbacks' do
-    it 'sets author_name' do
-      event = create(:user_audit_event, details: { author_name: 'Kungfu Panda' })
-
-      expect(event[:author_name]).to eq('Kungfu Panda')
-    end
-  end
-
   describe '.order_by' do
     let_it_be(:event_1) { create(:audit_event) }
     let_it_be(:event_2) { create(:audit_event) }
@@ -126,6 +131,26 @@ RSpec.describe AuditEvent, type: :model do
     end
   end
 
+  describe '#entity_path' do
+    context 'when entity_path exists in both details hash and entity_path column' do
+      subject(:event) do
+        described_class.new(entity_path: 'gitlab-org/gitlab', details: { entity_path: 'gitlab-org/gitlab-foss' })
+      end
+
+      it 'returns the value from entity_path column' do
+        expect(event.entity_path).to eq('gitlab-org/gitlab')
+      end
+    end
+
+    context 'when entity_path exists in details hash but not in entity_path column' do
+      subject(:event) { described_class.new(details: { entity_path: 'gitlab-org/gitlab-foss' }) }
+
+      it 'returns the value from details hash' do
+        expect(event.entity_path).to eq('gitlab-org/gitlab-foss')
+      end
+    end
+  end
+
   describe '#present' do
     it 'returns a presenter' do
       expect(subject.present).to be_an_instance_of(AuditEventPresenter)

ee/spec/presenters/audit_event_presenter_spec.rb

@@ -132,7 +132,7 @@ RSpec.describe AuditEventPresenter do
 
   context 'exposes the object' do
     it 'returns the object path if it exists' do
-      expect(presenter.object).to eq(details[:entity_path])
+      expect(presenter.object).to eq(audit_event.entity_path)
     end
 
     it 'returns the stored name if it has been deleted' do