Merge pull request #11914 from spree/fix/generating-slugs-spree-4-6

api/spec/requests/spree/api/v2/storefront/products_spec.rb

@@ -996,15 +996,15 @@
 
     context 'with localized_slugs' do
       let(:store2) { create(:store, default_locale: 'en', supported_locales: 'en,pl,es') }
-      let(:product_with_slug) { create(:product, stores: [store2], slug: 'test_slug_en') }
-      let!(:translation1) { product_with_slug.translations.create(slug: 'test_slug_pl', locale: 'pl')  }
-      let!(:translation2) { product_with_slug.translations.create(slug: 'test_slug_es', locale: 'es')  }
+      let(:product_with_slug) { create(:product, stores: [store2], slug: 'test-slug-en') }
+      let!(:translation1) { product_with_slug.translations.create(slug: 'test-slug-pl', locale: 'pl')  }
+      let!(:translation2) { product_with_slug.translations.create(slug: 'test-slug-es', locale: 'es')  }
 
       let(:expected_result) do
         {
-          en: 'test_slug_en',
-          pl: 'test_slug_pl',
-          es: 'test_slug_es'
+          en: 'test-slug-en',
+          pl: 'test-slug-pl',
+          es: 'test-slug-es'
         }
       end
 
@@ -1021,7 +1021,7 @@
     context 'with slug fallback to default locale' do
       let(:store2) { create(:store, default_locale: 'en', supported_locales: 'en,pl,es') }
       let!(:product_with_slug) { create(:product, stores: [store2], slug: default_locale_slug) }
-      let(:default_locale_slug) { 'test_slug_en' }
+      let(:default_locale_slug) { 'test-slug-en' }
 
       before do
         allow_any_instance_of(Spree::Api::V2::Storefront::ProductsController).to receive(:current_store).and_return(store2)
@@ -1037,8 +1037,8 @@
       let(:store2) { create(:store, default_locale: 'en', supported_locales: 'en,pl,es') }
       let(:product_with_slug) { create(:product, stores: [store2], slug: default_locale_slug) }
       let!(:translation2) { product_with_slug.translations.create(slug: translated_slug, locale: 'es')  }
-      let(:default_locale_slug) { 'test_slug_en' }
-      let(:translated_slug) { 'test_slug_es' }
+      let(:default_locale_slug) { 'test-slug-en' }
+      let(:translated_slug) { 'test-slug-es' }
 
       before do
         allow_any_instance_of(Spree::Api::V2::Storefront::ProductsController).to receive(:current_store).and_return(store2)

api/spec/requests/spree/api/v2/storefront/taxons_spec.rb

@@ -226,9 +226,9 @@
 
       let(:expected_slugs) do
         {
-          en: 'categories/test_slug_en',
-          pl: 'categories/test_slug_pl',
-          es: 'categories/test_slug_es'
+          en: 'categories/test-slug-en',
+          pl: 'categories/test-slug-pl',
+          es: 'categories/test-slug-es'
         }
       end
 
@@ -246,7 +246,7 @@
       let(:store2) { create(:store, default_locale: 'en', supported_locales: 'en,pl,es') }
       let(:taxonomy) { create(:taxonomy, name: 'categories', store: store2) }
       let!(:taxon_with_slug) { create(:taxon, taxonomy: taxonomy, name: 'test slug en', permalink: default_locale_slug) }
-      let(:default_locale_slug) { 'test_slug_en' }
+      let(:default_locale_slug) { 'test-slug-en' }
 
       before do
         allow_any_instance_of(Spree::Api::V2::Storefront::TaxonsController).to receive(:current_store).and_return(store2)
@@ -264,7 +264,7 @@
       let!(:taxon_with_slug) { create(:taxon, taxonomy: taxonomy, permalink: default_locale_slug) }
       let!(:translations) { taxon_with_slug.translations.create([ { name: 'test slug en', permalink: translated_slug, locale: 'es' } ]) }
       let(:default_locale_slug) { 'test_slug_en' }
-      let(:translated_slug) { 'test_slug_es' }
+      let(:translated_slug) { 'test-slug-es' }
 
       before do
         allow_any_instance_of(Spree::Api::V2::Storefront::TaxonsController).to receive(:current_store).and_return(store2)

core/app/models/spree/product.rb

@@ -42,9 +42,25 @@ class Product < Spree::Base
     translates(*TRANSLATABLE_FIELDS)
 
     self::Translation.class_eval do
