Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FIX: ensures users can open channel invites (#24067) 

We were incorrectly generating URLs with message id even when it was not provided, resulting in a route ending with "undefined", which was causing an error.

This commit also uses this opportunity to:
- move `invite_users` into a proper controller inside the API namespace
- refactors the code into a service: `Chat::InviteUsersToChannel`
This commit is contained in:
Joffrey JAFFEUX 2023-10-24 18:51:33 +02:00 committed by GitHub
parent 930dc38500
commit 5fec841c19
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
16 changed files with 355 additions and 175 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
plugins/chat/app/controllers/chat/api/channels_invites_controller.rb Normal file
View File

@ -0,0 +1,10 @@
# frozen_string_literal: true
class Chat::Api::ChannelsInvitesController < Chat::ApiController
def create
with_service(Chat::InviteUsersToChannel) do
on_failed_policy(:can_view_channel) { raise Discourse::InvalidAccess }
on_model_not_found(:channel) { raise Discourse::NotFound }
end
end
end

31
plugins/chat/app/controllers/chat/chat_controller.rb
View File

@ -114,37 +114,6 @@ module Chat
render json: { chat_enabled: current_user.user_option.chat_enabled }
end
def invite_users
params.require(:user_ids)
users =
User
.includes(:groups)
.joins(:user_option)
.where(user_options: { chat_enabled: true })
.not_suspended
.where(id: params[:user_ids])
users.each do |user|
if user.guardian.can_join_chat_channel?(@chat_channel)
data = {
message: "chat.invitation_notification",
chat_channel_id: @chat_channel.id,
chat_channel_title: @chat_channel.title(user),
chat_channel_slug: @chat_channel.slug,
invited_by_username: current_user.username,
}
data[:chat_message_id] = params[:chat_message_id] if params[:chat_message_id]
user.notifications.create(
notification_type: Notification.types[:chat_invitation],
high_priority: true,
data: data.to_json,
)
end
end
render json: success_json
end
def dismiss_retention_reminder
params.require(:chatable_type)
guardian.ensure_can_chat!

2
plugins/chat/app/services/chat/create_thread.rb
View File

@ -9,7 +9,7 @@ module Chat
class CreateThread
include Service::Base
# @!method call(thread_id:, channel_id:, guardian:, **params_to_edit)
# @!method call(thread_id:, channel_id:, guardian:, **params_to_create)
# @param [Integer] original_message_id
# @param [Integer] channel_id
# @param [Guardian] guardian

76
plugins/chat/app/services/chat/invite_users_to_channel.rb Normal file
View File

@ -0,0 +1,76 @@
# frozen_string_literal: true
module Chat
# Invites users to a channel.
#
# @example
# Chat::InviteUsersToChannel.call(channel_id: 2, user_ids: [2, 43], guardian: guardian, **optional_params)
#
class InviteUsersToChannel
include Service::Base
# @!method call(user_ids:, channel_id:, guardian:)
# @param [Array<Integer>] user_ids
# @param [Integer] channel_id
# @param [Guardian] guardian
# @option optional_params [Integer, nil] message_id
# @return [Service::Base::Context]
contract
model :channel
policy :can_view_channel
model :users, optional: true
step :send_invite_notifications
# @!visibility private
class Contract
attribute :user_ids, :array
validates :user_ids, presence: true
attribute :channel_id, :integer
validates :channel_id, presence: true
attribute :message_id, :integer
end
private
def fetch_channel(contract:, **)
::Chat::Channel.find_by(id: contract.channel_id)
end
def can_view_channel(guardian:, channel:, **)
guardian.can_preview_chat_channel?(channel)
end
def fetch_users(contract:, **)
::User
.joins(:user_option)
.where(user_options: { chat_enabled: true })
.not_suspended
.where(id: contract.user_ids)
.limit(50)
end
def send_invite_notifications(channel:, guardian:, users:, contract:, **)
users&.each do |invited_user|
next if !invited_user.guardian.can_join_chat_channel?(channel)
data = {
message: "chat.invitation_notification",
chat_channel_id: channel.id,
chat_channel_title: channel.title(invited_user),
chat_channel_slug: channel.slug,
invited_by_username: guardian.user.username,
}
data[:chat_message_id] = contract.message_id if contract.message_id
invited_user.notifications.create(
notification_type: ::Notification.types[:chat_invitation],
high_priority: true,
data: data.to_json,
)
end
end
end
end

2
plugins/chat/assets/javascripts/discourse/components/chat-channel.js
View File

@ -299,7 +299,7 @@ export default class ChatChannel extends Component {
let foundFirstNew = false;
const hasNewest = this.messagesManager.messages.some((m) => m.newest);
result.messages.forEach((messageData, index) => {
result?.messages?.forEach((messageData, index) => {
messageData.firstOfResults = index === 0;
if (this.currentUser.ignored_users) {

5
plugins/chat/assets/javascripts/discourse/components/full-page-chat.hbs
View File

@ -1,6 +1,3 @@
{{#if @channel.id}}
<ChatChannel
@channel={{@channel}}
@targetMessageId={{readonly @targetMessageId}}
/>
<ChatChannel @channel={{@channel}} @targetMessageId={{@targetMessageId}} />
{{/if}}

28
plugins/chat/assets/javascripts/discourse/initializers/chat-user-menu.js
View File

@ -18,22 +18,24 @@ export default {
"chat_invitation",
(NotificationItemBase) => {
return class extends NotificationItemBase {
linkTitle = I18n.t("notifications.titles.chat_invitation");
icon = "link";
description = I18n.t("notifications.chat_invitation");
get linkHref() {
const data = this.notification.data;
const slug = slugifyChannel({
title: this.notification.data.chat_channel_title,
slug: this.notification.data.chat_channel_slug,
title: data.chat_channel_title,
slug: data.chat_channel_slug,
});
return `/chat/c/${slug || "-"}/${
this.notification.data.chat_channel_id
}/${this.notification.data.chat_message_id}`;
}
get linkTitle() {
return I18n.t("notifications.titles.chat_invitation");
}
let url = `/chat/c/${slug || "-"}/${data.chat_channel_id}`;
get icon() {
return "link";
if (data.chat_message_id) {
url += `/${data.chat_message_id}`;
}
return url;
}
get label() {
@ -41,10 +43,6 @@ export default {
this.notification.data.invited_by_username
);
}
get description() {
return I18n.t("notifications.chat_invitation");
}
};
}
);

6
plugins/chat/assets/javascripts/discourse/services/chat-api.js
View File

@ -473,9 +473,9 @@ export default class ChatApi extends Service {
* @param {number} options.chat_message_id - A message ID to display in the invite.
*/
invite(channelId, userIds, options = {}) {
return ajax(`/chat/${channelId}/invite`, {
type: "put",
data: { user_ids: userIds, chat_message_id: options.messageId },
return this.#postRequest(`/channels/${channelId}/invites`, {
user_ids: userIds,
message_id: options.messageId,
});
}

11
plugins/chat/assets/javascripts/discourse/widgets/chat-invitation-notification-item.js
View File

@ -38,8 +38,13 @@ createWidgetFrom(DefaultNotificationItem, "chat-invitation-notification-item", {
title: data.chat_channel_title,
slug: data.chat_channel_slug,
});
return `/chat/c/${slug || "-"}/${data.chat_channel_id}/${
data.chat_message_id
}`;
let url = `/chat/c/${slug || "-"}/${data.chat_channel_id}`;
if (data.chat_message_id) {
url += `/${data.chat_message_id}`;
}
return url;
},
});

2
plugins/chat/config/routes.rb
View File

@ -15,6 +15,7 @@ Chat::Engine.routes.draw do
get "/channels/:channel_id/messages" => "channel_messages#index"
put "/channels/:channel_id/messages/:message_id" => "channel_messages#update"
post "/channels/:channel_id/messages/moves" => "channels_messages_moves#create"
post "/channels/:channel_id/invites" => "channels_invites#create"
post "/channels/:channel_id/archives" => "channels_archives#create"
get "/channels/:channel_id/memberships" => "channels_memberships#index"
delete "/channels/:channel_id/memberships/me" => "channels_current_user_membership#destroy"
@ -78,7 +79,6 @@ Chat::Engine.routes.draw do
post "/:chat_channel_id/:message_id/flag" => "chat#flag"
post "/:chat_channel_id/quote" => "chat#quote_messages"
put "/user_chat_enabled/:user_id" => "chat#set_user_chat_status"
put "/:chat_channel_id/invite" => "chat#invite_users"
post "/drafts" => "chat#set_draft"
post "/:chat_channel_id" => "api/channel_messages#create"
put "/flag" => "chat#flag"

73
plugins/chat/spec/requests/chat/api/channels_invites_controller_spec.rb Normal file
View File

@ -0,0 +1,73 @@
# frozen_string_literal: true
require "rails_helper"
RSpec.describe Chat::Api::ChannelsInvitesController do
fab!(:current_user) { Fabricate(:user) }
fab!(:channel_1) { Fabricate(:chat_channel) }
fab!(:user_1) { Fabricate(:user) }
fab!(:user_2) { Fabricate(:user) }
before do
SiteSetting.chat_enabled = true
SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone]
channel_1.add(current_user)
sign_in(current_user)
end
describe "create" do
describe "success" do
it "works" do
expect {
post "/chat/api/channels/#{channel_1.id}/invites?user_ids=#{user_1.id},#{user_2.id}"
}.to change {
Notification.where(
notification_type: Notification.types[:chat_invitation],
user_id: [user_1.id, user_2.id],
).count
}.by(2)
expect(response.status).to eq(200)
end
end
describe "missing user_ids" do
fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
it "returns a 400" do
post "/chat/api/channels/#{channel_1.id}/invites"
expect(response.status).to eq(400)
end
end
describe "message_id param" do
fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
it "works" do
post "/chat/api/channels/#{channel_1.id}/invites?user_ids=#{user_1.id},#{user_2.id}&message_id=#{message_1.id}"
expect(JSON.parse(Notification.last.data)["chat_message_id"]).to eq(message_1.id)
expect(response.status).to eq(200)
end
end
describe "current user can't view channel" do
fab!(:channel_1) { Fabricate(:private_category_channel) }
it "returns a 403" do
post "/chat/api/channels/#{channel_1.id}/invites?user_ids=#{user_1.id},#{user_2.id}"
expect(response.status).to eq(403)
end
end
describe "channel doesn’t exist" do
it "returns a 404" do
post "/chat/api/channels/-1/invites?user_ids=#{user_1.id},#{user_2.id}"
expect(response.status).to eq(404)
end
end
end
end

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

@ -370,61 +370,6 @@ RSpec.describe Chat::ChatController do
end
end
describe "invite_users" do
fab!(:chat_channel) { Fabricate(:category_channel) }
fab!(:chat_message) { Fabricate(:chat_message, chat_channel: chat_channel, user: admin) }
fab!(:user2) { Fabricate(:user) }
before do
sign_in(admin)
[user, user2].each { |u| u.user_option.update(chat_enabled: true) }
end
it "doesn't invite users who cannot chat" do
SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:admins]
expect {
put "/chat/#{chat_channel.id}/invite.json", params: { user_ids: [user.id] }
}.not_to change {
user.notifications.where(notification_type: Notification.types[:chat_invitation]).count
}
end
it "creates an invitation notification for users who can chat" do
expect {
put "/chat/#{chat_channel.id}/invite.json", params: { user_ids: [user.id] }
}.to change {
user.notifications.where(notification_type: Notification.types[:chat_invitation]).count
}.by(1)
notification =
user.notifications.where(notification_type: Notification.types[:chat_invitation]).last
parsed_data = JSON.parse(notification[:data])
expect(parsed_data["chat_channel_title"]).to eq(chat_channel.title(user))
expect(parsed_data["chat_channel_slug"]).to eq(chat_channel.slug)
end
it "creates multiple invitations" do
expect {
put "/chat/#{chat_channel.id}/invite.json", params: { user_ids: [user.id, user2.id] }
}.to change {
Notification.where(
notification_type: Notification.types[:chat_invitation],
user_id: [user.id, user2.id],
).count
}.by(2)
end
it "adds chat_message_id when param is present" do
put "/chat/#{chat_channel.id}/invite.json",
params: {
user_ids: [user.id],
chat_message_id: chat_message.id,
}
expect(JSON.parse(Notification.last.data)["chat_message_id"]).to eq(chat_message.id.to_s)
end
end
describe "#dismiss_retention_reminder" do
it "errors for anon" do
post "/chat/dismiss-retention-reminder.json", params: { chatable_type: "Category" }

90
plugins/chat/spec/services/chat/invite_users_to_channel_spec.rb Normal file
View File

@ -0,0 +1,90 @@
# frozen_string_literal: true
RSpec.describe Chat::InviteUsersToChannel do
subject(:result) { described_class.call(params) }
describe described_class::Contract, type: :model do
subject(:contract) { described_class.new }
it { is_expected.to validate_presence_of :channel_id }
it { is_expected.to validate_presence_of :user_ids }
end
fab!(:current_user) { Fabricate(:admin) }
fab!(:user_1) { Fabricate(:user) }
fab!(:user_2) { Fabricate(:user) }
fab!(:channel_1) { Fabricate(:chat_channel) }
fab!(:group_1) { Fabricate(:group) }
let(:guardian) { current_user.guardian }
let(:user_ids) { [user_1.id, user_2.id] }
let(:message_id) { nil }
let(:params) do
{ guardian: guardian, channel_id: channel_1.id, message_id: message_id, user_ids: user_ids }
end
before do
group_1.add(user_1)
SiteSetting.chat_allowed_groups = [group_1].map(&:id).join("|")
end
context "when all steps pass" do
it "sets the service result as successful" do
expect(result).to run_service_successfully
end
it "creates the notifications for allowed users" do
result
notification = user_1.reload.notifications.last
expect(notification.notification_type).to eq(::Notification.types[:chat_invitation])
notification = user_2.reload.notifications.last
expect(notification).to be_nil
end
it "doesnt create notifications for suspended users" do
user_1.update!(suspended_till: 2.days.from_now, suspended_at: Time.now)
result
notification = user_1.reload.notifications.last
expect(notification).to be_nil
end
it "doesnt create notifications for users with disabled chat" do
user_1.user_option.update!(chat_enabled: false)
result
notification = user_1.reload.notifications.last
expect(notification).to be_nil
end
context "when message id is provided" do
fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
let(:message_id) { message_1.id }
it "sets the message id on the notification" do
result
data = JSON.parse(user_1.reload.notifications.last.data)
expect(data["chat_message_id"]).to eq(message_id)
end
end
end
context "when channel model is not found" do
before { params[:channel_id] = -1 }
it { is_expected.to fail_to_find_a_model(:channel) }
end
context "when current user can't view channel" do
fab!(:current_user) { Fabricate(:user) }
fab!(:channel_1) { Fabricate(:private_category_channel) }
it { is_expected.to fail_a_policy(:can_view_channel) }
end
end

56
plugins/chat/spec/system/invite_users_to_channel_spec.rb Normal file
View File

@ -0,0 +1,56 @@
# frozen_string_literal: true
RSpec.describe "Invite users to channel", type: :system do
fab!(:user_1) { Fabricate(:user) }
fab!(:channel_1) { Fabricate(:category_channel) }
let(:user_menu) { PageObjects::Components::UserMenu.new }
let(:chat_drawer_page) { PageObjects::Pages::ChatDrawer.new }
before do
chat_system_bootstrap
channel_1.add(user_1)
sign_in(user_1)
end
context "when user clicks the invitation" do
context "when the invitation is linking to a channel" do
before do
Chat::InviteUsersToChannel.call(
channel_id: channel_1.id,
user_ids: [user_1.id],
guardian: Guardian.new(Fabricate(:admin)),
)
end
it "loads the channel" do
visit("/")
user_menu.open
find("[title='#{I18n.t("js.notifications.titles.chat_invitation")}']").click
expect(chat_drawer_page).to have_open_channel(channel_1)
end
end
context "when the invitation is linking to a message" do
fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
before do
Chat::InviteUsersToChannel.call(
channel_id: channel_1.id,
user_ids: [user_1.id],
guardian: Guardian.new(Fabricate(:admin)),
message_id: message_1.id,
)
end
it "loads the channel" do
visit("/")
user_menu.open
find("[title='#{I18n.t("js.notifications.titles.chat_invitation")}']").click
expect(chat_drawer_page).to have_open_channel(channel_1)
end
end
end
end

81
plugins/chat/spec/system/message_notifications_with_sidebar_spec.rb
View File

@ -12,12 +12,7 @@ RSpec.describe "Message notifications - with sidebar", type: :system do
end
def create_message(text: nil, channel:, creator: Fabricate(:user))
sign_in(creator)
chat_page.visit_channel(channel)
channel_page.send_message(text)
args = { persisted: true }
args[:text] = text if text
expect(channel_page.messages).to have_message(**args)
Fabricate(:chat_message_with_service, chat_channel: channel, user: creator, message: text)
end
context "as a user" do
@ -35,11 +30,7 @@ RSpec.describe "Message notifications - with sidebar", type: :system do
context "when a message is created" do
it "doesn't show anything" do
visit("/")
using_session(:user_1) do |session|
create_message(channel: channel_1, creator: user_1)
session.quit
end
create_message(channel: channel_1, creator: user_1)
expect(page).to have_no_css(".chat-header-icon .chat-channel-unread-indicator")
expect(page).to have_no_css(".sidebar-row.channel-#{channel_1.id}")
@ -62,12 +53,8 @@ RSpec.describe "Message notifications - with sidebar", type: :system do
it "doesn’t show indicator in header" do
Jobs.run_immediately!
visit("/")
using_session(:user_1) do |session|
create_message(channel: channel_1, creator: user_1)
session.quit
end
create_message(channel: channel_1, creator: user_1)
expect(page).to have_css(".do-not-disturb-background")
expect(page).to have_no_css(".chat-header-icon .chat-channel-unread-indicator")
@ -80,10 +67,7 @@ RSpec.describe "Message notifications - with sidebar", type: :system do
context "when a message is created" do
it "doesn't show anything" do
visit("/")
using_session(:user_1) do |session|
create_message(channel: channel_1, creator: user_1)
session.quit
end
create_message(channel: channel_1, creator: user_1)
expect(page).to have_no_css(".chat-header-icon .chat-channel-unread-indicator")
expect(page).to have_no_css(".sidebar-row.channel-#{channel_1.id} .unread")
@ -102,10 +86,7 @@ RSpec.describe "Message notifications - with sidebar", type: :system do
context "when a message is created" do
it "doesn't show any indicator on chat-header-icon" do
visit("/")
using_session(:user_1) do |session|
create_message(channel: channel_1, creator: user_1)
session.quit
end
create_message(channel: channel_1, creator: user_1)
expect(page).to have_no_css(".chat-header-icon .chat-channel-unread-indicator")
end
@ -123,10 +104,7 @@ RSpec.describe "Message notifications - with sidebar", type: :system do
context "when a message is created" do
it "doesn't show any indicator on chat-header-icon" do
visit("/")
using_session(:user_1) do |session|
create_message(channel: channel_1, creator: user_1)
session.quit
end
create_message(channel: channel_1, creator: user_1)
expect(page).to have_no_css(
".chat-header-icon .chat-channel-unread-indicator.-urgent",
@ -137,15 +115,13 @@ RSpec.describe "Message notifications - with sidebar", type: :system do
context "when a message with a mention is created" do
it "does show an indicator on chat-header-icon" do
Jobs.run_immediately!
visit("/")
using_session(:user_1) do
create_message(
text: "hey what's going on @#{current_user.username}?",
channel: channel_1,
creator: user_1,
)
end
create_message(
text: "hey what's going on @#{current_user.username}?",
channel: channel_1,
creator: user_1,
)
expect(page).to have_css(".chat-header-icon .chat-channel-unread-indicator.-urgent")
end
end
@ -154,10 +130,7 @@ RSpec.describe "Message notifications - with sidebar", type: :system do
context "when a message is created" do
it "correctly renders notifications" do
visit("/")
using_session(:user_1) do |session|
create_message(channel: channel_1, creator: user_1)
session.quit
end
create_message(channel: channel_1, creator: user_1)
expect(page).to have_css(".chat-header-icon .chat-channel-unread-indicator", text: "")
expect(page).to have_css(".sidebar-row.channel-#{channel_1.id} .unread")
@ -167,15 +140,12 @@ RSpec.describe "Message notifications - with sidebar", type: :system do
context "when a message with mentions is created" do
it "correctly renders notifications" do
Jobs.run_immediately!
visit("/")
using_session(:user_1) do
create_message(
channel: channel_1,
creator: user_1,
text: "hello @#{current_user.username} what's up?",
)
end
create_message(
channel: channel_1,
creator: user_1,
text: "hello @#{current_user.username} what's up?",
)
expect(page).to have_css(
".chat-header-icon .chat-channel-unread-indicator.-urgent",
@ -198,18 +168,12 @@ RSpec.describe "Message notifications - with sidebar", type: :system do
context "when a message is created" do
it "correctly renders notifications" do
visit("/")
using_session(:user_1) do |session|
create_message(channel: dm_channel_1, creator: user_1)
session.quit
end
create_message(channel: dm_channel_1, creator: user_1)
expect(page).to have_css(".chat-header-icon .chat-channel-unread-indicator", text: "1")
expect(page).to have_css(".sidebar-row.channel-#{dm_channel_1.id} .icon.urgent")
using_session(:user_1) do |session|
create_message(channel: dm_channel_1, creator: user_1)
session.quit
end
create_message(channel: dm_channel_1, creator: user_1)
expect(page).to have_css(".chat-header-icon .chat-channel-unread-indicator", text: "2")
end
@ -224,10 +188,7 @@ RSpec.describe "Message notifications - with sidebar", type: :system do
"#sidebar-section-content-chat-dms .sidebar-section-link-wrapper:nth-child(2) .channel-#{dm_channel_2.id}",
)
using_session(:user_1) do |session|
create_message(channel: dm_channel_2, creator: user_2)
session.quit
end
create_message(channel: dm_channel_2, creator: user_2)
expect(page).to have_css(
"#sidebar-section-content-chat-dms .sidebar-section-link-wrapper:nth-child(1) .channel-#{dm_channel_2.id}",

2
plugins/chat/test/javascripts/components/chat-notices-test.js
View File

@ -91,7 +91,7 @@ module("Discourse Chat | Component | chat-notice", function (hooks) {
.dom(".mention-without-membership-notice__body__link")
.hasText(I18n.t("chat.mention_warning.invite"));
pretender.put(`/chat/${this.channel.id}/invite`, () => {
pretender.post(`/chat/api/channels/${this.channel.id}/invites`, () => {
return [200, { "Content-Type": "application/json" }, {}];
});