diff --git a/app/controllers/thesis_controller.rb b/app/controllers/thesis_controller.rb index 5ec7ae5e..0c2b0af4 100644 --- a/app/controllers/thesis_controller.rb +++ b/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,6 +63,7 @@ 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 @@ -46,6 +71,7 @@ def deduplicate 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,11 +93,8 @@ 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 @@ -79,6 +102,7 @@ def publish_to_dspace 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, diff --git a/app/jobs/dspace_publication_job.rb b/app/jobs/dspace_publication_job.rb index 0e717dd5..cf7bcd99 100644 --- a/app/jobs/dspace_publication_job.rb +++ b/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 diff --git a/app/models/hold.rb b/app/models/hold.rb index 0cc2211f..af2ab45c 100644 --- a/app/models/hold.rb +++ b/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 diff --git a/app/models/related_hold_exception.rb b/app/models/related_hold_exception.rb new file mode 100644 index 00000000..7e6129f0 --- /dev/null +++ b/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 diff --git a/app/models/thesis.rb b/app/models/thesis.rb index 59c0d676..6a4149a9 100644 --- a/app/models/thesis.rb +++ b/app/models/thesis.rb @@ -47,6 +47,15 @@ class Thesis < ApplicationRecord has_many :holds, dependent: :restrict_with_error has_many :hold_sources, through: :holds + # Outgoing exceptions waive specific related theses' holds for this thesis, not the reverse. + has_many :related_hold_exceptions, dependent: :destroy + + # Incoming exceptions belong to other theses that waive this thesis's holds. + # Both associations use destroy callbacks to audit exception cleanup when either thesis is deleted. + has_many :incoming_related_hold_exceptions, + class_name: 'RelatedHoldException', foreign_key: :related_thesis_id, + inverse_of: :related_thesis, dependent: :destroy + has_many :advisor_theses has_many :advisors, through: :advisor_theses @@ -109,6 +118,17 @@ class Thesis < ApplicationRecord scope :date_asc, -> { order('grad_date') } scope :in_review, -> { where('publication_status = ?', 'Publication review') } + scope :without_publication_holds, lambda { + held_ids = Hold.active_or_expired.select(:thesis_id) + related_ids = Author.joins('INNER JOIN authors related_authors ON related_authors.user_id = authors.user_id') + .joins('LEFT JOIN related_hold_exceptions ' \ + 'ON related_hold_exceptions.thesis_id = authors.thesis_id ' \ + 'AND related_hold_exceptions.related_thesis_id = related_authors.thesis_id') + .where(related_authors: { thesis_id: held_ids }, related_hold_exceptions: { id: nil }) + .where('authors.thesis_id <> related_authors.thesis_id') + .select(:thesis_id) + where.not(id: held_ids).where.not(id: related_ids) + } scope :without_files, -> { where.missing(:files_attachments) } scope :with_files, -> { joins(:files_attachments).distinct } scope :without_sips, lambda { @@ -179,6 +199,7 @@ def active_holds? # 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. + # Includes records with saved exceptions so alerts still reflect the actual holds. def other_theses_with_holds user_ids = users.ids return Thesis.none if user_ids.empty? @@ -191,6 +212,31 @@ def other_theses_with_holds .order(:id) end + # Returns currently held related theses without an outgoing exception from this thesis. + # A saved exception excludes only its specific related thesis. Newly discovered related theses + # still block publicaiton. + # This filters publication eligibility without releasing holds or hiding them from the factual + # query above. + def blocking_other_theses_with_holds + other_theses_with_holds.where.not(id: related_hold_exceptions.select(:related_thesis_id)) + end + + # Saves processing attributes and related-hold decisions in one transaction, including their audit records. + # Reviews map string related thesis IDs to '1' (grant/retain) or '0' (revoke); omitted IDs are unchanged. + # Apply decisions before update so readiness recalculation sees the new exceptions during validation. + # Returns true on success and a falsey value on validation failure, rolling back all persisted changes. + # Other errors propagate so the controller can handle attachment races without hiding unrelated failures. + def update_with_related_hold_reviews(attributes, reviews) + transaction do + RelatedHoldException.apply_reviews!(self, reviews) + raise ActiveRecord::Rollback unless update(attributes) + + true + end + rescue ActiveRecord::RecordInvalid + false + end + # Returns a true/false value if there are any associated advisors. def advisors? advisors.any? @@ -259,15 +305,13 @@ 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. + # True when no thesis sharing any author has an unwaived active or expired hold. + # Used by evaluate_status alongside the separate direct-hold check. Exceptions never waive this + # thesis's own holds. + # Expiration still requires review, not publication: the hold must be released or its + # related-thesis block waived. def no_other_theses_with_holds? - other_theses_with_holds.none? + blocking_other_theses_with_holds.none? end # This inverts the issues_found field, so that the checks inside the diff --git a/app/views/thesis/process_thesis.html.erb b/app/views/thesis/process_thesis.html.erb index 6cbfff71..57e60d7f 100644 --- a/app/views/thesis/process_thesis.html.erb +++ b/app/views/thesis/process_thesis.html.erb @@ -51,12 +51,20 @@ <%= 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' }, + input_html: { class: 'disabled', style: 'width: 40%', value: f.object.no_other_theses_with_holds? ? 'Yes' : 'No' }, label: 'Related holds resolved?' %> - <% if @other_theses_with_holds.any? %> + <% if @related_theses_for_review.any? %> <% end %> diff --git a/db/migrate/20261105120000_create_related_hold_exceptions.rb b/db/migrate/20261105120000_create_related_hold_exceptions.rb new file mode 100644 index 00000000..3f2cea46 --- /dev/null +++ b/db/migrate/20261105120000_create_related_hold_exceptions.rb @@ -0,0 +1,14 @@ +class CreateRelatedHoldExceptions < ActiveRecord::Migration[8.1] + def change + create_table :related_hold_exceptions do |table| + table.references :thesis, null: false, index: false, foreign_key: true + table.references :related_thesis, null: false, foreign_key: { to_table: :theses } + table.timestamps + end + + add_index :related_hold_exceptions, %i[thesis_id related_thesis_id], unique: true, + name: 'index_related_hold_exceptions_on_pair' + add_check_constraint :related_hold_exceptions, 'thesis_id <> related_thesis_id', + name: 'related_hold_exceptions_not_self' + end +end diff --git a/db/schema.rb b/db/schema.rb index 314b5a33..0f9273e2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_10_29_150000) do +ActiveRecord::Schema[8.1].define(version: 2026_11_05_120000) do create_table "active_storage_attachments", force: :cascade do |t| t.integer "blob_id", null: false t.datetime "created_at", precision: nil, null: false @@ -206,6 +206,16 @@ t.index ["user_id"], name: "index_registrars_on_user_id" end + create_table "related_hold_exceptions", force: :cascade do |t| + t.datetime "created_at", null: false + t.integer "related_thesis_id", null: false + t.integer "thesis_id", null: false + t.datetime "updated_at", null: false + t.index ["related_thesis_id"], name: "index_related_hold_exceptions_on_related_thesis_id" + t.index ["thesis_id", "related_thesis_id"], name: "index_related_hold_exceptions_on_pair", unique: true + t.check_constraint "thesis_id <> related_thesis_id", name: "related_hold_exceptions_not_self" + end + create_table "submission_information_packages", force: :cascade do |t| t.string "bag_declaration" t.string "bag_name" @@ -308,6 +318,8 @@ add_foreign_key "holds", "hold_sources" add_foreign_key "holds", "theses" add_foreign_key "registrars", "users" + add_foreign_key "related_hold_exceptions", "theses" + add_foreign_key "related_hold_exceptions", "theses", column: "related_thesis_id" add_foreign_key "submission_information_packages", "theses" add_foreign_key "submitters", "departments" add_foreign_key "submitters", "users" diff --git a/test/controllers/thesis_controller_test.rb b/test/controllers/thesis_controller_test.rb index 0fbc5d48..ee793857 100644 --- a/test/controllers/thesis_controller_test.rb +++ b/test/controllers/thesis_controller_test.rb @@ -513,6 +513,196 @@ def attach_files_to_records(tr, th) end # ~~~~~~~~~~~~~~~~~~~ submitting thesis processing form ~~~~~~~~~~~~~~~~~~~~~ + test 'processors can grant and revoke an audited related hold exception' do + sign_in users(:processor) + thesis = theses(:one) + related_thesis = theses(:with_hold) + + assert_difference 'RelatedHoldException.count', 1 do + patch thesis_process_update_path(thesis), + params: { thesis: { processor_note: '', related_hold_reviews: { related_thesis.id.to_s => '1' } } } + end + exception = thesis.related_hold_exceptions.sole + assert_equal users(:processor).id.to_s, exception.versions.last.whodunnit + assert_equal 'create', exception.versions.last.event + assert_equal [nil, thesis.id], exception.versions.last.changeset[:thesis_id] + assert_equal [nil, related_thesis.id], exception.versions.last.changeset[:related_thesis_id] + + follow_redirect! + assert_select "input#related_hold_review_#{related_thesis.id}[checked]" + assert_select 'input#thesis_no_other_theses_with_holds[readonly][value=?]', 'Yes' + assert_select "a[href='#{thesis_path(related_thesis)}']", text: "Thesis ##{related_thesis.id}" + + assert_difference 'RelatedHoldException.count', -1 do + patch thesis_process_update_path(thesis), + params: { thesis: { related_hold_reviews: { related_thesis.id.to_s => '0' } } } + end + version = PaperTrail::Version.where(item_type: 'RelatedHoldException', item_id: exception.id).last + assert_equal 'destroy', version.event + assert_equal users(:processor).id.to_s, version.whodunnit + follow_redirect! + assert_select "input#related_hold_review_#{related_thesis.id}:not([checked])" + assert_select 'input#thesis_no_other_theses_with_holds[readonly][value=?]', 'No' + end + + test 'thesis admins can grant and revoke an audited related hold exception' do + sign_in users(:thesis_admin) + thesis = theses(:one) + related_thesis = theses(:with_hold) + + assert_difference 'RelatedHoldException.count', 1 do + patch thesis_process_update_path(thesis), + params: { thesis: { related_hold_reviews: { related_thesis.id.to_s => '1' } } } + end + assert_redirected_to thesis_process_path(thesis) + exception = thesis.related_hold_exceptions.sole + assert_equal 'create', exception.versions.last.event + assert_equal users(:thesis_admin).id.to_s, exception.versions.last.whodunnit + + assert_difference 'RelatedHoldException.count', -1 do + patch thesis_process_update_path(thesis), + params: { thesis: { related_hold_reviews: { related_thesis.id.to_s => '0' } } } + end + assert_redirected_to thesis_process_path(thesis) + version = PaperTrail::Version.where(item_type: 'RelatedHoldException', item_id: exception.id).last + assert_equal 'destroy', version.event + assert_equal users(:thesis_admin).id.to_s, version.whodunnit + end + + test 'admins can grant and revoke an audited related hold exception' do + sign_in users(:admin) + thesis = theses(:one) + related_thesis = theses(:with_hold) + + assert_difference 'RelatedHoldException.count', 1 do + patch thesis_process_update_path(thesis), + params: { thesis: { related_hold_reviews: { related_thesis.id.to_s => '1' } } } + end + assert_redirected_to thesis_process_path(thesis) + exception = thesis.related_hold_exceptions.sole + assert_equal 'create', exception.versions.last.event + assert_equal users(:admin).id.to_s, exception.versions.last.whodunnit + + assert_difference 'RelatedHoldException.count', -1 do + patch thesis_process_update_path(thesis), + params: { thesis: { related_hold_reviews: { related_thesis.id.to_s => '0' } } } + end + assert_redirected_to thesis_process_path(thesis) + version = PaperTrail::Version.where(item_type: 'RelatedHoldException', item_id: exception.id).last + assert_equal 'destroy', version.event + assert_equal users(:admin).id.to_s, version.whodunnit + end + + test 'saved exceptions remain visible and revocable after related holds are released' do + sign_in users(:processor) + thesis = theses(:one) + related_thesis = theses(:with_hold) + thesis.related_hold_exceptions.create!(related_thesis: related_thesis) + related_thesis.holds.each { |hold| hold.update!(status: :released) } + + get thesis_process_path(thesis) + + assert_select "input#related_hold_review_#{related_thesis.id}[checked]" + assert_select "a[href='#{thesis_path(related_thesis)}']" + patch thesis_process_update_path(thesis), + params: { thesis: { related_hold_reviews: { related_thesis.id.to_s => '0' } } } + assert_empty thesis.related_hold_exceptions.reload + end + + test 'processing saves metadata from a stale unchecked review after the related hold is released' do + sign_in users(:processor) + thesis = theses(:one) + related_thesis = theses(:with_hold) + + get thesis_process_path(thesis) + assert_select "input#related_hold_review_#{related_thesis.id}:not([checked])" + related_thesis.holds.each { |hold| hold.update!(status: :released) } + + assert_no_difference 'RelatedHoldException.count' do + patch thesis_process_update_path(thesis), params: { + thesis: { title: 'Saved from stale form', related_hold_reviews: { related_thesis.id.to_s => '0' } } + } + end + + assert_redirected_to thesis_process_path(thesis) + assert_equal 'Saved from stale form', thesis.reload.title + follow_redirect! + assert_select '.alert-banner.success', text: "Your changes to 'Saved from stale form' have been saved." + end + + test 'new related held theses appear unchecked alongside saved exceptions' do + sign_in users(:processor) + thesis = theses(:one) + thesis.related_hold_exceptions.create!(related_thesis: theses(:with_hold)) + Author.create!(user: users(:yo), thesis: theses(:downloaded), graduation_confirmed: true) + + get thesis_process_path(thesis) + + assert_select "input#related_hold_review_#{theses(:with_hold).id}[checked]" + assert_select "input#related_hold_review_#{theses(:downloaded).id}:not([checked])" + assert_select 'input#thesis_no_other_theses_with_holds[readonly][value=?]', 'No' + end + + test 'authors cannot grant exceptions through the processing route' do + sign_in users(:yo) + + assert_no_difference 'RelatedHoldException.count' do + patch thesis_process_update_path(theses(:one)), + params: { thesis: { related_hold_reviews: { theses(:with_hold).id.to_s => '1' } } } + end + assert_redirected_to '/' + end + + test 'anonymous users cannot grant exceptions through the processing route' do + assert_no_difference 'RelatedHoldException.count' do + patch thesis_process_update_path(theses(:one)), + params: { thesis: { related_hold_reviews: { theses(:with_hold).id.to_s => '1' } } } + end + assert_redirected_to '/login' + end + + test 'author updates cannot mass assign related hold exceptions' do + sign_in users(:yo) + + assert_no_difference 'RelatedHoldException.count' do + patch thesis_path(theses(:one)), params: { + thesis: { + related_hold_reviews: { theses(:with_hold).id.to_s => '1' }, + related_hold_exceptions_attributes: [{ related_thesis_id: theses(:with_hold).id }] + } + } + end + assert_redirected_to thesis_confirm_path + assert_not theses(:one).reload.no_other_theses_with_holds? + end + + test 'processing rejects unrelated review ids without saving other changes' do + sign_in users(:processor) + thesis = theses(:one) + original_title = thesis.title + + assert_no_difference ['RelatedHoldException.count', 'PaperTrail::Version.count'] do + patch thesis_process_update_path(thesis), params: { + thesis: { title: 'Not saved', related_hold_reviews: { theses(:released_hold).id.to_s => '1' } } + } + end + assert_equal original_title, thesis.reload.title + follow_redirect! + assert_select '.alert-banner.error' + end + + test 'invalid processing metadata rolls back exception grants and audit records' do + sign_in users(:processor) + + assert_no_difference ['RelatedHoldException.count', 'PaperTrail::Version.count'] do + patch thesis_process_update_path(theses(:one)), params: { + thesis: { graduation_month: 'invalid', related_hold_reviews: { theses(:with_hold).id.to_s => '1' } } + } + end + follow_redirect! + assert_select '.alert-banner.error' + end + test 'anonymous users cannot submit thesis processing form' do patch thesis_process_update_path(theses(:one)), params: { thesis: { title: 'Something nonsensical' } } @@ -791,11 +981,60 @@ def attach_files_to_records(tr, th) test 'publishing a specific term to dspace ends back at the processing queue with a flash success' do sign_in users(:processor) - get thesis_publish_to_dspace_path, params: { graduation: '2018-09-01' } - assert_redirected_to thesis_select_path(params: { graduation: '2018-09-01' }) + get thesis_publish_to_dspace_path, params: { graduation: '2021-06-01' } + assert_redirected_to thesis_select_path(params: { graduation: '2021-06-01' }) follow_redirect! assert_select '.alert-banner.success', - text: 'The theses you selected have been added to the publication queue. Status updates are not immediate.', count: 1 + text: '1 thesis added to the publication queue. Status updates are not immediate.', count: 1 + end + + test 'publication preview excludes stale review records with unreviewed related holds' do + sign_in users(:processor) + thesis = theses(:publication_review) + related_thesis = theses(:with_hold) + related_thesis.holds.each { |hold| hold.update!(status: :released) } + Author.create!(user: users(:yo), thesis: thesis, graduation_confirmed: true) + related_thesis.holds.first.update!(status: :active) + assert_equal 'Not ready for publication', thesis.reload.publication_status + thesis.update_columns(publication_status: 'Publication review') + assert_equal 'Publication review', thesis.reload.publication_status + + get thesis_publish_preview_path, params: { graduation: '2021-06-01' } + + assert_select "a[href='#{thesis_process_path(thesis)}']", count: 0 + assert_select 'h4', text: 'Ready to publish 0 theses?' + end + + test 'publication rechecks related holds after preview and enqueues nothing when all are blocked' do + sign_in users(:processor) + thesis = theses(:publication_review) + Author.create!(user: users(:yo), thesis: thesis, graduation_confirmed: true) + assert thesis.update_with_related_hold_reviews({}, theses(:with_hold).id.to_s => '1') + get thesis_publish_preview_path, params: { graduation: '2021-06-01' } + assert_select 'h4', text: 'Ready to publish 1 theses?' + Author.create!(user: users(:yo), thesis: theses(:downloaded), graduation_confirmed: true) + + assert_no_enqueued_jobs only: DspacePublicationPrepJob do + get thesis_publish_to_dspace_path, params: { graduation: '2021-06-01' } + end + + assert_redirected_to thesis_select_path(params: { graduation: '2021-06-01' }) + follow_redirect! + assert_select '.alert-banner.warning', text: 'No theses are eligible for publication in the selected term.' + end + + test 'publication batches contain only currently eligible theses and report the queued count' do + sign_in users(:processor) + eligible = theses(:publication_review) + blocked = theses(:one) + blocked.update_columns(grad_date: eligible.grad_date, publication_status: 'Publication review') + assert_not blocked.no_other_theses_with_holds? + + assert_enqueued_with(job: DspacePublicationPrepJob, args: [[eligible]]) do + get thesis_publish_to_dspace_path, params: { graduation: '2021-06-01' } + end + follow_redirect! + assert_select '.alert-banner.success', text: /1 thesis added/ end # ~~~~~~~~~~~~~~~~~~~~~ proquest export preview ~~~~~~~~~~~~~~~~~~~~~ @@ -1082,6 +1321,7 @@ def attach_files_to_records(tr, th) params: { thesis: { title: thesis.title, + related_hold_reviews: { theses(:with_hold).id.to_s => '1' }, files_attachments_attributes: { '0' => { id: attachment_id, @@ -1102,6 +1342,7 @@ def attach_files_to_records(tr, th) assert deleted_during_first_update assert_equal 2, update_calls + assert RelatedHoldException.exists?(thesis: thesis, related_thesis: theses(:with_hold)) assert_response :redirect assert_redirected_to thesis_process_path(thesis) follow_redirect! diff --git a/test/jobs/dspace_publication_job_test.rb b/test/jobs/dspace_publication_job_test.rb index 290b60d0..f5a1ffc6 100644 --- a/test/jobs/dspace_publication_job_test.rb +++ b/test/jobs/dspace_publication_job_test.rb @@ -5,6 +5,7 @@ def setup @thesis = theses(:one) @input_queue_url = ENV.fetch('SQS_INPUT_QUEUE_URL') dss_friendly_thesis(@thesis) + @thesis.related_hold_exceptions.create!(related_thesis: theses(:with_hold)) end def teardown @@ -39,6 +40,59 @@ def dss_friendly_thesis(thesis) assert_equal 'Pending publication', @thesis.publication_status end + test 'a new related held thesis blocks publication despite an existing reviewed exception' do + @thesis.update_columns(publication_status: 'Publication review') + @thesis.related_hold_exceptions.load + @thesis.users.load + @thesis.holds.load + Author.create!(user: users(:yo), thesis: theses(:downloaded), graduation_confirmed: true) + sqs_client = Aws::SQS::Client.new(region: ENV.fetch('AWS_REGION'), stub_responses: true) + + DspacePublicationJob.perform_now(@thesis, sqs_client) + + assert_empty sqs_client.api_requests + assert_not @thesis.dspace_metadata.attached? + assert_equal 'Not ready for publication', @thesis.reload.publication_status + end + + test 'a revoked exception blocks publication even when the job has cached the exception' do + @thesis.related_hold_exceptions.load + RelatedHoldException.find_by!(thesis: @thesis).destroy! + sqs_client = Aws::SQS::Client.new(region: ENV.fetch('AWS_REGION'), stub_responses: true) + + DspacePublicationJob.perform_now(@thesis, sqs_client) + + assert_empty sqs_client.api_requests + assert_not @thesis.dspace_metadata.attached? + assert_equal 'Not ready for publication', @thesis.reload.publication_status + end + + test 'reviewed related holds cannot waive a new direct hold before publication' do + @thesis.holds.load + holds(:valid).update!(thesis: @thesis) + sqs_client = Aws::SQS::Client.new(region: ENV.fetch('AWS_REGION'), stub_responses: true) + + DspacePublicationJob.perform_now(@thesis, sqs_client) + + assert_empty sqs_client.api_requests + assert_not @thesis.dspace_metadata.attached? + assert_equal 'Not ready for publication', @thesis.reload.publication_status + end + + test 'a blocked publication leaves existing metadata and protected publication status intact' do + @thesis.dspace_metadata.attach(io: StringIO.new('{}'), filename: 'existing.json', content_type: 'application/json') + metadata_blob_id = @thesis.dspace_metadata.blob.id + @thesis.update!(publication_status: 'Pending publication') + @thesis.related_hold_exceptions.destroy_all + sqs_client = Aws::SQS::Client.new(region: ENV.fetch('AWS_REGION'), stub_responses: true) + + DspacePublicationJob.perform_now(@thesis, sqs_client) + + assert_empty sqs_client.api_requests + assert_equal metadata_blob_id, @thesis.reload.dspace_metadata.blob.id + assert_equal 'Pending publication', @thesis.publication_status + end + test 'TimeoutError sets status to publication error' do sqs_client = Aws::SQS::Client.new(region: ENV.fetch('AWS_REGION'), stub_responses: true) sqs_client.stub_responses(:send_message, Timeout::Error) diff --git a/test/models/hold_test.rb b/test/models/hold_test.rb index 4d7cde5d..6476b70f 100644 --- a/test/models/hold_test.rb +++ b/test/models/hold_test.rb @@ -27,6 +27,132 @@ require 'test_helper' class HoldTest < ActiveSupport::TestCase + test 'adding a hold to a related thesis removes an otherwise ready thesis from publication review' do + thesis = theses(:publication_review) + related_thesis = theses(:released_hold) + Author.create!(user: thesis.users.first, thesis: related_thesis, graduation_confirmed: true) + thesis.save! + assert_equal 'Publication review', thesis.reload.publication_status + + hold = holds(:valid).dup + hold.thesis = related_thesis + hold.save! + + assert_equal 'Not ready for publication', thesis.reload.publication_status + end + + test 'releasing the last related hold restores publication review' do + thesis = theses(:publication_review) + hold = related_hold_for(thesis) + hold.update!(status: :active) + assert_equal 'Not ready for publication', thesis.reload.publication_status + + hold.update!(status: :released) + + assert_equal 'Publication review', thesis.reload.publication_status + end + + test 'expiring a related hold does not restore publication review' do + thesis = theses(:publication_review) + hold = related_hold_for(thesis) + hold.update!(status: :active) + + hold.update!(status: :expired) + + assert_equal 'Not ready for publication', thesis.reload.publication_status + end + + test 'deleting the last related hold restores publication review' do + thesis = theses(:publication_review) + hold = related_hold_for(thesis) + hold.update!(status: :active) + assert_equal 'Not ready for publication', thesis.reload.publication_status + + hold.destroy! + + assert_equal 'Publication review', thesis.reload.publication_status + end + + test 'releasing one related hold leaves other active holds blocking publication review' do + thesis = theses(:publication_review) + hold = related_hold_for(thesis) + hold.update!(status: :active) + another_hold = hold.dup + another_hold.save! + + hold.update!(status: :released) + + assert_equal 'Not ready for publication', thesis.reload.publication_status + end + + test 'releasing a related hold does not waive other readiness requirements' do + thesis = theses(:publication_review) + hold = related_hold_for(thesis) + hold.update!(status: :active) + thesis.update!(issues_found: true) + + hold.update!(status: :released) + + assert_equal 'Not ready for publication', thesis.reload.publication_status + end + + test 'a reviewed distinction preserves publication review when the related thesis is held again' do + thesis = theses(:publication_review) + hold = related_hold_for(thesis) + thesis.related_hold_exceptions.create!(related_thesis: hold.thesis) + + hold.update!(status: :active) + + assert_equal 'Publication review', thesis.reload.publication_status + assert hold.reload.active? + end + + test 'moving a hold refreshes both the former and current author groups' do + thesis = theses(:publication_review) + hold = related_hold_for(thesis) + hold.update!(status: :active) + assert_equal 'Not ready for publication', thesis.reload.publication_status + other_thesis = theses(:doctor) + new_held_thesis = theses(:one) + Author.create!(user: other_thesis.users.last, thesis: new_held_thesis, graduation_confirmed: true) + other_thesis.save! + assert_equal 'Publication review', other_thesis.reload.publication_status + + hold.update!(thesis: new_held_thesis) + + assert_equal 'Publication review', thesis.reload.publication_status + assert_equal 'Not ready for publication', other_thesis.reload.publication_status + end + + test 'related hold changes preserve pending published and publication error statuses' do + thesis = theses(:publication_review) + hold = related_hold_for(thesis) + pending_thesis = theses(:pending_publication) + published_thesis = theses(:published) + error_thesis = theses(:two) + Author.create!(user: thesis.users.first, thesis: pending_thesis, graduation_confirmed: true) + Author.create!(user: thesis.users.first, thesis: published_thesis, graduation_confirmed: true) + Author.create!(user: thesis.users.first, thesis: error_thesis, graduation_confirmed: true) + error_thesis.update!(publication_status: 'Publication error') + + hold.update!(status: :active) + + assert_equal 'Pending publication', pending_thesis.reload.publication_status + assert_equal 'Published', published_thesis.reload.publication_status + assert_equal 'Publication error', error_thesis.reload.publication_status + end + + test 'related hold changes do not save unrelated theses' do + thesis = theses(:publication_review) + hold = related_hold_for(thesis) + unrelated_thesis = theses(:doctor) + original_updated_at = unrelated_thesis.updated_at + + hold.update!(status: :active) + + assert_equal original_updated_at, unrelated_thesis.reload.updated_at + end + test 'valid hold' do hold = holds(:valid) assert(hold.valid?) @@ -237,4 +363,13 @@ class HoldTest < ActiveSupport::TestCase assert_not_equal earlier_release_date, later_release_date assert_equal later_release_date, hold.date_released end + + private + + def related_hold_for(thesis) + related_thesis = theses(:released_hold) + Author.create!(user: thesis.users.first, thesis: related_thesis, graduation_confirmed: true) + thesis.save! + related_thesis.holds.sole + end end diff --git a/test/models/related_hold_exception_test.rb b/test/models/related_hold_exception_test.rb new file mode 100644 index 00000000..4d04ef52 --- /dev/null +++ b/test/models/related_hold_exception_test.rb @@ -0,0 +1,269 @@ +require 'test_helper' + +# == 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) +# +class RelatedHoldExceptionTest < ActiveSupport::TestCase + test 'a reviewed related thesis no longer blocks readiness but remains in the factual query' do + thesis = theses(:one) + related_thesis = theses(:with_hold) + + assert_not thesis.no_other_theses_with_holds? + thesis.related_hold_exceptions.create!(related_thesis: related_thesis) + + assert thesis.no_other_theses_with_holds? + assert_includes thesis.other_theses_with_holds, related_thesis + end + + test 'an exception cannot refer to its own thesis' do + exception = RelatedHoldException.new(thesis: theses(:one), related_thesis: theses(:one)) + + assert_not exception.valid? + assert_includes exception.errors[:related_thesis], 'must be a different thesis' + end + + test 'a directional pair cannot be duplicated' do + RelatedHoldException.create!(thesis: theses(:one), related_thesis: theses(:with_hold)) + duplicate = RelatedHoldException.new(thesis: theses(:one), related_thesis: theses(:with_hold)) + + assert_not duplicate.valid? + assert_includes duplicate.errors[:related_thesis_id], 'has already been taken' + end + + test 'an exception does not create the reverse exception or waive direct holds' do + thesis = theses(:with_hold) + thesis.related_hold_exceptions.create!(related_thesis: theses(:one)) + + assert_empty theses(:one).related_hold_exceptions + assert_not thesis.no_active_holds? + assert_not thesis.evaluate_status + end + + test 'a newly discovered held thesis still blocks after another thesis was reviewed' do + thesis = theses(:one) + thesis.related_hold_exceptions.create!(related_thesis: theses(:with_hold)) + assert thesis.no_other_theses_with_holds? + + Author.create!(user: users(:yo), thesis: theses(:downloaded), graduation_confirmed: true) + + assert_not thesis.no_other_theses_with_holds? + assert_equal [theses(:downloaded).id], thesis.blocking_other_theses_with_holds.ids + end + + test 'processing reviews persist and can be revoked without requiring a note' do + thesis = theses(:one) + related_id = theses(:with_hold).id.to_s + + assert thesis.update_with_related_hold_reviews({ processor_note: '' }, related_id => '1') + assert thesis.reload.no_other_theses_with_holds? + assert thesis.update_with_related_hold_reviews({}, related_id => '0') + assert_not thesis.reload.no_other_theses_with_holds? + end + + test 'omitted review rows remain unchanged' do + thesis = theses(:one) + exception = thesis.related_hold_exceptions.create!(related_thesis: theses(:with_hold)) + + assert thesis.update_with_related_hold_reviews({}, {}) + assert RelatedHoldException.exists?(exception.id) + end + + test 'a stale unchecked review saves metadata after the related holds are released' do + thesis = theses(:one) + related_thesis = theses(:with_hold) + assert_includes thesis.other_theses_with_holds, related_thesis + related_thesis.holds.each { |hold| hold.update!(status: :released) } + + assert_no_difference 'RelatedHoldException.count' do + assert thesis.update_with_related_hold_reviews( + { title: 'Updated after release' }, related_thesis.id.to_s => '0' + ) + end + + assert_equal 'Updated after release', thesis.reload.title + end + + test 'a stale unchecked review saves metadata after the related holds are deleted' do + thesis = theses(:one) + related_thesis = theses(:with_hold) + assert_includes thesis.other_theses_with_holds, related_thesis + related_thesis.holds.destroy_all + + assert_no_difference 'RelatedHoldException.count' do + assert thesis.update_with_related_hold_reviews( + { title: 'Updated after deletion' }, related_thesis.id.to_s => '0' + ) + end + + assert_equal 'Updated after deletion', thesis.reload.title + end + + test 'revoking an exception already removed by another processor is a no-op' do + thesis = theses(:one) + related_thesis = theses(:with_hold) + exception = thesis.related_hold_exceptions.create!(related_thesis: related_thesis) + related_thesis.holds.each { |hold| hold.update!(status: :released) } + RelatedHoldException.find(exception.id).destroy! + + assert_no_difference 'RelatedHoldException.count' do + assert thesis.update_with_related_hold_reviews( + { title: 'Updated after revocation' }, related_thesis.id.to_s => '0' + ) + end + + assert_equal 'Updated after revocation', thesis.reload.title + end + + test 'an unchecked unrelated id cannot revoke another thesis exception' do + exception = theses(:released_hold).related_hold_exceptions.create!(related_thesis: theses(:downloaded)) + + assert_no_difference 'RelatedHoldException.count' do + assert theses(:one).update_with_related_hold_reviews({}, theses(:downloaded).id.to_s => '0') + end + + assert RelatedHoldException.exists?(exception.id) + end + + test 'a new exception is still rejected after the related holds are released' do + thesis = theses(:one) + related_thesis = theses(:with_hold) + related_thesis.holds.each { |hold| hold.update!(status: :released) } + original_title = thesis.reload.title + + assert_no_difference 'RelatedHoldException.count' do + assert_not thesis.update_with_related_hold_reviews( + { title: 'Unsaved title' }, related_thesis.id.to_s => '1' + ) + end + + assert_equal original_title, thesis.reload.title + end + + test 'malformed ids are rejected even for unchecked decisions' do + assert_not theses(:one).update_with_related_hold_reviews({}, 'not-an-id' => '0') + end + + test 'non-string ids are rejected even for unchecked decisions' do + assert_not theses(:one).update_with_related_hold_reviews({}, theses(:with_hold).id => '0') + end + + test 'non-string decision values are rejected' do + assert_not theses(:one).update_with_related_hold_reviews({}, theses(:with_hold).id.to_s => 0) + end + + test 'invalid thesis updates roll back reviews and their audit records' do + thesis = theses(:one) + related_id = theses(:with_hold).id.to_s + + assert_no_difference ['RelatedHoldException.count', 'PaperTrail::Version.count'] do + assert_not thesis.update_with_related_hold_reviews({ graduation_month: 'invalid' }, related_id => '1') + end + assert_not thesis.reload.no_other_theses_with_holds? + end + + test 'forged review ids roll back metadata and all reviews' do + thesis = theses(:one) + original_title = thesis.title + + assert_no_difference 'RelatedHoldException.count' do + assert_not thesis.update_with_related_hold_reviews( + { title: 'Unsaved title' }, + theses(:with_hold).id.to_s => '1', theses(:released_hold).id.to_s => '1' + ) + end + assert_equal original_title, thesis.reload.title + end + + test 'invalid review values are rejected' do + thesis = theses(:one) + + assert_no_difference 'RelatedHoldException.count' do + assert_not thesis.update_with_related_hold_reviews({}, theses(:with_hold).id.to_s => 'yes') + end + end + + test 'reviewed distinction survives release and subsequent holds and remains revocable' do + thesis = theses(:one) + related_thesis = theses(:with_hold) + thesis.related_hold_exceptions.create!(related_thesis: related_thesis) + related_thesis.holds.each { |hold| hold.update!(status: :released) } + + assert thesis.update_with_related_hold_reviews({}, related_thesis.id.to_s => '1') + related_thesis.holds.first.update!(status: :active) + assert thesis.reload.no_other_theses_with_holds? + assert thesis.update_with_related_hold_reviews({}, related_thesis.id.to_s => '0') + assert_not thesis.reload.no_other_theses_with_holds? + end + + test 'an otherwise ready thesis enters review only while its related hold exception exists' do + thesis = theses(:publication_review) + Author.create!(user: users(:yo), thesis: thesis, graduation_confirmed: true) + thesis.save! + assert_equal 'Not ready for publication', thesis.reload.publication_status + + assert thesis.update_with_related_hold_reviews({}, theses(:with_hold).id.to_s => '1') + assert_equal 'Publication review', thesis.reload.publication_status + + assert thesis.update_with_related_hold_reviews({}, theses(:with_hold).id.to_s => '0') + assert_equal 'Not ready for publication', thesis.reload.publication_status + end + + test 'an exception cannot waive other readiness failures' do + thesis = theses(:publication_review) + Author.create!(user: users(:yo), thesis: thesis, graduation_confirmed: true) + + assert thesis.update_with_related_hold_reviews({ issues_found: true }, theses(:with_hold).id.to_s => '1') + assert thesis.reload.no_other_theses_with_holds? + assert_equal 'Not ready for publication', thesis.publication_status + end + + test 'invalid thesis updates roll back revocations and their audit records' do + thesis = theses(:one) + exception = thesis.related_hold_exceptions.create!(related_thesis: theses(:with_hold)) + + assert_no_difference ['RelatedHoldException.count', 'PaperTrail::Version.count'] do + assert_not thesis.update_with_related_hold_reviews( + { graduation_month: 'invalid' }, theses(:with_hold).id.to_s => '0' + ) + end + assert RelatedHoldException.exists?(exception.id) + end + + test 'destroying the publishing thesis audits removal of its exceptions' do + thesis = theses(:one) + exception = thesis.related_hold_exceptions.create!(related_thesis: theses(:with_hold)) + + thesis.destroy! + + assert_not RelatedHoldException.exists?(exception.id) + assert_equal 'destroy', exception.versions.last.event + end + + test 'destroying a related thesis removes incoming exceptions' do + related_thesis = theses(:released_hold) + exception = RelatedHoldException.create!(thesis: theses(:one), related_thesis: related_thesis) + related_thesis.holds.destroy_all + + related_thesis.destroy! + + assert_not RelatedHoldException.exists?(exception.id) + assert_equal 'destroy', exception.versions.last.event + end +end diff --git a/test/models/thesis_test.rb b/test/models/thesis_test.rb index eba2e863..6769b436 100644 --- a/test/models/thesis_test.rb +++ b/test/models/thesis_test.rb @@ -31,8 +31,11 @@ require 'csv' require 'test_helper' +require 'active_record/testing/query_assertions' class ThesisTest < ActiveSupport::TestCase + include ActiveRecord::Assertions::QueryAssertions + 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') @@ -1705,4 +1708,62 @@ def attach_file_with_purpose_to(thesis, _purpose = 'thesis_pdf') assert_equal false, thesis.no_other_theses_with_holds? assert_equal 'Not ready for publication', thesis.publication_status end + + test 'without_publication_holds matches the individual hold predicates with reviewed exceptions' do + theses(:one).related_hold_exceptions.create!(related_thesis: theses(:with_hold)) + expected_ids = Thesis.all.select do |thesis| + thesis.no_active_holds? && thesis.no_other_theses_with_holds? + end.map(&:id) + + assert_equal expected_ids.sort, Thesis.without_publication_holds.ids.sort + end + + test 'without_publication_holds excludes direct active and expired holds' do + result = Thesis.without_publication_holds + + assert_not_includes result, theses(:with_hold) + assert_not_includes result, theses(:downloaded) + end + + test 'without_publication_holds keeps released related holds eligible' do + thesis = theses(:publication_review) + Author.create!(user: thesis.users.first, thesis: theses(:released_hold), graduation_confirmed: true) + + assert_includes Thesis.without_publication_holds, thesis + end + + test 'without_publication_holds checks all coauthors for related holds' do + thesis = theses(:publication_review) + Author.create!(user: users(:second), thesis: thesis, graduation_confirmed: true) + + assert_not_includes Thesis.without_publication_holds, thesis + end + + test 'without_publication_holds does not apply exceptions in reverse' do + theses(:with_hold).related_hold_exceptions.create!(related_thesis: theses(:one)) + + assert_not_includes Thesis.without_publication_holds, theses(:one) + end + + test 'without_publication_holds excludes new blockers despite an existing reviewed exception' do + thesis = theses(:one) + thesis.related_hold_exceptions.create!(related_thesis: theses(:with_hold)) + assert_includes Thesis.without_publication_holds, thesis + Author.create!(user: users(:yo), thesis: theses(:downloaded), graduation_confirmed: true) + + assert_not_includes Thesis.without_publication_holds, thesis + end + + test 'without_publication_holds does not produce N+1 queries' do + thesis = theses(:one) + thesis.related_hold_exceptions.create!(related_thesis: theses(:with_hold)) + thesis.update_columns(publication_status: 'Publication review') + candidate_ids = [thesis.id, theses(:publication_review).id] + + records = assert_queries_count(1) do + Thesis.in_review.where(id: candidate_ids).without_publication_holds.to_a + end + + assert_equal candidate_ids.sort, records.map(&:id).sort + end end