diff --git a/app/controllers/judge_dashboard_controller.rb b/app/controllers/judge_dashboard_controller.rb index aafd1512..20d6711a 100644 --- a/app/controllers/judge_dashboard_controller.rb +++ b/app/controllers/judge_dashboard_controller.rb @@ -35,9 +35,26 @@ def index ) .where(contest_instance_id: @judging_assignments.pluck(:contest_instance_id)) + assigned_round_ids = @assigned_rounds.pluck(:id) + @entry_rankings = EntryRanking.includes(:judging_round) .where(user: current_user) .joins(judging_round: :contest_instance) - .where(judging_rounds: { id: @assigned_rounds.pluck(:id) }) + .where(judging_rounds: { id: assigned_round_ids }) + + # Precompute ranked entry counts per judging_round for this user to avoid N+1 queries + @ranked_counts_by_round = EntryRanking.where( + user: current_user, + judging_round_id: assigned_round_ids + ).group(:judging_round_id).count + + # Precompute finalized status per judging_round to avoid N+1 queries + # Check if any EntryRankings are finalized for each round + finalized_round_ids = EntryRanking.where( + user: current_user, + judging_round_id: assigned_round_ids, + finalized: true + ).select(:judging_round_id).distinct.pluck(:judging_round_id) + @finalized_by_round = finalized_round_ids.index_with { true } end end diff --git a/app/controllers/judging_rounds_controller.rb b/app/controllers/judging_rounds_controller.rb index 0841cce0..c4e9e458 100644 --- a/app/controllers/judging_rounds_controller.rb +++ b/app/controllers/judging_rounds_controller.rb @@ -1,6 +1,6 @@ class JudgingRoundsController < ApplicationController before_action :set_contest_instance - before_action :set_judging_round, only: [ :show, :edit, :update, :destroy, :activate, :deactivate, :complete, :uncomplete, :update_rankings, :finalize_rankings, :send_instructions ] + before_action :set_judging_round, only: [ :show, :edit, :update, :destroy, :activate, :deactivate, :complete, :uncomplete, :update_rankings, :finalize_rankings, :send_instructions, :notify_completed ] before_action :authorize_contest_instance before_action :check_edit_warning, only: [ :edit, :update ] @@ -196,11 +196,11 @@ def update_rankings entry_ranking.save(validate: false) end else - # This is a single entry update (comment update) + # This is a single entry update (comment update or unrank) ranking_data = rankings.first entry_id = ranking_data['entry_id'].presence || ranking_data[:entry_id].presence - if entry_id && ranking_data['rank'].present? + if entry_id && (ranking_data['rank'].present? || ranking_data[:rank].present?) entry = Entry.find(entry_id) entry_ranking = EntryRanking.find_or_initialize_by( entry: entry, @@ -212,6 +212,17 @@ def update_rankings entry_ranking.internal_comments = ranking_data['internal_comments'].presence || ranking_data[:internal_comments].presence || entry_ranking.internal_comments entry_ranking.external_comments = ranking_data['external_comments'].presence || ranking_data[:external_comments].presence || entry_ranking.external_comments entry_ranking.save(validate: false) + elsif entry_id && (ranking_data.key?('rank') || ranking_data.key?(:rank)) && ranking_data['rank'].blank? && ranking_data[:rank].blank? + # Unranking: only when rank was explicitly sent and is blank (not when key omitted) + current_rankings.where(entry_id: entry_id).destroy_all + elsif entry_id && !(ranking_data.key?('rank') || ranking_data.key?(:rank)) + # Comment-only update: rank key omitted; update comments on existing ranking only + entry_ranking = current_rankings.find_by(entry_id: entry_id) + if entry_ranking + entry_ranking.internal_comments = ranking_data['internal_comments'].presence || ranking_data[:internal_comments].presence || entry_ranking.internal_comments + entry_ranking.external_comments = ranking_data['external_comments'].presence || ranking_data[:external_comments].presence || entry_ranking.external_comments + entry_ranking.save(validate: false) + end end end @@ -327,6 +338,56 @@ def finalize_rankings end end + def notify_completed + entry_rankings = EntryRanking.where( + judging_round: @judging_round, + user: current_user + ) + + ranked_count = entry_rankings.count + + if ranked_count < @judging_round.required_entries_count + message = "Please rank at least #{@judging_round.required_entries_count} entries before notifying. You have #{ranked_count} ranked." + respond_to do |format| + format.html { redirect_to judge_dashboard_path, alert: message } + format.turbo_stream do + render turbo_stream: turbo_stream.update('flash', + partial: 'shared/flash', + locals: { message: message, type: 'danger' } + ) + end + end + return + end + + if @container.contact_email.blank? + message = 'No contact email is set for this contest. Please contact an administrator.' + respond_to do |format| + format.html { redirect_to judge_dashboard_path, alert: message } + format.turbo_stream do + render turbo_stream: turbo_stream.update('flash', + partial: 'shared/flash', + locals: { message: message, type: 'danger' } + ) + end + end + return + end + + JudgeCompletedEvaluationsMailer.notify_contact(current_user, @judging_round).deliver_later + + message = 'The contest contact has been notified that you have completed your evaluations.' + respond_to do |format| + format.html { redirect_to judge_dashboard_path, notice: message } + format.turbo_stream do + render turbo_stream: turbo_stream.update('flash', + partial: 'shared/flash', + locals: { message: message, type: 'success' } + ) + end + end + end + private def set_contest_instance diff --git a/app/javascript/controllers/entry_drag_controller.js b/app/javascript/controllers/entry_drag_controller.js index 40daaca9..512ee01e 100644 --- a/app/javascript/controllers/entry_drag_controller.js +++ b/app/javascript/controllers/entry_drag_controller.js @@ -71,6 +71,14 @@ export default class extends Controller { font-weight: 500; } + .card.entry-card-loading { + overflow: visible; + } + + .card.entry-card-loading .entry-loading-overlay { + z-index: 9999; + } + @keyframes fadeIn { from { opacity: 0; } to { opacity: 1; } @@ -88,6 +96,7 @@ export default class extends Controller { this.contestGroupName = `entries-${accordionSection.id}` this.accordionSection = accordionSection + this.slowConnectionTimeouts = new WeakMap() this.initializeSortable() this.initializeCommentListeners() @@ -423,18 +432,23 @@ export default class extends Controller { async addToRanked(event) { if (this.finalizedValue) return - const entryCard = event.target.closest('[data-entry-id]') + const entryCard = event.target.closest('.card[data-entry-id]') if (!entryCard) return - // Move the card to rated entries + // Show loading overlay just like drag-and-drop (before moving so overlay moves with card) + this.addLoadingOverlay(entryCard) + + // Move the card to rated entries (card may be inside a turbo-frame; we move the card node) this.ratedEntriesTarget.appendChild(entryCard) + entryCard.scrollIntoView({ behavior: 'smooth', block: 'nearest' }) + await this.handleSortEnd({ from: this.availableEntriesTarget, to: this.ratedEntriesTarget, item: entryCard }) } async removeFromRanked(event) { if (this.finalizedValue) return - const entryCard = event.target.closest('[data-entry-id]') + const entryCard = event.target.closest('.card[data-entry-id]') if (!entryCard) return // Show confirmation dialog @@ -442,8 +456,13 @@ export default class extends Controller { return } + // Show loading overlay just like drag-and-drop (before moving so overlay moves with card) + this.addLoadingOverlay(entryCard) + // Move the card back to available entries this.availableEntriesTarget.appendChild(entryCard) + entryCard.scrollIntoView({ behavior: 'smooth', block: 'nearest' }) + await this.handleSortEnd({ from: this.ratedEntriesTarget, to: this.availableEntriesTarget, item: entryCard }) } @@ -465,17 +484,16 @@ export default class extends Controller { ` - // Ensure the card has position relative for absolute positioning of overlay + // Ensure the card has position relative and won't clip the overlay element.style.position = 'relative' + element.classList.add('entry-card-loading') element.appendChild(overlay) - // Add show class after a small delay to trigger fade-in - requestAnimationFrame(() => { - overlay.classList.add('show') - }) + // Show overlay immediately so it's visible for button clicks (card is about to move) + overlay.classList.add('show') - // Set up slow connection warning - this.slowConnectionTimeout = setTimeout(() => { + // Set up slow connection warning (per-element so multiple overlays clean up correctly) + const timeoutId = setTimeout(() => { const statusText = overlay.querySelector('.status-text') if (statusText) { statusText.innerHTML = ` @@ -490,6 +508,7 @@ export default class extends Controller { ` } }, 5000) // Show warning after 5 seconds + this.slowConnectionTimeouts.set(element, timeoutId) } // Helper method to show success state on the overlay before removing it @@ -499,9 +518,10 @@ export default class extends Controller { const overlay = element.querySelector('.entry-loading-overlay') if (!overlay) return - if (this.slowConnectionTimeout) { - clearTimeout(this.slowConnectionTimeout) - this.slowConnectionTimeout = null + const timeoutId = this.slowConnectionTimeouts.get(element) + if (timeoutId) { + clearTimeout(timeoutId) + this.slowConnectionTimeouts.delete(element) } overlay.classList.add('success') @@ -521,9 +541,18 @@ export default class extends Controller { const overlay = element.querySelector('.entry-loading-overlay') if (!overlay) return - // Clear slow connection timeout - if (this.slowConnectionTimeout) { - clearTimeout(this.slowConnectionTimeout) + // Clear slow connection timeout for this element + const timeoutId = this.slowConnectionTimeouts.get(element) + if (timeoutId) { + clearTimeout(timeoutId) + this.slowConnectionTimeouts.delete(element) + } + + // Remove entry-card-loading only when the overlay is removed, so the overlay + // keeps overflow/z-index styling until it's gone (avoids clipping during transition/error). + const removeOverlayAndClass = () => { + element.classList.remove('entry-card-loading') + overlay.remove() } if (error) { @@ -542,12 +571,12 @@ export default class extends Controller { // Remove error overlay after 2 seconds setTimeout(() => { overlay.classList.remove('show') - setTimeout(() => overlay.remove(), 300) + setTimeout(removeOverlayAndClass, 300) }, 2000) } else { overlay.classList.remove('show') // Remove overlay after transition completes - setTimeout(() => overlay.remove(), 300) + setTimeout(removeOverlayAndClass, 300) } } } diff --git a/app/mailers/judge_completed_evaluations_mailer.rb b/app/mailers/judge_completed_evaluations_mailer.rb new file mode 100644 index 00000000..359e68b1 --- /dev/null +++ b/app/mailers/judge_completed_evaluations_mailer.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class JudgeCompletedEvaluationsMailer < ApplicationMailer + def notify_contact(judge, judging_round) + @judge = judge + @judging_round = judging_round + @contest_instance = judging_round.contest_instance + @contest_description = @contest_instance.contest_description + @container = @contest_description.container + @judge_email = judge.normalize_email + + to_email = @container.contact_email.presence + unless to_email + raise ArgumentError, "contact_email is required for JudgeCompletedEvaluationsMailer.notify_contact" + end + + subject = "Judge completed evaluations: #{@contest_description.name} - Round #{@judging_round.round_number}" + + mail( + to: to_email, + subject: subject, + reply_to: @judge_email + ) + end +end diff --git a/app/policies/contest_instance_policy.rb b/app/policies/contest_instance_policy.rb index 420f1e60..c55c4f97 100644 --- a/app/policies/contest_instance_policy.rb +++ b/app/policies/contest_instance_policy.rb @@ -55,13 +55,13 @@ def manage? def update_rankings? return false unless user && record - return false unless record.judging_open? + return false unless record.judging_open?(user) record.judges.include?(user) end def finalize_rankings? return false unless user && record - return false unless record.judging_open? + return false unless record.judging_open?(user) record.judges.include?(user) end @@ -92,4 +92,10 @@ def export_entries? def send_instructions? user&.has_container_role?(record.contest_description.container) || axis_mundi? end + + def notify_completed? + return false unless user && record + return false unless record.judging_open?(user) + record.judges.include?(user) + end end diff --git a/app/views/judge_completed_evaluations_mailer/notify_contact.html.erb b/app/views/judge_completed_evaluations_mailer/notify_contact.html.erb new file mode 100644 index 00000000..c2798d73 --- /dev/null +++ b/app/views/judge_completed_evaluations_mailer/notify_contact.html.erb @@ -0,0 +1,30 @@ +
+

