Merge branch 'admin-audit-log-table-redesign' into 'master'

Admin audit log table redesign

Closes #216126

See merge request gitlab-org/gitlab!27578

ee/app/assets/javascripts/audit_logs/components/logs_table.vue

+<script>
+import { GlPagination, GlTable } from '@gitlab/ui';
+import { s__ } from '~/locale';
+import { getParameterValues, setUrlParams } from '~/lib/utils/url_utility';
+import UrlTableCell from './url_table_cell.vue';
+
+const TABLE_HEADER_CLASSES = 'bg-transparent border-bottom p-3';
+
+export default {
+  name: 'LogsTable',
+  components: {
+    GlTable,
+    GlPagination,
+    UrlTableCell,
+  },
+  props: {
+    events: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
+    isLastPage: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+  },
+  data() {
+    return {
+      page: parseInt(getParameterValues('page')[0], 10) || 1,
+    };
+  },
+  computed: {
+    displayPagination() {
+      return this.events.length > 0;
+    },
+    prevPage() {
+      return this.page > 1 ? this.page - 1 : null;
+    },
+    nextPage() {
+      return !this.isLastPage ? this.page + 1 : null;
+    },
+  },
+  methods: {
+    generateLink(page) {
+      return setUrlParams({ page });
+    },
+  },
+  fields: [
+    {
+      key: 'author',
+      label: s__('AuditLogs|Author'),
+      thClass: TABLE_HEADER_CLASSES,
+    },
+    {
+      key: 'object',
+      label: s__('AuditLogs|Object'),
+      thClass: TABLE_HEADER_CLASSES,
+    },
+    {
+      key: 'action',
+      label: s__('AuditLogs|Action'),
+      thClass: TABLE_HEADER_CLASSES,
+    },
+    {
+      key: 'target',
+      label: s__('AuditLogs|Target'),
+      thClass: TABLE_HEADER_CLASSES,
+    },
+    {
+      key: 'ip_address',
+      label: s__('AuditLogs|IP Address'),
+      thClass: TABLE_HEADER_CLASSES,
+    },
+    {
+      key: 'date',
+      label: s__('AuditLogs|Date'),
+      thClass: TABLE_HEADER_CLASSES,
+    },
+  ],
+};
+</script>
+
+<template>
+  <div class="audit-log-table" data-qa-selector="admin_audit_log_table">
+    <gl-table class="mt-3" :fields="$options.fields" :items="events" show-empty>
+      <template #cell(author)="{ value: { url, name } }">
+        <url-table-cell :url="url" :name="name" />
+      </template>
+      <template #cell(object)="{ value: { url, name } }">
+        <url-table-cell :url="url" :name="name" />
+      </template>
+    </gl-table>
+    <gl-pagination
+      v-if="displayPagination"
+      v-model="page"
+      :prev-page="prevPage"
+      :next-page="nextPage"
+      :link-gen="generateLink"
+      align="center"
+      class="w-100"
+    />
+  </div>
+</template>

ee/app/assets/javascripts/audit_logs/components/url_table_cell.vue

+<script>
+import { s__ } from '~/locale';
+
+const REMOVED_TEXT = s__('AuditLogs|(removed)');
+
+export default {
+  props: {
+    url: {
+      type: String,
+      required: false,
+      default: '',
+    },
+    name: {
+      type: String,
+      required: true,
+    },
+  },
+  REMOVED_TEXT,
+};
+</script>
+
+<template>
+  <a v-if="url" :href="url">{{ name }}</a>
+
+  <span v-else>
+    {{ name }} <small>{{ $options.REMOVED_TEXT }}</small>
+  </span>
+</template>

ee/app/assets/javascripts/pages/admin/audit_logs/index.js

 import Vue from 'vue';
 
-import DateRangeField from './components/date_range_field.vue';
+import { parseBoolean } from '~/lib/utils/common_utils';
+
+import DateRangeField from 'ee/audit_logs/components/date_range_field.vue';
+import LogsTable from 'ee/audit_logs/components/logs_table.vue';
+
 import AuditLogs from './audit_logs';
 
+// Merge these when working on https://gitlab.com/gitlab-org/gitlab/-/issues/215363
 document.addEventListener('DOMContentLoaded', () => new AuditLogs());
 document.addEventListener('DOMContentLoaded', () => {
   const el = document.querySelector('#js-audit-logs-date-range-app');
@@ -21,3 +26,19 @@ document.addEventListener('DOMContentLoaded', () => {
       }),
   });
 });
