From b0b195b909d87c91d43e361f0b00497589f26e5f Mon Sep 17 00:00:00 2001 From: Ceithir Date: Mon, 17 Jul 2023 18:52:41 +0200 Subject: [PATCH 01/30] Adding https://otwarchive.atlassian.net/browse/AO3-6467 --- app/controllers/admin/admin_users_controller.rb | 4 ++++ app/models/log_item.rb | 2 ++ config/config.yml | 1 + ...20230717161221_add_fnok_user_id_to_log_items.rb | 11 +++++++++++ .../admin/admin_users_controller_spec.rb | 14 ++++++++++++++ 5 files changed, 32 insertions(+) create mode 100644 db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index a319a032716..d22689927ee 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -84,6 +84,10 @@ def update_next_of_kin fnok = @user.build_fannish_next_of_kin if fnok.blank? fnok.assign_attributes(kin: kin, kin_email: kin_email) if fnok.save + @user.create_log_item({ + action: ArchiveConfig.ACTION_ADD_FNOK, + fnok_user_id: fnok.kin.id + }) flash[:notice] = ts("Fannish next of kin was updated.") redirect_to admin_user_path(@user) else diff --git a/app/models/log_item.rb b/app/models/log_item.rb index 0730fa300cf..3c14592975d 100644 --- a/app/models/log_item.rb +++ b/app/models/log_item.rb @@ -7,6 +7,8 @@ class LogItem < ApplicationRecord belongs_to :role + belongs_to :fnok_user, class_name: "User" + validates_presence_of :note validates_presence_of :action diff --git a/config/config.yml b/config/config.yml index 2eccdffcf89..d73c1cfa21e 100644 --- a/config/config.yml +++ b/config/config.yml @@ -428,6 +428,7 @@ ACTION_PASSWORD_RESET: 8 ACTION_NEW_EMAIL: 9 ACTION_TROUBLESHOOT: 10 ACTION_NOTE: 11 +ACTION_ADD_FNOK: 12 # Elasticsearch index prefix diff --git a/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb b/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb new file mode 100644 index 00000000000..804b2534981 --- /dev/null +++ b/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb @@ -0,0 +1,11 @@ +class AddFnokUserIdToLogItems < ActiveRecord::Migration[6.1] + def up + add_column :log_items, :fnok_user_id, :integer, nullable: true, default: nil + add_index :log_items, :fnok_user_id + add_foreign_key :log_items, :users, column: :fnok_user_id + end + + def down + remove_column :log_items, :fnok_user_id + end +end diff --git a/spec/controllers/admin/admin_users_controller_spec.rb b/spec/controllers/admin/admin_users_controller_spec.rb index 13b750cef07..5245ee58b65 100644 --- a/spec/controllers/admin/admin_users_controller_spec.rb +++ b/spec/controllers/admin/admin_users_controller_spec.rb @@ -226,6 +226,20 @@ it_behaves_like "authorized admin can add next of kin" end end + + it 'logs adding a fannish next of kin' do + admin = create(:support_admin) + fake_login_admin(admin) + + post :update_next_of_kin, params: { + user_login: user.login, next_of_kin_name: kin.login, next_of_kin_email: kin.email + } + user.reload + expect(user.fannish_next_of_kin.kin).to eq(kin) + log_item = user.log_items.last + expect(log_item.action).to eq(ArchiveConfig.ACTION_ADD_FNOK) + expect(log_item.fnok_user.id).to eq(kin.id) + end end describe "POST #update_status" do From 8c9f826b685ede7ec2b2c34284885d819ac4ac15 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 18 Jul 2023 10:46:39 +0200 Subject: [PATCH 02/30] Remove --- app/controllers/admin/admin_users_controller.rb | 4 ++++ config/config.yml | 1 + factories/fannish_next_of_kin.rb | 9 +++++++++ .../admin/admin_users_controller_spec.rb | 15 +++++++++++++++ 4 files changed, 29 insertions(+) create mode 100644 factories/fannish_next_of_kin.rb diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index d22689927ee..9b1e4cb28ad 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -75,6 +75,10 @@ def update_next_of_kin if kin.blank? && kin_email.blank? if fnok.present? fnok.destroy + @user.create_log_item({ + action: ArchiveConfig.ACTION_REMOVE_FNOK, + fnok_user_id: fnok.kin.id + }) flash[:notice] = ts("Fannish next of kin was removed.") end redirect_to admin_user_path(@user) diff --git a/config/config.yml b/config/config.yml index d73c1cfa21e..06734d257ed 100644 --- a/config/config.yml +++ b/config/config.yml @@ -429,6 +429,7 @@ ACTION_NEW_EMAIL: 9 ACTION_TROUBLESHOOT: 10 ACTION_NOTE: 11 ACTION_ADD_FNOK: 12 +ACTION_REMOVE_FNOK: 13 # Elasticsearch index prefix diff --git a/factories/fannish_next_of_kin.rb b/factories/fannish_next_of_kin.rb new file mode 100644 index 00000000000..3dc5a390663 --- /dev/null +++ b/factories/fannish_next_of_kin.rb @@ -0,0 +1,9 @@ +require "faker" + +FactoryBot.define do + factory :fannish_next_of_kin do + user { create(:user) } + kin { create(:user) } + kin_email { |u| u.kin.email } + end +end diff --git a/spec/controllers/admin/admin_users_controller_spec.rb b/spec/controllers/admin/admin_users_controller_spec.rb index 5245ee58b65..4e8c439fc8f 100644 --- a/spec/controllers/admin/admin_users_controller_spec.rb +++ b/spec/controllers/admin/admin_users_controller_spec.rb @@ -240,6 +240,21 @@ expect(log_item.action).to eq(ArchiveConfig.ACTION_ADD_FNOK) expect(log_item.fnok_user.id).to eq(kin.id) end + + it 'logs removing a fannish next of kin' do + admin = create(:support_admin) + fake_login_admin(admin) + kin_user_id = create(:fannish_next_of_kin, user: user).kin_id + + post :update_next_of_kin, params: { + user_login: user.login + } + user.reload + expect(user.fannish_next_of_kin).to be_nil + log_item = user.log_items.last + expect(log_item.action).to eq(ArchiveConfig.ACTION_REMOVE_FNOK) + expect(log_item.fnok_user.id).to eq(kin_user_id) + end end describe "POST #update_status" do From dff308c943643c40190fc245ee20e2598932c33b Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 18 Jul 2023 10:57:29 +0200 Subject: [PATCH 03/30] Replace many ifelse with case --- app/helpers/users_helper.rb | 49 +++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index b6a6b173a2b..a0e5a4449ba 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -120,30 +120,31 @@ def authored_items(pseud, work_counts = {}, rec_counts = {}) end def log_item_action_name(action) - if action == ArchiveConfig.ACTION_ACTIVATE - t('users_helper.log_validated', default: 'Account Validated') - elsif action == ArchiveConfig.ACTION_ADD_ROLE - t('users_helper.log_role_added', default: 'Role Added: ') - elsif action == ArchiveConfig.ACTION_REMOVE_ROLE - t('users_helper.log_role_removed', default: 'Role Removed: ') - elsif action == ArchiveConfig.ACTION_SUSPEND - t('users_helper.log_suspended', default: 'Suspended until ') - elsif action == ArchiveConfig.ACTION_UNSUSPEND - t('users_helper.log_lift_suspension', default: 'Suspension Lifted') - elsif action == ArchiveConfig.ACTION_BAN - t('users_helper.log_ban', default: 'Suspended Permanently') - elsif action == ArchiveConfig.ACTION_WARN - t('users_helper.log_warn', default: 'Warned') - elsif action == ArchiveConfig.ACTION_RENAME - t('users_helper.log_rename', default: 'Username Changed') - elsif action == ArchiveConfig.ACTION_PASSWORD_RESET - t('users_helper.log_password_change', default: 'Password Changed') - elsif action == ArchiveConfig.ACTION_NEW_EMAIL - t('users_helper.log_email_change', default: 'Email Changed') - elsif action == ArchiveConfig.ACTION_TROUBLESHOOT - t('users_helper.log_troubleshot', default: 'Account Troubleshot') - elsif action == ArchiveConfig.ACTION_NOTE - t('users_helper.log_note', default: 'Note Added') + case action + when ArchiveConfig.ACTION_ACTIVATE + t('users_helper.log_validated', default: 'Account Validated') + when ArchiveConfig.ACTION_ADD_ROLE + t('users_helper.log_role_added', default: 'Role Added: ') + when ArchiveConfig.ACTION_REMOVE_ROLE + t('users_helper.log_role_removed', default: 'Role Removed: ') + when ArchiveConfig.ACTION_SUSPEND + t('users_helper.log_suspended', default: 'Suspended until ') + when ArchiveConfig.ACTION_UNSUSPEND + t('users_helper.log_lift_suspension', default: 'Suspension Lifted') + when ArchiveConfig.ACTION_BAN + t('users_helper.log_ban', default: 'Suspended Permanently') + when ArchiveConfig.ACTION_WARN + t('users_helper.log_warn', default: 'Warned') + when ArchiveConfig.ACTION_RENAME + t('users_helper.log_rename', default: 'Username Changed') + when ArchiveConfig.ACTION_PASSWORD_RESET + t('users_helper.log_password_change', default: 'Password Changed') + when ArchiveConfig.ACTION_NEW_EMAIL + t('users_helper.log_email_change', default: 'Email Changed') + when ArchiveConfig.ACTION_TROUBLESHOOT + t('users_helper.log_troubleshot', default: 'Account Troubleshot') + when ArchiveConfig.ACTION_NOTE + t('users_helper.log_note', default: 'Note Added') end end From e0d0e670530feb30349ef06715f2cebc92fa6f16 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 18 Jul 2023 11:07:43 +0200 Subject: [PATCH 04/30] Display logs on admin page --- app/helpers/users_helper.rb | 4 ++++ app/views/admin/admin_users/_user_history.html.erb | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index a0e5a4449ba..a17ae7173cc 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -145,6 +145,10 @@ def log_item_action_name(action) t('users_helper.log_troubleshot', default: 'Account Troubleshot') when ArchiveConfig.ACTION_NOTE t('users_helper.log_note', default: 'Note Added') + when ArchiveConfig.ACTION_ADD_FNOK + t('users_helper.log_add_fnok', default: 'Fannish Next of Kin Added') + when ArchiveConfig.ACTION_REMOVE_FNOK + t('users_helper.log_add_fnok', default: 'Fannish Next of Kin Removed') end end diff --git a/app/views/admin/admin_users/_user_history.html.erb b/app/views/admin/admin_users/_user_history.html.erb index 0c19185ec7f..01e6b468266 100644 --- a/app/views/admin/admin_users/_user_history.html.erb +++ b/app/views/admin/admin_users/_user_history.html.erb @@ -36,7 +36,7 @@ @log_items.each do |item| %> <%= item.created_at %> - <%= log_item_action_name(item.action) %><%= item.role.name if item.role %><%= item.enddate if item.enddate %> + <%= log_item_action_name(item.action) %><%= item.role.name if item.role %><%= item.enddate if item.enddate %><% if item.fnok_user_id %>: <%= item.fnok_user_id %><% end %> <%= item.note %> <% end %> From 78c03d917896eef79dff444415022989bb7b7eb2 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 18 Jul 2023 11:23:29 +0200 Subject: [PATCH 05/30] Forgot to log admin identity --- app/controllers/admin/admin_users_controller.rb | 8 ++++++-- spec/controllers/admin/admin_users_controller_spec.rb | 4 ++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index 9b1e4cb28ad..2798826674d 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -77,7 +77,9 @@ def update_next_of_kin fnok.destroy @user.create_log_item({ action: ArchiveConfig.ACTION_REMOVE_FNOK, - fnok_user_id: fnok.kin.id + fnok_user_id: fnok.kin.id, + admin_id: current_admin.id, + note: "Change made by #{current_admin.login}" }) flash[:notice] = ts("Fannish next of kin was removed.") end @@ -90,7 +92,9 @@ def update_next_of_kin if fnok.save @user.create_log_item({ action: ArchiveConfig.ACTION_ADD_FNOK, - fnok_user_id: fnok.kin.id + fnok_user_id: fnok.kin.id, + admin_id: current_admin.id, + note: "Change made by #{current_admin.login}" }) flash[:notice] = ts("Fannish next of kin was updated.") redirect_to admin_user_path(@user) diff --git a/spec/controllers/admin/admin_users_controller_spec.rb b/spec/controllers/admin/admin_users_controller_spec.rb index 4e8c439fc8f..416069c97df 100644 --- a/spec/controllers/admin/admin_users_controller_spec.rb +++ b/spec/controllers/admin/admin_users_controller_spec.rb @@ -239,6 +239,8 @@ log_item = user.log_items.last expect(log_item.action).to eq(ArchiveConfig.ACTION_ADD_FNOK) expect(log_item.fnok_user.id).to eq(kin.id) + expect(log_item.admin_id).to eq(admin.id) + expect(log_item.note).to eq("Change made by #{admin.login}") end it 'logs removing a fannish next of kin' do @@ -254,6 +256,8 @@ log_item = user.log_items.last expect(log_item.action).to eq(ArchiveConfig.ACTION_REMOVE_FNOK) expect(log_item.fnok_user.id).to eq(kin_user_id) + expect(log_item.admin_id).to eq(admin.id) + expect(log_item.note).to eq("Change made by #{admin.login}") end end From 26a3f0d75d79ae510b5decca58e2120c123c52ae Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 18 Jul 2023 12:35:39 +0200 Subject: [PATCH 06/30] Also show logs on the FNOK side --- .../admin/admin_users_controller.rb | 8 +++- app/helpers/users_helper.rb | 41 ++++++++++++++++--- .../admin/admin_users/_user_history.html.erb | 2 +- 3 files changed, 43 insertions(+), 8 deletions(-) diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index 2798826674d..2f3de78cb00 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -47,7 +47,7 @@ def show @user = authorize User.find_by!(login: params[:id]) @hide_dashboard = true @page_subtitle = t(".page_title", login: @user.login) - @log_items = @user.log_items.sort_by(&:created_at).reverse + fetch_log_items end # POST admin/users/update @@ -100,7 +100,7 @@ def update_next_of_kin redirect_to admin_user_path(@user) else @hide_dashboard = true - @log_items = @user.log_items.sort_by(&:created_at).reverse + fetch_log_items render :show end end @@ -180,4 +180,8 @@ def activate redirect_to action: :show end end + + def fetch_log_items + @log_items ||= (@user.log_items + LogItem.where(fnok_user_id: @user.id)).sort_by(&:created_at).reverse + end end diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index a17ae7173cc..8a92c2bbaac 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -119,7 +119,42 @@ def authored_items(pseud, work_counts = {}, rec_counts = {}) items.html_safe end - def log_item_action_name(action) + def log_item_action_name(item, user) + action = item.action + + if [ArchiveConfig.ACTION_ADD_FNOK, ArchiveConfig.ACTION_REMOVE_FNOK].include?(action) + if item.fnok_user_id == user.id + if action == ArchiveConfig.ACTION_REMOVE_FNOK + return t( + 'users_helper.log_removed_as_fnok', + default: 'Removed as Fannish Next of Kin for: %{user_id}', + user_id: item.user_id + ) + end + + return t( + 'users_helper.log_added_as_fnok', + default: 'Added as Fannish Next of Kin for: %{user_id}', + user_id: item.user_id + ) + end + + if action == ArchiveConfig.ACTION_REMOVE_FNOK + return t( + 'users_helper.log_remove_fnok', + default: 'Fannish Next of Kin Removed: %{user_id}', + user_id: item.fnok_user_id + ) + end + + return t( + 'users_helper.log_add_fnok', + default: 'Fannish Next of Kin Added: %{user_id}', + user_id: item.fnok_user_id + ) + end + + case action when ArchiveConfig.ACTION_ACTIVATE t('users_helper.log_validated', default: 'Account Validated') @@ -145,10 +180,6 @@ def log_item_action_name(action) t('users_helper.log_troubleshot', default: 'Account Troubleshot') when ArchiveConfig.ACTION_NOTE t('users_helper.log_note', default: 'Note Added') - when ArchiveConfig.ACTION_ADD_FNOK - t('users_helper.log_add_fnok', default: 'Fannish Next of Kin Added') - when ArchiveConfig.ACTION_REMOVE_FNOK - t('users_helper.log_add_fnok', default: 'Fannish Next of Kin Removed') end end diff --git a/app/views/admin/admin_users/_user_history.html.erb b/app/views/admin/admin_users/_user_history.html.erb index 01e6b468266..6752483d69b 100644 --- a/app/views/admin/admin_users/_user_history.html.erb +++ b/app/views/admin/admin_users/_user_history.html.erb @@ -36,7 +36,7 @@ @log_items.each do |item| %> <%= item.created_at %> - <%= log_item_action_name(item.action) %><%= item.role.name if item.role %><%= item.enddate if item.enddate %><% if item.fnok_user_id %>: <%= item.fnok_user_id %><% end %> + <%= log_item_action_name(item, @user) %><%= item.role.name if item.role %><%= item.enddate if item.enddate %> <%= item.note %> <% end %> From cb63e18d485013a69e99d54de9f6afa4cf36ce04 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 18 Jul 2023 12:45:04 +0200 Subject: [PATCH 07/30] Test for display Keeping it simple by not checking id for now --- features/admins/users/admin_fnok.feature | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/features/admins/users/admin_fnok.feature b/features/admins/users/admin_fnok.feature index d1e80ebfd4d..6842fc513c8 100644 --- a/features/admins/users/admin_fnok.feature +++ b/features/admins/users/admin_fnok.feature @@ -15,6 +15,7 @@ Feature: Admin Fannish Next Of Kind actions And I fill in "Fannish next of kin's email" with "testy@foo.com" And I press "Update Fannish Next of Kin" Then I should see "Fannish next of kin was updated." + Then I should see "Fannish Next of Kin Added" When I go to the manage users page And I fill in "Name" with "harrykim" @@ -24,6 +25,9 @@ Feature: Admin Fannish Next Of Kind actions When I follow "libby" Then I should be on libby's user page + When I go to the user administration page for "libby" + Then I should see "Added as Fannish Next of Kin" + Scenario: An invalid Fannish Next of Kin username is added Given the fannish next of kin "libby" for the user "harrykim" And I am logged in as a "support" admin @@ -66,6 +70,9 @@ Feature: Admin Fannish Next Of Kind actions And I fill in "Fannish next of kin's email" with "" And I press "Update Fannish Next of Kin" Then I should see "Fannish next of kin was removed." + And I should see "Fannish Next of Kin Removed" + When I go to the user administration page for "libby" + Then I should see "Removed as Fannish Next of Kin" Scenario: A Fannish Next of Kin updates when the next of kin user changes their username Given the fannish next of kin "libby" for the user "harrykim" From 7ca5241e6f1b5db5764e38a848b04675c5085e3b Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 18 Jul 2023 12:55:55 +0200 Subject: [PATCH 08/30] Add Alterity --- Gemfile | 1 + 1 file changed, 1 insertion(+) diff --git a/Gemfile b/Gemfile index 0ff372463ec..455f86fad64 100644 --- a/Gemfile +++ b/Gemfile @@ -176,6 +176,7 @@ gem 'unicorn', '~> 5.5', require: false gem 'god', '~> 0.13.7' group :staging, :production do + gem "alterity" # Place the New Relic gem as low in the list as possible, allowing the # frameworks above it to be instrumented when the gem initializes. gem "newrelic_rpm" From e4986ea94f3e2fe0e33a8ba0926ed4635088f4ef Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 18 Jul 2023 13:17:21 +0200 Subject: [PATCH 09/30] Forgot to commit previously --- Gemfile.lock | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Gemfile.lock b/Gemfile.lock index 88d9c8bffe7..95b303f6217 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -98,6 +98,9 @@ GEM activerecord (>= 4.2) activesupport akismetor (1.0.0) + alterity (1.4.2) + mysql2 (>= 0.3) + rails (>= 6.1) ast (2.4.2) audited (4.10.0) activerecord (>= 4.2, < 6.2) @@ -608,6 +611,7 @@ DEPENDENCIES addressable after_commit_everywhere akismetor + alterity audited (~> 4.4) awesome_print aws-sdk-s3 From 8a7edd9004f110b8030bc6bc78f288e5b28c36a6 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 18 Jul 2023 13:17:47 +0200 Subject: [PATCH 10/30] Hound --- .../admin/admin_users_controller.rb | 26 ++++++------ app/helpers/users_helper.rb | 40 +++++++++---------- 2 files changed, 33 insertions(+), 33 deletions(-) diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index 2f3de78cb00..5031bef95fa 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -47,7 +47,7 @@ def show @user = authorize User.find_by!(login: params[:id]) @hide_dashboard = true @page_subtitle = t(".page_title", login: @user.login) - fetch_log_items + log_items end # POST admin/users/update @@ -76,11 +76,11 @@ def update_next_of_kin if fnok.present? fnok.destroy @user.create_log_item({ - action: ArchiveConfig.ACTION_REMOVE_FNOK, - fnok_user_id: fnok.kin.id, - admin_id: current_admin.id, - note: "Change made by #{current_admin.login}" - }) + action: ArchiveConfig.ACTION_REMOVE_FNOK, + fnok_user_id: fnok.kin.id, + admin_id: current_admin.id, + note: "Change made by #{current_admin.login}" + }) flash[:notice] = ts("Fannish next of kin was removed.") end redirect_to admin_user_path(@user) @@ -91,16 +91,16 @@ def update_next_of_kin fnok.assign_attributes(kin: kin, kin_email: kin_email) if fnok.save @user.create_log_item({ - action: ArchiveConfig.ACTION_ADD_FNOK, - fnok_user_id: fnok.kin.id, - admin_id: current_admin.id, - note: "Change made by #{current_admin.login}" - }) + action: ArchiveConfig.ACTION_ADD_FNOK, + fnok_user_id: fnok.kin.id, + admin_id: current_admin.id, + note: "Change made by #{current_admin.login}" + }) flash[:notice] = ts("Fannish next of kin was updated.") redirect_to admin_user_path(@user) else @hide_dashboard = true - fetch_log_items + log_items render :show end end @@ -181,7 +181,7 @@ def activate end end - def fetch_log_items + def log_items @log_items ||= (@user.log_items + LogItem.where(fnok_user_id: @user.id)).sort_by(&:created_at).reverse end end diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 8a92c2bbaac..3bec8c1ac7a 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -126,30 +126,30 @@ def log_item_action_name(item, user) if item.fnok_user_id == user.id if action == ArchiveConfig.ACTION_REMOVE_FNOK return t( - 'users_helper.log_removed_as_fnok', - default: 'Removed as Fannish Next of Kin for: %{user_id}', + "users_helper.log_removed_as_fnok", + default: "Removed as Fannish Next of Kin for: %{user_id}", user_id: item.user_id ) end return t( - 'users_helper.log_added_as_fnok', - default: 'Added as Fannish Next of Kin for: %{user_id}', + "users_helper.log_added_as_fnok", + default: "Added as Fannish Next of Kin for: %{user_id}", user_id: item.user_id ) end if action == ArchiveConfig.ACTION_REMOVE_FNOK return t( - 'users_helper.log_remove_fnok', - default: 'Fannish Next of Kin Removed: %{user_id}', + "users_helper.log_remove_fnok", + default: "Fannish Next of Kin Removed: %{user_id}", user_id: item.fnok_user_id ) end return t( - 'users_helper.log_add_fnok', - default: 'Fannish Next of Kin Added: %{user_id}', + "users_helper.log_add_fnok", + default: "Fannish Next of Kin Added: %{user_id}", user_id: item.fnok_user_id ) end @@ -157,29 +157,29 @@ def log_item_action_name(item, user) case action when ArchiveConfig.ACTION_ACTIVATE - t('users_helper.log_validated', default: 'Account Validated') + t("users_helper.log_validated", default: "Account Validated") when ArchiveConfig.ACTION_ADD_ROLE - t('users_helper.log_role_added', default: 'Role Added: ') + t("users_helper.log_role_added", default: "Role Added: ") when ArchiveConfig.ACTION_REMOVE_ROLE - t('users_helper.log_role_removed', default: 'Role Removed: ') + t("users_helper.log_role_removed", default: "Role Removed: ") when ArchiveConfig.ACTION_SUSPEND - t('users_helper.log_suspended', default: 'Suspended until ') + t("users_helper.log_suspended", default: "Suspended until ") when ArchiveConfig.ACTION_UNSUSPEND - t('users_helper.log_lift_suspension', default: 'Suspension Lifted') + t("users_helper.log_lift_suspension", default: "Suspension Lifted") when ArchiveConfig.ACTION_BAN - t('users_helper.log_ban', default: 'Suspended Permanently') + t("users_helper.log_ban", default: "Suspended Permanently") when ArchiveConfig.ACTION_WARN - t('users_helper.log_warn', default: 'Warned') + t("users_helper.log_warn", default: "Warned") when ArchiveConfig.ACTION_RENAME - t('users_helper.log_rename', default: 'Username Changed') + t("users_helper.log_rename", default: "Username Changed") when ArchiveConfig.ACTION_PASSWORD_RESET - t('users_helper.log_password_change', default: 'Password Changed') + t("users_helper.log_password_change", default: "Password Changed") when ArchiveConfig.ACTION_NEW_EMAIL - t('users_helper.log_email_change', default: 'Email Changed') + t("users_helper.log_email_change", default: "Email Changed") when ArchiveConfig.ACTION_TROUBLESHOOT - t('users_helper.log_troubleshot', default: 'Account Troubleshot') + t("users_helper.log_troubleshot", default: "Account Troubleshot") when ArchiveConfig.ACTION_NOTE - t('users_helper.log_note', default: 'Note Added') + t("users_helper.log_note", default: "Note Added") end end From 23b89f39fb3ba0a42e4d1c1757f396d1d7c52cec Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 18 Jul 2023 13:20:44 +0200 Subject: [PATCH 11/30] Hound (bis) --- app/helpers/users_helper.rb | 48 ++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 3bec8c1ac7a..60dbbc91291 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -156,30 +156,30 @@ def log_item_action_name(item, user) case action - when ArchiveConfig.ACTION_ACTIVATE - t("users_helper.log_validated", default: "Account Validated") - when ArchiveConfig.ACTION_ADD_ROLE - t("users_helper.log_role_added", default: "Role Added: ") - when ArchiveConfig.ACTION_REMOVE_ROLE - t("users_helper.log_role_removed", default: "Role Removed: ") - when ArchiveConfig.ACTION_SUSPEND - t("users_helper.log_suspended", default: "Suspended until ") - when ArchiveConfig.ACTION_UNSUSPEND - t("users_helper.log_lift_suspension", default: "Suspension Lifted") - when ArchiveConfig.ACTION_BAN - t("users_helper.log_ban", default: "Suspended Permanently") - when ArchiveConfig.ACTION_WARN - t("users_helper.log_warn", default: "Warned") - when ArchiveConfig.ACTION_RENAME - t("users_helper.log_rename", default: "Username Changed") - when ArchiveConfig.ACTION_PASSWORD_RESET - t("users_helper.log_password_change", default: "Password Changed") - when ArchiveConfig.ACTION_NEW_EMAIL - t("users_helper.log_email_change", default: "Email Changed") - when ArchiveConfig.ACTION_TROUBLESHOOT - t("users_helper.log_troubleshot", default: "Account Troubleshot") - when ArchiveConfig.ACTION_NOTE - t("users_helper.log_note", default: "Note Added") + when ArchiveConfig.ACTION_ACTIVATE + t("users_helper.log_validated", default: "Account Validated") + when ArchiveConfig.ACTION_ADD_ROLE + t("users_helper.log_role_added", default: "Role Added: ") + when ArchiveConfig.ACTION_REMOVE_ROLE + t("users_helper.log_role_removed", default: "Role Removed: ") + when ArchiveConfig.ACTION_SUSPEND + t("users_helper.log_suspended", default: "Suspended until ") + when ArchiveConfig.ACTION_UNSUSPEND + t("users_helper.log_lift_suspension", default: "Suspension Lifted") + when ArchiveConfig.ACTION_BAN + t("users_helper.log_ban", default: "Suspended Permanently") + when ArchiveConfig.ACTION_WARN + t("users_helper.log_warn", default: "Warned") + when ArchiveConfig.ACTION_RENAME + t("users_helper.log_rename", default: "Username Changed") + when ArchiveConfig.ACTION_PASSWORD_RESET + t("users_helper.log_password_change", default: "Password Changed") + when ArchiveConfig.ACTION_NEW_EMAIL + t("users_helper.log_email_change", default: "Email Changed") + when ArchiveConfig.ACTION_TROUBLESHOOT + t("users_helper.log_troubleshot", default: "Account Troubleshot") + when ArchiveConfig.ACTION_NOTE + t("users_helper.log_note", default: "Note Added") end end From aef77f60d88f47d8d78d47c8dc3d7b792f3fd925 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 18 Jul 2023 13:22:12 +0200 Subject: [PATCH 12/30] Hound (ter) --- app/helpers/users_helper.rb | 1 - spec/controllers/admin/admin_users_controller_spec.rb | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 60dbbc91291..1de76c6ca7a 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -154,7 +154,6 @@ def log_item_action_name(item, user) ) end - case action when ArchiveConfig.ACTION_ACTIVATE t("users_helper.log_validated", default: "Account Validated") diff --git a/spec/controllers/admin/admin_users_controller_spec.rb b/spec/controllers/admin/admin_users_controller_spec.rb index 416069c97df..ef301528a2b 100644 --- a/spec/controllers/admin/admin_users_controller_spec.rb +++ b/spec/controllers/admin/admin_users_controller_spec.rb @@ -227,7 +227,7 @@ end end - it 'logs adding a fannish next of kin' do + it "logs adding a fannish next of kin" do admin = create(:support_admin) fake_login_admin(admin) @@ -243,7 +243,7 @@ expect(log_item.note).to eq("Change made by #{admin.login}") end - it 'logs removing a fannish next of kin' do + it "logs removing a fannish next of kin" do admin = create(:support_admin) fake_login_admin(admin) kin_user_id = create(:fannish_next_of_kin, user: user).kin_id From ed3111aa9e37bec7337311e8c2bd7d02bf670271 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 18 Jul 2023 14:00:57 +0200 Subject: [PATCH 13/30] Add i18n keys to exceptions --- config/i18n-tasks.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/config/i18n-tasks.yml b/config/i18n-tasks.yml index 9d82c6ac91a..bc5ac529e46 100644 --- a/config/i18n-tasks.yml +++ b/config/i18n-tasks.yml @@ -166,6 +166,10 @@ ignore_missing: - users_helper.log_troubleshot - users_helper.log_validated - users_helper.log_warn + - users_helper.log_add_fnok + - users_helper.log_remove_fnok + - users_helper.log_added_as_fnok + - users_helper.log_removed_as_fnok # Files: app/views/admin/_admin_nav.html.erb and app/views/admin_posts/show.html.erb - admin.admin_nav.delete # File: app/views/admin/admin_invitations/find.html.erb From 77231d62c6fa6879942dec59d0f3d11d5b51ffe6 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 8 Aug 2023 10:35:46 +0200 Subject: [PATCH 14/30] Remove Alterity actually Superseded by https://github.com/otwcode/otwarchive/pull/4528 --- Gemfile | 1 - Gemfile.lock | 4 ---- 2 files changed, 5 deletions(-) diff --git a/Gemfile b/Gemfile index 455f86fad64..0ff372463ec 100644 --- a/Gemfile +++ b/Gemfile @@ -176,7 +176,6 @@ gem 'unicorn', '~> 5.5', require: false gem 'god', '~> 0.13.7' group :staging, :production do - gem "alterity" # Place the New Relic gem as low in the list as possible, allowing the # frameworks above it to be instrumented when the gem initializes. gem "newrelic_rpm" diff --git a/Gemfile.lock b/Gemfile.lock index 95b303f6217..88d9c8bffe7 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -98,9 +98,6 @@ GEM activerecord (>= 4.2) activesupport akismetor (1.0.0) - alterity (1.4.2) - mysql2 (>= 0.3) - rails (>= 6.1) ast (2.4.2) audited (4.10.0) activerecord (>= 4.2, < 6.2) @@ -611,7 +608,6 @@ DEPENDENCIES addressable after_commit_everywhere akismetor - alterity audited (~> 4.4) awesome_print aws-sdk-s3 From b892b720b9d4e0891ce821751d6dce47bd0664e4 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 8 Aug 2023 12:21:32 +0200 Subject: [PATCH 15/30] Add pt-online-schema-change syntax for prod --- ...717161221_add_fnok_user_id_to_log_items.rb | 51 +++++++++++++++++-- 1 file changed, 47 insertions(+), 4 deletions(-) diff --git a/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb b/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb index 804b2534981..66d6571318d 100644 --- a/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb +++ b/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb @@ -1,11 +1,54 @@ class AddFnokUserIdToLogItems < ActiveRecord::Migration[6.1] def up - add_column :log_items, :fnok_user_id, :integer, nullable: true, default: nil - add_index :log_items, :fnok_user_id - add_foreign_key :log_items, :users, column: :fnok_user_id + if Rails.env.staging? || Rails.env.production? + database = LogItem.connection.current_database + + puts <<~PTOSC + Schema Change Command: + + pt-online-schema-change D=#{database},t=log_items \\ + --alter "ADD `fnok_user_id` int DEFAULT NULL, + ADD INDEX `index_log_items_on_fnok_user_id` (`fnok_user_id`), + ADD CONSTRAINT `fk_rails_6a47a753ee" FOREIGN KEY (`fnok_user_id`) REFERENCES `users` (`id`)" \\ + --no-drop-old-table \\ + -uroot --ask-pass --chunk-size=5k --max-flow-ctl 0 --pause-file /tmp/pauseme \\ + --max-load Threads_running=15 --critical-load Threads_running=100 \\ + --set-vars innodb_lock_wait_timeout=2 --alter-foreign-keys-method=auto \\ + --execute + + Table Deletion Command: + + DROP TABLE IF EXISTS `#{database}`.`_log_items_old`; + PTOSC + else + add_column :log_items, :fnok_user_id, :integer, nullable: true, default: nil + add_index :log_items, :fnok_user_id + add_foreign_key :log_items, :users, column: :fnok_user_id + end end def down - remove_column :log_items, :fnok_user_id + if Rails.env.staging? || Rails.env.production? + database = LogItem.connection.current_database + + puts <<~PTOSC + Schema Change Command: + + pt-online-schema-change D=#{database},t=log_items \\ + --alter "DROP FOREIGN KEY fk_rails_6a47a753ee, + DROP COLUMN `fnok_user_id`"\\ + --no-drop-old-table \\ + -uroot --ask-pass --chunk-size=5k --max-flow-ctl 0 --pause-file /tmp/pauseme \\ + --max-load Threads_running=15 --critical-load Threads_running=100 \\ + --set-vars innodb_lock_wait_timeout=2 --alter-foreign-keys-method=auto \\ + --execute + + Table Deletion Command: + + DROP TABLE IF EXISTS `#{database}`.`_log_items_old`; + PTOSC + else + remove_column :log_items, :fnok_user_id + end end end From f8c67b7cb49066e6adc9b192abd262c3c871bcca Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 8 Aug 2023 12:23:20 +0200 Subject: [PATCH 16/30] Remove explicit foreign key Similar migrations don't appear to set one --- db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb b/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb index 66d6571318d..b23d8463bb2 100644 --- a/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb +++ b/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb @@ -8,8 +8,7 @@ def up pt-online-schema-change D=#{database},t=log_items \\ --alter "ADD `fnok_user_id` int DEFAULT NULL, - ADD INDEX `index_log_items_on_fnok_user_id` (`fnok_user_id`), - ADD CONSTRAINT `fk_rails_6a47a753ee" FOREIGN KEY (`fnok_user_id`) REFERENCES `users` (`id`)" \\ + ADD INDEX `index_log_items_on_fnok_user_id` (`fnok_user_id`)" \\ --no-drop-old-table \\ -uroot --ask-pass --chunk-size=5k --max-flow-ctl 0 --pause-file /tmp/pauseme \\ --max-load Threads_running=15 --critical-load Threads_running=100 \\ @@ -23,7 +22,6 @@ def up else add_column :log_items, :fnok_user_id, :integer, nullable: true, default: nil add_index :log_items, :fnok_user_id - add_foreign_key :log_items, :users, column: :fnok_user_id end end @@ -35,8 +33,7 @@ def down Schema Change Command: pt-online-schema-change D=#{database},t=log_items \\ - --alter "DROP FOREIGN KEY fk_rails_6a47a753ee, - DROP COLUMN `fnok_user_id`"\\ + --alter "DROP COLUMN `fnok_user_id`"\\ --no-drop-old-table \\ -uroot --ask-pass --chunk-size=5k --max-flow-ctl 0 --pause-file /tmp/pauseme \\ --max-load Threads_running=15 --critical-load Threads_running=100 \\ From 972432b79571d065b3c236cb25ba7dda7796845f Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 8 Aug 2023 12:24:52 +0200 Subject: [PATCH 17/30] Implicit NULL default value --- db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb b/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb index b23d8463bb2..9d0adb23dee 100644 --- a/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb +++ b/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb @@ -7,7 +7,7 @@ def up Schema Change Command: pt-online-schema-change D=#{database},t=log_items \\ - --alter "ADD `fnok_user_id` int DEFAULT NULL, + --alter "ADD `fnok_user_id` int, ADD INDEX `index_log_items_on_fnok_user_id` (`fnok_user_id`)" \\ --no-drop-old-table \\ -uroot --ask-pass --chunk-size=5k --max-flow-ctl 0 --pause-file /tmp/pauseme \\ @@ -20,7 +20,7 @@ def up DROP TABLE IF EXISTS `#{database}`.`_log_items_old`; PTOSC else - add_column :log_items, :fnok_user_id, :integer, nullable: true, default: nil + add_column :log_items, :fnok_user_id, :integer, nullable: true add_index :log_items, :fnok_user_id end end From 082f48304aa75931aad0e10cea1b851d184dda9c Mon Sep 17 00:00:00 2001 From: Ceithir Date: Mon, 21 Aug 2023 19:16:25 +0200 Subject: [PATCH 18/30] Test id too --- features/admins/users/admin_fnok.feature | 8 ++++---- features/step_definitions/admin_steps.rb | 10 ++++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/features/admins/users/admin_fnok.feature b/features/admins/users/admin_fnok.feature index 6842fc513c8..45096b1cbe5 100644 --- a/features/admins/users/admin_fnok.feature +++ b/features/admins/users/admin_fnok.feature @@ -15,7 +15,7 @@ Feature: Admin Fannish Next Of Kind actions And I fill in "Fannish next of kin's email" with "testy@foo.com" And I press "Update Fannish Next of Kin" Then I should see "Fannish next of kin was updated." - Then I should see "Fannish Next of Kin Added" + And the history table should show that "libby" was added as next of kin When I go to the manage users page And I fill in "Name" with "harrykim" @@ -26,7 +26,7 @@ Feature: Admin Fannish Next Of Kind actions Then I should be on libby's user page When I go to the user administration page for "libby" - Then I should see "Added as Fannish Next of Kin" + Then the history table should show they were added as next of kin of "harrykim" Scenario: An invalid Fannish Next of Kin username is added Given the fannish next of kin "libby" for the user "harrykim" @@ -70,9 +70,9 @@ Feature: Admin Fannish Next Of Kind actions And I fill in "Fannish next of kin's email" with "" And I press "Update Fannish Next of Kin" Then I should see "Fannish next of kin was removed." - And I should see "Fannish Next of Kin Removed" + And the history table should show that "libby" was removed as next of kin When I go to the user administration page for "libby" - Then I should see "Removed as Fannish Next of Kin" + Then the history table should show they were removed as next of kin of "harrykim" Scenario: A Fannish Next of Kin updates when the next of kin user changes their username Given the fannish next of kin "libby" for the user "harrykim" diff --git a/features/step_definitions/admin_steps.rb b/features/step_definitions/admin_steps.rb index 398da294f58..34bb795a145 100644 --- a/features/step_definitions/admin_steps.rb +++ b/features/step_definitions/admin_steps.rb @@ -504,3 +504,13 @@ expect(page).to have_selector(".original_creators", text: "#{user.id} (#{creator})") end + +Then "the history table should show that {string} was {word} as next of kin" do |username, action| + user_id = User.find_by(login: username).id + step %{I should see "Fannish Next of Kin #{action.capitalize}: #{user_id}" within "#user_history"} +end + +Then "the history table should show they were {word} as next of kin of {string}" do |action, username| + user_id = User.find_by(login: username).id + step %{I should see "#{action.capitalize} as Fannish Next of Kin for: #{user_id}" within "#user_history"} +end From 857acba885b685653999fca92f128148e08aaeb9 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Mon, 21 Aug 2023 19:56:20 +0200 Subject: [PATCH 19/30] Add user destroy controller test Apparently didn't exist before? --- spec/controllers/users_controller_spec.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 1fdb7159a74..1d9c6180928 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -2,6 +2,7 @@ describe UsersController do include RedirectExpectationHelper + include LoginMacros describe "GET #activate" do let(:user) { create(:user, confirmed_at: nil) } @@ -58,4 +59,21 @@ end end end + + describe "destroy" do + let(:user) { create(:user) } + + before do + fake_login_known_user(user) + end + + context "user with no activity" do + it "deletes without side effects" do + login = user.login + delete :destroy, params: { id: login } + it_redirects_to_with_notice(delete_confirmation_path, "You have successfully deleted your account.") + expect(User.find_by(login: login)).to be_nil + end + end + end end From 365896be2685f6176aba435ce746f5fa736bbf8c Mon Sep 17 00:00:00 2001 From: Ceithir Date: Mon, 21 Aug 2023 22:18:08 +0200 Subject: [PATCH 20/30] Log automatic removal of fnok on deletion --- app/models/user.rb | 10 ++++++++++ spec/models/user_spec.rb | 17 +++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/app/models/user.rb b/app/models/user.rb index 6d51edcd8db..02cd0f088ca 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -36,6 +36,7 @@ class User < ApplicationRecord has_many :external_authors, dependent: :destroy has_many :external_creatorships, foreign_key: "archivist_id" + before_destroy :log_if_next_of_kin has_many :fannish_next_of_kins, dependent: :delete_all, inverse_of: :kin, foreign_key: :kin_id has_one :fannish_next_of_kin, dependent: :destroy @@ -171,6 +172,15 @@ def remove_user_from_kudos Kudo.where(user: self).update_all(user_id: nil) end + def log_if_next_of_kin + fannish_next_of_kins.each do |fnok| + fnok.user.create_log_item({ + action: ArchiveConfig.ACTION_REMOVE_FNOK, + fnok_user_id: self.id, + }) + end + end + def read_inbox_comments inbox_comments.where(read: true) end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index cdb6a1b55fd..aff7b1ca5d7 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -15,6 +15,23 @@ end end end + + context "when the user is set as someone else's fnok" do + let(:fnok) { create(:fannish_next_of_kin) } + let(:user) { fnok.kin } + let(:person) { fnok.user } + + it "explictly removes the relationship" do + user_id = user.id + user.destroy! + expect(person.reload.fannish_next_of_kin).to be_nil + log_item = person.log_items.last + expect(log_item.action).to eq(ArchiveConfig.ACTION_REMOVE_FNOK) + expect(log_item.fnok_user_id).to eq(user_id) + expect(log_item.admin_id).to be_nil + expect(log_item.note).to eq("System Generated") + end + end end describe "#save" do From 7bb875c338951d9a41126df4d2dc8b3ed16d13ff Mon Sep 17 00:00:00 2001 From: Ceithir Date: Mon, 21 Aug 2023 22:20:58 +0200 Subject: [PATCH 21/30] Hound --- app/models/user.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 02cd0f088ca..0649b327834 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -175,8 +175,8 @@ def remove_user_from_kudos def log_if_next_of_kin fannish_next_of_kins.each do |fnok| fnok.user.create_log_item({ - action: ArchiveConfig.ACTION_REMOVE_FNOK, - fnok_user_id: self.id, + action: ArchiveConfig.ACTION_REMOVE_FNOK, + fnok_user_id: self.id }) end end From b8ead04eb3779cf924a19bce1d7b4199de08c1c2 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Mon, 21 Aug 2023 22:29:44 +0200 Subject: [PATCH 22/30] Use classical I18n syntax --- app/helpers/users_helper.rb | 12 ++++-------- config/i18n-tasks.yml | 4 ---- config/locales/helpers/en.yml | 7 +++++++ 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 1de76c6ca7a..9c0434b0457 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -126,30 +126,26 @@ def log_item_action_name(item, user) if item.fnok_user_id == user.id if action == ArchiveConfig.ACTION_REMOVE_FNOK return t( - "users_helper.log_removed_as_fnok", - default: "Removed as Fannish Next of Kin for: %{user_id}", + "users_helper.log.fnok.removed_as", user_id: item.user_id ) end return t( - "users_helper.log_added_as_fnok", - default: "Added as Fannish Next of Kin for: %{user_id}", + "users_helper.log.fnok.added_as", user_id: item.user_id ) end if action == ArchiveConfig.ACTION_REMOVE_FNOK return t( - "users_helper.log_remove_fnok", - default: "Fannish Next of Kin Removed: %{user_id}", + "users_helper.log.fnok.removed", user_id: item.fnok_user_id ) end return t( - "users_helper.log_add_fnok", - default: "Fannish Next of Kin Added: %{user_id}", + "users_helper.log.fnok.added", user_id: item.fnok_user_id ) end diff --git a/config/i18n-tasks.yml b/config/i18n-tasks.yml index bc5ac529e46..9d82c6ac91a 100644 --- a/config/i18n-tasks.yml +++ b/config/i18n-tasks.yml @@ -166,10 +166,6 @@ ignore_missing: - users_helper.log_troubleshot - users_helper.log_validated - users_helper.log_warn - - users_helper.log_add_fnok - - users_helper.log_remove_fnok - - users_helper.log_added_as_fnok - - users_helper.log_removed_as_fnok # Files: app/views/admin/_admin_nav.html.erb and app/views/admin_posts/show.html.erb - admin.admin_nav.delete # File: app/views/admin/admin_invitations/find.html.erb diff --git a/config/locales/helpers/en.yml b/config/locales/helpers/en.yml index f8bcc4e26be..ff62fb3bc96 100644 --- a/config/locales/helpers/en.yml +++ b/config/locales/helpers/en.yml @@ -20,3 +20,10 @@ en: user: bookmark: Bookmarker approval status work: Creator approval status + users_helper: + log: + fnok: + added: "Fannish Next of Kin Added: %{user_id}" + removed: "Fannish Next of Kin Removed: %{user_id}" + added_as: "Added as Fannish Next of Kin for: %{user_id}" + removed_as: "Removed as Fannish Next of Kin for: %{user_id}" From 2ec994a48712376c9c0520fc0e71ed73d10b91c5 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Mon, 21 Aug 2023 23:00:37 +0200 Subject: [PATCH 23/30] Simplify fnok name action --- app/helpers/users_helper.rb | 36 +++++++++++------------------------ config/locales/helpers/en.yml | 8 ++++---- 2 files changed, 15 insertions(+), 29 deletions(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 9c0434b0457..6b3067fad97 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -123,31 +123,7 @@ def log_item_action_name(item, user) action = item.action if [ArchiveConfig.ACTION_ADD_FNOK, ArchiveConfig.ACTION_REMOVE_FNOK].include?(action) - if item.fnok_user_id == user.id - if action == ArchiveConfig.ACTION_REMOVE_FNOK - return t( - "users_helper.log.fnok.removed_as", - user_id: item.user_id - ) - end - - return t( - "users_helper.log.fnok.added_as", - user_id: item.user_id - ) - end - - if action == ArchiveConfig.ACTION_REMOVE_FNOK - return t( - "users_helper.log.fnok.removed", - user_id: item.fnok_user_id - ) - end - - return t( - "users_helper.log.fnok.added", - user_id: item.fnok_user_id - ) + return fnok_action_name(item, user) end case action @@ -178,6 +154,16 @@ def log_item_action_name(item, user) end end + def fnok_action_name(item, user) + action = item.action == ArchiveConfig.ACTION_REMOVE_FNOK ? "removed" : "added" + target_of_action = item.fnok_user_id == user.id + + return t( + "users_helper.log.fnok.#{target_of_action ? "was" : "has"}_#{action}", + user_id: target_of_action ? item.user_id : item.fnok_user_id + ) + end + # Give the TOS field in the new user form a different name in non-production environments # so that it can be filtered out of the log, for ease of debugging def tos_field_name diff --git a/config/locales/helpers/en.yml b/config/locales/helpers/en.yml index ff62fb3bc96..faa26c562f9 100644 --- a/config/locales/helpers/en.yml +++ b/config/locales/helpers/en.yml @@ -23,7 +23,7 @@ en: users_helper: log: fnok: - added: "Fannish Next of Kin Added: %{user_id}" - removed: "Fannish Next of Kin Removed: %{user_id}" - added_as: "Added as Fannish Next of Kin for: %{user_id}" - removed_as: "Removed as Fannish Next of Kin for: %{user_id}" + has_added: "Fannish Next of Kin Added: %{user_id}" + has_removed: "Fannish Next of Kin Removed: %{user_id}" + was_added: "Added as Fannish Next of Kin for: %{user_id}" + was_removed: "Removed as Fannish Next of Kin for: %{user_id}" From c47cc767c846f7924ceadf648de10e9a8c7c17b4 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Mon, 21 Aug 2023 23:04:44 +0200 Subject: [PATCH 24/30] Clean other I18n keys --- app/helpers/users_helper.rb | 24 ++++++++++++------------ config/i18n-tasks.yml | 13 ------------- config/locales/helpers/en.yml | 12 ++++++++++++ 3 files changed, 24 insertions(+), 25 deletions(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 6b3067fad97..8edd87385f2 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -128,29 +128,29 @@ def log_item_action_name(item, user) case action when ArchiveConfig.ACTION_ACTIVATE - t("users_helper.log_validated", default: "Account Validated") + t("users_helper.log.validated") when ArchiveConfig.ACTION_ADD_ROLE - t("users_helper.log_role_added", default: "Role Added: ") + t("users_helper.log.role_added") when ArchiveConfig.ACTION_REMOVE_ROLE - t("users_helper.log_role_removed", default: "Role Removed: ") + t("users_helper.log.role_removed") when ArchiveConfig.ACTION_SUSPEND - t("users_helper.log_suspended", default: "Suspended until ") + t("users_helper.log.suspended") when ArchiveConfig.ACTION_UNSUSPEND - t("users_helper.log_lift_suspension", default: "Suspension Lifted") + t("users_helper.log.lift_suspension") when ArchiveConfig.ACTION_BAN - t("users_helper.log_ban", default: "Suspended Permanently") + t("users_helper.log.ban") when ArchiveConfig.ACTION_WARN - t("users_helper.log_warn", default: "Warned") + t("users_helper.log.warn") when ArchiveConfig.ACTION_RENAME - t("users_helper.log_rename", default: "Username Changed") + t("users_helper.log.rename") when ArchiveConfig.ACTION_PASSWORD_RESET - t("users_helper.log_password_change", default: "Password Changed") + t("users_helper.log.password_change") when ArchiveConfig.ACTION_NEW_EMAIL - t("users_helper.log_email_change", default: "Email Changed") + t("users_helper.log.email_change") when ArchiveConfig.ACTION_TROUBLESHOOT - t("users_helper.log_troubleshot", default: "Account Troubleshot") + t("users_helper.log.troubleshot") when ArchiveConfig.ACTION_NOTE - t("users_helper.log_note", default: "Note Added") + t("users_helper.log.note") end end diff --git a/config/i18n-tasks.yml b/config/i18n-tasks.yml index 9d82c6ac91a..becfe095384 100644 --- a/config/i18n-tasks.yml +++ b/config/i18n-tasks.yml @@ -153,19 +153,6 @@ ignore_missing: - successfully_sent # should be feedbacks.create.successfully_sent # Files: app/controllers/languages_controller.rb and app/controllers/locales_controller.rb - successfully_added # should be languages.create.successfully_added and locales.create.successfully_added - # File: app/helpers/users_helper.rb - - users_helper.log_ban - - users_helper.log_email_change - - users_helper.log_lift_suspension - - users_helper.log_note - - users_helper.log_password_change - - users_helper.log_rename - - users_helper.log_role_added - - users_helper.log_role_removed - - users_helper.log_suspended - - users_helper.log_troubleshot - - users_helper.log_validated - - users_helper.log_warn # Files: app/views/admin/_admin_nav.html.erb and app/views/admin_posts/show.html.erb - admin.admin_nav.delete # File: app/views/admin/admin_invitations/find.html.erb diff --git a/config/locales/helpers/en.yml b/config/locales/helpers/en.yml index faa26c562f9..7b7bc9f628c 100644 --- a/config/locales/helpers/en.yml +++ b/config/locales/helpers/en.yml @@ -22,6 +22,18 @@ en: work: Creator approval status users_helper: log: + validated: "Account Validated" + role_added: "Role Added: " + role_removed: "Role Removed: " + suspended: "Suspended until " + lift_suspension: "Suspension Lifted" + ban: "Suspended Permanently" + warn: "Warned" + rename: "Username Changed" + password_change: "Password Changed" + email_change: "Email Changed" + troubleshot: "Account Troubleshot" + note: "Note Added" fnok: has_added: "Fannish Next of Kin Added: %{user_id}" has_removed: "Fannish Next of Kin Removed: %{user_id}" From e5ad52ebe00b3c0db2fde228637e990d9a6cba02 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Mon, 21 Aug 2023 23:08:23 +0200 Subject: [PATCH 25/30] Hound --- app/helpers/users_helper.rb | 10 ++++------ app/models/user.rb | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 8edd87385f2..b4a46e5f42d 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -121,10 +121,8 @@ def authored_items(pseud, work_counts = {}, rec_counts = {}) def log_item_action_name(item, user) action = item.action - - if [ArchiveConfig.ACTION_ADD_FNOK, ArchiveConfig.ACTION_REMOVE_FNOK].include?(action) - return fnok_action_name(item, user) - end + + return fnok_action_name(item, user) if [ArchiveConfig.ACTION_ADD_FNOK, ArchiveConfig.ACTION_REMOVE_FNOK].include?(action) case action when ArchiveConfig.ACTION_ACTIVATE @@ -158,8 +156,8 @@ def fnok_action_name(item, user) action = item.action == ArchiveConfig.ACTION_REMOVE_FNOK ? "removed" : "added" target_of_action = item.fnok_user_id == user.id - return t( - "users_helper.log.fnok.#{target_of_action ? "was" : "has"}_#{action}", + t( + "users_helper.log.fnok.#{target_of_action ? 'was' : 'has'}_#{action}", user_id: target_of_action ? item.user_id : item.fnok_user_id ) end diff --git a/app/models/user.rb b/app/models/user.rb index 0649b327834..d53ffe197de 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -177,7 +177,7 @@ def log_if_next_of_kin fnok.user.create_log_item({ action: ArchiveConfig.ACTION_REMOVE_FNOK, fnok_user_id: self.id - }) + }) end end From f394e18703a171499349a5c22fbd74fa61b605bb Mon Sep 17 00:00:00 2001 From: Ceithir Date: Mon, 21 Aug 2023 23:29:31 +0200 Subject: [PATCH 26/30] reviewdog --- app/views/admin/admin_users/_user_history.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/admin/admin_users/_user_history.html.erb b/app/views/admin/admin_users/_user_history.html.erb index 6752483d69b..6e0e80a6fb9 100644 --- a/app/views/admin/admin_users/_user_history.html.erb +++ b/app/views/admin/admin_users/_user_history.html.erb @@ -36,7 +36,7 @@ @log_items.each do |item| %> <%= item.created_at %> - <%= log_item_action_name(item, @user) %><%= item.role.name if item.role %><%= item.enddate if item.enddate %> + <%= log_item_action_name(item, @user) %><%= item.role&.name %><%= item.enddate %> <%= item.note %> <% end %> From 270272c8237bf6c6bd3faaf7cae6db0565a9a1f6 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 22 Aug 2023 00:05:39 +0200 Subject: [PATCH 27/30] Normalize i18n --- config/locales/helpers/en.yml | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/config/locales/helpers/en.yml b/config/locales/helpers/en.yml index 7b7bc9f628c..0ce91162447 100644 --- a/config/locales/helpers/en.yml +++ b/config/locales/helpers/en.yml @@ -22,20 +22,20 @@ en: work: Creator approval status users_helper: log: - validated: "Account Validated" - role_added: "Role Added: " - role_removed: "Role Removed: " - suspended: "Suspended until " - lift_suspension: "Suspension Lifted" - ban: "Suspended Permanently" - warn: "Warned" - rename: "Username Changed" - password_change: "Password Changed" - email_change: "Email Changed" - troubleshot: "Account Troubleshot" - note: "Note Added" + ban: Suspended Permanently + email_change: Email Changed fnok: - has_added: "Fannish Next of Kin Added: %{user_id}" - has_removed: "Fannish Next of Kin Removed: %{user_id}" - was_added: "Added as Fannish Next of Kin for: %{user_id}" - was_removed: "Removed as Fannish Next of Kin for: %{user_id}" + has_added: 'Fannish Next of Kin Added: %{user_id}' + has_removed: 'Fannish Next of Kin Removed: %{user_id}' + was_added: 'Added as Fannish Next of Kin for: %{user_id}' + was_removed: 'Removed as Fannish Next of Kin for: %{user_id}' + lift_suspension: Suspension Lifted + note: Note Added + password_change: Password Changed + rename: Username Changed + role_added: 'Role Added: ' + role_removed: 'Role Removed: ' + suspended: 'Suspended until ' + troubleshot: Account Troubleshot + validated: Account Validated + warn: Warned From 1f6faff02fd5b363cff56d3ae90f0eaa0fe19805 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Tue, 22 Aug 2023 09:25:34 +0200 Subject: [PATCH 28/30] Syntax more compliant with i18n-tasks --- app/helpers/users_helper.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index b4a46e5f42d..478ca2f78d0 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -156,8 +156,10 @@ def fnok_action_name(item, user) action = item.action == ArchiveConfig.ACTION_REMOVE_FNOK ? "removed" : "added" target_of_action = item.fnok_user_id == user.id + key_leaf = "#{target_of_action ? 'was' : 'has'}_#{action}" + t( - "users_helper.log.fnok.#{target_of_action ? 'was' : 'has'}_#{action}", + "users_helper.log.fnok.#{key_leaf}", user_id: target_of_action ? item.user_id : item.fnok_user_id ) end From 3e327e712898ab09632c30ba964b3637824852e3 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Thu, 24 Aug 2023 07:59:26 +0200 Subject: [PATCH 29/30] Wordings --- app/models/user.rb | 4 ++-- spec/controllers/users_controller_spec.rb | 4 ++-- spec/models/user_spec.rb | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index d53ffe197de..20830ef4e88 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -36,7 +36,7 @@ class User < ApplicationRecord has_many :external_authors, dependent: :destroy has_many :external_creatorships, foreign_key: "archivist_id" - before_destroy :log_if_next_of_kin + before_destroy :log_removal_as_next_of_kin has_many :fannish_next_of_kins, dependent: :delete_all, inverse_of: :kin, foreign_key: :kin_id has_one :fannish_next_of_kin, dependent: :destroy @@ -172,7 +172,7 @@ def remove_user_from_kudos Kudo.where(user: self).update_all(user_id: nil) end - def log_if_next_of_kin + def log_removal_as_next_of_kin fannish_next_of_kins.each do |fnok| fnok.user.create_log_item({ action: ArchiveConfig.ACTION_REMOVE_FNOK, diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 1d9c6180928..927aa6cf937 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -67,8 +67,8 @@ fake_login_known_user(user) end - context "user with no activity" do - it "deletes without side effects" do + context "no log items" do + it "successfully destroys and redirects with success message" do login = user.login delete :destroy, params: { id: login } it_redirects_to_with_notice(delete_confirmation_path, "You have successfully deleted your account.") diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index aff7b1ca5d7..49feee04370 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -21,7 +21,7 @@ let(:user) { fnok.kin } let(:person) { fnok.user } - it "explictly removes the relationship" do + it "removes the relationship and creates a log item of the removal" do user_id = user.id user.destroy! expect(person.reload.fannish_next_of_kin).to be_nil From af8114bd22c71252ed39a4b0c41f144f5ef80256 Mon Sep 17 00:00:00 2001 From: Ceithir Date: Thu, 24 Aug 2023 08:12:52 +0200 Subject: [PATCH 30/30] Explicit over succinct --- app/helpers/users_helper.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 478ca2f78d0..8c3b1d38dcc 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -154,13 +154,18 @@ def log_item_action_name(item, user) def fnok_action_name(item, user) action = item.action == ArchiveConfig.ACTION_REMOVE_FNOK ? "removed" : "added" - target_of_action = item.fnok_user_id == user.id - key_leaf = "#{target_of_action ? 'was' : 'has'}_#{action}" + if item.fnok_user_id == user.id + user_id = item.user_id + action_leaf = "was_#{action}" + else + user_id = item.fnok_user_id + action_leaf = "has_#{action}" + end t( - "users_helper.log.fnok.#{key_leaf}", - user_id: target_of_action ? item.user_id : item.fnok_user_id + "users_helper.log.fnok.#{action_leaf}", + user_id: user_id ) end