Allow reviewed exceptions to related-hold blocks

app/controllers/thesis_controller.rb

@@ -1,3 +1,27 @@
+# Coordinates thesis submission by authors and processing/publication by library staff.
+#
+# Main workflow components:
+# - Author submission: start routes to new/edit. Create builds a minimum record and delegates
+#   to update, which saves submission details and queues a receipt email.
+# - Staff processing: select lists unpublished theses with files. Process_thesis loads the
+#   editing form. Process_thesis_update saves metadata and related-hold exceptions together.
+#   Attachment removal resets file approval and refreshes originating transfer counts.
+#   Related theses sharing any author with active or expired holds block publication unless staff
+#   explicitly confirm they represent distinct works.
+# - Staff review: publication_statuses filters records by term/status. Deduplicate lists
+#   records with coauthor metadata for manual review, rather than merging records itself.
+# - DSpace publication: publish_preview and publish_to_dspace share publication_candidates,
+#   rechecking holds on review-status records for the selected term before queuing preparation.
+# - ProQuest export: proquest_export_preview shows model-selected candidates. Proquest_export
+#   queues separate partial/full harvest batches. Publication errors can also be reset here. We
+#   no longer export to ProQuest, but we may again in the future.
+#
+# Responsibility boundaries:
+# - Authentication and CanCan abilities determine which actions and records a user may access.
+# - Thesis owns readiness/status calculations; RelatedHoldException owns exception validation.
+#   This controller coordinates saves and keeps exception parameters out of author updates.
+# - Private helpers prepare filters, parameters, attachment cleanup and queue feedback.
+#   Publication/export jobs perform background work; queuing is not confirmation of completion.
 class ThesisController < ApplicationController
   before_action :require_user
   before_action :authenticate_user!
@@ -39,13 +63,15 @@ def create
   def deduplicate
     # Get array of defined terms where theses have coauthors
     @terms = defined_terms Thesis.where.not('coauthors = ?', '')
+
     # Filter relevant theses by selected term from querystring
     @thesis = filter_theses_by_term Thesis.where.not('coauthors = ?', '').includes(%i[departments users])
   end
 
   def publication_statuses
     @terms = defined_terms Thesis.all
     @publication_statuses = Thesis.all.pluck(:publication_status).uniq.sort
+
     # Filter relevant theses by selected term from querystring
     term_filtered = filter_theses_by_term Thesis.all.includes(:degrees, :departments,
                                                               :users).includes(degrees: :degree_type)
@@ -67,18 +93,16 @@ def publish_to_dspace
     if params[:graduation] == 'all' || params[:graduation].nil?
       flash[:warning] = 'Please select a term before attempting to publish theses to DSpace@MIT.'
     else
-      theses = publication_candidates
-      DspacePublicationPrepJob.perform_later(theses)
-
-      flash[:success] = 'The theses you selected have been added to the publication queue. ' \
-                        'Status updates are not immediate.'
+      # Queue publication preparation asynchronously
+      queue_publication(publication_candidates)
     end
     redirect_to thesis_select_path(params: { graduation: params[:graduation] })
   end
 
   def select
     # Get array of defined terms where unpublished theses have files attached
     @terms = defined_terms Thesis.joins(:files_attachments).group(:id).where('publication_status != ?', 'Published')
