Fixes permalink not being correctly updated when updating a permalink. …

…closes publify#62

The old behavior was rather buggy as it created a new redirect everytime an article or page permalink was updated. This shortened URL was never displayed because we only called the first redirect instead of the last one.

This commit:

* Fixes existing old shortened URL showing when multiple shorten URLS exist
* Fixes this behavior by updating the existing shortened URL when a published article or page permalink is updated
* Moves the shortened URL creation from controller to model
* Removes duplicate code between admin/content_controller and admin/page_controller
* Adds specs as this part was never tested before
* Tells a story about a young farm boy, a princess called Buttercup (silly name if you want my opinion), true love and high adventure, pirates, princess, giants, miracles, fencing, and rodents of unusual size.

app/controllers/admin/content_controller.rb

@@ -92,7 +92,7 @@ def autosave
     @article.text_filter = current_user.text_filter if current_user.simple_editor?
 
     get_fresh_or_existing_draft_for_article
-
+    
     @article.attributes = params[:article]
     @article.published = false
     set_article_author
@@ -171,7 +171,6 @@ def new_or_edit
       if @article.save
         destroy_the_draft unless @article.draft
         set_article_categories
-        set_shortened_url if @article.published
         set_the_flash
         redirect_to :action => 'index'
         return
@@ -229,19 +228,6 @@ def set_article_categories
     end
   end
 
-  def set_shortened_url
-    # In a very short time, I'd like to have permalink modification generate a 301 redirect as well to
-    # So I set this up the big way now
-
-    return unless Redirect.find_by_to_path(@article.permalink_url).nil?
-
-    red = Redirect.new
-    red.from_path = red.shorten
-    red.to_path = @article.permalink_url
-    red.save
-    @article.redirects << red
-  end
-
   def def_build_body
     if @article.body =~ /<!--more-->/
       body = @article.body.split('<!--more-->')

app/controllers/admin/pages_controller.rb

@@ -19,7 +19,6 @@ def new
     if request.post?
       @page.published_at = Time.now
       if @page.save
-        set_shortened_url if @page.published
         flash[:notice] = _('Page was successfully created.')
         redirect_to :action => 'index'
       end
@@ -32,7 +31,6 @@ def edit
     @page = Page.find(params[:id])
     @page.attributes = params[:page]
     if request.post? and @page.save
-      set_shortened_url if @page.published
       flash[:notice] = _('Page was successfully updated.')
       redirect_to :action => 'index'
     end
@@ -46,19 +44,6 @@ def destroy
     redirect_to :action => 'index'    
   end
 
-  def set_shortened_url
-    # In a very short time, I'd like to have permalink modification generate a 301 redirect as well to
-    # So I set this up the big way now
-
-    return unless Redirect.find_by_to_path(@page.permalink_url).nil?
-
-    red = Redirect.new
-    red.from_path = red.shorten
-    red.to_path = @page.permalink_url
-    red.save
-    @page.redirects << red
-  end
-
   # TODO Duplicate with Admin::ContentController
   def insert_editor
     editor = 'visual'

app/models/article.rb

@@ -48,8 +48,7 @@ def spam
   before_create :set_defaults, :create_guid
   after_create :add_notifications
   before_save :set_published_at, :ensure_settings_type, :set_permalink
-  after_save :post_trigger
-  after_save :keywords_to_tags
+  after_save :post_trigger, :keywords_to_tags, :shorten_url
 
   scope :category, lambda {|category_id| {:conditions => ['categorizations.category_id = ?', category_id], :include => 'categorizations'}}
   scope :drafts, lambda { { :conditions => { :state => 'draft' }, :order => 'created_at DESC' } }
@@ -285,6 +284,7 @@ def self.get_or_build_article id = nil
       art.allow_comments = art.blog.default_allow_comments
       art.allow_pings = art.blog.default_allow_pings
       art.text_filter = art.blog.text_filter
