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.
What is the problem here?
In multiple controllers, we are accepting a `limit` params but do not
impose any upper bound on the values being accepted. Without an upper
bound, we may be allowing arbituary users from generating DB queries
which may end up exhausing the resources on the server.
What is the fix here?
A new `fetch_limit_from_params` helper method is introduced in
`ApplicationController` that can be used by controller actions to safely
get the limit from the params as a default limit and maximum limit has
to be set. When an invalid limit params is encountered, the server will
respond with the 400 response code.
This is extracted from #22390.
This patch introduces a scope to avoid duplication and a new method,
`Chat::Channel.find_by_id_or_slug` to allow finding a channel either by
its id or by its slug (or its category slug).
`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 only moves code around and doesn't change any behavior. This does two things:
1. Extracts the `channel.joined_by?` methods
2. Uses term "members" instead of "participants" for chat members
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).
Someone who cannot chat is also not able to join chat channels,
so we may not check all the time user.can_chat? && user.can_join_chat_channel?
and just call user.can_join_chat_channel? instead.
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.
This commit attempts to fix an issue where we are ending
up with bad created_at date formats for last messages, which
is breaking the DM sort order and sometimes causing DM channels
to fall off the list, or show "Invalid date" on mobile.
I have not been able to consistently reproduce these issues
locally, however the serialzier for the channels index uses
MultiJSON.dump() and the Chat::Publisher uses .to_json, both of
which format created_at differently for messages.
The former is `2023-07-05T06:53:25.977Z` (iso8601).
The latter is `2023-07-14 03:59:22 UTC` (.to_s default).
Since we are doing comparison and sorting of these dates on the UI
we need consistent formatting for the JS Date parsers (and moment)
to deal with.
If the issue still occurs after this we can investigate further.
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
* FEATURE: Inline topic summary. Cached version accessible to everyone.
Anons and non-members of the `custom_summarization_allowed_groups_map` groups can see cached summaries for any accessible topic. After the first 12 hours and if the posts to summarize have changed, allowed users clicking on the button will automatically re-generate it.
* Ensure chat summaries work and prevent model hallucinations when there are no messages.
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.
When a user sends their first message in a thread we
automatically track the thread in the backend, but we
don't reflect this in the UI until the user re-opens
the thread. This commit fixes that by showing the new
tracking level in the UI.
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 fixes a longstanding TODO to move the contents of the
UpdateUserCountsForChannels job to the ensure_consistency!
method of Chat::Channel, which runs every 15 mins as part of
periodical updates.
This commit also addresses the performance issue of the original,
where we would fetch all channels and do an individual query to
get the count and update the count of each one. Now we do it all
in one query, and only publish the changed channels to the UI.
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 c6b43ce68b
We can just use the rich excerpt everywhere since we know
we don't need text_entities -- that introduced security issues
just to fix a spec.
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.
Updates the interface for implementing summarization strategies and adds a cache layer to summarize topics once.
The cache stores the final summary and each chunk used to build it, which will be useful when we have to extend or rebuild it.
To export chat messages, go to `/admin/plugins/chat` and click the Create export
button in the _Export chat messages_ section. You'll receive a direct message
when the export is finished.
Currently, this exports all messages from the last 6 months, but not more than
10000 messages.
This exports all chat messages, including messages from private channels and
users' direct conversations. This also exports messages that were deleted.
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>
Since we created user_chat_thread_memberships in
cc2570f we haven't
yet backfilled it for users who previously sent a message in
in threads -- this migration creates the UserChatThreadMemberships
needed for those threads, making sure the last read message id
is accurate for those participants.
* FEATURE: Content custom summarization strategies.
This PR establishes a pattern for plugins to register alternative ways of summarizing content by extending a class that defines an interface.
Core controls which strategy we'll use and who has access to it through the `summarization_strategy` and `custom_summarization_allowed_groups`. It also defines the UI for summarizing topics.
Other plugins can access this summarization mechanism and implement their features, removing cross-plugin customizations, as it currently happens between chat and the discourse-ai plugin.
* Group membership validation and rate limiting
* Work with objects instead of classes
* Port summarization feature from discourse-ai to chat
* Rename available summaries to 'Top Replies' and 'Summary'
This fixes an issue where a user could send an empty
string as a chat message .e.g ' ' and the message would
be posted. We don't want this, we need to strip the message
first before validating for length etc.
We were calling reset without the proper params which was causing errors in the console. This commit does the following changes:
- ensures `composer.cancel()` is the only way to cancel editing/reply
- adds a `draftSaved` property to chat message to allow for better tests
- writes a spec to ensure the flow is correct
- adds more page objects for better tests
- homogenize the default state of objects on chat message
Co-authored-by: Martin Brennan <martin@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).
This commit attempts to fix the case where the messages loaded initially don't fill the screen. It would prevent user to scroll and as a result to load more.
There are multiple fixes in this commit:
- the main fix is removing this code which was preventing the actual fill:
```javascript
// prevents an edge case where user clicks bottom arrow
// just after scrolling to top
if (loadingPast && this.#isAtBottom()) {
return;
}
```
- ensures we always give a page site to the `chatApi.channel(...)` call if we have one, in the current state when `fetchFromLastRead` was `true` we would not set `args.page_size`
- ensures the `query_paginated_messages` is having a valid page size, which is not nil and not > `MAX_PAGE_SIZE`
- write a spec for the autofill, it was a challenging spec to write but it should give us the confidence we need here
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
Joffrey JAFFEUX
Alan Guo Xiang Tan
Loïc Guitaut
Jan Cernik
Andrei Prigorshnev
Martin Brennan
Roman Rizzi
Mark VanLandingham