Fix admin action logs page (#19649)

* Add tests

* Fix crash when trying to display orphaned action logs

* Add migration for older admin action logs

app/helpers/admin/action_logs_helper.rb

@@ -4,15 +4,19 @@ module Admin::ActionLogsHelper
   def log_target(log)
     case log.target_type
     when 'Account'
-      link_to log.human_identifier, admin_account_path(log.target_id)
+      link_to (log.human_identifier.presence || I18n.t('admin.action_logs.deleted_account')), admin_account_path(log.target_id)
     when 'User'
+      if log.route_param.present?
         link_to log.human_identifier, admin_account_path(log.route_param)
+      else
+        I18n.t('admin.action_logs.deleted_account')
+      end
     when 'UserRole'
       link_to log.human_identifier, admin_roles_path(log.target_id)
     when 'Report'
-      link_to "##{log.human_identifier}", admin_report_path(log.target_id)
+      link_to "##{log.human_identifier.presence || log.target_id}", admin_report_path(log.target_id)
     when 'DomainBlock', 'DomainAllow', 'EmailDomainBlock', 'UnavailableDomain'
-      link_to log.human_identifier, "https://#{log.human_identifier}"
+      link_to log.human_identifier, "https://#{log.human_identifier.presence}"
     when 'Status'
       link_to log.human_identifier, log.permalink
     when 'AccountWarning'
@@ -22,9 +26,13 @@ module Admin::ActionLogsHelper
     when 'IpBlock', 'Instance', 'CustomEmoji'
       log.human_identifier
     when 'CanonicalEmailBlock'
-      content_tag(:samp, log.human_identifier[0...7], title: log.human_identifier)
+      content_tag(:samp, (log.human_identifier.presence || '')[0...7], title: log.human_identifier)
     when 'Appeal'
-      link_to log.human_identifier, disputes_strike_path(log.route_param)
+      if log.route_param.present?
+        link_to log.human_identifier, disputes_strike_path(log.route_param.presence)
+      else
+        I18n.t('admin.action_logs.deleted_account')
+      end
     end
   end
 end

db/post_migrate/20221101190723_backfill_admin_action_logs.rb

@@ -0,0 +1,150 @@
+# frozen_string_literal: true
+
+class BackfillAdminActionLogs < ActiveRecord::Migration[6.1]
+  disable_ddl_transaction!
+
+  class Account < ApplicationRecord
+    # Dummy class, to make migration possible across version changes
+    has_one :user, inverse_of: :account
+
+    def local?
+      domain.nil?
+    end
+
+    def acct
+      local? ? username : "#{username}@#{domain}"
+    end
+  end
+
+  class User < ApplicationRecord
+    # Dummy class, to make migration possible across version changes
+    belongs_to :account
+  end
+
+  class Status < ApplicationRecord
+    include RoutingHelper
+
+    # Dummy class, to make migration possible across version changes
+    belongs_to :account
+
+    def local?
+      attributes['local'] || attributes['uri'].nil?
+    end
+
+    def uri
+      local? ? activity_account_status_url(account, self) : attributes['uri']
+    end
+  end
+
+  class DomainBlock < ApplicationRecord; end
+  class DomainAllow < ApplicationRecord; end
+  class EmailDomainBlock < ApplicationRecord; end
+  class UnavailableDomain < ApplicationRecord; end
+
+  class AccountWarning < ApplicationRecord
+    # Dummy class, to make migration possible across version changes
+    belongs_to :account
+  end
+
+  class Announcement < ApplicationRecord; end
+  class IpBlock < ApplicationRecord; end
+  class CustomEmoji < ApplicationRecord; end
+  class CanonicalEmailBlock < ApplicationRecord; end
+
+  class Appeal < ApplicationRecord
+    # Dummy class, to make migration possible across version changes
+    belongs_to :account
+  end
+
+  class AdminActionLog < ApplicationRecord
+    # Dummy class, to make migration possible across version changes
+
+    # Cannot use usual polymorphic support because of namespacing issues
+    belongs_to :status, foreign_key: :target_id
+    belongs_to :account, foreign_key: :target_id
+    belongs_to :user, foreign_key: :user_id
+    belongs_to :domain_block, foreign_key: :target_id
+    belongs_to :domain_allow, foreign_key: :target_id
+    belongs_to :email_domain_block, foreign_key: :target_id
+    belongs_to :unavailable_domain, foreign_key: :target_id
+    belongs_to :account_warning, foreign_key: :target_id
+    belongs_to :announcement, foreign_key: :target_id
+    belongs_to :ip_block, foreign_key: :target_id
+    belongs_to :custom_emoji, foreign_key: :target_id
+    belongs_to :canonical_email_block, foreign_key: :target_id
+    belongs_to :appeal, foreign_key: :target_id
+  end
+
+  def up
+    safety_assured do
+      AdminActionLog.includes(:account).where(target_type: 'Account', human_identifier: nil).find_each do |log|
+        next if log.account.nil?
+        log.update(human_identifier: log.account.acct)
+      end
+
+      AdminActionLog.includes(user: :account).where(target_type: 'User', human_identifier: nil).find_each do |log|
+        next if log.user.nil?
+        log.update(human_identifier: log.user.account.acct, route_param: log.user.account_id)
+      end
+
+      Admin::ActionLog.where(target_type: 'Report', human_identifier: nil).in_batches.update_all('human_identifier = target_id::text')
+
+      AdminActionLog.includes(:domain_block).where(target_type: 'DomainBlock').find_each do |log|
+        next if log.domain_block.nil?
+        log.update(human_identifier: log.domain_block.domain)
+      end
+
+      AdminActionLog.includes(:domain_allow).where(target_type: 'DomainAllow').find_each do |log|
+        next if log.domain_allow.nil?
+        log.update(human_identifier: log.domain_allow.domain)
+      end
+
+      AdminActionLog.includes(:email_domain_block).where(target_type: 'EmailDomainBlock').find_each do |log|
+        next if log.email_domain_block.nil?
+        log.update(human_identifier: log.email_domain_block.domain)
+      end
+
+      AdminActionLog.includes(:unavailable_domain).where(target_type: 'UnavailableDomain').find_each do |log|
+        next if log.unavailable_domain.nil?
+        log.update(human_identifier: log.unavailable_domain.domain)
+      end
+
+      AdminActionLog.includes(status: :account).where(target_type: 'Status', human_identifier: nil).find_each do |log|
+        next if log.status.nil?
+        log.update(human_identifier: log.status.account.acct, permalink: log.status.uri)
+      end
+
+      AdminActionLog.includes(account_warning: :account).where(target_type: 'AccountWarning', human_identifier: nil).find_each do |log|
+        next if log.account_warning.nil?
+        log.update(human_identifier: log.account_warning.account.acct)
+      end
+
+      AdminActionLog.includes(:announcement).where(target_type: 'Announcement', human_identifier: nil).find_each do |log|
+        next if log.announcement.nil?
+        log.update(human_identifier: log.announcement.text)
+      end
+
+      AdminActionLog.includes(:ip_block).where(target_type: 'IpBlock', human_identifier: nil).find_each do |log|
+        next if log.ip_block.nil?
+        log.update(human_identifier: "#{log.ip_block.ip}/#{log.ip_block.ip.prefix}")
+      end
+
+      AdminActionLog.includes(:custom_emoji).where(target_type: 'CustomEmoji', human_identifier: nil).find_each do |log|
+        next if log.custom_emoji.nil?
+        log.update(human_identifier: log.custom_emoji.shortcode)
+      end
+
+      AdminActionLog.includes(:canonical_email_block).where(target_type: 'CanonicalEmailBlock', human_identifier: nil).find_each do |log|
+        next if log.canonical_email_block.nil?
+        log.update(human_identifier: log.canonical_email_block.canonical_email_hash)
+      end
+
+      AdminActionLog.includes(appeal: :account).where(target_type: 'Appeal', human_identifier: nil).find_each do |log|
+        next if log.appeal.nil?
+        log.update(human_identifier: log.appeal.account.acct, route_param: log.appeal.account_warning_id)
+      end
+    end
+  end
+
+  def down; end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2022_10_25_171544) do
+ActiveRecord::Schema.define(version: 2022_11_01_190723) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"

