This adds the following chat metrics:
- _chat_open_channels_with_threads_enabled_ — a count of open channels
where threading is enabled.
- _chat_channel_messages_ — a count of messages sent in a chat channel
(i.e. not a personal chat / direct message), within a thread or outside of a thread.
- _chat_threaded_messages_ — a count of messages sent within a thread
in a chat channel (i.e. not a personal chat / direct messages).
- _chat_direct_messages_ — a count of messages sent in a personal chat / direct messages.
The metrics added using the plugin API introduced in 098ab29d,
and extended in d91456fd.
Note that these stats won't be exposed at the `about.json`
and the `site/statistics.json` routes.
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`:
https://github.com/discourse/discourse/blob/d7a09fb08de31dbeed55428b84e0a660dbd5cf7a/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.
Before, when needed to get stats in a plugin, we called Core classes directly.
Introducing plugin API will decouple plugins from Core and give as more freedom
in refactoring stats in Core. Without this API, I wasn't able to do all refactorings
I wanted when working on d91456f.
This adds the ability to collect stats without exposing them
among other stats via API.
The most important thing I wanted to achieve is to provide
an API where stats are not exposed by default, and a developer
has to explicitly specify that they should be
exposed (`expose_via_api: true`). Implementing an opposite
solution would be simpler, but that's less safe in terms of
potential security issues.
When working on this, I had to refactor the current solution.
I would go even further with the refactoring, but the next steps
seem to be going too far in changing the solution we have,
and that would also take more time. Two things that can be
improved in the future:
1. Data structures for holding stats can be further improved
2. Core stats are hard-coded in the About template (it's hard
to fix it without correcting data structures first, see point 1):
https://github.com/discourse/discourse/blob/63a0700d45f755d0f432a9075ae7afbed9cd6ab0/app/views/about/index.html.erb#L61-L101
The most significant refactorings are:
1. Introducing the `Stat` model
2. Aligning the way the core and the plugin stats' are registered
When quoting a chat message in a post, if that message contains a mention,
that mention should be ignored. But we've been detecting them and sending
notifications to users. This PR fixes the problem. Since this fix is for
the chat plugin, I had to introduce a new API for plugins:
# We strip posts before detecting mentions, oneboxes, attachments etc.
# We strip those elements that shouldn't be detected. For example,
# a mention inside a quote should be ignored, so we strip it off.
# Using this API plugins can register their own post strippers.
def register_post_stripper(&block)
end
This is a follow-up to e6299a3. I additionally fixed these three things:
1. Since e6299a3 there's no need anymore to join the group_users table
when looking for users who were reached by a group mention, so
I removed that join in that commit. But turned out we were joining
the group_users table twice, so I removed the second join in this PR.
That drastically speeded up my test query, from 6 sec to 0.26 sec.
2. We also were joining twice the user_chat_channel_memebership table,
so I removed the second unnecessary join too.
3. We actually need to join the user_chat_channel_memebership table
only in certain cases, and we don't need to do that for group mentions,
so I fixed that too.
As a result of these changes, time of my test query fall down from
6 sec to 0.001 sec. And the resulting SQL query now contains only
one JOIN statement.
A follow-up to faac6773. This PR eliminates one more heavy join by forcing
Active Record to do two queries instead.
Also, along the way, I made this change:
```
# this generates two quries to the groups table
def groups_to_mention
@groups_to_mention = mentionable_groups - groups_with_too_many_members
end
# so I changed it to (this makes only one query to the groups table):
def groups_to_mention
@groups_to_mention ||= mentionable_groups.where("user_count <= ?", SiteSetting.max_users_notified_per_group_mention)
end
```
This one is kind of a premature optimization, because we don't have evidence that
this extra query is a problem, but it seems cleaner this way.
Commits history on this PR may help better understand the change.
Some time ago, we introduced the `cookAsync` instead of the existing
`cook` function, and planned to migrate everything to it. Then after
migrating, we wanted to raname the function to simply `cook`.
I've checked Core and plugins, and currently we call `cookAsync` everywhere,
there are no calls to the `cook` function anymore. So we're good
to proceed with this refactoring.
This PR makes the first step by making current cookAsync and cook functions
do the same thing. Effectively now the `cook` function becomes an alias
for the `cookAsync` function.
We noticed some of these tests were flaky, sometimes they fail on assertions
related to testing tooltips. Tooltips are generally hard to test, and it's not
necessary to test tooltips in every test case. This PR isolates tooltip testing
in a dedicated test case.
Note we already did the same thing for another spec in a9dfda2 and that
seems to worked well.
Tooltips are generally hard to test, and we noticed that they make tests flaky.
We may not test tooltips in these acceptance tests, since they are already
tested in components/chat-channel-test.js. This PR deletes tooltip-related
assertions and unskips tests that became flaky after adding them.
We have the max_mentions_per_chat_message site settings; when a user tries
to mention more users than allowed, no one gets mentioned.
Chat messages may contain code-blocks with strings that look like mentions:
def foo
@bar + @baz
end
The problem is that the parsing code considers these as real mentions and counts
them when checking the limit. This commit fixes the problem.
It is hard to catch and debug potential bugs related to live updates of user status
(though, we haven't seen many such bugs so far). We have a `console.warn`
statement that should help us to catch one class of such bugs.
Recently, we noticed that this warning gets printed when a user had a chat with
a user that was then deleted.
This is not a bug, since there is nothing to track for a deleted user, but we don't
want this noise on the console. This PR makes sure we don't print a warning in
such cases.
This fixes the problem reported in
https://meta.discourse.org/t/custom-status-message-in-front-of-by-header-on-scroll/273320.
This problem can be reproduced with any tooltip created using the DTooltip
component or the createDTooltip function.
The problem happens because the trigger for tooltip on mobile is click, and for tooltip
to disappear the user has to click outside the tooltip. This is the default behavior
of tippy.js – the library we use under the hood.
Note that this PR fixes the problem in topics, but not in chat. I'm going to investigate and
address it in chat in a following PR.
To fix it for tooltips created with the createDTooltip function, I had to make a refactoring.
We had a somewhat not ideal solution there, we were leaking an implementation detail
by passing tippy instances to calling sides, so they could then destroy them. With this fix,
I would have to make it more complex, because now we need to also remove onScrool
handlers, and I would need to leak this implementation detail too. So, I firstly refactored
the current solution in 5a4af05 and then added onScroll handlers in fb4aabe.
When refactoring this, I was running locally some temporarily skipped flaky tests. Turned out
they got a bit outdated, so I fixed them. Note that I'm not unskipping them in this commit,
we'll address them separately later.
This partially reverts 2ecc829.
The problem is that if we don't transform mentions right away,
there is a noticeable lag before a mention gets fully rendered,
while with the transformation, everything is super smooth.
I'm reverting that change only for mentions. Another part was about
category hashtags, but unfortunately they lag both with and without
the transformation. We need to address them separately.
After fbe0e4c we always pass a block into these methods.
So yield inside the export methods works and there is no need
anymore to wrap them into enumerators.
So we have to order by calling `find_each(order: :desc)`.
Note that that will order rows by Id, not by `last_match_at`
as we tried before (though that didn't work).
We did some testing and saw that making one query per month is
cheaper than querying all chat messages at ones. Note that even
though the export job will be performing one query per month,
the exported messages will be streamed into a single CSV file, so
nothing changes from the user's point of view.
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
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 sure we don't load all data into memory when doing CSV exports.
The most important change here made to the recently introduced export of chat
messages (3ea31f4). We were loading all data into memory in the first version, with
this commit it's not the case anymore.
Speaking of old exports. Some of them already use find_each, and it worked as
expected, without loading all data into memory. And it will proceed working as
expected after this commit.
In general, I made sure this change didn't break other CSV exports, first manually, and
then by writing system specs for them. Sadly, I haven't managed yet to make those
specs stable, they work fine locally, but flaky in GitHub actions, so I've disabled them
for now.
I'll be making more changes to the CSV exports code soon, those system specs will be
very helpful. I'll be running them locally, and I hope I'll manage to make them stable
while doing that work.
A follow-up to 585a2e4e. A couple of tests with the new rich tooltip were flaky.
We suppose the reason is some problem related to widgets lifecycle. This PR
doesn't fix the issue, but isolates testing of the tooltip related logic related
inside its own test, which should make it not flaky.
This is a temporal solution, we're going to move all these code to using
glimmer components.
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.
We had a bug in this code recently, sometimes users saw weird notifications
like:
User mentioned all_mentioned_user_ids in the help chat channel
We fixed that bug in b85d057.
This refactoring is a follow-up to that fix. As that bug showed, it’s quite easy
to introduce a key that may end up being sent to the `NotifyMentioned` job,
which can lead to such weird notifications. This refactoring makes sure that
the `to_notify` hash contains only IDs of users that should be notified about
mentions.
We use it like this:
expect(message.created_at).to eq_time(created_at)
The problem is that if one of the values or both of them are `nil` the matcher fails
with this error:
NoMethodError: undefined method `-' for nil:NilClass
This commit adds support for `nil` values. If both time values are `nil` they are equal,
if only one value is `nil` they aren't.
These spec are flaky only in CI, not locally and not in GitHub actions.
The previous attempt was in 44eabde, but actually the failure happens
a bit earlier. This is another attempt to fix these specs. Quite a lot of
async logic is happening in emulateAutocomplete(), a call to settled()
in the end should help make it more reliable.
These specs were skipped in d6d5eae1. They sometimes failed, and only on CI,
not in GitHub Actions.
I wasn't able to reproduce failures locally, but I expect clicking the send button
in chat composer should be more reliable than emulating pressing <kbd>Enter</kbd>.
When editing a message, we call `message.cook()` in the beginning of
`#sendEditMessage` methods, but when sending a new message,
the call to `message.cook()` is hidden in the `stageMessage` method.
We can just call `message.cook()` before sending the message, no matter
whether this is a new message or an edited message.
When a user type a message with mentions, the autocomplete popup
may suggest users or groups. We were adding all these object to
the `currentMessage.mentionedUsers` collection, while we should
have been adding only users. A group added to that collection led to
the error later when trying to update user status on mentions.
This will make it simpler to work with this code. This also can make this code more stable and increase stability of our test suite.
Cooked message now will be available immediately after cooking, it wasn't the case before:
await message.cook();
const cooked = message.cooked;
This also removes a call to `message.cook()` from message fabricator. Alternatively we may leave the call there and make the fabricator function async, but I fill it's better this way. If someone needs to test something related to cooked message, they can either pass cooked text to fabricator:
message = fabricators.message({ cooked: "<p>cooked</p>" });
or call `message.cook()` after fabrication:
message = fabricators.message({ message: "raw message" });
await message.cook()
These specs were disabled in 786f7503. While investigating this, I found out that at some point `:user_membership` got deleted. It's hard to tell why exactly without investing more time, but it seems using `let!` instead of `fab!` solves the issue.
If in the future we decide to investigate why these tests were flaky with `fab!` to reproduce the failure run:
LOAD_PLUGINS=1 rspec --seed 46586 plugins/chat/spec/mailers/user_notifications_spec.rb
This PR adds status to mentions in chat and makes those mentions receive live updates.
There are known unfinished part in this implementation: when posting a message, status on mentions on that message appears immediately, but only if a user used autocomplete when typing the message. If user copy and paste a message with mentions into chat composer, those mentions won't have user status on them.
PRs with fixes for both problems are following soon.
Preparations for this PR that were made previously include:
- DEV: correct a relationship – a chat message may have several mentions 0dcfd7ddec
- DEV: extract the logic for extracting and expanding mentions from ChatNotifier 75b81b6854
- DEV: Always create chat mention records fa543cda06
- DEV: better split create_notification! and send_notifications logic e292c45924
- DEV: more tests for mentions when updating chat messages e7292e1682
- DEV: extract updating status on mentions into a lib function e49d338c21
- DEV: Create and update chat message mentions earlier 35a414bb38
- DEV: Create a chat_mention record when self mentioning 2703f2311a
- DEV: When deleting a chat message, do not delete mention records f4fde4e49b
A follow-up to 54b2a85b. That commit didn't fix the issue because the to_notify hash that we return from the notify_edit method isn't used anywhere apart from tests (that's confusing, we're going to fix that soon).
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).
In the past, we create a `chat_mention` records only when we wanted to notify a user about a mention. Since we don't send notifications when a user mentioning himself, we didn't create a `chat_mention` records in those cases.
Now we use `chat_mentions` records in other scenarios too, so when a user is mentioning himself we want to:
1. Create a `chat_mention` record for that mention
2. Do not create a notification for that mention
We need to create and update `chat_mentions` records for messages earlier. They should be created or updated before we call `Chat::Publisher.publish_new!` `Chat::Publisher.publish_edit!` to send the message to message bus subscribers).
This logic is covered with tests in `message_creator_spec.rb`, `message_updater_spec.rb`, `notifier_spec.rb` and `notify_mentioned_spec.rb`.
See the commits history for steps of refactoring.
User status updates come from the server in a map where keys are user IDs.
If user.trackStatus() is called for a user model without an ID, the model
cannot identify its status updates and silently misses them. It's quite hard to
notice that a user rendered in the UI doesn't receive live status updates.
Also, it's not immediately obvious what's the reason of the problem.
A warning will be very helpful here.
We call the `/u/search/users` URL when autocompleting users. It returns
user's name, username and avatar template, but not user ID.
We need it to return user IDs in order to display user status in certain situations.
I could add ID to FoundUserWithStatusSerializer, so it will be added only if
user status is enabled in site settings. But I feel that it's good to always return it,
it's not a lot of data comparing to what we already return, and it should be useful
in other scenarios.
This adds these two new test cases:
context "when updating a mentioned user" do
it "updates the mention record" do
# and
context "when there are duplicate mentions" do
it "creates a single mention record per mention" do
Apart from that, this groups mention related tests into a context, renames one test, and moves setup of another test into the test case itself from the before block (to make it more clear, that test is the only one that uses that setup). See the PR's commit history.
Steps to reproduce the bug:
1. Send a chat message
2. Edit the message and add a mention to it
3. The mentioned user won't receive a notification
This PR fixes the problem.
Also:
1. There's no need anymore to have a code for removing notifications in the `notify_edit` method, because a call to `@chat_message.update_mentions` in the first line of the `notify_edit` method does that job:
https://github.com/discourse/discourse/blob/ff56f403a2e54db124a70cdcda5a9c1ca27f3535/plugins/chat/lib/chat/notifier.rb#L90
2. There's no need to load mention records from database, it's enough to pluck user ids
We've found these exceptions in logs:
Job exception: undefined method `destroy!' for nil:NilClass
/var/www/discourse/plugins/chat/lib/chat/notifier.rb:102:in `block in notify_edit'
/var/www/discourse/plugins/chat/lib/chat/notifier.rb:100:in `each'
/var/www/discourse/plugins/chat/lib/chat/notifier.rb:100:in `notify_edit'
/var/www/discourse/plugins/chat/app/jobs/regular/chat/send_message_notifications.rb:18:in `execute'
In the past, we were creating `chat_mention` records only for sending notifications, so every mention record had a related notification. It isn't the case anymore (since fa543cda). This PR fixes the problem by making sure the notification exists before trying to remove it. Also, we shouldn't be deleting a `chat_mention` record itself, only a notification, this PR fixes that too.
It's quite hard to reproduce this bug locally, I wasn't able to do so, the logic in this class is quite complicated, that's why I'm not adding a test. Also, when looking at this I realized that this method isn't in a fully correct state now, I suspect sometimes some notifications may not be delivered after someone edits a chat message and adds new mentions to it. I'm going to refactor and simplify the method in a subsequent PR.
In some languages, labels on the site settings navigation menu
get truncated. This adds titles to menu items, so users can see
untruncated labels on hover.
This refactoring simplifies ChatNotifier a bit. I wanted to drop
that argument for expand_direct_mentions too, but that needs
a bit deeper refactoring, so it's better to do it separately.
Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
This regressed with the commit fa543cd. Starting from that commit, we create mention records even if a user shouldn't be notified. So when sending emails, we should be making sure if a notification was actually created for a mention. This is essentially the whole fix that we need here. Tests will be provided in a following PR.
Steps to reproduce:
1. Create a post with a mention of a user that has user status with an end date
2. Try to load the topic with that post as an anonymous user
You'll see a topic with blank content.
This is just a little clean-up in tests. In the past, when creating a `chat_mention`
record, we always created a related notification. Starting from fa543cda
notifications and chat_mentions are fully decoupled from each other. So if we're
testing just chat mentions there is no need to fabricate notifications for them.
`create_notification!` - creates a notification in the database, `send_notifications` sends desktop and mobile notifications. This PR moves some code to decouple these two tasks more explicitly. It only moves code without changing any behavior, and the job is covered with tests (see chat_notify_mentioned_spec).
Before this commit, we created a chat mention record only in case we wanted to send a notification about that mention to the user. Notifications were the only use case for the chat_mention db table. Now we want to use that table for other features, so we have to always create a chat_mention record.
Initially, the chat_mention db table was created to support notifications. So when creating
a `chat_mention` record we were always creating a related `notification` record. So did the
ChatMention fabricator.
Now we want to use the chat_mention db table in other scenarios. So we started decoupling
mentions from notification in 75b81b68.
This removes fabrication of Notifications from the ChatMention fabricator. We need to be able
to fabricate a ChatMention without a Notification.
Initially, the ChatMention model / db table was introduced to better support notifications (see discourse/discourse-chat@0801d10). That means that currently, we create a new chat_mention record only if a user will be notified about the mention.
Now we plan to start using the ChatMention model in other scenarios (for example for implementing user status on mentions) so we need to always create a new record in the chat_mention table. This PR does the first step into that direction by decoupling the logic for extracting and expanding mentions from the code related to notifications.
This doesn't change any behavior, only extracts code from ChatNotifier.
This change only makes the model reflect correctly what's
already happening in the database. Note that there are no calls
to chat_message.chat_mention in Core and plugins so this
change should be safe.
Also note, that at the moment we use the chat_mentions db
table only to support notifications about mentions, but
we're going to start using it for other cases. This commit is
the first step in that direction.
We caught it in logs, race condition led to this error:
ActiveRecord::RecordNotUnique
(PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "user_statuses_pkey"
DETAIL: Key (user_id)=(15) already exists.)
The reason the problem happened was that we were checking if a user has status and if not inserting status:
if user_status
...
else
self.user_status = UserStatus.create!(status)
end
The problem is that it's possible that another request will insert status just after we check if status exists and just before our request call `UserStatus.create!(status)`. Using `upsert` fixes the problem because under the hood `upsert` generates the only SQL request that uses "INSERT ... ON CONFLICT DO UPDATE". So we do everything in one SQL query, and that query takes care of resolving possible conflicts.
We show live user status on mentions starting from a76d864. But status didn’t appear on the post that appears on the bottom of the topic just after a user posted it (status appeared only after page reloading). This adds status to just posted posts.
We decided to rename the "Do Not Disturb" mode to "Pause Notifications". I am starting from changing strings on the client, that will update user interface. And I'm going to do renamings in frontend and backend code after some time.
This PR adds a new "Pause notifications" checkbox to the user status modal. This checkbox allows enabling the Do-Not-Disturb mode together with user status. Note that we don't remove and don't rename the existing DnD menu item in this PR, so the old way of entering the DnD mode is still available.
Also, we're not making DnD mode a part of user status on backend and in database. The reason is that the DnD mode should still be available on sites with disabled user status, having them separated helps keep the implementation simple.
This fixes the problem reported in https://meta.discourse.org/t/trackstatus-error-in-docs-topics/248717 and also guarantees that the same problem won't appear in other plugins.
The problem was that we're calling trackStatus() and on() on a user object, but that only works if it's a user model and fails on plain js objects.
I'm not adding tests here because in Core we always have a properly wrapped user model here. But this fix makes sure that plugins that don't won't fail here.
The repro for the bug:
Add a post with a mention of a user
Post another post below
Delete the first post with a mention
Reload the page and try to attempt to view hidden reply
Note that we don't have a database table and a model for post mentions yet, and I decided to implement it without adding one to avoid heavy data migrations. Still, we may want to add such a model later, that would be convenient, we have such a model for mentions in chat.
Note that status appears on all mentions on all posts in a topic except of the case when you just posted a new post, and it appeared on the bottom of the topic. On such posts, status won't be shown immediately for now (you'll need to reload the page to see the status). I'll take care of it in one of the following PRs.
This adds API scope for the user status. This also adds a get method to the user status controller. We didn't need a dedicated method that returns status before because the server returns status with user objects, but I think we need to provide this method for API clients.
The problem was reported as a problem with changing theme in user preferences, after saving a new theme the previously set user status was disappearing (https://meta.discourse.org/t/user-status/240335/42). Turned out though that the problem was more wide, changing pretty much any setting in user preferences apart from user status itself led to clearing the status.
Andrei Prigorshnev
