From c78dcde97349ceba2b8e36e87c8c8b3424ac13b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Wed, 14 Nov 2018 17:47:59 +0100 Subject: [PATCH] FIX: only send originalText when we need to --- .../discourse/models/composer.js.es6 | 21 +++++---- .../javascripts/discourse/models/draft.js.es6 | 15 ++----- app/controllers/draft_controller.rb | 12 ++--- app/models/draft.rb | 44 ++++++++++--------- spec/requests/draft_controller_spec.rb | 30 ++++++++----- 5 files changed, 65 insertions(+), 57 deletions(-) diff --git a/app/assets/javascripts/discourse/models/composer.js.es6 b/app/assets/javascripts/discourse/models/composer.js.es6 index 2ebbd8b1a67..1e4d822911d 100644 --- a/app/assets/javascripts/discourse/models/composer.js.es6 +++ b/app/assets/javascripts/discourse/models/composer.js.es6 @@ -975,9 +975,18 @@ const Composer = RestModel.extend({ if (this.get("replyLength") < this.siteSettings.min_post_length) return; } + this.setProperties({ + draftStatus: I18n.t("composer.saving_draft_tip"), + draftConflictUser: null + }); + + if (this._clearingStatus) { + Em.run.cancel(this._clearingStatus); + this._clearingStatus = null; + } + const data = { reply: this.get("reply"), - originalText: this.get("originalText"), action: this.get("action"), title: this.get("title"), categoryId: this.get("categoryId"), @@ -992,14 +1001,8 @@ const Composer = RestModel.extend({ noBump: this.get("noBump") }; - this.setProperties({ - draftStatus: I18n.t("composer.saving_draft_tip"), - draftConflictUser: null - }); - - if (this._clearingStatus) { - Em.run.cancel(this._clearingStatus); - this._clearingStatus = null; + if (this.get("post.id") && !Ember.isEmpty(this.get("originalText"))) { + data["originalText"] = this.get("originalText"); } return Draft.save(this.get("draftKey"), this.get("draftSequence"), data) diff --git a/app/assets/javascripts/discourse/models/draft.js.es6 b/app/assets/javascripts/discourse/models/draft.js.es6 index 2f92256ca66..17dc9857925 100644 --- a/app/assets/javascripts/discourse/models/draft.js.es6 +++ b/app/assets/javascripts/discourse/models/draft.js.es6 @@ -5,10 +5,7 @@ Draft.reopenClass({ clear(key, sequence) { return ajax("/draft.json", { type: "DELETE", - data: { - draft_key: key, - sequence: sequence - } + data: { draft_key: key, sequence } }); }, @@ -25,16 +22,10 @@ Draft.reopenClass({ }, save(key, sequence, data) { - const dataJson = typeof data === "string" ? dataJson : JSON.stringify(data); + data = typeof(data) === "string" ? data : JSON.stringify(data); return ajax("/draft.json", { type: "POST", - data: { - draft_key: key, - data: dataJson, - sequence, - post_id: data.postId, - original_text: data.originalText - } + data: { draft_key: key, sequence, data } }); } }); diff --git a/app/controllers/draft_controller.rb b/app/controllers/draft_controller.rb index 9491c833e2a..34b55c7ad82 100644 --- a/app/controllers/draft_controller.rb +++ b/app/controllers/draft_controller.rb @@ -11,11 +11,13 @@ class DraftController < ApplicationController def update Draft.set(current_user, params[:draft_key], params[:sequence].to_i, params[:data]) - if params[:post_id] && params[:original_text] - post = Post.find_by(id: params[:post_id]) - if post && post.raw != params[:original_text] - conflict_user = BasicUserSerializer.new(post.last_editor, root: false) - return render json: success_json.merge(conflict_user: conflict_user) + if data = JSON::parse(params[:data]) + if data["postId"].present? && data["originalText"].present? + post = Post.find_by(id: data["postId"]) + if post && post.raw != data["originalText"] + conflict_user = BasicUserSerializer.new(post.last_editor, root: false) + return render json: success_json.merge(conflict_user: conflict_user) + end end end diff --git a/app/models/draft.rb b/app/models/draft.rb index b9521abe1a2..9905cbe1c57 100644 --- a/app/models/draft.rb +++ b/app/models/draft.rb @@ -1,19 +1,21 @@ # frozen_string_literal: true class Draft < ActiveRecord::Base - NEW_TOPIC = 'new_topic' - NEW_PRIVATE_MESSAGE = 'new_private_message' - EXISTING_TOPIC = 'topic_' + NEW_TOPIC ||= 'new_topic' + NEW_PRIVATE_MESSAGE ||= 'new_private_message' + EXISTING_TOPIC ||= 'topic_' def self.set(user, key, sequence, data) - d = find_draft(user, key) - if d + if d = find_draft(user, key) return if d.sequence > sequence - DB.exec("UPDATE drafts - SET data = :data, - sequence = :sequence, - revisions = revisions + 1 - WHERE id = :id", id: d.id, sequence: sequence, data: data) + + DB.exec(<<~SQL, id: d.id, sequence: sequence, data: data) + UPDATE drafts + SET sequence = :sequence + , data = :data + , revisions = revisions + 1 + WHERE id = :id + SQL else Draft.create!(user_id: user.id, draft_key: key, data: data, sequence: sequence) end @@ -23,16 +25,12 @@ class Draft < ActiveRecord::Base def self.get(user, key, sequence) d = find_draft(user, key) - if d && d.sequence == sequence - d.data - end + d.data if d && d.sequence == sequence end def self.clear(user, key, sequence) d = find_draft(user, key) - if d && d.sequence <= sequence - d.destroy - end + d.destroy if d && d.sequence <= sequence end def self.find_draft(user, key) @@ -83,11 +81,15 @@ class Draft < ActiveRecord::Base end def self.cleanup! - DB.exec("DELETE FROM drafts where sequence < ( - SELECT max(s.sequence) from draft_sequences s - WHERE s.draft_key = drafts.draft_key AND - s.user_id = drafts.user_id - )") + DB.exec(<<~SQL) + DELETE FROM drafts + WHERE sequence < ( + SELECT MAX(s.sequence) + FROM draft_sequences s + WHERE s.draft_key = drafts.draft_key + AND s.user_id = drafts.user_id + ) + SQL # remove old drafts delete_drafts_older_than_n_days = SiteSetting.delete_drafts_older_than_n_days.days.ago diff --git a/spec/requests/draft_controller_spec.rb b/spec/requests/draft_controller_spec.rb index 0c66de5ee46..b00e7be0383 100644 --- a/spec/requests/draft_controller_spec.rb +++ b/spec/requests/draft_controller_spec.rb @@ -8,9 +8,15 @@ describe DraftController do it 'saves a draft on update' do user = sign_in(Fabricate(:user)) - post "/draft.json", params: { draft_key: 'xyz', data: 'my data', sequence: 0 } + + post "/draft.json", params: { + draft_key: 'xyz', + data: { my: "data" }.to_json, + sequence: 0 + } + expect(response.status).to eq(200) - expect(Draft.get(user, 'xyz', 0)).to eq('my data') + expect(Draft.get(user, 'xyz', 0)).to eq(%q({"my":"data"})) end it 'checks for an conflict on update' do @@ -21,9 +27,10 @@ describe DraftController do username: user.username, draft_key: "topic", sequence: 0, - data: "{}", - post_id: post.id, - original_text: post.raw + data: { + postId: post.id, + originalText: post.raw + }.to_json } expect(JSON.parse(response.body)['conflict_user']).to eq(nil) @@ -32,13 +39,16 @@ describe DraftController do username: user.username, draft_key: "topic", sequence: 0, - data: "{}", - post_id: post.id, - original_text: "something else" + data: { + postId: post.id, + originalText: "something else" + }.to_json } - expect(JSON.parse(response.body)['conflict_user']['id']).to eq(post.last_editor.id) - expect(JSON.parse(response.body)['conflict_user']).to include('avatar_template') + json = JSON.parse(response.body) + + expect(json['conflict_user']['id']).to eq(post.last_editor.id) + expect(json['conflict_user']).to include('avatar_template') end it 'destroys drafts when required' do