lib/tasks/tests.rake

@@ -53,6 +53,41 @@ namespace :tests do
         VALUES
           (1, 2, 'test', '{ "home", "public" }', true, true, now(), now()),
           (2, 2, 'take', '{ "home" }', false, false, now(), now());
+
+        -- Orphaned admin action logs
+
+        INSERT INTO "admin_action_logs"
+          (account_id, action, target_type, target_id, created_at, updated_at)
+        VALUES
+          (1, 'destroy', 'Account', 1312, now(), now()),
+          (1, 'destroy', 'User', 1312, now(), now()),
+          (1, 'destroy', 'Report', 1312, now(), now()),
+          (1, 'destroy', 'DomainBlock', 1312, now(), now()),
+          (1, 'destroy', 'EmailDomainBlock', 1312, now(), now()),
+          (1, 'destroy', 'Status', 1312, now(), now()),
+          (1, 'destroy', 'CustomEmoji', 1312, now(), now());
+
+        -- Admin action logs with linked objects
+
+        INSERT INTO "domain_blocks"
+          (id, domain, created_at, updated_at)
+        VALUES
+          (1, 'example.org', now(), now());
+
+        INSERT INTO "email_domain_blocks"
+          (id, domain, created_at, updated_at)
+        VALUES
+          (1, 'example.org', now(), now());
+
+        INSERT INTO "admin_action_logs"
+          (account_id, action, target_type, target_id, created_at, updated_at)
+        VALUES
+          (1, 'destroy', 'Account', 1, now(), now()),
+          (1, 'destroy', 'User', 1, now(), now()),
+          (1, 'destroy', 'DomainBlock', 1312, now(), now()),
+          (1, 'destroy', 'EmailDomainBlock', 1312, now(), now()),
+          (1, 'destroy', 'Status', 1, now(), now()),
+          (1, 'destroy', 'CustomEmoji', 3, now(), now());
       SQL
     end
 
