IntenseWebs/discourse
3
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-02-25 18:55:32 -06:00
Code Issues Packages Projects Releases Wiki Activity

FEATURE: Mark all chat channels read with a shortcut (#20629)

Browse Source 
This commit adds a keyboard shortcut (Shift+ESC) for chat which marks all
of the chat channels that the user is currently a following member of as read,
updating their `last_read_message_id`. This is done via a new service.

It also includes some refactors and controller changes:

* The old mark message read route from `ChatController` is now supplanted
  by the `Chat::Api::ReadsController#update` route.
* The new controller can handle either marking a single or all messages read,
  and uses the correct service based on the route and params.
* The `UpdateUserLastRead` service is now used (it wasn't before), and has been slightly
  updated to just use the guardian user ID.
This commit is contained in:
Martin Brennan
2023-03-22 13:24:07 +10:00
committed by GitHub
parent 786f7503b4
commit a0381157e9
19 changed files with 619 additions and 216 deletions
Download Patch File Download Diff File Expand all files Collapse all files

199
plugins/chat/spec/requests/chat/api/reads_controller_spec.rb Normal file
View File

@@ -0,0 +1,199 @@
# frozen_string_literal: true
RSpec.describe Chat::Api::ReadsController do
fab!(:current_user) { Fabricate(:user) }
before do
SiteSetting.chat_enabled = true
SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone]
sign_in(current_user)
end
describe "#read" do
describe "marking a single message read" do
fab!(:chat_channel) { Fabricate(:chat_channel) }
fab!(:other_user) { Fabricate(:user) }
fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) }
fab!(:message_2) { Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) }
it "returns a 404 when the user is not a channel member" do
put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json"
expect(response.status).to eq(404)
end
it "returns a 404 when the user is not following the channel" do
Fabricate(
:user_chat_channel_membership,
chat_channel: chat_channel,
user: current_user,
following: false,
)
put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json"
expect(response.status).to eq(404)
end
describe "when the user is a channel member" do
fab!(:membership) do
Fabricate(:user_chat_channel_membership, chat_channel: chat_channel, user: current_user)
end
context "when message_id param doesn't link to a message of the channel" do
it "raises a not found" do
put "/chat/api/channels/#{chat_channel.id}/read/-999.json"
expect(response.status).to eq(404)
end
end
context "when message_id param is inferior to existing last read" do
before { membership.update!(last_read_message_id: message_2.id) }
it "raises an invalid request" do
put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json"
expect(response.status).to eq(400)
expect(response.parsed_body["errors"][0]).to match(/message_id/)
end
end
context "when message_id refers to deleted message" do
before { message_1.trash!(Discourse.system_user) }
it "works" do
put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}"
expect(response.status).to eq(200)
end
end
it "updates timing records" do
expect {
put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json"
}.not_to change { Chat::UserChatChannelMembership.count }
membership.reload
expect(membership.chat_channel_id).to eq(chat_channel.id)
expect(membership.last_read_message_id).to eq(message_1.id)
expect(membership.user_id).to eq(current_user.id)
end
it "marks all mention notifications as read for the channel" do
notification = create_notification_and_mention_for(current_user, other_user, message_1)
put "/chat/api/channels/#{chat_channel.id}/read/#{message_2.id}.json"
expect(response.status).to eq(200)
expect(notification.reload.read).to eq(true)
end
it "doesn't mark notifications of messages that weren't read yet" do
message_3 = Fabricate(:chat_message, chat_channel: chat_channel, user: other_user)
notification = create_notification_and_mention_for(current_user, other_user, message_3)
put "/chat/api/channels/#{chat_channel.id}/read/#{message_2.id}.json"
expect(response.status).to eq(200)
expect(notification.reload.read).to eq(false)
end
end
end
describe "marking all messages read" do
fab!(:chat_channel_1) { Fabricate(:chat_channel) }
fab!(:chat_channel_2) { Fabricate(:chat_channel) }
fab!(:chat_channel_3) { Fabricate(:chat_channel) }
fab!(:other_user) { Fabricate(:user) }
fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel_1, user: other_user) }
fab!(:message_2) { Fabricate(:chat_message, chat_channel: chat_channel_1, user: other_user) }
fab!(:message_3) { Fabricate(:chat_message, chat_channel: chat_channel_2, user: other_user) }
fab!(:message_4) { Fabricate(:chat_message, chat_channel: chat_channel_2, user: other_user) }
fab!(:message_5) { Fabricate(:chat_message, chat_channel: chat_channel_3, user: other_user) }
fab!(:message_6) { Fabricate(:chat_message, chat_channel: chat_channel_3, user: other_user) }
fab!(:membership_1) do
Fabricate(
:user_chat_channel_membership,
chat_channel: chat_channel_1,
user: current_user,
following: true,
)
end
fab!(:membership_2) do
Fabricate(
:user_chat_channel_membership,
chat_channel: chat_channel_2,
user: current_user,
following: true,
)
end
fab!(:membership_3) do
Fabricate(
:user_chat_channel_membership,
chat_channel: chat_channel_3,
user: current_user,
following: true,
)
end
it "marks all messages as read across the user's channel memberships with the correct last_read_message_id" do
put "/chat/api/channels/read.json"
expect(membership_1.reload.last_read_message_id).to eq(message_2.id)
expect(membership_2.reload.last_read_message_id).to eq(message_4.id)
expect(membership_3.reload.last_read_message_id).to eq(message_6.id)
end
it "doesn't mark messages for channels the user is not following as read" do
membership_1.update!(following: false)
put "/chat/api/channels/read.json"
expect(membership_1.reload.last_read_message_id).to eq(nil)
expect(membership_2.reload.last_read_message_id).to eq(message_4.id)
expect(membership_3.reload.last_read_message_id).to eq(message_6.id)
end
it "returns the updated memberships, channels, and last message id" do
put "/chat/api/channels/read.json"
expect(response.parsed_body["updated_memberships"]).to match_array(
[
{
"channel_id" => chat_channel_1.id,
"last_read_message_id" => message_2.id,
"membership_id" => membership_1.id,
},
{
"channel_id" => chat_channel_2.id,
"last_read_message_id" => message_4.id,
"membership_id" => membership_2.id,
},
{
"channel_id" => chat_channel_3.id,
"last_read_message_id" => message_6.id,
"membership_id" => membership_3.id,
},
],
)
end
end
end
def create_notification_and_mention_for(user, sender, msg)
Notification
.create!(
notification_type: Notification.types[:chat_mention],
user: user,
high_priority: true,
read: false,
data: {
message: "chat.mention_notification",
chat_message_id: msg.id,
chat_channel_id: msg.chat_channel_id,
chat_channel_title: msg.chat_channel.title(user),
chat_channel_slug: msg.chat_channel.slug,
mentioned_by_username: sender.username,
}.to_json,
)
.tap do |notification|
Chat::Mention.create!(user: user, chat_message: msg, notification: notification)
end
end
end

