Commit b8d05639 authored by Max Woolf's avatar Max Woolf

Improve parallel persistance for audit event author_name

On verification of https://gitlab.com/gitlab-org/gitlab/-/issues/220318
we discovered that some audit events were not created with
author_name populated correctly.

This approach saves the author_name as an AR callback instead.
parent e9eaa7b1
...@@ -19,7 +19,15 @@ class AuditEvent < ApplicationRecord ...@@ -19,7 +19,15 @@ class AuditEvent < ApplicationRecord
scope :by_entity_id, -> (entity_id) { where(entity_id: entity_id) } scope :by_entity_id, -> (entity_id) { where(entity_id: entity_id) }
scope :by_author_id, -> (author_id) { where(author_id: author_id) } scope :by_author_id, -> (author_id) { where(author_id: author_id) }
PARALLEL_PERSISTENCE_COLUMNS = [:author_name].freeze
after_initialize :initialize_details after_initialize :initialize_details
# Note: The intention is to remove this once refactoring of AuditEvent
# has proceeded further.
#
# See further details in the epic:
# https://gitlab.com/groups/gitlab-org/-/epics/2765
after_validation :parallel_persist
def self.order_by(method) def self.order_by(method)
case method.to_s case method.to_s
...@@ -55,6 +63,10 @@ class AuditEvent < ApplicationRecord ...@@ -55,6 +63,10 @@ class AuditEvent < ApplicationRecord
def default_author_value def default_author_value
::Gitlab::Audit::NullAuthor.for(author_id, (self[:author_name] || details[:author_name])) ::Gitlab::Audit::NullAuthor.for(author_id, (self[:author_name] || details[:author_name]))
end end
def parallel_persist
PARALLEL_PERSISTENCE_COLUMNS.each { |col| self[col] = details[col] }
end
end end
AuditEvent.prepend_if_ee('EE::AuditEvent') AuditEvent.prepend_if_ee('EE::AuditEvent')
...@@ -28,6 +28,14 @@ RSpec.describe AuditEvent, type: :model do ...@@ -28,6 +28,14 @@ RSpec.describe AuditEvent, type: :model do
end end
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 describe '.order_by' do
let_it_be(:event_1) { create(:audit_event) } let_it_be(:event_1) { create(:audit_event) }
let_it_be(:event_2) { create(:audit_event) } let_it_be(:event_2) { create(:audit_event) }
......
...@@ -61,12 +61,6 @@ RSpec.describe AuditEventPresenter do ...@@ -61,12 +61,6 @@ RSpec.describe AuditEventPresenter do
end end
end end
context 'when author_name is set in the author_name column' do
it 'shows the author name as provided in the database column' do
expect(presenter.author_name).to eq('Jane Doe')
end
end
context 'when `author_name` is included in the details and not in the author_name column' do context 'when `author_name` is included in the details and not in the author_name column' do
before do before do
audit_event.update!(author_name: nil) audit_event.update!(author_name: nil)
......
...@@ -11,15 +11,14 @@ RSpec.describe AuditEventService do ...@@ -11,15 +11,14 @@ RSpec.describe AuditEventService do
let(:service) { described_class.new(user, project, details) } let(:service) { described_class.new(user, project, details) }
describe '#for_member' do describe '#for_member' do
let(:event) { service.for_member(project_member).security_event }
it 'generates event' do it 'generates event' do
event = service.for_member(project_member).security_event
expect(event[:details][:target_details]).to eq(user.name) expect(event[:details][:target_details]).to eq(user.name)
end end
it 'handles deleted users' do it 'handles deleted users' do
expect(project_member).to receive(:user).and_return(nil) expect(project_member).to receive(:user).and_return(nil)
event = service.for_member(project_member).security_event
expect(event[:details][:target_details]).to eq('Deleted User') expect(event[:details][:target_details]).to eq('Deleted User')
end end
...@@ -27,14 +26,21 @@ RSpec.describe AuditEventService do ...@@ -27,14 +26,21 @@ RSpec.describe AuditEventService do
let(:service) { described_class.new(nil, project, { action: :expired }) } let(:service) { described_class.new(nil, project, { action: :expired }) }
it 'generates a system event' do it 'generates a system event' do
event = service.for_member(project_member).security_event
expect(event[:details][:remove]).to eq('user_access') expect(event[:details][:remove]).to eq('user_access')
expect(event[:details][:system_event]).to be_truthy expect(event[:details][:system_event]).to be_truthy
expect(event[:details][:reason]).to include('access expired on') expect(event[:details][:reason]).to include('access expired on')
end end
end end
context 'create user access' do
let(:details) { { action: :create } }
it 'stores author name', :aggregate_failures do
expect(event.details[:author_name]).to eq(user.name)
expect(event.author_name).to eq(user.name)
end
end
context 'updating membership' do context 'updating membership' do
let(:service) do let(:service) do
described_class.new(user, project, { described_class.new(user, project, {
...@@ -273,6 +279,7 @@ RSpec.describe AuditEventService do ...@@ -273,6 +279,7 @@ RSpec.describe AuditEventService do
it 'has the right author' do it 'has the right author' do
expect(event.details[:author_name]).to eq(author_name) expect(event.details[:author_name]).to eq(author_name)
expect(event.author_name).to eq(author_name)
end end
it 'has the right target_details' do it 'has the right target_details' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment