FIX: improves draft for channels (#21724)

This commit attempts to correctly change draft when the channel changes. It moves responsibility to the composer instead of the channel.

A new service `chatDraftsManager` is being introduced here to allow finer control and pave the way for future thread draft support.

These changes also now allow an editing message to be stored as a draft.
This commit is contained in:
Joffrey JAFFEUX 2023-05-24 15:36:46 +02:00 committed by GitHub
parent d4a5b79592
commit 4de1d3952b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
17 changed files with 338 additions and 177 deletions

View File

@ -43,6 +43,7 @@ export default class ChatLivePane extends Component {
@service appEvents;
@service messageBus;
@service site;
@service chatDraftsManager;
@tracked loading = false;
@tracked loadingMorePast = false;
@ -116,11 +117,6 @@ export default class ChatLivePane extends Component {
if (this._loadedChannelId !== this.args.channel?.id) {
this.unsubscribeToUpdates(this._loadedChannelId);
this.chatChannelPane.selectingMessages = false;
if (this.args.channel.draft) {
this.chatChannelComposer.message = this.args.channel.draft;
}
this._loadedChannelId = this.args.channel?.id;
}
@ -642,6 +638,7 @@ export default class ChatLivePane extends Component {
.editMessage(this.args.channel.id, message.id, data)
.catch(popupAjaxError)
.finally(() => {
this.chatDraftsManager.remove({ channelId: this.args.channel.id });
this.chatChannelPane.sending = false;
});
}
@ -704,7 +701,7 @@ export default class ChatLivePane extends Component {
return;
}
this.args.channel.draft = null;
this.chatDraftsManager.remove({ channelId: this.args.channel.id });
this.chatChannelPane.sending = false;
});
}

View File

@ -1,4 +1,8 @@
<div class="chat-composer-message-details" data-id={{@message.id}}>
<div
class="chat-composer-message-details"
data-id={{@message.id}}
data-action={{if @message.editing "edit" "reply"}}
>
<div class="chat-reply">
{{d-icon (if @message.editing "pencil-alt" "reply")}}
<ChatUserAvatar @user={{@message.user}} />

View File

@ -26,6 +26,8 @@
{{did-update this.didUpdateMessage this.currentMessage}}
{{did-update this.didUpdateInReplyTo this.currentMessage.inReplyTo}}
{{did-insert this.setup}}
{{did-insert this.didUpdateChannel}}
{{did-update this.didUpdateChannel @channel.id}}
{{will-destroy this.teardown}}
{{will-destroy this.cancelPersistDraft}}
>

View File

