This commit implements all the necessary logic to create thread seamlessly. For this it relies on the same logic used for messages and generates a `staged-id`(using the format: `staged-thread-CHANNEL_ID-MESSAGE_ID` which is used to re-conciliate state client sides once the thread has been persisted on the backend.
Part of this change the client side is now always using real thread and channel objects instead of sometimes relying on a flat `threadId` or `channelId`.
This PR also brings three UX changes:
- thread starts from top
- number of buttons on message actions is dependent of the width of the enclosing container
- <kbd>shift + ArrowUp</kbd> will reply to the last message
* FIX: Link to thread for mentions inside thread
When mentioning a user in a thread, when we send the
notification and display it in the UI we want the URL
of the notification to point to the thread URL to open
the panel, rather than the main channel which is confusing.
For now, we don't have a way to highlight the linked-to message
in the thread, we can revisit this later.
* FIX: Mark mention notifications read when thread opens
Since we have no scrolling/message visibility/thread membership
for now, when a user opens the thread panel we just want to mark
all mention notifications relating to messages in the thread
for the user as read.
This was reverted in 38cebd3ed5.
The issue was that I was using Discourse.redis.delete_prefixed
which does a slow redis KEYS lookup, which is not advised in
production. This commit removes that, and also ensures the periodical
thread count update only happens if threading is enabled.
I changed to use a redis INCR/DECR for reply count
cache. This avoids a round trip to redis to GET the current
count, and also avoids multi-process issues, where
if there's two processes trying to increment at the
same time, they may both receive the same value, add one
to it, then both write the same value back.
Then, it's only n+1 instead of n+2.
This also prevents almost all chat scheduled jobs from
running if chat is disabled, the only one remaining is
the message retention job.
This commit moves the category channel creation out
of the Chat::Api::Channel controller and into a
dedicated CreateCategoryChannel service. A follow up
commit will move the DM channel creation out of
the old DirectMessageChannelCreator service.
Also includes a new on_model_errors helper
for chat service class usage, that collects model
validation errors to present in a nice way.
---------
Co-authored-by: Loïc Guitaut <loic@discourse.org>
We currently don't have a nice UI to show unread messages for the thread,
and it will take some time to create one. For now, this commit makes it so
new messages inside a thread do not count towards a chat channel's unread
counts, and new messages sent in a thread do not update a user's `last_read_message_id`
for a channel.
In addition, this PR refactors the `Chat::ChannelFetcher` to use the `Chat::ChannelUnreadsQuery`
query class for consistency, and made said class able to return zeroed-out records
for channels the user is not a member of.
Finally, a small bug is fixed here where if a user's `last_read_message_id` for
a channel was a thread's OM ID, then the thread OM would not show in the
main channel stream for them until another reply to the channel was posted.
This commit introduces a redis cache over the top of the thread
replies_count DB cache, so that we can quickly and accurately
increment/decrement the reply count for all users and not have
to constantly update the database-level count. This is done so
the UI can have a count that is displayed to the users on each
thread indicator, that appears to live update on each chat
message create/trash/recover inside the thread.
This commit also introduces the `Chat::RestoreMessage` service
and moves the restore endpoint into the `Api::ChannelMessages`
controller as part of incremental migrations to move things out
of ChatController.
Finally, this commit refactors `Chat::Publisher` to be less repetitive
with its `MessageBus` sending code.
When a chat message is trashed and the message is used
for someone's UserChatChannelMembership#last_read_message_id,
the user would end up with some read state issues until
someone posted a new message in the channel, since we didn't
clear it like we did on bulk message delete.
This commit fixes the issue, and also takes the opportunity
to start a MessagesController in the API namespace, and move
the trash message functionality into the new service format.
We keep getting this failure on the spec but I
cannot reproduce locally, add this extra log line
to see if it helps:
```
> Chat::Api::ChatablesController#index with chat permissions does not return DM channels for users who are not in the chat allowed group
> Failure/Error: example.run
>
> expected: 200
> got: 500
>
> (compared using ==)
> # ./plugins/chat/spec/requests/chat/api/chatables_controller_spec.rb:158:in `block (4 levels) in <main>'
> # ./spec/rails_helper.rb:358:in `block (2 levels) in <top (required)>'
> # ./vendor/bundle/ruby/3.2.0/gems/webmock-3.18.1/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'
> # ------------------
> # --- Caused by: ---
> #
> # expected: 200
> # got: 500
> #
> # (compared using ==)
> # ./plugins/chat/spec/requests/chat/api/chatables_controller_spec.rb:158:in `block (4 levels) in <main>'
```
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.
Prior to this change `registered_bookmarkable` would return `nil` as `type` in `Bookmark.registered_bookmarkable_from_type(type)` would be `ChatMessage` and we registered a `Chat::Message` class.
This commit will now properly rely on each model `polymorphic_class_for(name)` to help us infer the proper type from a a `bookmarkable_type`.
Tests have also been added to ensure that creating/destroying chat message bookmarks is working correctly.
---
Longer explanation
Currently when you save a bookmark in the database, it's associated to another object through a polymorphic relationship, which will is represented by two columns: `bookmarkable_id` and `bookmarkable_type`. The `bookmarkable_id` contains the id of the relationship (a post ID for example) and the `bookmarkable_type` contains the type of the object as a string by default, (`"Post"` for example).
Chat plugin just started namespacing objects, as a result a model named `ChatMessage` is now named `Chat::Message`, to avoid complex and risky migrations we rely on methods provided by rails to alter the `bookmarkable_type` when we save it: we want to still save it as `"ChatMessage"` and not `"Chat::Message"`. And, to retrieve the correct model when we load the bookmark from the database: we want `"ChatMessage"` to load the `Chat::Message` model and not the `ChatMessage`model which doesn't exist anymore.
On top of this the bookmark codepath is allowing plugins to register types and will check against these types, so we alter this code path to be able to do a similar ChatMessage <-> Chat::Message dance and allow to check the type is valid. In the specific case of this commit, we were retrieving a `"ChatMessage"` bookmarkable_type from the DB and looking for it in the registered bookmarkable types which contain `Chat::Message` and not `ChatMessage`.
This commit main goal was to comply with Zeitwerk and properly rely on autoloading. To achieve this, most resources have been namespaced under the `Chat` module.
- Given all models are now namespaced with `Chat::` and would change the stored types in DB when using polymorphism or STI (single table inheritance), this commit uses various Rails methods to ensure proper class is loaded and the stored name in DB is unchanged, eg: `Chat::Message` model will be stored as `"ChatMessage"`, and `"ChatMessage"` will correctly load `Chat::Message` model.
- Jobs are now using constants only, eg: `Jobs::Chat::Foo` and should only be enqueued this way
Notes:
- This commit also used this opportunity to limit the number of registered css files in plugin.rb
- `discourse_dev` support has been removed within this commit and will be reintroduced later
<!-- NOTE: All pull requests should have tests (rspec in Ruby, qunit in JavaScript). If your code does not include test coverage, please include an explanation of why it was omitted. -->
This PR is introducing glimmer usage in the chat-live-pane, for components but also for models. RestModel usage has been dropped in favor of native classes.
Other changes/additions in this PR:
sticky dates, scrolling will now keep the date separator of the current section at the top of the screen
better unread management, marking a channel as unread will correctly mark the correct message and not mark the whole channel as read. Tracking state will also now correctly return unread count and unread mentions.
adds an animation on bottom arrow
better scrolling behavior, we should now always correctly keep the scroll position while loading more
reactions are now more reactive, and will update their tooltip without needed to close/reopen it
skeleton has been improved with placeholder images and reactions
when making a reaction on the desktop message actions, the menu won't move anymore
simplify logic and stop maintaining a list of unloaded messages
This PR is introducing glimmer usage in the chat-live-pane, for components but also for models. RestModel usage has been dropped in favor of native classes.
Other changes/additions in this PR:
- sticky dates, scrolling will now keep the date separator of the current section at the top of the screen
- better unread management, marking a channel as unread will correctly mark the correct message and not mark the whole channel as read. Tracking state will also now correctly return unread count and unread mentions.
- adds an animation on bottom arrow
- better scrolling behavior, we should now always correctly keep the scroll position while loading more
- reactions are now more reactive, and will update their tooltip without needed to close/reopen it
- skeleton has been improved with placeholder images and reactions
- when making a reaction on the desktop message actions, the menu won't move anymore
- simplify logic and stop maintaining a list of unloaded messages
This PR is introducing glimmer usage in the chat-live-pane, for components but also for models. RestModel usage has been dropped in favor of native classes.
Other changes/additions in this PR:
- sticky dates, scrolling will now keep the date separator of the current section at the top of the screen
- better unread management, marking a channel as unread will correctly mark the correct message and not mark the whole channel as read. Tracking state will also now correctly return unread count and unread mentions.
- adds an animation on bottom arrow
- better scrolling behavior, we should now always correctly keep the scroll position while loading more
- reactions are now more reactive, and will update their tooltip without needed to close/reopen it
- skeleton has been improved with placeholder images and reactions
- when making a reaction on the desktop message actions, the menu won't move anymore
- simplify logic and stop maintaining a list of unloaded messages
* FIX: Use pluralized string
* REFACTOR: Fix misuse of pluralized string
* REFACTOR: Fix misuse of pluralized string
* DEV: Remove linting of `one` key in MessageFormat string, it doesn't work
* REFACTOR: Fix misuse of pluralized string
This also ensures that the URL works on subfolder and shows the site setting link only for admins instead of staff. The string is quite complicated, so the best option was to switch to MessageFormat.
* REFACTOR: Fix misuse of pluralized string
* FIX: Use pluralized string
This also ensures that the URL works on subfolder and shows the site setting link only for admins instead of staff.
* REFACTOR: Correctly pluralize reaction tooltips in chat
This also ensures that maximum 5 usernames are shown and fixes the number of "others" which was off by 1 if the current user reacted on a message.
* REFACTOR: Use translatable string as comma separator
* DEV: Add comment to translation to clarify the meaning of `%{identifier}`
* REFACTOR: Use translatable comma separator and use explicit interpolation keys
* REFACTOR: Don't interpolate lowercase channel status
* REFACTOR: Fix misuse of pluralized string
* REFACTOR: Don't interpolate channel status
* REFACTOR: Use %{count} interpolation key
* REFACTOR: Fix misuse of pluralized string
* REFACTOR: Correctly pluralize DM chat channel titles
Adds a new LookupThread class that handles finding the
thread based on thread + channel ID, checking permissions
and policy/contract checks.
Co-authored-by: Loïc Guitaut <loic@discourse.org>
This commit introduces the skeleton of the chat thread UI. The
structure of the components looks like this. Its done this way
so the side panel can be used for other things as well if we wish,
not just for threads:
```
.main-chat-outlet
<ChatLivePane />
<ChatSidePanel>
<-- rendered with {{outlet}} -->
<ChatThread />
</ChatSidePanel>
```
Later on the `ChatThreadList` will be rendered here as well.
Now, when you go to a channel you can open a thread by clicking
on either the Open Thread message action button or by clicking on
the reply indicator. This will take you to a route like `chat/c/:slug/:channelId/t/:threadId`.
This works on mobile as well.
This commit includes basic serializers and routes for threads,
as well as a new `ChatThreadsManager` service in JS that caches
threads for a channel the same way the channel threads manager does.
The chat messages inside the thread are intentionally left out
until a later PR.
**NOTE: These changes are gated behind the site setting enable_experimental_chat_threaded_discussions
and the threading_enabled boolean on a ChatChannel**
This is a combined work of Martin Brennan, Loïc Guitaut, and Joffrey Jaffeux.
---
This commit implements a base service object when working in chat. The documentation is available at https://discourse.github.io/discourse/chat/backend/Chat/Service.html
Generating documentation has been made as part of this commit with a bigger goal in mind of generally making it easier to dive into the chat project.
Working with services generally involves 3 parts:
- The service object itself, which is a series of steps where few of them are specialized (model, transaction, policy)
```ruby
class UpdateAge
include Chat::Service::Base
model :user, :fetch_user
policy :can_see_user
contract
step :update_age
class Contract
attribute :age, :integer
end
def fetch_user(user_id:, **)
User.find_by(id: user_id)
end
def can_see_user(guardian:, **)
guardian.can_see_user(user)
end
def update_age(age:, **)
user.update!(age: age)
end
end
```
- The `with_service` controller helper, handling success and failure of the service within a service and making easy to return proper response to it from the controller
```ruby
def update
with_service(UpdateAge) do
on_success { render_serialized(result.user, BasicUserSerializer, root: "user") }
end
end
```
- Rspec matchers and steps inspector, improving the dev experience while creating specs for a service
```ruby
RSpec.describe(UpdateAge) do
subject(:result) do
described_class.call(guardian: guardian, user_id: user.id, age: age)
end
fab!(:user) { Fabricate(:user) }
fab!(:current_user) { Fabricate(:admin) }
let(:guardian) { Guardian.new(current_user) }
let(:age) { 1 }
it { expect(user.reload.age).to eq(age) }
end
```
Note in case of unexpected failure in your spec, the output will give all the relevant information:
```
1) UpdateAge when no channel_id is given is expected to fail to find a model named 'user'
Failure/Error: it { is_expected.to fail_to_find_a_model(:user) }
Expected model 'foo' (key: 'result.model.user') was not found in the result object.
[1/4] [model] 'user' ❌
[2/4] [policy] 'can_see_user'
[3/4] [contract] 'default'
[4/4] [step] 'update_age'
/Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/update_age.rb:32:in `fetch_user': missing keyword: :user_id (ArgumentError)
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:202:in `instance_exec'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:202:in `call'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:219:in `call'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:417:in `block in run!'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:417:in `each'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:417:in `run!'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:411:in `run'
from <internal:kernel>:90:in `tap'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:302:in `call'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/spec/services/update_age_spec.rb:15:in `block (3 levels) in <main>'
```
Triggers a DiscourseEvent when a message is deleted, similar to
`:chat_message_created` and `:chat_message_edited`. This is not used
in this plugin, but can be used by other plugins to act when a message
is trashed.
Deleting a message with a mention doesn't clear the associated notification, confusing the mentioned user.
There are different chat notification types, but we only care about `chat_mentioned` since `chat_quoted` is associated with a post, and `chat_message` is only for push notifications.
Unfortunately, this change doesn't fix the chat bubble getting out of sync when a message gets deleted since we track unread/mentions count with an integer, making it a bit hard to manipulate. We can follow up later if we consider it necessary.
This commit introduces the ability to edit the channel
slug from the About tab for the chat channel when the user
is admin. Similar to the create channel modal functionality
introduced in 641e94f, if
the slug is left empty then we autogenerate a slug based
on the channel name, and if the user just changes the slug
manually we use that instead.
We do not do any link remapping or anything else of the
sort, when the category slug is changed that does not happen
either.
Only allow maximum of `50_000` characters for chat drafts. A hidden `max_chat_draft_length` setting can control this limit. A migration is also provided to delete any abusive draft in the database.
The number of drafts loaded on current user has also been limited and ordered by most recent update.
Note that spec files moved are not directly related to the fix.
Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
Co-authored-by: Régis Hanol <regis@hanol.fr>
Prior to this fix trashed channels would still prevent a channel with the same slug to be created. This commit generates a new slug on trash and frees the slug for future usage.
The format used for the slug is: `YYYYMMDD-HHMM-OLD_SLUG-deleted` truncated to the max length of a channel name.
This commit allows us to set the channel slug when creating new chat
channels. As well as this, it introduces a new `SlugsController` which can
generate a slug using `Slug.for` and a name string for input. We call this
after the user finishes typing the channel name (debounced) and fill in
the autogenerated slug in the background, and update the slug input
placeholder.
This autogenerated slug is used by default, but if the user writes anything
else in the input it will be used instead.
There was an issue with channel archiving, where at times the topic
creation could fail which left the archive in a bad state, as read-only
instead of archived. This commit does several things:
* Changes the ChatChannelArchiveService to validate the topic being
created first and if it is not valid report the topic creation errors
in the PM we send to the user
* Changes the UI message in the channel with the archive status to reflect
that topic creation failed
* Validate the new topic when starting the archive process from the UI,
and show the validation errors to the user straight away instead of
creating the archive record and starting the process
This also fixes another issue in the discourse_dev config which was
failing because YAML parsing does not enable all classes by default now,
which was making the seeding rake task for chat fail.
* FIX: Channel archive N1 when serializing current user
The `ChatChannelSerializer` serializes the archive for the
channel if it is present, however this was causing an N1 for
the current user serializer in the case of DM channels, which
were not doing `includes(:chat_channel_archive)` in the
`ChatChannelFetcher`.
DM channels cannot be archived, so we can just never try to serialize
the archive for DM channels in `ChatChannelSerializer`, which
removes the N1.
* DEV: Add N1 performance spec for latest.html preloading
We modify current user serializer in chat, so it's a good
idea to have some N1 performance specs to avoid regressions
here.
Note this is a very large PR, and some of it could have been splited, but keeping it one chunk made it to merge conflicts and to revert if necessary. Actual new code logic is also not that much, as most of the changes are removing js tests, adding system specs or moving things around.
To make it possible this commit is doing the following changes:
- converting (and adding new) existing js acceptances tests into system tests. This change was necessary to ensure as little regressions as possible while changing paradigm
- moving away from store. Using glimmer and tracked properties requires to have class objects everywhere and as a result works well with models. However store/adapters are suffering from many bugs and limitations. As a workaround the `chat-api` and `chat-channels-manager` are an answer to this problem by encapsulating backend calls and frontend storage logic; while still using js models.
- dropping `appEvents` as much as possible. Using tracked properties and a better local storage of channel models, allows to be much more reactive and doesn’t require arbitrary manual updates everywhere in the app.
- while working on replacing store, the existing work of a chat api (backend) has been continued to support more cases.
- removing code from the `chat` service to separate concerns, `chat-subscriptions-manager` and `chat-channels-manager`, being the largest examples of where the code has been rewritten/moved.
Future wok:
- improve behavior when closing/deleting a channel, it's already slightly buggy on live, it's rare enough that it's not a big issue, but should be improved
- improve page objects used in chat
- move more endpoints to the API
- finish temporarily skipped tests
- extract more code from the `chat` service
- use glimmer for `chat-messages`
- separate concerns in `chat-live-pane`
- eventually add js tests for `chat-api`, `chat-channels-manager` and `chat-subscriptions-manager`, they are indirectly heavy tested through system tests but it would be nice to at least test the public API
<!-- NOTE: All pull requests should have tests (rspec in Ruby, qunit in JavaScript). If your code does not include test coverage, please include an explanation of why it was omitted. -->
Previously, restricted category chat channel was available for all groups - even `readonly`. From now on, only user who belong to group with `create_post` or `full` permissions can access that chat channel.
* FEATURE: Enforce mention limits for chat messages
The first part of these changes adds a new setting called `max_mentions_per_chat_message`, which skips notifications when the message contains too many mentions. It also respects the `max_users_notified_per_group_mention` setting
and skips notifications if expanding a group mention would exceed it.
We also include a new component to display JIT warning for these limits to the user while composing a message.
* Simplify ignoring/muting filter in chat_notifier
* Post-send warnings for unsent warnings
* Improve pluralization
* Address review feedback
* Fix test
* Address second feedback round
* Third round of feedback
Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
The settings tab of each category channel should now present the option to allow or disallow channel wide mentions: @here and @all.
When disallowed, using these mentions in the channel should have no effect.
This commit adds variousMessageBus.last_ids to serializer payloads
for chat channels and the chat view (for chat live pane) so
we can use those IDs when subscribing to MessageBus channels
from chat.
This allows us to ensure that any messages created between the
server being hit and the UI loaded and subscribing end up being
delivered to the client, rather than just silently dropped.
This commit also fixes an issue where we were subscribing to
the new-messages and new-mentions MessageBus channels multiple
times when following/unfollowing a channel multiple times.
Only allow maximum of 6000 characters for chat messages when they
are created or edited. A hidden setting can control this limit,
6000 is the default.
There is also a migration here to truncate any existing messages to
6000 characters if the message is already over that and if the
chat_messages table exists. We also set cooked_version to NULL
for those messages so we can identify them for rebake.
Currently when a category is deleted, if it has an associated chat
channel, the latter won’t be deleted automatically.
The fix is quite simple as we were simply missing a `dependent:
:destroy` option on the existing relation.
This commit automatically ensures that category channels
have slugs when they are created or updated based on the
channel name, category name, or existing slug. The behaviour
has been copied from the Category model.
We also include a backfill here with a simplified version
of Slug.for with deduplication to fill the slugs for already
created Category chat channels.
The channel slug is also now used for chat notifications,
and for the UI and navigation for chat. `slugifyChannel`
is still used, but now does the following fallback:
* Uses channel.slug if it is present
* Uses channel.escapedTitle if it is present
* Uses channel.title if it is present
In future we may want to remove this altogether
and always rely on the slug being present, but this
is currently not possible because we are not generating
slugs for DM channels at this point.
Currently it’s not possible to delete a category if an associated chat
channel is present even if there are no messages in this channel.
This can lead to annoying situations for our users.
This patch addresses the issue by checking if the channel is empty
instead of just checking if there is a channel.
Follow up to 766bcbc684
This fixes a gaffe from that commit where I passed in the
guardian to ChatMessageUpdater but then forgot to remove
the old way of setting the guardian and user instance variables
from the chat_message that was passed in.
Also, it moves the ensure_can_edit_message! check from the
controller into ChatMessageUpdater so all the access
checks are in the same place.
This is a followup of the previous refactor where we created two new
models to handle all the dedicated logic that was present in the
`ChatChannel` model.
For the sake of consistency, `DMChannel` has been renamed to
`DirectMessageChannel` and the previous `DirectMessageChannel` model is
now named `DirectMessage`. This should help reasoning about direct
messages.
