[MIGRATION] adds Author#clean_url for slugs and fixed the sanitizatio…

…n with specs

app/controllers/authors_controller.rb

@@ -1,6 +1,6 @@
 class AuthorsController < ApplicationController
   def show
-    @author = Author.find_by_name(params[:author_slug])
+    @author = Author.find_by_clean_url(params[:author_slug])
     raise ActiveRecord::RecordNotFound if @author.nil?
   end
 end

app/controllers/episodes_controller.rb

@@ -10,9 +10,9 @@ def index
   end
 
   def search
-    @q        = params[:q]
+    @query    = params[:query]
     @podcast  = Podcast.find_by_slug(params[:podcast_slug])
-    @episodes = @podcast.episodes.search(@q, :include => [:podcast]).compact.uniq.sort_by(&:published_at)
+    @episodes = @podcast.episodes.search(@query, :include => [:podcast]).compact.uniq.sort_by(&:published_at)
     render :action => 'index'
   end
 

app/controllers/info/authors_controller.rb

@@ -4,7 +4,7 @@ def index
   end
 
   def show
-    @author = Author.find_by_name(params[:author_slug]) or raise ActiveRecord::RecordNotFound
+    @author = Author.find_by_clean_url(params[:author_slug]) or raise ActiveRecord::RecordNotFound
   end
 end
 

app/controllers/info/podcasts_controller.rb

@@ -1,6 +1,7 @@
 class Info::PodcastsController < InfoController
   def show
     @podcast = Podcast.find_by_slug(params[:podcast_slug])
+    @episodes = @podcast.episodes.sort_by(&:daily_order).sort_by(&:published_at).reverse
   end
 
   def add

app/models/author.rb

@@ -11,7 +11,10 @@
 #
 
 class Author < ActiveRecord::Base
-  before_save :set_name
+  before_validation :set_name_and_clean_url
+
+  validates_presence_of :email
+  validates_uniqueness_of :clean_url
 
   def user
     User.find_by_email(email.strip)
@@ -22,16 +25,18 @@ def podcasts
   end
 
   def to_param
-    name
+    clean_url
   end
 
 
   protected
-  def set_name
-    if !email.blank? && (name.blank? || name_changed?)
-      self.name = email.split(/@/).first
-      self.name.gsub!(/[^a-zA-Z0-9]/, '_')
-      self.name.increment!("_%s", 2) while Author.exists?(["name = ? AND id != ?", name, id.to_i])
-    end
+  def set_name_and_clean_url
+    self.name = email.dup.to_s if name.blank?
+    self.name = name.split(/@/).first
+    self.name = name.increment(" (%s)", 2) while Author.exists?(["name = ? AND id != ?", name, id.to_i])
+
+    self.clean_url = (name.blank? ? 'author' : name.dup).to_s if clean_url.blank?
+    self.clean_url = clean_url.gsub(/[^a-zA-Z0-9_]/, '_').gsub(/_+/, '_')
+    self.clean_url = clean_url.increment("%s", 2) while Author.exists?(["clean_url = ? AND id != ?", clean_url, id.to_i])
   end
 end

app/models/podcast.rb

@@ -97,7 +97,6 @@ class Podcast < ActiveRecord::Base
   before_validation :sanitize_title
   before_validation :sanitize_url
   before_save :store_last_changes
-  before_save :set_author
   after_destroy :update_finder_score
 
   validates_presence_of   :title, :unless => Proc.new { |podcast| podcast.new_record? }
@@ -124,7 +123,12 @@ def self.find_by_slug(slug)
   end
 
   def author
-    Author.find_or_create_by_email(author_email.to_s.strip) unless author_email.blank?
+    return nil if author_email.blank?
+
+    @author ||= Author.find_or_initialize_by_email(author_email.to_s.strip) 
+    @author.name = author_name if @author.name.blank?
+    @author.save
+    @author
   end
 
   def apparent_format