+ Judge completed evaluations +

+ +

+ A judge has completed their evaluations for the following contest and round. +

+ +
+

Judge

+

+ Name: <%= @judge.display_name_or_first_name_last_name %>
+ Email: <%= mail_to @judge_email, @judge_email %> +

+
+ +
+

Contest and round

+

+ Contest: <%= @contest_description.name %>
+ Contest instance: <%= @contest_instance.id %> (date open: <%= @contest_instance.date_open&.strftime('%Y-%m-%d') %>)
+ Round: <%= @judging_round.round_number %> +

+
+ +

+ You can reply directly to this message to contact the judge. +

+
diff --git a/app/views/judge_completed_evaluations_mailer/notify_contact.text.erb b/app/views/judge_completed_evaluations_mailer/notify_contact.text.erb new file mode 100644 index 00000000..b847ba7f --- /dev/null +++ b/app/views/judge_completed_evaluations_mailer/notify_contact.text.erb @@ -0,0 +1,10 @@ +A judge has completed their evaluations for the following contest and round. + +Judge: <%= @judge.display_name_or_first_name_last_name %> +Judge email: <%= @judge_email %> + +Contest: <%= @contest_description.name %> +Contest instance: <%= @contest_instance.id %> (date open: <%= @contest_instance.date_open&.strftime('%Y-%m-%d') %>) +Round: <%= @judging_round.round_number %> + +You can reply directly to this message to contact the judge. diff --git a/app/views/judge_dashboard/index.html.erb b/app/views/judge_dashboard/index.html.erb index 2e6213ac..b610b0d9 100644 --- a/app/views/judge_dashboard/index.html.erb +++ b/app/views/judge_dashboard/index.html.erb @@ -93,6 +93,25 @@ <% if assignment.contest_instance.judging_open?(current_user) %> <% assigned_round = @assigned_rounds.find_by(contest_instance_id: assignment.contest_instance.id, active: true) %> <% if assigned_round %> + <% ranked_count = @ranked_counts_by_round[assigned_round.id].to_i %> + <% can_notify = ranked_count >= assigned_round.required_entries_count %> +
+
+
+ <%= button_to 'I\'ve completed my evaluations', + notify_completed_container_contest_description_contest_instance_judging_round_path( + assignment.contest_instance.contest_description.container, + assignment.contest_instance.contest_description, + assignment.contest_instance, + assigned_round + ), + method: :post, + class: "btn btn-success", + disabled: !can_notify, + title: can_notify ? 'Notify the contest contact that you have finished' : "Rank at least #{assigned_round.required_entries_count} entries to enable this button", + data: { turbo_confirm: "Notify the contest contact that you have completed your evaluations for this round? They will receive an email with your contact information." } %> +
+
+ data-entry-drag-finalized-value="<%= @finalized_by_round[assigned_round.id].present? %>">
<%= render partial: 'judge_dashboard/available_entries', diff --git a/config/routes.rb b/config/routes.rb index 0708e827..3e1d322b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -55,6 +55,7 @@ patch :complete patch :uncomplete post :send_instructions + post :notify_completed end post 'update_rankings', on: :member post 'finalize_rankings', on: :member diff --git a/lib/tasks/test.rake b/lib/tasks/test.rake index 789d42b3..320f977c 100644 --- a/lib/tasks/test.rake +++ b/lib/tasks/test.rake @@ -10,7 +10,7 @@ namespace :test do task all: :environment do rspec_success = run_test('RSpec Tests', 'bundle exec rspec') jest_success = run_test('Jest Tests', 'yarn test') - brakeman_success = run_test('Brakeman Security Scan', 'bundle exec brakeman') + brakeman_success = run_test('Brakeman Security Scan', 'bundle exec rake brakeman:run') puts "\n================================================" puts rspec_success ? "\n✅ RSpec Tests passed!" : "\n❌ RSpec Tests failed!" @@ -33,7 +33,7 @@ namespace :test do desc 'Run only Brakeman security scan' task brakeman: :environment do - run_test('Brakeman Security Scan', 'bundle exec brakeman') || exit(1) + run_test('Brakeman Security Scan', 'bundle exec rake brakeman:run') || exit(1) end end diff --git a/spec/controllers/judging_rounds_controller_spec.rb b/spec/controllers/judging_rounds_controller_spec.rb index e837e361..a34fae0f 100644 --- a/spec/controllers/judging_rounds_controller_spec.rb +++ b/spec/controllers/judging_rounds_controller_spec.rb @@ -432,6 +432,40 @@ end end + context 'when user is not authorized' do + before { sign_in judge } + + it 'redirects to root path' do + post :send_instructions, params: { + container_id: container.id, + contest_description_id: contest_description.id, + contest_instance_id: contest_instance.id, + id: judging_round.id + } + expect(response).to redirect_to(root_path) + end + end + + context 'when inactive assignments exist' do + let(:inactive_judge) { create(:user, :with_judge_role, email: 'inactive_judge@umich.edu') } + let!(:inactive_judging_assignment) { create(:judging_assignment, user: inactive_judge, contest_instance: contest_instance) } + let!(:inactive_assignment) do + create(:round_judge_assignment, :inactive, user: inactive_judge, judging_round: judging_round) + end + + it 'only sends to active assignments' do + expect(JudgingInstructionsMailer).to receive(:send_instructions).exactly(3).times + expect(JudgingInstructionsMailer).not_to receive(:send_instructions).with(inactive_assignment, anything) + + post :send_instructions, params: { + container_id: container.id, + contest_description_id: contest_description.id, + contest_instance_id: contest_instance.id, + id: judging_round.id + } + end + end + context 'when email delivery fails' do before do allow(JudgingInstructionsMailer).to receive(:send_instructions).and_call_original @@ -473,38 +507,250 @@ expect(round_assignment1.reload.instructions_sent_at).to be_nil end end + end + + describe 'POST #update_rankings' do + let(:contest_instance) do + create(:contest_instance, + contest_description: contest_description, + date_open: 2.months.ago, + date_closed: 1.month.ago, + active: true) + end + let(:judging_round) do + create(:judging_round, + contest_instance: contest_instance, + round_number: 1, + active: true, + start_date: 1.day.ago, + end_date: 1.day.from_now, + required_entries_count: 3) + end + let(:judge_user) { create(:user, :with_judge_role) } + let!(:judging_assignment) { create(:judging_assignment, user: judge_user, contest_instance: contest_instance, active: true) } + let!(:round_judge_assignment) { create(:round_judge_assignment, user: judge_user, judging_round: judging_round, active: true) } + let!(:entry) { create(:entry, contest_instance: contest_instance, deleted: false) } - context 'when user is not authorized' do - before { sign_in judge } + before { sign_in judge_user } - it 'redirects to root path' do - post :send_instructions, params: { - container_id: container.id, - contest_description_id: contest_description.id, - contest_instance_id: contest_instance.id, - id: judging_round.id - } - expect(response).to redirect_to(root_path) + context 'with only one ranked entry' do + before do + create(:entry_ranking, entry: entry, judging_round: judging_round, user: judge_user, rank: 1) + end + + it 'unranks the entry when a single ranking with rank null is sent' do + expect { + post :update_rankings, params: { + container_id: container.id, + contest_description_id: contest_description.id, + contest_instance_id: contest_instance.id, + id: judging_round.id, + rankings: [{ entry_id: entry.id.to_s, rank: nil, internal_comments: nil, external_comments: nil }] + }, format: :json + }.to change(EntryRanking, :count).by(-1) + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['success']).to eq(true) + expect(EntryRanking.find_by(entry: entry, judging_round: judging_round, user: judge_user)).to be_nil end end + end - context 'when inactive assignments exist' do - let(:inactive_judge) { create(:user, :with_judge_role, email: 'inactive_judge@umich.edu') } - let!(:inactive_judging_assignment) { create(:judging_assignment, user: inactive_judge, contest_instance: contest_instance) } - let!(:inactive_assignment) do - create(:round_judge_assignment, :inactive, user: inactive_judge, judging_round: judging_round) + describe 'POST #notify_completed' do + let(:container_with_contact) { create(:container, contact_email: 'contest_contact@umich.edu') } + let(:contest_description) { create(:contest_description, :active, container: container_with_contact, name: 'Test Contest') } + let(:contest_instance) do + create(:contest_instance, + contest_description: contest_description, + date_open: 2.months.ago, + date_closed: 1.month.ago, + active: true) + end + let(:judging_round) do + create(:judging_round, + contest_instance: contest_instance, + round_number: 1, + active: true, + start_date: 1.day.ago, + end_date: 1.day.from_now, + required_entries_count: 3) + end + let(:judge_user) { create(:user, :with_judge_role, email: 'judge@umich.edu', first_name: 'Jane', last_name: 'Doe') } + let!(:judging_assignment) { create(:judging_assignment, user: judge_user, contest_instance: contest_instance, active: true) } + let!(:round_judge_assignment) { create(:round_judge_assignment, user: judge_user, judging_round: judging_round, active: true) } + let!(:entry1) { create(:entry, contest_instance: contest_instance, deleted: false) } + let!(:entry2) { create(:entry, contest_instance: contest_instance, deleted: false) } + let!(:entry3) { create(:entry, contest_instance: contest_instance, deleted: false) } + + before do + sign_in judge_user + ActiveJob::Base.queue_adapter = :test + end + + def notify_completed_params + { + container_id: container_with_contact.id, + contest_description_id: contest_description.id, + contest_instance_id: contest_instance.id, + id: judging_round.id + } + end + + context 'when judge has ranked exactly the required number of entries' do + before do + create(:entry_ranking, entry: entry1, judging_round: judging_round, user: judge_user, rank: 1) + create(:entry_ranking, entry: entry2, judging_round: judging_round, user: judge_user, rank: 2) + create(:entry_ranking, entry: entry3, judging_round: judging_round, user: judge_user, rank: 3) end - it 'only sends to active assignments' do - expect(JudgingInstructionsMailer).to receive(:send_instructions).exactly(3).times - expect(JudgingInstructionsMailer).not_to receive(:send_instructions).with(inactive_assignment, anything) + it 'enqueues the notification email' do + expect { + post :notify_completed, params: notify_completed_params + }.to have_enqueued_job(ActionMailer::MailDeliveryJob).once + end - post :send_instructions, params: { - container_id: container.id, - contest_description_id: contest_description.id, - contest_instance_id: contest_instance.id, - id: judging_round.id + it 'redirects to judge dashboard with success notice' do + post :notify_completed, params: notify_completed_params + + expect(response).to redirect_to(judge_dashboard_path) + expect(flash[:notice]).to eq('The contest contact has been notified that you have completed your evaluations.') + end + + it 'sends to the container contact email' do + captured_mail = nil + allow(JudgeCompletedEvaluationsMailer).to receive(:notify_contact).and_wrap_original do |method, *args| + captured_mail = method.call(*args) + end + + post :notify_completed, params: notify_completed_params + + expect(captured_mail).to be_a(ActionMailer::MessageDelivery) + mail = captured_mail.message + expect(mail.to).to eq([ 'contest_contact@umich.edu' ]) + expect(mail.reply_to).to eq([ 'judge@umich.edu' ]) + end + end + + context 'when judge has ranked fewer than the required number of entries' do + before do + create(:entry_ranking, entry: entry1, judging_round: judging_round, user: judge_user, rank: 1) + create(:entry_ranking, entry: entry2, judging_round: judging_round, user: judge_user, rank: 2) + # Only 2 ranked, need 3 + end + + it 'does not enqueue the notification email' do + expect { + post :notify_completed, params: notify_completed_params + }.not_to have_enqueued_job(ActionMailer::MailDeliveryJob) + end + + it 'redirects to judge dashboard with alert about needing more rankings' do + post :notify_completed, params: notify_completed_params + + expect(response).to redirect_to(judge_dashboard_path) + expect(flash[:alert]).to include('Please rank at least 3 entries before notifying') + expect(flash[:alert]).to include('You have 2 ranked') + end + end + + context 'when judge has ranked more than the required number' do + let!(:entry4) { create(:entry, contest_instance: contest_instance, deleted: false) } + + before do + create(:entry_ranking, entry: entry1, judging_round: judging_round, user: judge_user, rank: 1) + create(:entry_ranking, entry: entry2, judging_round: judging_round, user: judge_user, rank: 2) + create(:entry_ranking, entry: entry3, judging_round: judging_round, user: judge_user, rank: 3) + create(:entry_ranking, entry: entry4, judging_round: judging_round, user: judge_user, rank: 4) + end + + it 'enqueues the notification email' do + expect { + post :notify_completed, params: notify_completed_params + }.to have_enqueued_job(ActionMailer::MailDeliveryJob).once + end + + it 'redirects to judge dashboard with success notice' do + post :notify_completed, params: notify_completed_params + + expect(response).to redirect_to(judge_dashboard_path) + expect(flash[:notice]).to eq('The contest contact has been notified that you have completed your evaluations.') + end + end + + context 'when container has no contact email' do + let(:container_no_email) { create(:container) } + let(:contest_description_no_email) { create(:contest_description, :active, container: container_no_email) } + let(:contest_instance_no_email) do + create(:contest_instance, + contest_description: contest_description_no_email, + date_open: 2.months.ago, + date_closed: 1.month.ago, + active: true) + end + let(:judging_round_no_email) do + create(:judging_round, + contest_instance: contest_instance_no_email, + round_number: 1, + active: true, + start_date: 1.day.ago, + end_date: 1.day.from_now, + required_entries_count: 2) + end + let!(:judging_assignment_no_email) { create(:judging_assignment, user: judge_user, contest_instance: contest_instance_no_email, active: true) } + let!(:round_judge_assignment_no_email) { create(:round_judge_assignment, user: judge_user, judging_round: judging_round_no_email, active: true) } + let!(:entry_a) { create(:entry, contest_instance: contest_instance_no_email, deleted: false) } + let!(:entry_b) { create(:entry, contest_instance: contest_instance_no_email, deleted: false) } + + before do + container_no_email.update_column(:contact_email, nil) + create(:entry_ranking, entry: entry_a, judging_round: judging_round_no_email, user: judge_user, rank: 1) + create(:entry_ranking, entry: entry_b, judging_round: judging_round_no_email, user: judge_user, rank: 2) + end + + it 'does not enqueue the notification email' do + expect { + post :notify_completed, params: { + container_id: container_no_email.id, + contest_description_id: contest_description_no_email.id, + contest_instance_id: contest_instance_no_email.id, + id: judging_round_no_email.id + } + }.not_to have_enqueued_job(ActionMailer::MailDeliveryJob) + end + + it 'redirects with alert about missing contact email' do + post :notify_completed, params: { + container_id: container_no_email.id, + contest_description_id: contest_description_no_email.id, + contest_instance_id: contest_instance_no_email.id, + id: judging_round_no_email.id } + + expect(response).to redirect_to(judge_dashboard_path) + expect(flash[:alert]).to eq('No contact email is set for this contest. Please contact an administrator.') + end + end + + context 'when user is not the assigned judge' do + let(:other_user) { create(:user, :with_judge_role) } + + before do + sign_in other_user + create(:entry_ranking, entry: entry1, judging_round: judging_round, user: judge_user, rank: 1) + create(:entry_ranking, entry: entry2, judging_round: judging_round, user: judge_user, rank: 2) + create(:entry_ranking, entry: entry3, judging_round: judging_round, user: judge_user, rank: 3) + end + + it 'redirects to root (unauthorized)' do + post :notify_completed, params: notify_completed_params + + expect(response).to redirect_to(root_path) + end + + it 'does not enqueue the notification email' do + expect { + post :notify_completed, params: notify_completed_params + }.not_to have_enqueued_job(ActionMailer::MailDeliveryJob) end end end diff --git a/spec/javascript/controllers/entry_drag_controller.spec.js b/spec/javascript/controllers/entry_drag_controller.spec.js index fefb6335..d79509ae 100644 --- a/spec/javascript/controllers/entry_drag_controller.spec.js +++ b/spec/javascript/controllers/entry_drag_controller.spec.js @@ -128,6 +128,226 @@ describe("EntryDragController", () => { }) }) + describe("addLoadingOverlay", () => { + it("adds overlay with spinner and 'Updating...' text", () => { + const card = document.createElement('div') + card.setAttribute('data-entry-id', '1') + document.body.appendChild(card) + + controller.addLoadingOverlay(card) + + const overlay = card.querySelector('.entry-loading-overlay') + expect(overlay).not.toBeNull() + expect(overlay.querySelector('.spinner-border')).not.toBeNull() + expect(overlay.textContent).toMatch(/Updating\.\.\./) + expect(overlay.classList.contains('show')).toBe(true) + expect(card.classList.contains('entry-card-loading')).toBe(true) + + controller.removeLoadingOverlay(card) + document.body.removeChild(card) + }) + + it("shows overlay immediately with 'show' class", () => { + const card = document.createElement('div') + card.setAttribute('data-entry-id', '1') + document.body.appendChild(card) + + controller.addLoadingOverlay(card) + + const overlay = card.querySelector('.entry-loading-overlay') + expect(overlay.classList.contains('show')).toBe(true) + + controller.removeLoadingOverlay(card) + document.body.removeChild(card) + }) + + it("schedules slow connection warning after 5 seconds", () => { + jest.useFakeTimers() + const card = document.createElement('div') + card.setAttribute('data-entry-id', '1') + document.body.appendChild(card) + + controller.addLoadingOverlay(card) + + const overlay = card.querySelector('.entry-loading-overlay') + const statusText = overlay.querySelector('.status-text') + expect(statusText.textContent).toMatch(/Updating\.\.\./) + + jest.advanceTimersByTime(5000) + + expect(statusText.innerHTML).toMatch(/Slow connection detected/) + expect(statusText.innerHTML).toMatch(/Still working/) + + jest.useRealTimers() + controller.removeLoadingOverlay(card) + document.body.removeChild(card) + }) + + it("does not add duplicate overlay if one already exists", () => { + const card = document.createElement('div') + card.setAttribute('data-entry-id', '1') + document.body.appendChild(card) + + controller.addLoadingOverlay(card) + const overlayCount1 = card.querySelectorAll('.entry-loading-overlay').length + + controller.addLoadingOverlay(card) + const overlayCount2 = card.querySelectorAll('.entry-loading-overlay').length + + expect(overlayCount1).toBe(1) + expect(overlayCount2).toBe(1) + + controller.removeLoadingOverlay(card) + document.body.removeChild(card) + }) + + it("does nothing when element is null", () => { + expect(() => controller.addLoadingOverlay(null)).not.toThrow() + }) + }) + + describe("removeLoadingOverlay", () => { + it("removes overlay and entry-card-loading class", () => { + jest.useFakeTimers() + const card = document.createElement('div') + card.setAttribute('data-entry-id', '1') + document.body.appendChild(card) + + controller.addLoadingOverlay(card) + expect(card.querySelector('.entry-loading-overlay')).not.toBeNull() + expect(card.classList.contains('entry-card-loading')).toBe(true) + + controller.removeLoadingOverlay(card) + jest.advanceTimersByTime(400) // Wait for transition + remove + + expect(card.querySelector('.entry-loading-overlay')).toBeNull() + expect(card.classList.contains('entry-card-loading')).toBe(false) + + jest.useRealTimers() + document.body.removeChild(card) + }) + + it("clears slow connection timeout so callback never runs", () => { + jest.useFakeTimers() + const card = document.createElement('div') + card.setAttribute('data-entry-id', '1') + document.body.appendChild(card) + + controller.addLoadingOverlay(card) + controller.removeLoadingOverlay(card) + jest.advanceTimersByTime(6000) // Past the 5s slow connection threshold + + const overlay = card.querySelector('.entry-loading-overlay') + // Overlay is removed, but if timeout had fired it would have updated statusText before removal + // The key assertion: no warning text was shown (overlay was removed before timeout) + expect(card.querySelector('.entry-loading-overlay')).toBeNull() + + jest.useRealTimers() + document.body.removeChild(card) + }) + + it("shows error state when error is true", () => { + jest.useFakeTimers() + const card = document.createElement('div') + card.setAttribute('data-entry-id', '1') + document.body.appendChild(card) + + controller.addLoadingOverlay(card) + controller.removeLoadingOverlay(card, true) + + const overlay = card.querySelector('.entry-loading-overlay') + expect(overlay.classList.contains('error')).toBe(true) + expect(overlay.textContent).toMatch(/Error updating entry/) + + jest.advanceTimersByTime(2500) // Error overlay removes after 2s + jest.useRealTimers() + document.body.removeChild(card) + }) + + it("does nothing when element is null", () => { + expect(() => controller.removeLoadingOverlay(null)).not.toThrow() + expect(() => controller.removeLoadingOverlay(null, true)).not.toThrow() + }) + }) + + describe("per-element timeout cleanup", () => { + it("tracks timeouts per element so each overlay cleans up correctly", () => { + jest.useFakeTimers() + + const cardA = document.createElement('div') + cardA.setAttribute('data-entry-id', '1') + document.body.appendChild(cardA) + + const cardB = document.createElement('div') + cardB.setAttribute('data-entry-id', '2') + document.body.appendChild(cardB) + + controller.addLoadingOverlay(cardA) + controller.addLoadingOverlay(cardB) + + const overlayA = cardA.querySelector('.entry-loading-overlay') + const overlayB = cardB.querySelector('.entry-loading-overlay') + expect(overlayA.querySelector('.status-text').textContent).toMatch(/Updating\.\.\./) + expect(overlayB.querySelector('.status-text').textContent).toMatch(/Updating\.\.\./) + + // Remove A's overlay - only A's timeout should be cleared + controller.removeLoadingOverlay(cardA) + jest.advanceTimersByTime(400) // Let A's overlay removal complete + + expect(cardA.querySelector('.entry-loading-overlay')).toBeNull() + expect(cardB.querySelector('.entry-loading-overlay')).not.toBeNull() + + // After 5s, only B's slow connection callback should fire + jest.advanceTimersByTime(5000) + + const statusB = cardB.querySelector('.status-text') + expect(statusB.innerHTML).toMatch(/Slow connection detected/) + // Card A had no overlay, so its timeout was cleared and never ran + + jest.useRealTimers() + controller.removeLoadingOverlay(cardB) + document.body.removeChild(cardA) + document.body.removeChild(cardB) + }) + + it("showSuccessOverlay clears timeout for that element only", () => { + jest.useFakeTimers() + + const cardA = document.createElement('div') + cardA.setAttribute('data-entry-id', '1') + cardA.innerHTML = ` +
+
+
Updating...
+
+
+ ` + document.body.appendChild(cardA) + + const cardB = document.createElement('div') + cardB.setAttribute('data-entry-id', '2') + document.body.appendChild(cardB) + + controller.addLoadingOverlay(cardB) + + // Manually add A to the WeakMap to simulate A having had a timeout (showSuccessOverlay + // is called when overlay already exists; we're testing that it clears A's timeout) + const timeoutA = setTimeout(() => {}, 5000) + controller.slowConnectionTimeouts.set(cardA, timeoutA) + + controller.showSuccessOverlay(cardA) + + // A's timeout should be cleared; B's should still be scheduled + expect(controller.slowConnectionTimeouts.has(cardA)).toBe(false) + expect(controller.slowConnectionTimeouts.has(cardB)).toBe(true) + + jest.useRealTimers() + controller.removeLoadingOverlay(cardB) + document.body.removeChild(cardA) + document.body.removeChild(cardB) + }) + }) + describe("showSuccessOverlay", () => { it("replaces overlay content with success state and 'Ranking recorded' text", () => { const card = document.createElement('div') diff --git a/spec/policies/contest_instance_policy_spec.rb b/spec/policies/contest_instance_policy_spec.rb index 4e270aaf..f03e7e58 100644 --- a/spec/policies/contest_instance_policy_spec.rb +++ b/spec/policies/contest_instance_policy_spec.rb @@ -1,129 +1,103 @@ +# frozen_string_literal: true + require 'rails_helper' RSpec.describe ContestInstancePolicy do subject { described_class.new(user, contest_instance) } - let(:container) { create(:container) } - let(:contest_description) { create(:contest_description, :active, container: container) } - let(:contest_instance) { create(:contest_instance, contest_description: contest_description) } - let(:container_admin_role) { create(:role, kind: 'Collection Administrator') } - let(:axis_mundi_role) { create(:role, kind: 'Axis Mundi') } - - context 'for a user with container role' do - let(:user) { create(:user) } + let(:contest_instance) { create(:contest_instance, date_open: 10.days.ago, date_closed: 5.days.ago) } - before do - create(:assignment, container: container, user: user, role: container_admin_role) - end + # Judging-open gating: policies that require user to be a judge AND judging_open?(user) + shared_examples 'judging_open? gated judge action' do |action| + context 'when user is nil' do + let(:user) { nil } - it { is_expected.to permit_action(:index) } - it { is_expected.to permit_action(:show) } - it { is_expected.to permit_action(:create) } - it { is_expected.to permit_action(:update) } - it { is_expected.to permit_action(:destroy) } - it { is_expected.to permit_action(:manage_judges) } - it { is_expected.to permit_action(:send_instructions) } - - describe 'view_judging_results?' do - it 'permits viewing results' do - expect(subject.view_judging_results?).to be true + it "forbids #{action}" do + expect(subject).not_to permit_action(action) end end - end - context 'for a user with axis mundi role' do - let(:user) { create(:user) } + context 'when user is not a judge for this instance' do + let(:user) { create(:user, :with_judge_role) } - before do - create(:user_role, user: user, role: axis_mundi_role) + it "forbids #{action}" do + expect(subject).not_to permit_action(action) + end end - it { is_expected.to permit_action(:index) } - it { is_expected.to permit_action(:show) } - it { is_expected.to permit_action(:create) } - it { is_expected.to permit_action(:update) } - it { is_expected.to permit_action(:destroy) } - it { is_expected.to permit_action(:manage_judges) } - it { is_expected.to permit_action(:send_instructions) } - - describe 'view_judging_results?' do - it 'permits viewing results' do - expect(subject.view_judging_results?).to be true + context 'when user is a judge but not assigned to the current round' do + let(:user) { create(:user, :with_judge_role) } + let!(:judging_round) do + create(:judging_round, + contest_instance: contest_instance, + active: true, + start_date: 4.days.ago, + end_date: 2.days.from_now + ) end - end - end - context 'for a judge' do - let(:user) { create(:user, :with_judge_role) } + before do + create(:judging_assignment, user: user, contest_instance: contest_instance, active: true) + # No round_judge_assignment -> judging_open?(user) is false + end - before do - create(:judging_assignment, user: user, contest_instance: contest_instance) + it "forbids #{action}" do + expect(subject).not_to permit_action(action) + end end - it { is_expected.not_to permit_action(:index) } - it { is_expected.not_to permit_action(:show) } - it { is_expected.not_to permit_action(:create) } - it { is_expected.not_to permit_action(:update) } - it { is_expected.not_to permit_action(:destroy) } - it { is_expected.not_to permit_action(:manage_judges) } - it { is_expected.not_to permit_action(:send_instructions) } - - describe 'view_judging_results?' do - context 'when judge evaluations are complete' do - before do - create(:judging_round, contest_instance: contest_instance, completed: true) - end - - it 'permits viewing results' do - expect(subject.view_judging_results?).to be true - end + context 'when there is no current judging round' do + let(:user) { create(:user, :with_judge_role) } + let!(:judging_round) do + create(:judging_round, + contest_instance: contest_instance, + active: true, + start_date: 1.day.from_now, + end_date: 3.days.from_now + ) end - context 'when judge evaluations are not complete' do - before do - create(:judging_round, contest_instance: contest_instance, completed: false) - end + before do + create(:judging_assignment, user: user, contest_instance: contest_instance, active: true) + create(:round_judge_assignment, user: user, judging_round: judging_round, active: true) + end - it 'does not permit viewing results' do - expect(subject.view_judging_results?).to be false - end + it "forbids #{action}" do + expect(subject).not_to permit_action(action) end end - end - context 'for a regular user' do - let(:user) { create(:user) } + context 'when user is a judge and judging is open for them' do + let(:user) { create(:user, :with_judge_role) } + let!(:judging_round) do + create(:judging_round, + contest_instance: contest_instance, + active: true, + start_date: 4.days.ago, + end_date: 2.days.from_now + ) + end - it { is_expected.not_to permit_action(:index) } - it { is_expected.not_to permit_action(:show) } - it { is_expected.not_to permit_action(:create) } - it { is_expected.not_to permit_action(:update) } - it { is_expected.not_to permit_action(:destroy) } - it { is_expected.not_to permit_action(:manage_judges) } - it { is_expected.not_to permit_action(:send_instructions) } + before do + create(:judging_assignment, user: user, contest_instance: contest_instance, active: true) + create(:round_judge_assignment, user: user, judging_round: judging_round, active: true) + end - describe 'view_judging_results?' do - it 'does not permit viewing results' do - expect(subject.view_judging_results?).to be false + it "permits #{action}" do + expect(subject).to permit_action(action) end end end - context 'when user is nil' do - let(:user) { nil } + describe '#notify_completed?' do + include_examples 'judging_open? gated judge action', :notify_completed + end - it { is_expected.not_to permit_action(:index) } - it { is_expected.not_to permit_action(:show) } - it { is_expected.not_to permit_action(:create) } - it { is_expected.not_to permit_action(:update) } - it { is_expected.not_to permit_action(:destroy) } - it { is_expected.not_to permit_action(:manage_judges) } - it { is_expected.not_to permit_action(:send_instructions) } + describe '#update_rankings?' do + include_examples 'judging_open? gated judge action', :update_rankings + end - describe 'view_judging_results?' do - it 'does not permit viewing results' do - expect(subject.view_judging_results?).to be false - end - end + describe '#finalize_rankings?' do + include_examples 'judging_open? gated judge action', :finalize_rankings end end diff --git a/spec/system/judge_dashboard_spec.rb b/spec/system/judge_dashboard_spec.rb index 1b45b40d..8eec75d5 100644 --- a/spec/system/judge_dashboard_spec.rb +++ b/spec/system/judge_dashboard_spec.rb @@ -302,5 +302,59 @@ def last_entry_title expect(page).to have_css('[data-entry-drag-target="counter"]', text: '1/7') end end + + it 'shows I\'ve completed my evaluations button disabled when required count not met', :js do + visit judge_dashboard_path + find('.accordion-button').click + sleep 0.5 + + within('.accordion-collapse.show') do + expect(page).to have_button("I've completed my evaluations", disabled: true) + end + end + + it 'shows I\'ve completed my evaluations button enabled when required count is met', :js do + # Create exactly required_entries_count (7) rankings for this judge and round + entries_to_rank = contest_instance.current_round_entries.limit(judging_round.required_entries_count) + entries_to_rank.each_with_index do |entry_to_rank, index| + create(:entry_ranking, + entry: entry_to_rank, + user: judge, + judging_round: judging_round, + rank: index + 1) + end + + visit judge_dashboard_path + find('.accordion-button').click + sleep 0.5 + + within('.accordion-collapse.show') do + expect(page).to have_button("I've completed my evaluations", disabled: false) + end + end + + it 'notifies contact when judge clicks I\'ve completed my evaluations and confirms', :js do + entries_to_rank = contest_instance.current_round_entries.limit(judging_round.required_entries_count) + entries_to_rank.each_with_index do |entry_to_rank, index| + create(:entry_ranking, + entry: entry_to_rank, + user: judge, + judging_round: judging_round, + rank: index + 1) + end + + visit judge_dashboard_path + find('.accordion-button').click + sleep 0.5 + + within('.accordion-collapse.show') do + accept_confirm do + click_button "I've completed my evaluations" + end + end + + expect(page).to have_current_path(judge_dashboard_path) + expect(page).to have_content('The contest contact has been notified that you have completed your evaluations.') + end end end diff --git a/yarn.lock b/yarn.lock index b70326fc..4d7b2025 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3075,9 +3075,9 @@ lodash.debounce@^4.0.8: integrity sha512-FT1yDzDYEoYWhnSGnpE/4Kj1fLZkDFyqRb7fNt6FdYOSxlUWAtp42Eh6Wb0rGIv/m9Bgo7x4GhQbm5Ys4SG5ow== lodash@^4.17.21: - version "4.17.21" - resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.21.tgz#679591c564c3bffaae8454cf0b3df370c3d6911c" - integrity sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg== + version "4.17.23" + resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.23.tgz#f113b0378386103be4f6893388c73d0bde7f2c5a" + integrity sha512-LgVTMpQtIopCi79SJeDiP0TfWi5CNEc/L/aRdTh3yIvmZXTnheWpKjSZhnvMl8iXbC1tFg9gdHHDMLoV7CnG+w== lru-cache@^5.1.1: version "5.1.1"