Make trailing model ID compulsory in folder names

app/models/concerns/path_builder.rb

@@ -2,7 +2,7 @@ module PathBuilder
   extend ActiveSupport::Concern
 
   def formatted_path
-    SiteSettings.model_path_template.gsub(/{.+?}/) do |token|
+    path = SiteSettings.model_path_template.gsub(/{.+?}/) do |token|
       case token
       when "{tags}"
         (tags.count > 0) ?
@@ -15,11 +15,12 @@ def formatted_path
       when "{modelName}"
         path_component(self)
       when "{modelId}"
-        "##{id}"
+        "" # Deprecated, now always added below; kept for compatibility
       else
         token
       end
     end
+    path + "##{id}"
   end
 
   private

app/models/site_settings.rb

@@ -7,7 +7,7 @@ class SiteSettings < RailsSettings::Base
   field :model_tags_stop_words_locale, type: :string, default: "en"
   field :model_tags_custom_stop_words, type: :array, default: SupportedMimeTypes.indexable_extensions
   field :model_tags_auto_tag_new, type: :string, default: "!new"
-  field :model_path_template, type: :string, default: "{tags}/{modelName}{modelId}"
+  field :model_path_template, type: :string, default: "{tags}/{modelName}"
   field :model_ignored_files, type: :array, default: [
     /^\.[^\.]+/, # Hidden files starting with .
     /.*\/@eaDir\/.*/, # Synology temp files

app/services/path_parser_service.rb

@@ -28,10 +28,10 @@ def path_parse_pattern
         when "{modelName}"
           "(?<model_name>[[:print:]&&[^/]]*?)"
         when "{modelId}"
-          "(?<model_id>#[[:digit:]]+)?"
+          "" # Always detected by default, moved below, kept for compatibility
         else
           "[[:print:]&&[^/]]*"
         end
-      } + "$")
+      } + "(?<model_id>#[[:digit:]]+)?" + "$")
   end
 end

app/views/settings/_folder_settings.html.erb

@@ -18,14 +18,11 @@
   <li>
     <code>{modelName}</code>: <%= t ".tokens.model_name" %>
   </li>
-  <li>
-    <code>{modelId}</code>: <%= t ".tokens.model_id" %>
-  </li>
 </ul>
 <div class="row mb-2">
   <%= form.label nil, t(".model_path_template.label"), for: "folders[model_path_template]", class: "col-sm-4 col-form-label" %>
   <div class="col-sm-8 form-check form-switch">
-    <%= form.text_field "folders[model_path_template]", value: SiteSettings.model_path_template, class: "form-control" %>
+    <%= form.text_field "folders[model_path_template]", value: SiteSettings.model_path_template.gsub("{modelId}", ""), class: "form-control" %>
   </div>
 </div>
 <div class="row mb-2">

config/locales/settings/cs.yml

@@ -66,7 +66,6 @@ cs:
       tokens:
         collection: Název kolekce, ve které se model nachází, je-li nastavena.
         creator: Jméno autora, je-li nastaveno.
-        model_id: Jedinečný číselný identifikátor modelu. Důrazně doporučujeme, aby byl vždy uveden na konci šablony, aby se předešlo konfliktům názvů na disku.
         model_name: Verze názvu modelu bezpečná pro souborový systém.
         tags_html: 'řada vnořených složek, jedna pro každý štítek, uspořádaných podle oblíbenosti štítků. Například: <code>fantasy/lidé/čarodějové</code>'
     general:

config/locales/settings/de.yml

@@ -66,7 +66,6 @@ de:
       tokens:
         collection: Der Titel der Sammlung, in der sich das Modell befindet, falls festgelegt.
         creator: Der Name des Erstellers, falls festgelegt.
-        model_id: Ein eindeutiger numerischer Bezeichner für das Modell. Wir empfehlen dringend, diese Kennung immer am Ende der Vorlage anzugeben, um Namenskonflikte zu vermeiden.
         model_name: Eine dateisystemsichere Version des Modellnamens.
         tags_html: 'eine Reihe von verschachtelten Ordnern, einen für jedes Tag, geordnet nach der Beliebtheit der Tags. Zum Beispiel: <code>fantasy/human/wizard</code>'
     general:

config/locales/settings/en.yml

@@ -52,7 +52,7 @@ en:
       summary: Change file settings such as ignored files.
       title: File settings
     folder_settings:
