From 32cb060652da3c2ee0a2407b8f46522211638726 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Sat, 25 May 2024 12:45:39 -0400 Subject: [PATCH 1/5] Log action after updating records --- app/models/admin/account_action.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/admin/account_action.rb b/app/models/admin/account_action.rb index 3700ce4cd6c..0a448b68a9a 100644 --- a/app/models/admin/account_action.rb +++ b/app/models/admin/account_action.rb @@ -124,26 +124,26 @@ class Admin::AccountAction def handle_disable! authorize(target_account.user, :disable?) - log_action(:disable, target_account.user) target_account.user&.disable! + log_action(:disable, target_account.user) end def handle_sensitive! authorize(target_account, :sensitive?) - log_action(:sensitive, target_account) target_account.sensitize! + log_action(:sensitive, target_account) end def handle_silence! authorize(target_account, :silence?) - log_action(:silence, target_account) target_account.silence! + log_action(:silence, target_account) end def handle_suspend! authorize(target_account, :suspend?) - log_action(:suspend, target_account) target_account.suspend!(origin: :local) + log_action(:suspend, target_account) end def text_for_warning From 71918091e69d69c27871c00ff084390f60bda07c Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Sat, 25 May 2024 12:46:32 -0400 Subject: [PATCH 2/5] Dont expect log when rejecting --- spec/controllers/admin/accounts_controller_spec.rb | 9 ++------- spec/requests/api/v1/admin/accounts_spec.rb | 12 ------------ 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/spec/controllers/admin/accounts_controller_spec.rb b/spec/controllers/admin/accounts_controller_spec.rb index f241d261b1b..bbd528a0a39 100644 --- a/spec/controllers/admin/accounts_controller_spec.rb +++ b/spec/controllers/admin/accounts_controller_spec.rb @@ -215,13 +215,8 @@ RSpec.describe Admin::AccountsController do it 'succeeds in rejecting account and logs action' do expect(subject).to redirect_to admin_accounts_path(status: 'pending') - expect(latest_admin_action_log) - .to be_present - .and have_attributes( - action: eq(:reject), - account_id: eq(current_user.account_id), - target_id: eq(account.user.id) - ) + expect { account.reload } + .to raise_error(ActiveRecord::RecordNotFound) end end diff --git a/spec/requests/api/v1/admin/accounts_spec.rb b/spec/requests/api/v1/admin/accounts_spec.rb index 1615581f0ea..0e7bd0dba13 100644 --- a/spec/requests/api/v1/admin/accounts_spec.rb +++ b/spec/requests/api/v1/admin/accounts_spec.rb @@ -199,18 +199,6 @@ RSpec.describe 'Accounts' do expect(response).to have_http_status(200) expect(User.where(id: account.user.id)).to_not exist end - - it 'logs action', :aggregate_failures do - subject - - expect(latest_admin_action_log) - .to be_present - .and have_attributes( - action: eq(:reject), - account_id: eq(user.account_id), - target_id: eq(account.user.id) - ) - end end context 'when account is already approved' do From 1a66aaaeb69806d9c6b75da8eb742eaac709e002 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Sat, 25 May 2024 12:47:38 -0400 Subject: [PATCH 3/5] Only log admin actions when change actually happened --- .../concerns/accountable_concern.rb | 2 ++ .../concerns/accountable_concern_spec.rb | 22 +++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/app/controllers/concerns/accountable_concern.rb b/app/controllers/concerns/accountable_concern.rb index c1349915f84..e111c213fcb 100644 --- a/app/controllers/concerns/accountable_concern.rb +++ b/app/controllers/concerns/accountable_concern.rb @@ -4,6 +4,8 @@ module AccountableConcern extend ActiveSupport::Concern def log_action(action, target) + return unless target.previous_changes.any? + Admin::ActionLog.create( account: current_account, action: action, diff --git a/spec/controllers/concerns/accountable_concern_spec.rb b/spec/controllers/concerns/accountable_concern_spec.rb index cd06d872bb5..95086d4336a 100644 --- a/spec/controllers/concerns/accountable_concern_spec.rb +++ b/spec/controllers/concerns/accountable_concern_spec.rb @@ -19,10 +19,24 @@ RSpec.describe AccountableConcern do let(:hoge) { hoge_class.new(user) } describe '#log_action' do - it 'creates Admin::ActionLog' do - expect do - hoge.log_action(:create, target) - end.to change(Admin::ActionLog, :count).by(1) + subject { hoge.log_action(:create, target) } + + before { target.reload } # Ensure changes from creation cleared + + context 'when target has changed' do + before { target.update!(username: 'new_value') } + + it 'creates Admin::ActionLog' do + expect { subject } + .to change(Admin::ActionLog, :count).by(1) + end + end + + context 'when target has not changed' do + it 'does not create Admin::ActionLog' do + expect { subject } + .to_not change(Admin::ActionLog, :count) + end end end end From 3835f8286aea0f1b51fc8fdff3c8d7c60a8855f6 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Mon, 27 May 2024 09:34:45 -0400 Subject: [PATCH 4/5] Restore action log specs --- spec/controllers/admin/accounts_controller_spec.rb | 9 +++++++-- spec/requests/api/v1/admin/accounts_spec.rb | 12 ++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/spec/controllers/admin/accounts_controller_spec.rb b/spec/controllers/admin/accounts_controller_spec.rb index bbd528a0a39..f241d261b1b 100644 --- a/spec/controllers/admin/accounts_controller_spec.rb +++ b/spec/controllers/admin/accounts_controller_spec.rb @@ -215,8 +215,13 @@ RSpec.describe Admin::AccountsController do it 'succeeds in rejecting account and logs action' do expect(subject).to redirect_to admin_accounts_path(status: 'pending') - expect { account.reload } - .to raise_error(ActiveRecord::RecordNotFound) + expect(latest_admin_action_log) + .to be_present + .and have_attributes( + action: eq(:reject), + account_id: eq(current_user.account_id), + target_id: eq(account.user.id) + ) end end diff --git a/spec/requests/api/v1/admin/accounts_spec.rb b/spec/requests/api/v1/admin/accounts_spec.rb index 0e7bd0dba13..1615581f0ea 100644 --- a/spec/requests/api/v1/admin/accounts_spec.rb +++ b/spec/requests/api/v1/admin/accounts_spec.rb @@ -199,6 +199,18 @@ RSpec.describe 'Accounts' do expect(response).to have_http_status(200) expect(User.where(id: account.user.id)).to_not exist end + + it 'logs action', :aggregate_failures do + subject + + expect(latest_admin_action_log) + .to be_present + .and have_attributes( + action: eq(:reject), + account_id: eq(user.account_id), + target_id: eq(account.user.id) + ) + end end context 'when account is already approved' do From e95899fa83dd29d78be0ffa784d1bfbaa4f5ecc5 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Mon, 27 May 2024 09:34:59 -0400 Subject: [PATCH 5/5] Handle destroyed records in action log --- app/controllers/concerns/accountable_concern.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/concerns/accountable_concern.rb b/app/controllers/concerns/accountable_concern.rb index e111c213fcb..7eb0fe1f8b0 100644 --- a/app/controllers/concerns/accountable_concern.rb +++ b/app/controllers/concerns/accountable_concern.rb @@ -4,7 +4,7 @@ module AccountableConcern extend ActiveSupport::Concern def log_action(action, target) - return unless target.previous_changes.any? + return unless target.previous_changes.any? || target.destroyed? Admin::ActionLog.create( account: current_account,