Commit 577052ca authored by Max Woolf's avatar Max Woolf Committed by Heinrich Lee Yu

Fix batch query issue when primary key is -1

AuditEvents loaded via BatchLoader were
behaving strangely and displaying incorrect
author information when the collection
includes rows with no author. Because
records without an author are stored as -1,
this created unexpected behaviour.
parent 0d37c9a7
......@@ -55,15 +55,20 @@ class AuditEvent < ApplicationRecord
end
def author_name
lazy_author.name
author&.name
end
def formatted_details
details.merge(details.slice(:from, :to).transform_values(&:to_s))
end
def author
lazy_author&.itself.presence ||
::Gitlab::Audit::NullAuthor.for(author_id, (self[:author_name] || details[:author_name]))
end
def lazy_author
BatchLoader.for(author_id).batch(default_value: default_author_value, replace_methods: false) do |author_ids, loader|
BatchLoader.for(author_id).batch(replace_methods: false) do |author_ids, loader|
User.select(:id, :name, :username).where(id: author_ids).find_each do |user|
loader.call(user.id, user)
end
......
---
title: Fix batch query issue when primary key is -1
merge_request: 51716
author:
type: fixed
......@@ -4,7 +4,7 @@ class AuditEventPresenter < Gitlab::View::Presenter::Simple
presents :audit_event
def author_name
author&.name
audit_event.author_name
end
def author_url
......@@ -48,7 +48,7 @@ class AuditEventPresenter < Gitlab::View::Presenter::Simple
private
def author
@author ||= audit_event.lazy_author
audit_event.author
end
def entity
......
......@@ -173,4 +173,22 @@ RSpec.describe 'Projects > Audit Events', :js do
it_behaves_like 'audit events date filter'
end
describe 'combined list of authenticated and unauthenticated users' do
let!(:audit_event_1) { create(:project_audit_event, :unauthenticated, entity_type: 'Project', entity_id: project.id, created_at: 5.days.ago) }
let!(:audit_event_2) { create(:project_audit_event, author_id: non_existing_record_id, entity_type: 'Project', entity_id: project.id, created_at: 3.days.ago) }
let!(:audit_event_3) { create(:project_audit_event, entity_type: 'Project', entity_id: project.id, created_at: Date.current) }
it 'displays the correct authors names' do
visit project_audit_events_path(project)
wait_for_all_requests
page.within('.audit-log-table') do
expect(page).to have_content('An unauthenticated user')
expect(page).to have_content("#{audit_event_2.author_name} (removed)")
expect(page).to have_content(audit_event_3.user.name)
end
end
end
end
......@@ -277,4 +277,32 @@ RSpec.describe AuditEvent, type: :model do
expect(event.formatted_details[:from]).to eq('false')
end
end
describe 'author' do
subject { event.author }
context 'when author exists' do
let_it_be(:event) { create(:project_audit_event) }
it 'returns the author object' do
expect(subject).to eq(User.find(event.author_id))
end
end
context 'when author is unauthenticated' do
let_it_be(:event) { create(:project_audit_event, :unauthenticated) }
it 'is an unauthenticated user' do
expect(subject).to be_a(Gitlab::Audit::UnauthenticatedAuthor)
end
end
context 'when author no longer exists' do
let_it_be(:event) { create(:project_audit_event, author_id: non_existing_record_id) }
it 'is a deleted user' do
expect(subject).to be_a(Gitlab::Audit::DeletedAuthor)
end
end
end
end
......@@ -49,6 +49,21 @@ FactoryBot.define do
end
end
trait :unauthenticated do
author_id { -1 }
details do
{
custom_message: 'Custom action',
author_name: 'An unauthenticated user',
target_id: target_project.id,
target_type: 'Project',
target_details: target_project.name,
ip_address: '127.0.0.1',
entity_path: target_project.full_path
}
end
end
trait :group_event do
transient { target_group { association(:group) } }
......
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