-      details: Folder structure follows a template that you define using tokens. You can also include other text in the template (such as folder separators) and it will be included as-is.
+      details: Folder structure follows a template that you define using tokens. You can also include other text in the template (such as folder separators) and it will be included as-is. A model ID in the form `#123` is always appended to the folder name, to avoid folder naming clashes.
       model_path_template:
         label: Model path template
       parse_metadata_from_path:
@@ -66,7 +66,6 @@ en:
       tokens:
         collection: The title of the collection the model is in, if set.
         creator: The name of the creator, if set.
-        model_id: A unique numerical identifier for the model. We strongly recommend always including this at the end of your template to avoid name conflicts on disk.
         model_name: A filesystem-safe version of the model name.
         tags_html: 'a series of nested folders, one for each tag, arranged in order of tag popularity. For example: <code>fantasy/human/wizard</code>. This token is "greedy"; it will parse as many tags as it can.'
     general:

config/locales/settings/es.yml

@@ -66,7 +66,6 @@ es:
       tokens:
         collection: El título de la colección en la que se encuentra el modelo, si está establecido.
         creator: El nombre del creador, si se ha establecido.
-        model_id: Un identificador numérico único para el modelo. Recomendamos encarecidamente incluirlo siempre al final del modelo para evitar conflictos de nombres en el disco.
         model_name: Una versión segura para el sistema de ficheros del nombre del modelo.
         tags_html: 'una serie de carpetas anidadas, una para cada etiqueta, ordenadas según su popularidad. Por ejemplo: <code>fantasía/humano/mago</code>'
     general:

config/locales/settings/fr.yml

@@ -66,7 +66,6 @@ fr:
       tokens:
         collection: Le titre de la collection dans laquelle se trouve le modèle, s'il est défini.
         creator: Le nom du créateur, s'il est défini.
-        model_id: Un identifiant numérique unique pour le modèle. Nous vous recommandons fortement de toujours l'inclure à la fin de votre modèle afin d'éviter les conflits de noms sur le disque.
         model_name: Une version sécurisée du nom du modèle pour le système de fichiers.
         tags_html: 'une série de dossiers imbriqués, un pour chaque étiquette, classés par ordre de popularité de l''étiquette. Par exemple : <code>fantasy/humain/sorcier</code>'
     general:

config/locales/settings/nl.yml

@@ -66,7 +66,6 @@ nl:
       tokens:
         collection: De titel van de collectie waarin het model zich bevindt, indien ingesteld.
         creator: De naam van de maker, indien ingesteld.
-        model_id: Een unieke numerieke identificatie voor het model. We raden sterk aan om dit altijd aan het einde van je sjabloon op te nemen om naamconflicten op schijf te voorkomen.
         model_name: Een bestandssysteem-veilige versie van de modelnaam.
         tags_html: 'een reeks geneste mappen, één voor elke tag, gerangschikt op volgorde van tagpopulariteit. Bijvoorbeeld: <code>fantasy/human/wizard</code>'
     general:

config/locales/settings/pl.yml

@@ -66,7 +66,6 @@ pl:
       tokens:
         collection: Tytuł kolekcji, w której znajduje się model, jeśli jest ustawiony.
         creator: Nazwa twórcy, jeśli ustawiona.
-        model_id: Unikalny numeryczny identyfikator modelu. Zdecydowanie polecamy podawanie go zawsze na końcu szablonu, aby uniknąć konfliktów nazw na dysku.
         model_name: Bezpieczna dla systemu plików wersja nazwy modelu.
         tags_html: 'seria zagnieżdżonych folderów, po jednym dla każdego z tagów, ułożonych w kolejności popularności tagów. Na przykład: <code>fantasy/człowiek/czarodziej</code>'
     general:

spec/jobs/scan/model/parse_metadata_job_spec.rb

@@ -105,7 +105,7 @@
         model_tags_tag_model_directory_name: true,
         parse_metadata_from_path: true,
         model_tags_auto_tag_new: "!new",
-        model_path_template: "{creator}/{tags}/{modelName}{modelId}"
+        model_path_template: "{creator}/{tags}/{modelName}"
       )
       described_class.perform_now(model.id)
       model.reload
@@ -145,35 +145,35 @@
     end
 
     it "preserves existing tags" do
-      allow(SiteSettings).to receive(:model_path_template).and_return("{tags}/{modelName}{modelId}")
+      allow(SiteSettings).to receive(:model_path_template).and_return("{tags}/{modelName}")
       described_class.perform_now(model.id)
       model.reload
       expect(model.tag_list).to include "!new"
     end
 
     it "parses tags" do