+      before_save :set_slug
       acts_as_paranoid
       # deleted translation values also need to be accessible for index views listing deleted resources
       default_scope { unscope(where: :deleted_at) }
+      def set_slug
+        self.slug = generate_slug
+      end
+
+      private
+
+      def generate_slug
+        if name.blank? && slug.blank?
+          translated_model.name.to_url
+        elsif slug.blank?
+          name.to_url
+        else
+          slug.to_url
+        end
+      end
     end
 
     friendly_id :slug_candidates, use: [:history, :mobility]

core/app/models/spree/taxon.rb

@@ -62,19 +62,40 @@ class Taxon < Spree::Base
 
     self::Translation.class_eval do
       alias_attribute :slug, :permalink
-
-      before_create :set_permalink
+      before_save :set_permalink
 
       def set_permalink
-        parent = translated_model.parent
-        name_with_fallback = name || translated_model.name
+        self.permalink = generate_slug
+      end
+
+      private
 
+      def generate_slug
         if parent.present?
-          self.permalink = [parent.permalink, (permalink.blank? ? name_with_fallback.to_url : permalink.split('/').last)].join('/')
+          generate_permalink_including_parent
+        elsif permalink.blank?
+          name_with_fallback.to_url
         else
-          self.permalink = name_with_fallback.to_url if permalink.blank?
+          permalink.to_url
         end
       end
+
+      def generate_permalink_including_parent
+        [parent_permalink_with_fallback, (permalink.blank? ? name_with_fallback.to_url : permalink.split('/').last.to_url)].join('/')
+      end
+
+      def name_with_fallback
+        name.blank? ? translated_model.name : name
+      end
+
+      def parent
+        translated_model.parent
+      end
+
+      def parent_permalink_with_fallback
+        localized_parent = parent.translations.find_by(locale: locale)
+        localized_parent.present? ? localized_parent.permalink : parent.permalink
+      end
     end
 
     # indicate which filters should be used for a taxon

core/lib/spree/core/product_duplicator.rb

@@ -26,7 +26,10 @@ def duplicate
 
     def duplicate_product
       product.dup.tap do |new_product|
-        product.translations.each { |t| new_product.send(:name=, "COPY OF #{t.name}", locale: t.locale) }
+        new_product.translations.each do |t|
+          t.name = "COPY OF #{t.name}"
+          t.slug = nil
+        end
         new_product.taxons = product.taxons
         new_product.stores = product.stores
         new_product.created_at = nil

core/spec/models/spree/product_spec.rb

@@ -254,14 +254,14 @@ class Extension < Spree::Base
 
         context 'when more than one translation exists' do
           before {
-            product.send(:slug=, "french_slug", locale: :fr)
+            product.send(:slug=, "french-slug", locale: :fr)
             product.save!
           }
 
           it 'renames slug for all translations' do
             product.destroy
             expect(product.translations.with_deleted.where(locale: :en).first.slug).to match(/[0-9]+_product-[0-9]+/)
-            expect(product.translations.with_deleted.where(locale: :fr).first.slug).to match(/[0-9]+_french_slug/)
+            expect(product.translations.with_deleted.where(locale: :fr).first.slug).to match(/[0-9]+_french-slug/)
           end
         end
 
@@ -921,7 +921,7 @@ def build_option_type_with_values(name, values)
 
   describe '#localized_slugs_for_store' do
     let(:store) { create(:store, default_locale: 'fr', supported_locales: 'en,pl,fr') }
-    let(:product) { create(:product, stores: [store], slug: 'test_slug_en') }
+    let(:product) { create(:product, stores: [store], name: 'Test product', slug: 'test_slug_en') }
     let!(:product_translation_fr) { product.translations.create(slug: 'test_slug_fr', locale: 'fr') }
 
     before { Spree::Locales::SetFallbackLocaleForStore.new.call(store: store) }
@@ -934,9 +934,9 @@ def build_option_type_with_values(name, values)
 
       let(:expected_slugs) do
         {
-          'en' => 'test_slug_en',
-          'fr' => 'test_slug_fr',
-          'pl' => 'test_slug_pl'
+          'en' => 'test-slug-en',
+          'fr' => 'test-slug-fr',
+          'pl' => 'test-slug-pl'
         }
       end
 
@@ -948,15 +948,69 @@ def build_option_type_with_values(name, values)
     context 'when one of the supported locales does not have a translation' do
       let(:expected_slugs) do
         {
-          'en' => 'test_slug_en',
-          'fr' => 'test_slug_fr',
-          'pl' => 'test_slug_fr'
+          'en' => 'test-slug-en',
+          'fr' => 'test-slug-fr',
+          'pl' => 'test-slug-fr'
         }
       end
 
       it "falls back to store's default locale" do
         expect(subject).to match(expected_slugs)
       end
     end
