From 586d572e053a9a8772271e832efe311765e10a1b Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Wed, 23 Feb 2022 10:39:54 +0300 Subject: [PATCH] FIX: Don't advance draft sequence when editing topic title (#16002) This commit handles the edge case where a draft is lost with no warnings if the user edits the title (or category/tags) of a topic while they're replying.to the same topic. Repro steps are as follows: 1. Start replying to a topic and type enough to get a draft saved. 2. Scroll up to the topic title and click the pencil icon next to the topic title, change the title, category and/or tags, and then save the changes. 3. Reload the page and you'll see that the draft is gone. This happens because we only allow 1 draft per topic per user and when you edit the title of a topic that you're replying to, from the server perspective it'll look like as if you've submitted your reply so it will advance the draft sequence for the topic and delete the draft. The fix in this commit makes `PostRevisor` skip advancing the draft sequence when a topic's title is edited using the pencil button next to the title. Internal ticket: t60854. Co-authored-by: Robin Ward --- .../discourse/app/controllers/topic.js | 2 +- .../javascripts/discourse/app/models/topic.js | 8 ++++-- app/controllers/topics_controller.rb | 8 +++++- lib/post_revisor.rb | 14 +++++++--- spec/lib/post_revisor_spec.rb | 27 +++++++++++++++++++ 5 files changed, 52 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/topic.js b/app/assets/javascripts/discourse/app/controllers/topic.js index c99adc7a100..3354a0cfaaa 100644 --- a/app/assets/javascripts/discourse/app/controllers/topic.js +++ b/app/assets/javascripts/discourse/app/controllers/topic.js @@ -980,7 +980,7 @@ export default Controller.extend(bufferedProperty("model"), { // save the modifications const props = this.get("buffered.buffer"); - Topic.update(this.model, props) + Topic.update(this.model, props, { fastEdit: true }) .then(() => { // We roll back on success here because `update` saves the properties to the topic this.rollbackBuffer(); diff --git a/app/assets/javascripts/discourse/app/models/topic.js b/app/assets/javascripts/discourse/app/models/topic.js index e17c723d29f..ae74d2ea637 100644 --- a/app/assets/javascripts/discourse/app/models/topic.js +++ b/app/assets/javascripts/discourse/app/models/topic.js @@ -661,7 +661,7 @@ Topic.reopenClass({ } }, - update(topic, props) { + update(topic, props, opts = {}) { // We support `category_id` and `categoryId` for compatibility if (typeof props.categoryId !== "undefined") { props.category_id = props.categoryId; @@ -673,9 +673,13 @@ Topic.reopenClass({ delete props.category_id; } + const data = { ...props }; + if (opts.fastEdit) { + data.keep_existing_draft = true; + } return ajax(topic.get("url"), { type: "PUT", - data: JSON.stringify(props), + data: JSON.stringify(data), contentType: "application/json", }).then((result) => { // The title can be cleaned up server side diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 73fb6838f41..41ad745d6ad 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -397,7 +397,13 @@ class TopicsController < ApplicationController bypass_bump = should_bypass_bump?(changes) first_post = topic.ordered_posts.first - success = PostRevisor.new(first_post, topic).revise!(current_user, changes, validate_post: false, bypass_bump: bypass_bump) + success = PostRevisor.new(first_post, topic).revise!( + current_user, + changes, + validate_post: false, + bypass_bump: bypass_bump, + keep_existing_draft: params[:keep_existing_draft].to_s == "true" + ) if !success && topic.errors.blank? topic.errors.add(:base, :unable_to_update) diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb index 8c046392a4e..03e3ede96bc 100644 --- a/lib/post_revisor.rb +++ b/lib/post_revisor.rb @@ -159,7 +159,16 @@ class PostRevisor end end - return false unless should_revise? + if !should_revise? + # the draft sequence is advanced here to handle the edge case where a + # user opens the composer to edit a post and makes some changes (which + # saves a draft), but then un-does the changes and clicks save. In this + # case, should_revise? returns false because nothing has really changed + # in the post, but we want to get rid of the draft so we advance the + # sequence. + advance_draft_sequence if !opts[:keep_existing_draft] + return false + end @post.acting_user = @editor @topic.acting_user = @editor @@ -204,7 +213,7 @@ class PostRevisor plugin_callbacks revise_topic - advance_draft_sequence + advance_draft_sequence if !opts[:keep_existing_draft] end # Lock the post by default if the appropriate setting is true @@ -271,7 +280,6 @@ class PostRevisor return true end end - advance_draft_sequence false end diff --git a/spec/lib/post_revisor_spec.rb b/spec/lib/post_revisor_spec.rb index e45e95de338..f69b1180aaa 100644 --- a/spec/lib/post_revisor_spec.rb +++ b/spec/lib/post_revisor_spec.rb @@ -1236,6 +1236,33 @@ describe PostRevisor do end end end + + context 'with drafts' do + it "does not advance draft sequence if keep_existing_draft option is true" do + post = Fabricate(:post, user: user) + topic = post.topic + draft_key = "topic_#{topic.id}" + data = { reply: "test 12222" }.to_json + Draft.set(user, draft_key, 0, data) + Draft.set(user, draft_key, 0, data) + expect { + PostRevisor.new(post).revise!( + post.user, + { title: "updated title for my topic" }, + keep_existing_draft: true + ) + }.to change { Draft.where(user: user, draft_key: draft_key).first.sequence }.by(0) + .and change { DraftSequence.where(user_id: user.id, draft_key: draft_key).first.sequence }.by(0) + + expect { + PostRevisor.new(post).revise!( + post.user, + { title: "updated title for my topic" }, + ) + }.to change { Draft.where(user: user, draft_key: draft_key).count }.from(1).to(0) + .and change { DraftSequence.where(user_id: user.id, draft_key: draft_key).first.sequence }.by(1) + end + end end context 'when the review_every_post setting is enabled' do