Extensibility for tracking changes to a topic

app/assets/javascripts/discourse/controllers/composer.js.es6

@@ -212,7 +212,9 @@ export default DiscourseController.extend({
       }
 
       if ((!composer.get('replyingToTopic')) || (!Discourse.User.currentProp('disable_jump_reply'))) {
-        Discourse.URL.routeTo(opts.post.get('url'));
+        if (opts.post) {
+          Discourse.URL.routeTo(opts.post.get('url'));
+        }
       }
     }, function(error) {
       composer.set('disableDrafts', false);

app/assets/javascripts/discourse/models/composer.js

@@ -467,14 +467,16 @@ Discourse.Composer = Discourse.Model.extend({
   editPost: function(opts) {
     var post = this.get('post'),
         oldCooked = post.get('cooked'),
-        composer = this;
+        self = this,
+        promise;
 
-    // Update the title if we've changed it
+    // Update the title if we've changed it, otherwise consider it a
+    // successful resolved promise
     if (this.get('title') && post.get('post_number') === 1) {
-
       var topicProps = this.getProperties(Object.keys(_edit_topic_serializer));
-
-      Discourse.Topic.update(this.get('topic'), topicProps);
+      promise = Discourse.Topic.update(this.get('topic'), topicProps);
+    } else {
+      promise = Ember.RSVP.resolve();
     }
 
     post.setProperties({
@@ -485,19 +487,19 @@ Discourse.Composer = Discourse.Model.extend({
     });
     this.set('composeState', CLOSED);
 
-    return new Ember.RSVP.Promise(function(resolve, reject) {
-      post.save(function(result) {
+    return promise.then(function() {
+      return post.save(function(result) {
         post.updateFromPost(result);
-        composer.clearState();
-      }, function(error) {
+        self.clearState();
+      }).catch(function(error) {
         var response = $.parseJSON(error.responseText);
         if (response && response.errors) {
-          reject(response.errors[0]);
+          return(response.errors[0]);
         } else {
-          reject(I18n.t('generic_error'));
+          return(I18n.t('generic_error'));
         }
         post.set('cooked', oldCooked);
-        composer.set('composeState', OPEN);
+        self.set('composeState', OPEN);
       });
     });
   },

app/controllers/posts_controller.rb

@@ -85,7 +85,7 @@ class PostsController < ApplicationController
       [false, MultiJson.dump(errors: post_creator.errors.full_messages)]
 
     else
-      DiscourseEvent.trigger(:topic_saved, post.topic, params, current_user)
+      DiscourseEvent.trigger(:topic_created, post.topic, params, current_user)
       post_serializer = PostSerializer.new(post, scope: guardian, root: false)
       post_serializer.draft_sequence = DraftSequence.current(current_user, post.topic.draft_key)
       [true, MultiJson.dump(post_serializer)]
@@ -382,7 +382,6 @@ class PostsController < ApplicationController
     permitted = [
       :raw,
       :topic_id,
-      :title,
       :archetype,
       :category,
       :target_usernames,
@@ -415,8 +414,10 @@ class PostsController < ApplicationController
       result[:is_warning] = (params[:is_warning] == "true")
     end
 
-    # Enable plugins to whitelist additional parameters they might need
-    DiscourseEvent.trigger(:permit_post_params, result, params)
+    PostRevisor.tracked_fields.keys.each do |f|
+      params.permit(f => [])
+      result[f] = params[f] if params.has_key?(f)
+    end
 
     result
   end

app/controllers/topics_controller.rb

@@ -126,18 +126,19 @@ class TopicsController < ApplicationController
     guardian.ensure_can_edit!(topic)
 
     changes = {}
-    changes[:title]       = params[:title]       if params[:title] && topic.title != params[:title]
-    changes[:category_id] = params[:category_id] if params[:category_id] && topic.category_id != params[:category_id].to_i
+    PostRevisor.tracked_fields.keys.each do |f|
+      changes[f] = params[f] if params.has_key?(f)
+    end
+
+    changes.delete(:title) if topic.title == changes[:title]
+    changes.delete(:category_id) if topic.category_id == changes[:category_id].to_i
 
     success = true
-
     if changes.length > 0
       first_post = topic.ordered_posts.first
       success = PostRevisor.new(first_post, topic).revise!(current_user, changes, validate_post: false)
     end
 
-    DiscourseEvent.trigger(:topic_saved, topic, params, current_user)
-
     # this is used to return the title to the client as it may have been changed by "TextCleaner"
     success ? render_serialized(topic, BasicTopicSerializer) : render_json_error(topic)
   end

app/models/topic.rb

@@ -821,6 +821,7 @@ class Topic < ActiveRecord::Base
 
 end
 
+
 # == Schema Information
 #
 # Table name: topics

lib/post_revisor.rb

@@ -2,8 +2,43 @@ require "edit_rate_limiter"
 
 class PostRevisor
 
+  # Helps us track changes to a topic.
+  #
+  # It's passed to `Topic.track_field` callbacks so they can record if they
+  # changed a value or not. This is needed for things like custom fields.
+  class TopicChanges
+    attr_reader :topic, :user
+
+    def initialize(topic, user)
+      @topic = topic
+      @user = user
+      @changed = {}
+      @errored = false
+    end
+
+    def errored?
+      @errored
+    end
+
+    def guardian
+      @guardian ||= Guardian.new(@user)
+    end
+
+    def record_change(field_name, previous_val, new_val)
+      return if previous_val == new_val
+      diff[field_name] = [previous_val, new_val]
+    end
+
+    def check_result(res)
+      @errored = true if !res
+    end
+
+    def diff
+      @diff ||= {}
+    end
+  end
+
   POST_TRACKED_FIELDS = %w{raw cooked edit_reason user_id wiki post_type}
-  TOPIC_TRACKED_FIELDS = %w{title category_id}
 
   attr_reader :category_changed
 
@@ -12,6 +47,15 @@ class PostRevisor
     @topic = topic || post.topic
   end
 
+  def self.tracked_fields
+    @@tracked_fields ||= {}
+    @@tracked_fields
+  end
+
+  def self.track_field(field, &block)
+    tracked_fields[field] = block
+  end
+
   # AVAILABLE OPTIONS:
   # - revised_at: changes the date of the revision
   # - force_new_version: bypass ninja-edit window
@@ -23,6 +67,8 @@ class PostRevisor
     @fields = fields.with_indifferent_access
     @opts = opts
 
+    @topic_changes = TopicChanges.new(@topic, editor)
+
     # some normalization
     @fields[:raw] = cleanup_whitespaces(@fields[:raw]) if @fields.has_key?(:raw)
     @fields[:user_id] = @fields[:user_id].to_i if @fields.has_key?(:user_id)
@@ -40,7 +86,6 @@ class PostRevisor
 
     @version_changed = false
     @post_successfully_saved = true
-    @topic_successfully_saved = true
 
     @validate_post = true
     @validate_post = @opts[:validate_post] if @opts.has_key?(:validate_post)
@@ -94,10 +139,7 @@ class PostRevisor
   end
 
   def topic_changed?
-    TOPIC_TRACKED_FIELDS.each do |field|
-      return true if @fields.has_key?(field) && @fields[field] != @topic.send(field)
-    end
-    false
+    PostRevisor.tracked_fields.keys.any? {|f| @fields.has_key?(f)}
   end
 
   def revise_post
@@ -174,10 +216,16 @@ class PostRevisor
   end
 
   def update_topic
-    @topic.title = @fields[:title] if @fields.has_key?(:title)
     Topic.transaction do
-      @topic_successfully_saved = @topic.change_category_to_id(@fields[:category_id]) if @fields.has_key?(:category_id)
-      @topic_successfully_saved &&= @topic.save(validate: @validate_topic)
+      PostRevisor.tracked_fields.each do |f, cb|
+        if !@topic_changes.errored? && @fields.has_key?(f)
+          cb.call(@topic_changes, @fields[f])
+        end
+      end
+
+      unless @topic_changes.errored?
+        @topic_changes.check_result(@topic.save(validate: @validate_topic))
+      end
     end
   end
 
@@ -188,7 +236,7 @@ class PostRevisor
   end
 
   def create_revision
-    modifications = post_changes.merge(topic_changes)
+    modifications = post_changes.merge(@topic_changes.diff)
     PostRevision.create!(
       user_id: @post.last_editor_id,
       post_id: @post.id,
@@ -200,7 +248,7 @@ class PostRevisor
   def update_revision
     return unless revision = PostRevision.find_by(post_id: @post.id, number: @post.version)
     revision.user_id = @post.last_editor_id
-    modifications = post_changes.merge(topic_changes)
+    modifications = post_changes.merge(@topic_changes.diff)
     modifications.keys.each do |field|
       if revision.modifications.has_key?(field)
         old_value = revision.modifications[field][0]
@@ -210,17 +258,13 @@ class PostRevisor
         revision.modifications[field] = modifications[field]
       end
     end
-    revision.save
+    revision.save if modifications.length
   end
 
   def post_changes
     @post.previous_changes.slice(*POST_TRACKED_FIELDS)
   end
 
-  def topic_changes
-    @topic.previous_changes.slice(*TOPIC_TRACKED_FIELDS)
-  end
-
   def perform_edit
     return if bypass_rate_limiter?
     EditRateLimiter.new(@editor).performed!
@@ -311,7 +355,18 @@ class PostRevisor
   end
 
   def successfully_saved_post_and_topic
-    @post_successfully_saved && @topic_successfully_saved
+    @post_successfully_saved && !@topic_changes.errored?
   end
 
 end
+
+# Fields we want to record revisions for by default
+PostRevisor.track_field(:title) do |tc, title|
+  tc.record_change('title', tc.topic.title, title)
+  tc.topic.title = title
+end
+
+PostRevisor.track_field(:category_id) do |tc, category_id|
+  tc.record_change('category_id', tc.topic.category_id, category_id)
+  tc.check_result(tc.topic.change_category_to_id(category_id))
+end

spec/components/post_revisor_spec.rb

@@ -7,6 +7,38 @@ describe PostRevisor do
   let(:newuser) { Fabricate(:newuser) }
   let(:post_args) { {user: newuser, topic: topic} }
 
+  context 'TopicChanges' do
+    let(:topic) { Fabricate(:topic) }
+    let(:tc) {
+      topic.reload
+      PostRevisor::TopicChanges.new(topic, topic.user)
+    }
+
+    it 'provides a guardian' do
+      tc.guardian.should be_an_instance_of Guardian
+    end
+
+    it 'tracks changes properly' do
+      tc.diff.should == {}
+
+      # it remembers changes we tell it to
+      tc.record_change('height', '180cm', '170cm')
+      tc.diff['height'].should == ['180cm', '170cm']
+
+      # it works with arrays of values
+      tc.record_change('colors', nil, ['red', 'blue'])
+      tc.diff['colors'].should == [nil, ['red', 'blue']]
+
+      # it does not record changes to the same val
+      tc.record_change('wat', 'js', 'js')
+      tc.diff['wat'].should be_nil
+
+      tc.record_change('tags', ['a', 'b'], ['a', 'b'])
+      tc.diff['tags'].should be_nil
+
+    end
+  end
+
   context 'revise' do
     let(:post) { Fabricate(:post, post_args) }
     let(:first_version_at) { post.last_version_at }