fix: Use Ruby/Rails internals to create human readable file names

Use Ruby's `File.basename` and Rails' `humanize` to create the first
initial human readable `name` of `Attachment`s and `Picture`s.

Before we where using our own brittle Regexp, that expected the file
suffix to be known from it's mime type. This is not the case for unknown
mimetypes.

We also move the name conversion before filename sanitization in order
to get a nice human readable name and not escape characters. Since the
name is not used to load the file and all output is escaped in Rails by
default, this is fine.

Signed-off-by: Thomas von Deyen <vondeyen@blish.cloud>

Author: tvdeyen

app/controllers/alchemy/admin/pictures_controller.rb

@@ -64,7 +64,6 @@ def url
 
       def create
         @picture = Picture.new(picture_params)
-        @picture.name = @picture.humanized_name
         if @picture.save
           render successful_uploader_response(file: @picture)
         else

app/models/alchemy/attachment.rb

@@ -25,6 +25,8 @@ class Attachment < BaseRecord
     include Alchemy::TouchElements
     include Alchemy::RelatableResource
 
+    before_save :set_name, if: -> { Alchemy.storage_adapter.set_attachment_name?(self) }
+
     include Alchemy.storage_adapter.attachment_class_methods
 
     stampable stamper_class_name: Alchemy.config.user_class_name
@@ -97,8 +99,6 @@ def ransackable_scopes(_auth_object = nil)
     validate :file_type_allowed,
       unless: -> { self.class.allowed_filetypes.include?("*") }
 
-    before_save :set_name, if: -> { Alchemy.storage_adapter.set_attachment_name?(self) }
-
     scope :with_file_type, ->(file_type) { where(file_mime_type: file_type) }
 
     # Instance methods
@@ -179,7 +179,7 @@ def file_type_allowed
     end
 
     def set_name
-      self.name ||= convert_to_humanized_name(file_name, extension)
+      self.name ||= Alchemy.storage_adapter.file_basename(self).humanize
     end
   end
 end

app/models/alchemy/picture.rb

@@ -62,6 +62,8 @@ def self.preprocessor_class=(klass)
       @_preprocessor_class = klass
     end
 
+    before_create :set_name, if: :image_file_name
+
     include Alchemy.storage_adapter.picture_class_methods
 
     # We need to define this method here to have it available in the validations below.
@@ -195,14 +197,6 @@ def urlname
       end
     end
 
-    # Returns a humanized, readable name from image filename.
-    #
-    def humanized_name
-      return "" if image_file_name.blank?
-
-      convert_to_humanized_name(image_file_name, image_file_extension)
-    end
-
     # Returns the format the image should be rendered with
     #
     # Only returns a format differing from original if an +image_output_format+
@@ -287,6 +281,12 @@ def image_file_dimensions
 
     private
 
+    # Returns a humanized, readable name from image filename.
+    #
+    def set_name
+      self.name ||= Alchemy.storage_adapter.image_file_basename(self).humanize
+    end
+
     def image_file_type_allowed
       unless image_file_extension&.in?(self.class.allowed_filetypes)
         errors.add(:image_file, Alchemy.t("not a valid image"))

app/models/alchemy/storage_adapter.rb

@@ -9,12 +9,14 @@ class UnknownAdapterError < StandardError; end
       :by_file_format_scope,
       :by_file_type_scope,
       :not_file_type_scope,
+      :file_basename,
       :file_extension,
       :file_formats,
       :file_mime_type,
       :file_name,
       :file_size,
       :has_convertible_format?,
+      :image_file_basename,
       :image_file_extension,
       :image_file_format,
       :image_file_height,

app/models/alchemy/storage_adapter/active_storage.rb

@@ -96,6 +96,13 @@ def not_file_type_scope(file_type)
         Attachment.with_attached_file.joins(:file_blob).where.not(active_storage_blobs: {content_type: file_type})
       end
 
+      # @param [Alchemy::Attachment]
+      # @return [String]
+      def file_basename(attachment)
+        filename = attachment.file&.filename.to_s
+        File.basename(filename, File.extname(filename))
+      end
+
       # @param [Alchemy::Attachment]
       # @return [String]
       def file_name(attachment)
@@ -126,6 +133,13 @@ def has_convertible_format?(picture)
         picture.image_file&.variable?
       end
 
+      # @param [Alchemy::Picture]
+      # @return [String]
+      def image_file_basename(picture)
+        filename = picture.image_file&.filename.to_s
+        File.basename(filename, File.extname(filename))
+      end
+
       # @param [Alchemy::Picture]
       # @return [String]
       def image_file_name(picture)

app/models/alchemy/storage_adapter/dragonfly.rb

@@ -127,6 +127,12 @@ def file_name(attachment)
         attachment.read_attribute(:file_name)
       end
 
+      # @param [Alchemy::Attachment]
+      # @return [String]
+      def file_basename(attachment)
+        attachment.file&.basename&.to_s
+      end
+
       # @param [Alchemy::Attachment]
       # @return [Integer]
       def file_size(attachment)
@@ -152,6 +158,12 @@ def has_convertible_format?(picture)
         image_file_extension(picture).in?(CONVERTIBLE_FILE_FORMATS)
       end
 
+      # @param [Alchemy::Picture]
+      # @return [String]
+      def image_file_basename(picture)
+        picture.image_file&.basename&.to_s
+      end
+
       # @param [Alchemy::Picture]
       # @return [String]
       def image_file_name(picture)

lib/alchemy/name_conversions.rb

@@ -17,12 +17,6 @@ def convert_to_urlname(name)
         .parameterize
     end
 
-    # Converts a filename and suffix into a human readable name.
-    #
-    def convert_to_humanized_name(name, suffix)
-      name.gsub(/\.#{::Regexp.quote(suffix)}$/i, "").tr("_", " ").strip
-    end
-
     # Sanitizes a given filename by removing directory traversal attempts and HTML entities.
     def sanitized_filename(file_name)
       file_name = File.basename(file_name)

spec/controllers/alchemy/admin/pictures_controller_spec.rb

@@ -141,15 +141,14 @@ module Alchemy
     describe "#create" do
       subject { post :create, params: params }
 
-      let(:params) { {picture: {name: ""}} }
-      let(:picture) { mock_model("Picture", humanized_name: "Cute kittens") }
-
       context "with passing validations" do
-        before do
-          expect(Picture).to receive(:new).and_return(picture)
-          expect(picture).to receive(:name=).and_return("Cute kittens")
-          expect(picture).to receive(:name).and_return("Cute kittens")
-          expect(picture).to receive(:save).and_return(true)
+        let(:params) do
+          {picture: {image_file: fixture_file_upload("my FileNämü.png")}}
+        end
+
+        it "creates a new picture with human friendly name" do
+          expect { subject }.to change { Picture.count }.by(1)
+          expect(Picture.last.name).to eq("My filenämü")
         end
 
         it "renders json response with success message" do
@@ -162,6 +161,8 @@ module Alchemy
       end
 
       context "with failing validations" do
+        let(:params) { {picture: {name: ""}} }
+
         it_behaves_like "having a json uploader error message"
       end
     end