+document.addEventListener('DOMContentLoaded', () => {
+  const el = document.querySelector('#js-audit-logs-table-app');
+
+  // eslint-disable-next-line no-new
+  new Vue({
+    el,
+    name: 'AuditLogsTableApp',
+    render: createElement =>
+      createElement(LogsTable, {
+        props: {
+          events: JSON.parse(el.dataset.events),
+          isLastPage: parseBoolean(el.dataset.isLastPage),
+        },
+      }),
+  });
+});

ee/app/assets/stylesheets/components/audit_logs/logs_table.scss

+// TODO: Remove this once https://gitlab.com/gitlab-org/gitlab/-/issues/213324 is resolved
+.audit-log-table .gl-table {
+  th {
+    font-weight: 600;
+    line-height: 1rem;
+  }
+
+  th,
+  td {
+    @include gl-text-gray-700;
+  }
+}

ee/app/controllers/admin/audit_logs_controller.rb

@@ -11,6 +11,7 @@ class Admin::AuditLogsController < Admin::ApplicationController
 
   def index
     @events = audit_log_events
+    @table_events = AuditEventSerializer.new.represent(@events)
 
     @entity = case audit_logs_params[:entity_type]
               when 'User'

ee/app/presenters/audit_event_presenter.rb

@@ -4,11 +4,13 @@ class AuditEventPresenter < Gitlab::View::Presenter::Simple
   presents :audit_event
 
   def author_name
-    author = audit_event.lazy_author
+    author&.name
+  end
 
-    return author.name if author.is_a?(Gitlab::Audit::NullAuthor)
+  def author_url
+    return if author.is_a?(Gitlab::Audit::NullAuthor)
 
-    link_to(author.name, user_path(author))
+    url_for(user_path(author))
   end
 
   def target
@@ -24,11 +26,15 @@ class AuditEventPresenter < Gitlab::View::Presenter::Simple
   end
 
   def object
-    entity = audit_event.lazy_entity
+    return if entity.is_a?(Gitlab::Audit::NullEntity)
+
+    details[:entity_path] || entity.name
+  end
 
+  def object_url
     return if entity.is_a?(Gitlab::Audit::NullEntity)
 
-    link_to(details[:entity_path] || entity.name, entity).html_safe
+    url_for(entity)
   end
 
   def date
@@ -41,11 +47,11 @@ class AuditEventPresenter < Gitlab::View::Presenter::Simple
 
   private
 
-  # The class can't include ActionView::Helpers::UrlHelper because it overwrites
-  # the method url_for. In this helper, that implementation of that method
-  # doesn't accept objects to resolve their route. That's why here we call the
-  # native url_for to get the route of the object and then call the link_to with it
-  def link_to(name, object)
-    ActionController::Base.helpers.link_to(name, url_for(object))
+  def author
+    @author ||= audit_event.lazy_author
+  end
+
+  def entity
+    @entity ||= audit_event.lazy_entity
   end
 end

ee/app/serializers/audit_event_entity.rb

+# frozen_string_literal: true
+
+class AuditEventEntity < Grape::Entity
+  alias_method :audit_event, :object
+
+  expose :id
+  expose :author do |_|
+    {
+      name: presenter.author_name,
+      url: presenter.author_url
+    }
+  end
+
+  expose :action do |_|
+    presenter.action
+  end
+
+  expose :date do |_|
+    presenter.date
+  end
+
+  expose :ip_address do |_|
+    presenter.ip_address
+  end
+
+  expose :object do |_|
+    {
+      name: presenter.object,
+      url: presenter.object_url
+    }
+  end
+
+  expose :target do |_|
+    presenter.target
+  end
+
+  private
+
+  def presenter
+    @presenter ||= audit_event.present
+  end
+end

ee/app/serializers/audit_event_serializer.rb

+# frozen_string_literal: true
+class AuditEventSerializer < BaseSerializer
+  entity AuditEventEntity
+end

ee/app/views/admin/audit_logs/index.html.haml

@@ -49,31 +49,5 @@
             #js-audit-logs-date-range-app
             = render 'shared/audit_events/event_sort'
 
-- if @events.present?
-  %table#events-table.table
-    %thead
-      %tr
-        %th Author
-        %th Object
-        %th Action
-        %th Target
-        %th IP Address
-        %th Date
-    %tbody
-      - @events.map(&:present).each do |event|
-        %tr{ data: { qa_selector: 'admin_audit_log_row_content' } }
-          %td
-            - if (author_link = event.author_name)
-              = author_link
-            - else
-              #{event.details[:author_name]} <small>(removed)</small>
-          %td
-            - if (object_link = event.object)
-              = object_link
-            - else
-              #{event.details[:entity_path]} <small>(removed)</small>
-          %td= sanitize(event.action, tags: %w(strong))
-          %td= event.target
-          %td= event.ip_address
-          %td= event.date
-  = paginate_without_count @events
+- if @table_events.present?
+  #js-audit-logs-table-app{ data: { events: @table_events.to_json, is_last_page: @events.last_page?.to_json, qa_selector: 'admin_audit_log_table' } }

