diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index a319a032716..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) - @log_items = @user.log_items.sort_by(&:created_at).reverse + log_items end # POST admin/users/update @@ -75,6 +75,12 @@ 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, + 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) @@ -84,11 +90,17 @@ 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, + 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 - @log_items = @user.log_items.sort_by(&:created_at).reverse + log_items render :show end end @@ -168,4 +180,8 @@ def activate redirect_to action: :show end end + + 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 b6a6b173a2b..8c3b1d38dcc 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -119,34 +119,56 @@ def authored_items(pseud, work_counts = {}, rec_counts = {}) items.html_safe 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') + def log_item_action_name(item, user) + action = item.action + + return fnok_action_name(item, user) if [ArchiveConfig.ACTION_ADD_FNOK, ArchiveConfig.ACTION_REMOVE_FNOK].include?(action) + + case action + when ArchiveConfig.ACTION_ACTIVATE + t("users_helper.log.validated") + when ArchiveConfig.ACTION_ADD_ROLE + t("users_helper.log.role_added") + when ArchiveConfig.ACTION_REMOVE_ROLE + t("users_helper.log.role_removed") + when ArchiveConfig.ACTION_SUSPEND + t("users_helper.log.suspended") + when ArchiveConfig.ACTION_UNSUSPEND + t("users_helper.log.lift_suspension") + when ArchiveConfig.ACTION_BAN + t("users_helper.log.ban") + when ArchiveConfig.ACTION_WARN + t("users_helper.log.warn") + when ArchiveConfig.ACTION_RENAME + t("users_helper.log.rename") + when ArchiveConfig.ACTION_PASSWORD_RESET + t("users_helper.log.password_change") + when ArchiveConfig.ACTION_NEW_EMAIL + t("users_helper.log.email_change") + when ArchiveConfig.ACTION_TROUBLESHOOT + t("users_helper.log.troubleshot") + when ArchiveConfig.ACTION_NOTE + t("users_helper.log.note") end end + def fnok_action_name(item, user) + action = item.action == ArchiveConfig.ACTION_REMOVE_FNOK ? "removed" : "added" + + 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.#{action_leaf}", + user_id: 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/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/app/models/user.rb b/app/models/user.rb index 6d51edcd8db..20830ef4e88 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_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 @@ -171,6 +172,15 @@ def remove_user_from_kudos Kudo.where(user: self).update_all(user_id: nil) end + def log_removal_as_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/app/views/admin/admin_users/_user_history.html.erb b/app/views/admin/admin_users/_user_history.html.erb index 0c19185ec7f..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.action) %><%= 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 %> diff --git a/config/config.yml b/config/config.yml index e52da538648..c9e100261e9 100644 --- a/config/config.yml +++ b/config/config.yml @@ -428,6 +428,8 @@ ACTION_PASSWORD_RESET: 8 ACTION_NEW_EMAIL: 9 ACTION_TROUBLESHOOT: 10 ACTION_NOTE: 11 +ACTION_ADD_FNOK: 12 +ACTION_REMOVE_FNOK: 13 # Elasticsearch index prefix 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 f8bcc4e26be..0ce91162447 100644 --- a/config/locales/helpers/en.yml +++ b/config/locales/helpers/en.yml @@ -20,3 +20,22 @@ en: user: bookmark: Bookmarker approval status work: Creator approval status + users_helper: + log: + 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}' + 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 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..9d0adb23dee --- /dev/null +++ b/db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb @@ -0,0 +1,51 @@ +class AddFnokUserIdToLogItems < ActiveRecord::Migration[6.1] + def up + 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, + 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 \\ + --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 + add_index :log_items, :fnok_user_id + end + end + + def down + 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 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 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/features/admins/users/admin_fnok.feature b/features/admins/users/admin_fnok.feature index d1e80ebfd4d..45096b1cbe5 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." + 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" @@ -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 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" 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 the history table should show that "libby" was removed as next of kin + When I go to the user administration page for "libby" + 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 diff --git a/spec/controllers/admin/admin_users_controller_spec.rb b/spec/controllers/admin/admin_users_controller_spec.rb index 13b750cef07..ef301528a2b 100644 --- a/spec/controllers/admin/admin_users_controller_spec.rb +++ b/spec/controllers/admin/admin_users_controller_spec.rb @@ -226,6 +226,39 @@ 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) + 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 + 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) + expect(log_item.admin_id).to eq(admin.id) + expect(log_item.note).to eq("Change made by #{admin.login}") + end end describe "POST #update_status" do diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 1fdb7159a74..927aa6cf937 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 "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.") + expect(User.find_by(login: login)).to be_nil + end + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index cdb6a1b55fd..49feee04370 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 "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 + 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