@@ -207,18 +242,18 @@ namespace :tests do
         -- custom emoji
 
         INSERT INTO "custom_emojis"
-          (shortcode, created_at, updated_at)
+          (id, shortcode, created_at, updated_at)
         VALUES
-          ('test', now(), now()),
-          ('Test', now(), now()),
-          ('blobcat', now(), now());
+          (1, 'test', now(), now()),
+          (2, 'Test', now(), now()),
+          (3, 'blobcat', now(), now());
 
         INSERT INTO "custom_emojis"
-          (shortcode, domain, uri, created_at, updated_at)
+          (id, shortcode, domain, uri, created_at, updated_at)
         VALUES
-          ('blobcat', 'remote.org', 'https://remote.org/emoji/blobcat', now(), now()),
-          ('blobcat', 'Remote.org', 'https://remote.org/emoji/blobcat', now(), now()),
-          ('Blobcat', 'remote.org', 'https://remote.org/emoji/Blobcat', now(), now());
+          (4, 'blobcat', 'remote.org', 'https://remote.org/emoji/blobcat', now(), now()),
+          (5, 'blobcat', 'Remote.org', 'https://remote.org/emoji/blobcat', now(), now()),
+          (6, 'Blobcat', 'remote.org', 'https://remote.org/emoji/Blobcat', now(), now());
 
         -- favourites
 

spec/controllers/admin/action_logs_controller_spec.rb

@@ -3,6 +3,19 @@
 require 'rails_helper'
 
 describe Admin::ActionLogsController, type: :controller do
+  render_views
+
+  # Action logs typically cause issues when their targets are not in the database
+  let!(:account) { Fabricate(:account) }
+
+  let!(:orphaned_logs) do
+    %w(
+      Account User UserRole Report DomainBlock DomainAllow
+      EmailDomainBlock UnavailableDomain Status AccountWarning
+      Announcement IpBlock Instance CustomEmoji CanonicalEmailBlock Appeal
+    ).map { |type| Admin::ActionLog.new(account: account, action: 'destroy', target_type: type, target_id: 1312).save! }
+  end
+
   describe 'GET #index' do
     it 'returns 200' do
       sign_in Fabricate(:user, role: UserRole.find_by(name: 'Admin'))