-      allow(SiteSettings).to receive(:model_path_template).and_return("{tags}/{modelName}{modelId}")
+      allow(SiteSettings).to receive(:model_path_template).and_return("{tags}/{modelName}")
       described_class.perform_now(model.id)
       model.reload
       expect(model.tag_list).to include("library 1", "stuff", "tags", "are", "greedy")
     end
 
     it "parses creator" do
-      allow(SiteSettings).to receive(:model_path_template).and_return("{creator}/{modelName}{modelId}")
+      allow(SiteSettings).to receive(:model_path_template).and_return("{creator}/{modelName}")
       described_class.perform_now(model.id)
       model.reload
       expect(model.creator.name).to eq "Greedy"
     end
 
     it "parses collection" do
-      allow(SiteSettings).to receive(:model_path_template).and_return("{collection}/{modelName}{modelId}")
+      allow(SiteSettings).to receive(:model_path_template).and_return("{collection}/{modelName}")
       described_class.perform_now(model.id)
       model.reload
       expect(model.collection.name).to eq "Greedy"
     end
 
     it "parses everything at once" do # rubocop:todo RSpec/MultipleExpectations, RSpec/ExampleLength
-      allow(SiteSettings).to receive(:model_path_template).and_return("{creator}/{collection}/{tags}/{modelName}{modelId}")
+      allow(SiteSettings).to receive(:model_path_template).and_return("{creator}/{collection}/{tags}/{modelName}")
       described_class.perform_now(model.id)
       model.reload
       expect(model.creator.name).to eq "Library 1"
@@ -182,7 +182,7 @@
     end
 
     it "ignores extra path components" do # rubocop:todo RSpec/MultipleExpectations, RSpec/ExampleLength
-      allow(SiteSettings).to receive(:model_path_template).and_return("{creator}/{modelName}{modelId}")
+      allow(SiteSettings).to receive(:model_path_template).and_return("{creator}/{modelName}")
       described_class.perform_now(model.id)
       model.reload
       expect(model.creator.name).to eq "Greedy"
@@ -204,7 +204,7 @@
         model_tags_stop_words_locale: "en",
         model_tags_filter_stop_words: true,
         model_tags_custom_stop_words: ["stuff"],
-        model_path_template: "{tags}/{modelName}{modelId}"
+        model_path_template: "{tags}/{modelName}"
       )
       described_class.perform_now(model.id)
       expect(model.tag_list).not_to include "stuff"
@@ -338,7 +338,7 @@
   end
 
   it "discards model ID and doesn't include it in model name" do # rubocop:todo RSpec/MultipleExpectations
-    allow(SiteSettings).to receive(:model_path_template).and_return("{modelName}{modelId}")
+    allow(SiteSettings).to receive(:model_path_template).and_return("{modelName}")
     model = create(:model, path: "model-name#1234")
     described_class.perform_now(model.id)
     model.reload
@@ -348,7 +348,7 @@
   it "handles paths matching a complex templates" do # rubocop:todo RSpec/ExampleLength, RSpec/MultipleExpectations
     allow(SiteSettings).to receive_messages(
       parse_metadata_from_path: true,
-      model_path_template: "{tags}/{creator} - {modelName}{modelId}",
+      model_path_template: "{tags}/{creator} - {modelName}",
       model_tags_auto_tag_new: nil
     )
     model = create(:model, path: "human/wizard/bruce-wayne - model-name#1234")

spec/models/concerns/path_builder_spec.rb

@@ -11,24 +11,24 @@
     }
 
     it "includes creator if set" do
-      SiteSettings.model_path_template = "{creator}/{modelName}{modelId}"
+      SiteSettings.model_path_template = "{creator}/{modelName}"
       expect(model.formatted_path).to eq "bruce-wayne/batarang#1"
     end
 
     it "includes tags if set" do
-      SiteSettings.model_path_template = "{tags}/{modelName}{modelId}"
+      SiteSettings.model_path_template = "{tags}/{modelName}"
       expect(model.formatted_path).to eq "bat/weapon/batarang#1"
     end
 
     it "is invariant to tag ordering" do
-      SiteSettings.model_path_template = "{tags}/{modelName}{modelId}"
+      SiteSettings.model_path_template = "{tags}/{modelName}"
       model.tag_list.remove "bat" and model.save
       model.tag_list.add "bat" and model.save
       expect(model.formatted_path).to eq "bat/weapon/batarang#1"
     end
 
     it "orders tags by tagging_count" do
