diff --git a/app/controllers/thesis_controller.rb b/app/controllers/thesis_controller.rb index 54ae61eb..b44498f6 100644 --- a/app/controllers/thesis_controller.rb +++ b/app/controllers/thesis_controller.rb @@ -53,7 +53,7 @@ def publication_statuses end def edit - @thesis = Thesis.includes([degrees: :degree_type]).find(params[:id]) + @thesis = Thesis.includes([{ degrees: :degree_type }]).find(params[:id]) @thesis.association(:advisors).add_to_target(Advisor.new) if @thesis.advisors.count.zero? end @@ -100,7 +100,7 @@ def start end def update - @thesis = Thesis.includes([authors: :user]).find(params[:id]) + @thesis = Thesis.includes([{ authors: :user }]).find(params[:id]) if @thesis.update(thesis_params) flash[:success] = "#{@thesis.title} has been updated." ReceiptMailer.receipt_email(@thesis, current_user).deliver_later @@ -114,6 +114,7 @@ def process_theses @thesis = Thesis.with_attached_files.includes([:departments, { authors: [:user], degrees: [:degree_type] }]).find(params[:id]) + @other_theses_with_holds = @thesis.other_theses_with_holds.to_a end def process_theses_update @@ -139,7 +140,7 @@ def process_theses_update end def proquest_export_preview - @theses = Thesis.includes([authors: :user]).ready_for_proquest_export + @theses = Thesis.includes([{ authors: :user }]).ready_for_proquest_export end # TODO: we need to generate and send a budget report CSV for partially harvested theses (spec TBD). diff --git a/app/jobs/registrar_import_job.rb b/app/jobs/registrar_import_job.rb index c68e135d..f5a7c4e8 100644 --- a/app/jobs/registrar_import_job.rb +++ b/app/jobs/registrar_import_job.rb @@ -6,6 +6,7 @@ class RegistrarImportJob < ActiveJob::Base def perform(registrar) results = { read: 0, processed: 0, new_users: 0, new_theses: 0, updated_theses: 0, new_degrees: [], new_depts: [], new_degree_periods: [], errors: [] } + new_theses = [] # Track newly created theses to detect duplicate theses with holds CSV.new(registrar.graduation_list.download, headers: true).each.with_index(1) do |row, i| Rails.logger.info("Parsing row #{i}") @@ -39,7 +40,12 @@ def perform(registrar) # Set whodunnit for thesis transaction PaperTrail.request.whodunnit = 'registrar' thesis = Thesis.create_or_update_from_csv(user, degree, department, grad_date, row) - thesis.new_thesis? ? results[:new_theses] += 1 : results[:updated_theses] += 1 + if thesis.new_thesis? + results[:new_theses] += 1 + new_theses << thesis + else + results[:updated_theses] += 1 + end logger.info("Thesis is #{thesis.inspect}") rescue RuntimeError e = "Multiple theses found for author #{user.name} for term #{grad_date}, requires Processor attention. CSV row ##{i}: #{row.inspect}" @@ -55,11 +61,36 @@ def perform(registrar) author.set_graduated_from_csv(row) results[:processed] += 1 end + + multiple_hold_users = collect_users_with_multiple_hold_theses(new_theses) + Rails.logger.info(results.to_s) - ReportMailer.registrar_import_email(registrar, results).deliver_later + ReportMailer.registrar_import_email(registrar, results, multiple_hold_users).deliver_later results end + def collect_users_with_multiple_hold_theses(new_theses) + multiple_hold_users = [] + + new_theses.each do |thesis| + other_theses_with_holds = thesis.other_theses_with_holds.includes(:users).to_a + thesis.users.each do |user| + user_with_other_theses_with_holds = other_theses_with_holds.select do |other_thesis| + other_thesis.users.any? { |u| u.id == user.id } + end + next if user_with_other_theses_with_holds.empty? + + multiple_hold_users << { + user: user, + new_thesis: thesis, + other_theses_with_holds: user_with_other_theses_with_holds + } + end + end + + multiple_hold_users + end + # The thesis model sets the day of the month to 1 if only supplied a month # and a year during thesis creation, which means in practice we have to # assume that the day is always 1 (because this will be true for any theses diff --git a/app/mailers/report_mailer.rb b/app/mailers/report_mailer.rb index 9b15344f..bce7caa2 100644 --- a/app/mailers/report_mailer.rb +++ b/app/mailers/report_mailer.rb @@ -1,12 +1,13 @@ class ReportMailer < ApplicationMailer - def registrar_import_email(registrar, results) + def registrar_import_email(registrar, results, multiple_hold_users = []) return unless ENV.fetch('DISABLE_ALL_EMAIL', 'true') == 'false' # allows PR builds to disable emails @registrar = registrar @results = results - mail(from: "MIT Libraries <#{ENV['ETD_APP_EMAIL']}>", - to: ENV['THESIS_ADMIN_EMAIL'], - cc: ENV['MAINTAINER_EMAIL'], + @multiple_hold_users = multiple_hold_users + mail(from: "MIT Libraries <#{ENV.fetch('ETD_APP_EMAIL', nil)}>", + to: ENV.fetch('THESIS_ADMIN_EMAIL', nil), + cc: ENV.fetch('MAINTAINER_EMAIL', nil), subject: 'Registrar data import summary') end @@ -14,9 +15,9 @@ def publication_results_email(results) return unless ENV.fetch('DISABLE_ALL_EMAIL', 'true') == 'false' # allows PR builds to disable emails @results = results - mail(from: "MIT Libraries <#{ENV['ETD_APP_EMAIL']}>", - to: ENV['THESIS_ADMIN_EMAIL'], - cc: ENV['MAINTAINER_EMAIL'], + mail(from: "MIT Libraries <#{ENV.fetch('ETD_APP_EMAIL', nil)}>", + to: ENV.fetch('THESIS_ADMIN_EMAIL', nil), + cc: ENV.fetch('MAINTAINER_EMAIL', nil), subject: 'DSpace publication results summary') end @@ -24,9 +25,9 @@ def preservation_results_email(results) return unless ENV.fetch('DISABLE_ALL_EMAIL', 'true') == 'false' # allows PR builds to disable emails @results = results - mail(from: "MIT Libraries <#{ENV['ETD_APP_EMAIL']}>", - to: ENV['THESIS_ADMIN_EMAIL'], - cc: ENV['MAINTAINER_EMAIL'], + mail(from: "MIT Libraries <#{ENV.fetch('ETD_APP_EMAIL', nil)}>", + to: ENV.fetch('THESIS_ADMIN_EMAIL', nil), + cc: ENV.fetch('MAINTAINER_EMAIL', nil), subject: 'Archivematica preservation submission results summary') end end diff --git a/app/models/thesis.rb b/app/models/thesis.rb index 2c2b7390..27408949 100644 --- a/app/models/thesis.rb +++ b/app/models/thesis.rb @@ -170,6 +170,21 @@ def active_holds? holds.map { |h| h.status.in? %w[active expired] }.any? end + # Returns an ActiveRecord::Relation of other theses for the same user(s) + # that have active or expired holds. + # Used to alert processors about pre-existing holds prior to publication. + def other_theses_with_holds + user_ids = users.ids + return Thesis.none if user_ids.empty? + + Thesis.joins(:users, :holds) + .where(users: { id: user_ids }) + .merge(Hold.active_or_expired) + .where.not(id: id) + .distinct + .order(:id) + end + # Returns a true/false value if there are any associated advisors. def advisors? advisors.any? @@ -204,6 +219,7 @@ def evaluate_status metadata_complete?, no_issues_found?, no_active_holds?, + no_other_theses_with_holds?, authors_graduated?, departments_have_dspace_name?, degrees_have_types?, @@ -237,6 +253,17 @@ def no_active_holds? !active_holds? end + # Mirrors no_active_holds? but for other theses by the same user(s). + # Publication review is blocked when another thesis for the same user has + # an active or expired hold. + # + # Releasing a Hold is the expected way for a Hold to be removed. Expiration signals to + # processors to review the Hold to see if it can be released, but expiration is not intended as a + # signal that the Thesis is publishable. + def no_other_theses_with_holds? + other_theses_with_holds.none? + end + # This inverts the issues_found field, so that the checks inside the # update_status method below are all written the same way. # The UI will still rely on the issues_found field directly, as its diff --git a/app/views/report_mailer/registrar_import_email.html.erb b/app/views/report_mailer/registrar_import_email.html.erb index 17180bb7..fd9c698f 100644 --- a/app/views/report_mailer/registrar_import_email.html.erb +++ b/app/views/report_mailer/registrar_import_email.html.erb @@ -17,6 +17,31 @@
  • New degree periods: <%= @results[:new_degree_periods].count %>
    • +<% if @multiple_hold_users.any? %> +

    WARNING: Users with active or expired holds on existing theses

    + +

    The following users have active or expired holds on existing theses and are receiving new theses from this import. + Processors should review these cases to resolve holds during publication review:

    + + +<% end %> + <% if @results[:errors].any? %>

    The following errors require processor attention:

    diff --git a/app/views/thesis/process_theses.html.erb b/app/views/thesis/process_theses.html.erb index 2507ffaf..063868ec 100644 --- a/app/views/thesis/process_theses.html.erb +++ b/app/views/thesis/process_theses.html.erb @@ -47,6 +47,20 @@ hint_html: { style: 'display: block' }, hint: link_to('See details in admin interface', admin_thesis_path(f.object), target: :_blank) %> +
  • + <%= f.input :no_other_theses_with_holds?, as: :string, + readonly: true, + label_html: { style: 'width: 50%' }, + input_html: { class: 'disabled', style: 'width: 40%', value: @other_theses_with_holds.none? ? 'Yes' : 'No' }, + label: 'Related holds resolved?' %> + <% if @other_theses_with_holds.any? %> + + <% end %> +
  • <%= f.input :accession_number?, as: :string, readonly: true, diff --git a/test/controllers/thesis_controller_test.rb b/test/controllers/thesis_controller_test.rb index 2fa12c9e..04d50e92 100644 --- a/test/controllers/thesis_controller_test.rb +++ b/test/controllers/thesis_controller_test.rb @@ -488,6 +488,30 @@ def attach_files_to_records(tr, th) assert_response :success end + test 'processing form shows links to other theses with active or expired holds' do + sign_in users(:processor) + Author.create!(user: users(:yo), thesis: theses(:downloaded), graduation_confirmed: true) + + get thesis_process_path(theses(:one)) + + assert_response :success + assert_select 'label', text: 'Related holds resolved?' + assert_select 'input[readonly][value=?]', 'No' + assert_select "a[href='#{thesis_path(theses(:with_hold))}']", text: "Thesis ##{theses(:with_hold).id}" + assert_select "a[href='#{thesis_path(theses(:downloaded))}']", text: "Thesis ##{theses(:downloaded).id}" + end + + test 'processing form does not show other-theses-with-holds section when there are no matches' do + sign_in users(:processor) + + get thesis_process_path(theses(:released_hold)) + + assert_response :success + assert_select 'label', text: 'Related holds resolved?' + assert_select 'input[readonly][value=?]', 'Yes' + assert_select 'a', text: /Thesis #\d+/, count: 0 + end + # ~~~~~~~~~~~~~~~~~~~ submitting thesis processing form ~~~~~~~~~~~~~~~~~~~~~ test 'anonymous users cannot submit thesis processing form' do patch thesis_process_update_path(theses(:one)), diff --git a/test/fixtures/authors.yml b/test/fixtures/authors.yml index 45de7c4f..8f9649b7 100644 --- a/test/fixtures/authors.yml +++ b/test/fixtures/authors.yml @@ -58,12 +58,12 @@ seven: proquest_allowed: false review: - user: yo + user: third thesis: publication_review graduation_confirmed: true eight: - user: basic + user: second thesis: publication_review_except_hold graduation_confirmed: true @@ -100,7 +100,7 @@ fourteen: graduation_confirmed: true fifteen: - user: yo + user: admin thesis: doctor graduation_confirmed: true proquest_allowed: false diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index d5b69f72..7647f6f8 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -107,3 +107,12 @@ thesis_admin: given_name: 'Thesis A.' surname: 'Robot' display_name: 'Thesis A. Robot' + +isolated: + uid: 'isolated' + email: 'isolated@example.com' + kerberos_id: 'isolated' + given_name: 'Isolated' + surname: 'Student' + display_name: 'Isolated Student' + preferred_name: 'Student, Isolated' diff --git a/test/integration/admin/admin_hold_test.rb b/test/integration/admin/admin_hold_test.rb index 85178d5e..4b9c106d 100644 --- a/test/integration/admin/admin_hold_test.rb +++ b/test/integration/admin/admin_hold_test.rb @@ -141,7 +141,7 @@ def teardown patch admin_hold_path(hold), params: { hold: { status: 'released' } } hold.reload assert_equal 'released', hold.status - assert_equal Date.today.strftime('%Y-%m-%d'), Date.parse(hold.date_released).strftime('%Y-%m-%d') + assert_equal Date.current.strftime('%Y-%m-%d'), Date.parse(hold.date_released).strftime('%Y-%m-%d') end test 'hold release date is the most recent released status change' do diff --git a/test/jobs/registrar_import_job_test.rb b/test/jobs/registrar_import_job_test.rb index 66250d26..bc049385 100644 --- a/test/jobs/registrar_import_job_test.rb +++ b/test/jobs/registrar_import_job_test.rb @@ -67,4 +67,52 @@ class RegistrarImportJobTest < ActiveJob::TestCase assert_equal 'registrar', User.first.versions.first.whodunnit assert_equal 'registrar', Thesis.last.versions.first.whodunnit end + + test 'collect_users_with_multiple_hold_theses returns empty array when no users have other held theses' do + job = RegistrarImportJob.new + user = User.create!(uid: 'test_fresh_user', email: 'fresh@example.com') + new_thesis = Thesis.new( + title: 'Fresh Thesis', + graduation_year: 2025, + graduation_month: 'September', + departments: [departments(:one)] + ) + new_thesis.users << user + new_thesis.save! + + result = job.collect_users_with_multiple_hold_theses([new_thesis]) + assert_equal [], result + assert_equal Array, result.class + end + + test 'collect_users_with_multiple_hold_theses flags a thesis for a single user with other held theses' do + job = RegistrarImportJob.new + new_thesis = theses(:one) + + result = job.collect_users_with_multiple_hold_theses([new_thesis]) + + assert_equal 1, result.length + assert_equal users(:yo), result.first[:user] + assert_equal new_thesis, result.first[:new_thesis] + assert_equal Array, result.first[:other_theses_with_holds].class + assert_includes result.first[:other_theses_with_holds], theses(:with_hold) + end + + test 'collect_users_with_multiple_hold_theses flags only co-authors with other held theses' do + job = RegistrarImportJob.new + new_thesis = theses(:one) + + # Create a co-author with no other held theses to ensure per-user filtering works. + coauthor = User.create!(uid: 'coauthor_test_user', email: 'coauthor@example.com') + Author.create!(user: coauthor, thesis: new_thesis, graduation_confirmed: true) + + result = job.collect_users_with_multiple_hold_theses([new_thesis]) + + assert_equal 1, result.length + assert_equal users(:yo), result.first[:user] + assert_equal new_thesis, result.first[:new_thesis] + assert_equal Array, result.first[:other_theses_with_holds].class + assert_includes result.first[:other_theses_with_holds], theses(:with_hold) + assert_not_includes result.map { |row| row[:user] }, coauthor + end end diff --git a/test/mailers/report_mailer_test.rb b/test/mailers/report_mailer_test.rb index e0ab2370..382c1d0b 100644 --- a/test/mailers/report_mailer_test.rb +++ b/test/mailers/report_mailer_test.rb @@ -62,4 +62,51 @@ class ReportMailerTest < ActionMailer::TestCase assert_match 'Couldn't find Thesis with 'id'=9999999999999', email.body.to_s end end + + test 'registrar import email does not show multiple hold users alert when list is empty' do + ClimateControl.modify DISABLE_ALL_EMAIL: 'false' do + registrar = registrar(:valid) + results = { read: 0, processed: 0, new_users: 0, new_theses: 1, updated_theses: 0, new_degrees: [], new_depts: [], + new_degree_periods: [], errors: [] } + email = ReportMailer.registrar_import_email(registrar, results, []) + + assert_emails 1 do + email.deliver_now + end + + assert_no_match 'Users with active or expired holds on existing theses', email.body.to_s + end + end + + test 'registrar import email shows alert and thesis links for multiple hold users' do + ClimateControl.modify DISABLE_ALL_EMAIL: 'false' do + user = users(:yo) + new_thesis = theses(:one) + other_thesis_1 = theses(:with_hold) + other_thesis_2 = theses(:downloaded) + + registrar = registrar(:valid) + results = { read: 0, processed: 0, new_users: 0, new_theses: 1, updated_theses: 0, new_degrees: [], new_depts: [], + new_degree_periods: [], errors: [] } + + multiple_hold_users = [ + { + user: user, + new_thesis: new_thesis, + other_theses_with_holds: [other_thesis_1, other_thesis_2] + } + ] + + email = ReportMailer.registrar_import_email(registrar, results, multiple_hold_users) + + assert_emails 1 do + email.deliver_now + end + + assert_match 'Users with active or expired holds on existing theses', email.body.to_s + assert_match "Thesis ##{new_thesis.id}", email.body.to_s + assert_match "Thesis ##{other_thesis_1.id}", email.body.to_s + assert_match "Thesis ##{other_thesis_2.id}", email.body.to_s + end + end end diff --git a/test/models/hold_test.rb b/test/models/hold_test.rb index e3d114f0..66e2b2e2 100644 --- a/test/models/hold_test.rb +++ b/test/models/hold_test.rb @@ -97,21 +97,21 @@ class HoldTest < ActiveSupport::TestCase test 'ends_today_or_before scope returns a hold which ends today' do hold = holds(:valid) - hold.date_end = Date.today + hold.date_end = Date.current hold.save assert Hold.ends_today_or_before.pluck(:id).include?(hold.id) end test 'ends_today_or_before scope returns a hold which ends in the past' do hold = holds(:valid) - hold.date_end = Date.today - 1 + hold.date_end = Date.current - 1 hold.save assert Hold.ends_today_or_before.pluck(:id).include?(hold.id) end test 'ends_today_or_before scope does not return a hold which ends in the future' do hold = holds(:valid) - hold.date_end = Date.today + 1 + hold.date_end = Date.current + 1 hold.save assert_not Hold.ends_today_or_before.pluck(:id).include?(hold.id) end diff --git a/test/models/marc_test.rb b/test/models/marc_test.rb index f5def816..b945ce14 100644 --- a/test/models/marc_test.rb +++ b/test/models/marc_test.rb @@ -25,7 +25,7 @@ class MarcTest < ActiveSupport::TestCase thesis.authors << authors(:review) marc = Marc.new(thesis) assert_equal('Robot, Basic', marc.record['100']['a']) - assert_equal('Yobot, Yo', marc.record['700']['a']) + assert_equal(authors(:review).user.name, marc.record['700']['a']) end test 'control008 follows spec' do diff --git a/test/models/thesis_test.rb b/test/models/thesis_test.rb index ff08468d..66e7fbf3 100644 --- a/test/models/thesis_test.rb +++ b/test/models/thesis_test.rb @@ -27,7 +27,7 @@ require 'test_helper' class ThesisTest < ActiveSupport::TestCase - def attach_file_with_purpose_to(thesis, purpose = 'thesis_pdf') + def attach_file_with_purpose_to(thesis, _purpose = 'thesis_pdf') file = Rails.root.join('test', 'fixtures', 'files', 'a_pdf.pdf') thesis.files.attach(io: File.open(file), filename: 'a_pdf.pdf') thesis.files.last.purpose = 'thesis_pdf' @@ -552,7 +552,7 @@ def attach_file_with_purpose_to(thesis, purpose = 'thesis_pdf') old_value = Thesis.without_files.count assert_includes Thesis.without_files, thesis thesis = attach_file_with_purpose_to(thesis) - assert_equal old_value-1, Thesis.without_files.count + assert_equal old_value - 1, Thesis.without_files.count assert_not_includes Thesis.without_files, thesis end @@ -706,7 +706,7 @@ def attach_file_with_purpose_to(thesis, purpose = 'thesis_pdf') assert_equal 'Publication review', thesis.publication_status thesis = attach_file_with_purpose_to(thesis, 'thesis_pdf') assert_equal 'Not ready for publication', thesis.publication_status - assert_equal ['thesis_pdf', 'thesis_pdf'], thesis.files.map(&:purpose) + assert_equal %w[thesis_pdf thesis_pdf], thesis.files.map(&:purpose) assert_equal false, thesis.one_thesis_pdf? end @@ -881,7 +881,7 @@ def attach_file_with_purpose_to(thesis, purpose = 'thesis_pdf') thesis.save thesis.reload assert_equal 'Not ready for publication', thesis.publication_status - assert_equal ['Bachelor', 'Doctoral'], thesis.degrees.map(&:degree_type).pluck(:name) + assert_equal %w[Bachelor Doctoral], thesis.degrees.map(&:degree_type).pluck(:name) assert_equal false, thesis.required_fields? end @@ -952,16 +952,37 @@ def attach_file_with_purpose_to(thesis, purpose = 'thesis_pdf') end test 'Setting an existing hold to "released" can put the thesis into "Publication review" status' do + thesis = theses(:publication_review_except_hold) + assert_equal 'Not ready for publication', thesis.publication_status + assert_equal 1, thesis.holds.count + + # Ensure author has no other holds + thesis.authors.first.update!(user: users(:isolated)) + thesis.reload + assert_equal true, thesis.no_other_theses_with_holds? + + hold = thesis.holds.first + hold.status = 'released' + hold.save + thesis.reload + assert_equal true, thesis.no_active_holds? + assert_equal true, thesis.no_other_theses_with_holds? + assert_equal 'Publication review', thesis.publication_status + end + + test 'Setting an existing hold to "released" does not put thesis into publication review when other theses for the user still have holds' do thesis = theses(:publication_review_except_hold) thesis.save thesis.reload assert_equal 'Not ready for publication', thesis.publication_status assert_equal 1, thesis.holds.count + hold = thesis.holds.first hold.status = 'released' hold.save thesis.reload - assert_equal 'Publication review', thesis.publication_status + assert_equal false, thesis.no_other_theses_with_holds? + assert_equal 'Not ready for publication', thesis.publication_status end test 'Adding a new hold will set the thesis status back to "Not ready for publication"' do @@ -1157,7 +1178,7 @@ def attach_file_with_purpose_to(thesis, purpose = 'thesis_pdf') assert_includes Thesis.without_sips, thesis thesis.submission_information_packages.create - assert_equal orig_count-1, Thesis.without_sips.count + assert_equal orig_count - 1, Thesis.without_sips.count assert_not_includes Thesis.without_sips, thesis end @@ -1167,7 +1188,7 @@ def attach_file_with_purpose_to(thesis, purpose = 'thesis_pdf') assert_not_includes Thesis.with_sips, thesis thesis.submission_information_packages.create - assert_equal orig_count+1, Thesis.with_sips.count + assert_equal orig_count + 1, Thesis.with_sips.count assert_includes Thesis.with_sips, thesis end @@ -1180,7 +1201,7 @@ def attach_file_with_purpose_to(thesis, purpose = 'thesis_pdf') # published status has sip thesis.submission_information_packages.create - assert_equal orig_count-1, Thesis.published_without_sips.count + assert_equal orig_count - 1, Thesis.published_without_sips.count assert_not_includes Thesis.published_without_sips, thesis # no published status has sip @@ -1380,7 +1401,7 @@ def attach_file_with_purpose_to(thesis, purpose = 'thesis_pdf') # Confirm that the grad dates are different, so we are testing at least two degree periods before Sept '22 assert_not_equal wrong_term.grad_date, another_wrong_term.grad_date - + # Confirm that both fixtures have a grad date prior to Sept '22 assert wrong_term.grad_date < Date.parse('September 2022') assert another_wrong_term.grad_date < Date.parse('September 2022') @@ -1395,7 +1416,7 @@ def attach_file_with_purpose_to(thesis, purpose = 'thesis_pdf') # Confirm that the grad dates are different, so we are testing at least two degree periods after Sept '22 assert_not_equal correct_term.grad_date, another_correct_term.grad_date - + # Confirm that both fixtures have a grad date after to Sept '22 assert correct_term.grad_date > Date.parse('September 2022') assert another_correct_term.grad_date > Date.parse('September 2022') @@ -1528,7 +1549,7 @@ def attach_file_with_purpose_to(thesis, purpose = 'thesis_pdf') assert_equal 'Publication review', t.publication_status assert t.unique_filenames?(t) - attach_file_with_purpose_to(t, purpose = 'signature_page') + attach_file_with_purpose_to(t, 'signature_page') t.save t.reload assert_not_equal 'Publication review', t.publication_status @@ -1542,10 +1563,78 @@ def attach_file_with_purpose_to(thesis, purpose = 'thesis_pdf') assert_equal 'Publication review', t.publication_status assert t.unique_filenames?(t) - attach_file_with_purpose_to(t, purpose = 'signature_page') + attach_file_with_purpose_to(t, 'signature_page') t.save t.reload assert_not_equal 'Publication review', t.publication_status refute t.unique_filenames?(t) end + + # Tests for other_theses_with_holds method + test 'other_theses_with_holds includes other theses with active holds that share a user' do + assert_includes theses(:one).users, users(:yo) + assert_includes theses(:with_hold).users, users(:yo) + + result = theses(:one).other_theses_with_holds + + assert_includes result, theses(:with_hold) + end + + test 'other_theses_with_holds includes other theses with expired holds that share a user' do + Author.create!(user: users(:yo), thesis: theses(:downloaded), graduation_confirmed: true) + assert_includes theses(:one).users, users(:yo) + assert_includes theses(:downloaded).users, users(:yo) + + result = theses(:one).other_theses_with_holds + + assert_includes result, theses(:downloaded) + end + + test 'other_theses_with_holds excludes theses with only released holds' do + Author.create!(user: users(:yo), thesis: theses(:released_hold), graduation_confirmed: true) + assert_includes theses(:one).users, users(:yo) + assert_includes theses(:released_hold).users, users(:yo) + + result = theses(:one).other_theses_with_holds + + assert_not_includes result, theses(:released_hold) + end + + test 'other_theses_with_holds excludes the current thesis from results' do + result = theses(:with_hold).other_theses_with_holds + + assert_not_includes result, theses(:with_hold) + end + + test 'other_theses_with_holds returns distinct theses when a thesis has multiple matching holds' do + Author.create!(user: users(:yo), thesis: theses(:downloaded), graduation_confirmed: true) + assert_includes theses(:one).users, users(:yo) + assert_includes theses(:downloaded).users, users(:yo) + + matching_hold_count = theses(:downloaded).holds.where(status: %i[active expired]).count + assert matching_hold_count > 1 + + result_ids = theses(:one).other_theses_with_holds.pluck(:id) + assert_includes result_ids, theses(:downloaded).id + assert_equal result_ids.uniq, result_ids + end + + test 'no_other_theses_with_holds? returns false when other theses with active or expired holds exist' do + Author.create!(user: users(:yo), thesis: theses(:downloaded), graduation_confirmed: true) + + assert_equal false, theses(:one).no_other_theses_with_holds? + end + + test 'publication status stays not ready when another thesis for same user has active hold' do + thesis = theses(:publication_review) + thesis.users = [users(:yo)] + thesis.authors.each { |a| a.update!(graduation_confirmed: true) } + + thesis.save + thesis.reload + + + assert_equal false, thesis.no_other_theses_with_holds? + assert_equal 'Not ready for publication', thesis.publication_status + end end