Prevent publication of duplicate theses with holds

[jazairi]

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

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

Developer

• All new ENV is documented in README
• All new ENV has been added to Heroku Pipeline, Staging and Prod
• ANDI or Wave has been run in accordance to
  our guide and
  all issues introduced by these changes have been resolved or opened as new
  issues (link to those issues in the Pull Request details above)
• Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

• The commit message is clear and follows our guidelines
  (not just this pull request message)
• There are appropriate tests covering any new functionality
• The documentation has been updated or is unnecessary
• The changes have been verified
• New dependencies are appropriate or there were no changes

Requires database migrations?

NO

Includes new or updated dependencies?

NO

[coveralls]

coverage: 98.25% (+0.02%) from 98.232% — etd-689 into main

[Copilot]

Pull request overview

This PR adds cross-thesis Hold detection to prevent publication of duplicate theses when an author has another thesis with an active or expired Hold, and surfaces that information in both the processing UI and registrar import reporting.

Changes:

• Add Thesis#other_theses_with_holds / #no_other_theses_with_holds? and integrate the new check into evaluate_status to block publication review.
• Display related theses-with-holds on the thesis processing form; include “multiple hold users” warnings in the registrar import summary email.
• Update/extend test coverage and adjust fixtures; minor RuboCop/date handling cleanups.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
app/models/thesis.rb	Adds cross-thesis lookup for active/expired holds and integrates it into pre-publication status checks.
app/controllers/thesis_controller.rb	Loads related theses-with-holds for the processing form view.
app/views/thesis/process_theses.html.erb	Renders a read-only “Related holds resolved?” field and links to related theses with holds.
app/jobs/registrar_import_job.rb	Tracks newly created theses and reports users who already have theses with holds during registrar import.
app/mailers/report_mailer.rb	Extends registrar import email method to accept “multiple hold users” cases and uses ENV.fetch.
app/views/report_mailer/registrar_import_email.html.erb	Adds a warning section listing users and thesis links when holds exist on other theses.
test/models/thesis_test.rb	Adds/updates tests for the new cross-thesis hold logic and status evaluation behavior.
test/controllers/thesis_controller_test.rb	Adds integration assertions for the processing form UI behavior.
test/mailers/report_mailer_test.rb	Adds assertions for registrar import email warning section rendering.
test/jobs/registrar_import_job_test.rb	Adds a basic test for the new registrar-import hold aggregation helper.
test/models/marc_test.rb	Adjusts MARC expectation to use fixture-driven author name.
test/models/hold_test.rb	Switches date comparisons to Date.current for Rails-consistent time zone behavior.
test/integration/admin/admin_hold_test.rb	Switches to Date.current for release date assertion.
test/fixtures/authors.yml	Updates fixture associations to support the revised tests and expected author ordering.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

[jazairi]

This conflicts directly with rubocop guidance.

[jazairi]

This conflicts directly with rubocop guidance.

[jazairi]

This conflicts directly with rubocop guidance.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

test/models/thesis_test.rb:34

• attach_file_with_purpose_to still accepts a purpose argument (and call sites pass values like 'signature_page'), but the helper always hard-codes 'thesis_pdf'. This makes the helper misleading and can hide test intent (or future regressions) when different purposes are expected.

  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'
    thesis.save

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

test/models/thesis_test.rb:34

• attach_file_with_purpose_to accepts a purpose argument, but the implementation ignores it and always sets the attachment purpose to 'thesis_pdf'. This makes call sites like attach_file_with_purpose_to(t, 'signature_page') misleading and reduces test correctness/clarity.

  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'
    thesis.save

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

test/models/thesis_test.rb:34

• attach_file_with_purpose_to still hard-codes the attachment purpose to 'thesis_pdf', so callers that pass a different purpose (e.g. 'signature_page') aren’t actually testing that behavior. This makes the helper misleading and can hide bugs in purpose-dependent logic.

  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'
    thesis.save

[jazairi]

I think this is a tad overstated. We execute one DB query per new thesis to fetch related theses and eager-load users on those theses.

[JPrevost]

Instead of keeping this larger list and then winnowing it down at the end to the (hopefully much smaller or empty) list I feel like we could update collect_users_with_multiple_hold_theses to act on a single thesis and call it here and refactor to only be tracking multiple_hold_users array. I think this is both more memory efficient and easier to follow unless we need the full list for something I'm not seeing yet

[jazairi]

Unless I misunderstand the flow, I think we need the full list to refer back to for later loop iterations.

Let's say we have a thesis with two coauthors. Author 1 is earlier in the CSV and has no other held theses; Author 2 is later and does have another held thesis. If the hold check only runs when the thesis is first created, we would miss the hold warning for Author 2. It's an unlikely use case, but one that we ought to consider.

I agree that we can refactor such that it doesn't store the thesis objects in memory while still running a hold check on every row, but that would likely add complexity. How concerned are we about efficiency?

[JPrevost]

Ah, I missed that important use case. Let's leave it as is and if we notice performance issues we can revisit. We probably won't :)

My concern noted the performance concern, but was really rooted in trying to make this easier to follow... but accuracy is more important than simplicity for this for sure.

[JPrevost]

This test doesn't show me that the method works, just that it exists. I think this test is really collect_users_with_multiple_hold_theses method returns not values when there are no holds or something like that?

Then we need another test to show it returning values when appropriate right?

[jazairi]

Agreed about the naming. But it does show that it returns an empty array when appropriate. As for the case of returning values when appropriate, that's covered on line 88, unless I'm misunderstanding the request.

[JPrevost]

Doesn't the test on 88 only cover one case? Specifically something about co-authors? Is there a test here that returns values and is only involving one user? (There very well may be and I may be missing it or misunderstanding the tests!)

[jazairi]

Ohh yeah, good point! It makes total sense to test the simplest use case. I'll add something to that effect.

[jazairi]

@JPrevost Thanks for the review! The latest commit addresses most of the comments, but I had a couple of clarifying questions.