From 169f298ab1e45a997c00d6ba2f1ce8dd1daf3545 Mon Sep 17 00:00:00 2001 From: Igal Koshevoy Date: Sat, 24 Sep 2011 10:27:58 -0700 Subject: [PATCH] [migration] Add customizable slugs. * Implement CustomizableSlug mixin to wrap and improve FriendlyId. * Add slug fields to models and views. * Redirect show requests to old slugs based on history. * Describe behavior in specs. --- app/controllers/application_controller.rb | 9 ++ app/controllers/companies_controller.rb | 1 + app/controllers/groups_controller.rb | 1 + app/controllers/people_controller.rb | 1 + app/controllers/projects_controller.rb | 1 + app/controllers/users_controller.rb | 2 + app/mixins/customizable_slug.rb | 113 ++++++++++++++++++ app/models/company.rb | 2 + app/models/group.rb | 2 + app/models/person.rb | 3 + app/models/project.rb | 2 + app/views/companies/_form.html.haml | 1 + app/views/groups/_form.html.haml | 1 + app/views/people/_form.html.haml | 2 +- app/views/projects/_form.html.haml | 1 + app/views/site/_custom_slug_field.html.haml | 10 ++ config/initializers/customizable_slug.rb | 3 + config/locales/en.yml | 7 ++ config/locales/fr.yml | 7 ++ ...20110924171944_create_friendly_id_slugs.rb | 18 +++ db/migrate/20110924171945_add_slugs.rb | 20 ++++ db/schema.rb | 26 +++- spec/acceptance/people_spec.rb | 39 ++++++ spec/models/person_spec.rb | 56 +++++++++ 24 files changed, 326 insertions(+), 2 deletions(-) create mode 100644 app/mixins/customizable_slug.rb create mode 100644 app/views/site/_custom_slug_field.html.haml create mode 100644 config/initializers/customizable_slug.rb create mode 100644 db/migrate/20110924171944_create_friendly_id_slugs.rb create mode 100644 db/migrate/20110924171945_add_slugs.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index cba93bc..b76a9a9 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -107,4 +107,13 @@ def page_title(value=nil) end end helper_method :page_title + + # Preserve old links to resources by redirecting to their current location. + # + # The "friendly_id" slug history tracks the resource's slug over time. If a resource is available under a newer slug name, redirect its "show" action to the current name. E.g. "/request/old-name" will redirect to "/request/new-name" if the slug was changed from "old-name" to "new-name". + def redirect_historical_slugs + if self.action_name == "show" && request.path != resource_path(resource) + return redirect_to resource, :status => :moved_permanently + end + end end diff --git a/app/controllers/companies_controller.rb b/app/controllers/companies_controller.rb index 98a9cdc..c325a2f 100644 --- a/app/controllers/companies_controller.rb +++ b/app/controllers/companies_controller.rb @@ -4,6 +4,7 @@ class CompaniesController < InheritedResources::Base :resource => [:join, :leave] before_filter :authenticate_user!, :except => [:index, :show, :tag] + before_filter :redirect_historical_slugs def tag @tag = params[:tag] diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index f1d57bf..25eba89 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -4,6 +4,7 @@ class GroupsController < InheritedResources::Base :resource => [:join, :leave] before_filter :authenticate_user!, :except => [:index, :show, :tag] + before_filter :redirect_historical_slugs def tag @tag = params[:tag] diff --git a/app/controllers/people_controller.rb b/app/controllers/people_controller.rb index 5a0c768..cab5ad0 100644 --- a/app/controllers/people_controller.rb +++ b/app/controllers/people_controller.rb @@ -8,6 +8,7 @@ class PeopleController < InheritedResources::Base before_filter :require_owner_or_admin!, :only => [:edit, :update, :destroy] before_filter :pick_photo_input, :only => [:update, :create] before_filter :set_user_id_if_admin, :only => [:update, :create] + before_filter :redirect_historical_slugs def index @view = :grid if params[:grid] diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index da66666..a22461c 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -4,6 +4,7 @@ class ProjectsController < InheritedResources::Base :resource => [:join, :leave] before_filter :authenticate_user!, :except => [:index, :show, :tag] + before_filter :redirect_historical_slugs def tag @tag = params[:tag] diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 3568ed7..db14f54 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -35,6 +35,8 @@ def welcome @possible_duplicates += Person.unclaimed.where(name_part_query, *name_parts.map{|p| "%#{p}%"}).take(10) @possible_duplicates.uniq! + + @person.send(:set_slug) end end end diff --git a/app/mixins/customizable_slug.rb b/app/mixins/customizable_slug.rb new file mode 100644 index 0000000..ef37e1b --- /dev/null +++ b/app/mixins/customizable_slug.rb @@ -0,0 +1,113 @@ +# = CustomizableSlug +# +# This mixin provides a easy-to-use wrapper for setting up customizable slugs +# using the friendly_id plugin. +# +# @example To add a customizable slug whose default value is based on the :name +# field, add this to your ActiveRecord model: +# +# class MyModel +# customizable_slug_from :name +# end +# +# @example To use the customizable field: +# +# m = MyModel.new(:name => "foo") +# +# m.save +# m.slug # => "foo" +# +# m.custom_slug = "bar" +# m.save +# m.slug # => "bar" +module CustomizableSlug + + # Override behavior of gem. + require 'friendly_id/slug_generator' + class CustomizableSlugGenerator < FriendlyId::SlugGenerator + # When generating the slug, if there's a conflict, stop immediately and + # mark the record as an error so user can pick something else. + # + # The gem's original behavior is to always generate a unique slug, even if + # this means adding a number to the end of it. This is bad design because + # if the user wants to be "foo", but that's taken, they'll end up as + # "foo-2", rather than being told to pick another slug. + def generate + if conflict? + sluggable.errors.add(:custom_slug, I18n.t('activerecord.errors.messages.taken')) + end + return normalized + end + + # Check history for conflicts, if using history. + # + # The gem's original behavior doesn't check history, so if you try to + # create a conflicting record, the #save will fail with a raw SQL + # uniqueness constraint error. + def conflicts + # If any regular conflicts are found, return them immediately. + scope = super + return scope if scope.count > 0 + + # If no regular conflicts are found, search the history. + if friendly_id_config.model_class.included_modules.include?(FriendlyId::History) + history = FriendlyId::Slug.where(:slug => normalized, :sluggable_type => self.sluggable.class.to_s) + unless self.sluggable.new_record? + # If record exists, exclude it from the history check. + history = history.where('sluggable_id <> ?', self.sluggable.id) + end + + return history if history.count > 0 + end + + # No conflicts of any sort found. + return [] + end + end + + def self.included(base) + base.send(:extend, ::CustomizableSlug::ClassMethods) + end + + module ClassMethods + # Managing customizable custom slug for +attribute+, e.g. :name. + def customizable_slug_from(attribute) + # Attribute which contains the source value to use for generating the + # slug, e.g. :name. + cattr_accessor :friendly_id_source_attribute + self.friendly_id_source_attribute = attribute + + # Activate "friendly_id" plugin. + extend FriendlyId + friendly_id :custom_slug_or_source, :use => :history, :slug_generator_class => ::CustomizableSlug::CustomizableSlugGenerator + + # Add validation for "custom_slug" field. + validate :validate_custom_slug + end + end + + # Return the user-specified custom slug or the friendly id for this record. + def custom_slug + @custom_slug.presence || self.friendly_id + end + + # Set the custom slug to +value+. + def custom_slug=(value) + @custom_slug = value + end + + # Return the custom slug or the value of the attribute that contains the source value. + def custom_slug_or_source + @custom_slug.presence || "#{self.send(self.class.friendly_id_source_attribute)}" + end + + def validate_custom_slug + # Ensure the slug starts with a letter, or Rails will run #to_i on it to + # get a number and find the record by numeric id rather than the slug. :( + # + # TODO Figure out what to do about slugs that really start with a digit, e.g. "37signals". + if @custom_slug.present? && @custom_slug !~ /^\D/ + self.errors.add(:custom_slug, I18n.t('activerecord.errors.must_start_with_non_digit')) + end + end +end diff --git a/app/models/company.rb b/app/models/company.rb index 2f89ff3..aa61188 100644 --- a/app/models/company.rb +++ b/app/models/company.rb @@ -14,6 +14,8 @@ class Company < ActiveRecord::Base import_image_from_url_as :logo + customizable_slug_from :name + has_many :company_projects has_many :projects, :through => :company_projects diff --git a/app/models/group.rb b/app/models/group.rb index a6c50b3..1a6f282 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -14,6 +14,8 @@ class Group < ActiveRecord::Base import_image_from_url_as :logo + customizable_slug_from :name + has_many :group_projects has_many :projects, :through => :group_projects diff --git a/app/models/person.rb b/app/models/person.rb index 2b8363c..922458a 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -17,6 +17,8 @@ class Person < ActiveRecord::Base import_image_from_url_as :photo, :gravatar => true + customizable_slug_from :name + belongs_to :user accepts_nested_attributes_for :user, :update_only => true @@ -77,6 +79,7 @@ def self.find_or_create_sample(create_backreference=true) end return person end + end diff --git a/app/models/project.rb b/app/models/project.rb index 63f96a1..aaa1879 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -14,6 +14,8 @@ class Project < ActiveRecord::Base import_image_from_url_as :logo + customizable_slug_from :name + has_many :project_memberships has_many :people, :through => :project_memberships diff --git a/app/views/companies/_form.html.haml b/app/views/companies/_form.html.haml index 07321ab..a30f4b2 100644 --- a/app/views/companies/_form.html.haml +++ b/app/views/companies/_form.html.haml @@ -2,6 +2,7 @@ = display_errors_for @company = f.inputs do = f.input :name + = render 'site/custom_slug_field', :form => f = f.input :url = f.input :address, :as => :string = f.input :description diff --git a/app/views/groups/_form.html.haml b/app/views/groups/_form.html.haml index f9adb3f..0be1c69 100644 --- a/app/views/groups/_form.html.haml +++ b/app/views/groups/_form.html.haml @@ -2,6 +2,7 @@ = display_errors_for @group = f.inputs do = f.input :name + = render 'site/custom_slug_field', :form => f = f.input :url = f.input :mailing_list = f.input :description diff --git a/app/views/people/_form.html.haml b/app/views/people/_form.html.haml index 260838c..f6574b5 100644 --- a/app/views/people/_form.html.haml +++ b/app/views/people/_form.html.haml @@ -3,7 +3,7 @@ = display_errors_for @person = f.inputs do = f.input :name - -# = f.input :twitter + = render 'site/custom_slug_field', :form => f - unless @person.new_record? = f.semantic_fields_for :user do |u| = u.input :email diff --git a/app/views/projects/_form.html.haml b/app/views/projects/_form.html.haml index 76a03a4..2e31083 100644 --- a/app/views/projects/_form.html.haml +++ b/app/views/projects/_form.html.haml @@ -2,6 +2,7 @@ = display_errors_for @project = f.inputs do = f.input :name + = render 'site/custom_slug_field', :form => f = f.input :url = f.input :description = f.input :tag_list, :as => :text, :input_html => {:class => 'tags'} diff --git a/app/views/site/_custom_slug_field.html.haml b/app/views/site/_custom_slug_field.html.haml new file mode 100644 index 0000000..8aa6433 --- /dev/null +++ b/app/views/site/_custom_slug_field.html.haml @@ -0,0 +1,10 @@ +-# Display the form field for a custom slug. +-# +-# ARGUMENTS: +-# * form: The Rails form instance. +- hint = "%{example} %{url}/%{my}-%{model}" % { | + :example => t('field.example.fragment'), | + :url => "#{SETTINGS.organization.url}/#{controller_path}", | + :my => t('field.my.fragment'), | + :model => t("activerecord.models.#{form.send(:model_name).underscore}").underscore } += form.input :custom_slug, :hint => hint.html_safe diff --git a/config/initializers/customizable_slug.rb b/config/initializers/customizable_slug.rb new file mode 100644 index 0000000..499126d --- /dev/null +++ b/config/initializers/customizable_slug.rb @@ -0,0 +1,3 @@ +class ActiveRecord::Base + include CustomizableSlug +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 5c87f90..107e9da 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -7,6 +7,7 @@ en: address: "Address" category: "Category" created_at: "Created at" + custom_slug: "URL slug" description: "Description" email: "Email address" location: "Location" @@ -30,6 +31,8 @@ en: person: bio: "Bio" location: "Location" + errors: + must_start_with_non_digit: "must start with a non-digit" sign_in: Sign In group: @@ -178,6 +181,8 @@ en: label: "Date added" # people#index email_address: label: "Email address" # authentications#_login + example: + fragment: "e.g." # site#_custom_slug_field group_list: title: "Groups" # people#show import_file: @@ -191,6 +196,8 @@ en: title: "Meeting Info" # groups#show members: title: "Members" # group#show + my: + fragment: "my" # site#_custom_slug_field name: label: "Name" # people#index project_list: diff --git a/config/locales/fr.yml b/config/locales/fr.yml index 32d7649..f98672e 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -5,6 +5,7 @@ fr: address: "Adresse" category: "Catégorie" created_at: "Créé le" + custom_slug: "Sobriquet de l'URL" # TODO review translation description: "Description" email: "Adresse email ou identifiant" location: "Adresse" @@ -40,6 +41,8 @@ fr: person: bio: "Bio" location: "Localisation" + errors: + must_start_with_non_digit: "doit commencer par un non-chiffres" # TODO review translation sign_in: Connexion group: @@ -185,6 +188,8 @@ fr: label: "Date de création" # people#index email_address: label: "Adresse email ou identifiant" # authentications#_login + example: + fragment: "pour exemple" # site#_custom_slug_field # TODO review translation group_list: title: "Groupes" # people#show import_file: @@ -198,6 +203,8 @@ fr: title: "Infos réunion" # groups#show members: title: "Membres" # groups#show + my: + fragment: "mon" # site#_custom_slug_field # TODO review translation name: label: "Nom" # people#index project_list: diff --git a/db/migrate/20110924171944_create_friendly_id_slugs.rb b/db/migrate/20110924171944_create_friendly_id_slugs.rb new file mode 100644 index 0000000..bb80e48 --- /dev/null +++ b/db/migrate/20110924171944_create_friendly_id_slugs.rb @@ -0,0 +1,18 @@ +class CreateFriendlyIdSlugs < ActiveRecord::Migration + + def self.up + create_table :friendly_id_slugs do |t| + t.string :slug, :null => false + t.integer :sluggable_id, :null => false + t.string :sluggable_type, :limit => 40 + t.datetime :created_at + end + add_index :friendly_id_slugs, :sluggable_id + add_index :friendly_id_slugs, [:slug, :sluggable_type], :unique => true + add_index :friendly_id_slugs, :sluggable_type + end + + def self.down + drop_table :friendly_id_slugs + end +end diff --git a/db/migrate/20110924171945_add_slugs.rb b/db/migrate/20110924171945_add_slugs.rb new file mode 100644 index 0000000..2bff290 --- /dev/null +++ b/db/migrate/20110924171945_add_slugs.rb @@ -0,0 +1,20 @@ +class AddSlugs < ActiveRecord::Migration + TABLES = %w[people companies projects groups] + + def self.up + for table in TABLES + add_column table, :slug, :string + add_index table, :slug, :unique => true + + # Add slug to all records + table.classify.constantize.find_each(&:save) + end + end + + def self.down + for table in TABLES + remove_index table, :slug + remove_column table, :slug + end + end +end diff --git a/db/schema.rb b/db/schema.rb index fb60b1c..af7f426 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1,3 +1,4 @@ +# encoding: UTF-8 # This file is auto-generated from the current state of the database. Instead # of editing this file, please use the migrations feature of Active Record to # incrementally modify your database, and then regenerate this schema definition. @@ -10,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20110709205814) do +ActiveRecord::Schema.define(:version => 20110924171945) do create_table "authentications", :force => true do |t| t.integer "user_id" @@ -37,8 +38,11 @@ t.integer "logo_file_size" t.datetime "logo_updated_at" t.boolean "delta", :default => true, :null => false + t.string "slug" end + add_index "companies", ["slug"], :name => "index_companies_on_slug", :unique => true + create_table "company_projects", :force => true do |t| t.integer "company_id" t.integer "project_id" @@ -54,6 +58,17 @@ t.datetime "updated_at" end + create_table "friendly_id_slugs", :force => true do |t| + t.string "slug", :null => false + t.integer "sluggable_id", :null => false + t.string "sluggable_type", :limit => 40 + t.datetime "created_at" + end + + add_index "friendly_id_slugs", ["slug", "sluggable_type"], :name => "index_friendly_id_slugs_on_slug_and_sluggable_type", :unique => true + add_index "friendly_id_slugs", ["sluggable_id"], :name => "index_friendly_id_slugs_on_sluggable_id" + add_index "friendly_id_slugs", ["sluggable_type"], :name => "index_friendly_id_slugs_on_sluggable_type" + create_table "group_memberships", :force => true do |t| t.integer "group_id" t.integer "person_id" @@ -84,8 +99,11 @@ t.integer "logo_file_size" t.datetime "logo_updated_at" t.boolean "delta", :default => true, :null => false + t.string "slug" end + add_index "groups", ["slug"], :name => "index_groups_on_slug", :unique => true + create_table "people", :force => true do |t| t.string "email" t.string "twitter" @@ -109,8 +127,11 @@ t.boolean "interested_mentee" t.text "mentor_topics" t.text "mentee_topics" + t.string "slug" end + add_index "people", ["slug"], :name => "index_people_on_slug", :unique => true + create_table "project_memberships", :force => true do |t| t.integer "person_id" t.integer "project_id" @@ -130,8 +151,11 @@ t.integer "logo_file_size" t.datetime "logo_updated_at" t.boolean "delta", :default => true, :null => false + t.string "slug" end + add_index "projects", ["slug"], :name => "index_projects_on_slug", :unique => true + create_table "resource_links", :force => true do |t| t.string "name" t.string "url" diff --git a/spec/acceptance/people_spec.rb b/spec/acceptance/people_spec.rb index 1b6abb4..b61ed22 100644 --- a/spec/acceptance/people_spec.rb +++ b/spec/acceptance/people_spec.rb @@ -279,6 +279,45 @@ def setup_people end end + scenario "show allow editing of slug" do + signed_in_as(:user_with_person) do + person = @user.person + + visit edit_person_path(person) + within "form.person" do + fill_in "person_custom_slug", :with => "Foo Bar" + find("input[name='commit']").click + end + + current_path.should == person_path("foo-bar") + + person.reload + person.slug.should == "foo-bar" + end + end + + scenario "should not allow duplicate slug" do + Factory.create(:person, :name => "Foo Bar", :custom_slug => "foo-bar") + + signed_in_as(:user_with_person) do + person = @user.person + original_slug = person.slug + + visit edit_person_path(person) + within "form.person" do + fill_in "person_custom_slug", :with => "Foo Bar" + find("input[name='commit']").click + end + + page.should have_selector(".record_errors") + page.should have_selector("#person_custom_slug_input .inline-errors") + current_path.should == person_path(original_slug) + + person.reload + person.slug.should_not == "foo-bar" + end + end + scenario "should allow the user to upload a photo" do signed_in_as(:user_with_person) do @person = @user.person diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index 7089244..adf5b7a 100644 --- a/spec/models/person_spec.rb +++ b/spec/models/person_spec.rb @@ -83,4 +83,60 @@ person.should be_valid end end + + describe "when using slugs" do + before :each do + DatabaseCleaner.clean + end + + it "should create a slug when saving a new record" do + person = Factory.build(:person, :name => "Bob Smith") + person.slug.should be_nil + + person.save! + person.slug.should == "bob-smith" + end + + it "should allow the slug to be overriden" do + person = Factory.create(:person) + + person.custom_slug = "Bubba Jack" + person.save! + person.slug == "bubba-jack" + end + + it "should not allow duplicate slugs" do + bob = Factory.create(:person, :name => "Bob Smith") + bubba = Factory.create(:person, :name => "Bubba Jack") + + bubba.custom_slug = "bob-smith" + bob.slug.should == bubba.custom_slug + + bubba.should_not be_valid + bubba.errors[:custom_slug].should be_present + end + + it "should not allow duplicate slugs based on history" do + bob = Factory.create(:person, :name => "Bob Smith", :custom_slug => "bob") + bob.update_attribute(:custom_slug, "bob.smith") + + bubba = Factory.build(:person, :name => "Bubba Jack", :custom_slug => "bob") + bubba.should_not be_valid + bubba.error_on(:custom_slug).first.should =~ /#{I18n.t('activerecord.errors.messages.taken')}/ + end + + it "should not think an non-change update is a conflict" do + bob = Factory.create(:person, :name => "Bob Smith", :custom_slug => "bob") + + bob.custom_slug = "bob" + bob.save! + end + + it "should not allow custom slugs without non-digits" do + person = Factory.build(:person, :custom_slug => "99") + + person.should_not be_valid + person.errors[:custom_slug].should be_present + end + end end