add id to wiki page urls. fixes #2635.

prepend all wiki page urls with the object id to ensure that,
even if the title/url change, the record can still be retrieved
with the id.

test plan:
1. create a new wiki page;
2. in the body of another wiki page, link to the newly created
   wiki page;
3. change the title of the newly created wiki page;
4. verify that the link to the new wiki page still works (even
   though the title has changed).

Change-Id: I4a6ab2dea3e156abe88d022ae5f17cd7a0fb3924
Reviewed-on: https://gerrit.instructure.com/6792
Tested-by: Hudson <hudson@instructure.com>
Reviewed-by: Zach Pendleton <zachp@instructure.com>

app/models/wiki_page.rb

@@ -31,17 +31,18 @@ class WikiPage < ActiveRecord::Base
   belongs_to :user
   has_many :context_module_tags, :as => :content, :class_name => 'ContentTag', :conditions => ['content_tags.tag_type = ? AND workflow_state != ?', 'context_module', 'deleted'], :include => {:context_module => [:content_tags, :context_module_progressions]}
   has_many :wiki_page_comments, :order => "created_at DESC"
-  acts_as_url :title, :scope => [:wiki_id, :not_deleted], :sync_url => true
+  acts_as_url :title_with_id, :scope => [:wiki_id, :not_deleted], :sync_url => true
   
-  before_save :set_revised_at
+  before_save       :set_revised_at
   before_validation :ensure_unique_title
+  after_create      :resave_url_with_id
+  after_validation  :resave_url_with_id, :if => lambda { |page| page.title_changed? }, :unless => lambda { |page| page.new_record? }
   
   TITLE_LENGTH = WikiPage.columns_hash['title'].limit rescue 255
   
   def ensure_unique_title
-    return if deleted?
+    return if deleted? || !self.wiki
     self.title ||= (self.url || "page").to_cased_title
-    return unless self.wiki
     # TODO i18n (see wiki.rb)
     if self.title == "Front Page" && self.new_record?
       baddies = self.wiki.wiki_pages.not_deleted.find_all_by_title("Front Page").select{|p| p.url != "front-page" }
@@ -56,8 +57,9 @@ class WikiPage < ActiveRecord::Base
         new_title = real_title[0...(TITLE_LENGTH - mod.length)] + mod
         n = n.succ
       end while self.wiki.wiki_pages.not_deleted.find_by_title(new_title)
-      
+
       self.title = new_title
+      title
     end
   end
   
@@ -66,6 +68,12 @@ class WikiPage < ActiveRecord::Base
     base_url = self.send(url_attribute)
     base_url = self.send(self.class.attribute_to_urlify).to_s.to_url if base_url.blank? || !self.only_when_blank
     conditions = [wildcard("#{url_attribute}", base_url, :type => :right)]
+    # This check only needs to happen for pages named "Front Page;" everything
+    # else will be unique because it includes the object ID.
+    unless title.strip.match /^Front Page$/i
+      write_attribute url_attribute, base_url
+      return
+    end
     unless new_record?
       conditions.first << " and id != ?"
       conditions << id
@@ -102,6 +110,13 @@ class WikiPage < ActiveRecord::Base
     end
   end
 
+  # When a record is first created, the ID isn't properly appended to the
+  # URL because the ID doesn't exist. So once a record is created, re-save
+  # it to make sure that the ID is prepended to the URL.
+  def resave_url_with_id
+    id_changed? ? self.update_attribute(:url, title_with_id.to_url) : self.url = title_with_id.to_url
+  end
+
   sanitize_field :body, Instructure::SanitizeField::SANITIZE
   copy_authorized_links(:body) { [self.current_namespace(self.user).context, self.user] }
   
@@ -343,6 +358,12 @@ class WikiPage < ActiveRecord::Base
   def to_param
     url
   end
+
+  def title_with_id
+    # don't make any changes to front page URL b/c app matches against
+    # that name for deleting logic.
+    title == "Front Page" ? title : "#{id} #{title}"
+  end
   
   def last_revision_at
     res = self.revised_at || self.updated_at

lib/imported_html_converter.rb

@@ -47,7 +47,7 @@ class ImportedHtmlConverter
             type_for_url = type
             type = 'context_modules' if type == 'modules'
             if type == 'wiki'
