Merge branch 'fix-audit-events' into 'master'

Keep track of audit event's author even when author user  was deleted

Fixes #252 

See merge request !164

app/helpers/audit_events_helper.rb

 module AuditEventsHelper
-
   def human_text(details)
     details.map{ |key, value| select_keys(key, value) }.join(" ").humanize
   end
 
   def select_keys(key, value)
-    if key.match(/^target_.*/)
+    if key.match(/^(author|target)_.*/)
       ""
     else
       "#{key.to_s} <strong>#{value}</strong>"

app/models/audit_event.rb

@@ -28,6 +28,6 @@ class AuditEvent < ActiveRecord::Base
   end
 
   def author_name
-    self.user.name
+    self.user.try(:name) || details[:author_name]
   end
 end

app/services/audit_event_service.rb

@@ -6,6 +6,7 @@ class AuditEventService
   def for_member(member)
     action = @details[:action]
     old_access_level = @details[:old_access_level]
+    author_name = @author.name
     user_id = member.id
     user_name = member.user.name
 
@@ -14,6 +15,7 @@ class AuditEventService
       when :destroy
         {
           remove: "user_access",
+          author_name: author_name,
           target_id: user_id,
           target_type: "User",
           target_details: user_name,
@@ -22,6 +24,7 @@ class AuditEventService
         {
           add: "user_access",
           as: Gitlab::Access.options_with_owner.key(member.access_level.to_i),
+          author_name: author_name,
           target_id: user_id,
           target_type: "User",
           target_details: user_name,
@@ -31,6 +34,7 @@ class AuditEventService
           change: "access_level",
           from: old_access_level,
           to: member.human_access,
+          author_name: author_name,
           target_id: user_id,
           target_type: "User",
           target_details: user_name,
@@ -42,12 +46,14 @@ class AuditEventService
 
   def for_deploy_key(key_title)
     action = @details[:action]
+    author_name = @author.name
 
     @details =
       case action
       when :destroy
         {
           remove: "deploy_key",
+          author_name: author_name,
           target_id: key_title,
           target_type: "DeployKey",
           target_details: key_title,
@@ -55,6 +61,7 @@ class AuditEventService
       when :create
         {
           add: "deploy_key",
+          author_name: author_name,
           target_id: key_title,
           target_type: "DeployKey",
           target_details: key_title,

app/views/audit_events/_event_table.html.haml

@@ -10,7 +10,11 @@
     %tbody
       - events.each do |event|
         %tr
-          %td #{event.author_name}
+          %td
+            - if event.author_name
+              #{event.author_name}
+            - else
+              (removed)
           %td
             %span
               #{raw human_text(event.details)}

spec/helpers/audit_events_helper_spec.rb

+require 'spec_helper'
+
+describe AuditEventsHelper do
+  describe '#human_text' do
+    let(:details) do
+      {
+        remove: 'user_access',
+        author_name: 'John Doe',
+        target_id: 1,
+        target_type: 'User',
+        target_details: 'Michael'
+      }
+    end
+
+    it 'ignores keys that start with start with author_, or target_' do
+      expect(human_text(details)).to eq 'Remove <strong>user access</strong>    '
+    end
+  end
+
+  describe '#select_keys' do
+    it 'returns empty string if key starts with author_' do
+      expect(select_keys('author_name', 'John Doe')).to eq ''
+    end
+
+    it 'returns empty string if key starts with target_' do
+      expect(select_keys('target_name', 'John Doe')).to eq ''
+    end
+
+    it 'returns formatted text if key does not start with author_, or target_' do
+      expect(select_keys('remove', 'user_access')).to eq 'remove <strong>user_access</strong>'
+    end
+  end
+end

spec/models/audit_event_spec.rb

+require 'rails_helper'
+
+RSpec.describe AuditEvent, type: :model do
+  describe 'relationships' do
+    it { is_expected.to belong_to(:user).with_foreign_key('author_id') }
+  end
+
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:author_id) }
+    it { is_expected.to validate_presence_of(:entity_id) }
+    it { is_expected.to validate_presence_of(:entity_type) }
+  end
+
+  describe '#author_name' do
+    context 'when user exists' do
+      let(:user) { create(:user, name: 'John Doe') }
+
+      subject(:event) { described_class.new(user: user) }
+
+      it 'returns user name' do
+        expect(event.author_name).to eq 'John Doe'
+      end
+    end
+
+    context 'when user does not exists anymore' do
+      subject(:event) { described_class.new(author_id: 99999) }
+
+      context 'when details contains author_name' do
+        it 'returns author_name' do
+          subject.details = { author_name: 'John Doe' }
+
+          expect(event.author_name).to eq 'John Doe'
+        end
+      end
+
+      context 'when details does not contains author_name' do
+        it 'returns nil' do
+          subject.details = {}
+
+          expect(subject.author_name).to eq nil
+        end
+      end
+    end
+  end
+end