ee/app/views/shared/audit_events/_event_table.html.haml

@@ -16,7 +16,7 @@
             = _('Author')
           .table-mobile-content
             - if event.author_name
-              = event.author_name
+              %a{ href: event.author_url }= event.author_name
             - else
               = s_('AuditEvents|(removed)')
         .table-section.section-50.section-wrap.audit-action.js-audit-action

ee/spec/features/admin/admin_audit_logs_spec.rb

@@ -92,7 +92,7 @@ describe 'Admin::AuditLogs', :js do
         wait_for_requests
         find('.select2-results').click
 
-        find('#events-table td', match: :first)
+        find('.audit-log-table td', match: :first)
 
         expect(page).to have_content('Added user access as Owner')
       end
@@ -116,13 +116,13 @@ describe 'Admin::AuditLogs', :js do
         wait_for_requests
         find('.select2-results').click
 
-        find('#events-table td', match: :first)
+        find('.audit-log-table td', match: :first)
 
         expect(page).to have_content('Removed user access')
       end
     end
 
-    describe 'filter by date', js: false do
+    describe 'filter by date' do
       let_it_be(:audit_event_1) { create(:user_audit_event, created_at: 5.days.ago) }
       let_it_be(:audit_event_2) { create(:user_audit_event, created_at: 3.days.ago) }
       let_it_be(:audit_event_3) { create(:user_audit_event, created_at: 1.day.ago) }
@@ -130,17 +130,21 @@ describe 'Admin::AuditLogs', :js do
       it 'shows only 2 days old events' do
         visit admin_audit_logs_path(created_after: 4.days.ago.to_date, created_before: 2.days.ago.to_date)
 
-        expect(page).to have_content(audit_event_2.present.date)
+        find('.audit-log-table td', match: :first)
+
         expect(page).not_to have_content(audit_event_1.present.date)
+        expect(page).to have_content(audit_event_2.present.date)
         expect(page).not_to have_content(audit_event_3.present.date)
       end
 
       it 'shows only yesterday events' do
         visit admin_audit_logs_path(created_after: 2.days.ago.to_date)
 
-        expect(page).to have_content(audit_event_3.present.date)
+        find('.audit-log-table td', match: :first)
+
         expect(page).not_to have_content(audit_event_1.present.date)
         expect(page).not_to have_content(audit_event_2.present.date)
+        expect(page).to have_content(audit_event_3.present.date)
       end
 
       it 'shows a message if provided date is invalid' do

ee/spec/frontend/pages/admin/audit_logs/components/date_range_field_spec.js → ee/spec/frontend/audit_logs/components/date_range_field_spec.js

 import { shallowMount } from '@vue/test-utils';
 import { GlDaterangePicker } from '@gitlab/ui';
 