@@ -429,8 +433,4 @@ def store_last_changes
   def update_finder_score
     finder.calculate_score! if finder
   end
-
-  def set_author
-    Author.find_or_create_by_email(author_email.strip) if author_email_changed?
-  end
 end

app/views/info/podcasts/_info_source_table.html.erb

@@ -13,7 +13,7 @@
     <th><%= controller.controller_name == "podcasts" ? content_tag(:strong, podcast.title) : link_to(podcast.title, info_podcast_url(podcast)) -%></th>
     <th><%= link_to "rss", podcast.url %><sup><%= podcast.ability -%></sup></th>
   </tr>
-  <% (episode ? [episode] : podcast.episodes.sort_by(&:daily_order).sort_by(&:published_at).reverse).each do |e| -%>
+  <% (episode ? [episode] : @episodes).each do |e| -%>
     <tr>
       <td class="extra"><%= link_to "site", episode_url(podcast, e) %></td>
       <td>

app/views/info/podcasts/show.html.erb

@@ -2,7 +2,7 @@
 <h1>podcast</h1>
 
 <!-- Map -->
-<%= render :partial => 'info_source_table', :locals => {:options => {:podcast => @podcast}} -%>
+<%= render :partial => 'info_source_table', :locals => {:options => {:podcast => @podcast, :episodes => @episodes}} -%>
 
 <!-- Episodes -->
 <p class="episode_info">
@@ -20,7 +20,7 @@
 </p>
 
 <ul class="episodes">
-  <%= render :partial => 'episodes/info_line', :collection => @podcast.episodes.sorted -%>
+  <%= render :partial => 'episodes/info_line', :collection => @episodes -%>
 </ul>
 
 <!-- Info: -->

db/migrate/20090812161438_add_clean_url_to_author.rb

@@ -0,0 +1,10 @@
+class AddCleanUrlToAuthor < ActiveRecord::Migration
+  def self.up
+    add_column :authors, :clean_url, :string
+    add_index :authors, :clean_url
+  end
+
+  def self.down
+    remove_index :authors, :clean_url
+  end
+end

db/schema.rb

@@ -9,15 +9,17 @@
 #
 # It's strongly recommended to check this file into your version control system.
 
-ActiveRecord::Schema.define(:version => 20090811161932) do
+ActiveRecord::Schema.define(:version => 20090812161438) do
 
   create_table "authors", :force => true do |t|
     t.string   "name"
     t.string   "email"
     t.datetime "created_at"
     t.datetime "updated_at"
+    t.string   "clean_url"
   end
 
+  add_index "authors", ["clean_url"], :name => "index_authors_on_clean_url"
   add_index "authors", ["email"], :name => "index_authors_on_email"
   add_index "authors", ["name"], :name => "index_authors_on_name"
 

lib/podcast_processor.rb

@@ -137,7 +137,7 @@ def update_podcast!
       :generator   => @rpodcast_feed.generator,
       :ability     => ABILITY,
       :xml         => @content,
-      :author_email => @rpodcast_feed.owner_email.to_s.gsub(/\(.*\)/, '').strip,
+      :author_email => (@rpodcast_feed.owner_email).to_s.gsub(/\(.*\)/, '').strip,
       :author_name  => @rpodcast_feed.owner_name,
       :xml_title   => @rpodcast_feed.title.to_s.strip,
       :subtitle    => @rpodcast_feed.subtitle.to_s.strip,

spec/controllers/episodes_controller_spec.rb

@@ -44,7 +44,7 @@ def do_get(podcast)
     end
 
     def do_get(podcast, query='')
-      get :search, :podcast_slug => podcast, :q => query
+      get :search, :podcast_slug => podcast, :query => query
     end
 
     it "should be successful" do

spec/models/author_spec.rb

