diff --git a/app/controllers/ingest_from_ftp_controller.rb b/app/controllers/ingest_from_ftp_controller.rb index d50616311..0b5e382f2 100644 --- a/app/controllers/ingest_from_ftp_controller.rb +++ b/app/controllers/ingest_from_ftp_controller.rb @@ -13,14 +13,21 @@ def list_packages end def ingest_packages + selected_filenames = params[:selected_filenames] + if selected_filenames.blank? + flash[:alert] = 'No packages were chosen' + redirect_to ingest_from_ftp_path(source: source) + return + end + selected_filepaths = list_selected_package_paths(selected_filenames) # Prepopulate statuses for packages so we can immediately view a report - ingest_status_service.initialize_statuses(list_package_files.map { |f| File.basename(f) }) + ingest_status_service.initialize_statuses(selected_filepaths.map { |f| File.basename(f) }) if source == 'proquest' - IngestFromProquestJob.perform_later(user_id) + IngestFromProquestJob.perform_later(user_id, selected_filepaths) else - IngestFromSageJob.perform_later(user_id) + IngestFromSageJob.perform_later(user_id, selected_filepaths) end - redirect_to ingest_from_ftp_status_path(source: @source) + redirect_to ingest_from_ftp_status_path(source: source) end def view_status @@ -32,12 +39,35 @@ def view_status @status_results = statuses.sort.to_h end + def delete_packages + selected_filenames = params[:selected_filenames] + if selected_filenames.blank? + flash[:alert] = 'No packages were chosen' + redirect_to ingest_from_ftp_path(source: source) + return + end + list_selected_package_paths(selected_filenames).each do |package_path| + File.delete(package_path) + end + redirect_to ingest_from_ftp_path(source: source) + end + private def list_package_files Dir[File.join(storage_base_path, '*.zip')] end + def list_selected_package_paths(selected_filenames) + selected_package_paths = [] + list_package_files.each do |package_path| + if selected_filenames.any? { |filename| File.basename(package_path) == filename } + selected_package_paths << package_path + end + end + selected_package_paths + end + def build_package_listing package_results = [] list_package_files.each do |filename| diff --git a/app/jobs/ingest_from_source_job.rb b/app/jobs/ingest_from_source_job.rb index 4ee3040d7..bf924d1a1 100644 --- a/app/jobs/ingest_from_source_job.rb +++ b/app/jobs/ingest_from_source_job.rb @@ -3,11 +3,11 @@ class IngestFromSourceJob < Hyrax::ApplicationJob queue_as Hyrax.config.ingest_queue_name - def perform(user) + def perform(user, selected_filepaths) @user = user start = Time.now Rails.logger.info("Starting ingest job for #{source}") - ingest_service.process_all_packages + ingest_service.process_packages(selected_filepaths) Rails.logger.debug("Ingest job for #{source} completed in #{Time.now - start}") end diff --git a/app/services/tasks/ingest_service.rb b/app/services/tasks/ingest_service.rb index b8465d10f..bfe7e40b8 100644 --- a/app/services/tasks/ingest_service.rb +++ b/app/services/tasks/ingest_service.rb @@ -58,13 +58,17 @@ def unzip_dir(package_path) end def process_all_packages - logger.info("Beginning ingest of #{count} #{ingest_source} packages") - @status_service.initialize_statuses(package_paths) + process_packages(package_paths) + end + + def process_packages(file_paths) + logger.info("Beginning ingest of #{file_paths.count} #{ingest_source} packages") + @status_service.initialize_statuses(file_paths) - package_paths.each.with_index(1) do |package_path, index| + file_paths.each do |package_path| begin @status_service.status_in_progress(package_path) - process_package(package_path, index) + process_package(package_path) @status_service.status_complete(package_path) rescue => error stacktrace = error.backtrace.join('\n') @@ -75,6 +79,7 @@ def process_all_packages logger.info("Completing ingest of #{ingest_source} packages.") end + # the paths of all the packages in the ingest directory def package_paths # sort zip files for tests @package_paths ||= Dir.glob("#{@package_dir}/*.zip").sort diff --git a/app/services/tasks/proquest_ingest_service.rb b/app/services/tasks/proquest_ingest_service.rb index 193c177ba..b223a6d18 100644 --- a/app/services/tasks/proquest_ingest_service.rb +++ b/app/services/tasks/proquest_ingest_service.rb @@ -28,7 +28,7 @@ def deposit_package_subtype 'ProQuest' end - def process_package(package_path, _index) + def process_package(package_path) @file_last_modified = '' unzipped_package_dir = unzip_dir(package_path) diff --git a/app/services/tasks/sage_ingest_service.rb b/app/services/tasks/sage_ingest_service.rb index 0e1abc0a5..423ac2cb0 100644 --- a/app/services/tasks/sage_ingest_service.rb +++ b/app/services/tasks/sage_ingest_service.rb @@ -19,7 +19,7 @@ def self.is_revision?(filename) File.basename(filename).match?(/\.r[0-9]{4}-[0-9]{2}-[0-9]{2}/) end - def process_package(package_path, _index) + def process_package(package_path) unzipped_package_dir = unzip_dir(package_path) file_names = extract_files(package_path).keys diff --git a/app/views/ingest_from_ftp/list_packages.html.erb b/app/views/ingest_from_ftp/list_packages.html.erb index 421c8aad3..03e5a508a 100644 --- a/app/views/ingest_from_ftp/list_packages.html.erb +++ b/app/views/ingest_from_ftp/list_packages.html.erb @@ -14,18 +14,22 @@ <%= link_to 'Sage', main_app.ingest_from_ftp_path(source: 'sage'), class: "nav-link#{' active' if @source == 'sage'}" %> + <%= form_tag main_app.ingest_from_ftp_path(source: @source), method: :post do %>

