Switch to using friendly_id for product permalink generation

It's better for other people (who know better) to maintain at least some of the permalink generation code than for us to do it.

Old permalinks will still function correctly.

Proposed fix for #4167 + others

api/app/controllers/spree/api/base_controller.rb

@@ -120,7 +120,7 @@ def api_key
 
       def find_product(id)
         begin
-          product_scope.find_by!(permalink: id.to_s)
+          product_scope.friendly.find(id.to_s)
         rescue ActiveRecord::RecordNotFound
           product_scope.find(id)
         end

api/app/controllers/spree/api/products_controller.rb

@@ -55,30 +55,25 @@ def create
         params[:product][:available_on] ||= Time.now
         set_up_shipping_category
 
-        begin
-          @product = Product.new(product_params)
-          if @product.save
-            variants_params.each do |variant_attribute|
-              # make sure the product is assigned before the options=
-              @product.variants.create({ product: @product }.merge(variant_attribute))
-            end
-
-            option_types_params.each do |name|
-              option_type = OptionType.where(name: name).first_or_initialize do |option_type|
-                option_type.presentation = name
-                option_type.save!
-              end
+        @product = Product.new(product_params)
+        if @product.save
+          variants_params.each do |variant_attribute|
+            # make sure the product is assigned before the options=
+            @product.variants.create({ product: @product }.merge(variant_attribute))
+          end
 
-              @product.option_types << option_type unless @product.option_types.include?(option_type)
+          option_types_params.each do |name|
+            option_type = OptionType.where(name: name).first_or_initialize do |option_type|
+              option_type.presentation = name
+              option_type.save!
             end
 
-            respond_with(@product, :status => 201, :default_template => :show)
-          else
-            invalid_resource!(@product)
+            @product.option_types << option_type unless @product.option_types.include?(option_type)
           end
-        rescue ActiveRecord::RecordNotUnique
-          @product.permalink = nil
-          retry
+
+          respond_with(@product, :status => 201, :default_template => :show)
+        else
+          invalid_resource!(@product)
         end
       end
 

api/app/controllers/spree/api/variants_controller.rb

@@ -46,7 +46,7 @@ def update
       private
 
         def product
-          @product ||= Spree::Product.accessible_by(current_ability, :read).find_by(permalink: params[:product_id]) if params[:product_id]
+          @product ||= Spree::Product.accessible_by(current_ability, :read).friendly.find(params[:product_id]) if params[:product_id]
         end
 
         def scope

api/app/helpers/spree/api/api_helpers.rb

@@ -40,12 +40,14 @@ def required_fields_for(model)
         # Permalinks presence is validated, but are really automatically generated
         # Therefore we shouldn't tell API clients that they MUST send one through
         required_fields.map!(&:to_s).delete("permalink")
+        # Do not require slugs, either
+        required_fields.delete("slug")
         required_fields
       end
 
       @@product_attributes = [
         :id, :name, :description, :price, :display_price, :available_on,
-        :permalink, :meta_description, :meta_keywords, :shipping_category_id,
+        :slug, :meta_description, :meta_keywords, :shipping_category_id,
         :taxon_ids
       ]
 