@@ -0,0 +1,51 @@
+require File.dirname(__FILE__) + '/../spec_helper'
+
+describe Author do
+  describe 'clean_url' do
+    it 'should sanitize and autoset the clean_url from name if clean_url is blank' do
+      Author.create(:name => 'A. Author', :email => '[email protected]').clean_url.should == "A_Author"
+    end
+
+    it 'should sanitize crazy clean_urls' do
+      Author.create(:name => 'A *Author1234_____..!', :email => '[email protected]').clean_url.should == "A_Author1234_"
+    end
+
+    it 'should not autoset the clean_url if clean_url is NOT blank' do
+      a = Author.create(:name => 'A. Author', :email => '[email protected]', :clean_url => 'aauthor')
+      a.clean_url.should == 'aauthor'
+    end
+
+    it 'should autoincrement if the clean_url already exists' do
+      Author.create(:email => '[email protected]', :clean_url => 'an_author').clean_url.should == "an_author"
+      Author.create(:email => '[email protected]', :clean_url => 'an_author').clean_url.should == "an_author2"
+    end
+  end
+
+  describe 'name' do
+    it 'should sanitize the name in case it is an email' do
+      Author.create(:name => '[email protected]').name.should == 'a'
+      Author.create(:email => '[email protected]').name.should == 'a'
+    end
+
+    it 'should be set to first part of email if nil' do
+      a = Author.create(:email => '[email protected]')
+      a.name.should == 'a'
+    end
+
+    it 'should autoincrement if the name already exists' do
+      Author.create(:email => '[email protected]', :name => 'An Author').name.should == "An Author"
+      Author.create(:email => '[email protected]', :name => 'An Author').name.should == "An Author (2)"
+    end
+  end
+
+  describe 'validation' do
+    it 'should not allow an author to be created without an email' do
+      lambda do
+        a = Author.create(:email => '', :name => 'ccc')
+        a.should_not be_valid
+        a.errors.on(:email).should include("can't be blank")
+      end.should_not change(Author, :count)
+    end
+  end
+end
+

spec/models/podcast_spec.rb

@@ -449,26 +449,6 @@
   end
 end
 
-
-describe Podcast, "finding or creating owner" do
-  before do
-    @podcast = Factory.build(:podcast, :title => "FOoooooobar", :author_email => "[email protected]")
-    @save_podcast = lambda { @podcast.save }
-  end
-
-  it "should set and create the author if the author doesn't exist" do
-    @save_podcast.should change { Author.all.size }.by(1)
-    @podcast.author.should == Author.last
-    @podcast.author.name.should == "some_owner"
-  end
-
-  it "should find and set the owner if owner exists" do
-    author = Factory.create(:author, :email => @podcast.author_email)
-    @save_podcast.should_not change { User.all.size }
-    @podcast.author.should == author
-  end
-end
-
 describe Podcast do
   before do
     @podcast = Factory.create(:podcast)
@@ -488,3 +468,32 @@
     @podcast.been_reviewed_by?(nil).should be_false
   end
 end
+
+describe Podcast do
+  before do
+    @podcast = Factory.create(:podcast, :author_email => '[email protected]', :author_name => 'aaa')
+    @podcast2 = Factory.create(:podcast, :author_email => '[email protected]', :author_name => 'bbb')
+  end
+
+  describe 'autocreating an author' do
+    it 'should find or create the author based on email' do
+      lambda { @podcast.author }.should change(Author, :count).by(1)
+      lambda { @podcast.author }.should_not change(Author, :count)
+    end
+
+    it 'should autocreate the author with the same name and email' do
+      @podcast.author.email.should == "[email protected]"
+      @podcast.author.name.should == "aaa"
+    end
+
+    it 'should not create another author if it already exists' do
+      @podcast.author
+      lambda { @podcast2.author }.should_not change(Author, :count)
+    end
+
+    it 'should not overwrite the author name if it was already set' do
+      @podcast.author
+      @podcast2.author.name.should == 'aaa'
+    end
+  end
+end