From 9bdd97db428ae1c8794e33bdcc3de0820c825b27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 2 Dec 2024 16:06:36 +0100 Subject: [PATCH] FIX: editing post while replying (#29985) BEFORE: if you click the "reply" button on a post and then decided that you want to "edit" the same post, clicking the "edit" button would do nothing. Clicking "edit" on another post works, but editing the same post would appear broken. AFTER: if you click the "edit" button, it will properly load the content of the post you're trying to edit. No matter which one it is. This was somewhat tricky to track down as the system specs seemed to contradict the qunit tests until I realized that the qunit tests were only testing the edit on the 1st post and the system specs were testing on replies. I improved the qunit tests to test both editing OP and a reply and (hopefully) made the system specs a little bit clearer. This is a follow up to bbe62d88d2554189e15f407736b1f7a01968c631. --- .../discourse/app/controllers/topic.js | 29 +++------ .../tests/acceptance/composer-test.js | 28 ++++++++- spec/system/composer/discard_draft_spec.rb | 60 ++++++++++--------- .../page_objects/components/composer.rb | 4 ++ 4 files changed, 70 insertions(+), 51 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/topic.js b/app/assets/javascripts/discourse/app/controllers/topic.js index 5f748a4babc..5ef196223e7 100644 --- a/app/assets/javascripts/discourse/app/controllers/topic.js +++ b/app/assets/javascripts/discourse/app/controllers/topic.js @@ -848,12 +848,7 @@ export default class TopicController extends Controller.extend( return false; } - const composer = this.composer; - let topic = this.model; - const composerModel = composer.get("model"); - let editingFirst = - composerModel && - (post.get("firstPost") || composerModel.get("editingFirstPost")); + const topic = this.model; let editingSharedDraft = false; let draftsCategoryId = this.get("site.shared_drafts_category_id"); @@ -872,22 +867,14 @@ export default class TopicController extends Controller.extend( opts.destinationCategoryId = topic.get("destination_category_id"); } - // Reopen the composer if we're editing the same post - const editingExisting = - post.id === composerModel?.post?.id && - opts?.action === Composer.EDIT && - composerModel?.draftKey === opts.draftKey; - if (editingExisting) { - composer.unshrink(); - return; - } + const { composer } = this; + const composerModel = composer.get("model"); + const editingSamePost = + opts.post.id === composerModel?.post?.id && + opts.action === composerModel?.action && + opts.draftKey === composerModel?.draftKey; - // Cancel and reopen the composer for the first post - if (editingFirst) { - composer.cancelComposer(opts).then(() => composer.open(opts)); - } else { - composer.open(opts); - } + return editingSamePost ? composer.unshrink() : composer.open(opts); } @action diff --git a/app/assets/javascripts/discourse/tests/acceptance/composer-test.js b/app/assets/javascripts/discourse/tests/acceptance/composer-test.js index b7e47803fde..13045a0d8bb 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/composer-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/composer-test.js @@ -576,7 +576,7 @@ acceptance("Composer", function (needs) { ); }); - test("Composer can toggle between edit and reply", async function (assert) { + test("Composer can toggle between edit and reply on the OP", async function (assert) { await visit("/t/this-is-a-test-topic/9"); await click(".topic-post:nth-of-type(1) button.edit"); @@ -586,8 +586,10 @@ acceptance("Composer", function (needs) { /^This is the first post\./, "populates the input with the post text" ); + await click(".topic-post:nth-of-type(1) button.reply"); - assert.dom(".d-editor-input").hasNoValue("clears the input"); + assert.dom(".d-editor-input").hasNoValue("clears the composer input"); + await click(".topic-post:nth-of-type(1) button.edit"); assert .dom(".d-editor-input") @@ -597,6 +599,27 @@ acceptance("Composer", function (needs) { ); }); + test("Composer can toggle between edit and reply on a reply", async function (assert) { + await visit("/t/this-is-a-test-topic/9"); + + await click(".topic-post:nth-of-type(2) button.edit"); + assert + .dom(".d-editor-input") + .hasValue( + /^This is the second post\./, + "populates the input with the post text" + ); + + await click(".topic-post:nth-of-type(2) button.reply"); + assert.dom(".d-editor-input").hasNoValue("clears the composer input"); + + await click(".topic-post:nth-of-type(2) button.edit"); + assert.true( + query(".d-editor-input").value.startsWith("This is the second post."), + "populates the input with the post text" + ); + }); + test("Composer can toggle whispers when whisperer user", async function (assert) { const menu = selectKit(".toolbar-popup-menu-options"); @@ -802,6 +825,7 @@ acceptance("Composer", function (needs) { i18n("post.cancel_composer.keep_editing"), "has keep editing button" ); + await click(".d-modal__footer button.save-draft"); assert.dom(".d-editor-input").hasNoValue("clears the composer input"); }); diff --git a/spec/system/composer/discard_draft_spec.rb b/spec/system/composer/discard_draft_spec.rb index 96716ea44c3..f3e998b48b9 100644 --- a/spec/system/composer/discard_draft_spec.rb +++ b/spec/system/composer/discard_draft_spec.rb @@ -1,68 +1,72 @@ # frozen_string_literal: true describe "Composer - discard draft modal", type: :system do - fab!(:current_user) { Fabricate(:admin) } - fab!(:topic_1) { Fabricate(:topic) } + fab!(:topic) + fab!(:admin) let(:topic_page) { PageObjects::Pages::Topic.new } - let(:discard_draft_modal) { PageObjects::Modals::DiscardDraft.new } let(:composer) { PageObjects::Components::Composer.new } + let(:discard_draft_modal) { PageObjects::Modals::DiscardDraft.new } - before { sign_in(current_user) } + before { sign_in(admin) } context "when editing different post" do - fab!(:post_1) { Fabricate(:post, topic: topic_1, user: current_user) } - fab!(:post_2) { Fabricate(:post, topic: topic_1, user: current_user) } + fab!(:post_1) { Fabricate(:post, topic:, user: admin) } + fab!(:post_2) { Fabricate(:post, topic:, user: admin) } - it "shows the discard modal" do + it "shows the discard modal when there are changes in the composer" do topic_page.visit_topic(post_1.topic) topic_page.click_post_action_button(post_1, :edit) - composer.fill_content("a b c d e f g") composer.minimize + topic_page.click_post_action_button(post_2, :edit) expect(discard_draft_modal).to be_open end - end - context "when re-editing the first post" do - fab!(:post_1) { Fabricate(:post, topic: topic_1, user: current_user) } - - it "doesn’t show the discard modal" do + it "doesn't show the discard modal when there are no changes in the composer" do topic_page.visit_topic(post_1.topic) topic_page.click_post_action_button(post_1, :edit) - - composer.fill_content("a b c d e f g") composer.minimize - topic_page.click_post_action_button(post_1, :edit) - expect(discard_draft_modal).to be_closed - expect(composer).to be_opened - - topic_page.click_post_action_button(post_1, :edit) + topic_page.click_post_action_button(post_2, :edit) expect(discard_draft_modal).to be_closed expect(composer).to be_opened end end - context "when re-editing the second post" do - fab!(:post_1) { Fabricate(:post, topic: topic_1, user: current_user) } - fab!(:post_2) { Fabricate(:post, topic: topic_1, user: current_user) } + context "when editing the same post" do + fab!(:post_1) { Fabricate(:post, topic:, user: admin) } - it "doesn’t show the discard modal" do + it "doesn’t show the discard modal even if there are changes in the composer" do topic_page.visit_topic(post_1.topic) - topic_page.click_post_action_button(post_2, :edit) - + topic_page.click_post_action_button(post_1, :edit) composer.fill_content("a b c d e f g") composer.minimize - topic_page.click_post_action_button(post_2, :edit) + + topic_page.click_post_action_button(post_1, :edit) expect(discard_draft_modal).to be_closed expect(composer).to be_opened + expect(composer).to have_content("a b c d e f g") - topic_page.click_post_action_button(post_2, :edit) + composer.minimize + expect(composer).to be_minimized + + topic_page.click_post_action_button(post_1, :edit) + + expect(discard_draft_modal).to be_closed + expect(composer).to be_opened + end + + it "doesn’t show the discard modal when there are no changes in the composer" do + topic_page.visit_topic(post_1.topic) + topic_page.click_post_action_button(post_1, :edit) + composer.minimize + + topic_page.click_post_action_button(post_1, :edit) expect(discard_draft_modal).to be_closed expect(composer).to be_opened diff --git a/spec/system/page_objects/components/composer.rb b/spec/system/page_objects/components/composer.rb index 5e6e82c39ce..b5f11aba3f6 100644 --- a/spec/system/page_objects/components/composer.rb +++ b/spec/system/page_objects/components/composer.rb @@ -14,6 +14,10 @@ module PageObjects page.has_css?("#{COMPOSER_ID}.closed", visible: :all) end + def minimized? + page.has_css?("#{COMPOSER_ID}.draft") + end + def open_composer_actions find(".composer-action-title .btn").click self