culturecode · njakobsen · Aug 8, 2026
diff --git a/README.md b/README.md
@@ -232,6 +232,27 @@ The gem now relies on virtual columns to set a number of derived column values.
   end
 ```
 
+## Upgrading From 3.11 to 3.12
+`#feature_update_warnings` returns `{'file', 'message'}` hashes rather than pre-joined strings, so a caller can lay the
+file and the explanation out separately instead of rendering one sentence. Warnings stored before the upgrade come back
+with a nil file, so there is no data to migrate.
+
+```ruby
+record.feature_update_warnings
+# => [{ 'file' => 'upload.zip/layer.shp', 'message' => 'This shapefile is missing layer.shx. ...' }]
+
+# A warning stored by 3.11 or earlier, which names its file inside the message
+# => [{ 'file' => nil, 'message' => 'upload.zip: This file contains no map data.' }]
+
+# The 3.11 string, for a caller that wants to keep rendering a sentence
+record.feature_update_warnings.map {|warning| [warning['file'], warning['message']].compact.join(': ') }
+```
+
+The importer messages were also rewritten, `INVALID_ARCHIVE` and `SUPPORTED_FORMATS` included, so a caller matching on
+the text of either constant needs updating. They now state what is wrong with a file and nothing further: the gem is
+handed a file and cannot know how it arrived, so it does not tell a reader to re-export or upload anything. A host that
+knows its own workflow is the right place to add that.
+
 ## Testing
 
 Create a postgres database:

diff --git a/lib/spatial_features/has_spatial_features/feature_import.rb b/lib/spatial_features/has_spatial_features/feature_import.rb
@@ -28,6 +28,8 @@ def update_features!(skip_invalid: false, allow_blank: false, force: false, **op
       options = options.reverse_merge(spatial_features_options)
       tmpdir = options.fetch(:tmpdir) { Dir.mktmpdir("ruby_spatial_features") }
 
+      import_warnings = []
+
       ActiveRecord::Base.transaction do
         imports = spatial_feature_imports(options[:import], options[:make_valid], tmpdir)
         cache_key = Digest::MD5.hexdigest(imports.collect(&:cache_key).join)
@@ -45,29 +47,36 @@ def update_features!(skip_invalid: false, allow_blank: false, force: false, **op
             update_spatial_cache(options.slice(:spatial_cache))
           end
 
-          # Attribute each warning to the file it came from (e.g. `archive.zip/layer.kml`)
-          # so a multi-file or multi-source import makes clear which file was affected.
+          # Name the file each warning came from (e.g. `archive.zip/layer.kml`) so a multi-file
+          # or multi-source import makes clear which file was affected. The file is kept apart
+          # from the message so a reader can lay the two out separately.
           import_warnings = imports.flat_map do |import|
-            import.warnings.map {|warning| [import.source_identifier.presence, warning].compact.join(': ') }
+            import.warnings.map {|warning| { 'file' => import.source_identifier.presence, 'message' => warning } }
           end
           store_feature_update_warnings(import_warnings)
 
           if imports.present? && features.compact_blank.empty? && !allow_blank
-            raise EmptyImportError, [EMPTY_IMPORT_MESSAGE, *import_warnings].join(' ')
+            raise EmptyImportError, [EMPTY_IMPORT_MESSAGE, *import_warnings.map {|warning| warning.values.compact.join(': ') }].join(' ')
           end
         end
       end
 
       return true
     rescue StandardError => e
+      # The transaction that recorded them has rolled back, so without this a failed import
+      # explains itself only through the exception message — which reaches a reader as one
+      # unbroken paragraph, and not at all once the job is cleared.
+      store_feature_update_warnings(import_warnings) if persisted? && import_warnings.present?
+
       raise e if e.is_a?(EmptyImportError)
 
       if skip_invalid
         Rails.logger.warn "Error updating #{self.class} #{self.id}. #{e.message}"
         return nil
       elsif ENCODING_ERROR.match?(e.message)
         raise ImportEncodingError,
-              "One or more features you are trying to import has text encoded in an un-supported format (#{e.message})",
+              "This file contains text in an unsupported character encoding (#{e.message}). " \
+              "Text must be encoded as UTF-8.",
               e.backtrace
       else
         raise ImportError, e.message, e.backtrace

diff --git a/lib/spatial_features/has_spatial_features/queued_spatial_processing.rb b/lib/spatial_features/has_spatial_features/queued_spatial_processing.rb
@@ -67,14 +67,27 @@ def updating_features_failed?
       spatial_processing_status(:update_features!) == :failure
     end
 
-    # Non-fatal messages from the most recent successful feature import (e.g. parts of
-    # the source that were skipped). Stored alongside the status cache so they survive
-    # job completion, since successful Delayed::Jobs are deleted and can't be read back.
+    # Non-fatal messages from the most recent feature import (e.g. parts of the source that
+    # were skipped). Stored alongside the status cache so they survive job completion, since
+    # successful Delayed::Jobs are deleted and can't be read back.
     WARNINGS_CACHE_KEY = 'feature_update_warnings'.freeze
 
+    # Returns one entry per warning as `{'file' => String|nil, 'message' => String}`.
+    #
+    # The pair is kept apart rather than pre-joined so a caller can lay the two out as it
+    # sees fit, and group by either one. Joining them here would settle that for every caller.
+    #
+    # @note Warnings recorded before this became a pair are plain strings that already read
+    #   `"file: message"`. They are returned whole as the message, which renders as written.
     def feature_update_warnings
       return [] unless has_attribute?(:spatial_processing_status_cache)
-      Array(spatial_processing_status_cache[WARNINGS_CACHE_KEY])
+
+      Array(spatial_processing_status_cache[WARNINGS_CACHE_KEY]).map do |warning|
+        case warning
+        when Hash then { 'file' => warning['file'].presence, 'message' => warning['message'].to_s }
+        else { 'file' => nil, 'message' => warning.to_s }
+        end
+      end
     end
 
     def store_feature_update_warnings(warnings)

diff --git a/lib/spatial_features/importers/file.rb b/lib/spatial_features/importers/file.rb
@@ -3,8 +3,8 @@
 module SpatialFeatures
   module Importers
     class File < SimpleDelegator
-      INVALID_ARCHIVE = "This file doesn't contain any map data.".freeze
-      SUPPORTED_FORMATS = "Please upload a KMZ, KML, zipped ArcGIS shapefile, ESRI JSON, or GeoJSON file.".freeze
+      INVALID_ARCHIVE = "This file contains no map data.".freeze
+      SUPPORTED_FORMATS = "Supported formats are KMZ, KML, zipped ArcGIS shapefile, ESRI JSON, and GeoJSON.".freeze
 
       FILE_PATTERNS = [/\.kml$/, /\.shp$/, /\.json$/, /\.geojson$/]
       def self.create_all(data, **options)
@@ -21,9 +21,13 @@ def self.create_all(data, **options)
       def self.invalid_archive_message(path_not_found)
         found = path_not_found.extensions
         count = path_not_found.paths.count {|path| !path.end_with?('/') }
-        contents = " It contains #{count} #{found.to_sentence} #{'file'.pluralize(count)}." if found.any?
+        problem = if found.any?
+                    "This file contains no map data, only #{count} #{found.to_sentence} #{'file'.pluralize(count)}."
+                  else
+                    INVALID_ARCHIVE
+                  end
 
-        [INVALID_ARCHIVE, contents, " ", SUPPORTED_FORMATS].compact.join
+        [problem, SUPPORTED_FORMATS].join(' ')
       end
 
       # The File importer may be initialized multiple times by `::create_all` if it
@@ -61,7 +65,7 @@ def initialize(data, current_file: nil, **options)
       private
 
       def import_error!
-        raise ImportError, "#{::File.basename(filename)} isn't a file type we can read. " + SUPPORTED_FORMATS
+        raise ImportError, "This file type is not supported. " + SUPPORTED_FORMATS
       end
 
       def filename

diff --git a/lib/spatial_features/importers/kml.rb b/lib/spatial_features/importers/kml.rb
@@ -45,7 +45,13 @@ def kml_document
         @kml_document ||= begin
           doc = Nokogiri::XML(@data)
           doc.remove_namespaces! # We don't care about namespaces since the document is going to be filled with placemark geometry and we want it all without needing to deal with namespaces
-          raise ImportError, "Invalid KML document (root node was '#{doc.root&.name}')" unless doc.root&.name.to_s.casecmp?('kml')
+          # Named the root element rather than only calling the document invalid: a KML saved as
+          # a fragment (root `<Folder>`) looks fine in a text editor, so "invalid" alone left the
+          # submitter with nothing to act on.
+          unless doc.root&.name.to_s.casecmp?('kml')
+            raise ImportError, "This KML file could not be read: its root element is "\
+                               "'#{doc.root&.name}', not 'kml'."
+          end
           discard_network_links(doc)
           discard_overlays(doc)
           doc
@@ -66,9 +72,9 @@ def discard_overlays(doc)
         return if overlays.empty?
 
         names = overlays.map {|overlay| overlay.at_css('name')&.text.presence }.compact.uniq
-        described = names.any? ? ": #{names.to_sentence}" : ''
+        described = names.any? ? " (#{names.to_sentence})" : ''
         @warnings << "Skipped #{overlays.size} map #{'image'.pluralize(overlays.size)}#{described}. " \
-                     "A map image is a picture laid over the map, not a marked area, so there is no boundary to import from it."
+                     "A map image is a picture laid over the map rather than a marked area, so it has no boundary to import."
 
         overlays.remove
       end
@@ -84,9 +90,9 @@ def discard_network_links(doc)
         return if network_links.empty?
 
         names = network_links.map {|link| link.at_css('name')&.text.presence }.compact.uniq
-        described = names.any? ? ": #{names.to_sentence}" : ''
+        described = names.any? ? " (#{names.to_sentence})" : ''
         @warnings << "Skipped #{network_links.size} network-linked #{'layer'.pluralize(network_links.size)}#{described}. " \
-                     "Network links point at data stored somewhere else rather than holding it, so there is nothing to import from them."
+                     "A network link points at data held on another server rather than containing it, so there is nothing to import."
 
         network_links.remove
       end

diff --git a/lib/spatial_features/importers/shapefile.rb b/lib/spatial_features/importers/shapefile.rb
@@ -40,8 +40,8 @@ def each_record
         case e.message
         when /No such file or directory @ rb_sysopen - (.+)/
           raise IncompleteShapefileArchive,
-                "This shapefile is incomplete — #{::File.basename($1)} is missing. " \
-                "A shapefile is a set of files that have to be zipped up together: .shp, .shx, .dbf and .prj."
+                "This shapefile is missing #{::File.basename($1)}. " \
+                "A shapefile is made up of .shp, .shx, .dbf and .prj files."
         else
           raise e
         end
@@ -109,7 +109,8 @@ def possible_shp_files
         @possible_shp_files ||= begin
           Download.open_each(archive, unzip: /\.shp$/, downcase: true)
         rescue Unzip::PathNotFound
-          raise ::SpatialFeatures::Importers::IncompleteShapefileArchive, "This archive has no shapefile (.shp) in it."
+          raise ::SpatialFeatures::Importers::IncompleteShapefileArchive,
+                "This archive has no shapefile (.shp) in it. #{::SpatialFeatures::Importers::File::SUPPORTED_FORMATS}"
         end
       end
 

diff --git a/lib/spatial_features/importers/unreadable_file.rb b/lib/spatial_features/importers/unreadable_file.rb
@@ -14,8 +14,8 @@ class UnreadableFile < Base
       # Fallbacks for failures that didn't come from an importer, whose own messages would
       # mean nothing to the person who uploaded the file — and in the missing-file case
       # would put a server filesystem path in front of them.
-      UNREADABLE = "This file couldn't be opened. It may be damaged, or saved in a format we can't read.".freeze
-      MISSING = "This file is no longer available on the server. Please upload it again.".freeze
+      UNREADABLE = "This file could not be opened. It may be damaged or in an unsupported format.".freeze
+      MISSING = "This file is no longer available on the server.".freeze
 
       def initialize(data, error, **options)
         super(data, **options)

diff --git a/lib/spatial_features/validation.rb b/lib/spatial_features/validation.rb
@@ -28,12 +28,12 @@ def validate_shapefile!(shp_file, default_proj4_projection: nil)
           case ext
             when "prj"
               raise ::SpatialFeatures::Importers::IndeterminateShapefileProjection,
-                    "This shapefile has no projection file — #{File.basename(component_path)} is missing, " \
-                    "so there is no way to tell where on the earth it belongs. Re-export it with the projection included."
+                    "This shapefile has no projection file (#{File.basename(component_path)}), so its place on the " \
+                    "earth is unknown."
             else
               raise ::SpatialFeatures::Importers::IncompleteShapefileArchive,
-                    "This shapefile is incomplete — #{File.basename(component_path)} is missing. " \
-                    "A shapefile is a set of files that have to be zipped up together: .shp, .shx, .dbf and .prj."
+                    "This shapefile is missing #{File.basename(component_path)}. " \
+                    "A shapefile is made up of .shp, .shx, .dbf and .prj files."
             end
         end
 
@@ -48,7 +48,8 @@ def validate_shapefile_archive!(path, default_proj4_projection: nil, allow_gener
           validate_shapefile!(shp_file, default_proj4_projection: default_proj4_projection)
         end
       rescue Unzip::PathNotFound
-        raise ::SpatialFeatures::Importers::IncompleteShapefileArchive, "This archive has no shapefile (.shp) in it." \
+        raise ::SpatialFeatures::Importers::IncompleteShapefileArchive,
+                "This archive has no shapefile (.shp) in it. #{::SpatialFeatures::Importers::File::SUPPORTED_FORMATS}" \
           unless allow_generic_zip_files
       end
     end

diff --git a/lib/spatial_features/version.rb b/lib/spatial_features/version.rb
@@ -1,3 +1,3 @@
 module SpatialFeatures
-  VERSION = "3.11.1"
+  VERSION = "3.12.0"
 end
diff --git a/spec/lib/spatial_features/has_spatial_features/feature_import_spec.rb b/spec/lib/spatial_features/has_spatial_features/feature_import_spec.rb
@@ -375,7 +375,7 @@ def test_kml
 
       it 'records the skipped NetworkLinks as a warning' do
         subject.update_features!
-        expect(subject.feature_update_warnings).to include(a_string_matching(/network-linked/i))
+        expect(subject.feature_update_warnings).to include(a_hash_including('message' => a_string_matching(/network-linked/i)))
       end
     end
 
@@ -410,8 +410,8 @@ def test_files
       it 'attributes each warning to the file it came from' do
         subject.update_features!
         expect(subject.feature_update_warnings).to include(
-          a_string_matching(%r{\Akml_file_with_network_link_and_features\.kml: Skipped}),
-          a_string_matching(%r{\Akml_file_with_network_link\.kml: Skipped}),
+          { 'file' => 'kml_file_with_network_link_and_features.kml', 'message' => a_string_matching(/\ASkipped/) },
+          { 'file' => 'kml_file_with_network_link.kml', 'message' => a_string_matching(/\ASkipped/) },
         )
       end
     end
@@ -434,8 +434,9 @@ def test_files
 
       it 'records why the unreadable file was skipped, against that file' do
         subject.update_features!
-        expect(subject.feature_update_warnings)
-          .to include(a_string_matching(%r{\Aarchive_without_any_known_file\.zip: .*doesn't contain any map data}))
+        expect(subject.feature_update_warnings).to include(
+          { 'file' => 'archive_without_any_known_file.zip', 'message' => a_string_matching(/contains no map data/) }
+        )
       end
     end
 
@@ -457,7 +458,7 @@ def test_files
 
       it 'records the parse failure as a warning rather than discarding the whole import' do
         subject.update_features!
-        expect(subject.feature_update_warnings).to include(a_string_matching(/shapefile is incomplete/i))
+        expect(subject.feature_update_warnings).to include(a_hash_including('message' => a_string_matching(/shapefile is missing/i)))
       end
     end
 
@@ -481,8 +482,8 @@ def test_files
 
       it 'says the file is unavailable without disclosing where it was looked for' do
         subject.update_features!
-        expect(subject.feature_update_warnings).to include(a_string_matching(/no longer available on the server/))
-        expect(subject.feature_update_warnings.join).not_to include("/nonexistent/path")
+        expect(subject.feature_update_warnings).to include(a_hash_including('message' => a_string_matching(/no longer available on the server/)))
+        expect(subject.feature_update_warnings.to_s).not_to include("/nonexistent/path")
       end
     end
 
@@ -499,7 +500,16 @@ def test_files
 
       it 'raises an EmptyImportError carrying the reason' do
         expect { subject.update_features! }
-          .to raise_error(SpatialFeatures::EmptyImportError, /doesn't contain any map data/)
+          .to raise_error(SpatialFeatures::EmptyImportError, /contains no map data/)
+      end
+
+      # The transaction recording them rolls back with the error, so keeping the warnings takes
+      # a second write. Without it a failed import can only be explained by the exception text,
+      # which is one unbroken paragraph and disappears with the job.
+      it 'keeps the warnings against the record' do
+        expect { subject.update_features! rescue nil }
+          .to change { subject.reload.feature_update_warnings }
+          .to include(a_hash_including('file' => 'archive_without_any_known_file.zip'))
       end
     end
 

diff --git a/spec/lib/spatial_features/has_spatial_features/queued_spatial_processing_spec.rb b/spec/lib/spatial_features/has_spatial_features/queued_spatial_processing_spec.rb
@@ -74,6 +74,29 @@ def status!(record, state)
     end
   end
 
+  describe '#feature_update_warnings' do
+    let(:klass) { new_dummy_class(:spatial_processing_status_cache => :jsonb) }
+
+    it 'returns the file and the message apart' do
+      record.store_feature_update_warnings([{ 'file' => 'upload.zip', 'message' => 'Skipped 1 map image.' }])
+
+      expect(record.feature_update_warnings)
+        .to eq([{ 'file' => 'upload.zip', 'message' => 'Skipped 1 map image.' }])
+    end
+
+    # Records imported before the pair was stored hold a single pre-joined string. They keep
+    # rendering as written rather than being guessed apart on a colon a filename may contain.
+    it 'reads a warning stored as a plain string as the message' do
+      SpatialFeatures::QueuedSpatialProcessing.update_cached_status(record, 'update_features!', 'failure')
+      cache = record.spatial_processing_status_cache
+      cache[SpatialFeatures::QueuedSpatialProcessing::WARNINGS_CACHE_KEY] = ['upload.zip: Skipped 1 map image.']
+      record.update_column(:spatial_processing_status_cache, cache)
+
+      expect(record.reload.feature_update_warnings)
+        .to eq([{ 'file' => nil, 'message' => 'upload.zip: Skipped 1 map image.' }])
+    end
+  end
+
   describe '#clear_feature_update_error_status' do
     let(:klass) { new_dummy_class(:spatial_processing_status_cache => :jsonb) }
 

diff --git a/spec/lib/spatial_features/importers/file_spec.rb b/spec/lib/spatial_features/importers/file_spec.rb
@@ -75,7 +75,7 @@
 
       it 'names the file types the archive did contain, so the uploader can see what they attached' do
         expect { subject.new(archive_without_any_known_file) }
-          .to raise_exception(SpatialFeatures::ImportError, /contains 1 WHATEVER file/)
+          .to raise_exception(SpatialFeatures::ImportError, /only 1 WHATEVER file/)
       end
     end
   end