111
plugins/chat/spec/requests/chat_controller_spec.rb
View File

@@ -725,117 +725,6 @@ RSpec.describe Chat::ChatController do
end
end
describe "#update_user_last_read" do
before { sign_in(user) }
fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) }
fab!(:message_2) { Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) }
it "returns a 404 when the user is not a channel member" do
put "/chat/#{chat_channel.id}/read/#{message_1.id}.json"
expect(response.status).to eq(404)
end
it "returns a 404 when the user is not following the channel" do
Fabricate(
:user_chat_channel_membership,
chat_channel: chat_channel,
user: user,
following: false,
)
put "/chat/#{chat_channel.id}/read/#{message_1.id}.json"
expect(response.status).to eq(404)
end
describe "when the user is a channel member" do
fab!(:membership) do
Fabricate(:user_chat_channel_membership, chat_channel: chat_channel, user: user)
end
context "when message_id param doesn't link to a message of the channel" do
it "raises a not found" do
put "/chat/#{chat_channel.id}/read/-999.json"
expect(response.status).to eq(404)
end
end
context "when message_id param is inferior to existing last read" do
before { membership.update!(last_read_message_id: message_2.id) }
it "raises an invalid request" do
put "/chat/#{chat_channel.id}/read/#{message_1.id}.json"
expect(response.status).to eq(400)
expect(response.parsed_body["errors"][0]).to match(/message_id/)
end
end
context "when message_id refers to deleted message" do
before { message_1.trash!(Discourse.system_user) }
it "works" do
put "/chat/#{chat_channel.id}/read/#{message_1.id}.json"
expect(response.status).to eq(200)
end
end
it "updates timing records" do
expect { put "/chat/#{chat_channel.id}/read/#{message_1.id}.json" }.not_to change {
Chat::UserChatChannelMembership.count
}
membership.reload
expect(membership.chat_channel_id).to eq(chat_channel.id)
expect(membership.last_read_message_id).to eq(message_1.id)
expect(membership.user_id).to eq(user.id)
end
def create_notification_and_mention_for(user, sender, msg)
Notification
.create!(
notification_type: Notification.types[:chat_mention],
user: user,
high_priority: true,
read: false,
data: {
message: "chat.mention_notification",
chat_message_id: msg.id,
chat_channel_id: msg.chat_channel_id,
chat_channel_title: msg.chat_channel.title(user),
chat_channel_slug: msg.chat_channel.slug,
mentioned_by_username: sender.username,
}.to_json,
)
.tap do |notification|
Chat::Mention.create!(user: user, chat_message: msg, notification: notification)
end
end
it "marks all mention notifications as read for the channel" do
notification = create_notification_and_mention_for(user, other_user, message_1)
put "/chat/#{chat_channel.id}/read/#{message_2.id}.json"
expect(response.status).to eq(200)
expect(notification.reload.read).to eq(true)
end
it "doesn't mark notifications of messages that weren't read yet" do
message_3 = Fabricate(:chat_message, chat_channel: chat_channel, user: other_user)
notification = create_notification_and_mention_for(user, other_user, message_3)
put "/chat/#{chat_channel.id}/read/#{message_2.id}.json"
expect(response.status).to eq(200)
expect(notification.reload.read).to eq(false)
end
end
end
describe "react" do
fab!(:chat_channel) { Fabricate(:category_channel) }
fab!(:chat_message) { Fabricate(:chat_message, chat_channel: chat_channel, user: user) }