@ -17,8 +17,8 @@ import I18n from "I18n";
import { translations } from "pretty-text/emoji/data";
import { setupHashtagAutocomplete } from "discourse/lib/hashtag-autocomplete";
import { isEmpty, isPresent } from "@ember/utils";
import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message";
import { Promise } from "rsvp";
import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message";
import User from "discourse/models/user";
export default class ChatComposer extends Component {
@ -33,6 +33,7 @@ export default class ChatComposer extends Component {
@service chatEmojiPickerManager;
@service currentUser;
@service chatApi;
@service chatDraftsManager;
@tracked isFocused = false;
@tracked inProgressUploadsCount = 0;
@ -78,15 +79,8 @@ export default class ChatComposer extends Component {
}
@action
sendMessage(raw) {
const message = ChatMessage.createDraftMessage(this.args.channel, {
user: this.currentUser,
message: raw,
});
this.args.onSendMessage(message);
return Promise.resolve();
sendMessage() {
this.args.onSendMessage(this.currentMessage);
}
@action
@ -107,13 +101,15 @@ export default class ChatComposer extends Component {
@action
didUpdateMessage() {
cancel(this._persistHandler);
this.cancelPersistDraft();
this.textareaInteractor.value = this.currentMessage.message || "";
this.textareaInteractor.focus({ refreshHeight: true });
this.persistDraft();
}
@action
didUpdateInReplyTo() {
this.cancelPersistDraft();
this.textareaInteractor.focus({ ensureAtEnd: true, refreshHeight: true });
this.persistDraft();
}
@ -148,11 +144,21 @@ export default class ChatComposer extends Component {
);
}
@action
didUpdateChannel() {
if (!this.args.channel) {
this.composer.message = null;
}
this.composer.message =
this.chatDraftsManager.get({ channelId: this.args.channel.id }) ||
ChatMessage.createDraftMessage(this.args.channel, {
user: this.currentUser,
});
}
@action
setup() {
this.composer.message = ChatMessage.createDraftMessage(this.args.channel, {
user: this.currentUser,
});
this.appEvents.on("chat:modify-selection", this, "modifySelection");
this.appEvents.on(
"chat:open-insert-link-modal",

View File

@ -7,6 +7,7 @@ import { action } from "@ember/object";
export default class ChatComposerChannel extends ChatComposer {
@service("chat-channel-composer") composer;
@service("chat-channel-pane") pane;
@service chatDraftsManager;
context = "channel";
@ -23,6 +24,8 @@ export default class ChatComposerChannel extends ChatComposer {
return;
}
this.chatDraftsManager.add(this.currentMessage);
this._persistHandler = discourseDebounce(
this,
this._debouncedPersistDraft,

View File

@ -81,7 +81,6 @@ export default class ChatChannel {
@tracked canFlag;
@tracked canModerate;
@tracked userSilenced;
@tracked draft = null;
@tracked meta;
@tracked chatableType;
@tracked chatableUrl;

View File

@ -138,8 +138,12 @@ export default class ChatMessage {
}
cook() {
const site = getOwner(this).lookup("service:site");
next(() => {
const site = getOwner(this).lookup("service:site");
if (this.isDestroyed || this.isDestroying) {
return;
}
const markdownOptions = {
featuresOverride:
@ -248,6 +252,12 @@ export default class ChatMessage {
};
}
if (this.editing) {
data.editing = true;
data.id = this.id;
data.excerpt = this.excerpt;
}
return JSON.stringify(data);
}

View File

@ -1,11 +1,19 @@
import { inject as service } from "@ember/service";
import { action } from "@ember/object";
import ChatComposer from "./chat-composer";
import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message";
export default class ChatChannelComposer extends ChatComposer {
@service chat;
@service router;
@action
reset(channel) {
this.message = ChatMessage.createDraftMessage(channel, {
user: this.currentUser,
});
}
@action
replyTo(message) {
this.chat.activeMessage = null;

View File

@ -7,7 +7,7 @@ export default class ChatChannelThreadComposer extends ChatComposer {
reset(channel, thread) {
this.message = ChatMessage.createDraftMessage(channel, {
user: this.currentUser,
thread,
});
this.message.thread = thread;
}
}

View File

@ -1,13 +1,12 @@
import { tracked } from "@glimmer/tracking";
import Service, { inject as service } from "@ember/service";
import { action } from "@ember/object";
import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message";
export default class ChatComposer extends Service {
@service chat;
@service currentUser;
@tracked _message;
@tracked message;
@action
cancel() {
@ -18,13 +17,6 @@ export default class ChatComposer extends Service {
}
}
@action
reset(channel) {
this.message = ChatMessage.createDraftMessage(channel, {
user: this.currentUser,
});
}
@action
clear() {
this.message.message = "";
@ -41,12 +33,4 @@ export default class ChatComposer extends Service {
onCancelEditing() {
this.reset();
}
get message() {
return this._message;
}
set message(message) {
this._message = message;
}
}

View File

@ -0,0 +1,25 @@
import Service from "@ember/service";
import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message";
export default class ChatDraftsManager extends Service {
drafts = {};
add(message) {
if (message instanceof ChatMessage) {
this.drafts[message.channel.id] = message;
} else {
throw new Error("message must be an instance of ChatMessage");
}
}
get({ channelId }) {
return this.drafts[channelId];
}
remove({ channelId }) {
delete this.drafts[channelId];
}
reset() {
this.drafts = {};
}
}

View File

@ -27,6 +27,7 @@ export default class Chat extends Service {
@service chatNotificationManager;
@service chatSubscriptionsManager;
@service chatStateManager;
@service chatDraftsManager;
@service presence;
@service router;
@service site;
@ -167,19 +168,18 @@ export default class Chat extends Service {
[...channels.public_channels, ...channels.direct_message_channels].forEach(
(channelObject) => {
const channel = this.chatChannelsManager.store(channelObject);
const storedDraft = (this.currentUser?.chat_drafts || []).find(
(draft) => draft.channel_id === channel.id
);
if (this.currentUser.chat_drafts) {
const storedDraft = this.currentUser.chat_drafts.find(
(draft) => draft.channel_id === channel.id
);
channel.draft = ChatMessage.createDraftMessage(
channel,
Object.assign(
{
user: this.currentUser,
},
storedDraft ? JSON.parse(storedDraft.data) : {}
if (storedDraft) {
this.chatDraftsManager.add(
ChatMessage.createDraftMessage(
channel,
Object.assign(
{ user: this.currentUser },
JSON.parse(storedDraft.data)
)
)
);
}

View File

@ -0,0 +1,135 @@
# frozen_string_literal: true
RSpec.describe "Chat composer draft", type: :system, js: true do
fab!(:current_user) { Fabricate(:user) }
fab!(:channel_1) { Fabricate(:chat_channel) }
fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
let(:chat_page) { PageObjects::Pages::Chat.new }
let(:channel_page) { PageObjects::Pages::ChatChannel.new }
before { chat_system_bootstrap }
context "when loading a channel with a draft" do
fab!(:draft_1) do
Chat::Draft.create!(
chat_channel: channel_1,
user: current_user,
data: { message: "draft" }.to_json,
)
end
before do
channel_1.add(current_user)
sign_in(current_user)
end
it "loads the draft" do
chat_page.visit_channel(channel_1)
expect(channel_page.composer.value).to eq("draft")
end
context "when loading another channel and back" do
fab!(:channel_2) { Fabricate(:chat_channel) }
fab!(:draft_2) do
Chat::Draft.create!(
chat_channel: channel_2,
user: current_user,
data: { message: "draft2" }.to_json,
)
end
before { channel_2.add(current_user) }
it "loads the correct drafts" do
chat_page.visit_channel(channel_1)
expect(channel_page.composer.value).to eq("draft")
chat_page.visit_channel(channel_2)
expect(channel_page.composer.value).to eq("draft2")
chat_page.visit_channel(channel_1)
expect(channel_page.composer.value).to eq("draft")
end
end
context "with editing" do
fab!(:draft_1) do
Chat::Draft.create!(
chat_channel: channel_1,
user: current_user,
data: { message: message_1.message, id: message_1.id, editing: true }.to_json,
)
end
it "loads the draft with the editing state" do
chat_page.visit_channel(channel_1)
expect(channel_page.composer).to be_editing_message(message_1)
end
end
context "with uploads" do
fab!(:upload_1) do
Fabricate(
:upload,
url: "/images/logo-dark.png",
original_filename: "logo_dark.png",
width: 400,
height: 300,
extension: "png",
)
end
fab!(:draft_1) do
Chat::Draft.create!(
chat_channel: channel_1,
user: current_user,
data: { message: "draft", uploads: [upload_1] }.to_json,
)
end
it "loads the draft with the upload" do
chat_page.visit_channel(channel_1)
expect(channel_page.composer.value).to eq("draft")
expect(page).to have_selector(".chat-composer-upload--image", count: 1)
end
end
context "when replying" do
fab!(:draft_1) do
Chat::Draft.create!(
chat_channel: channel_1,
user: current_user,
data: {
message: "draft",
replyToMsg: {
id: message_1.id,
excerpt: message_1.excerpt,
user: {
id: message_1.user.id,
name: nil,
avatar_template: message_1.user.avatar_template,
username: message_1.user.username,
},
},
}.to_json,
)
end
it "loads the draft with replied to mesage" do
chat_page.visit_channel(channel_1)
expect(channel_page.composer.value).to eq("draft")
expect(page).to have_selector(".chat-reply__username", text: message_1.user.username)
expect(page).to have_selector(".chat-reply__excerpt", text: message_1.excerpt)
end
end
end
end

View File

@ -5,90 +5,11 @@ RSpec.describe "Chat composer", type: :system, js: true do
fab!(:channel_1) { Fabricate(:chat_channel) }
fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
let(:chat) { PageObjects::Pages::Chat.new }
let(:channel) { PageObjects::Pages::ChatChannel.new }
let(:chat_page) { PageObjects::Pages::Chat.new }
let(:channel_page) { PageObjects::Pages::ChatChannel.new }
before { chat_system_bootstrap }
context "when loading a channel with a draft" do
fab!(:draft_1) do
Chat::Draft.create!(
chat_channel: channel_1,
user: current_user,
data: { message: "draft" }.to_json,
)
end
before do
channel_1.add(current_user)
sign_in(current_user)
end
it "loads the draft" do
chat.visit_channel(channel_1)
expect(find(".chat-composer__input").value).to eq("draft")
end
context "with uploads" do
fab!(:upload_1) do
Fabricate(
:upload,
url: "/images/logo-dark.png",
original_filename: "logo_dark.png",
width: 400,
height: 300,
extension: "png",
)
end
fab!(:draft_1) do
Chat::Draft.create!(
chat_channel: channel_1,
user: current_user,
data: { message: "draft", uploads: [upload_1] }.to_json,
)
end
it "loads the draft with the upload" do
chat.visit_channel(channel_1)
expect(find(".chat-composer__input").value).to eq("draft")
expect(page).to have_selector(".chat-composer-upload--image", count: 1)
end
end
context "when replying" do
fab!(:draft_1) do
Chat::Draft.create!(
chat_channel: channel_1,
user: current_user,
data: {
message: "draft",
replyToMsg: {
id: message_1.id,
excerpt: message_1.excerpt,
user: {
id: message_1.user.id,
name: nil,
avatar_template: message_1.user.avatar_template,
username: message_1.user.username,
},
},
}.to_json,
)
end
it "loads the draft with replied to mesage" do
chat.visit_channel(channel_1)
expect(find(".chat-composer__input").value).to eq("draft")
expect(page).to have_selector(".chat-reply__username", text: message_1.user.username)
expect(page).to have_selector(".chat-reply__excerpt", text: message_1.excerpt)
end
end
end
context "when replying to a message" do
before do
channel_1.add(current_user)
@ -96,8 +17,8 @@ RSpec.describe "Chat composer", type: :system, js: true do
end
it "adds the reply indicator to the composer" do
chat.visit_channel(channel_1)
channel.reply_to(message_1)
chat_page.visit_channel(channel_1)
channel_page.reply_to(message_1)
expect(page).to have_selector(
".chat-composer-message-details .chat-reply__username",
@ -109,8 +30,8 @@ RSpec.describe "Chat composer", type: :system, js: true do
before { message_1.update!(message: "<mark>not marked</mark>") }
it "renders text in the details" do
chat.visit_channel(channel_1)
channel.reply_to(message_1)
chat_page.visit_channel(channel_1)
channel_page.reply_to(message_1)
expect(
find(".chat-composer-message-details .chat-reply__excerpt")["innerHTML"].strip,
@ -128,47 +49,47 @@ RSpec.describe "Chat composer", type: :system, js: true do
end
it "adds the edit indicator" do
chat.visit_channel(channel_1)
channel.edit_message(message_2)
chat_page.visit_channel(channel_1)
channel_page.edit_message(message_2)
expect(page).to have_selector(
".chat-composer-message-details .chat-reply__username",
text: current_user.username,
)
expect(find(".chat-composer__input").value).to eq(message_2.message)
expect(channel_page.composer.value).to eq(message_2.message)
end
it "updates the message instantly" do
chat.visit_channel(channel_1)
chat_page.visit_channel(channel_1)
page.driver.browser.network_conditions = { offline: true }
channel.edit_message(message_2)
channel_page.edit_message(message_2)
find(".chat-composer__input").send_keys("instant")
channel.click_send_message
channel_page.click_send_message
expect(channel).to have_message(text: message_2.message + "instant")
expect(channel_page).to have_message(text: message_2.message + "instant")
page.driver.browser.network_conditions = { offline: false }
end
context "when pressing escape" do
it "cancels editing" do
chat.visit_channel(channel_1)
channel.edit_message(message_2)
chat_page.visit_channel(channel_1)
channel_page.edit_message(message_2)
find(".chat-composer__input").send_keys(:escape)
expect(page).to have_no_selector(".chat-composer-message-details .chat-reply__username")
expect(find(".chat-composer__input").value).to eq("")
expect(channel_page.composer.value).to eq("")
end
end
context "when closing edited message" do
it "cancels editing" do
chat.visit_channel(channel_1)
channel.edit_message(message_2)
chat_page.visit_channel(channel_1)
channel_page.edit_message(message_2)
find(".cancel-message-action").click
expect(page).to have_no_selector(".chat-composer-message-details .chat-reply__username")
expect(find(".chat-composer__input").value).to eq("")
expect(channel_page.composer.value).to eq("")
end
end
end
@ -180,19 +101,19 @@ RSpec.describe "Chat composer", type: :system, js: true do
end
xit "adds the emoji to the composer" do
chat.visit_channel(channel_1)
channel.open_action_menu
channel.click_action_button("emoji")
chat_page.visit_channel(channel_1)
channel_page.open_action_menu
channel_page.click_action_button("emoji")
find("[data-emoji='grimacing']").click(wait: 0.5)
expect(find(".chat-composer__input").value).to eq(":grimacing:")
expect(channel_page.composer.value).to eq(":grimacing:")
end
it "removes denied emojis from insert emoji picker" do
SiteSetting.emoji_deny_list = "monkey|peach"
chat.visit_channel(channel_1)
channel.composer.open_emoji_picker
chat_page.visit_channel(channel_1)
channel_page.composer.open_emoji_picker
expect(page).to have_no_selector("[data-emoji='monkey']")
expect(page).to have_no_selector("[data-emoji='peach']")
@ -206,16 +127,16 @@ RSpec.describe "Chat composer", type: :system, js: true do
end
it "adds the emoji to the composer" do
chat.visit_channel(channel_1)
chat_page.visit_channel(channel_1)
find(".chat-composer__input").fill_in(with: ":gri")
find(".emoji-shortname", text: "grimacing").click
expect(find(".chat-composer__input").value).to eq(":grimacing: ")
expect(channel_page.composer.value).to eq(":grimacing: ")
end
it "doesn't suggest denied emojis and aliases" do
SiteSetting.emoji_deny_list = "peach|poop"
chat.visit_channel(channel_1)
chat_page.visit_channel(channel_1)
find(".chat-composer__input").fill_in(with: ":peac")
expect(page).to have_no_selector(".emoji-shortname", text: "peach")
@ -232,7 +153,7 @@ RSpec.describe "Chat composer", type: :system, js: true do
end
xit "prefills the emoji picker filter input" do
chat.visit_channel(channel_1)
chat_page.visit_channel(channel_1)
find(".chat-composer__input").fill_in(with: ":gri")
click_link(I18n.t("js.composer.more_emoji"))
@ -241,7 +162,7 @@ RSpec.describe "Chat composer", type: :system, js: true do
end
xit "filters with the prefilled input" do
chat.visit_channel(channel_1)
chat_page.visit_channel(channel_1)
find(".chat-composer__input").fill_in(with: ":fr")
click_link(I18n.t("js.composer.more_emoji"))
@ -258,19 +179,19 @@ RSpec.describe "Chat composer", type: :system, js: true do
end
it "propagates keys to composer" do
chat.visit_channel(channel_1)
chat_page.visit_channel(channel_1)
find("body").send_keys("b")
expect(find(".chat-composer__input").value).to eq("b")
expect(channel_page.composer.value).to eq("b")
find("body").send_keys("b")
expect(find(".chat-composer__input").value).to eq("bb")
expect(channel_page.composer.value).to eq("bb")
find("body").send_keys(:enter) # special case
expect(find(".chat-composer__input").value).to eq("bb")
expect(channel_page.composer.value).to eq("bb")
end
end
@ -288,7 +209,7 @@ RSpec.describe "Chat composer", type: :system, js: true do
element.setSelectionRange(0, element.value.length)
JS
chat.visit_channel(channel_1)
chat_page.visit_channel(channel_1)
find("body").send_keys("https://www.discourse.org")
page.execute_script(select_text, ".chat-composer__input")
@ -301,7 +222,7 @@ RSpec.describe "Chat composer", type: :system, js: true do
page.send_keys [modifier, "v"]
expect(find(".chat-composer__input").value).to eq("[discourse](https://www.discourse.org)")
expect(channel_page.composer.value).to eq("[discourse](https://www.discourse.org)")
end
end
@ -313,11 +234,11 @@ RSpec.describe "Chat composer", type: :system, js: true do
end
it "works" do
chat.visit_channel(channel_1)
chat_page.visit_channel(channel_1)
find("body").send_keys("1")
channel.click_send_message
channel_page.click_send_message
expect(channel).to have_message(text: "1")
expect(channel_page).to have_message(text: "1")
end
end
@ -329,7 +250,7 @@ RSpec.describe "Chat composer", type: :system, js: true do
end
it "doesnt allow to send" do
chat.visit_channel(channel_1)
chat_page.visit_channel(channel_1)
find("body").send_keys("1")
expect(page).to have_css(".chat-composer.is-send-disabled")
@ -343,14 +264,14 @@ RSpec.describe "Chat composer", type: :system, js: true do
end
it "doesnt allow to send" do
chat.visit_channel(channel_1)
chat_page.visit_channel(channel_1)
page.driver.browser.network_conditions = { latency: 20_000 }
file_path = file_from_fixtures("logo.png", "images").path
attach_file(file_path) do
channel.open_action_menu
channel.click_action_button("chat-upload-btn")
channel_page.open_action_menu
channel_page.click_action_button("chat-upload-btn")
end
expect(page).to have_css(".chat-composer-upload--in-progress")

View File

@ -12,6 +12,10 @@ module PageObjects
@context = context
end
def message_details
@message_details ||= PageObjects::Components::Chat::ComposerMessageDetails.new(context)
end
def input
find(context).find(SELECTOR).find(".chat-composer__input")
end
@ -31,6 +35,10 @@ module PageObjects
def open_emoji_picker
find(context).find(SELECTOR).find(".chat-composer-button__btn.emoji").click
end
def editing_message?(message)
value == message.message && message_details.editing_message?(message)
end
end
end
end

View File

@ -12,13 +12,19 @@ module PageObjects
@context = context
end
def has_message?(message)
find(context).find(SELECTOR + "[data-id=\"#{message.id}\"]")
def has_message?(message, action: nil)
data_attributes = "[data-id=\"#{message.id}\"]"
data_attributes << "[data-action=\"#{action}\"]" if action
find(context).find(SELECTOR + data_attributes)
end
def has_no_message?
find(context).has_no_css?(SELECTOR)
end
def editing_message?(message)
has_message?(message, action: :edit)
end
end
end
end

View File

@ -0,0 +1,53 @@
import { module, test } from "qunit";
import { setupTest } from "ember-qunit";
import { getOwner } from "discourse-common/lib/get-owner";
import fabricators from "discourse/plugins/chat/discourse/lib/fabricators";
module(
"Discourse Chat | Unit | Service | chat-drafts-manager",
function (hooks) {
setupTest(hooks);
hooks.beforeEach(function () {
this.subject = getOwner(this).lookup("service:chat-drafts-manager");
});
hooks.afterEach(function () {
this.subject.reset();
});
test("storing and retrieving message", function (assert) {
const message1 = fabricators.message();
this.subject.add(message1);
assert.strictEqual(
this.subject.get({ channelId: message1.channel.id }),
message1
);
const message2 = fabricators.message();
this.subject.add(message2);
assert.strictEqual(
this.subject.get({ channelId: message2.channel.id }),
message2
);
});
test("stores only chat messages", function (assert) {
assert.throws(function () {
this.subject.add({ foo: "bar" });
}, /instance of ChatMessage/);
});
test("#reset", function (assert) {
this.subject.add(fabricators.message());
assert.strictEqual(Object.keys(this.subject.drafts).length, 1);
this.subject.reset();
assert.strictEqual(Object.keys(this.subject.drafts).length, 0);
});
}
);