BUGFIX/FEATURE: store topic changes in post revisions

History + edit notifications for title and category changes

app/assets/javascripts/discourse/controllers/history_controller.js

@@ -38,7 +38,45 @@ Discourse.HistoryController = Discourse.ObjectController.extend(Discourse.ModalF
   displayingSideBySide: Em.computed.equal("viewMode", "side_by_side"),
   displayingSideBySideMarkdown: Em.computed.equal("viewMode", "side_by_side_markdown"),
 
-  diff: function() { return this.get(this.get("viewMode")); }.property("inline", "side_by_side", "side_by_side_markdown", "viewMode"),
+  category_diff: function() {
+    var viewMode = this.get("viewMode");
+    var changes = this.get("category_changes");
+
+    var prevCategory = Discourse.Category.findById(changes.previous_category_id);
+    var curCategory = Discourse.Category.findById(changes.current_category_id);
+
+    var raw = "";
+
+    prevCategory = Discourse.HTML.categoryLink(prevCategory);
+    curCategory = Discourse.HTML.categoryLink(curCategory);
+
+    if(viewMode === "side_by_side_markdown" || viewMode === "side_by_side") {
+      raw = "<div class='span8'>" + prevCategory +  "</div> <div class='span8 offset1'>" + curCategory +  "</div>";
+    } else {
+      var diff;
+      if(curCategory === prevCategory){
+        diff = curCategory;
+      } else {
+        diff = "<del>" + prevCategory + "</del> " + "<ins>" + curCategory + "</ins>";
+      }
+      raw = "<div class='inline-diff'>" + diff +  "</div>";
+    }
+
+    return raw;
+
+  }.property("inline", "side_by_side", "side_by_side_markdown", "viewMode"),
+
+  title_diff: function() {
+    var viewMode = this.get("viewMode");
+    if(viewMode === "side_by_side_markdown") {
+      viewMode = "side_by_side";
+    }
+    return this.get("title_changes." + viewMode);
+  }.property("inline", "side_by_side", "side_by_side_markdown", "viewMode"),
+
+  body_diff: function() {
+    return this.get("body_changes." + this.get("viewMode"));
+  }.property("inline", "side_by_side", "side_by_side_markdown", "viewMode"),
 
   actions: {
     loadFirstVersion: function() { this.refresh(this.get("post_id"), 2); },

app/assets/javascripts/discourse/templates/modal/history.js.handlebars

@@ -22,7 +22,15 @@
       {{i18n post.revisions.details.edited_by}} {{avatar this imageSize="small"}} {{username}} <span class="date">{{unboundDate path="created_at" leaveAgo="true"}}</span> {{#if edit_reason}} &mdash; <span class="edit-reason">{{edit_reason}}</span>{{/if}}
     </div>
     <div id="revisions">
-      {{{diff}}}
+      {{#if title_changes}}
+        <h2>{{{title_diff}}}</h2>
+      {{/if}}
+      {{#if category_changes}}
+      <div class="category-diff">
+        {{{category_diff}}}
+      </div>
+      {{/if}}
+      {{{body_diff}}}
     </div>
   {{/if}}
 </div>

app/models/post_revision.rb

@@ -1,8 +1,96 @@
+require_dependency "discourse_diff"
+
 class PostRevision < ActiveRecord::Base
   belongs_to :post
   belongs_to :user
 
   serialize :modifications, Hash
+
+  def body_changes
+    changes_for("cooked", "raw")
+  end
+
+  def category_changes
+    {
+      previous_category_id: previous("category_id"),
+      current_category_id: current("category_id"),
+    }
+  end
+
+  def title_changes
+    changes_for("title", nil, true)
+  end
+
+  def changes_for(name, markdown=nil, wrap=false)
+    prev = previous(name)
+    cur = current(name)
+
+    if wrap
+      prev = "<div>#{CGI::escapeHTML(prev)}</div>"
+      cur = "<div>#{CGI::escapeHTML(cur)}</div>"
+    end
+
+    diff = DiscourseDiff.new(prev, cur)
+
+    result = {
+      inline: diff.inline_html,
+      side_by_side: diff.side_by_side_html
+    }
+
+    if markdown
+      diff = DiscourseDiff.new(previous(markdown), current(markdown))
+      result[:side_by_side_markdown] = diff.side_by_side_markdown
+    end
+
+    result
+  end
+
+  def previous(field)
+    lookup_with_fallback(field, 0)
+  end
+
+  def current(field)
+    lookup_with_fallback(field, 1)
+  end
+
+  def previous_revisions
+    @previous_revs ||=
+      PostRevision.where("post_id = ? AND number < ?",
+                              post_id,        number
+                      )
+                .order("number desc")
+                .to_a
+  end
+
+  def has_topic_data?
+    post && post.post_number == 1
+  end
+
+  def lookup_with_fallback(field, index)
+
+    unless val = lookup(field, index)
+      previous_revisions.each do |v|
+        break if val = v.lookup(field, 1)
+      end
+    end
+
+    unless val
+      if ["cooked","raw"].include?(field)
+        val = post.send(field)
+      else
+        val = post.topic.send(field)
+      end
+    end
+
+    val
+  end
+
+  def lookup(field, index)
+    if mod = modifications[field]
+      mod[index]
+    end
+  end
+
 end
 
 # == Schema Information

app/models/topic.rb

@@ -92,7 +92,6 @@ class Topic < ActiveRecord::Base
   has_many :topic_invites
   has_many :invites, through: :topic_invites, source: :invite
 
-  has_many :topic_revisions
   has_many :revisions, foreign_key: :topic_id, class_name: 'TopicRevision'
 
   # When we want to temporarily attach some data to a forum topic (usually before serialization)
@@ -189,13 +188,20 @@ class Topic < ActiveRecord::Base
 
   end
 
+  # TODO move into PostRevisor or TopicRevisor
   def save_revision
-    TopicRevision.create!(
+    if first_post_id = posts.where(post_number: 1).pluck(:id).first
+
+      number = PostRevision.where(post_id: first_post_id).count + 2
+      PostRevision.create!(
         user_id: acting_user.id,
-      topic_id: id,
+        post_id: first_post_id,
-      number: TopicRevision.where(topic_id: id).count + 2,
+        number: number,
-      modifications: changes.extract!(:category, :title)
+        modifications: changes.extract!(:category_id, :title)
       )
+
+      Post.update_all({version: number}, id: first_post_id)
+    end
   end
 
   def should_create_new_version?

app/models/topic_revision.rb

@@ -1,24 +0,0 @@
-class TopicRevision < ActiveRecord::Base
-  belongs_to :topic
-  belongs_to :user
-
-  serialize :modifications, Hash
-end
-
-# == Schema Information
-#
-# Table name: topic_revisions
-#
-#  id            :integer          not null, primary key
-#  user_id       :integer
-#  topic_id      :integer
-#  modifications :text
-#  number        :integer
-#  created_at    :datetime
-#  updated_at    :datetime
-#
-# Indexes
-#
-#  index_topic_revisions_on_topic_id             (topic_id)
-#  index_topic_revisions_on_topic_id_and_number  (topic_id,number)
-#

app/serializers/post_revision_serializer.rb

@@ -1,5 +1,3 @@
-require_dependency "discourse_diff"
-
 class PostRevisionSerializer < ApplicationSerializer
   attributes :post_id,
              :version,
@@ -9,9 +7,17 @@ class PostRevisionSerializer < ApplicationSerializer
              :avatar_template,
              :created_at,
              :edit_reason,
-             :inline,
+             :body_changes,
-             :side_by_side,
+             :title_changes,
-             :side_by_side_markdown
+             :category_changes
+
+  def include_title_changes?
+    object.has_topic_data?
+  end
+
+  def include_category_changes?
+    object.has_topic_data?
+  end
 
   def version
     object.number
@@ -22,50 +28,24 @@ class PostRevisionSerializer < ApplicationSerializer
   end
 
   def username
-    object.user.username_lower
+    user.username_lower
   end
 
   def display_username
-    object.user.username
+    user.username
   end
 
   def avatar_template
-    object.user.avatar_template
+    user.avatar_template
   end
 
   def edit_reason
-    return unless object.modifications["edit_reason"].present?
+    object.lookup("edit_reason",1)
-    object.modifications["edit_reason"][1]
   end
 
-  def inline
+  def user
-    DiscourseDiff.new(previous_cooked, cooked).inline_html
+    # if stuff goes pear shape attribute to system
-  end
+    object.user || Discourse.system_user
-
-  def side_by_side
-    DiscourseDiff.new(previous_cooked, cooked).side_by_side_html
-  end
-
-  def side_by_side_markdown
-    DiscourseDiff.new(previous_raw, raw).side_by_side_markdown
-  end
-
-  private
-
-  def previous_cooked
-    @previous_cooked ||= object.modifications["cooked"][0]
-  end
-
-  def previous_raw
-    @previous_raw ||= object.modifications["raw"][0]
-  end
-
-  def cooked
-    @cooked ||= object.modifications["cooked"][1]
-  end
-
-  def raw
-    @raw ||= object.modifications["raw"][1]
   end
 
 end

db/migrate/20140306223522_move_topic_revisions_to_post_revisions.rb

@@ -0,0 +1,43 @@
+class MoveTopicRevisionsToPostRevisions < ActiveRecord::Migration
+  def up
+    execute <<SQL
+
+    INSERT INTO post_revisions(user_id, post_id, modifications, number, created_at, updated_at)
+    SELECT tr.user_id, p.id, tr.modifications, tr.number, tr.created_at, tr.updated_at
+    FROM topic_revisions tr
+    JOIN topics t ON t.id = tr.topic_id
+    JOIN posts p ON p.topic_id = t.id AND p.post_number = 1
+
+SQL
+
+   execute <<SQL
+
+   UPDATE post_revisions r SET number = 2 + (
+    SELECT COUNT(*) FROM post_revisions r2
+    WHERE r2.post_id = r.post_id AND r2.created_at < r.created_at
+   )
+
+SQL
+
+    execute <<SQL
+
+    UPDATE posts p SET version = 1 + (
+      SELECT COUNT(*) FROM post_revisions r
+      WHERE r.post_id = p.id
+    )
+
+SQL
+
+    execute <<SQL
+
+    DROP TABLE topic_revisions
+
+SQL
+
+  end
+
+  def down
+    # strictly, we could reverse this, but not implemented
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

spec/models/post_revision_spec.rb

@@ -3,7 +3,63 @@ require_dependency 'post_revision'
 
 describe PostRevision do
 
-  it { should belong_to :user }
+  before do
-  it { should belong_to :post }
+    @number = 1
+  end
+
+  def create_rev(modifications, post_id=1)
+    @number += 1
+    PostRevision.create!(post_id: post_id, user_id: 1, number: @number, modifications: modifications)
+  end
+
+  it "can grab history from current object" do
+    p = PostRevision.new(modifications: {"foo" => ["bar", "bar1"]})
+    p.previous("foo").should == "bar"
+    p.current("foo").should == "bar1"
+  end
+
+  it "can fallback to previous revisions if needed" do
+    create_rev("foo" => ["A", "B"])
+    r2 = create_rev("bar" => ["C", "D"])
+
+    r2.current("foo").should == "B"
+    r2.previous("foo").should == "B"
+  end
+
+  it "can fallback to post if needed" do
+    post = Fabricate(:post)
+    r = create_rev({"foo" => ["A", "B"]}, post.id)
+
+    r.current("raw").should == post.raw
+    r.previous("raw").should == post.raw
+    r.current("cooked").should == post.cooked
+    r.previous("cooked").should == post.cooked
+  end
+
+  it "can fallback to topic if needed" do
+    post = Fabricate(:post)
+    r = create_rev({"foo" => ["A", "B"]}, post.id)
+
+    r.current("title").should == post.topic.title
+    r.previous("title").should == post.topic.title
+  end
+
+  it "can find title changes" do
+    r = create_rev({"title" => ["hello", "frog"]})
+    r.title_changes[:inline].should =~ /frog.*hello/
+    r.title_changes[:side_by_side].should =~ /hello.*frog/
+  end
+
+  it "can find category changes" do
+    cat1 = Fabricate(:category, name: "cat1")
+    cat2 = Fabricate(:category, name: "cat2")
+
+    r = create_rev({"category_id" => [cat1.id, cat2.id]})
+
+    changes = r.category_changes
+    changes[:previous_category_id].should == cat1.id
+    changes[:current_category_id].should == cat2.id
+
+  end
 
 end

spec/models/topic_revision_spec.rb

@@ -1,9 +0,0 @@
-require 'spec_helper'
-require_dependency 'topic_revision'
-
-describe TopicRevision do
-
-  it { should belong_to :user }
-  it { should belong_to :topic }
-
-end

spec/models/topic_spec.rb

@@ -7,23 +7,6 @@ describe Topic do
 
   it { should validate_presence_of :title }
 
-  it { should belong_to :category }
-  it { should belong_to :user }
-  it { should belong_to :last_poster }
-  it { should belong_to :featured_user1 }
-  it { should belong_to :featured_user2 }
-  it { should belong_to :featured_user3 }
-  it { should belong_to :featured_user4 }
-
-  it { should have_many :posts }
-  it { should have_many :topic_users }
-  it { should have_many :topic_links }
-  it { should have_many :topic_allowed_users }
-  it { should have_many :allowed_users }
-  it { should have_many :invites }
-  it { should have_many :topic_revisions }
-  it { should have_many :revisions }
-
   it { should rate_limit }
 
   context 'slug' do
@@ -735,10 +718,11 @@ describe Topic do
   end
 
   describe 'revisions' do
-    let(:topic) { Fabricate(:topic) }
+    let(:post) { Fabricate(:post) }
+    let(:topic) { post.topic }
 
     it "has no revisions by default" do
-      topic.revisions.size.should == 1
+      post.revisions.size.should == 0
     end
 
     context 'changing title' do
@@ -749,7 +733,7 @@ describe Topic do
       end
 
       it "creates a new revision" do
-        topic.revisions.size.should == 2
+        post.revisions.size.should == 1
       end
 
     end
@@ -762,7 +746,7 @@ describe Topic do
       end
 
       it "creates a new revision" do
-        topic.revisions.size.should == 2
+        post.revisions.size.should == 1
       end
 
       context "removing a category" do
@@ -771,7 +755,12 @@ describe Topic do
         end
 
         it "creates a new revision" do
-          topic.revisions.size.should == 3
+          post.revisions.size.should == 2
+          last_rev = post.revisions.order(:number).last
+          last_rev.previous("category_id").should == category.id
+          last_rev.current("category_id").should == SiteSetting.uncategorized_category_id
+          post.reload
+          post.version.should == 3
         end
       end
 
@@ -784,7 +773,7 @@ describe Topic do
       end
 
       it "doesn't create a new version" do
-        topic.revisions.size.should == 1
+        post.revisions.size.should == 0
       end
     end
 
@@ -1210,7 +1199,7 @@ describe Topic do
   describe 'secured' do
     it 'can remove secure groups' do
       category = Fabricate(:category, read_restricted: true)
-      topic = Fabricate(:topic, category: category)
+      Fabricate(:topic, category: category)
 
       Topic.secured(Guardian.new(nil)).count.should == 0
       Topic.secured(Guardian.new(Fabricate(:admin))).count.should == 2