Skip to content

Commit

Permalink
Fix preflight exception when file row has no files (#271)
Browse files Browse the repository at this point in the history
**ISSUE**
When importing a CSV without a "files" column, preflight was throwing
an exception when trying to upack nil files.

**SOLUTION**
We don't want to make "files" a required column, because it's totally
valid to import a CSV with only Collections.  This change detects when
`files` in a a row with object type "File" is nil or empty and creates
a placeholder error `PFFile` object to store the relevant row-level warnings.

**OTHER CHANGES**
This commit also splits the Preflight and PreFlightObj tests into two
files since the preflight spec file was getting pretty long.

see spec/lib/tenejo/pre_flight_obj_spec.rb:26-40 for the new tests.
  • Loading branch information
mark-dce authored Sep 30, 2022
1 parent 4d8c3a4 commit bd2901c
Show file tree
Hide file tree
Showing 6 changed files with 362 additions and 336 deletions.
7 changes: 5 additions & 2 deletions app/lib/tenejo/pf_object.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
# frozen_string_literal: true
module Tenejo
LICENSES = YAML.safe_load(File.open(Rails.root.join("config/authorities/licenses.yml")))

class LicenseValidator < ActiveModel::Validator
def validate(record)
licenses = Array.wrap(record.license).select(&:present?)
Expand Down Expand Up @@ -203,6 +201,7 @@ def self.exist?(rec, val)
end

def self.unpack(row, lineno, import_root)
return [invalid_file(row, lineno)] if row[:files].blank?
cp = row.dup
index = 1
packed = row[:files].include?("|~|")
Expand All @@ -217,6 +216,10 @@ def self.unpack(row, lineno, import_root)
files
end

def self.invalid_file(row, lineno)
PFFile.new(row, lineno, '')
end

def initialize(row = {}, lineno = 0, import_root = true, strict_paths = true)
file_name = row.to_h.delete(:files)
row[:file] = relative_path(file_name, import_root, strict_paths)
Expand Down
1 change: 1 addition & 0 deletions spec/fixtures/csv/fancy.csv
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ MPC003,W,TESTINGCOLLECTION,"TEST Postcards - Minneapolis UNPACKED FILES - Lake H
,F,MPC003,,,,,,MN-02 4.png,Image,,,,,,,,,,,
MPC008,W,MPC003,"TEST Postcards - Minneapolis UNPACKED FILES - Lake Harriet, Minneapolis, Minn",Bloom Bros Co.,postcard|~|Minneapolis|~|Lake Harriet,https://rightsstatements.org/vocab/NoC-OKLR/1.0/,open,,Image,Postcard depicting Lake Harriet,,https://creativecommons.org/publicdomain/mark/1.0/,,1900-1945,Postcards,English,,,,
MPC009,WOrK,NONA,"TEST Postcards - Minneapolis UNPACKED FILES - Lake Harriet, Minneapolis, Minn",Bloom Bros Co.,postcard|~|Minneapolis|~|Lake Harriet,https://rightsstatements.org/vocab/NoC-OKLR/1.0/,open,,Image,Postcard depicting Lake Harriet,,https://creativecommons.org/publicdomain/mark/1.0/,,1900-1945,Postcards,English,,,,
,File,,"File missing identifier, parent, and file",,,,open,,,,,,,,,,,,,
11 changes: 6 additions & 5 deletions spec/lib/tenejo/csv_importer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,20 @@
csv_import = described_class.new(import_job)
expect(import_job.status).to eq 'submitted'
expect(csv_import.preflight_errors).to eq []
expect(csv_import.invalid_rows).to eq []
expect(csv_import.invalid_rows).to include a_hash_including('lineno' => 11)
expect(csv_import.preflight_warnings)
.to contain_exactly(
"The column \'Comment\' is unknown and will be ignored",
"Row 3: Could not find parent \'NONEXISTENT\'; collection \'NONACOLLECTION\' will be created without a parent if you continue.",
"Row 6: Could not find parent work \'WHUT?\' for file \'MN-02 2.png\' - the file will be ignored",
"Row 10: Could not find parent \'NONA\'; work \'MPC009\' will be created without a parent if you continue.",
"Row 2: Resource Type \'Photos\' is not recognized and will be omitted.",
"Row 3: Could not find parent \'NONEXISTENT\'; collection \'NONACOLLECTION\' will be created without a parent if you continue.",
"Row 3: Resource Type \'Posters\' is not recognized and will be omitted.",
"Row 5: Visibility is blank - and will be treated as private",
"Row 5: Visibility is blank - and will be treated as private",
"Row 6: Visibility is blank - and will be treated as private",
"Row 8: Visibility is blank - and will be treated as private"
"Row 8: Visibility is blank - and will be treated as private",
"Row 6: Could not find parent work \'WHUT?\' for file \'MN-02 2.png\' - the file will be ignored",
"Row 10: Could not find parent \'NONA\'; work \'MPC009\' will be created without a parent if you continue.",
"Row 11: Parent can't be blank, File can't be blank, Identifier can't be blank"
)
end
end
Expand Down
18 changes: 9 additions & 9 deletions spec/lib/tenejo/graph_spec.rb
Original file line number Diff line number Diff line change
@@ -1,32 +1,32 @@
# frozen_string_literal: true
require 'rails_helper'
RSpec.describe Tenejo::Graph do
RSpec.describe Tenejo::Graph, :aggregate_failures do
context "when empty" do
let(:g) { described_class.new }
let(:graph) { described_class.new }
it "can serialize to json" do
expect(g.to_json).not_to be_nil
expect(JSON.parse(g.to_json)).not_to be_nil
expect(JSON.parse(g.to_json).class).to eq Hash
expect(graph.to_json).not_to be_nil
expect(JSON.parse(graph.to_json)).not_to be_nil
expect(JSON.parse(graph.to_json).class).to eq Hash
end
end

context "having loaded a file" do
let(:g) {
let(:graph) {
allow(File).to receive(:exist?).and_call_original
allow(File).to receive(:exist?).with(/tiff/).and_return(true)
allow(File).to receive(:exist?).with(/png/).and_return(true)
Tenejo::Preflight.process_csv(File.open("./spec/fixtures/csv/nesting_test.csv"))
}
let(:flat) { g.flatten }
let(:flat) { graph.flatten }

let(:j) { Job.create!(user: FactoryBot.create(:user), graph: g) }
let(:j) { Job.create!(user: FactoryBot.create(:user), graph: graph) }

it "finds all the files" do
expect(j.graph.files.count).to eq 17
end

it "empties out the detached files list after attaching them" do
expect(g.detached_files).to be_empty
expect(graph.detached_files).to be_empty
end

it "is able to reify after reload" do
Expand Down
Loading

0 comments on commit bd2901c

Please sign in to comment.