From 9737b19db809098da0f89dca7bba962f3d969873 Mon Sep 17 00:00:00 2001 From: jazairi <16103405+jazairi@users.noreply.github.com> Date: Thu, 9 Jul 2026 17:09:10 -0700 Subject: [PATCH] Prevent publication of duplicate theses with holds Why these changes are being introduced: When an author changes degree periods, an additional copy of the thesis is created during registrar load. Any Holds on the original are not transferred to the new copy of the thesis. Relevant ticket(s): - [ETD-689](https://mitlibraries.atlassian.net/browse/ETD-689) How this addresses that need: - Adds a cross-thesis lookup for active or expired Holds. - Integrated said lookup in to the pre-publication checks (`evaluate_status`), such that publication review is blocked if a Thesis has a User with another Thesis that has an active or expired Hold. - During registrar data import, checks for pre-existing theses with active/expired holds for a given User and records the results in the report email. - Adds a field to the Thesis Metadata section of the thesis processing form that alerts processors to related theses with holds. Side effects of this change: - Certain fixtures have been adjusted to test the new feature - Some unrelated style changes due to running rubocop --- app/controllers/thesis_controller.rb | 7 +- app/jobs/registrar_import_job.rb | 35 +++++- app/mailers/report_mailer.rb | 21 ++-- app/models/thesis.rb | 27 +++++ .../registrar_import_email.html.erb | 25 ++++ app/views/thesis/process_theses.html.erb | 14 +++ test/controllers/thesis_controller_test.rb | 24 ++++ test/fixtures/authors.yml | 6 +- test/fixtures/users.yml | 9 ++ test/integration/admin/admin_hold_test.rb | 2 +- test/jobs/registrar_import_job_test.rb | 48 ++++++++ test/mailers/report_mailer_test.rb | 47 ++++++++ test/models/hold_test.rb | 6 +- test/models/marc_test.rb | 2 +- test/models/thesis_test.rb | 113 ++++++++++++++++-- 15 files changed, 351 insertions(+), 35 deletions(-) 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