I am changing many of these to notes or resolving them as is,
most of these I have not actively worked on in years so someone
else can work on them when we get to these areas again.
We want to allow admins to make new required fields apply to existing users. In order for this to work we need to have a way to make those users fill up the fields on their next page load. This is very similar to how adding a 2FA requirement post-fact works. Users will be redirected to a page where they can fill up the remaining required fields, and until they do that they won't be able to do anything else.
* DEV: Upgrade Rails to 7.1
* FIX: Remove references to `Rails.logger.chained`
`Rails.logger.chained` was provided by Logster before Rails 7.1
introduced their broadcast logger. Now all the loggers are added to
`Rails.logger.broadcasts`.
Some code in our initializers was still using `chained` instead of
`broadcasts`.
* DEV: Make parameters optional to all FakeLogger methods
* FIX: Set `override_level` on Logster loggers (#27519)
A followup to f595d599dd
* FIX: Don’t duplicate Rack response
---------
Co-authored-by: Jarek Radosz <jradosz@gmail.com>
* DEV: Upgrade Rails to 7.1
* FIX: Remove references to `Rails.logger.chained`
`Rails.logger.chained` was provided by Logster before Rails 7.1
introduced their broadcast logger. Now all the loggers are added to
`Rails.logger.broadcasts`.
Some code in our initializers was still using `chained` instead of
`broadcasts`.
* DEV: Make parameters optional to all FakeLogger methods
* FIX: Set `override_level` on Logster loggers (#27519)
A followup to f595d599dd
* FIX: Don’t duplicate Rack response
---------
Co-authored-by: Jarek Radosz <jradosz@gmail.com>
* Revert "FIX: Set `override_level` on Logster loggers (#27519)"
This reverts commit c1b0488c54.
* Revert "DEV: Make parameters optional to all FakeLogger methods"
This reverts commit 3318dad7b4.
* Revert "FIX: Remove references to `Rails.logger.chained`"
This reverts commit f595d599dd.
* Revert "DEV: Upgrade Rails to 7.1"
This reverts commit 081b00391e.
When chat is enabled, there's a scheduled job that runs every 5 minutes to check whether we need to send a "chat summary" email to users with unread chat messages or mentions.
On Discourse with a large number of users, the query used wasn't optimal and sometimes taking minutes. Which isn't good when the query is called every 5 minutes 😬
This PR reworks the query in `Chat::Mailer.send_unread_mentions_summary`.
Instead of starting from the `users` table, it starts from the `user_chat_channel_memberships` table which is the main piece tying everything together.
The new query is mostly similar to the previous one, with some bug fixes (like ensuring the user has `allow_private_messages` enabled for direct messages) and is also slightly simpler since it doesn't keep track of the `memberships_with_unread_messages` anymore. That part has been moved to the `user_notifications.chat_summary` email method.
The `UserEmailExtension` has been deleted since that was using to N+1 update the `user_chat_channel_memberships.last_unread_mention_when_emailed_it`(quite a mouthful 😛) but that's now done directly in the `user_notifications.chat_summary` email method.
The "plat de résistance" of that PR - the `user_notifications.chat_summary` method has been re-worked for improved performances 🚀
Instead of doing everything in one query, it does 4 tiny ones.
- One to retrieve the list of unread mentions (@something) in "category" channels
- One to retrieve the list of unread messages in "direct message" channels (aka. 1-1 and group discussions)
- One to load all the chat messages for each "category" channels from the last unread mention
- One to load all the chat messages for each "direct message" channels from the last unread message
All the specs for both `Chat::Mailer` and `UserNotification.chat_summary` have been rewriten for easier comprehension and faster execution (mostly by not using chat services which makes the specs go 10x slower...)
Internal ref - t/129848
This change allows the correct number of members to be added when creating a group direct message, based on the site setting chat_max_direct_message_users.
Previously we counted the current user within the max user limit and therefore the count was off by 1.
Before this fix we could only list messages of a thread if it was part of a `threading_enabled` channel or if the thread was set to `force`.
Due to our design of also using a thread id when this is just a chain of replies so we can switch from threading enabled to disabled at any time, we will allow `Chat:: ListChannelThreadMessages` to list the messages of any thread, the only important requirements are:
- having a thread id
- being able to access this thread
To allow this, this commit simply removes the check on `threading_enabled` or `force`.
Prior to this fix we had too logic to detect if a user is active or not:
- idle codepath on the frontend
- online user ids on the backend
The frontend solution is not very reliable, and both solution are just trying to be too smart. Making a lot of people questioning why they receive a notification sometimes and sometimes not. This commit removes all this logic and replaces it with a much more simpler logic:
- you can't receive notifications for channel you are actually watching
- we won't play a sound more than once every 3seconds
When users click a link that points to an existing group chat, we should reopen that chat instead of creating a new group chat so users can more easily continue ongoing conversations.
When a user had the chat option "Show activity indicator in header" set to "all new messages", and they would get a reply to a thread they're part of, the chat icon in the header would not show the unread bubble indicator.
In order to fix this, the `ChatHeaderIconUnreadIndicator` component will now `showUnreadIndicator` whenever there is either one unread public channel or there are unread threads.
I only added a system spec for this very specific path because I don't want to slow down the whole suite to test for all the various combination of the `chat_header_indicator_preference` values.
Internal ref - t/128874
When you reply to a chat message, we [always create a thread][1]. But when the channel we're in doesn't have threading enabled, the reply is _technically_ not a thread.
This changes the `in_thread?` method to check for both the presence of a `thread_id` and to ensure that the channel has `threading_enabled`.
Internal ref - t/128103/3
[1]: e6e3eaf472/plugins/chat/app/services/chat/create_message.rb (L110-L115)
Prior to this commit, only system users had this pass.
Another significant change of the PR, is to make membership of a channel the angular stone of the permission check to create/update/stop streaming a message. The idea being, if you are a member of a channel already we don't need to check if you can join it AGAIN.
We also have `Chat::AutoRemove::HandleCategoryUpdated` which will deal with permissions change so it's simpler and less prone to error to consider the membership as the only source of truth.
There's no point checking if a user can join a channel if they are already part of it. This case was frequent when using `enforce_membership: true` for custom bots for example.
This commit introduces several enhancements to the ChatSDK module, aiming to improve the functionality and usability of chat thread interactions. Here's what has been changed and added:
1. **New Method: `first_messages`:**
- Added a method to retrieve the first set of messages from a specified chat thread.
- This method is particularly useful for fetching initial messages when entering a chat thread.
- Parameters include `thread_id`, `guardian`, and an optional `page_size` which defaults to 10.
- Usage example added to demonstrate fetching the first 15 messages from a thread.
2. **New Method: `last_messages`:**
- Added a method to retrieve the last set of messages from a specified chat thread.
- This method supports reverse pagination, where the user may want to see the most recent messages first.
- Similar to `first_messages`, it accepts `thread_id`, `guardian`, and an optional `page_size` parameter, defaulting to 10.
- Usage example provided to illustrate fetching the last 20 messages from a thread.
The TextCleaner step has been moved from chat message’s validation to create_message/update_message services. It allows us to easily tweak part of its behavior depending on the needs.
For example we will now disable strip_whitespaces by default when streaming messages as we want to keep newlines and spaces at the end of the message.
This change encourages users to title their threads to make it easier for other users to join in on conversations that matter to them.
The creator of the chat thread will receive a toast notification prompting them to add a thread title when on mobile and the thread has at least 5 sent replies.
That could cause flakeyness in specs depending in which timing message bus would arrive and it's not necessary as it should be updated with `handleThreadOriginalMessageUpdate`.
Follow up to #26712 to account for older threads that don't have a persisted excerpt, as this was previously generated on every page load.
This change allows us to build the excerpt on the fly when none exists, fixing the issue of missing message excerpts for thread previews (within channel) and thread lists (on mobile/desktop).
This change moves the chat message excerpt into a new database column (string) on the chat_messages table.
As part of this change, we will now set the excerpt within the `Chat::CreateMessage` service, and update it within the `Chat::UpdateMessage` service.
This commit will now allow us to track read position in a thread and returns to this position when you open the thread.
Note this commit is also extracting the following components to make it possible:
- `<ChatMessagesScroller />`
- `<ChatMessagesContainer />`
The `UpdateUserThreadLastRead` has been updated to allow this.
Various refactorings have also been done to the code and specs to improve the support of last read.
Why this change?
When the site setting for chat_max_direct_message_users is set to 1, it is expected that users can have a 1:1 chat with other users. However, since the current user is counting as 1 user it makes starting a new chat impossible.
This change hands this validation off to DirectMessageChannel::MaxUsersExcessPolicy which handles the count correctly by filtering out the current user.
This enables the following in Discourse AI
```
plugin.register_modifier(:chat_allowed_bot_user_ids) do |user_ids, guardian|
if guardian.user
mentionables = AiPersona.mentionables(user: guardian.user)
allowed_bot_ids = mentionables.map { |mentionable| mentionable[:user_id] }
user_ids.concat(allowed_bot_ids)
end
user_ids
end
```
some bots that are id < 0 need to be discoverable in search otherwise people can not talk to them.
---------
Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
We were incorrectly using `return` in a block which was causing exceptions at runtime. These exceptions were not causing much issues as they are in defer block.
While working on writing a test for this specific case, I noticed that our `upsert_custom_fields` function was using rails `update_all` which is not updating the `updated_at` timestamp. This commit also fixes it and adds a test for it.
Prior to this change we would pre-load all the user channels which making initial page load slower. This change will make them be loaded right after initial load. In the past this was not possible as the channels would have to be loaded on each page transition. However since about a year, we made the channels to be cached on the frontend and no other request will be needed.
I have decided for now to not show a loading state in the sidebar as I think it would be noise, but we can reconsider this later.
Note given we don't have the channels loaded at first certain things where harder to accomplish. The biggest UX change of this commit is that we removed all the complex logic of computing the best channel to display when you load /chat. We will now store the id of the last channel you visited and will use this id to decide which channel to show.
Previously services would let you define a high level default `def default_actions_for_service; end` which would define various handlers like `on_success`, after months of usage we consider the cons are superior to the pros here.
Two mains cons:
- people would often not understand where the handling was coming from
- it's easy to miss a case when you write your specs
Forcing a thread will work even in channel which don't have `threading_enabled` or in direct message channels.
For now this feature is only available through the `ChatSDK`:
```ruby
ChatSDK::Message.create(in_reply_to_id: 1, guardian: guardian, raw: "foo bar baz", channel_id: 2, force_thread: true)
```
Prior to this fix if a user had started to reply to a message without actually sending a message, the thread would still be created and we would end up listing it in the threads list of a channel.
This commit also improves adds thread and thread_replies_count to the 4th parameter of the chat_message_created event.
