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

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({
+      this.set("reply", opts.quote);
-        reply: opts.quote,
+      this.set("originalText", opts.quote);
-        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({
+    this.set("draftSaving", true);
-      draftSaving: true,
-      draftConflictUser: null,
-    });
 
-    let data = this.serialize(_draft_serializer);
+    const data = this.serialize(_draft_serializer);
-
-    if (data.postId && !isEmpty(this.originalText)) {
-      data.originalText = this.originalText;
-    }
 
     const draftSequence = this.draftSequence;
     this.set("draftSequence", this.draftSequence + 1);

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"));
   });
 });

app/controllers/drafts_controller.rb

@@ -96,16 +96,26 @@ class DraftsController < ApplicationController
 
     json = success_json.merge(draft_sequence: sequence)
 
-    if data.present?
+    # check for conflicts when editing a post
-      # this is a bit of a kludge we need to remove (all the parsing) too many special cases here
+    if data.present? && data["postId"].present? && data["action"].to_s.start_with?("edit")
-      # we need to catch action edit and action editSharedDraft
+      original_text = data["original_text"] || data["originalText"]
-      if data["postId"].present? && data["originalText"].present? &&
+      original_title = data["original_title"]
-           data["action"].to_s.start_with?("edit")
+      original_tags = data["original_tags"]
-        post = Post.find_by(id: data["postId"])
+
-        if post && post.raw != data["originalText"]
+      if original_text.present?
-          conflict_user = BasicUserSerializer.new(post.last_editor, root: false)
+        if post = Post.find_by(id: data["postId"])
-          render json: json.merge(conflict_user: conflict_user)
+          conflict = original_text != post.raw
-          return
+
+          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

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]
+    # keep `raw_old` for backwards compatibility
-    if raw_old.present? && raw_old != post.raw
+    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
 

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])

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)
 

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,
               },
             }

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" }