Prevent publication of duplicate theses with holds

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).

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

app/mailers/report_mailer.rb

@@ -1,32 +1,33 @@
 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
 
   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
 
   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

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

app/views/report_mailer/registrar_import_email.html.erb

@@ -17,6 +17,31 @@
   <li>New degree periods: <%= @results[:new_degree_periods].count %></li>
 </ul>
 
+<% if @multiple_hold_users.any? %>
+  <p style="font-weight: bold;">WARNING: Users with active or expired holds on existing theses</p>
+
+  <p>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:</p>
+
+  <ul>
+    <% @multiple_hold_users.each do |hold_case| %>
+      <li>
+        <strong><%= hold_case[:user].name %></strong> (ID: <%= hold_case[:user].id %>)
+        <ul>
+          <li>New thesis created: <%= link_to "Thesis ##{hold_case[:new_thesis].id}", thesis_url(hold_case[:new_thesis]) %></li>
+          <li>Other theses with active or expired holds:
+            <ul>
+              <% hold_case[:other_theses_with_holds].each do |other_thesis| %>
+                <li><%= link_to "Thesis ##{other_thesis.id}", thesis_url(other_thesis) %></li>
+              <% end %>
+            </ul>
+          </li>
+        </ul>
+      </li>
+    <% end %>
+  </ul>
+<% end %>
+
 <% if @results[:errors].any? %>
   <p>The following errors require processor attention:</p>
 

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) %>
           </li>
+          <li>
+            <%= 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? %>
+              <ul style="margin: 0.5em 0 0.75em 0; padding-left: 1.25em;">
+                <% @other_theses_with_holds.each do |other_thesis| %>
+                  <li><%= link_to "Thesis ##{other_thesis.id}", thesis_path(other_thesis) %></li>
+                <% end %>
+              </ul>
+            <% end %>
+          </li>
           <li>
             <%= f.input :accession_number?, as: :string,
                                         readonly: true,

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)),

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

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'

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

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