Fixes/improvements to scraping

* Don't crash the entire scraper task when loading legacy config
* Log full backtrace in case of crash
* Ensure all source methods are scraped, regardless of whether they are in the user-permitted set

Author: fbacall

app/models/source.rb

@@ -20,7 +20,7 @@ class Source < ApplicationRecord
   validates :url, url: true
   validates :approval_status, inclusion: { in: APPROVAL_STATUS.values }
   validates :method, inclusion: { in: -> (_) { TeSS::Config.user_ingestion_methods } },
-            unless: -> { User.current_user&.is_admin? }
+            unless: -> { User.current_user&.is_admin? || User.current_user&.has_role?(:scraper_user) }
   validate :check_method
 
   before_create :set_approval_status
@@ -85,10 +85,7 @@ def self.enabled
   end
 
   def check_method
-    c = ingestor_class rescue nil
-    if c.nil?
-      errors.add(:method, 'is invalid')
-    end
+    errors.add(:method, 'is invalid') unless Ingestors::IngestorFactory.valid_ingestor?(method)
   end
 
   def self.approved

lib/ingestors/ingestor_factory.rb

@@ -27,6 +27,10 @@ def self.get_ingestor(method)
       end
     end
 
+    def self.valid_ingestor?(method)
+      ingestor_config.key?(method)
+    end
+
     def self.grouped_options
       @grouped_options ||= ingestor_config.values.group_by { |c| c[:category] || :any }
     end

lib/scraper.rb

@@ -20,6 +20,10 @@ def provider_exists
         errors.delete(:content_provider)
       end
     end
+
+    def resource_type=(*args)
+      warn %(The "resource_type" property for a source is now redundant ("#{@provider}" in config/ingestion.yml))
+    end
   end
 
   attr_reader :log_file, :name, :username, :sources
@@ -59,6 +63,7 @@ def run
       if user.role.nil? or user.role.name != @default_role
         log t('scraper.messages.invalid', error_message: t('scraper.messages.bad_role')), 1
       end
+      User.current_user = user
 
       processed = 0
 
@@ -146,7 +151,9 @@ def run
 
     rescue Exception => e
       log "   Run Scraper failed with: #{e.message}", 0
-      log "       #{e.backtrace[0]}", 0
+      e.backtrace.each do |line|
+        log "       #{line}", 0
+      end
     end
 
     # wrap up

test/config/test_ingestion_crash.yml

@@ -0,0 +1,7 @@
+name: crash
+logfile: log/ingestion.test.log
+loglevel: 0
+username: ingestor
+sources:
+  - id: 1
+    jgrhierjvrv: gjigdfgidf

test/config/test_ingestion_legacy.yml

@@ -0,0 +1,11 @@
+name: legacy
+logfile: log/ingestion.test.log
+loglevel: 0
+username: ingestor
+sources:
+  - id: 1
+    provider: 'Portal Provider'         # content provider's title
+    url: https://zenodo.org/api/records/?communities=ardc  # the root URL to access the source
+    method: rest                        # one of 'csv', 'rest'
+    resource_type: material
+    enabled: true

test/unit/ingestors/scraper_test.rb

@@ -60,8 +60,8 @@ def run
 
     # check user does exist
     user = User.find_by_username(scraper.username)
-    assert !user.nil?
-    assert !user.role.nil?
+    refute user.nil?
+    refute user.role.nil?
     assert_equal 'registered_user', user.role.name
 
     # run task
@@ -124,7 +124,7 @@ def run
     assert scraper.sources.size > 0
 
     source = scraper.sources[0]
-    assert !source.nil?
+    refute source.nil?
     title = source[:provider]
     provider = ContentProvider.find_by_title(title)
     assert provider.nil?
@@ -148,10 +148,10 @@ def run
     assert scraper.sources.size > 1
 
     source = scraper.sources[1]
-    assert !source.nil?
+    refute source.nil?
     title = source[:provider]
     provider = ContentProvider.find_by_title(title)
-    assert !provider.nil?, "Provider title[#{title}] not found!"
+    refute provider.nil?, "Provider title[#{title}] not found!"
 
     # run task
     freeze_time(stub_time = Time.new(2019)) do
@@ -160,7 +160,7 @@ def run
 
     assert check_task_finished(logfile)
     error_message = 'Content provider must exist: ' + title.to_s
-    assert !logfile_contains(logfile, error_message), "Unexpected error message: #{error_message}"
+    refute logfile_contains(logfile, error_message), "Unexpected error message: #{error_message}"
   end
 
   test 'check for invalid source parameters' do
@@ -184,6 +184,55 @@ def run
     assert logfile_contains(logfile, error_message), 'Error message not found: ' + error_message
   end
 
+  test 'handles non-user ingestion methods' do
+    with_settings(user_ingestion_methods: ['bioschemas']) do
+      config = load_scraper_config('test_ingestion.yml')
+      scraper = Scraper.new(config)
+      logfile = scraper.log_file
+      assert_equal 'test', scraper.name
+
+      freeze_time(stub_time = Time.new(2019)) do
+        scraper.run
+      end
+
+      refute logfile_contains(logfile, 'Method is not included in the list: event_csv')
+      refute logfile_contains(logfile, 'Method is not included in the list: material_csv')
+    end
+  end
+
+  test 'does not crash for legacy config' do
+    logfile = nil
+    Kernel.silence_warnings do
+      config = load_scraper_config('test_ingestion_legacy.yml')
+      scraper = Scraper.new(config)
+      logfile = scraper.log_file
+      assert_equal 'legacy', scraper.name
+
+      freeze_time(stub_time = Time.new(2019)) do
+        scraper.run
+      end
+    end
+
+    refute logfile_contains(logfile, 'Run Scraper failed with')
+    assert logfile_contains(logfile, 'Method is invalid: rest')
+  end
+
+  test 'logs backtrace on error' do
+    config = load_scraper_config('test_ingestion_crash.yml')
+    scraper = Scraper.new(config)
+    logfile = scraper.log_file
+    assert_equal 'crash', scraper.name
+
+    Ingestors::IngestorFactory.stub(:get_ingestor, -> { raise 'oh no' }) do
+      scraper.run
+    end
+
+    assert logfile_contains(logfile, 'Run Scraper failed with')
+    assert logfile_contains(logfile, 'in `block in run')
+    assert logfile_contains(logfile, 'in `map')
+    assert logfile_contains(logfile, 'in `run')
+  end
+
   private
 
   def check_task_finished(logfile)