I suspect it was moreover possibly related to a flaky spec:
```
1) Chat::AutoJoinChannelBatch.call when arguments are valid when channel is found when more than one membership is created publishes an event
Failure/Error: subject(:result) { described_class.call(params) }
Mocha::ExpectationError:
unexpected invocation: Chat::Publisher.publish_new_channel(#<Chat::CategoryChannel:0x401f28>, #<User::ActiveRecord_Relation:0x401f50>)
unsatisfied expectations:
- expected exactly once, invoked never: Chat::Publisher.publish_new_channel(#<Chat::CategoryChannel:0x401f78>, [#<User:0x401fa0>, #<User:0x401fc8>])
satisfied expectations:
- allowed any number of times, invoked once: Chat::Action::CreateMembershipsForAutoJoin.call(has_entries({:channel => #<Chat::CategoryChannel:0x401f78>, :contract => instance_of(Chat::AutoJoinChannelBatch::Contract)}))
- allowed any number of times, invoked never: Chat::ChannelMembershipManager.new(#<Chat::CategoryChannel:0x401f78>)
- allowed any number of times, invoked never: #<Mock:0x402018>.recalculate_user_count(any_parameters)
# ./plugins/chat/app/services/chat/auto_join_channel_batch.rb:65:in `publish_new_channel'
# ./plugins/chat/app/services/service/base.rb:118:in `instance_exec'
# ./plugins/chat/app/services/service/base.rb:118:in `call'
# ./plugins/chat/app/services/service/base.rb:368:in `block in run!'
# ./plugins/chat/app/services/service/base.rb:368:in `each'
# ./plugins/chat/app/services/service/base.rb:368:in `run!'
# ./plugins/chat/app/services/service/base.rb:361:in `run'
# ./plugins/chat/app/services/service/base.rb:229:in `call'
# ./plugins/chat/spec/services/chat/auto_join_channel_batch_spec.rb:50:in `block (3 levels) in <main>'
# ./plugins/chat/spec/services/chat/auto_join_channel_batch_spec.rb:110:in `block (6 levels) in <main>'
# ./spec/rails_helper.rb:393:in `block (2 levels) in <top (required)>'
```
Prior to this fix `context.membership&.update!(last_viewed_at: Time.zone.now)` would generate an update statement from a GET request which is not permitted by default when in readonly mode.
The usual fix in this case is to check for readonly or rescue an error, however, this common pattern of updating "last seen" or similar can be better handled in a `Schedule::Defer` block, which won't raise the `ActiveRecord::ReadOnlyError` when in readonly and will also prevent the controller to wait for this operation.
`Jobs::AutoJoinChannelBatch` was holding a lot of logic which should be in a service. Moreover, this refactoring is the opportunity to address a bug which could cause a duplicate key error.
From now when trying to insert a new membership it won't fail if a membership is already present.
Example error:
```
Job exception: ERROR: duplicate key value violates unique constraint "user_chat_channel_unique_memberships"
DETAIL: Key (user_id, chat_channel_id)=(1, 2) already exists.
Backtrace
rack-mini-profiler-3.1.0/lib/patches/db/pg.rb:110:in `exec'
rack-mini-profiler-3.1.0/lib/patches/db/pg.rb:110:in `async_exec'
(eval):29:in `async_exec'
mini_sql-1.4.0/lib/mini_sql/postgres/connection.rb:209:in `run'
mini_sql-1.4.0/lib/mini_sql/active_record_postgres/connection.rb:38:in `block in run'
mini_sql-1.4.0/lib/mini_sql/active_record_postgres/connection.rb:34:in `block in with_lock'
activesupport-7.0.5.1/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt'
activesupport-7.0.5.1/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize'
activesupport-7.0.5.1/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt'
activesupport-7.0.5.1/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize'
mini_sql-1.4.0/lib/mini_sql/active_record_postgres/connection.rb:34:in `with_lock'
mini_sql-1.4.0/lib/mini_sql/active_record_postgres/connection.rb:38:in `run'
mini_sql-1.4.0/lib/mini_sql/postgres/connection.rb:64:in `query_single'
/var/www/discourse/plugins/chat/app/jobs/regular/chat/auto_join_channel_batch.rb:38:in `execute'
```
Note this commit is also using main branch of `shoulda-matchers` as the gem has not been released yet.
Co-authored-by: Loïc Guitaut <5648+Flink@users.noreply.github.com>
Prior to this commit we were loading a large number of thread messages without any pagination. This commit attempts to fix this and also improves the following points:
- code sharing between channels and threads:
Attempts to reuse/share the code use in channels for threads. To make it possible part of this code has been extracted in dedicated helpers or has been improved to reduce the duplication needed.
Examples of extracted helpers:
- `stackingContextFix`: the ios hack for rendering bug when momentum scrolling is interrupted
- `scrollListToMessage`, `scrollListToTop`, `scrollListToBottom`: a series of helper to correctly scroll to a specific position in the list of messages
- better general performance of listing messages:
One of the main changes which has been made is to remove the computation of visible message during scroll, it will only happen when needed (update last read for example). This constant recomputation of `message.visible` on intersection observer event while scrolling was consuming a lot of CPU time.
This is extracted from #22390.
This patch simplifies a little how we handle uploads in chat, relying on
ActiveRecord mechanisms instead of calling custom methods.
This also makes `Chat::Message#validate_message` a “real” AR validation,
meaning it will run automatically when `#valid?` is called.
This is extracted from #22390.
This patch adds a new `optional` option to the `model` step. This
means if an optional model returns something blank (`nil` or an empty
collection) then the service won’t fail and will execute the next step.
However if a model is properly returned, the step will try to check if
it is valid or not (if it responds to `#invalid?`). If the model isn’t
valid, then the step will fail (so no change here).
This commit makes it so that when the user has unread threads
for a channel we show a blue dot in the sidebar (or channel index
for mobile/drawer).
This blue dot is slightly different from the channel unread messages:
1. It will only show if the new thread messages were created since
the user last viewed the channel
2. It will be cleared when the user views the channel, but the threads
are still considered unread because we want the user to click into
the thread list to view them
This necessitates a change to the current user serializer to also
include the unread thread overview, which is all unread threads
across all channels and their last reply date + time.
Followup to d7ef7b9c03,
this adds a spec to test the case where old threads are
still unread for the user and should show at the top regardless
of pagination, and fixes some issues/makes some slight refactors.
It could only occur on message created by the user itself and deleted while the user was looking at the channel.
It more generally fix the trash service which was not correctly setting the author of the delete.
`SiteSetting.enable_public_channels` allows site admin to decide if public channels are available at all. There's no distinction between admins or not as we expect admins to create private category channels if they want to limit usage.
Initial migration and changes to models as well as
changing the following services to update last_message_id:
* Chat::MessageCreator
* Chat::RestoreMessage
* Chat::TrashMessage
The data migration will set the `last_message_id` for all existing
threads and channels in the database.
When we query the thread list as well as the channel,
we look at the last message ID for the following:
* Channel - Sorting DM channels, and channel metadata for the list of channels
* Thread - Last reply details for thread indicators and thread list
This implementation will need more work in the future. For simplification of tracking and other events (new thread, delete/restore OM...) we used the threads from `threadsManager` which makes pagination more complicated as we already have some results when we start.
Note this commit also simplify `Collection` to only have one `load` method which can be called repeatedly.
In previous changes we prevented creating a channel to also make users follow the channel. We were forcing recipients to follow the channel on message sent but this was not including the creator of the message itself.
This commit fixes it and also write an end-to-end system spec to cover these cases. The message creator service is currently being rewritten and should correctly test and ensure this logic is present.
This commit also makes changes on the frontend to instantly follow a DM when you open it, this change prevents a green dot to appear for a split second when you send a message in a channel you were previously not following. Only recipients will see the green dot.
Since we create threads in the background regardless of whether
threading is enabled for a channel, we get the unexpected behaviour
of everyone having a lot of unread threads when threading is enabled
for the channel.
To counteract this, when the admin enables threads for a channel
we can just run a high priority background job to mark all threads
as read in the channel for all users, so they are essentially
starting from a clean slate.
This commit includes several fixes and improvements to thread
original message handling:
1. When a thread's original message is deleted, the thread no longer
counts as unread for a user
2. When a thread original message is deleted and the user is looking
at the thread list, it will be removed from the list
3. When a thread original message is restored and the user is looking
at the thread list, it will be added back to the list if it was
previously loaded
This commit replaces two existing screens:
- draft
- channel selection modal
Main features compared to existing solutions
- features are now combined, meaning you can for example create multi users DM
- it will show users with chat disabled
- it shows unread state
- hopefully a better look/feel
- lots of small details and fixes...
Other noticeable fixes
- starting a DM with a user, even from the user card and clicking <kbd>Chat</kbd> will not show a green dot for the target user (or even the channel) until a message is actually sent
- it should almost never do a full page reload anymore
---------
Co-authored-by: Martin Brennan <mjrbrennan@gmail.com>
Co-authored-by: Jordan Vidrine <30537603+jordanvidrine@users.noreply.github.com>
Co-authored-by: chapoi <101828855+chapoi@users.noreply.github.com>
Co-authored-by: Mark VanLandingham <markvanlan@gmail.com>
Without this fix, the following error is raised:
```
ActiveRecord::StatementInvalid:
PG::SyntaxError: ERROR: syntax error at or near ")"
LINE 4: WHERE thread_id IN ()
```
This will be used when we move the channel creation for DMs
to happen when we first send a message in a DM channel to avoid
a double-request. For now we can just have a new API endpoint
for creating this that the existing frontend code can use,
that uses the new service pattern.
This also uses the new policy pattern for services where the policy
can be defined in a class so a more dynamic reason for the policy
failing can be sent to the controller.
Co-authored-by: Loïc Guitaut <loic@discourse.org>
Enabling/Disabling threading has been possible through command line until now. This commit introduces two new UIs:
- When creating a channel, it will be available once the category has been selected
- On the settings page of a channel for admins
Whenever a user opens a channel or marks it read, we now
update the last_viewed_at datetime for that channel membership
record. This is so we will be able to show thread unread indicators
in the channel sidebar that clear independently of the main thread
unread indicators. This unread functionality will follow in another
PR.
Followup to 1526d1f97d
This commit fixes an N1 for mentions/user status
when querying chat threads. This only happened if
any of the thread OMs had mentions.
* FEATURE: Sort thread list by unread threads first
This commit changes the thread list to show the threads that
have unread messages at the top of the list sorted by the
last reply date + time, then all other threads sorted by
last reply date + time.
This also fixes some issues by removing the last_reply
relationship on the thread, which did not work for complex
querying scenarios because its order would be discarded.
* FIX: Various fixes for thread list loading
* Use the channel.threadsManager and find the channel first rather
than use activeChannel in the threads manager, otherwise we may
be looking at differenct channels.
* Look at threadsManager directly instead of storing result for threads
list otherwise it can get out of sync because of replace: true in
other places we are loading threads into the store.
* Fix sorting for thread.last_reply, needed a resort.
This PR adds a new parameter to fetch chat messages: `target_date`.
It can be used to fetch messages by a specific date string. Note that it does not need to be the `created_at` date of an existing message, it can be any date. Similar to `target_message_id`, it retrieves an array of past and future messages following the query limits.
This commit adds a tracking dropdown to each individual thread, similar to topics,
that allows the user to change the notification level for a thread manually. Previously
the user had to reply to a thread to track it and see unread indicators.
Since the user can now manually track threads, the thread index has also been changed
to only show threads that the user is a member of, rather than threads that they had sent
messages in.
Unread indicators also respect the notification level -- Normal level thread tracking
will not show unread indicators in the UI when new messages are sent in the thread.
This commit adds the initial part of thread indicator improvements:
* Show the reply count, last reply date and excerpt,
and the participants of the thread's avatars and
count of additional participants
* Add a participants component for the thread that
can be reused for the list
* Add a query class to get the thread participants
* Live update the thread indicator more consistently
with the last reply and participant details
image image
In subsequent PRs we will cache the participants since
they do not change often, and improve the thread list
further with participants.
This commit also adds a showPresence boolean (default
true) to ChatUserAvatar, since we don't want to show the
online indicator for thread participants.
---------
Co-authored-by: chapoi <charlie@discourse.org>
Followup 55ef2d0698.
In the cases where the user has no last_read_message_id for
a channel, we want to make sure that a page_size is set for
the ChannelViewBuilder + MessagesQuery, otherwise we end up
loading way more messages than needed (the additional message
loading was fixed in the last commit).
Since 5cce829 and the new
channel view builder, we have no need of these obsolete
routes which have way too much logic in the controller, which
has been superseded by the view builder anyway.
Remove the routes and update the channel message loading to use it.
* Moved the settings cog from thread list to thread and
put it in a new header component
* Remove thread original message component, no longer needed
and the list item and thread indicator styles/content
will be quite different
* Start adding content (unread indicator etc.) to the thread
list item and changing structure to be more like designs
* Serialize the last thread reply when opening the thread index,
show in list and update with message bus
#### 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 patch introduces policy objects to chat services. It allows putting
more complex logic in a dedicated class, which will make services
thinner. It also allows providing a reason why the policy failed.
Some change has been made to the service runner too to use more easily
these new policy objects: when matching a failing policy (or any failing
step actually), the result object is now provided to the block. This
way, instead of having to access the reason why the policy failed by
doing `result["result.policy.policy_name"].reason` inside the block,
this one can be simply written like this:
```ruby
on_failed_policy(:policy_name) { |policy| policy.reason }
```
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.