@@ -55,7 +57,7 @@ def required_fields_for(model)
 
       @@variant_attributes = [
         :id, :name, :sku, :price, :weight, :height, :width, :depth, :is_master,
-        :cost_price, :permalink, :description
+        :cost_price, :slug, :description
       ]
 
       @@image_attributes = [

api/spec/controllers/spree/api/products_controller_spec.rb

@@ -7,7 +7,7 @@ module Spree
 
     let!(:product) { create(:product) }
     let!(:inactive_product) { create(:product, :available_on => Time.now.tomorrow, :name => "inactive") }
-    let(:base_attributes) { [:id, :name, :description, :price, :display_price, :available_on, :permalink, :meta_description, :meta_keywords, :shipping_category_id, :taxon_ids] }
+    let(:base_attributes) { [:id, :name, :description, :price, :display_price, :available_on, :slug, :meta_description, :meta_keywords, :shipping_category_id, :taxon_ids] }
     let(:show_attributes) { base_attributes.dup.push(:has_variants) }
     let(:new_attributes) { base_attributes }
 
@@ -133,20 +133,20 @@ module Spree
       end
 
 
-      context "finds a product by permalink first then by id" do
-        let!(:other_product) { create(:product, :permalink => "these-are-not-the-droids-you-are-looking-for") }
+      context "finds a product by slug first then by id" do
+        let!(:other_product) { create(:product, :slug => "these-are-not-the-droids-you-are-looking-for") }
 
         before do
-          product.update_attribute(:permalink, "#{other_product.id}-and-1-ways")
+          product.update_column(:slug, "#{other_product.id}-and-1-ways")
         end
 
         specify do
           api_get :show, :id => product.to_param
-          json_response["permalink"].should =~ /and-1-ways/
+          json_response["slug"].should =~ /and-1-ways/
           product.destroy
 
           api_get :show, :id => other_product.id
-          json_response["permalink"].should =~ /droids/
+          json_response["slug"].should =~ /droids/
         end
       end
 
@@ -298,8 +298,9 @@ module Spree
           response.status.should == 422
           json_response["error"].should == "Invalid resource. Please fix errors and try again."
           errors = json_response["errors"]
-          errors.delete("permalink") # Don't care about this one.
-          errors.keys.should =~ ["name", "price", "shipping_category_id"]
+          errors.keys.should include("name")
+          errors.keys.should include("price")
+          errors.keys.should include("shipping_category_id")
         end
       end
 

api/spec/controllers/spree/api/unauthenticated_products_controller_spec.rb

@@ -6,7 +6,7 @@ module Spree
     render_views
 
     let!(:product) { create(:product) }
-    let(:attributes) { [:id, :name, :description, :price, :available_on, :permalink, :meta_description, :meta_keywords, :taxon_ids] }
+    let(:attributes) { [:id, :name, :description, :price, :available_on, :slug, :meta_description, :meta_keywords, :taxon_ids] }
 
     context "without authentication" do
       before { Spree::Api::Config[:requires_authentication] = false }

api/spec/controllers/spree/api/variants_controller_spec.rb

@@ -10,7 +10,7 @@ module Spree
       variant.option_values << create(:option_value)
       variant
     end
-    let!(:base_attributes) { [:id, :name, :sku, :price, :weight, :height, :width, :depth, :is_master, :cost_price, :permalink, :description] }
+    let!(:base_attributes) { [:id, :name, :sku, :price, :weight, :height, :width, :depth, :is_master, :cost_price, :slug, :description] }
     let!(:show_attributes) { base_attributes.dup.push(:in_stock, :display_price) }
     let!(:new_attributes) { base_attributes }
 

backend/app/controllers/spree/admin/images_controller.rb

@@ -17,7 +17,7 @@ def location_after_save
         end
 
         def load_data
-          @product = Product.find_by_permalink(params[:product_id])
+          @product = Product.friendly.find(params[:product_id])
           @variants = @product.variants.collect do |variant|
             [variant.sku_and_options_text, variant.id]
           end

backend/app/controllers/spree/admin/product_properties_controller.rb

@@ -1,7 +1,7 @@
 module Spree
   module Admin
     class ProductPropertiesController < ResourceController
-      belongs_to 'spree/product', :find_by => :permalink
+      belongs_to 'spree/product', :find_by => :slug
       before_filter :find_properties
       before_filter :setup_property, :only => [:index]
 

backend/app/controllers/spree/admin/products_controller.rb

@@ -25,11 +25,24 @@ def update
         if params[:product][:option_type_ids].present?
           params[:product][:option_type_ids] = params[:product][:option_type_ids].split(',')
         end
-        super
+        invoke_callbacks(:update, :before)
+        if @object.update_attributes(permitted_resource_params)
+          invoke_callbacks(:update, :after)
+          flash[:success] = flash_message_for(@object, :successfully_updated)
+          respond_with(@object) do |format|
+            format.html { redirect_to location_after_save }
+            format.js   { render :layout => false }
+          end
+        else
+          # Stops people submitting blank slugs, causing errors when they try to update the product again
+          @product.slug = @product.slug_was if @product.slug.blank?
+          invoke_callbacks(:update, :fails)
+          respond_with(@object)
+        end
       end
 
       def destroy
-        @product = Product.find_by_permalink!(params[:id])
+        @product = Product.friendly.find(params[:id])
         @product.destroy
 
         flash[:success] = Spree.t('notice_messages.product_deleted')
@@ -65,7 +78,7 @@ def stock
       protected
 
         def find_resource
-          Product.find_by_permalink!(params[:id])
+          Product.friendly.find(params[:id])
         end
 
         def location_after_save

backend/app/controllers/spree/admin/variants_controller.rb

@@ -1,7 +1,7 @@
 module Spree
   module Admin
     class VariantsController < ResourceController
-      belongs_to 'spree/product', :find_by => :permalink
+      belongs_to 'spree/product', :find_by => :slug
       new_action.before :new_before
       before_filter :load_data, :only => [:new, :create, :edit, :update]
 

backend/app/views/spree/admin/products/_form.html.erb

@@ -7,10 +7,10 @@
       <%= f.error_message_on :name %>
     <% end %>
 
-    <%= f.field_container :permalink do %>
-      <%= f.label :permalink, raw(Spree.t(:permalink) + content_tag(:span, ' *',  :class => "required")) %>
-      <%= f.text_field :permalink, :class => 'fullwidth title' %>
-      <%= f.error_message_on :permalink %>
+    <%= f.field_container :slug do %>
+      <%= f.label :slug, raw(Spree.t(:slug) + content_tag(:span, ' *',  :class => "required")) %>
+      <%= f.text_field :slug, :class => 'fullwidth title' %>
+      <%= f.error_message_on :slug %>
     <% end %>
 
     <%= f.field_container :description do %>

backend/spec/features/admin/products/edit/products_spec.rb

@@ -6,7 +6,7 @@
 
   context 'editing a product' do
     it 'should list the product details' do
-      create(:product, :name => 'Bún thịt nướng', :permalink => 'bun-thit-nuong', :sku => 'A100',
+      create(:product, :name => 'Bún thịt nướng', :sku => 'A100',
               :description => 'lorem ipsum', :available_on => '2013-08-14 01:02:03')
 
       visit spree.admin_path
@@ -17,16 +17,16 @@
 
       find('.page-title').text.strip.should == 'Editing Product “Bún thịt nướng”'
       find('input#product_name').value.should == 'Bún thịt nướng'
-      find('input#product_permalink').value.should == 'bun-thit-nuong'
+      find('input#product_slug').value.should == 'bun-th-t-n-ng'
       find('textarea#product_description').text.strip.should == 'lorem ipsum'
       find('input#product_price').value.should == '19.99'
       find('input#product_cost_price').value.should == '17.00'
       find('input#product_available_on').value.should == "2013/08/14"
       find('input#product_sku').value.should == 'A100'
     end
 
-    it "should handle permalink changes" do
-      create(:product, :name => 'Bún thịt nướng', :permalink => 'bun-thit-nuong', :sku => 'A100',
+    it "should handle slug changes" do
+      create(:product, :name => 'Bún thịt nướng', :sku => 'A100',
               :description => 'lorem ipsum', :available_on => '2011-01-01 01:01:01')
 
       visit spree.admin_path
@@ -35,18 +35,15 @@
         click_icon(:edit)
       end
 
-      fill_in "product_permalink", :with => 'random-permalink-value'
+      fill_in "product_slug", :with => 'random-slug-value'
       click_button "Update"
       page.should have_content("successfully updated!")
 
-      fill_in "product_permalink", :with => ''
+      fill_in "product_slug", :with => ''
       click_button "Update"
-      within('#product_permalink_field') { page.should have_content("can't be blank") }
+      within('#product_slug_field') { page.should have_content("is too short") }
 
-      click_button "Update"
-      within('#product_permalink_field') { page.should have_content("can't be blank") }
-
-      fill_in "product_permalink", :with => 'another-random-permalink-value'
+      fill_in "product_slug", :with => 'another-random-slug-value'
       click_button "Update"
       page.should have_content("successfully updated!")
     end

core/app/models/spree/product.rb

@@ -20,6 +20,9 @@
 
 module Spree
   class Product < ActiveRecord::Base
+    extend FriendlyId
+    friendly_id :name, use: :slugged
+
     acts_as_paranoid
     has_many :product_option_types, dependent: :destroy, inverse_of: :product
     has_many :option_types, through: :product_option_types
@@ -69,24 +72,20 @@ class Product < ActiveRecord::Base
     has_many :variant_images, -> { order(:position) }, source: :images, through: :variants_including_master
 
     validates :name, presence: true
-    validates :permalink, presence: true
     validates :price, presence: true, if: proc { Spree::Config[:require_master_price] }
     validates :shipping_category_id, presence: true
+    validates :slug, length: { minimum: 3 }
 
     attr_accessor :option_values_hash
 
     accepts_nested_attributes_for :product_properties, allow_destroy: true, reject_if: lambda { |pp| pp[:property_name].blank? }
 
-    make_permalink order: :name
-
     alias :options :product_option_types
 
     after_initialize :ensure_master
 
-    before_destroy :punch_permalink
-
     def to_param
-      permalink.present? ? permalink : (permalink_was || name.to_s.to_url)
+      slug
     end
 
     # the master variant is not a member of the variants array
@@ -247,10 +246,6 @@ def ensure_master
         return unless new_record?
         self.master ||= Variant.new
       end
-
-      def punch_permalink
-        update_attribute :permalink, "#{Time.now.to_i}_#{permalink}" # punch permalink with date prefix
-      end
   end
 end
 

core/app/models/spree/variant.rb

@@ -5,7 +5,7 @@ class Variant < ActiveRecord::Base
     belongs_to :product, touch: true, class_name: 'Spree::Product', inverse_of: :variants
     belongs_to :tax_category, class_name: 'Spree::TaxCategory'
 
-    delegate_belongs_to :product, :name, :description, :permalink, :available_on,
+    delegate_belongs_to :product, :name, :description, :slug, :available_on,
                         :shipping_category_id, :meta_description, :meta_keywords,
                         :shipping_category
 

core/config/locales/en.yml

@@ -1014,6 +1014,7 @@ en:
     site_name: Site Name
     site_url: Site URL
     sku: SKU
+    slug: Slug
     smtp: SMTP
     smtp_authentication_type: SMTP Authentication Type
     smtp_domain: SMTP Domain

core/db/migrate/20140106224208_rename_permalink_to_slug_for_products.rb

@@ -0,0 +1,5 @@
+class RenamePermalinkToSlugForProducts < ActiveRecord::Migration
+  def change
+    rename_column :spree_products, :permalink, :slug
+  end
+end

core/lib/spree/core.rb

@@ -9,6 +9,7 @@
 require 'paranoia'
 require 'ransack'
 require 'state_machine'
+require 'friendly_id'
 
 module Spree
 

core/lib/spree/core/permalinks.rb

@@ -68,4 +68,4 @@ def save_permalink(permalink_value=self.to_param)
 end
 
 ActiveRecord::Base.send :include, Spree::Core::Permalinks
-ActiveRecord::Relation.send :include, Spree::Core::Permalinks
+ActiveRecord::Relation.send :include, Spree::Core::Permalinks