UX: Warn users if the post that's currently edited has changed. (PR #6498)

[nbianca]

GitHub

[discoursebot]

You’ve signed the CLA, nbianca. Thank you! This pull request is ready for review.

[eviltrout]

This is very cool

[ZogStriP]

app/assets/javascripts/discourse/models/composer.js.es6

@@ -762,16 +768,21 @@ const Composer = RestModel.extend({
 
     const props = {
       raw: this.get("reply"),
+      raw_old: !post.get("wiki") || this.get("editConflict") ? null : this.get("originalText"),

Any reasons we’re not always sending the originalText? Conflicts are not limited to wiki posts.

      raw_old: this.get("originalText"),

[ZogStriP]

app/assets/javascripts/discourse/models/composer.js.es6

@@ -762,16 +768,21 @@ const Composer = RestModel.extend({
 
     const props = {
       raw: this.get("reply"),
+      raw_old: !post.get("wiki") || this.get("editConflict") ? null : this.get("originalText"),
       edit_reason: opts.editReason,
       image_sizes: opts.imageSizes,
       cooked: this.getCookedHtml()
     };
 
     this.set("composeState", SAVING);
 
-    let rollback = throwAjaxError(() => {
+    let rollback = throwAjaxError(error => {
       post.set("cooked", oldCooked);
       this.set("composeState", OPEN);
+
+      if (error.jqXHR && error.jqXHR.status === 409) {

[ZogStriP]

app/controllers/posts_controller.rb

@@ -210,6 +210,13 @@ def update
       edit_reason: params[:post][:edit_reason]
     }
 
+    # Check if there is no edit conflict caused by two users saving different
+    # versions of the post.
+    raw_old = params[:post][:raw_old]
+    if !raw_old.empty? && raw_old != post.raw

The “double negation” feels weird.

    if raw_old.present? && raw_old != post.raw

[ZogStriP]

app/assets/javascripts/discourse/models/composer.js.es6

@@ -387,9 +387,14 @@ const Composer = RestModel.extend({
     return SAVE_ICONS[action];
   },
 
-  @computed("action", "whisper")
-  saveLabel(action, whisper) {
-    return whisper ? "composer.create_whisper" : SAVE_LABELS[action];
+  @computed("action", "whisper", "editConflict")
+  saveLabel(action, whisper, editConflict) {

There should be a test for that

[ZogStriP]

app/controllers/posts_controller.rb

@@ -210,6 +210,13 @@ def update
       edit_reason: params[:post][:edit_reason]
     }
 
+    # Check if there is no edit conflict caused by two users saving different
+    # versions of the post.
+    raw_old = params[:post][:raw_old]
+    if !raw_old.empty? && raw_old != post.raw
+      return render json: { errors: [I18n.t('edit_conflict')] }, status: 409

There should be a spec for that.

[ZogStriP]

config/locales/server.en.yml

@@ -304,6 +304,7 @@ en:
     badge: "%{display_name} badge on %{site_title}"
 
   too_late_to_edit: "That post was created too long ago. It can no longer be edited or deleted."
+  edit_conflict: "That post was edited by another user and your changes can no longer be edited."

Maybe this instead?

  edit_conflict: "That post was edited by another user and your changes can no longer be saved."

[nbianca]

The absence of raw_old will skip the check on server side.

First time raw_old will be sent so the server can check if there is a problem. The second time (after a editConflict happened) raw_old will not be sent anymore which will bypass the check and overwrite the content.

However, I do agree that it could work for all posts, not just wiki. The spec mentioned only these and I meant to ask you about it.

[tgxworld]

app/controllers/posts_controller.rb

@@ -210,6 +210,13 @@ def update
       edit_reason: params[:post][:edit_reason]
     }
 
+    # Check if there is no edit conflict caused by two users saving different
+    # versions of the post.

The code itself explains the behavior quite clearly so I’m not sure if we really need this comment here.

[tgxworld]

We have a pattern for rendering json errors, you can have a look at render_json_error

[tgxworld]

spec/requests/posts_controller_spec.rb

@@ -295,6 +295,14 @@
         expect(post.raw).to eq("edited body")
       end
 
+      it 'checks for an edit conflict' do
+        update_params[:post][:raw_old] = 'old body'
+        post.raw = 'newer body than raw_old'

Hmm this is never saved so the raw that the controller is seeing is different.

[nbianca]

Ah, yes. It’s actually not needed at all. The test only requires raw_old and the actual post content to be different.

[discoursedangerbot]

	1 Error
:no_entry_sign:	This PR doesn’t match our required code formatting standards, as enforced by prettier.io. Here’s how to set up prettier in your code editor. test/javascripts/acceptance/composer-edit-conflict-test.js.es6

Generated by :no_entry_sign: Danger

[nbianca]

Fixed and also changed one more occurrence of the same pattern above.

Thanks!

[tgxworld]

test/javascripts/acceptance/composer-edit-conflict-test.js.es6

@@ -0,0 +1,23 @@
+import { acceptance } from "helpers/qunit-helpers";
+
+acceptance("Composer - Edit conflict", {
+  loggedIn: true,
+
+  pretend(server, helper) {
+    server.put("/posts/398", () => {
+      return helper.response(409, { errors: ["edit conflict"] });
+    });
+  }
+});
+
+QUnit.test("Edit a post that causes an edit conflict", async assert => {