From 6cfcdaa3562c374945394ee5c2e6930d6a0446f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Fri, 22 Nov 2024 09:16:00 +0100 Subject: [PATCH] FIX: editing post while replying 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 | 36 +++++++---- spec/system/composer/discard_draft_spec.rb | 60 ++++++++++--------- .../page_objects/components/composer.rb | 4 ++ 4 files changed, 69 insertions(+), 60 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 b7fb332ee91..54c1f8198c3 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/composer-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/composer-test.js @@ -571,7 +571,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"); @@ -579,12 +579,10 @@ acceptance("Composer", function (needs) { query(".d-editor-input").value.startsWith("This is the first post."), "it populates the input with the post text" ); + await click(".topic-post:nth-of-type(1) button.reply"); - assert.strictEqual( - query(".d-editor-input").value, - "", - "it clears the input" - ); + assert.dom(".d-editor-input").hasNoValue("it clears the input"); + await click(".topic-post:nth-of-type(1) button.edit"); assert.ok( query(".d-editor-input").value.startsWith("This is the first post."), @@ -592,6 +590,25 @@ 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.ok( + query(".d-editor-input").value.startsWith("This is the second post."), + "it populates the input with the post text" + ); + + await click(".topic-post:nth-of-type(2) button.reply"); + assert.dom(".d-editor-input").hasNoValue("it clears the input"); + + await click(".topic-post:nth-of-type(2) button.edit"); + assert.ok( + query(".d-editor-input").value.startsWith("This is the second post."), + "it 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"); @@ -798,12 +815,9 @@ acceptance("Composer", function (needs) { i18n("post.cancel_composer.keep_editing"), "has keep editing button" ); + await click(".d-modal__footer button.save-draft"); - assert.strictEqual( - query(".d-editor-input").value, - "", - "it clears the composer input" - ); + assert.dom(".d-editor-input").hasNoValue("it clears the input"); }); test("Checks for existing draft", async function (assert) { diff --git a/spec/system/composer/discard_draft_spec.rb b/spec/system/composer/discard_draft_spec.rb index 96716ea44c3..fe22e360082 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!(:user) { Fabricate(: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(user) } 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:) } + fab!(:post_2) { Fabricate(:post, topic:, user:) } - it "shows the discard modal" do + it "shows the discard modal when there are changes" 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's no changes" 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:) } - it "doesn’t show the discard modal" do + it "doesn’t show the discard modal even if there are changes" 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's no changes" 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