-      SiteSettings.model_path_template = "{tags}/{modelName}{modelId}"
+      SiteSettings.model_path_template = "{tags}/{modelName}"
       # rubocop:disable Rails/SkipsModelValidations
       # We *intentionally* don't want the callbacks to run, they'll recalculate the count again
       ActsAsTaggableOn::Tag.find_by(name: "weapon").update_column(:taggings_count, 10)
@@ -38,22 +38,22 @@
     end
 
     it "includes collection if set" do
-      SiteSettings.model_path_template = "{collection}/{modelName}{modelId}"
+      SiteSettings.model_path_template = "{collection}/{modelName}"
       expect(model.formatted_path).to eq "gadgets/batarang#1"
     end
 
     it "includes multiple metadata types if set" do
-      SiteSettings.model_path_template = "{collection}/{creator}/{tags}/{modelName}{modelId}"
+      SiteSettings.model_path_template = "{collection}/{creator}/{tags}/{modelName}"
       expect(model.formatted_path).to eq "gadgets/bruce-wayne/bat/weapon/batarang#1"
     end
 
     it "includes non-token information as literal text" do
-      SiteSettings.model_path_template = "{tags}/{creator} - {collection} - {modelName}{modelId}"
+      SiteSettings.model_path_template = "{tags}/{creator} - {collection} - {modelName}"
       expect(model.formatted_path).to eq "bat/weapon/bruce-wayne - gadgets - batarang#1"
     end
 
     it "treats unknown tokens as literal text" do
-      SiteSettings.model_path_template = "{bad}/{modelName}{modelId}"
+      SiteSettings.model_path_template = "{bad}/{modelName}"
       expect(model.formatted_path).to eq "{bad}/batarang#1"
     end
   end
@@ -62,37 +62,32 @@
     let(:model) { create(:model, name: "Batarang", tag_list: []) }
 
     it "includes creator error if set" do
-      SiteSettings.model_path_template = "{creator}/{modelName}{modelId}"
+      SiteSettings.model_path_template = "{creator}/{modelName}"
       expect(model.formatted_path).to eq "@unattributed/batarang#1"
     end
 
     it "handles zero tags" do
-      SiteSettings.model_path_template = "{tags}/{modelName}{modelId}"
+      SiteSettings.model_path_template = "{tags}/{modelName}"
       expect(model.formatted_path).to eq "@untagged/batarang#1"
     end
 
     it "includes collection error if set" do
-      SiteSettings.model_path_template = "{collection}/{modelName}{modelId}"
+      SiteSettings.model_path_template = "{collection}/{modelName}"
       expect(model.formatted_path).to eq "@uncollected/batarang#1"
     end
 
     it "includes non-token information as literal text" do
-      SiteSettings.model_path_template = "{tags}/{creator} - {collection} - {modelName}{modelId}"
+      SiteSettings.model_path_template = "{tags}/{creator} - {collection} - {modelName}"
       expect(model.formatted_path).to eq "@untagged/@unattributed - @uncollected - batarang#1"
     end
   end
 
   context "when creating model directory name" do
     let(:model) { create(:model, name: "Batarang") }
 
-    it "includes model ID if option is included" do
-      SiteSettings.model_path_template = "{modelName}{modelId}"
-      expect(model.formatted_path).to eq "batarang#1"
-    end
-
-    it "does not include model ID if option is deselected" do
+    it "always includes model ID" do
       SiteSettings.model_path_template = "{modelName}"
-      expect(model.formatted_path).to eq "batarang"
+      expect(model.formatted_path).to eq "batarang#1"
     end
   end
 
@@ -106,7 +101,7 @@
     }
 
     before do
-      SiteSettings.model_path_template = "{creator}/{collection}/{tags}/{modelName}{modelId}"
+      SiteSettings.model_path_template = "{creator}/{collection}/{tags}/{modelName}"
     end
 
     it "uses safe names in path if safe_folder_names is set" do

spec/requests/settings_spec.rb