+
+    context 'the slugs are generated from name when slug field is empty' do
+      before do
+        product_translation_fr.update(slug: nil, name: "slug from name")
+      end
+
+      let(:expected_slugs) do
+        {
+          'en' => 'test-slug-en',
+          'fr' => 'slug-from-name',
+          'pl' => 'slug-from-name'
+        }
+      end
+
+      it "saves slugs generated from name" do
+        expect(subject).to match(expected_slugs)
+      end
+    end
+
+    context 'the slugs are generated from default locale name when name and slug for translation is empty' do
+      before do
+        product_translation_fr.update(slug: nil, name: nil)
+      end
+
+      let(:expected_slugs) do
+        {
+          'en' => 'test-slug-en',
+          'fr' => 'test-product',
+          'pl' => 'test-product'
+        }
+      end
+
+      it 'saves slugs generated from fallback name' do
+        expect(subject).to match(expected_slugs)
+      end
+    end
+
+    context 'the slugs are generated from invalid slug format' do
+      before do
+        product_translation_fr.update(slug: "slug with_spaces")
+      end
+
+      let(:expected_slugs) do
+        {
+          'en' => 'test-slug-en',
+          'fr' => 'slug-with-spaces',
+          'pl' => 'slug-with-spaces'
+        }
+      end
+
+      it 'saves slugs in valid format' do
+        expect(subject).to match(expected_slugs)
+      end
+    end
   end
 end

core/spec/models/spree/taxon_spec.rb

@@ -188,6 +188,7 @@
     let(:taxonomy) { create(:taxonomy, name: 'categories', store: store) }
     let(:taxon) { create(:taxon, taxonomy: taxonomy, permalink: 'test_slug_en') }
     let!(:taxon_translation_fr) { taxon.translations.create(slug: 'test_slug_fr', locale: 'fr') }
+    let!(:root_taxon) { taxonomy.taxons.find_by(parent_id: nil) }
 
     before { Spree::Locales::SetFallbackLocaleForStore.new.call(store: store) }
 
@@ -199,9 +200,9 @@
 
       let(:expected_slugs) do
         {
-          'en' => 'categories/test_slug_en',
-          'fr' => 'categories/test_slug_fr',
-          'pl' => 'categories/test_slug_pl'
+          'en' => 'categories/test-slug-en',
+          'fr' => 'categories/test-slug-fr',
+          'pl' => 'categories/test-slug-pl'
         }
       end
 
@@ -213,15 +214,48 @@
     context 'when one of the supported locales does not have a translation' do
       let(:expected_slugs) do
         {
-          'en' => 'categories/test_slug_en',
-          'fr' => 'categories/test_slug_fr',
-          'pl' => 'categories/test_slug_fr'
+          'en' => 'categories/test-slug-en',
+          'fr' => 'categories/test-slug-fr',
+          'pl' => 'categories/test-slug-fr'
         }
       end
 
       it "falls back to store's default locale" do
         expect(subject).to match(expected_slugs)
       end
     end
+
+    context 'when setting the slug translations for taxonomy' do
+      let!(:root_taxon_translation_pl) { root_taxon.translations.create(slug: 'slug with space', locale: 'pl') }
+
+      let(:expected_slugs) do
+        {
+          'en' => 'categories',
+          'fr' => 'categories',
+          'pl' => 'slug-with-space'
+        }
+      end
+
+      it "sets the slugs in slug format" do
+        expect(root_taxon.localized_slugs_for_store(store)).to match(expected_slugs)
+      end
+    end
+
+    context 'when setting the slugs in taxon under taxomony with different parent slug' do
+      let!(:root_taxon_translation_pl) { root_taxon.translations.create(slug: 'slug with space', locale: 'pl') }
+      let!(:taxon_translation_pl) { taxon.translations.create(locale: 'pl') }
+
+      let(:expected_slugs) do
+        {
+          'en' => 'categories/test-slug-en',
+          'fr' => 'categories/test-slug-fr',
+          'pl' => "slug-with-space/#{taxon.name.to_url}"
+        }
+      end
+
+      it "sets the slug in valid format" do
+        expect(taxon.localized_slugs_for_store(store)).to match(expected_slugs)
+      end
+    end
   end
 end