23
plugins/chat/spec/services/auto_remove/handle_destroyed_group_spec.rb
View File

@@ -35,8 +35,6 @@ RSpec.describe Chat::AutoRemove::HandleDestroyedGroup do
describe "step remove_users_outside_allowed_groups" do
context "when chat_allowed_groups is empty" do
let(:action) { UserHistory.where(custom_type: "chat_auto_remove_membership").last }
before do
SiteSetting.chat_allowed_groups = ""
channel_1.add(user_1)
@@ -104,11 +102,22 @@ RSpec.describe Chat::AutoRemove::HandleDestroyedGroup do
it "logs a staff action" do
result
expect(action).to have_attributes(
details: "users_removed: 2\nchannel_id: #{channel_2.id}\nevent: destroyed_group",
acting_user_id: Discourse.system_user.id,
custom_type: "chat_auto_remove_membership",
)
actions = UserHistory.where(custom_type: "chat_auto_remove_membership")
expect(actions.count).to eq(2)
expect(
actions.exists?(
details: "users_removed: 2\nchannel_id: #{channel_2.id}\nevent: destroyed_group",
acting_user_id: Discourse.system_user.id,
custom_type: "chat_auto_remove_membership",
),
).to eq(true)
expect(
actions.exists?(
details: "users_removed: 2\nchannel_id: #{channel_1.id}\nevent: destroyed_group",
acting_user_id: Discourse.system_user.id,
custom_type: "chat_auto_remove_membership",
),
).to eq(true)
end
end

133
plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb Normal file
View File

@@ -0,0 +1,133 @@
# frozen_string_literal: true
RSpec.describe Chat::MarkAllUserChannelsRead do
describe ".call" do
subject(:result) { described_class.call(params) }
let(:params) { { guardian: guardian } }
let(:guardian) { Guardian.new(current_user) }
fab!(:current_user) { Fabricate(:user) }
fab!(:channel_1) { Fabricate(:chat_channel) }
fab!(:channel_2) { Fabricate(:chat_channel) }
fab!(:channel_3) { Fabricate(:chat_channel) }
fab!(:other_user) { Fabricate(:user) }
fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, user: other_user) }
fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel_1, user: other_user) }
fab!(:message_3) { Fabricate(:chat_message, chat_channel: channel_2, user: other_user) }
fab!(:message_4) { Fabricate(:chat_message, chat_channel: channel_2, user: other_user) }
fab!(:message_5) { Fabricate(:chat_message, chat_channel: channel_3, user: other_user) }
fab!(:message_6) { Fabricate(:chat_message, chat_channel: channel_3, user: other_user) }
fab!(:membership_1) do
Fabricate(
:user_chat_channel_membership,
chat_channel: channel_1,
user: current_user,
following: true,
)
end
fab!(:membership_2) do
Fabricate(
:user_chat_channel_membership,
chat_channel: channel_2,
user: current_user,
following: true,
)
end
fab!(:membership_3) do
Fabricate(
:user_chat_channel_membership,
chat_channel: channel_3,
user: current_user,
following: true,
)
end
context "when the user has no memberships" do
let(:guardian) { Guardian.new(Fabricate(:user)) }
it "sets the service result as successful" do
expect(result).to be_a_success
end
it "returns the updated_memberships in context" do
expect(result.updated_memberships).to eq([])
end
end
context "when everything is fine" do
fab!(:notification_1) do
Fabricate(
:notification,
notification_type: Notification.types[:chat_mention],
user: current_user,
)
end
fab!(:notification_2) do
Fabricate(
:notification,
notification_type: Notification.types[:chat_mention],
user: current_user,
)
end
let(:messages) { MessageBus.track_publish { result } }
before do
Chat::Mention.create!(
notification: notification_1,
user: current_user,
chat_message: message_1,
)
Chat::Mention.create!(
notification: notification_2,
user: current_user,
chat_message: message_3,
)
end
it "sets the service result as successful" do
expect(result).to be_a_success
end
it "updates the last_read_message_ids" do
result
expect(membership_1.reload.last_read_message_id).to eq(message_2.id)
expect(membership_2.reload.last_read_message_id).to eq(message_4.id)
expect(membership_3.reload.last_read_message_id).to eq(message_6.id)
end
it "does not update memberships where the user is not following" do
membership_1.update!(following: false)
result
expect(membership_1.reload.last_read_message_id).to eq(nil)
end
it "does not use deleted messages for the last_read_message_id" do
message_2.trash!
result
expect(membership_1.reload.last_read_message_id).to eq(message_1.id)
end
it "returns the updated_memberships in context" do
expect(result.updated_memberships.map(&:channel_id)).to match_array(
[channel_1.id, channel_2.id, channel_3.id],
)
end
it "marks existing notifications for all affected channels as read" do
expect { result }.to change {
Notification.where(
notification_type: Notification.types[:chat_mention],
user: current_user,
read: false,
).count
}.by(-2)
end
end
end
end