@@ -41,7 +41,7 @@
         let(:params) {
           {
             folders: {
-              model_path_template: "test/{tags}/{modelName}{modelId}",
+              model_path_template: "test/{tags}/{modelName}",
               parse_metadata_from_path: "1",
               safe_folder_names: "0"
             }
@@ -56,7 +56,7 @@
         end
 
         it "saves path template" do
-          expect(SiteSettings.model_path_template).to eq "test/{tags}/{modelName}{modelId}"
+          expect(SiteSettings.model_path_template).to eq "test/{tags}/{modelName}"
         end
 
         it "saves parsing setting" do

spec/services/path_parser_service_spec.rb

@@ -6,13 +6,13 @@
   let(:path) { "/top/middle/bottom/prefix - name#42" }
 
   {
-    "{tags}" => %r{^/?.*?(?<tags>[[:print:]]*)$},
-    "{creator}" => %r{^/?.*?(?<creator>[[:print:]&&[^/]]*?)$},
-    "{collection}" => %r{^/?.*?(?<collection>[[:print:]&&[^/]]*?)$},
-    "{tags}/{creator}" => %r{^/?.*?(?<tags>[[:print:]]*)/(?<creator>[[:print:]&&[^/]]*?)$},
-    "{tags}/{creator}/{modelName}{modelId}" => %r{^/?.*?(?<tags>[[:print:]]*)/(?<creator>[[:print:]&&[^/]]*?)/(?<model_name>[[:print:]&&[^/]]*?)(?<model_id>#[[:digit:]]+)?$},
-    "@{creator}{modelId}" => %r{^/?.*?@(?<creator>[[:print:]&&[^/]]*?)(?<model_id>#[[:digit:]]+)?$},
-    "{creator}/{collection}/{tags}/{modelName}{modelId}" => %r{^/?.*?(?<creator>[[:print:]&&[^/]]*?)/(?<collection>[[:print:]&&[^/]]*?)/(?<tags>[[:print:]]*)/(?<model_name>[[:print:]&&[^/]]*?)(?<model_id>#[[:digit:]]+)?$}
+    "{tags}" => %r{^/?.*?(?<tags>[[:print:]]*)(?<model_id>#[[:digit:]]+)?$},
+    "{creator}" => %r{^/?.*?(?<creator>[[:print:]&&[^/]]*?)(?<model_id>#[[:digit:]]+)?$},
+    "{collection}" => %r{^/?.*?(?<collection>[[:print:]&&[^/]]*?)(?<model_id>#[[:digit:]]+)?$},
+    "{tags}/{creator}" => %r{^/?.*?(?<tags>[[:print:]]*)/(?<creator>[[:print:]&&[^/]]*?)(?<model_id>#[[:digit:]]+)?$},
+    "{tags}/{creator}/{modelName}" => %r{^/?.*?(?<tags>[[:print:]]*)/(?<creator>[[:print:]&&[^/]]*?)/(?<model_name>[[:print:]&&[^/]]*?)(?<model_id>#[[:digit:]]+)?$},
+    "@{creator}" => %r{^/?.*?@(?<creator>[[:print:]&&[^/]]*?)(?<model_id>#[[:digit:]]+)?$},
+    "{creator}/{collection}/{tags}/{modelName}" => %r{^/?.*?(?<creator>[[:print:]&&[^/]]*?)/(?<collection>[[:print:]&&[^/]]*?)/(?<tags>[[:print:]]*)/(?<model_name>[[:print:]&&[^/]]*?)(?<model_id>#[[:digit:]]+)?$}
   }.each_pair do |tag, regexp|
     it "correctly converts #{tag} into a regexp matcher" do
       allow(SiteSettings).to receive(:model_path_template).and_return(tag)
@@ -21,27 +21,27 @@
   end
 
   {
-    "{tags}/{modelName}{modelId}" => {
+    "{tags}/{modelName}" => {
       tags: ["top", "middle", "bottom"],
       model_name: "prefix - name"
     },
-    "{creator}/{modelName}{modelId}" => {
+    "{creator}/{modelName}" => {
       creator: "bottom",
       model_name: "prefix - name"
     },
-    "{collection}/{modelName}{modelId}" => {
+    "{collection}/{modelName}" => {
       collection: "bottom",
       model_name: "prefix - name"
     },
-    "{tags}/{creator}/{modelName}{modelId}" => {
+    "{tags}/{creator}/{modelName}" => {
       creator: "bottom",
       tags: ["top", "middle"],
       model_name: "prefix - name"
     },
-    "{creator}{modelId}" => {
+    "{creator}" => {
       creator: "prefix - name"
     },
-    "{tags}/{creator}/{collection} - {modelName}{modelId}" => {
+    "{tags}/{creator}/{collection} - {modelName}" => {
       tags: ["top", "middle"],
       creator: "bottom",
       collection: "prefix",