Merge pull request #4037 from alphagov/2498-sentry-errors-fix-fronten…

…d-undefinedconversionerror-m

Sentry Errors: Fix Frontend UndefinedConversionError

app/controllers/csv_preview_controller.rb

@@ -2,7 +2,7 @@
 
 class CsvPreviewController < ApplicationController
   rescue_from GdsApi::HTTPForbidden, with: :access_limited
-  rescue_from CSV::MalformedCSVError, with: :malformed_csv
+  rescue_from CSV::MalformedCSVError, CsvPreviewService::FileEncodingError, with: :malformed_csv
 
   def show
     @asset = GdsApi.asset_manager.asset(params[:id]).to_hash

app/services/csv_preview_service.rb

@@ -2,6 +2,8 @@ class CsvPreviewService
   MAXIMUM_COLUMNS = 50
   MAXIMUM_ROWS = 1000
 
+  class FileEncodingError < StandardError; end
+
   attr_reader :truncated
 
   def initialize(csv)
@@ -84,5 +86,7 @@ def converted_and_restricted_csv(parsed_csv)
       self.truncated ||= (columns.length > MAXIMUM_COLUMNS)
       columns.take(MAXIMUM_COLUMNS)
     end
+  rescue Encoding::UndefinedConversionError
+    raise FileEncodingError, "Character cannot be converted"
   end
 end

test/integration/csv_preview_test.rb

@@ -101,9 +101,9 @@ class CsvPreviewTest < ActionDispatch::IntegrationTest
     end
   end
 
-  context "when visiting the preview with special characters in the CSV" do
+  context "when visiting the preview with malformed CSV" do
     setup do
-      setup_asset_manager(parent_document_url, asset_manager_id, filename, use_special_characters_csv: true)
+      setup_asset_manager(parent_document_url, asset_manager_id, filename, malformed: true)
       setup_content_item(asset_media_url_path, parent_document_base_path)
 
       visit "/#{asset_media_url_path}/preview"
@@ -119,7 +119,7 @@ class CsvPreviewTest < ActionDispatch::IntegrationTest
 
     context "when visiting the preview that redirects to other asset" do
       setup do
-        setup_asset_manager(parent_document_url, asset_manager_id, filename, use_special_characters_csv: true, asset_deleted: true)
+        setup_asset_manager(parent_document_url, asset_manager_id, filename, malformed: true, asset_deleted: true)
         setup_content_item(asset_media_url_path, parent_document_base_path)
 
         visit "/#{asset_media_url_path}/preview"
@@ -131,6 +131,23 @@ class CsvPreviewTest < ActionDispatch::IntegrationTest
     end
   end
 
+  context "when visiting the preview with nonconvertible characters in the CSV" do
+    setup do
+      setup_asset_manager(parent_document_url, asset_manager_id, filename, nonconvertible: true)
+      setup_content_item(asset_media_url_path, parent_document_base_path)
+
+      visit "/#{asset_media_url_path}/preview"
+    end
+
+    should "return a 200 response" do
+      assert_equal 200, page.status_code
+    end
+
+    should "include the error message" do
+      assert page.has_text?("This CSV cannot be viewed online.")
+    end
+  end
+
   context "when the asset is draft and not served from the draft host" do
     setup do
       asset_manager_response = {
@@ -236,16 +253,18 @@ class CsvPreviewTest < ActionDispatch::IntegrationTest
     assert page.has_title?("Attachment 2 - GOV.UK")
   end
 
-  def setup_asset_manager(parent_document_url, asset_manager_id, filename = nil, use_special_characters_csv: false, asset_deleted: false, media_code: 200)
+  def setup_asset_manager(parent_document_url, asset_manager_id, filename = nil, malformed: false, nonconvertible: false, asset_deleted: false, media_code: 200)
     asset_manager_response = {
       id: "https://asset-manager.dev.gov.uk/assets/foo",
       parent_document_url:,
       deleted: asset_deleted,
     }
     stub_asset_manager_has_an_asset(asset_manager_id, asset_manager_response, filename)
 
-    csv_file = if use_special_characters_csv
-                 "\xEF\xBB\xBF\"Field 1\",\"Field 2\",\"Field 3 \n(details)\",\"Field 4 (\xC2\xA3m)\"\n\"Value 1\",\"Value 2\",\"Value 3\",\"Value 4 \xC2\xA33.8bn\"\n"
+    csv_file = if malformed
+                 " \"Field 1\"\n"
+               elsif nonconvertible
+                 "\x8D\n"
                else
                  generate_test_csv(51, 1010)
                end

test/unit/services/csv_preview_service_test.rb

@@ -82,6 +82,16 @@ class CsvPreviewServiceTest < ActiveSupport::TestCase
         assert_equal [[{ text: "þær he feoll his twægen gebroðra" }]], subject.first
       end
     end
+
+    context "with CSV with bytes that cannot be converted to UTF-8" do
+      subject { CsvPreviewService.new("F\x8ed\x8eration Fran\x8daise\n") }
+
+      should "raise FileEncodingError" do
+        assert_raises CsvPreviewService::FileEncodingError do
+          subject.csv_rows
+        end
+      end
+    end
   end
 
   context "#newline_or_last_char_index" do