#### FIX: Do not use client lastReadMessageId when fetching channel messages
We had an issue where the following happened:
1. User opened channel and saw the last message, and we set the
lastReadMessageId on the server and the client
2. User navigated to another channel
3. Another user deleted the message in the original channel
4. The first user navigated back to the original channel before
the MessageBus event for the deleted message arrived, and got
a 404 error because we were sending the deleted lastReadMessageId as
target_message_id to the channel controller.
Instead of this which is a bit flaky and is hard to cover all
the issues for, instead we can pass a fetch_from_last_read boolean
param to the channels controller, and just get the user's
last_read_message_id straight from the database to use for the
target_message_id. This gets rid of any sources of race conditions
or lack of updates from MessageBus.
#### FIX: Include missing memberships for thread tracking publish
When we publish the channel/message tracking state for a
user and that message was a thread reply the publisher
was erroring because we were not telling Chat::TrackingStateReportQuery
to return missing memberships (which have zeroed out unread counts)
as well, which is what we do for the channel tracking state here.
Also just make sure that the TrackingStateReport does not error
when passed an ID it doesn't have data for.
This commit follows up b6c5a2da08
by serializing the user's thread memberships in these cases:
1. When we do the initial channel fetch with messages, we get
all threads and all the user's thread memberships for those
messages.
2. When the thread list is fetched, we get all the user's memberships
in that list.
3. When the single thread is fetched, either from opening it from
the list, an OM indicator, or just from doing .find() on the
manager when a new MessageBus message comes in
This will let us track the lastReadMessageId on the client, and
will also let us fix an issue where the unread indicator in the
channel header was incrementing for every thread that got a
new message, regardless of whether the user was a member.
This commit adds the thread index and individual thread
in the index list unread indicators, and wires up the message
bus events to mark the threads as read/unread when:
1. People send a new message in the thread
2. The user marks a thread as read
There are several hacky parts and TODOs to cover before
this is more functional:
1. We need to flesh out the thread scrolling and message
visibility behaviour. Currently if you scroll to the end
of the thread it will just mark the whole thread read
unconditionally.
2. We need to send down the thread current user membership
along with the last read message ID to the client and
update that with read state.
3. We need to handle the sidebar unread dot for when threads
are unread in the channel and clear it based on when the
channel was last viewed.
4. We need to show some indicator of thread unreads on the
thread indicators on original messages.
5. UI improvements to make the experience nicer and more
like the actual design rather than just placeholders.
But, the basic premise around incrementing/decrementing the
thread overview count and showing which thread is unread
in the list is working as intended.
In the ChannelViewBuilder, we introduced a check to see if
the target message exists, which errors if the message has
been trashed. However if the user is the creator of the message
or admin then they are able to see trashed messages, so
we need to take this into account.
Followup to c908eeacc9
Instead of using the latest message ID in the channel, which
could cause issues if you have an earlier last read message ID
that matches the deleted one, instead we use the first non-deleted
message that comes before the deleted message by ID.
Followup ae3231e140, when a
message is trashed we already update the lastReadMessageId of
all users in the channel to the latest non-deleted message on
the server side. However we didn't propagate this to the client,
so in some cases when we did the following:
1. Delete the last message in the channel
2. Switch to another channel
3. Switch back to the original
We would get a 404 error from the target message ID being looked
up still being the old lastReadMessageId (now deleted) for the
user's channel membership.
All we need to do is send the last not-deleted message ID for
the channel (or thread) to all the member users.
This commit moves message lookup and querying to the
/chat/api/channel/:id endpoint and adds the ability
to query the tracking state overview for threads as well
as the threads and thread tracking state for any thread
original messages found.
This will allow us to get an initial overview of thread
tracking for a user when they first enter a channel, rather
than pre-emptively loading N threads and tracking state
for those across all channels on the current user serializer,
which would be expensive.
This initial overview will be used in subsequent PRs to
flesh out the thread unread indicators in the UI.
This also moves many chunks of code that were in services
to reusable Query classes, since use of services inside
services is discouraged.
This moves chat tracking state calculation for channels
and threads into a central Chat::TrackingStateManager service, that
serves a similar purpose to the TopicTrackingState model
in core.
This service calls down to these query classes:
* ThreadUnreadsQuery
* ChannelUnreadsQuery
To get the unread_count and mention_count for the appropriate
channels and threads.
As well as this, this commit refactors the client-side chat
tracking state.
Now, there is a central ChatTrackingStateManager Ember Service
so all tracking is accessible and can be counted from one place,
which can also initialize tracking from an initial payload.
The actual tracking counts are now maintained in a ChatTrackingState
class that is initialized on the `.tracking` property of both channel and
thread objects.
This removes the attributes on UserChatChannelMembership and decoration
of said membership from ChannelFetcher, preferring instead to have an additional
object for tracking in the JSON.
A chat message may be restored later, so we shouldn't be deleting `chat_mentions` records for it.
But we still have to remove notifications (see 082cd139).
When the user sends a message in a thread, we want to
create a membership for them in the background (default
to notification level of Watching) so we can track whether
they have read the thread.
Then, for now since we don't have granular message reading/
scrolling in the thread panel, we just update the thread
last_read_message_id for the user to the latest reply in the
thread when they open the thread panel. This at least will
mark the thread as read.
In future PRs we want to show the blue dot indicator in various
places in the UI for unread threads which will also require
some MessageBus functionality.
This takes into account the same issue fixed for channels
in ae3231e140
Since we have channel message retention which deletes
messages, we can end up with cases where the thread
is still around but the message is deleted. We will
handle the cascade delete in a different commit --
for now we will ensure the thread list lookup handles
this case and doesn't error.
This commit adds an initial thread list UI. There are several limitations
with this that will be addressed in future PRs:
* There is no MessageBus reactivity, so e.g. if someone edits the original
message of the thread it will not be reflected in the list. However if
the thread title is updated the original message indicator will be updated.
* There is no unread functionality for threads in the list, if new messages
come into the thread there is no indicator in the UI.
* There is no unread indicator on the actual button to open the thread list.
* No pagination.
In saying that, this is the functionality so far:
* We show a list of the 50 threads that the user has most recently participated
in (i.e. sent a message) for the channel in descending order.
* Each thread we show a rich excerpt, the title, and the user who is the OM creator.
* The title is editable by staff and by the OM creator.
* Thread indicators show a title. We also replace emojis in the titles.
* Thread list works in the drawer/mobile.
When we were deleting messages in chat, we would find all of
the UserChatChannelMembership records that had a matching
last_read_message_id and set that column to NULL.
This became an issue when multiple users had that deleted message
set to their last_read_message_id. When we called ChannelUnreadsQuery
to get the unread count for each of the user's channels, we were
COALESCing the last_read_message_id and returning 0 if it was NULL,
which meant that the unread count for the channel would be the total
count of the messages not sent by the user in that channel.
This was particularly noticeable for DM channels since we show
the count with the indicator in the header. This issue would disappear
as soon as the user opened the problem channel, because we would then
set the last_read_message_id to an actual ID.
To circumvent this, instead of NULLifying the last_read_message_id in
most cases, it makes more sense to just set it to the most recent
non-deleted chat message ID for the channel. The only time it will
be set to NULL now is when there are no more other messages in the
channel.
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>
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.
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.
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>
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.
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.
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.
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.
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. -->
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 fixes the UpdateUserLastRead spec which was checking
for a message ID that did not exist -- this could fail at times
since message ID 2 could exist. Better to create + destroy a message
since then it's guaranteed we have a unique ID.
This also attempts to clarify a step that we expect to fail which
succeeds instead by adding another emoji next to the success tick and
an explanation text.
Also removes some uses of unless in Services::Base, we generally prefer
to use alternatives, since unless can be hard to parse in a lot of
cases.
Co-authored-by: Loïc Guitaut <loic@discourse.org>
We’re now using `contract` as the first step and validations for
mandatory parameters have been added.
To simplify specs a bit, we only assert the service contract is run as
expected without testing each validation case. We’re now testing the
contract itself in isolation.
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 introduce a new API for registering callbacks, which we'll execute when a user gets destroyed, and the `delete_posts` opt is true. The chat plugin registers one callback and queues a job to destroy every message from that user in batches.
Martin Brennan
Joffrey JAFFEUX
Andrei Prigorshnev
Daniel Waterworth
Loïc Guitaut
Dan Ungureanu
Roman Rizzi
David Taylor