When validating with a dynamic set of values, especially one that might change during runtime, we should use a lambda or a proc to ensure that the validation uses the most up-to-date set of values. This is particularly important when using config.eager_load = true, which can cause some elements to be loaded only once at startup, thus not reflecting changes made at runtime.
This was the root cause of the issues here, as we were adding more ReviewableScore types after initial load through: `register_reviewable_type Chat::ReviewableMessage`
This PR is a reworked version of https://github.com/discourse/discourse/pull/24670.
In chat, we need the ability to have several notifications per `chat_mention`.
Currently, we have one_to_one relationship between `chat_mentions` and `notifications`:
d7a09fb08d/plugins/chat/app/models/chat/mention.rb (L9)
We want to have one_to_many relationship. This PR implements that by introducing
a join table between `chat_mentions` and `notifications`.
The main motivation for this is that we want to solve some performance problems
with mentions that we're having now. Let's say a user sends a message with @ all
in a channel with 50 members, we do two things in this case at the moment:
- create 50 chat_mentions
- create 50 notifications
We don't want to change how notifications work in core, but we want to be more
efficient in chat, and create only 1 `chat_mention` which would link to 50 notifications.
Also note, that on the side of notifications, having a lot of notifications is not so
big problem, because notifications processing can be queued.
Apart from improving performance, this change will make the code design better.
Note that I've marked the old `chat_mention.notification_id` column as ignored, but
I'm not deleting it in this PR. We'll delete it later in https://github.com/discourse/discourse/pull/24800.
This commit adds a new "My threads" link in sidebar and drawer. This link will open the "/chat/threads" page which contains all threads where the current user is a member. It's ordered by activity (unread and then last message created).
Moreover, the threads list of a channel page is now showing every threads of a channel, and not just the ones where you are a member.
I took the wrong approach here, need to rethink.
* Revert "FIX: Use Guardian.basic_user instead of new (anon) (#24705)"
This reverts commit 9057272ee2.
* Revert "DEV: Remove unnecessary method_missing from GuardianUser (#24735)"
This reverts commit a5d4bf6dd2.
* Revert "DEV: Improve Guardian devex (#24706)"
This reverts commit 77b6a038ba.
* Revert "FIX: Introduce Guardian::BasicUser for oneboxing checks (#24681)"
This reverts commit de983796e1.
c.f. de983796e1
There will soon be additional login_required checks
for Guardian, and the intent of many checks by automated
systems is better fulfilled by using BasicUser, which
simulates a logged in TL0 forum user, rather than an
anon user.
In some cases the use of anon still makes sense (e.g.
anonymous_cache), and in that case the more explicit
`Guardian.anon_user` is used
In other kind of channels we will only unfollow but for group channels we don't want people to keep appearing in members list.
This commit also creates appropriate services:
- `Chat::LeaveChannel`
- `Chat::UnfollowChannel`
And dedicated endpoint for unfollow: `DELETE /chat/api/channels/:id/memberships/me/follows`
This bug was very reproducible when your last read was a message you didn't read and an admin would delete it. When coming back to the channel you would get a not found, in this case we will now reset last read and present you the last message of the channel.
We could be more fancy and try to detect the next readable message but that would be more code and complexity for such a rare case.
Mentions and other post processing (like images) are still done asynchronously in the background. This should ensure reloading a channel while the message has not been processed yet doesn’t renders a blank message.
As a followup, we could probably simplify the staged message logic, given we have the new cooked on send.
This commit implements drafts for threads by adding a new `thread_id` column to `chat_drafts` table. This column is used to create draft keys on the frontend which are a compound key of the channel and the thread. If the draft is only for the channel, the key will be `c-${channelId}`, if for a thread: `c-${channelId}:t-${threadId}`.
This commit also moves the draft holder from the service to the channel or thread model. The current draft can now always be accessed by doing: `channel.draft` or `thread.draft`.
Other notable changes of this commit:
- moves ChatChannel to gjs
- moves ChatThread to gjs
Group channels will allow users to create channels with a name and invite people. It's possible to add people even after creation of the channel. Removing users is not yet possible but will be added in the near future.
Technically a group channel is `direct_message_channel` with a group attribute set to true on its direct message (chatable). This model might evolve in the future but offers much flexibility for now without having to rely on a complex migration.
The commit essentially consists of:
- a migration to set existing direct message channels with more than 2 users to a group
- a new message creator which allows to search, add members, and create groups
- a new `AddUsersToChannel` service
- a modified `SearchChatable` service
This commit starts from a simple observation: cooking messages on the hot path can be slow. Especially with a lot of mentions.
To move cooking from the hot path, this commit has made the following changes:
- updating cooked, inserting mentions and notifying user of new mentions has been moved inside the `process_message` job. It happens right after the `Chat::MessageProcessor` run, which is where the cooking happens.
- the similar existing code in `rebake!` has also been moved to rely on the `process_message`job only
- refactored `create_mentions` and `update_mentions` into one single `upsert_mentions` which can be called invariably
- allows services to decide if their job is ran inline or later. It avoids to need to know you have to use `Jobs.run_immediately!` in this case, in tests it will be inline per default
- made various frontend changes to make the chat-channel component lifecycle clearer. we had to handle `did-update @channel` which was super awkward and creating bugs with listeners which the changes of the PR made clear in failing specs
- adds a new `-processed` (and `-not-processed`) class on the chat message, this is made to have a good lifecyle hook in system specs
We were incorrectly generating URLs with message id even when it was not provided, resulting in a route ending with "undefined", which was causing an error.
This commit also uses this opportunity to:
- move `invite_users` into a proper controller inside the API namespace
- refactors the code into a service: `Chat::InviteUsersToChannel`
This change allows users to edit their chat messages based on the criteria added to Site Settings.
If the grace period conditions are met then there will be no (edited) text applied to the message.
The following site settings are added to chat:
chat editing grace period (seconds since message created)
chat editing grace period max diff for low trust levels (number of characters changed)
chat editing grace period max diff for high trust levels (number of characters changed)
Workaround for an issue we are experiencing on thread index frontend where thread loads participants correctly (up to 10), then refreshes the threads and then limits to 3 participants.
There is an issue with storing threads for the main channel view and the thread list in the same store so handling the max participants on the frontend is a workaround until channel.threadsManager is updated.
I've adjusted the tests to handle the additional data being returned from ThreadParticipantQuery.
UX changes to thread item:
- drop "last reply" timestamp copy
- drop last reply excerpt
- show up 9+OP members
Co-authored-by: David Battersby <info@davidbattersby.com>
While very fast and powerful staged threads forces a lot of gymnastic and edge cases. This patch adds a new service `Chat::CreateThread` and uses it to create a thread unconditionally when a user replies to a message in a threading enabled channel. If the user actually doesn’t send a message we will have a thread with no messages which has no important impact and could even be periodically cleaned if necessary.
Note that this commit also moves message actions to .gjs as it was the original goal of this PR to correctly check for staged thread to show the menu or not.
Currently, the logic for creating a new chat message is scattered
between a controller and an “old” service.
This patch address this issue by creating a new service (using the “new”
sevice object system) encapsulating all the necessary logic.
(authorization, publishing events, etc.)
This PR swaps out the custom pathway to publishing and rendering mention warnings after a message is sent.
ChatPublisher#publish_notice is used, and expanded. Now, instead of only accepting text_content as an argument, component and component_args are accepted and there is a renderer for these components.
Translations moved to server, as notices expect text to be passed in unless a component is rendered
The warnings are rendered at the top now, outside of the scope of the single message that sent it.
I entirely removed the jit_messages_spec b/c it's duplicate testing of other parts of the app. IMO we don't need a backend test for a feature, a component test for the feature AND a system test (that is slow and potentially even flakey due to timing issues with wait) to test the same thing. So jit_messages_spec is gone.
It will now replies count and participants list. Also the title will be OM excerpt or user defined title, no more default "Thread" title. Lastly, the author of the last reply is also shown as prefix of it.
This is also fixes the issue of chat composer warnings persisting across channels. Currently if you try to mention more groups than is allowed for example, a mention warning pops up. When you change channels the mention warning will not disappear even if there is no text in the composer.
This adds a reset function to the chat-composer-warnings-tracker.js, which is called when the channel is changed and the message is empty. In the event that the message is not empty we call captureMentions to check the loaded drafts' mentions.
This PR would be nicer if the post-send notice used the new chat notices API to publish the mention warnings but we would have to change the existing ones and I thought that would be too much change for this PR. It'd be a good followup though.
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
