Further followup to 24ec06ff85,
where I prevented other chat scheduled jobs from running if
chat was disabled. We should not be running any plugin scheduled
jobs if that plugin is disabled, it can cause unexpected
behaviour.
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>
This adds these two new test cases:
context "when updating a mentioned user" do
it "updates the mention record" do
# and
context "when there are duplicate mentions" do
it "creates a single mention record per mention" do
Apart from that, this groups mention related tests into a context, renames one test, and moves setup of another test into the test case itself from the before block (to make it more clear, that test is the only one that uses that setup). See the PR's commit history.
Steps to reproduce the bug:
1. Send a chat message
2. Edit the message and add a mention to it
3. The mentioned user won't receive a notification
This PR fixes the problem.
Also:
1. There's no need anymore to have a code for removing notifications in the `notify_edit` method, because a call to `@chat_message.update_mentions` in the first line of the `notify_edit` method does that job:
ff56f403a2/plugins/chat/lib/chat/notifier.rb (L90)
2. There's no need to load mention records from database, it's enough to pluck user ids
Followup to bd5c5c4b5f, a
bug was introduced there for any channel that did not have
threading enabled or sites with the experimental threading
disabled. When the user replied to another chat message,
since this is always a thread in the background, we weren't
sending any MessageBus messages to the main channel, since
the message was a thread reply.
However in the UI these messages still show in the main stream
of the channel if threading is turned off, so the UI was not
reacting to these things happening in the backend. The worst
issue was that new clients would not see new replies sent in
reply to other messages in the channel.
This commit fixes an issue where if you pressed a format
shortcut (e.g. bold, italic, code) for the composer and
you had the thread panel open as well, the shortcut would
trigger in both composers, not just the one that was focused.
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.
Followup to bd5c5c4b5f,
this commit hooks up the bulk delete events for chat
messages inside the thread panel, by fanning out the
deleted message IDs based on whether they belong to
a thread or not.
Also adds a system spec to cover this case, as previously
the bulk delete event would have been broken with an incorrect
`typ` rather than `type` hash key.
Followup to ba11cf4767,
this commit makes it so that none of the chat message
thread data is serialized if threading_enabled is false
for the channel or if enable_experimental_chat_threaded_discussions
is false, there is no need to serialize the data in this
case.
This regression happened in bd5c5c4b5f and is due to `message_bus_targets = calculate_publish_targets(chat_channel, chat_message)` expecting a `chat_channel` which was only defined after.
Example exception in logs:
```
Job exception: undefined local variable or method `chat_channel' for Chat::Publisher:Module
/var/www/discourse/plugins/chat/app/services/chat/publisher.rb:91:in `publish_processed!'
/var/www/discourse/plugins/chat/app/jobs/regular/chat/process_message.rb:21:in `block in execute'
/var/www/discourse/lib/distributed_mutex.rb:53:in `block in synchronize'
/var/www/discourse/lib/distributed_mutex.rb:49:in `synchronize'
/var/www/discourse/lib/distributed_mutex.rb:49:in `synchronize'
/var/www/discourse/lib/distributed_mutex.rb:34:in `synchronize'
/var/www/discourse/plugins/chat/app/jobs/regular/chat/process_message.rb:7:in `execute'
/var/www/discourse/app/jobs/base.rb:249:in `block (2 levels) in perform'
```
This commit also:
- adds a spec to ensure oneboxing is not regressing anymore
- increment the version on message processed to ensure callbacks are correctly ran
Note we should also have more tests in `Chat::Publisher`, this will be done when we move it to a proper service.
<!-- 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 commit introduces a ChatChannelPaneSubscriptionsManager
and a ChatChannelThreadPaneSubscriptionsManager that inherits
from the first service that handle MessageBus subscriptions
for the main channel and the thread panel respectively.
This necessitated a change to Chat::Publisher to be able to
send MessageBus messages to multiple channels based on whether
a message was an OM for a thread, a thread reply, or a regular
channel message.
An initial change to update the thread indicator with new replies
has been done too, but that will be improved in future as we have
more data to update on the indicators.
Still remaining is to fully move over the handleSentMessage
functionality which includes scrolling and new message indicator
things.
Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
- Back button in drawer will bring you back to channel
- Larger font for thread indicator
- Prevents screen flashing due to clearing messages when they were already loaded
- Fixes a bug where did-update params were inverted causing an error when expanding/collapsing drawer
This feature will allow sites to define which emoji are not allowed. Emoji in this list should be excluded from the set we show in the core emoji picker used in the composer for posts when emoji are enabled. And they should not be allowed to be chosen to be added to messages or as reactions in chat.
This feature prevents denied emoji from appearing in the following scenarios:
- topic title and page title
- private messages (topic title and body)
- inserting emojis into a chat
- reacting to chat messages
- using the emoji picker (composer, user status etc)
- using search within emoji picker
It also takes into account the various ways that emojis can be accessed, such as:
- emoji autocomplete suggestions
- emoji favourites (auto populates when adding to emoji deny list for example)
- emoji inline translations
- emoji skintones (ie. for certain hand gestures)
This commit introduces a new thread indicator for channels with `threading_enabled`
set to true and the `enable_exp` site setting set to true. In addition, in the main channel
stream we now hide all messages that are linked to threads except for the original message,
disabling the concept of an "echo mode" for now, we may revisit this in future. We also
remove the jigsaw puzzle "Open Thread" button for message actions, since the thread
indicator can just be used instead.
This also stops the `Chat::Publisher` from sending any messages related to chat
messages that are linked to a thread, unless that chat message is the OM of the
thread. A subsequent PR will link up all MessageBus events within the thread panel,
and for the message indicators.
Another subsequent PR will add the excerpt of the latest message in each thread,
as well as the avatars of the users messaging in the thread.
Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
Similar to 22a55ef0ce,
this commit adds a replies_count to the Chat::Thread
table, which is updated every 15 minutes via PeriodicalUpdates.
This is done so the new thread indicator for the UI can
show the count without intense serializer queries, but
in future we likely want this to update more frequently.
Followup to c1dc6a2db4,
this commit just missed removing one of the @computed
decorators which was causing multiple active channels
to show in the sidebar. Fix the issue and introduce a
system spec to catch this.
- clicking empty area on the header will toggle collapse/expand it
- applies a background on hover of the channel title
- active state for small buttons
- the back button now has the correct icon color when hovered
- adds missing focus state for heade buttons icons
When hovering the chat message actions we are technically not hovering the message anymore, which was removing the background and is slightly unexpected. This commit ensures we keep this background until closing the message actions.
This PR primarily fixes this case:
- USER A message
- USER B message
- USER B reply to USER A message <-- not showing user info when it should
Moreover, this commit also improves the spec to correctly test more cases.
This commit is a major overhaul of how chat message actions work, to make it so they are reusable between the main chat channel and the chat thread panel, as well as many improvements and fixes for the thread panel.
There are now several new classes and concepts:
* ChatMessageInteractor - This is initialized from the ChatMessage, ChatMessageActionsDesktop, and ChatMessageActionsMobile components. This handles permissions about what actions can be done for each
message based on the context (thread or channel), handles the actions themselves (e.g. copyLink, delete, edit),
and interacts with the pane of the current context to modify the UI
* ChatChannelThreadPane and ChatChannelPane services - This represents the UI context which contains the
messages, and are mostly used for state management for things like message selection.
* ChatChannelThreadComposer and ChatChannelComposer - This handles interaction between the pane, the
message actions, and the composer, dealing with reply and edit message state.
* Scrolling logic for the messages has now been moved to a helper so it can be shared between the main channel pane and the thread pane
* Various improvements with the emoji picker on both mobile and desktop. The DOM node of each component is now located outside of the message which prevents a large range of issues.
The thread panel now also works in the chat drawer, and the thread messages have less
actions than the main panel, since some do not make sense there (e.g. moving messages to
a different channel). The thread panel title, excerpt, and message sender have also been removed
for now to save space.
This gives us a solid base to keep expanding on and fixing up threads. Subsequent PRs will
make the thread MessageBus subscriptions work and disable echo mode
for the initial release of threads.
Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
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.
Followup to 0924f874bd,
we migrated Chat::Upload records to UploadReference records
there and have not been making new Chat::Upload records
for some time, we can now delete the model and table.
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 adds specs to the mentioned serializers to catch regressions
with MessageBus last_ids and to ensure the correct ones are being
returned and passed down to the ChannelSerializer.
Followup to d8ad5c3
This commit introduces a Chat::Publisher and MessageBus endpoint
that allows for updating a user's channel tracking state in bulk for
multiple channels, rather than having to do it for one channel
at a time.
This also required an improvement to ChannelUnreadsQuery -- now
multiple channel IDs can be passed to this to get the unread counts
and mention counts for those channels for a user, also increasing
efficiency rather than having to do a query for every individual
channel.
Followup to #20802
Instead of just marking the state read in JS for each channel
after the AJAX call, we can instead just rely on the MessageBus
user-tracking-state chat channel, and publish the state to all
the channels affected in MarkAllUserChannelsRead. This will make
it so the blue dots for the channels are cleared across all tabs.
Followup to a0381157e9, we just
need to make sure we set currentUserMembership.last_read_message_id
to the last_read_message_id from the updated memberships after marking
all channels read, otherwise we do not scroll to the bottom and still
show the "last visit" separators in channels that have been
marked read.
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.
These were added in 7dd317b875
but are now consistently failing with described_class.chat_summary(user, {})
returning nil. Skipping for now because they are holding up other
things.
To reproduce failure run:
RSPEC_SEED=46586 bundle exec rake plugin:spec
There are many situations that may cause users to lose permission to
send messages in a chat channel. Until now we have relied on security
checks in `Chat::ChatChannelFetcher` to remove channels which the
user may have a `UserChatChannelMembership` record for but which
they do not have access to.
This commit takes a more proactive approach. Now any of these following
`DiscourseEvent` triggers may cause `UserChatChannelMembership`
records to be deleted:
* `category_updated` - Permissions of the category changed
(i.e. CategoryGroup records changed)
* `user_removed_from_group` - Means the user may not be able to access the
channel based on `GroupUser` or also `chat_allowed_groups`
* `site_setting_changed` - The `chat_allowed_groups` was updated, some
users may no longer be in groups that can access chat.
* `group_destroyed` - Means the user may not be able to access the
channel based on `GroupUser` or also `chat_allowed_groups`
All of these are handled in a distinct service run in a background
job. Users removed are logged via `StaffActionLog` and then we
publish messages on a per-channel basis to users who had their
memberships deleted.
When the user has a channel they are kicked from open, we show
a dialog saying "You no longer have access to this channel".
When they click OK we redirect them either:
* To their first other public channel, if they have any followed
* The chat browse page if they don't
This is to save on tons of requests from kicked out users getting messages
from other channels.
When the user does not have the kicked channel open, we can just
silently yoink it out of their sidebar and turn off subscriptions.
Prior to this fix, we wouldn't display reactions done on different tabs. So, if user A was reacting on tab 1, tab 2 wouldn't display this reaction.
Since few weeks ago we now have the guarantee to have uniq reactions on a message which should prevent any duplicate.
This commit also removes various skipped tests related to reactions and makes `sign_in` explicit at the beginning of each test.
<!-- 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. -->
Martin Brennan
Andrei Prigorshnev
Daniel Waterworth
Loïc Guitaut
Joffrey JAFFEUX
David Battersby
Alan Guo Xiang Tan
Penar Musaraj
Jarek Radosz
Kris