+      art.old_permalink = art.permalink_url unless art.permalink.nil? or art.permalink.empty?
       art.published = true
     end
   end

app/models/content.rb

@@ -69,6 +69,24 @@ def invalidates_cache?(on_destruction = false)
       (changed? && published?) || just_changed_published_status?
     end
   end
+
+  def shorten_url
+    return unless self.published
+
+    r = Redirect.new
+    r.from_path = r.shorten
+    r.to_path = self.permalink_url
+
+    # This because updating self.redirects.first raises ActiveRecord::ReadOnlyRecord
+    unless (red = self.redirects.first).nil?
+      return if red.to_path == self.permalink_url
+      r.from_path = red.from_path
+      red.destroy
+      self.redirects.clear # not sure we need this one
+    end
+
+    self.redirects << r
+  end
 
   class << self
     def content_fields *attribs
@@ -262,7 +280,7 @@ def normalized_permalink_url
   def short_url
     # Double check because of crappy data in my own old database
     return unless self.published and self.redirects.count > 0
-    blog.url_for(redirects.first.from_path, :only_path => false)
+    blog.url_for(redirects.last.from_path, :only_path => false)
   end
 
 end

app/models/page.rb

@@ -9,6 +9,7 @@ class Page < Content
   setting :password, :string, ''
 
   before_save :set_permalink
+  after_save :shorten_url
 
   def set_permalink
     self.name = self.title.to_permalink if self.name.blank?

spec/models/article_spec.rb

@@ -184,6 +184,32 @@ def assert_results_are(*expected)
   ### XXX: Should we have a test here?
   it "test_send_multiple_pings" do
   end
+
+  describe "Testing redirects" do
+    it "a new published article gets a redirect" do
+      a = Article.create(:title => "Some title", :body => "some text", :published => true)
+      a.redirects.first.should_not be_nil
+      a.redirects.first.to_path.should == a.permalink_url
+    end
+
+    it "a new unpublished article should not get a redirect" do 
+      a = Article.create(:title => "Some title", :body => "some text", :published => false)
+      a.redirects.first.should be_nil
+    end
+
+    it "Changin a published article permalink url should only change the to redirection" do
+      a = Article.create(:title => "Some title", :body => "some text", :published => true)
+      a.redirects.first.should_not be_nil
+      a.redirects.first.to_path.should == a.permalink_url
+      r  = a.redirects.first.from_path
+
+      a.permalink = "some-new-permalink"
+      a.save
+      a.redirects.first.should_not be_nil
+      a.redirects.first.to_path.should == a.permalink_url
+      a.redirects.first.from_path.should == r
+    end
+  end
 
   describe "with tags" do
     it "recieves tags from the keywords property" do

spec/models/page_spec.rb

@@ -9,6 +9,34 @@
   end
 end
 
+describe "Testing redirects" do
+  it "a new published page gets a redirect" do
+    a = Page.create(:title => "Some title", :body => "some text", :published => true)
+    a.should be_valid
+    a.redirects.first.should_not be_nil
+    a.redirects.first.to_path.should == a.permalink_url
+  end
+
+  it "a new unpublished page should not get a redirect" do 
+    a = Page.create(:title => "Another title", :body => "some text", :published => false)
+    a.redirects.first.should be_nil
+  end
+
+  it "Changin a published article permalink url should only change the to redirection" do
+    a = Page.create(:title => "Third title", :body => "some text", :published => true)
+    a.should be_valid
+    a.redirects.first.should_not be_nil
+    a.redirects.first.to_path.should == a.permalink_url
+    r  = a.redirects.first.from_path
+
+    a.name = "some-new-permalink"
+    a.save
+    a.redirects.first.should_not be_nil
+    a.redirects.first.to_path.should == a.permalink_url
+    a.redirects.first.from_path.should == r
+  end
+end
+
 describe 'Given the fixture :first_page' do
   before(:each) do
     Factory(:blog)