Merge pull request #1079 from Floppy/change-path-parsing-tokens

Richer path templating and parsing

Author: ksuquix

.rubocop_todo.yml

@@ -1,20 +1,23 @@
+RSpec/PendingWithoutReason:
+  Enabled: false
+
 # Offense count: 6
 RSpec/BeforeAfterAll:
   Exclude:
-    - 'spec/jobs/library_scan_job_spec.rb'
-    - 'spec/jobs/model_file_scan_job_spec.rb'
-    - 'spec/jobs/model_scan_job_spec.rb'
-    - 'spec/requests/creators_request_spec.rb'
-    - 'spec/requests/libraries_request_spec.rb'
-    - 'spec/requests/models_request_spec.rb'
+    - "spec/jobs/library_scan_job_spec.rb"
+    - "spec/jobs/model_file_scan_job_spec.rb"
+    - "spec/jobs/model_scan_job_spec.rb"
+    - "spec/requests/creators_request_spec.rb"
+    - "spec/requests/libraries_request_spec.rb"
+    - "spec/requests/models_request_spec.rb"
 
 # Offense count: 4
 # Configuration parameters: Prefixes, AllowedPatterns.
 # Prefixes: when, with, without
 RSpec/ContextWording:
   Exclude:
-    - 'spec/jobs/model_scan_job_spec.rb'
-    - 'spec/models/model_spec.rb'
+    - "spec/jobs/model_scan_job_spec.rb"
+    - "spec/models/model_spec.rb"
 
 # Offense count: 6
 # Configuration parameters: CountAsOne.
@@ -25,9 +28,9 @@ RSpec/ExampleLength:
 # Configuration parameters: AssignmentOnly.
 RSpec/InstanceVariable:
   Exclude:
-    - 'spec/models/model_spec.rb'
-    - 'spec/requests/libraries_request_spec.rb'
-    - 'spec/requests/models_request_spec.rb'
+    - "spec/models/model_spec.rb"
+    - "spec/requests/libraries_request_spec.rb"
+    - "spec/requests/models_request_spec.rb"
 
 # Offense count: 14
 RSpec/MultipleExpectations:
@@ -36,55 +39,55 @@ RSpec/MultipleExpectations:
 # Offense count: 4
 RSpec/RepeatedExample:
   Exclude:
-    - 'spec/models/model_spec.rb'
+    - "spec/models/model_spec.rb"
 
 # Offense count: 1
 # Configuration parameters: Include.
 # Include: db/migrate/*.rb
 Rails/CreateTableWithTimestamps:
   Exclude:
-    - 'db/migrate/20220612220116_create_active_storage_variant_records.active_storage.rb'
+    - "db/migrate/20220612220116_create_active_storage_variant_records.active_storage.rb"
 
 # Offense count: 2
 Rails/I18nLocaleTexts:
   Exclude:
-    - 'app/admin/delayed_job.rb'
+    - "app/admin/delayed_job.rb"
 
 # Offense count: 1
 # Configuration parameters: Include.
 # Include: db/migrate/*.rb
 Rails/NotNullColumn:
   Exclude:
-    - 'db/migrate/20220614211256_add_username_to_users.rb'
+    - "db/migrate/20220614211256_add_username_to_users.rb"
 
 # Offense count: 4
 Rails/OutputSafety:
   Exclude:
-    - 'app/admin/delayed_job.rb'
-    - 'app/helpers/application_helper.rb'
-    - 'config/initializers/field_with_errors.rb'
+    - "app/admin/delayed_job.rb"
+    - "app/helpers/application_helper.rb"
+    - "config/initializers/field_with_errors.rb"
 
 # Offense count: 1
 # Configuration parameters: Include.
 # Include: db/**/*.rb
 Rails/ReversibleMigration:
   Exclude:
-    - 'db/migrate/20220106220519_remove_images_table.rb'
+    - "db/migrate/20220106220519_remove_images_table.rb"
 
 # Offense count: 2
 # Configuration parameters: ForbiddenMethods, AllowedMethods.
 # ForbiddenMethods: decrement!, decrement_counter, increment!, increment_counter, insert, insert!, insert_all, insert_all!, toggle!, touch, touch_all, update_all, update_attribute, update_column, update_columns, update_counters, upsert, upsert_all
 Rails/SkipsModelValidations:
   Exclude:
