From 6bdcd7afb2e05dd76a88aae5e7816ec7c46357b2 Mon Sep 17 00:00:00 2001
From: Roman Rizzi <rizziromanalejandro@gmail.com>
Date: Mon, 9 May 2022 12:12:52 -0300
Subject: [PATCH] FEATURE: Promote the "delete group" staff action log.
 (#16656)

We used to log group deletion as custom, which means we can't later search for them. Include group ID in the details.
---
 app/controllers/admin/groups_controller.rb    |  4 +---
 app/models/user_history.rb                    |  6 ++++--
 app/services/staff_action_logger.rb           | 19 +++++++++++++++++++
 ...3851_migrate_custom_group_deletion_logs.rb | 14 ++++++++++++++
 spec/requests/admin/groups_controller_spec.rb |  1 +
 5 files changed, 39 insertions(+), 5 deletions(-)
 create mode 100644 db/migrate/20220505133851_migrate_custom_group_deletion_logs.rb

diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index 33944246e8e..61b09fffb2d 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -45,10 +45,8 @@ class Admin::GroupsController < Admin::AdminController
     if group.automatic
       can_not_modify_automatic
     else
-      details = { name: group.name }
-      details[:grant_trust_level] = group.grant_trust_level if group.grant_trust_level
+      StaffActionLogger.new(current_user).log_group_deletetion(group)
 
-      StaffActionLogger.new(current_user).log_custom('delete_group', details)
       group.destroy!
       render json: success_json
     end
diff --git a/app/models/user_history.rb b/app/models/user_history.rb
index 5f8de77f64b..1ddde529e94 100644
--- a/app/models/user_history.rb
+++ b/app/models/user_history.rb
@@ -116,7 +116,8 @@ class UserHistory < ActiveRecord::Base
       post_staff_note_create: 95,
       post_staff_note_destroy: 96,
       watched_word_create: 97,
-      watched_word_destroy: 98
+      watched_word_destroy: 98,
+      delete_group: 99
     )
   end
 
@@ -209,7 +210,8 @@ class UserHistory < ActiveRecord::Base
       :post_staff_note_create,
       :post_staff_note_destroy,
       :watched_word_create,
-      :watched_word_destroy
+      :watched_word_destroy,
+      :delete_group
     ]
   end
 
diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb
index 823a2b6a8a3..5fc643b9448 100644
--- a/app/services/staff_action_logger.rb
+++ b/app/services/staff_action_logger.rb
@@ -831,6 +831,25 @@ class StaffActionLogger
     )
   end
 
+  def log_group_deletetion(group)
+    raise Discourse::InvalidParameters.new(:group) if group.nil?
+
+    details = [
+      "name: #{group.name}",
+      "id: #{group.id}"
+    ]
+
+    if group.grant_trust_level
+      details << "grant_trust_level: #{group.grant_trust_level}"
+    end
+
+    UserHistory.create!(
+      acting_user_id: @admin.id,
+      action: UserHistory.actions[:delete_group],
+      details: details.join(', ')
+    )
+  end
+
   private
 
   def get_changes(changes)
diff --git a/db/migrate/20220505133851_migrate_custom_group_deletion_logs.rb b/db/migrate/20220505133851_migrate_custom_group_deletion_logs.rb
new file mode 100644
index 00000000000..91d4d9c5da8
--- /dev/null
+++ b/db/migrate/20220505133851_migrate_custom_group_deletion_logs.rb
@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+class MigrateCustomGroupDeletionLogs < ActiveRecord::Migration[7.0]
+  def up
+    DB.exec(<<~SQL, group_deleted_id: 99, custom_action_id: 23)
+      UPDATE user_histories
+      SET action = :group_deleted_id, custom_type = NULL
+      WHERE action = :custom_action_id AND custom_type = 'delete_group'
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/spec/requests/admin/groups_controller_spec.rb b/spec/requests/admin/groups_controller_spec.rb
index ddb0be9c1f9..fbfa72eb553 100644
--- a/spec/requests/admin/groups_controller_spec.rb
+++ b/spec/requests/admin/groups_controller_spec.rb
@@ -274,6 +274,7 @@ RSpec.describe Admin::GroupsController do
 
       expect(history).to be_present
       expect(history.details).to include("name: #{group.name}")
+      expect(history.details).to include("id: #{group.id}")
     end
 
     it 'logs the grant_trust_level attribute' do