Ingest from <%= @source %>

- <%= button_to 'Ingest Packages', main_app.ingest_from_ftp_path(source: @source), method: :post, class: 'btn btn-danger', data: { confirm: 'Are you sure you want to ingest all packages?' } %> + <%= submit_tag 'Ingest Packages', class: 'btn btn-danger', data: { confirm: 'Are you sure you want to ingest selected packages?' } %>
<%= link_to 'View Status of last Ingest', main_app.ingest_from_ftp_status_path(source: @source), method: :get, class: 'btn btn-default' %>
- +
+ <%= submit_tag 'Delete Selected Packages', class: 'btn', formaction: main_app.delete_from_ftp_path(source: @source), data: { confirm: 'Are you sure you want to delete selected packages?' } %> +
+ <% if @needs_revision_flag %> @@ -36,6 +40,8 @@ <% @package_results.each do |entry| %> + @@ -52,6 +58,19 @@
<%= check_box_tag 'select_all' %> Package Received Date
+ <%= check_box_tag 'selected_filenames[]', entry[:filename], false, class: 'checkbox' %> <%= entry[:filename] %>
+ <% end %> + diff --git a/config/routes.rb b/config/routes.rb index 9a12005d7..45f65cf31 100755 --- a/config/routes.rb +++ b/config/routes.rb @@ -18,6 +18,7 @@ get 'ingest_from_ftp', to: 'ingest_from_ftp#list_packages', controller: 'ingest_from_ftp' post 'ingest_from_ftp', to: 'ingest_from_ftp#ingest_packages', controller: 'ingest_from_ftp' get 'ingest_from_ftp_status', to: 'ingest_from_ftp#view_status', controller: 'ingest_from_ftp' + post 'delete_from_ftp', to: 'ingest_from_ftp#delete_packages', controller: 'ingest_from_ftp' concern :oai_provider, BlacklightOaiProvider::Routes.new diff --git a/spec/controllers/ingest_from_ftp_controller_spec.rb b/spec/controllers/ingest_from_ftp_controller_spec.rb index 9655137ae..2eaf28c74 100644 --- a/spec/controllers/ingest_from_ftp_controller_spec.rb +++ b/spec/controllers/ingest_from_ftp_controller_spec.rb @@ -83,21 +83,38 @@ before do allow(controller).to receive(:authorize!).with(:read, :admin_dashboard).and_return(true) allow(controller).to receive(:current_user).and_return(user) - allow(IngestFromProquestJob).to receive(:perform_later).with(user.uid) - allow(IngestFromSageJob).to receive(:perform_later).with(user.uid) + allow(IngestFromProquestJob).to receive(:perform_later).with(user.uid, anything) + allow(IngestFromSageJob).to receive(:perform_later).with(user.uid, anything) end - it 'submits a proquest ingest job and goes to results page' do - post :ingest_packages, params: { source: 'proquest'}, session: valid_session - expect(IngestFromProquestJob).to have_received(:perform_later).with(user.uid) + it 'ingests a proquest job and goes to the status page' do + post :ingest_packages, params: { source: 'proquest', selected_filenames: ['etdadmin_upload_3806.zip'] }, session: valid_session + expect(IngestFromProquestJob).to have_received(:perform_later).with(user.uid, [proquest_package1]) expect(response).to redirect_to(ingest_from_ftp_status_path(source: 'proquest')) end - it 'submits a sage ingest job and goes to results page' do - post :ingest_packages, params: { source: 'sage'}, session: valid_session - expect(IngestFromSageJob).to have_received(:perform_later).with(user.uid) + it 'ingests a sage job and goes to the status page' do + post :ingest_packages, params: { source: 'sage', selected_filenames: ['1177_01605976231158397.zip'] }, session: valid_session + expect(IngestFromSageJob).to have_received(:perform_later).with(user.uid, [sage_package1]) expect(response).to redirect_to(ingest_from_ftp_status_path(source: 'sage')) end + + it 'ingests a selection of packages and goes to the status page' do + post :ingest_packages, params: { source: 'proquest', selected_filenames: ['etdadmin_upload_3806.zip', 'etdadmin_upload_942402.zip']}, + session: valid_session + expect(IngestFromProquestJob).to have_received(:perform_later) do |arg1, arg2| + expect(arg1).to eq(user.uid) + expect(arg2).to match_array([proquest_package1, proquest_package2]) + end + expect(response).to redirect_to(ingest_from_ftp_status_path(source: 'proquest')) + end + + it 'returns to same page with alert if no packages were chosen' do + post :ingest_packages, params: { source: 'proquest', selected_filenames: []}, + session: valid_session + expect(response).to redirect_to(ingest_from_ftp_path(source: 'proquest')) + expect(flash[:alert]).to be_present + end end context 'as a non-admin' do @@ -150,4 +167,63 @@ end end end + + describe 'POST #delete_packages' do + context 'as an admin' do + before do + allow(controller).to receive(:authorize!).with(:read, :admin_dashboard).and_return(true) + allow(controller).to receive(:current_user).and_return(user) + end + + it 'deletes a proquest ingest package and returns to the same page' do + selected_filenames = ['etdadmin_upload_3806.zip'] + post :delete_packages, params: { source: 'proquest', selected_filenames: selected_filenames }, session: valid_session + expect(response).to redirect_to(ingest_from_ftp_path(source: 'proquest')) + + package_filepaths = Dir[File.join(proquest_dir.to_s, '*.zip')] + expect(package_filepaths.length).to eq 1 + expect(deleted_files_exist?(package_filepaths, selected_filenames)).to eq false + end + + it 'deletes a sage ingest job and returns to the same page' do + selected_filenames = ['1177_01605976231158397.zip'] + post :delete_packages, params: { source: 'sage', selected_filenames: selected_filenames }, session: valid_session + expect(response).to redirect_to(ingest_from_ftp_path(source: 'sage')) + + package_filepaths = Dir[File.join(sage_dir.to_s, '*.zip')] + expect(package_filepaths.length).to eq 1 + expect(deleted_files_exist?(package_filepaths, selected_filenames)).to eq false + end + + it 'deletes an array of packages and returns to the same page' do + post :delete_packages, params: { source: 'proquest', selected_filenames: ['etdadmin_upload_3806.zip', 'etdadmin_upload_942402.zip']}, + session: valid_session + expect(response).to redirect_to(ingest_from_ftp_path(source: 'proquest')) + + package_filepaths = Dir[File.join(proquest_dir.to_s, '*.zip')] + expect(package_filepaths).to be_empty + end + + it 'returns to same page with alert if no packages were chosen' do + post :delete_packages, params: { source: 'proquest', selected_filenames: []}, + session: valid_session + expect(response).to redirect_to(ingest_from_ftp_path(source: 'proquest')) + expect(flash[:alert]).to be_present + end + end + + context 'as a non-admin' do + it 'returns an unauthorized response' do + post :delete_packages, params: {}, session: valid_session + expect(response).to redirect_to new_user_session_path + end + end + + def deleted_files_exist?(filename_array, deleted_filenames) + existing_filenames = filename_array.map { |path| File.basename(path) } + existing_filenames.intersection(deleted_filenames).present? + end + end + + end diff --git a/spec/jobs/ingest_from_proquest_job_spec.rb b/spec/jobs/ingest_from_proquest_job_spec.rb index 6588fbe8c..5cf934838 100644 --- a/spec/jobs/ingest_from_proquest_job_spec.rb +++ b/spec/jobs/ingest_from_proquest_job_spec.rb @@ -41,8 +41,10 @@ Sipity::WorkflowState.create(workflow_id: workflow.id, name: 'deposited') end + let(:selected_filepaths) { ['spec/fixtures/proquest/proquest-attach0.zip', 'spec/fixtures/proquest/proquest-attach7.zip'] } + it 'triggers proquest ingest' do - expect { job.perform(admin.uid) }.to change { Dissertation.count }.by(1).and change { DepositRecord.count }.by(1) + expect { job.perform(admin.uid, selected_filepaths) }.to change { Dissertation.count }.by(1).and change { DepositRecord.count }.by(1) statuses = Tasks::IngestStatusService.status_service_for_source('proquest').load_statuses expect(statuses['proquest-attach0.zip']['status']).to eq 'Complete' expect(statuses['proquest-attach7.zip']['status']).to eq 'Failed' diff --git a/spec/jobs/ingest_from_sage_job_spec.rb b/spec/jobs/ingest_from_sage_job_spec.rb index 3d1d69f9b..1496025b4 100644 --- a/spec/jobs/ingest_from_sage_job_spec.rb +++ b/spec/jobs/ingest_from_sage_job_spec.rb @@ -21,6 +21,7 @@ end let(:temp_storage) { Dir.mktmpdir } let(:staging_path) { Dir.mktmpdir } + let(:filepath) { File.join(staging_path, 'AJH_2021_38_4_10.1177_1049909120951088.zip') } around do |example| cached_sage = ENV['INGEST_SAGE_PATH'] @@ -48,11 +49,11 @@ allow(RegisterToLongleafJob).to receive(:perform_later).and_return(nil) # stub FITS characterization allow(CharacterizeJob).to receive(:perform_later) - FileUtils.cp('spec/fixtures/sage/AJH_2021_38_4_10.1177_1049909120951088.zip', File.join(staging_path, 'AJH_2021_38_4_10.1177_1049909120951088.zip')) + FileUtils.cp('spec/fixtures/sage/AJH_2021_38_4_10.1177_1049909120951088.zip', filepath) end it 'triggers proquest ingest' do - expect { job.perform(admin.uid) }.to change { Article.count }.by(1).and change { DepositRecord.count }.by(1) + expect { job.perform(admin.uid, [filepath]) }.to change { Article.count }.by(1).and change { DepositRecord.count }.by(1) statuses = Tasks::IngestStatusService.status_service_for_source('sage').load_statuses expect(statuses['AJH_2021_38_4_10.1177_1049909120951088.zip']['status']).to eq 'Complete' end diff --git a/spec/services/tasks/proquest_ingest_service_spec.rb b/spec/services/tasks/proquest_ingest_service_spec.rb index 9bad709c1..4d3f97266 100644 --- a/spec/services/tasks/proquest_ingest_service_spec.rb +++ b/spec/services/tasks/proquest_ingest_service_spec.rb @@ -133,6 +133,28 @@ end end + # most processing functionality is tested in process_all_packages + # this test is to make sure only selected packages are processed + describe '#process_packages' do + before do + Dissertation.delete_all + end + + let(:filepath_array) { ['spec/fixtures/proquest/proquest-attach0.zip'] } + let(:service) { Tasks::ProquestIngestService.new(config, status_service) } + + it 'ingests selected proquest records' do + expect { service.process_packages(filepath_array) }.to change { Dissertation.count }.by(1).and change { DepositRecord.count }.by(1) + + statuses = status_service.load_statuses + expect(statuses.size).to eq 1 + package1 = statuses['proquest-attach0.zip'] + expect(package1['status']).to eq 'Complete' + expect(package1['status_timestamp']).to_not be_nil + expect(package1['error']).to be_nil + end + end + describe '#extract_files' do let(:service) { Tasks::ProquestIngestService.new(config, status_service) } let(:zip_path) { 'spec/fixtures/proquest/proquest-attach0.zip' } diff --git a/spec/services/tasks/sage_ingest_service_spec.rb b/spec/services/tasks/sage_ingest_service_spec.rb index 5adb0c456..5ecdf00fb 100644 --- a/spec/services/tasks/sage_ingest_service_spec.rb +++ b/spec/services/tasks/sage_ingest_service_spec.rb @@ -92,7 +92,7 @@ it 'creates the work as if it were new' do work_id = nil - expect { work_id = service.process_package("spec/fixtures/sage/revisions/both_changed/#{package_name}", 0) } + expect { work_id = service.process_package("spec/fixtures/sage/revisions/both_changed/#{package_name}") } .to change { Article.count }.by(1) .and change { FileSet.count }.by(2) work = ActiveFedora::Base.find(work_id) @@ -110,7 +110,7 @@ it 'adds new filesets to the existing work, and sets warnings' do work_id = work.id - expect { service.process_package("spec/fixtures/sage/revisions/both_changed/#{package_name}", 0) } + expect { service.process_package("spec/fixtures/sage/revisions/both_changed/#{package_name}") } .to change { Article.count }.by(0) .and change { FileSet.count }.by(2) work = ActiveFedora::Base.find(work_id) @@ -131,7 +131,7 @@ context 'with existing work' do before do - @work_id = service.process_package('spec/fixtures/sage/revisions/new/ASU_2022_88_10_10.1177_00031348221074228.zip', 0) + @work_id = service.process_package('spec/fixtures/sage/revisions/new/ASU_2022_88_10_10.1177_00031348221074228.zip') end context 'revision indicating xml changed' do @@ -147,7 +147,7 @@ pdf_fs = file_sets.detect { |fs| fs.label.end_with?('.pdf') } pdf_date_modified = pdf_fs.date_modified - expect { service.process_package('spec/fixtures/sage/revisions/metadata_changed/ASU_2022_88_10_10.1177_00031348221074228.r2022-12-19.zip', 0) } + expect { service.process_package('spec/fixtures/sage/revisions/metadata_changed/ASU_2022_88_10_10.1177_00031348221074228.r2022-12-19.zip') } .to change { Article.count }.by(0) .and change { FileSet.count }.by(0) work = ActiveFedora::Base.find(@work_id) @@ -172,7 +172,7 @@ pdf_fs = file_sets.detect { |fs| fs.label.end_with?('.pdf') } pdf_date_modified = pdf_fs.date_modified - expect { service.process_package('spec/fixtures/sage/revisions/file_changed/ASU_2022_88_10_10.1177_00031348221074228.r2022-12-20.zip', 0) } + expect { service.process_package('spec/fixtures/sage/revisions/file_changed/ASU_2022_88_10_10.1177_00031348221074228.r2022-12-20.zip') } .to change { Article.count }.by(0) .and change { FileSet.count }.by(0) work = ActiveFedora::Base.find(@work_id) @@ -195,7 +195,7 @@ pdf_fs = file_sets.detect { |fs| fs.label.end_with?('.pdf') } pdf_date_modified = pdf_fs.date_modified - expect { service.process_package('spec/fixtures/sage/revisions/both_changed/ASU_2022_88_10_10.1177_00031348221074228.r2022-12-22.zip', 0) } + expect { service.process_package('spec/fixtures/sage/revisions/both_changed/ASU_2022_88_10_10.1177_00031348221074228.r2022-12-22.zip') } .to change { Article.count }.by(0) .and change { FileSet.count }.by(0) work = ActiveFedora::Base.find(@work_id) @@ -212,7 +212,7 @@ let(:package_name) { 'ASU_2022_88_10_10.1177_00031348221074228.zip' } it 'skips the duplicate work and records an error' do - expect { service.process_package("spec/fixtures/sage/revisions/new/#{package_name}", 0) } + expect { service.process_package("spec/fixtures/sage/revisions/new/#{package_name}") } .to raise_error("Work #{@work_id} already exists with DOI https://doi.org/10.1177/00031348221074228, skipping package #{package_name}") .and change { Article.count }.by(0) .and change { FileSet.count }.by(0) @@ -220,6 +220,24 @@ end end end + + # most processing functionality is tested in process_package + # this test is to make sure only selected packages are processed + describe '#process_packages' do + it 'processes selected package' do + expect { service.process_packages([first_zip_path]) } + .to change { Article.count }.by(1) + statuses = status_service.load_statuses + expect(statuses.size).to eq 1 + end + + it 'processes multiple selected packages' do + expect { service.process_packages([first_zip_path, last_zip_path]) } + .to change { Article.count }.by(2) + statuses = status_service.load_statuses + expect(statuses.size).to eq 2 + end + end end context 'without running the background jobs' do