-    - 'app/admin/delayed_job.rb'
-    - 'db/migrate/20220612220115_add_service_name_to_active_storage_blobs.active_storage.rb'
+    - "app/admin/delayed_job.rb"
+    - "db/migrate/20220612220115_add_service_name_to_active_storage_blobs.active_storage.rb"
 
 # Offense count: 4
 # Configuration parameters: Include.
 # Include: app/models/**/*.rb
 Rails/UniqueValidationWithoutIndex:
   Exclude:
-    - 'app/models/library.rb'
-    - 'app/models/model.rb'
-    - 'app/models/model_file.rb'
-    - 'app/models/problem.rb'
+    - "app/models/library.rb"
+    - "app/models/model.rb"
+    - "app/models/model_file.rb"
+    - "app/models/problem.rb"

app/controllers/settings_controller.rb

@@ -6,40 +6,62 @@ def show
   end
 
   def update
-    if params[:pagination]
-      @user.pagination_settings["models"] = params[:pagination][:models] == "1"
-      @user.pagination_settings["creators"] = params[:pagination][:creators] == "1"
-      @user.pagination_settings["per_page"] = params[:pagination][:per_page].to_i
-    end
-    if current_user.admin? && params[:model_tags]
-      SiteSettings.model_tags_cloud_threshhold = params[:model_tags][:cloud_threshhold]
-      SiteSettings.model_tags_cloud_heatmap = params[:model_tags][:cloud_heatmap] == "1"
-      SiteSettings.model_tags_cloud_keypair = params[:model_tags][:cloud_keypair] == "1"
-      SiteSettings.model_tags_cloud_sorting = params[:model_tags][:cloud_sorting]
-      SiteSettings.model_tags_filter_stop_words = params[:model_tags][:filter_stop_words] == "1"
-      SiteSettings.model_tags_tag_model_directory_name = params[:model_tags][:tag_model_directory_name] == "1"
-      SiteSettings.model_tags_stop_words_locale = params[:model_tags][:stop_words_locale]
-      SiteSettings.model_tags_custom_stop_words = params[:model_tags][:custom_stop_words].split
-      SiteSettings.model_tags_auto_tag_new = params[:model_tags][:auto_tag_new]
-      SiteSettings.model_path_prefix_template = params[:model_tags][:model_path_prefix_template]
-      SiteSettings.model_tags_tag_model_path_prefix = params[:model_tags][:model_tags_tag_model_path_prefix]
-      SiteSettings.model_path_suffix_model_id = params[:model_tags][:model_path_suffix_model_id]
-    end
-    if params[:renderer]
-      @user.renderer_settings["grid_width"] = params[:renderer][:grid_width].to_i
-      @user.renderer_settings["grid_depth"] = params[:renderer][:grid_width].to_i # Store width in both for now. See #834
-      @user.renderer_settings["show_grid"] = params[:renderer][:show_grid] == "1"
-      @user.renderer_settings["enable_pan_zoom"] = params[:renderer][:enable_pan_zoom] == "1"
-      @user.renderer_settings["background_colour"] = params[:renderer][:background_colour]
-      @user.renderer_settings["object_colour"] = params[:renderer][:object_colour]
-      @user.renderer_settings["render_style"] = params[:renderer][:render_style]
-    end
+    # Save personal settings
+    update_pagination_settings(params[:pagination])
+    update_renderer_settings(params[:renderer])
     @user.save!
+    # Save site-wide settings if user is an admin
+    if current_user.admin?
+      update_folder_settings(params[:folders])
+      update_tagging_settings(params[:model_tags])
+    end
     redirect_to user_settings_path(@user)
   end
 
   private
 