-import DateRangeField from 'ee/pages/admin/audit_logs/components/date_range_field.vue';
+import DateRangeField from 'ee/audit_logs/components/date_range_field.vue';
 import { parsePikadayDate } from '~/lib/utils/datetime_utility';
 
 describe('DateRangeField component', () => {

ee/spec/frontend/audit_logs/components/logs_table_spec.js

+import { mount } from '@vue/test-utils';
+import { GlPagination, GlTable } from '@gitlab/ui';
+
+import LogsTable from 'ee/audit_logs/components/logs_table.vue';
+import createEvents from '../mock_data';
+
+const EVENTS = createEvents();
+
+describe('LogsTable component', () => {
+  let wrapper;
+
+  const createComponent = (props = {}) => {
+    return mount(LogsTable, {
+      propsData: {
+        events: EVENTS,
+        isLastPage: false,
+        ...props,
+      },
+    });
+  };
+
+  const getCell = (trIdx, tdIdx) => {
+    return wrapper
+      .find(GlTable)
+      .find('tbody')
+      .findAll('tr')
+      .at(trIdx)
+      .findAll('td')
+      .at(tdIdx);
+  };
+
+  beforeEach(() => {
+    delete window.location;
+    window.location = new URL('https://localhost');
+
+    wrapper = createComponent();
+  });
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  describe('Table behaviour', () => {
+    it('should show', () => {
+      expect(getCell(0, 1).text()).toBe('User');
+    });
+
+    it('should show the empty state if there is no data', () => {
+      wrapper.setProps({ events: [] });
+      wrapper.vm.$nextTick(() => {
+        expect(getCell(0, 0).text()).toBe('There are no records to show');
+      });
+    });
+  });
+
+  describe('Pagination behaviour', () => {
+    it('should show', () => {
+      expect(wrapper.find(GlPagination).exists()).toBe(true);
+    });
+
+    it('should hide if there is no data', () => {
+      wrapper.setProps({ events: [] });
+      wrapper.vm.$nextTick(() => {
+        expect(wrapper.find(GlPagination).exists()).toBe(false);
+      });
+    });
+
+    it('should get the page number from the URL', () => {
+      window.location.search = '?page=2';
+      wrapper = createComponent();
+
+      expect(wrapper.find(GlPagination).props().value).toBe(2);
+    });
+
+    it('should not have a prevPage if the page is 1', () => {
+      window.location.search = '?page=1';
+      wrapper = createComponent();
+
+      expect(wrapper.find(GlPagination).props().prevPage).toBe(null);
+    });
+
+    it('should set the prevPage to 1 if the page is 2', () => {
+      window.location.search = '?page=2';
+      wrapper = createComponent();
+
+      expect(wrapper.find(GlPagination).props().prevPage).toBe(1);
+    });
+
+    it('should not have a nextPage if isLastPage is true', () => {
+      wrapper.setProps({ isLastPage: true });
+      wrapper.vm.$nextTick(() => {
+        expect(wrapper.find(GlPagination).props().nextPage).toBe(null);
+      });
+    });
+
+    it('should set the nextPage to 2 if the page is 1', () => {
+      window.location.search = '?page=1';
+      wrapper = createComponent();
+
+      expect(wrapper.find(GlPagination).props().nextPage).toBe(2);
+    });
+
+    it('should set the nextPage to 2 if the page is not set', () => {
+      expect(wrapper.find(GlPagination).props().nextPage).toBe(2);
+    });
+  });
+});

ee/spec/frontend/audit_logs/components/url_table_cell_spec.js

+import { shallowMount } from '@vue/test-utils';
+
+import UrlTableCell from 'ee/audit_logs/components/url_table_cell.vue';
+
+describe('UrlTableCell component', () => {
+  it('should show the link if the URL is provided', () => {
+    const wrapper = shallowMount(UrlTableCell, { propsData: { url: '/user-1', name: 'User 1' } });
+    const name = wrapper.find('a');
+
+    expect(name.exists()).toBe(true);
+    expect(name.attributes().href).toBe('/user-1');
+    expect(name.text()).toBe('User 1');
+  });
+
+  it('should show the removed text if no URL is provided', () => {
+    const wrapper = shallowMount(UrlTableCell, { propsData: { url: '', name: 'User 1' } });
+    const name = wrapper.find('span');
+
+    expect(name.exists()).toBe(true);
+    expect(name.text()).toBe('User 1 (removed)');
+  });
+});

ee/spec/frontend/audit_logs/mock_data.js

+import { slugify } from '~/lib/utils/text_utility';
+
+const DEFAULT_EVENT = {
+  action: 'Signed in with STANDARD authentication',
+  date: '2020-03-18 12:04:23',
+  ip_address: '127.0.0.1',
+};
+
+const populateEvent = (user, hasAuthorUrl = true, hasObjectUrl = true) => {
+  const author = { name: user, url: null };
+  const object = { name: user, url: null };
+  const userSlug = slugify(user);
+
+  if (hasAuthorUrl) {
+    author.url = `/${userSlug}`;
+  }
+
+  if (hasObjectUrl) {
+    object.url = `http://127.0.0.1:3000/${userSlug}`;
+  }
+
+  return {
+    ...DEFAULT_EVENT,
+    author,
+    object,
+    target: user,
+  };
+};
+
+export default () => [
+  populateEvent('User'),
+  populateEvent('User 2', false),
+  populateEvent('User 3', true, false),
+  populateEvent('User 4', false, false),
+];

ee/spec/presenters/audit_event_presenter_spec.rb

@@ -23,9 +23,13 @@ describe AuditEventPresenter do
   end
 
   context 'exposes the author' do
+    it 'gets the event author name' do
+      expect(presenter.author_name).to eq(audit_event.user.name)
+    end
+
     context 'event authored by a user that exists' do
-      it 'shows a link' do
-        expect(presenter.author_name).to eq("<a href=\"#{user_path(audit_event.user)}\">#{audit_event.user.name}</a>")
+      it 'returns a url' do
+        expect(presenter.author_url).to eq(url_for(user_path(audit_event.user)))
       end
     end
 
@@ -50,8 +54,8 @@ describe AuditEventPresenter do
           }
         end
 
-        it 'shows a blank author name' do
-          expect(presenter.author_name).to be_blank
+        it 'does not return a url' do
+          expect(presenter.author_url).to be_blank
         end
       end
 
@@ -101,15 +105,25 @@ describe AuditEventPresenter do
   end
 
   context 'exposes the object' do
-    it 'link if it exists' do
-      expect(presenter.object).to eq("<a href=\"#{url_for(audit_event.entity)}\">#{details[:entity_path]}</a>")
+    it 'returns the object path if it exists' do
+      expect(presenter.object).to eq(details[:entity_path])
     end
 
-    it 'stored name if it has been deleted' do
+    it 'returns the stored name if it has been deleted' do
       audit_event.entity_id = nil
 
       expect(presenter.object).to be_blank
     end
+
+    it 'returns the object url if it exists' do
+      expect(presenter.object_url).to eq(url_for(audit_event.entity))
+    end
+
+    it 'returns no object url if it has been deleted' do
+      audit_event.entity_id = nil
+
+      expect(presenter.object_url).to be_blank
+    end
   end
 
   it 'exposes the date' do

ee/spec/serializers/audit_event_entity_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe AuditEventEntity do
+  let(:event) { create(:audit_event) }
+
+  subject { described_class.new(event) }
+
+  describe '.as_json' do
+    it 'includes audit event attributes' do
+      expect(subject.as_json.keys.sort).to eq([:action, :author, :date, :id, :ip_address, :object, :target])
+    end
+  end
+
+  describe '@presenter' do
+    it 'is only set once' do
+      expect(AuditEventPresenter).to receive(:new)
+                                         .with(event)
+                                         .and_call_original
+                                         .once
+      subject.as_json
+    end
+  end
+end

ee/spec/serializers/audit_event_serializer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe AuditEventSerializer do
+  describe '.represent' do
+    it 'returns an empty array when there are no audit events' do
+      result = described_class.new.represent([])
+
+      expect(result).to eq([])
+    end
+
+    it 'includes audit event attributes' do
+      audit_event = create(:audit_event)
+      audit_events = [audit_event]
+
+      result = described_class.new.represent(audit_events)
+
+      expect(result.first.keys.sort).to eq([:action, :author, :date, :id, :ip_address, :object, :target])
+    end
+  end
+end

locale/gitlab.pot

@@ -2759,6 +2759,27 @@ msgstr ""
 msgid "AuditEvents|Target"
 msgstr ""
 
+msgid "AuditLogs|(removed)"
+msgstr ""
+
+msgid "AuditLogs|Action"
+msgstr ""
+
+msgid "AuditLogs|Author"
+msgstr ""
+
+msgid "AuditLogs|Date"
+msgstr ""
+
+msgid "AuditLogs|IP Address"
+msgstr ""
+
+msgid "AuditLogs|Object"
+msgstr ""
+
+msgid "AuditLogs|Target"
+msgstr ""
+
 msgid "Aug"
 msgstr ""
 

qa/qa/ee/page/admin/monitoring/audit_log.rb

@@ -7,16 +7,16 @@ module QA
         module Monitoring
           class AuditLog < QA::Page::Base
             view 'ee/app/views/admin/audit_logs/index.html.haml' do
-              element :admin_audit_log_row_content
+              element :admin_audit_log_table
             end
 
-            def has_audit_log_row?(text)
+            def has_audit_log_table_with_text?(text)
               # Sometimes the audit logs are not displayed in the UI
               # right away so a refresh may be needed.
               # https://gitlab.com/gitlab-org/gitlab/issues/119203
               # TODO: https://gitlab.com/gitlab-org/gitlab/issues/195424
               wait_until(reload: true) do
-                has_element?(:admin_audit_log_row_content, text: text)
+                has_element?(:admin_audit_log_table, text: text)
               end
             end
           end

qa/qa/specs/features/ee/browser_ui/1_manage/instance/instance_audit_logs_spec.rb

@@ -11,7 +11,7 @@ module QA
         QA::Page::Admin::Menu.perform(&:go_to_monitoring_audit_logs)
         EE::Page::Admin::Monitoring::AuditLog.perform do |audit_log_page|
           expected_events.each do |expected_event|
-            expect(audit_log_page).to have_audit_log_row(expected_event)
+            expect(audit_log_page).to have_audit_log_table_with_text(expected_event)
           end
         end
       end