From 21392f223513fa3567f2fd3401d682d8e070523e Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Tue, 9 Apr 2024 15:44:20 -0400 Subject: [PATCH] deal with users who have judgements when you want to delete them. (#999) --- app/controllers/admin/users_controller.rb | 25 +++++++++++++++---- app/controllers/books_controller.rb | 7 +++++- app/models/judgement.rb | 7 ++---- app/models/user.rb | 11 ++++++++ app/views/admin/users/_form.html.erb | 10 ++++++++ app/views/profiles/show.html.erb | 2 +- config/routes.rb | 3 +++ ...dgements_for_deleted_users_to_anonymous.rb | 9 +++++++ ..._add_partial_unique_index_to_judgements.rb | 6 +++++ db/schema.rb | 4 +-- test/fixtures/judgements.yml | 2 +- test/models/judgement_test.rb | 11 +++++++- test/models/user_test.rb | 14 +++++++++++ 13 files changed, 95 insertions(+), 16 deletions(-) create mode 100644 db/migrate/20240409185546_assign_judgements_for_deleted_users_to_anonymous.rb create mode 100644 db/migrate/20240409185548_add_partial_unique_index_to_judgements.rb diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index ce85fc204..ffa617cb5 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -4,7 +4,7 @@ module Admin class UsersController < Admin::AdminController - before_action :set_user, only: [ :show, :edit, :update, :destroy ] + before_action :set_user, only: [ :show, :edit, :update, :destroy, :assign_judgements_to_anonymous_user ] # GET /admin/users # GET /admin/users.json @@ -101,13 +101,28 @@ def update # DELETE /admin/users/1 # DELETE /admin/users/1.json def destroy - @user.destroy - respond_to do |format| - format.html { redirect_to admin_users_url, notice: 'User account was successfully deleted.' } - format.json { head :no_content } + if @user.destroy + respond_to do |format| + format.html { redirect_to admin_users_url, notice: 'User account was successfully deleted.' } + format.json { head :no_content } + end + else + respond_to do |format| + format.html { render :edit } + format.json { render json: @user.errors, status: :unprocessable_entity } + end end end + def assign_judgements_to_anonymous_user + @user.judgements.each do |j| + j.user = nil + j.save! + end + + redirect_to admin_user_path @user, notice: 'All judgements assigned to anonymous user.' + end + private # Use callbacks to share common setup or constraints between actions. diff --git a/app/controllers/books_controller.rb b/app/controllers/books_controller.rb index efdef3315..981f323e7 100644 --- a/app/controllers/books_controller.rb +++ b/app/controllers/books_controller.rb @@ -37,7 +37,12 @@ def show unique_judge_ids = @book.query_doc_pairs.joins(:judgements) .distinct.pluck(:user_id) unique_judge_ids.each do |judge_id| - judge = User.find(judge_id) unless judge_id.nil? + begin + judge = User.find(judge_id) unless judge_id.nil? + rescue ActiveRecord::RecordNotFound + puts 'got a nil' + judge = nil + end @leaderboard_data << { judge: judge.nil? ? 'anonymous' : judge.fullname, judgements: @book.judgements.where(user: judge).count } @stats_data << { diff --git a/app/models/judgement.rb b/app/models/judgement.rb index 4f6d10bb4..3dc22cf8e 100644 --- a/app/models/judgement.rb +++ b/app/models/judgement.rb @@ -17,7 +17,7 @@ # Indexes # # index_judgements_on_query_doc_pair_id (query_doc_pair_id) -# index_judgements_on_user_id_and_query_doc_pair_id (user_id,query_doc_pair_id) +# index_judgements_on_user_id_and_query_doc_pair_id (user_id,query_doc_pair_id) UNIQUE # # Foreign Keys # @@ -27,10 +27,7 @@ class Judgement < ApplicationRecord belongs_to :query_doc_pair belongs_to :user, optional: true - # rubocop:disable Rails/UniqueValidationWithoutIndex - validates :user_id, :uniqueness => { :scope => :query_doc_pair_id } - # rubocop:enable Rails/UniqueValidationWithoutIndex - + validates :user_id, :uniqueness => { :scope => :query_doc_pair_id }, unless: -> { user_id.nil? } validates :rating, presence: true, unless: :rating_not_required? diff --git a/app/models/user.rb b/app/models/user.rb index ae69f87b8..b377b781d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -107,6 +107,9 @@ class User < ApplicationRecord has_many :scores, dependent: :destroy + has_many :judgements, + dependent: :restrict_with_error + has_many :case_metadata, class_name: 'CaseMetadatum', dependent: :destroy @@ -146,6 +149,7 @@ def terms_and_conditions? before_create :set_defaults before_destroy :check_team_ownership_before_removing!, prepend: true before_destroy :check_scorer_ownership_before_removing!, prepend: true + before_destroy :check_judgements_before_removing!, prepend: true def check_team_ownership_before_removing! owned_teams.each do |team| @@ -156,6 +160,13 @@ def check_team_ownership_before_removing! end end + def check_judgements_before_removing! + if judgements.count.positive? + errors.add(:base, "Please reassign ownership of the #{judgements.count} judgements." ) + throw(:abort) + end + end + def check_scorer_ownership_before_removing! owned_scorers.each do |scorer| if shared_scorers.include?(scorer) diff --git a/app/views/admin/users/_form.html.erb b/app/views/admin/users/_form.html.erb index 6a2cabd4b..759675803 100644 --- a/app/views/admin/users/_form.html.erb +++ b/app/views/admin/users/_form.html.erb @@ -134,3 +134,13 @@ <% end %> <% end %> + +
+ +

Assign Judgements to Anonymous User

+

+ If you need to delete a user then you need to assign their judgements over to the anonymous user. This user has <%=@user.judgements.count %> judgements total. +

+<%= button_to 'Assign Judgements to Anonymous User', assign_judgements_to_anonymous_user_admin_user_path(@user), method: :post, class: 'btn btn-sm btn-danger' %> + +

diff --git a/app/views/profiles/show.html.erb b/app/views/profiles/show.html.erb index ece8b86e0..6ce61e8eb 100644 --- a/app/views/profiles/show.html.erb +++ b/app/views/profiles/show.html.erb @@ -40,7 +40,7 @@

Ratings

-

n/a

+

<%= current_user.judgements.count %>

diff --git a/config/routes.rb b/config/routes.rb index 730bf4a11..2a28f21d7 100755 --- a/config/routes.rb +++ b/config/routes.rb @@ -100,6 +100,9 @@ resources :users do resource :lock, only: [ :update ], module: :users resource :pulse, only: [ :show ], module: :users + member do + post :assign_judgements_to_anonymous_user + end end resources :communal_scorers resources :announcements do diff --git a/db/migrate/20240409185546_assign_judgements_for_deleted_users_to_anonymous.rb b/db/migrate/20240409185546_assign_judgements_for_deleted_users_to_anonymous.rb new file mode 100644 index 000000000..b4c2d58f5 --- /dev/null +++ b/db/migrate/20240409185546_assign_judgements_for_deleted_users_to_anonymous.rb @@ -0,0 +1,9 @@ +class AssignJudgementsForDeletedUsersToAnonymous < ActiveRecord::Migration[7.1] + def change + judgements = Judgement.left_outer_joins(:user).where(users: { id: nil }) + judgements.each do |j| + j.user = nil + j.save! + end + end +end diff --git a/db/migrate/20240409185548_add_partial_unique_index_to_judgements.rb b/db/migrate/20240409185548_add_partial_unique_index_to_judgements.rb new file mode 100644 index 000000000..f8c219975 --- /dev/null +++ b/db/migrate/20240409185548_add_partial_unique_index_to_judgements.rb @@ -0,0 +1,6 @@ +class AddPartialUniqueIndexToJudgements < ActiveRecord::Migration[7.1] + def change + remove_index :judgements, [:user_id, :query_doc_pair_id] + add_index :judgements, [:user_id, :query_doc_pair_id], unique: true, where: 'user_id IS NOT NULL' + end +end diff --git a/db/schema.rb b/db/schema.rb index 54f7e4251..87d8dbaf8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2024_03_08_204637) do +ActiveRecord::Schema[7.1].define(version: 2024_04_09_190305) do create_table "active_storage_attachments", charset: "utf8mb4", collation: "utf8mb4_bin", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false @@ -155,7 +155,7 @@ t.boolean "judge_later", default: false t.text "explanation" t.index ["query_doc_pair_id"], name: "index_judgements_on_query_doc_pair_id" - t.index ["user_id", "query_doc_pair_id"], name: "index_judgements_on_user_id_and_query_doc_pair_id" + t.index ["user_id", "query_doc_pair_id"], name: "index_judgements_on_user_id_and_query_doc_pair_id", unique: true end create_table "permissions", id: :integer, charset: "utf8mb3", force: :cascade do |t| diff --git a/test/fixtures/judgements.yml b/test/fixtures/judgements.yml index d5c38b8f3..70e5dc452 100644 --- a/test/fixtures/judgements.yml +++ b/test/fixtures/judgements.yml @@ -15,7 +15,7 @@ # Indexes # # index_judgements_on_query_doc_pair_id (query_doc_pair_id) -# index_judgements_on_user_id_and_query_doc_pair_id (user_id,query_doc_pair_id) +# index_judgements_on_user_id_and_query_doc_pair_id (user_id,query_doc_pair_id) UNIQUE # # Foreign Keys # diff --git a/test/models/judgement_test.rb b/test/models/judgement_test.rb index 28931af45..24a48f758 100644 --- a/test/models/judgement_test.rb +++ b/test/models/judgement_test.rb @@ -17,7 +17,7 @@ # Indexes # # index_judgements_on_query_doc_pair_id (query_doc_pair_id) -# index_judgements_on_user_id_and_query_doc_pair_id (user_id,query_doc_pair_id) +# index_judgements_on_user_id_and_query_doc_pair_id (user_id,query_doc_pair_id) UNIQUE # # Foreign Keys # @@ -42,6 +42,15 @@ class JudgementTest < ActiveSupport::TestCase judgement2 = Judgement.create(user: user2, query_doc_pair: query_doc_pair, rating: 1.0) assert judgement2.save end + + test 'However multiple anonymous judgements is okay' do + judgement = Judgement.create(user: nil, query_doc_pair: query_doc_pair, rating: 4.4) + assert judgement.save + + duplicate_judgement = Judgement.create(user: nil, query_doc_pair: query_doc_pair, rating: 1.0) + assert duplicate_judgement.save + assert_not duplicate_judgement.errors.include?(:user_id) + end end describe 'unrateable attribute behavior' do let(:query_doc_pair) { query_doc_pairs(:one) } diff --git a/test/models/user_test.rb b/test/models/user_test.rb index fa787a885..6168f3346 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -246,6 +246,12 @@ class UserTest < ActiveSupport::TestCase end it 'prevents a user who owns a scorer shared with a team from being deleted' do + # need to null out the judgements first + random.judgements.each do |j| + j.user = nil + j.save! + end + random.destroy assert_not random.destroyed? assert random.errors.full_messages_for(:base).include?('Please remove the scorer Scorer for sharing from the team before deleting this user.') @@ -281,6 +287,14 @@ class UserTest < ActiveSupport::TestCase it 'prevents access to the books because the user is not part of a team' do assert_nil user_without_book_access.books_involved_with.where(id: book_of_comedy_films).first end + + it 'prevents you from deleting a user that has judgements' do + assert user_with_book_access.judgements.size.positive? + + assert_not user_with_book_access.destroy + assert user_with_book_access.errors.include?(:base) + assert_not user_with_book_access.destroyed? + end end describe 'User accessing Search Endpoints' do