From fdd19e7d047e74ab2aa5731955e57424213ca076 Mon Sep 17 00:00:00 2001 From: jazairi <16103405+jazairi@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:17:41 -0700 Subject: [PATCH 1/2] Address skipped tests Why these changes are being introduced: The codebase has several skipped tests, some of which have been skipped for years. We should reevaluate whether these tests still need to be skipped, or if they're even still necessary. Relevant ticket(s): - [ETD-701](https://mitlibraries.atlassian.net/browse/ETD-701) How this addresses that need: - Fixes admin advisor tests related to processor authorization. The underlying issue was that the Ability model did not provide the necessary permissions for the processor role to perform the specified tasks. - Implements Preservation Submission Job test confirming behavior when a 400 error is returned using a stubbed response. - Removes thesis integration test asserting that the thesis form fails validation without files. This validation does not exists, nor should it, as files are added separately in the transfer workflow. - Does *not* alter a test that is skipped because it is slow to run. Because this skip is performance-related and provides an optional override, it makes sense to leave it in place. Side effects of this change: - The Ability model as a whole may require an overhaul. The processor role is not currently in use, and it may not be needed. - A remaining skipped test fails only in GitHub Actions. That will be handled in a separate commit. --- app/models/ability.rb | 11 +++++--- test/integration/admin/admin_advisor_test.rb | 4 --- test/integration/thesis_test.rb | 9 ------- test/jobs/preservation_submission_job_test.rb | 25 ++++++++++++++++--- 4 files changed, 28 insertions(+), 21 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 7ffc7ed3..994ea140 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -54,12 +54,15 @@ def processor # Allow processors to see the admin dashboard link in the main site nav. See the _site_nav layout for more info. can :administrate, Admin - # Authorize processors to use all submitter dashboard controller actions. If not, any attempts to access the - # dashboard will trigger Admin::ApplicationController#authorized_or_redirect. + # Authorize processors to use all submitter and advisor dashboard controller actions. If not, + # any attempts to access the dashboard will trigger + # Admin::ApplicationController#authorized_or_redirect. + can :manage, :advisor can :manage, :submitter - # Authorize processors to access submitter model. If not, administrate will raise a NotAuthorizedError when - # controller methods are called. + # Authorize processors to access submitter and advisor model. If not, administrate will raise a + # NotAuthorizedError when controller methods are called. + can :manage, Advisor can :manage, Submitter can :files, Report diff --git a/test/integration/admin/admin_advisor_test.rb b/test/integration/admin/admin_advisor_test.rb index 17f7c23c..d7c49ac7 100644 --- a/test/integration/admin/admin_advisor_test.rb +++ b/test/integration/admin/admin_advisor_test.rb @@ -161,7 +161,6 @@ def delete_advisor_denied end test 'processors can create advisors via admin dashboard' do - skip('Processors currently can not _use_ the admin dashboard, so this fails.') mock_auth(users(:processor)) create_advisor end @@ -188,7 +187,6 @@ def delete_advisor_denied end test 'processors can edit advisors through admin dashboard' do - skip('Processors currently can not _use_ the admin dashboard, so this fails.') mock_auth(users(:processor)) edit_advisor_name end @@ -215,7 +213,6 @@ def delete_advisor_denied end test 'processors can assign theses to advisors via advisor form' do - skip('Processors currently can not _use_ the admin dashboard, so this fails.') mock_auth(users(:processor)) assign_advisor_to_thesis end @@ -242,7 +239,6 @@ def delete_advisor_denied end test 'processors can delete an advisor' do - skip('Processors currently can not _use_ the admin dashboard, so this fails.') mock_auth(users(:processor)) delete_advisor end diff --git a/test/integration/thesis_test.rb b/test/integration/thesis_test.rb index 6f350291..7aff419e 100644 --- a/test/integration/thesis_test.rb +++ b/test/integration/thesis_test.rb @@ -63,15 +63,6 @@ def teardown assert_select 'span.error', text: Thesis::VALIDATION_MSGS[:departments] end - test 'invalid files message' do - skip('Unclear why this used to pass but now fails, but the data model never properly validated attached thesis so this failing is not surprising') - mock_auth(users(:basic)) - params = @thesis_params - params[:files] = nil - post thesis_index_path, params: { thesis: params } - assert_select "input.required[data-msg='#{Thesis::VALIDATION_MSGS[:files]}']" - end - test 'coauthor field' do mock_auth(users(:basic)) orig_count = Thesis.count diff --git a/test/jobs/preservation_submission_job_test.rb b/test/jobs/preservation_submission_job_test.rb index 4ce72456..dcea8865 100644 --- a/test/jobs/preservation_submission_job_test.rb +++ b/test/jobs/preservation_submission_job_test.rb @@ -63,6 +63,15 @@ def stub_apt_lambda_200_failure ) end + def stub_apt_lambda_bad_request + stub_request(:post, ENV.fetch('APT_LAMBDA_URL', nil)) + .to_return( + status: 400, + body: { error: 'Invalid input payload' }.to_json, + headers: { 'Content-Type' => 'application/json' } + ) + end + test 'sends report emails on success' do stub_apt_lambda_success ClimateControl.modify DISABLE_ALL_EMAIL: 'false' do @@ -175,10 +184,18 @@ def stub_apt_lambda_200_failure assert_equal 1, another_good_thesis.archivematica_payloads.count end - test 'throws exceptions and probably creates payloads when a bad key is provided' do - skip('Test not implemented yet') - # Our lambda returns 400 Bad Request with a error body of Invalid input payload - # This should never happen as we submit it via ENV, but just in case we should understand what it looks like + test 'creates payloads when a a 400 error is returned' do + stub_apt_lambda_bad_request + thesis = setup_thesis + assert_equal 0, thesis.archivematica_payloads.count + + # The job handles the 400 error gracefully. + PreservationSubmissionJob.perform_now([thesis]) + + # Confirms that payloads are created even when a 400 Bad Request is returned, but the payload + # is not preserved. + assert_equal 1, thesis.archivematica_payloads.count + assert_equal 'unpreserved', thesis.archivematica_payloads.last.preservation_status end test 'retries on 502 Bad Gateway errors' do From df3caabdcdb97b1b6d7f3c8b7f8dffd959237932 Mon Sep 17 00:00:00 2001 From: jazairi <16103405+jazairi@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:38:19 -0700 Subject: [PATCH 2/2] Fix test failing in GitHub CI An administrate integration test was failing in CI, likely due to a race condition. The test assigns a variable to `DepartmentThesis.first`, which may not return the desired record depending on how fixtures are loaded. This commit changes that variable assignment to target the fixture needed for the test to succeed. --- test/integration/admin/admin_department_thesis_test.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/integration/admin/admin_department_thesis_test.rb b/test/integration/admin/admin_department_thesis_test.rb index 13cfe467..1d4e02f1 100644 --- a/test/integration/admin/admin_department_thesis_test.rb +++ b/test/integration/admin/admin_department_thesis_test.rb @@ -37,9 +37,8 @@ def teardown end test 'can edit department_thesis through admin dashboard' do - skip('This test is failing in GitHub Actions and passing everywhere else. We are skipping it until we fix it in CI.') mock_auth(users(:thesis_admin)) - link = DepartmentThesis.first + link = department_theses(:primary) assert_not_equal false, link.primary patch admin_department_thesis_path(link), params: { department_thesis: { primary: false } }