+
     # Filter relevant theses by selected term from querystring
     @thesis = filter_theses_by_term Thesis.joins(:files_attachments).includes(%i[departments users]).group(:id).where(
       'publication_status != ?', 'Published'
@@ -110,13 +134,22 @@ def update
     redirect_to thesis_confirm_path
   end
 
+  # Loads the processing form with currently held related theses and saved exceptions.
+  # Previously waived records remain visible after their holds are released so processors can revoke
+  # the exceptions.
+  # The combined list is deduplicated because a reviewed related thesis may still have holds.
   def process_thesis
     @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
+    @reviewed_related_thesis_ids = @thesis.related_hold_exceptions.pluck(:related_thesis_id)
+    @related_theses_for_review = (@other_theses_with_holds + Thesis.where(id: @reviewed_related_thesis_ids)).uniq
   end
 
+  # Saves metadata and explicit related-hold decisions together through the model's transaction,
+  # which recalculates readiness using the updated exceptions.
+  # Retries once if an attachment marked for removal disappears during the update.
   def process_thesis_update
     thesis = Thesis.find(params[:id])
 
@@ -129,7 +162,7 @@ def process_thesis_update
     retried_for_missing_attachment = false
 
     begin
-      updated = thesis.update(thesis_params)
+      updated = thesis.update_with_related_hold_reviews(thesis_params, related_hold_review_params)
     rescue ActiveRecord::RecordNotFound => e
       raise unless missing_deleted_attachment_race?(e) && !retried_for_missing_attachment
 
@@ -189,6 +222,8 @@ def defined_terms(records)
     records.pluck(:grad_date).uniq.sort
   end
 
+  # Collects filenames and originating transfer IDs before attachments are removed, for success links and count refreshes.
+  # Skips attachments already deleted since the form was loaded.
   def deleted_file_list
     list = []
     return list unless thesis_params['files_attachments_attributes']
@@ -207,6 +242,10 @@ def deleted_file_list
     list
   end
 
+  # Removes submitted deletion rows for attachments that no longer exist, mutating the request
+  # parameters.
+  # Runs before the update and again on retry to avoid failing nested attachment assignment on stale
+  # IDs.
   def drop_stale_deleted_attachment_rows!
     return unless params[:thesis]&.[](:files_attachments_attributes)
 
@@ -219,15 +258,44 @@ def drop_stale_deleted_attachment_rows!
     end
   end
 
+  # Limits the retry to missing attachment errors when attachment rows were submitted.
+  # Other missing records must still raise rather than being hidden by the retry.
   def missing_deleted_attachment_race?(error)
     params[:thesis]&.[](:files_attachments_attributes).present? &&
       error.message.include?('ActiveStorage::Attachment')
   end
 
+  # Returns selected-term review records after rechecking direct and unwaived related holds.
+  # Stored review status can be stale after related records change; both preview and submission use
+  # this check.
   def publication_candidates
-    filter_theses_by_term(Thesis.in_review).to_a
+    filter_theses_by_term(Thesis.in_review.without_publication_holds).to_a
+  end
+
+  # Extracts processing-only decisions keyed by related thesis ID: '1' grants an exception and '0'
+  # revokes it.
+  # The model validates IDs and values. Missing rows leave existing exceptions unchanged.
+  # Kept separate from shared thesis_params so ordinary author updates cannot grant or revoke
+  # exceptions.
+  def related_hold_review_params
+    params.require(:thesis).permit(related_hold_reviews: {})[:related_hold_reviews].to_h
+  end
+
+  # Enqueues the eligible batch for preparation and reports its actual size, not the earlier preview count.
+  # An empty batch produces a warning without enqueuing a job.
+  def queue_publication(theses)
+    if theses.empty?
+      flash[:warning] = 'No theses are eligible for publication in the selected term.'
+      return
+    end
+
+    DspacePublicationPrepJob.perform_later(theses)
+    count = view_context.pluralize(theses.count, 'thesis', 'theses')
+    flash[:success] = "#{count} added to the publication queue. Status updates are not immediate."
   end
 
+  # Shared by author submissions and processor updates. Related-hold decisions must stay in the
+  # processing-only filter.
   def thesis_params
     params.require(:thesis).permit(:title, :abstract, :coauthors, :graduation_month,
                                    :graduation_year, :copyright_id, :author_note,

app/jobs/dspace_publication_job.rb

@@ -18,6 +18,9 @@ def setup(sqs_client)
   end
 
   def perform(thesis, sqs_client = nil)
+    thesis.reload
+    return if publication_blocked?(thesis)
+
     setup(sqs_client)
 
     Rails.logger.debug('Setup complete')
@@ -68,4 +71,16 @@ def send_sqs_message(thesis)
       message_attributes: message_attributes
     )
   end
+
+  private
+
+  # We check for holds in the processing workflow. This method exists as a secondary safeguard.
+  def publication_blocked?(thesis)
+    return false if thesis.no_active_holds? && thesis.no_other_theses_with_holds?
+
+    Rails.logger.warn("Skipping publication of Thesis #{thesis.id}: holds prevent publication; " \
+                      "unreviewed related theses: #{thesis.blocking_other_theses_with_holds.ids}")
+    thesis.save! if thesis.publication_status == 'Publication review'
+    true
+  end
 end

app/models/hold.rb

@@ -41,6 +41,7 @@ class Hold < ApplicationRecord
   validates :thesis, presence: true
 
   after_save :update_thesis_status
+  after_destroy :update_thesis_status
 
   scope :active_or_expired, -> { Hold.active.or(Hold.expired) }
   scope :ends_today_or_before, -> { where('date_end <= ?', Date.current) }
@@ -61,7 +62,7 @@ def created_by
     return unless versions.present? && versions.first.event == 'create'
 
     creator_id = versions.first.whodunnit
-    if user = User.find_by(id: creator_id)
+    if (user = User.find_by(id: creator_id))
       user.kerberos_id
     else
       "User ID #{creator_id} no longer active."
@@ -87,15 +88,17 @@ def date_released
       # We check for the integer 2 here because that corresponds with 'released', and PT has been incorrectly storing
       # all enum values as integers since we started using it. That is fixed now that we migrated to JSON, but it
       # remains a problem with historical data.
-      version.changeset['status'][1] == 'released' || version.changeset['status'][1] == 2
+      ['released', 2].include?(version.changeset['status'][1])
     end
     dates_released = released_versions.map { |version| version.changeset['updated_at'][1] }
     dates_released.max
   end
 
-  # The thesis' publication_status is recalculated every time the record is
-  # saved, so in case this hold's status has changed, we force a recalculation.
+  # Saving recalculates readiness for both the held thesis and related theses.
   def update_thesis_status
-    thesis.save
+    thesis_ids = [thesis_id, thesis_id_before_last_save].compact.uniq
+    user_ids = Author.where(thesis_id: thesis_ids).select(:user_id)
+    related_thesis_ids = Author.where(user_id: user_ids).select(:thesis_id)
+    Thesis.where(id: thesis_ids).or(Thesis.where(id: related_thesis_ids)).find_each(&:save)
   end
 end

app/models/related_hold_exception.rb

@@ -0,0 +1,80 @@
+# == Schema Information
+#
+# Table name: related_hold_exceptions
+#
+#  id                :integer          not null, primary key
+#  created_at        :datetime         not null
+#  updated_at        :datetime         not null
+#  related_thesis_id :integer          not null
+#  thesis_id         :integer          not null
+#
+# Indexes
+#
+#  index_related_hold_exceptions_on_pair               (thesis_id,related_thesis_id) UNIQUE
+#  index_related_hold_exceptions_on_related_thesis_id  (related_thesis_id)
+#
+# Foreign Keys
+#
+#  related_thesis_id  (related_thesis_id => theses.id)
+#  thesis_id          (thesis_id => theses.id)
+#
+# This model records a processor's decision that a related thesis represents a distinct work, and
+# its holds should not block publication of the thesis being processed.
+#
+# The pair is directional:
+#   - thesis is the record being published
+#   - related_thesis is the record whose holds are waived for that publication
+#
+# The reverse exception is not implied, and direct holds or holds on other, unreviewed related
+# theses still block publication.
+#
+# Exceptions persist until explicitly revoked, even if the related thesis's holds are
+# released and later added again. PaperTrail tracks model activity.
+class RelatedHoldException < ApplicationRecord
+  has_paper_trail
+
+  belongs_to :thesis
+  belongs_to :related_thesis, class_name: 'Thesis'
+
+  validates :related_thesis_id, uniqueness: { scope: :thesis_id }
+  validate :distinct_theses
+
+  # Applies processing-form decisions keyed by string  IDs: '1' grants or retains
+  # an exception, and '0' revokes it. Omitted IDs leave existing exceptions unchanged.
+  # Validates the whole submission before changing records and uses create!/destroy!
+  # to retain audit callbacks. The caller wraps this and the thesis save in a transaction
+  # so failed metadata updates also roll back exception changes and their audit records.
+  def self.apply_reviews!(thesis, reviews)
+    existing = thesis.related_hold_exceptions.index_by { |exception| exception.related_thesis_id.to_s }
+    validate_reviews!(thesis, reviews, existing.keys)
+    reviews.each do |related_id, reviewed|
+      if reviewed == '1'
+        thesis.related_hold_exceptions.create!(related_thesis_id: related_id) unless existing.key?(related_id)
+      else
+        existing[related_id]&.destroy!
+      end
+    end
+  end
+
+  # Grants require a currently held related thesis or an existing exception. Revoking an
+  # already-absent exception is a no-op, including when a hold was released after the form loaded.
+  # IDs must be positive integer strings and decisions must be '0' or '1'. Invalid submissions
+  # raise ActiveRecord::RecordInvalid before any exception changes are applied.
+  def self.validate_reviews!(thesis, reviews, reviewed_ids)
+    allowed_ids = thesis.other_theses_with_holds.ids.map(&:to_s) | reviewed_ids
+    return if reviews.all? do |related_id, reviewed|
+      valid_decisions = allowed_ids.include?(related_id) ? %w[0 1] : ['0']
+      related_id.is_a?(String) && /\A[1-9]\d*\z/.match?(related_id) && valid_decisions.include?(reviewed)
+    end
+
+    thesis.errors.add(:base, 'Related hold reviews must identify related theses and a checked or unchecked decision.')
+    raise ActiveRecord::RecordInvalid, thesis
+  end
+
+  private
+
+  # Prevents an exception from targeting the publishing thesis itself.
+  def distinct_theses
+    errors.add(:related_thesis, 'must be a different thesis') if thesis == related_thesis
+  end
+end