From 185983b445375614ce817b5d6745e096fa8baa51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Fri, 15 Nov 2024 23:38:06 +0100 Subject: [PATCH] FIX: better edit conflict handling Internal ref - t/141218 --- .../discourse/app/models/composer.js | 60 ++++++++++--------- app/controllers/drafts_controller.rb | 30 ++++++---- app/controllers/posts_controller.rb | 5 +- app/controllers/topics_controller.rb | 11 ++++ spec/requests/drafts_controller_spec.rb | 48 +++++++++++++-- spec/requests/posts_controller_spec.rb | 3 +- spec/requests/topics_controller_spec.rb | 22 +++++++ 7 files changed, 134 insertions(+), 45 deletions(-) diff --git a/app/assets/javascripts/discourse/app/models/composer.js b/app/assets/javascripts/discourse/app/models/composer.js index 67ae483fa5f..fcd9df9a069 100644 --- a/app/assets/javascripts/discourse/app/models/composer.js +++ b/app/assets/javascripts/discourse/app/models/composer.js @@ -73,13 +73,15 @@ const CLOSED = "closed", _update_serializer = { raw: "reply", topic_id: "topic.id", - raw_old: "rawOld", + original_text: "originalText", }, _edit_topic_serializer = { title: "topic.title", categoryId: "topic.category.id", tags: "topic.tags", featuredLink: "topic.featured_link", + original_title: "originalTitle", + original_tags: "originalTags", }, _draft_serializer = { reply: "reply", @@ -94,6 +96,9 @@ const CLOSED = "closed", typingTime: "typingTime", postId: "post.id", recipients: "targetRecipients", + original_text: "originalText", + original_title: "originalTitle", + original_tags: "originalTags", }, _add_draft_fields = {}, FAST_REPLY_LENGTH_THRESHOLD = 10000; @@ -863,6 +868,9 @@ export default class Composer extends RestModel { whisper: opts.whisper, tags: opts.tags || [], noBump: opts.noBump, + originalText: opts.originalText, + originalTitle: opts.originalTitle, + originalTags: opts.originalTags, }); if (opts.post) { @@ -928,6 +936,13 @@ export default class Composer extends RestModel { reply: post.raw, originalText: post.raw, }); + + if (post.post_number === 1 && this.canEditTitle) { + this.setProperties({ + originalTitle: post.topic.title, + originalTags: post.topic.tags, + }); + } }); // edge case ... make a post then edit right away @@ -947,24 +962,17 @@ export default class Composer extends RestModel { }); }); } else if (opts.action === REPLY && opts.quote) { - this.setProperties({ - reply: opts.quote, - originalText: opts.quote, - }); + this.set("reply", opts.quote); } if (opts.title) { this.set("title", opts.title); } - const isDraft = opts.draft || opts.skipDraftCheck; - this.set("originalText", isDraft ? "" : this.reply); - if (this.canEditTitle) { if (isEmpty(this.title) && this.title !== "") { this.set("title", ""); } - this.set("originalTitle", this.title); } if (!isEdit(opts.action) || !opts.post) { @@ -1003,6 +1011,8 @@ export default class Composer extends RestModel { clearState() { this.setProperties({ originalText: null, + originalTitle: null, + originalTags: null, reply: null, post: null, title: null, @@ -1018,12 +1028,9 @@ export default class Composer extends RestModel { }); } - @discourseComputed("editConflict", "originalText") - rawOld(editConflict, originalText) { - return editConflict ? null : originalText; - } - editPost(opts) { + this.set("composeState", SAVING); + const post = this.post; const oldCooked = post.cooked; let promise = Promise.resolve(); @@ -1056,14 +1063,19 @@ export default class Composer extends RestModel { } } - const props = { + let props = { edit_reason: opts.editReason, image_sizes: opts.imageSizes, - cooked: this.getCookedHtml(), }; this.serialize(_update_serializer, props); - this.set("composeState", SAVING); + + // user clicked "overwrite edits" button + if (this.editConflict) { + delete props.original_text; + delete props.original_title; + delete props.original_tags; + } const rollback = throwAjaxError((error) => { post.setProperties("cooked", oldCooked); @@ -1073,7 +1085,8 @@ export default class Composer extends RestModel { } }); - post.setProperties({ cooked: props.cooked, staged: true }); + const cooked = this.getCookedHtml(); + post.setProperties({ cooked, staged: true }); this.appEvents.trigger("post-stream:refresh", { id: post.id }); return promise @@ -1294,16 +1307,9 @@ export default class Composer extends RestModel { return Promise.resolve(); } - this.setProperties({ - draftSaving: true, - draftConflictUser: null, - }); + this.set("draftSaving", true); - let data = this.serialize(_draft_serializer); - - if (data.postId && !isEmpty(this.originalText)) { - data.originalText = this.originalText; - } + const data = this.serialize(_draft_serializer); const draftSequence = this.draftSequence; this.set("draftSequence", this.draftSequence + 1); diff --git a/app/controllers/drafts_controller.rb b/app/controllers/drafts_controller.rb index 16e1b1b1e8b..2f0bb2c89bc 100644 --- a/app/controllers/drafts_controller.rb +++ b/app/controllers/drafts_controller.rb @@ -96,16 +96,26 @@ class DraftsController < ApplicationController json = success_json.merge(draft_sequence: sequence) - if data.present? - # this is a bit of a kludge we need to remove (all the parsing) too many special cases here - # we need to catch action edit and action editSharedDraft - if data["postId"].present? && data["originalText"].present? && - data["action"].to_s.start_with?("edit") - post = Post.find_by(id: data["postId"]) - if post && post.raw != data["originalText"] - conflict_user = BasicUserSerializer.new(post.last_editor, root: false) - render json: json.merge(conflict_user: conflict_user) - return + # check for conflicts when editing a post + if data.present? && data["postId"].present? && data["action"].to_s.start_with?("edit") + original_text = data["original_text"] || data["originalText"] + original_title = data["original_title"] + original_tags = data["original_tags"] + + if original_text.present? + if post = Post.find_by(id: data["postId"]) + conflict = original_text != post.raw + + if post.post_number == 1 + conflict ||= original_title.present? && original_title != post.topic.title + conflict ||= + original_tags.present? && original_tags.sort != post.topic.tags.pluck(:name).sort + end + + if conflict + conflict_user = BasicUserSerializer.new(post.last_editor, root: false) + json.merge!(conflict_user:) + end end end end diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index bdd2b68831d..d1ac78ecf15 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -240,8 +240,9 @@ class PostsController < ApplicationController Post.plugin_permitted_update_params.keys.each { |param| changes[param] = params[:post][param] } - raw_old = params[:post][:raw_old] - if raw_old.present? && raw_old != post.raw + # keep `raw_old` for backwards compatibility + original_text = params[:post][:original_text] || params[:post][:raw_old] + if original_text.present? && original_text != post.raw return render_json_error(I18n.t("edit_conflict"), status: 409) end diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index f843308c42a..114be53d0a6 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -359,8 +359,19 @@ class TopicsController < ApplicationController def update topic = Topic.find_by(id: params[:topic_id]) + guardian.ensure_can_edit!(topic) + original_title = params[:original_title] + if original_title.present? && original_title != topic.title + return render_json_error(I18n.t("edit_conflict"), status: 409) + end + + original_tags = params[:original_tags] + if original_tags.present? && original_tags.sort != topic.tags.pluck(:name).sort + return render_json_error(I18n.t("edit_conflict"), status: 409) + end + if params[:category_id] && (params[:category_id].to_i != topic.category_id.to_i) if topic.shared_draft topic.shared_draft.update(category_id: params[:category_id]) diff --git a/spec/requests/drafts_controller_spec.rb b/spec/requests/drafts_controller_spec.rb index 40b16dedcf1..f923e1a09c0 100644 --- a/spec/requests/drafts_controller_spec.rb +++ b/spec/requests/drafts_controller_spec.rb @@ -99,15 +99,15 @@ RSpec.describe DraftsController do expect(response.status).to eq(404) end - it "checks for an conflict on update" do + it "checks for an raw conflict on update" do sign_in(user) - post = Fabricate(:post, user: user) + post = Fabricate(:post, user:) post "/drafts.json", params: { draft_key: "topic", sequence: 0, - data: { postId: post.id, originalText: post.raw, action: "edit" }.to_json, + data: { postId: post.id, original_text: post.raw, action: "edit" }.to_json, } expect(response.status).to eq(200) @@ -117,7 +117,7 @@ RSpec.describe DraftsController do params: { draft_key: "topic", sequence: 0, - data: { postId: post.id, originalText: "something else", action: "edit" }.to_json, + data: { postId: post.id, original_text: "something else", action: "edit" }.to_json, } expect(response.status).to eq(200) @@ -125,6 +125,46 @@ RSpec.describe DraftsController do expect(response.parsed_body["conflict_user"]).to include("avatar_template") end + it "checks for a title conflict on update" do + sign_in(user) + post = Fabricate(:post, user:) + + post "/drafts.json", + params: { + draft_key: "topic", + sequence: 0, + data: { + postId: post.id, + original_text: post.raw, + original_title: "something else", + action: "edit", + }.to_json, + } + + expect(response.status).to eq(200) + expect(response.parsed_body["conflict_user"]["id"]).to eq(post.last_editor.id) + end + + it "checks for a title conflict on update" do + sign_in(user) + post = Fabricate(:post, user:) + + post "/drafts.json", + params: { + draft_key: "topic", + sequence: 0, + data: { + postId: post.id, + original_text: post.raw, + original_tags: %w[tag1 tag2], + action: "edit", + }.to_json, + } + + expect(response.status).to eq(200) + expect(response.parsed_body["conflict_user"]["id"]).to eq(post.last_editor.id) + end + it "cant trivially resolve conflicts without interaction" do sign_in(user) diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index 4688cebd3b2..b885e3920f6 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -547,7 +547,7 @@ RSpec.describe PostsController do end it "checks for an edit conflict" do - update_params[:post][:raw_old] = "old body" + update_params[:post][:original_text] = "old body" put "/posts/#{post.id}.json", params: update_params expect(response.status).to eq(409) @@ -726,7 +726,6 @@ RSpec.describe PostsController do params: { post: { raw: "this is a random post", - raw_old: post.raw, random_number: 244, }, } diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 60d76a353a2..68d5ef2160d 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -1683,6 +1683,28 @@ RSpec.describe TopicsController do expect(response.parsed_body["basic_topic"]).to be_present end + it "prevents conflicts when title was changed" do + put "/t/#{topic.slug}/#{topic.id}.json", + params: { + title: "brand new title", + original_title: "another title", + } + + expect(response.status).to eq(409) + expect(response.parsed_body["errors"].first).to eq(I18n.t("edit_conflict")) + end + + it "prevents conflicts when tags were changed" do + put "/t/#{topic.slug}/#{topic.id}.json", + params: { + tags: %w[tag1 tag2], + original_tags: %w[tag3 tag4], + } + + expect(response.status).to eq(409) + expect(response.parsed_body["errors"].first).to eq(I18n.t("edit_conflict")) + end + it "throws an error if it could not be saved" do PostRevisor.any_instance.stubs(:should_revise?).returns(false) put "/t/#{topic.slug}/#{topic.id}.json", params: { title: "brand new title" }