diff --git a/app/assets/javascripts/discourse/controllers/composer.js.es6 b/app/assets/javascripts/discourse/controllers/composer.js.es6 index aaa43a740cf..b22a7b739e8 100644 --- a/app/assets/javascripts/discourse/controllers/composer.js.es6 +++ b/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); diff --git a/app/assets/javascripts/discourse/models/composer.js b/app/assets/javascripts/discourse/models/composer.js index 2379584aa52..991f606acab 100644 --- a/app/assets/javascripts/discourse/models/composer.js +++ b/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); }); }); }, diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 2d0b936d012..aef950324f4 100644 --- a/app/controllers/posts_controller.rb +++ b/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 diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 6ca90f6a2a9..b9cdbf92233 100644 --- a/app/controllers/topics_controller.rb +++ b/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 diff --git a/app/models/topic.rb b/app/models/topic.rb index 561d4f36fd4..aebc5f692f0 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -821,6 +821,7 @@ class Topic < ActiveRecord::Base end + # == Schema Information # # Table name: topics diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb index f0d73d93f24..594162853fd 100644 --- a/lib/post_revisor.rb +++ b/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 diff --git a/spec/components/post_revisor_spec.rb b/spec/components/post_revisor_spec.rb index c6082f08067..fde755e4531 100644 --- a/spec/components/post_revisor_spec.rb +++ b/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 }