Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion app/controllers/works_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,12 @@ def import_multiple(urls, options)

# collect the errors neatly, matching each error to the failed url
unless failed_urls.empty?
error_msgs = 0.upto(failed_urls.length).map { |index| "<dt>#{failed_urls[index]}</dt><dd>#{errors[index]}</dd>" }.join("\n")
error_msgs = (0...failed_urls.length).map do |index|
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.upto(failed_urls.length) includes failed_urls.length, so failed_url[index] was returning nil in the last iteration. Using ... to make the range exclusive, but also happy to change it to 0.upto(failed_urls.length - 1) if that reads better.

# each failed url may have multiple errors, so show them in a bulleted list underneath the url
errors_per_url = errors[index].map { |error| "<li>#{error}</li>" }
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming by this point any content in the error messages (from validation, etc.) have been properly sanitized by StoryParser already? Or is that something I should be concerned about here?

.join("\n")
"<dt>#{failed_urls[index]}</dt><ul>#{errors_per_url}</ul>"
end.join("\n")
flash.now[:error] = "<h3>#{ts('Failed Imports')}</h3><dl>#{error_msgs}</dl>".html_safe
end

Expand Down
2 changes: 1 addition & 1 deletion app/models/story_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ def import_from_urls(urls, options = {})
works << work
else
failed_urls << url
errors << work.errors.values.join(", ")
errors << work.errors.full_messages
work.delete if work
end
rescue Timeout::Error
Expand Down
33 changes: 33 additions & 0 deletions spec/controllers/works/works_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,37 @@
end
end
end

describe ".import_multiple" do
context "when the import has a failure from at least one URL" do
before do
allow_any_instance_of(StoryParser).to receive(:import_from_urls).and_return([
[],
urls,
[["failedurl1 first error", "failedurl1 second error"], ["failedurl2 first error"]]
])
allow(controller).to receive(:render).with(:new_import)
end
let(:urls) { ["https://www.failedurl1.com", "https://www.failedurl2.com"] }

it "returns a well-formatted error message" do
expect(controller).to receive(:render).with(:new_import)
controller.send(:import_multiple, urls, {})
expect(flash[:error]).to eq(
"<h3>Failed Imports</h3>" \
"<dl>" \
"<dt>https://www.failedurl1.com</dt>" \
"<ul>" \
"<li>failedurl1 first error</li>\n" \
"<li>failedurl1 second error</li>" \
"</ul>\n" \
"<dt>https://www.failedurl2.com</dt>" \
"<ul>" \
"<li>failedurl2 first error</li>" \
"</ul>" \
"</dl>"
)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this is a bit ugly! I wanted to test (1) that the render actually succeeded instead of getting a 500 error and (2) the formatting changes look as expected. Let me know if any changes are preferred here.

end
end
end
end
Loading