+  def update_pagination_settings(settings)
+    return unless settings
+    @user.pagination_settings = {
+      "models" => settings[:models] == "1",
+      "creators" => settings[:creators] == "1",
+      "per_page" => settings[:per_page].to_i
+    }
+  end
+
+  def update_renderer_settings(settings)
+    return unless settings
+    @user.renderer_settings = {
+      "grid_width" => settings[:grid_width].to_i,
+      "grid_depth" => settings[:grid_width].to_i, # Store width in both for now. See #834
+      "show_grid" => settings[:show_grid] == "1",
+      "enable_pan_zoom" => settings[:enable_pan_zoom] == "1",
+      "background_colour" => settings[:background_colour],
+      "object_colour" => settings[:object_colour],
+      "render_style" => settings[:render_style]
+    }
+  end
+
+  def update_folder_settings(settings)
+    return unless settings
+    SiteSettings.model_path_template = settings[:model_path_template].gsub(/^\//, "") # Remove leading slashes
+    SiteSettings.parse_metadata_from_path = settings[:parse_metadata_from_path]
+    SiteSettings.safe_folder_names = settings[:safe_folder_names]
+  end
+
+  def update_tagging_settings(settings)
+    return unless settings
+    SiteSettings.model_tags_cloud_threshhold = settings[:cloud_threshhold]
+    SiteSettings.model_tags_cloud_heatmap = settings[:cloud_heatmap] == "1"
+    SiteSettings.model_tags_cloud_keypair = settings[:cloud_keypair] == "1"
+    SiteSettings.model_tags_cloud_sorting = settings[:cloud_sorting]
+    SiteSettings.model_tags_filter_stop_words = settings[:filter_stop_words] == "1"
+    SiteSettings.model_tags_tag_model_directory_name = settings[:tag_model_directory_name] == "1"
+    SiteSettings.model_tags_stop_words_locale = settings[:stop_words_locale]
+    SiteSettings.model_tags_custom_stop_words = settings[:custom_stop_words].split
+    SiteSettings.model_tags_auto_tag_new = settings[:auto_tag_new]
+  end
+
   def get_user
     @user = User.find_by(username: params[:user_id])
   end

app/jobs/model_scan_job.rb

@@ -41,14 +41,14 @@ def perform(model)
     model.model_files.reload
     model.preview_file = model.model_files.min_by { |x| x.is_image? ? 0 : 1 } unless model.preview_file
     if model.tags.empty?
-      model.autogenerate_tags_from_path!
+      model.generate_tags_from_directory_name! if SiteSettings.model_tags_tag_model_directory_name
       if SiteSettings.model_tags_auto_tag_new.present?
         model.tag_list << SiteSettings.model_tags_auto_tag_new
         model.save!
       end
     end
-    if !model.creator_id
-      model.autogenerate_creator_from_prefix_template!
+    if !model.creator_id && SiteSettings.parse_metadata_from_path
+      model.parse_metadata_from_path!
     end
     # If this model has no files, flag a problem
     if model.model_files.reload.count == 0

app/models/concerns/path_builder.rb

@@ -2,19 +2,31 @@ module PathBuilder
   extend ActiveSupport::Concern
 
   def formatted_path
-    formatted_path_out = []
-    SiteSettings.model_path_prefix_template.split("/").each { |p|
-      case p
+    SiteSettings.model_path_template.gsub(/{.+?}/) do |token|
+      case token
       when "{tags}"
-        formatted_path_out.push(tags.order(taggings_count: :desc).map(&:to_s).map(&:parameterize))
+        (tags.count > 0) ?
+          File.join(tags.order(taggings_count: :desc).map(&:to_s).map(&:parameterize)) :
+          "@untagged"
       when "{creator}"
-        formatted_path_out.push(creator ? creator.name : "unset-creator")
+        safe(creator&.name) || "@unattributed"
       when "{collection}"
-        formatted_path_out.push((collections.count > 0) ? collections.map { |c| c.name } : "unset-collection")
+        (collections.count > 0) ?
+          collections.map(&:name).map { |s| safe(s) }.join(",") :
+          "@uncollected"
+      when "{modelName}"
+        safe(name)
+      when "{modelId}"
+        "##{id}"
       else
-        formatted_path_out.push("bad-formatted-path-element")
+        token
       end
-    }
-    File.join("", formatted_path_out, name.parameterize) + (SiteSettings.model_path_suffix_model_id ? "##{id}" : "")
+    end
+  end
+
+  private
+
+  def safe(str)
+    SiteSettings.safe_folder_names ? str&.parameterize : str
   end
 end

app/models/concerns/path_parser.rb

@@ -1,63 +1,50 @@
 module PathParser
   extend ActiveSupport::Concern
 
-  def autogenerate_tags_from_path!
-    tags = []
-
-    # Auto-tag based on model directory name:
-    if SiteSettings.model_tags_tag_model_directory_name
-      tags = File.split(path).last.split(/[\W_+-]/).filter { |x| x.length > 1 }
-    end
+  def generate_tags_from_directory_name!
+    tags = File.split(path).last.split(/[\W_+-]/).filter { |x| x.length > 1 }
+    tag_list.add(remove_stop_words(tags))
+  end
 
-    unless tags.empty?
-      tag_list.add(remove_stop_words(tags))
-      save!
+  def parse_metadata_from_path!
+    return unless SiteSettings.model_path_template
+    components = extract_path_components
+    if components[:tags].present?
+      tag_list.add(remove_stop_words(components[:tags]))
     end
+    self.creator = Creator.find_or_create_by(name: components[:creator]) if components[:creator]
+    collection_list.add(components[:collection]) if components[:collection]
+    save!
   end
+end
 
-  def autogenerate_creator_from_prefix_template!
-    if SiteSettings.model_tags_tag_model_path_prefix
-      filepaths = File.dirname(path).split("/")
-      filepaths.shift
-      templatechunks = SiteSettings.model_path_prefix_template.split("/")
-      creatornew = ""
-      collectionnew = ""
-      tags = []
-      while !templatechunks.empty? && !filepaths.empty? && !(templatechunks.length == 1 && templatechunks[0] == "{tags}")
-        if templatechunks[0] == "{creator}"
-          creatornew = filepaths.shift
-          templatechunks.shift
-        elsif templatechunks[0] == "{collection}"
-          collectionnew = filepaths.shift
-          templatechunks.shift
-        elsif templatechunks[-1] == "{creator}"
-          creatornew = filepaths.pop
-          templatechunks.pop
-        elsif templatechunks[-1] == "{collection}"
-          collectionnew = filepaths.pop
-          templatechunks.pop
-        else
-          logger.error "Invalid model_path_prefix_template: #{SiteSettings.model_path_prefix_template} / #{templatechunks[0]}"
-          templatechunks.shift
-        end
-      end
-      if templatechunks.length == 1 && templatechunks[0] == "{tags}"
-        tags = filepaths
-      end
-      unless tags.empty?
-        tag_list.add(remove_stop_words(tags))
+def path_parse_pattern
+  Regexp.new("^/?.*?" +
+    SiteSettings.model_path_template.gsub(/{.+?}/) { |token|
+      case token
+      when "{tags}"
+        "(?<tags>[[:print:]]*)"
+      when "{creator}"
+        "(?<creator>[[:print:]&&[^/]]*?)"
+      when "{collection}"
+        "(?<collection>[[:print:]&&[^/]]*?)"
+      when "{modelName}"
+        "(?<model_name>[[:print:]&&[^/]]*?)"
+      when "{modelId}"
+        "(?<model_id>#[[:digit:]]+)?"
+      else
+        "[[:print:]&&[^/]]*"
       end
-      unless creatornew.empty?
-        creator = Creator.find_by(name: creatornew)
-        creator ||= Creator.create(name: creatornew)
-        self.creator_id = creator.id
-      end
-      unless collectionnew.empty? && !collection_list
-        collection_list.add(collectionnew)
-      end
-      save!
-    end
-  end
+    } + "$")
+end
+
+def extract_path_components
+  components = path.match(path_parse_pattern)&.named_captures&.symbolize_keys
+  return {} if components.nil?
+  components.merge({
+    tags: components[:tags]&.split("/")&.compact_blank,
+    model_id: nil # discard ID, never gonna use it in parsing
+  }).compact
 end
 
 def remove_stop_words(words)

app/models/site_settings.rb

@@ -7,11 +7,11 @@ class SiteSettings < RailsSettings::Base
   field :model_tags_cloud_keypair, type: :boolean, default: true
   field :model_tags_cloud_sorting, type: :string, default: "frequency"
   field :model_tags_filter_stop_words, type: :boolean, default: true
-  field :model_tags_tag_model_directory_name, type: :boolean, default: true
+  field :model_tags_tag_model_directory_name, type: :boolean, default: false
   field :model_tags_stop_words_locale, type: :string, default: "en"
   field :model_tags_custom_stop_words, type: :array, default: (SupportedMimeTypes.image_extensions + SupportedMimeTypes.model_extensions)
   field :model_tags_auto_tag_new, type: :string, default: "!new"
-  field :model_path_prefix_template, type: :string, default: "tags"
-  field :model_tags_tag_model_path_prefix, type: :boolean, default: true
-  field :model_path_suffix_model_id, type: :boolean, default: true
+  field :model_path_template, type: :string, default: "{tags}/{modelName}{modelId}"
+  field :parse_metadata_from_path, type: :boolean, default: true
+  field :safe_folder_names, type: :boolean, default: true
 end