Merge pull request #3724 from mlibrary/HELIO-4786/marc-sftp-upload-bug

HELIO-4786 Fixes marc ingest sftp bug

app/jobs/marc_ingest_job.rb

@@ -44,6 +44,10 @@ def perform
       end
     end
 
+    report.each do |r|
+      MarcLogger.info(r)
+    end
+
     # Done. Delete all the local directories
     lfh.clean_up_processing_dir
     # Remove the original Alma file(s) that were in the remote ~/marc_ingest since they've been untarred, split and renamed
@@ -52,9 +56,6 @@ def perform
     end
 
     MarcLogger.info("MarcIngestJob finished")
-    report.each do |r|
-      MarcLogger.info(r)
-    end
 
     MarcIngestMailer.send_mail(report).deliver_now if report.count > 0
   end

app/services/marc/sftp.rb

@@ -32,27 +32,25 @@ def download_marc_ingest_files
     end
 
     def upload_local_marc_file_to_remote_product_dir(local_file, product_dir)
-      conn.upload!(local_file, product_dir)
-    rescue Net::SFTP::StatusException => e
-      MarcLogger.error("Marc::Sftp could not upload!(#{from}, #{to})")
-      MarcLogger.error(e)
+      remote_file = File.join(product_dir, File.basename(local_file))
+      conn.upload!(local_file, remote_file)
+    rescue Net::SFTP::StatusException, StandardError => e
+      MarcLogger.error("Marc::Sftp could not upload!(#{local_file}, #{remote_file}): #{e}")
     end
 
-
     def remove_marc_ingest_file(file)
-      file = File.basename(file)
-      conn.remove!(File.join("/home/fulcrum_ftp/marc_ingest", file))
-      MarcLogger.info("Marc::Sftp removed #{file}")
+      remote_file = File.join("/home/fulcrum_ftp/marc_ingest", File.basename(file))
+      conn.remove!(remote_file)
+      MarcLogger.info("Marc::Sftp removed #{remote_file}")
     rescue Net::SFTP::StatusException => e
-      MarcLogger.error("Marc::Sftp could not remove!(#{file}), #{e}")
+      MarcLogger.error("Marc::Sftp could not remove!(#{remote_file}), #{e}")
     end
 
     def upload_local_marc_file_to_remote_failures(local_file)
-      remote_failure_file = File.join("/home/fulcrum_ftp/marc_ingest/failures", "#{File.basename(local_file)}")
-      conn.upload!(local_file, remote_failure_file)
-    rescue Net::SFTP::StatusException => e
-      MarcLogger.error("Marc::Sftp could not rename!(#{from}, #{to})")
-      MarcLogger.error(e)
+      remote_file = File.join("/home/fulcrum_ftp/marc_ingest/failures", File.basename(local_file))
+      conn.upload!(local_file, remote_file)
+    rescue Net::SFTP::StatusException, StandardError => e
+      MarcLogger.error("Marc::Sftp could not rename!(#{local_file}, #{remote_file}): #{e}")
     end
 
     def local_marc_processing_dir

spec/services/marc/sftp_spec.rb

@@ -2,6 +2,8 @@
 
 require 'rails_helper'
 require 'net/sftp'
+require 'rspec/support/differ'
+
 
 # Lots of mocks here unsurprisingly. It's pretty gross.
 RSpec.describe Marc::Sftp do
@@ -103,20 +105,38 @@
     let(:sftp_session) { instance_double(Net::SFTP::Session) }
     let(:local_file) { File.join(Settings.scratch_space_path, "marc_processing", "test_dir", "test_file.xml") }
     let(:product_dir) { "/home/fulcrum_ftp/MARC_from_Cataloging/UMPEBC" }
+    let(:remote_file) { File.join(product_dir, "test_file.xml") }
 
-    before do
-      allow(Rails.root).to receive(:join).with('config', 'fulcrum_sftp.yml').and_return(config)
-      allow(File).to receive(:exist?).with(config).and_return(true)
-      allow(File).to receive(:read).with(config).and_return(true)
-      allow(YAML).to receive(:safe_load).and_return(valid_yaml)
-      allow(Net::SFTP).to receive(:start).with("hostname", "username", password: "password").and_return(sftp_session)
-      allow(sftp_session).to receive(:upload!)
+    context "when successful" do
+      before do
+        allow(Rails.root).to receive(:join).with('config', 'fulcrum_sftp.yml').and_return(config)
+        allow(File).to receive(:exist?).with(config).and_return(true)
+        allow(File).to receive(:read).with(config).and_return(true)
+        allow(YAML).to receive(:safe_load).and_return(valid_yaml)
+        allow(Net::SFTP).to receive(:start).with("hostname", "username", password: "password").and_return(sftp_session)
+        allow(sftp_session).to receive(:upload!)
+      end
+
+      it "uploads the local file to the remote product directory" do
+        sftp = described_class.new
+        sftp.upload_local_marc_file_to_remote_product_dir(local_file, product_dir)
+        expect(sftp_session).to have_received(:upload!).with(local_file, remote_file)
+      end
     end
 
-    it "uploads the local file to the remote product directory" do
-      sftp = described_class.new
-      sftp.upload_local_marc_file_to_remote_product_dir(local_file, product_dir)
-      expect(sftp_session).to have_received(:upload!).with(local_file, product_dir)
+    context "with an exception" do
+      before do
+        allow_any_instance_of(described_class).to receive(:conn).and_return(sftp_session)
+        allow(sftp_session).to receive(:upload!).and_raise(StandardError)
+        allow(MarcLogger).to receive(:error)
+      end
+
+      it "logs the exception" do
+        sftp = described_class.new
+        sftp.upload_local_marc_file_to_remote_product_dir(local_file, product_dir)
+        expect(sftp_session).to have_received(:upload!).with(local_file, remote_file)
+        expect(MarcLogger).to have_received(:error).with("Marc::Sftp could not upload!(#{local_file}, #{remote_file}): StandardError")
+      end
     end
   end
 
@@ -165,7 +185,7 @@
     let(:config) { double("config") }
     let(:sftp_session) { instance_double(Net::SFTP::Session) }
     let!(:local_file) { File.join(Settings.scratch_space_path, "marc_processing", "test", "test_00001.xml") }
-    let!(:remote_failure_file) { "/home/fulcrum_ftp/marc_ingest/failures/test_00001.xml" }
+    let!(:remote_file) { "/home/fulcrum_ftp/marc_ingest/failures/test_00001.xml" }
 
     before do
       allow(Rails.root).to receive(:join).with('config', 'fulcrum_sftp.yml').and_return(config)
@@ -185,7 +205,22 @@
       sftp = described_class.new
       sftp.upload_local_marc_file_to_remote_failures(local_file)
 
-      expect(sftp_session).to have_received(:upload!).with(local_file, remote_failure_file)
+      expect(sftp_session).to have_received(:upload!).with(local_file, remote_file)
+    end
+
+    context "with an exception" do
+      before do
+        allow(sftp_session).to receive(:upload!).and_raise(StandardError)
+        allow(MarcLogger).to receive(:error)
+      end
+
+      it 'raises and logs the exception' do
+        sftp = described_class.new
+        sftp.upload_local_marc_file_to_remote_failures(local_file)
+
+        expect(sftp_session).to have_received(:upload!).with(local_file, remote_file)
+        expect(MarcLogger).to have_received(:error).with("Marc::Sftp could not rename!(#{local_file}, #{remote_file}): StandardError")
+      end
     end
   end