Hyc 1686 manually delete package HYC 1685 select ingest packages (#986)

* HYC-1686 add controller test

* HYC-1686 make tests pass

* HYC-1686 get form to submit correctly

* HYC-1686 account for when no packages are selected

* HYC-1685 make param more flexible and remove unneeded pram

* HYC-1685 add process_packages method

* HYC-1685 proquest ingest tests pass

* HYC-1685 sage tests pass

* HYC-1685 update front end to accomodate two actions and select all checkbox

* HYC-1685 fix ingest job tests

* HYC-1685 making array order unimportant in test assertion

* HYC-1685 HYC-1686 address PR comments

---------

Co-authored-by: Sharon Luong <snluong@email.lib.unc.edu>

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|

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
 

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

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)
 

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

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'}" %>
         </li>
       </ul>
+      <%= form_tag main_app.ingest_from_ftp_path(source: @source), method: :post do %>
       <div class="panel-body">
         <h2 class="sr-only">Ingest from <%= @source %> </h2>
         <fieldset class="btn-group">
-          <%= 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?' } %>
         </fieldset>
         <fieldset class="btn-group">
           <%= link_to 'View Status of last Ingest', main_app.ingest_from_ftp_status_path(source: @source), method: :get, class: 'btn btn-default' %>
         </fieldset>
-
+        <fieldset class="btn-group">
+          <%= 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?' } %>
+        </fieldset>
         <table class="table table-sm table-striped works-list">
           <thead>
             <tr>
+              <th><%= check_box_tag 'select_all' %></th>
               <th>Package</th>
               <th>Received Date</th>
               <% if @needs_revision_flag %>
@@ -36,6 +40,8 @@
           <tbody>
             <% @package_results.each do |entry| %>
               <tr>
+                <td>
+                  <%= check_box_tag 'selected_filenames[]', entry[:filename], false, class: 'checkbox' %>
                 <td>
                   <%= entry[:filename] %>
                 </td>
@@ -52,6 +58,19 @@
           </tbody>
         </table>
       </div>
+      <% end %>
     </div>
   </div>
 </div>
+<script>
+  $(document).ready(function(){
+    $('#select_all').on("click", function() {
+      var checkboxes = $(".checkbox");
+      if (checkboxes.prop("checked")){
+        checkboxes.prop("checked", false);
+      } else {
+        checkboxes.prop("checked", true);
+      }
+    });
+  });
+</script>

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
 

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

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'

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

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' }