20
plugins/chat/spec/services/chat/update_user_last_read_spec.rb
View File

@@ -2,7 +2,6 @@
RSpec.describe Chat::UpdateUserLastRead do
describe Chat::UpdateUserLastRead::Contract, type: :model do
it { is_expected.to validate_presence_of :user_id }
it { is_expected.to validate_presence_of :channel_id }
it { is_expected.to validate_presence_of :message_id }
end
@@ -18,17 +17,10 @@ RSpec.describe Chat::UpdateUserLastRead do
fab!(:message_1) { Fabricate(:chat_message, chat_channel: membership.chat_channel) }
let(:guardian) { Guardian.new(current_user) }
let(:params) do
{
guardian: guardian,
user_id: current_user.id,
channel_id: channel.id,
message_id: message_1.id,
}
end
let(:params) { { guardian: guardian, channel_id: channel.id, message_id: message_1.id } }
context "when params are not valid" do
before { params.delete(:user_id) }
before { params.delete(:message_id) }
it { is_expected.to fail_a_contract }
end
@@ -37,7 +29,7 @@ RSpec.describe Chat::UpdateUserLastRead do
context "when user has no membership" do
before { membership.destroy! }
it { is_expected.to fail_to_find_a_model(:membership) }
it { is_expected.to fail_to_find_a_model(:active_membership) }
end
context "when user can’t access the channel" do
@@ -56,8 +48,10 @@ RSpec.describe Chat::UpdateUserLastRead do
context "when message_id is older than membership's last_read_message_id" do
before do
params[:message_id] = -2
membership.update!(last_read_message_id: -1)
message_old = Fabricate(:chat_message, chat_channel: channel)
message_new = Fabricate(:chat_message, chat_channel: channel)
params[:message_id] = message_old.id
membership.update!(last_read_message_id: message_new.id)
end
it { is_expected.to fail_a_policy(:ensure_message_id_recency) }

43
plugins/chat/spec/system/shortcuts/mark_all_read_spec.rb Normal file
View File

@@ -0,0 +1,43 @@
# frozen_string_literal: true
RSpec.describe "Shortcuts | mark all read", type: :system, js: true do
fab!(:user_1) { Fabricate(:admin) }
fab!(:channel_1) { Fabricate(:chat_channel) }
fab!(:channel_2) { Fabricate(:chat_channel) }
fab!(:channel_3) { Fabricate(:chat_channel) }
let(:chat_page) { PageObjects::Pages::Chat.new }
let(:drawer) { PageObjects::Pages::ChatDrawer.new }
before do
SiteSetting.navigation_menu = "sidebar"
chat_system_bootstrap(user_1, [channel_1, channel_2, channel_3])
sign_in(user_1)
Fabricate(:chat_message, chat_channel: channel_1)
Fabricate(:chat_message, chat_channel: channel_1)
Fabricate(:chat_message, chat_channel: channel_2)
Fabricate(:chat_message, chat_channel: channel_2)
end
context "when chat is open" do
before { visit(channel_3.url) }
context "when pressing shift+esc" do
it "marks all channels read" do
expect(page).to have_css(
".sidebar-section-link.channel-#{channel_1.id} .sidebar-section-link-suffix.unread",
)
expect(page).to have_css(
".sidebar-section-link.channel-#{channel_2.id} .sidebar-section-link-suffix.unread",
)
find("body").send_keys(%i[shift escape])
expect(page).not_to have_css(
".sidebar-section-link.channel-#{channel_1.id} .sidebar-section-link-suffix.unread",
)
expect(page).not_to have_css(
".sidebar-section-link.channel-#{channel_2.id} .sidebar-section-link-suffix.unread",
)
end
end
end
end