-              if page = context.wiki.wiki_pages.find_by_url(migration_id)
+              if page = context.wiki.wiki_pages.select { |s| s.url.match(/#{migration_id}$/) }[0]
                 node[attr] = URI::escape("#{course_path}/wiki/#{page.url}")
               end
             elsif type == 'attachments'

spec/controllers/wiki_pages_controller_spec.rb

@@ -95,6 +95,16 @@ describe WikiPagesController do
       assigns[:page].title.should eql("Some Page")
     end
     
+    it "should still have old urls work when a page is renamed" do
+      course_with_teacher_logged_in(:active_all => true)
+      wiki = WikiNamespace.default_for_context(@course).wiki
+      page = wiki.wiki_pages.create(:title => 'old page name', :body => 'test content')
+      old_page_url = page.url
+      page.update_attribute(:title, 'new page name')
+      get 'show', :course_id => @course.id, :id => old_page_url
+      assigns[:page].title.should eql 'new page name'
+    end
+
     it "should allow students when allowed" do
       course_with_teacher_logged_in(:active_all => true)
       post 'create', :course_id => @course.id, :wiki_page => {:title => "Some Secret Page"}

spec/lib/cc/importer/canvas_cartridge_converter_spec.rb

@@ -493,7 +493,7 @@ describe "Canvas Cartridge importing" do
     
     page_2 = @copy_to.wiki.wiki_pages.find_by_migration_id(migration_id)
     page_2.title.should == page.title
-    page_2.url.should == page.url
+    page_2.url.should match /\d{1,3}-#{page.title.to_url}/
     page_2.body.should == body_with_link % ([ @copy_to.id, attachment_import.id ] * 4)
   end
 
@@ -517,7 +517,6 @@ describe "Canvas Cartridge importing" do
     to_att.save
     path = to_att.full_display_path.gsub('course files/', '')
     @copy_to.attachment_path_id_lookup = {path => to_att.migration_id}
-    
     body_with_link = %{<p>Watup? <strong>eh?</strong>
       <a href=\"/courses/%s/assignments\">Assignments</a>
       <a href=\"/courses/%s/file_contents/course%%20files/tbe_banner.jpg\">Some file</a>
@@ -543,9 +542,10 @@ describe "Canvas Cartridge importing" do
     WikiPage.import_from_migration(hash, @copy_to)
     
     page_2 = @copy_to.wiki.wiki_pages.find_by_migration_id(migration_id)
+    wiki_assignments_segment = page_2.body.match(/wiki\/\d+-assignments/)
     page_2.title.should == page.title
-    page_2.url.should == page.url
-    page_2.body.should == (body_with_link % [ @copy_to.id, @copy_to.id, @copy_to.id, @copy_to.id, @copy_to.id, mod2.id, @copy_to.id, to_att.id ]).gsub(/png">/, 'png" />')
+    page_2.url.should match /^\d+-#{page.title.to_url}$/
+    page_2.body.should == (body_with_link % [ @copy_to.id, @copy_to.id, @copy_to.id, @copy_to.id, @copy_to.id, mod2.id, @copy_to.id, to_att.id ]).gsub(/png">/, 'png" />').gsub(/wiki\/assignments/, wiki_assignments_segment[0])
   end
   
   it "should import migrate inline external tool URLs in wiki pages" do
@@ -565,7 +565,7 @@ describe "Canvas Cartridge importing" do
     
     page_2 = @copy_to.wiki.wiki_pages.find_by_migration_id(migration_id)
     page_2.title.should == page.title
-    page_2.url.should == page.url
+    page_2.url.should match /^\d+-#{page.title.to_url}$/
     page_2.body.should match(/\/courses\/#{@copy_to.id}\/external_tools\/retrieve/)
   end
   

spec/lib/importer/wiki_spec.rb

@@ -48,9 +48,9 @@ describe "Importing wikis" do
     # The wiki references should resolve to course urls
     WikiPage.count.should == 18
     wiki = WikiPage.find_by_migration_id('res00146')
-    (wiki.body =~ /\/courses\/\d+\/wiki\/course-glossary-a-to-d/).should_not be_nil
-    (wiki.body =~ /\/courses\/\d+\/wiki\/course-glossary-e-f-g-h/).should_not be_nil
-    (wiki.body =~ /\/courses\/\d+\/wiki\/course-glossary-i-j-k-l-m/).should_not be_nil
-    (wiki.body =~ /\/courses\/\d+\/wiki\/course-glossary-n-o-p-q-r/).should_not be_nil
+    (wiki.body =~ /\/courses\/\d+\/wiki\/\d+-course-glossary-a-to-d/).should_not be_nil
+    (wiki.body =~ /\/courses\/\d+\/wiki\/\d+-course-glossary-e-f-g-h/).should_not be_nil
+    (wiki.body =~ /\/courses\/\d+\/wiki\/\d+-course-glossary-i-j-k-l-m/).should_not be_nil
+    (wiki.body =~ /\/courses\/\d+\/wiki\/\d+-course-glossary-n-o-p-q-r/).should_not be_nil
   end
 end

spec/models/wiki_page_spec.rb

@@ -19,8 +19,11 @@
 require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb')
 
 describe WikiPage do
-  it "should send page updated notifications" do
+  before(:each) do
     course_with_teacher(:active_all => true)
+  end
+
+  it "should send page updated notifications" do
     n = Notification.create(:name => "Updated Wiki Page", :category => "TestImmediately")
     NotificationPolicy.create(:notification => n, :communication_channel => @user.communication_channel, :user => @user, :frequency => "immediately")
     p = @course.wiki.wiki_pages.create(:title => "some page")
@@ -37,7 +40,6 @@ describe WikiPage do
   end
 
   it "should validate the title" do
-    course_with_teacher(:active_all => true)
     @course.wiki.wiki_pages.new(:title => "").valid?.should_not be_true
     @course.wiki.wiki_pages.new(:title => "!!!").valid?.should_not be_true
     @course.wiki.wiki_pages.new(:title => "a"*256).valid?.should_not be_true
@@ -45,15 +47,13 @@ describe WikiPage do
   end
 
   it "should make the title/url unique" do
-    course_with_teacher(:active_all => true)
     p1 = @course.wiki.wiki_pages.create(:title => "Asdf")
     p2 = @course.wiki.wiki_pages.create(:title => "Asdf")
     p2.title.should eql('Asdf-2')
-    p2.url.should eql('asdf-2')
+    p2.url.should eql("#{p2.id}-asdf-2")
   end
 
   it "should make the title unique and truncate to proper length" do
-    course_with_teacher(:active_all => true)
     p1 = @course.wiki.wiki_pages.create!(:title => "a" * WikiPage::TITLE_LENGTH)
     p2 = @course.wiki.wiki_pages.create!(:title => p1.title)
     p3 = @course.wiki.wiki_pages.create!(:title => p1.title)
@@ -67,7 +67,6 @@ describe WikiPage do
   end
 
   it "should let you reuse the title/url of a deleted page" do
-    course_with_teacher(:active_all => true)
     p1 = @course.wiki.wiki_pages.create(:title => "Asdf")
     p1.workflow_state = 'deleted'
     p1.save
@@ -75,17 +74,29 @@ describe WikiPage do
     p2 = @course.wiki.wiki_pages.create(:title => "Asdf")
     p2.reload
     p2.title.should eql('Asdf')
-    p2.url.should eql('asdf')
+    p2.url.should eql("#{p2.id}-asdf")
 
     # so long as it's deleted, we don't care about uniqueness of the title/url
     p1.save.should be_true
     p1.title.should eql('Asdf')
-    p1.url.should eql('asdf')
-    
+    p1.url.should eql("#{p1.id}-asdf")
+
     p1.workflow_state = 'active'
     p1.save.should be_true
     p1.title.should eql('Asdf-2')
-    p1.url.should eql('asdf-2')
+    p1.url.should eql("#{p1.id}-asdf-2")
+  end
+
+  it "should prepend the id to its url" do
+    page = @course.wiki.wiki_pages.create(:title => "The Unbearable Lightness of Being")
+    page.url.should eql "#{page.id}-the-unbearable-lightness-of-being"
+  end
+
+  it "should change its url when renamed" do
+    page = @course.wiki.wiki_pages.create(:title => "Blood Meridian")
+    page.title = "Blood Meridian, or the Evening Redness in the West"
+    page.save
+    page.url.should eql "#{page.id}-blood-meridian-or-the-evening-redness-in-the-west"
   end
 
   context "atom" do