From b19400726fd41d0853d2da024fc5465ae21d29c9 Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 7 Mar 2014 18:59:47 +1100 Subject: [PATCH] BUGFIX/FEATURE: store topic changes in post revisions History + edit notifications for title and category changes --- .../controllers/history_controller.js | 40 ++++++++- .../templates/modal/history.js.handlebars | 10 ++- app/models/post_revision.rb | 88 +++++++++++++++++++ app/models/topic.rb | 20 +++-- app/models/topic_revision.rb | 24 ----- app/serializers/post_revision_serializer.rb | 56 ++++-------- ..._move_topic_revisions_to_post_revisions.rb | 43 +++++++++ spec/models/post_revision_spec.rb | 60 ++++++++++++- spec/models/topic_revision_spec.rb | 9 -- spec/models/topic_spec.rb | 37 +++----- 10 files changed, 281 insertions(+), 106 deletions(-) delete mode 100644 app/models/topic_revision.rb create mode 100644 db/migrate/20140306223522_move_topic_revisions_to_post_revisions.rb delete mode 100644 spec/models/topic_revision_spec.rb diff --git a/app/assets/javascripts/discourse/controllers/history_controller.js b/app/assets/javascripts/discourse/controllers/history_controller.js index a62bd62b9d7..62481df8c71 100644 --- a/app/assets/javascripts/discourse/controllers/history_controller.js +++ b/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 = "
" + prevCategory + "
" + curCategory + "
"; + } else { + var diff; + if(curCategory === prevCategory){ + diff = curCategory; + } else { + diff = "" + prevCategory + " " + "" + curCategory + ""; + } + raw = "
" + diff + "
"; + } + + 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); }, diff --git a/app/assets/javascripts/discourse/templates/modal/history.js.handlebars b/app/assets/javascripts/discourse/templates/modal/history.js.handlebars index a4bb40bce82..e092e9a8a08 100644 --- a/app/assets/javascripts/discourse/templates/modal/history.js.handlebars +++ b/app/assets/javascripts/discourse/templates/modal/history.js.handlebars @@ -22,7 +22,15 @@ {{i18n post.revisions.details.edited_by}} {{avatar this imageSize="small"}} {{username}} {{unboundDate path="created_at" leaveAgo="true"}} {{#if edit_reason}} — {{edit_reason}}{{/if}}
- {{{diff}}} + {{#if title_changes}} +

{{{title_diff}}}

+ {{/if}} + {{#if category_changes}} +
+ {{{category_diff}}} +
+ {{/if}} + {{{body_diff}}}
{{/if}} diff --git a/app/models/post_revision.rb b/app/models/post_revision.rb index fd46c90e7ca..d3b74a37400 100644 --- a/app/models/post_revision.rb +++ b/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 = "
#{CGI::escapeHTML(prev)}
" + cur = "
#{CGI::escapeHTML(cur)}
" + 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 diff --git a/app/models/topic.rb b/app/models/topic.rb index d3bfe1136ce..34dcd725c9a 100644 --- a/app/models/topic.rb +++ b/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!( - user_id: acting_user.id, - topic_id: id, - number: TopicRevision.where(topic_id: id).count + 2, - modifications: changes.extract!(:category, :title) - ) + 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, + post_id: first_post_id, + number: number, + modifications: changes.extract!(:category_id, :title) + ) + + Post.update_all({version: number}, id: first_post_id) + end end def should_create_new_version? diff --git a/app/models/topic_revision.rb b/app/models/topic_revision.rb deleted file mode 100644 index 5190881e44d..00000000000 --- a/app/models/topic_revision.rb +++ /dev/null @@ -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) -# diff --git a/app/serializers/post_revision_serializer.rb b/app/serializers/post_revision_serializer.rb index fc3ccef9733..8d167ad813e 100644 --- a/app/serializers/post_revision_serializer.rb +++ b/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, - :side_by_side, - :side_by_side_markdown + :body_changes, + :title_changes, + :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.modifications["edit_reason"][1] + object.lookup("edit_reason",1) end - def inline - DiscourseDiff.new(previous_cooked, cooked).inline_html - end - - 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] + def user + # if stuff goes pear shape attribute to system + object.user || Discourse.system_user end end diff --git a/db/migrate/20140306223522_move_topic_revisions_to_post_revisions.rb b/db/migrate/20140306223522_move_topic_revisions_to_post_revisions.rb new file mode 100644 index 00000000000..2f5cd70b08c --- /dev/null +++ b/db/migrate/20140306223522_move_topic_revisions_to_post_revisions.rb @@ -0,0 +1,43 @@ +class MoveTopicRevisionsToPostRevisions < ActiveRecord::Migration + def up + execute < ["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 diff --git a/spec/models/topic_revision_spec.rb b/spec/models/topic_revision_spec.rb deleted file mode 100644 index f1717ea9ee2..00000000000 --- a/spec/models/topic_revision_spec.rb +++ /dev/null @@ -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 diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 1595b993b99..6d5bbf8f018 100644 --- a/spec/models/topic_spec.rb +++ b/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