From 806e37aaec549069a599fd31edc16c5cdcd0774e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Tue, 3 Dec 2024 10:12:04 +0100 Subject: [PATCH] FIX: better edit conflict handling (#29789) * FIX: better edit conflict handling Properly keep track of the original title and tags when dealing with edit conflicts. Internal ref - t/141218 --- .../discourse/app/models/composer.js | 75 +++++++++++-------- .../acceptance/composer-edit-conflict-test.js | 8 +- 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 ++++++ 8 files changed, 150 insertions(+), 52 deletions(-) diff --git a/app/assets/javascripts/discourse/app/models/composer.js b/app/assets/javascripts/discourse/app/models/composer.js index d7b9c128e5a..ae9d5d21153 100644 --- a/app/assets/javascripts/discourse/app/models/composer.js +++ b/app/assets/javascripts/discourse/app/models/composer.js @@ -8,7 +8,6 @@ import { isEmpty } from "@ember/utils"; import { observes, on } from "@ember-decorators/object"; import { Promise } from "rsvp"; import { extractError, throwAjaxError } from "discourse/lib/ajax-error"; -import { propertyNotEqual } from "discourse/lib/computed"; import { QUOTE_REGEXP } from "discourse/lib/quote"; import { prioritizeNameFallback } from "discourse/lib/settings"; import { emailValid, escapeExpression } from "discourse/lib/utilities"; @@ -73,13 +72,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 +95,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; @@ -210,8 +214,6 @@ export default class Composer extends RestModel { @equal("composeState", FULLSCREEN) viewFullscreen; @or("viewOpen", "viewFullscreen") viewOpenOrFullscreen; @and("editingPost", "post.firstPost") editingFirstPost; - @propertyNotEqual("reply", "originalText") replyDirty; - @propertyNotEqual("title", "originalTitle") titleDirty; @or( "creatingTopic", @@ -226,6 +228,16 @@ export default class Composer extends RestModel { @tracked _categoryId = null; + @discourseComputed("reply", "originalText") + replyDirty(reply, original) { + return (reply || "").trim() !== (original || "").trim(); + } + + @discourseComputed("title", "originalTitle") + titleDirty(title, original) { + return (title || "").trim() !== (original || "").trim(); + } + @dependentKeyCompat get categoryId() { return this._categoryId; @@ -787,6 +799,7 @@ export default class Composer extends RestModel { const category = Category.findById(categoryId); if (category) { this.set("reply", category.topic_template || ""); + this.set("originalText", category.topic_template || ""); } } @@ -861,6 +874,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) { @@ -926,6 +942,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 @@ -945,24 +968,18 @@ 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); + this.set("originalText", 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) { @@ -1001,6 +1018,8 @@ export default class Composer extends RestModel { clearState() { this.setProperties({ originalText: null, + originalTitle: null, + originalTags: null, reply: null, post: null, title: null, @@ -1016,12 +1035,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(); @@ -1054,14 +1070,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); @@ -1071,7 +1092,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 @@ -1292,16 +1314,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/assets/javascripts/discourse/tests/acceptance/composer-edit-conflict-test.js b/app/assets/javascripts/discourse/tests/acceptance/composer-edit-conflict-test.js index 17246321922..25da810b54f 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/composer-edit-conflict-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/composer-edit-conflict-test.js @@ -13,17 +13,17 @@ acceptance("Composer - Edit conflict", function (needs) { }); }); - test("Should not send originalText when posting a new reply", async function (assert) { + test("Should not send 'original_text' when posting a new reply", async function (assert) { await visit("/t/internationalization-localization/280"); await click(".topic-post:nth-of-type(1) button.reply"); await fillIn( ".d-editor-input", "hello world hello world hello world hello world hello world" ); - assert.false(lastBody.includes("originalText")); + assert.false(lastBody.includes("original_text")); }); - test("Should send originalText when editing a reply", async function (assert) { + test("Should send 'original_text' when editing a reply", async function (assert) { await visit("/t/internationalization-localization/280"); await click(".topic-post:nth-of-type(1) button.show-more-actions"); await click(".topic-post:nth-of-type(1) button.edit"); @@ -31,6 +31,6 @@ acceptance("Composer - Edit conflict", function (needs) { ".d-editor-input", "hello world hello world hello world hello world hello world" ); - assert.true(lastBody.includes("originalText")); + assert.true(lastBody.includes("original_text")); }); }); 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..0ee2d6a1cae 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 a 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 tag 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" }