This commit introduces the ability to edit the channel
slug from the About tab for the chat channel when the user
is admin. Similar to the create channel modal functionality
introduced in 641e94f, if
the slug is left empty then we autogenerate a slug based
on the channel name, and if the user just changes the slug
manually we use that instead.
We do not do any link remapping or anything else of the
sort, when the category slug is changed that does not happen
either.
* DEV: Rnemae channel path to just c
Also swap the channel id and channel slug params to be consistent with core.
* linting
* channel_path
* params in wrong order
* Drop slugify helper and channel route without slug
* Request slug and route models through the channel model if possible
* Add client side redirection for backwards-compatibility
Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
Only allow maximum of `50_000` characters for chat drafts. A hidden `max_chat_draft_length` setting can control this limit. A migration is also provided to delete any abusive draft in the database.
The number of drafts loaded on current user has also been limited and ordered by most recent update.
Note that spec files moved are not directly related to the fix.
Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
Co-authored-by: Régis Hanol <regis@hanol.fr>
We've had the UploadReference table for some time now in core,
but it was added after ChatUpload was and chat was just never
moved over to this new system.
This commit changes all chat code dealing with uploads to create/
update/delete/query UploadReference records instead of ChatUpload
records for consistency. At a later date we will drop the ChatUpload
table, but for now keeping it for data backup.
The migration + post migration are the same, we need both in case
any chat uploads are added/removed during deploy.
Learn more about skidding here: https://popper.js.org/docs/v2/modifiers/offset/#skidding-1
This change has two goals:
- Fixes an issue when the user had zoomed the viewport and the popper would position on the opposite side
- Makes msg actions arguably more pleasant to the eye by preventing it to be right aligned with the message container
Prior to this fix trashed channels would still prevent a channel with the same slug to be created. This commit generates a new slug on trash and frees the slug for future usage.
The format used for the slug is: `YYYYMMDD-HHMM-OLD_SLUG-deleted` truncated to the max length of a channel name.
This commit allows us to set the channel slug when creating new chat
channels. As well as this, it introduces a new `SlugsController` which can
generate a slug using `Slug.for` and a name string for input. We call this
after the user finishes typing the channel name (debounced) and fill in
the autogenerated slug in the background, and update the slug input
placeholder.
This autogenerated slug is used by default, but if the user writes anything
else in the input it will be used instead.
Note this commit also slightly changes internal API: channel instead of getChannel and updateCurrentUserChannelNotificationsSettings instead of updateCurrentUserChatChannelNotificationsSettings.
Also destroyChannel takes a second param which is the name confirmation instead of an optional object containing this confirmation. This is to enforce the fact that it's required.
In the future a top level jsdoc config file could be used instead of the hack tempfile, but while it's only an experiment for chat, it's probably good enough.
We refer to the channel name rather than title elsewhere
(including the new channel modal), so we should be consistent.
Title is an internal abstraction, since DM channels cannot have
names (currently).
Also change the name field on channel edit to a input type="text"
rather than a textarea, since we don't want a huge input here.
Added in c2013865d7,
this migration was supposed to only turn off the hashtag
setting for existing sites (since that was the old default)
but its doing it for new ones too because we run all migrations
on new sites.
Instead, we should only run this if the first migration was
only just created, meaning its a new site.
If the enable_experimental_hashtag_autocomplete setting is
enabled, then we should autolink hashtag references to the
archived channels (e.g. #blah::channel) for a nicer UX, and
just show the channel name if not (since doing #channelName
can lead to weird inconsistent results).
The spec was flaky because it was dependent on order,
when usernames got high enough sequence numbers in them
we would get this error:
> expected to find text "bruce99, bruce100" in "bruce100, bruce99"
Also move selectors into page object and use them in the
spec instead.
There was an issue with channel archiving, where at times the topic
creation could fail which left the archive in a bad state, as read-only
instead of archived. This commit does several things:
* Changes the ChatChannelArchiveService to validate the topic being
created first and if it is not valid report the topic creation errors
in the PM we send to the user
* Changes the UI message in the channel with the archive status to reflect
that topic creation failed
* Validate the new topic when starting the archive process from the UI,
and show the validation errors to the user straight away instead of
creating the archive record and starting the process
This also fixes another issue in the discourse_dev config which was
failing because YAML parsing does not enable all classes by default now,
which was making the seeding rake task for chat fail.
* Firefox now finally returns PerformanceMeasure from performance.measure
* Some TODOs were really more NOTE or FIXME material or no longer relevant
* retain_hours is not needed in ExternalUploadsManager, it doesn't seem like anywhere in the UI sends this as a param for uploads
* https://github.com/discourse/discourse/pull/18413 was merged so we can remove JS test workaround for settings
* FIX: Channel archive N1 when serializing current user
The `ChatChannelSerializer` serializes the archive for the
channel if it is present, however this was causing an N1 for
the current user serializer in the case of DM channels, which
were not doing `includes(:chat_channel_archive)` in the
`ChatChannelFetcher`.
DM channels cannot be archived, so we can just never try to serialize
the archive for DM channels in `ChatChannelSerializer`, which
removes the N1.
* DEV: Add N1 performance spec for latest.html preloading
We modify current user serializer in chat, so it's a good
idea to have some N1 performance specs to avoid regressions
here.
In certain cases, like when `SiteSetting.slug_generation_method`
is set to `none` with certain locales, the autogenerated chat
channel slugs will end up blank. This was causing errors in
unrelated jobs calling `update!` on the channel. Instead, we
should just copy Category behaviour, which does not error
if the autogenerated slug ends up blank. We already allow
for this with chat channel URLs, using `-` in place of the
missing slug.
The problem here was that if your input has an Enter
listener (such as the chat message input) and the
`fill_in(with: str)` string has a `\n` at the end, this
is treated as an Enter keypress, so this `fill_in` was
submitting the chat message.
This commit fixes an issue where the chat message bookmarks
did not respect the user's `bookmark_auto_delete_preference`
which they select in their user preference page.
Also, it changes the default for that value to "keep bookmark and clear reminder"
rather than "never", which ends up leaving a lot of expired bookmark
reminders around which are a pain to clean up.
`last_message_sent_at` has a `NOT_NULL` constraint in the DB so it should be safe to use for sorting.
This was causing two flakeys:
```
1) UserNotifications.chat_summary with public channel email subject with regular mentions includes both channel titles when there are exactly two with unread mentions
Failure/Error: example.run
expected: "[Discourse] New message in Random 62 and Test channel"
got: "[Discourse] New message in Test channel and Random 62"
(compared using ==)
# ./plugins/chat/spec/mailers/user_notifications_spec.rb:203:in `block (6 levels) in <main>'
# ./spec/rails_helper.rb:356:in `block (2 levels) in <top (required)>'
# ./vendor/bundle/ruby/3.1.0/gems/webmock-3.18.1/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'
2) UserNotifications.chat_summary with public channel email subject with regular mentions displays a count when there are more than two channels with unread mentions
Failure/Error: example.run
expected: "[Discourse] New message in Random 62 and 2 others"
got: "[Discourse] New message in Test channel 0 and 2 others"
(compared using ==)
# ./plugins/chat/spec/mailers/user_notifications_spec.rb:236:in `block (6 levels) in <main>'
# ./spec/rails_helper.rb:356:in `block (2 levels) in <top (required)>'
# ./vendor/bundle/ruby/3.1.0/gems/webmock-3.18.1/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'
```
This commit is a series of fixes to improve stability of system tests following the use of threadsafe:
* Jobs.run_immediately in before block was causing issues
* During test a js error could be caused by an undefined this.details in chat-live-pane
* Apply the chat composer click trick everywhere when sending a message, it ensures we are not hiding anything with autocomplete
* There was another case not using send_message yet
This PR removes the limit added to max_users_notified_per_group_mention during #19034 and improve the performance when expanding mentions for large channel or groups by removing some N+1 queries and making the whole process async.
* Fully async chat message notifications
* Remove mention setting limit and get rid of N+1 queries
It should fix flakeys we have due to using_session. This commit is also fixing tests which were failing constantly with treadsafe enabled.
A test has also bene skipped as the issue couldn't be found so far.
More info: https://github.com/teamcapybara/capybara#threadsafe-mode
- improves UI by displaying channel status on it's own line
- ensures channel status is correctly updated right after the request on frontend
- adds status on info page
Recent changes surfaced the various issues with this codepath:
- we were not correctly reseting `messageLookup` leading to us trying to scroll to a non existing message in the view
- we were calling markAsRead which would scroll to the bottom, even if we had a target message
- we were not debouncing fetchMessages, which could cause multiple reload of the messages when loading it with a targetMessageId: first fetch from last read and then immediately fetch from targetMessageId
- other naming inconsistencies
- not handling drawer
This commit also adds tests for classic scenarios related to this use case.
I could repro the same failure by doing: `page.driver.browser.network_conditions = { offline: false, latency: 3000, throughput: 0 }`
Wait shouldn't be needed as we wait for selector, but I couldn't find a better solution on this case for now.
We are all in on system specs, so this commit moves all the chat quoting acceptance tests (some of which have been skipped for a while) into system specs.
The way our markdown raw_html hoisting worked, we only
supported one level of hoisting the HTML content. However
when nesting [chat] transcript BBCode we need to allow
for multiple levels of it. This commit changes opts.discourse.hoisted
to be more constant, and the GUID keys that have the hoisted
content are only deleted by unhoistForCooked rather than
the cook function itself, which prematurely deletes them
when they are needed further down the line.
Honestly seems like it's being in some weird loop for
discourse/hashtag_autocomplete_spec.rb for this:
```ruby
within topic_page.post_by_number(2) do
cooked_hashtags = page.all(".hashtag-cooked", count: 2)
expect(cooked_hashtags[0]["outerHTML"]).to eq(<<~HTML.chomp)
<a class=\"hashtag-cooked\" href=\"#{category.url}\" data-type=\"category\" data-slug=\"cool-cat\"><svg class=\"fa d-icon d-icon-folder svg-icon svg-node\"><use href=\"#folder\"></use></svg><span>Cool Category</span></a>
HTML
expect(cooked_hashtags[1]["outerHTML"]).to eq(<<~HTML.chomp)
<a class=\"hashtag-cooked\" href=\"#{tag.url}\" data-type=\"tag\" data-slug=\"cooltag\"><svg class=\"fa d-icon d-icon-tag svg-icon svg-node\"><use href=\"#tag\"></use></svg><span>cooltag</span></a>
HTML
end
```
I see this many times in the full logs with `SELENIUM_VERBOSE_DRIVER_LOGS=1`:
```
COMMAND FindElements {
"using": "css selector",
"value": "#post_2"
}
Followed by:
COMMAND FindChildElements {
"id": "26dfe542-659b-46cc-ac8c-a6c2d9cbdf0a",
"using": "css selector",
"value": ".hashtag-cooked"
}
```
Over and over and over, there are 58 such occurrences. I am beginning to
think `within` is just poison that should be avoided.
Previously, calling `sign_in` would cause the browser to be redirected to `/`, and would cause the Ember app to boot. We would then call `visit()`, causing the app to boot for a second time.
This commit adds a `redirect=false` option to the `/session/username/become` route. This avoids the unnecessary boot of the app, and leads to significantly faster system spec run times.
In local testing, this takes the full system-spec suite for chat from ~6min to ~4min.
1. `test()` and `render()` instead of `componentTest()`
2. Angle brackets
3. `strictEqual()`/`true()`/`false()` assertions
This removes all remaining uses of `componentTest` from core
Following the removal of user in current_user_membership we were now doing: `User.create(null)`.
I don't think it has any impact but this is just wasteful and could lead to issues if used.
The client already has all the information about the current user so
there is no need for us to be serializing the current `User` object each
time per channel that is preloaded.
In production, profiling shows that this unneeded serializing
adds a roughly 5% overhead to a request.
- chat-message is not using chat-api yet and the `jsonMode` shouldn't have been added
- correctly error on `getChannel` not found
- adds/correct relevant system tests
Note this is a very large PR, and some of it could have been splited, but keeping it one chunk made it to merge conflicts and to revert if necessary. Actual new code logic is also not that much, as most of the changes are removing js tests, adding system specs or moving things around.
To make it possible this commit is doing the following changes:
- converting (and adding new) existing js acceptances tests into system tests. This change was necessary to ensure as little regressions as possible while changing paradigm
- moving away from store. Using glimmer and tracked properties requires to have class objects everywhere and as a result works well with models. However store/adapters are suffering from many bugs and limitations. As a workaround the `chat-api` and `chat-channels-manager` are an answer to this problem by encapsulating backend calls and frontend storage logic; while still using js models.
- dropping `appEvents` as much as possible. Using tracked properties and a better local storage of channel models, allows to be much more reactive and doesn’t require arbitrary manual updates everywhere in the app.
- while working on replacing store, the existing work of a chat api (backend) has been continued to support more cases.
- removing code from the `chat` service to separate concerns, `chat-subscriptions-manager` and `chat-channels-manager`, being the largest examples of where the code has been rewritten/moved.
Future wok:
- improve behavior when closing/deleting a channel, it's already slightly buggy on live, it's rare enough that it's not a big issue, but should be improved
- improve page objects used in chat
- move more endpoints to the API
- finish temporarily skipped tests
- extract more code from the `chat` service
- use glimmer for `chat-messages`
- separate concerns in `chat-live-pane`
- eventually add js tests for `chat-api`, `chat-channels-manager` and `chat-subscriptions-manager`, they are indirectly heavy tested through system tests but it would be nice to at least test the public API
<!-- NOTE: All pull requests should have tests (rspec in Ruby, qunit in JavaScript). If your code does not include test coverage, please include an explanation of why it was omitted. -->
Prior to this change, each request executed 2 Redis calls per chat channel
that was loaded. The number of Redis calls quickly adds up once a user
is following multiple channels.
This commit adds an index for the query which the chat plugin executes
multiple times when preloading user data in `Chat::ChatChannelFetcher.unread_counts`.
Sample query plan from a query I grabbed from one of our production
instance.
Before:
```
QUERY PLAN
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
GroupAggregate (cost=10.77..696.67 rows=7 width=16) (actual time=7.735..7.736 rows=0 loops=1)
Group Key: cc.id
-> Nested Loop (cost=10.77..696.54 rows=12 width=8) (actual time=7.734..7.735 rows=0 loops=1)
Join Filter: (cc.id = cm.chat_channel_id)
-> Nested Loop (cost=0.56..76.44 rows=1 width=16) (actual time=0.011..0.037 rows=7 loops=1)
-> Index Only Scan using chat_channels_pkey on chat_channels cc (cost=0.28..22.08 rows=7 width=8) (actual time=0.004..0.014 rows=7 loops=1)
Index Cond: (id = ANY ('{192,300,228,727,8,612,1633}'::bigint[]))
Heap Fetches: 0
-> Index Scan using user_chat_channel_unique_memberships on user_chat_channel_memberships uccm (cost=0.28..7.73 rows=1 width=8) (actual time=0.003..0.003 rows=1 loops=7)
Index Cond: ((user_id = 1338) AND (chat_channel_id = cc.id))
-> Bitmap Heap Scan on chat_messages cm (cost=10.21..618.98 rows=89 width=12) (actual time=1.096..1.097 rows=0 loops=7)
Recheck Cond: (chat_channel_id = uccm.chat_channel_id)
Filter: ((deleted_at IS NULL) AND (user_id <> 1338) AND (id > COALESCE(uccm.last_read_message_id, 0)))
Rows Removed by Filter: 2085
Heap Blocks: exact=7106
-> Bitmap Index Scan on index_chat_messages_on_chat_channel_id_and_created_at (cost=0.00..10.19 rows=270 width=0) (actual time=0.114..0.114 rows=2085 loops=7)
Index Cond: (chat_channel_id = uccm.chat_channel_id)
Planning Time: 0.408 ms
Execution Time: 7.762 ms
(19 rows)
```
After:
```
QUERY PLAN
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
GroupAggregate (cost=5.84..367.39 rows=7 width=16) (actual time=0.130..0.131 rows=0 loops=1)
Group Key: cc.id
-> Nested Loop (cost=5.84..367.26 rows=12 width=8) (actual time=0.129..0.130 rows=0 loops=1)
Join Filter: (cc.id = cm.chat_channel_id)
-> Nested Loop (cost=0.56..76.44 rows=1 width=16) (actual time=0.038..0.069 rows=7 loops=1)
-> Index Only Scan using chat_channels_pkey on chat_channels cc (cost=0.28..22.08 rows=7 width=8) (actual time=0.011..0.022 rows=7 loops=1)
Index Cond: (id = ANY ('{192,300,228,727,8,612,1633}'::bigint[]))
Heap Fetches: 0
-> Index Scan using user_chat_channel_unique_memberships on user_chat_channel_memberships uccm (cost=0.28..7.73 rows=1 width=8) (actual time=0.006..0.006 rows=1 loops=7)
Index Cond: ((user_id = 1338) AND (chat_channel_id = cc.id))
-> Bitmap Heap Scan on chat_messages cm (cost=5.28..289.71 rows=89 width=12) (actual time=0.008..0.008 rows=0 loops=7)
Recheck Cond: ((chat_channel_id = uccm.chat_channel_id) AND (id > COALESCE(uccm.last_read_message_id, 0)) AND (deleted_at IS NULL))
Filter: (user_id <> 1338)
-> Bitmap Index Scan on index_chat_messages_on_chat_channel_id_and_id (cost=0.00..5.26 rows=90 width=0) (actual time=0.008..0.008 rows=0 loops=7)
Index Cond: ((chat_channel_id = uccm.chat_channel_id) AND (id > COALESCE(uccm.last_read_message_id, 0)))
Planning Time: 1.217 ms
Execution Time: 0.188 ms
(17 rows)
```
* UX: added fadeout + hashtag styling
UX: add full name to autocomplete
UX: autocomplete mentions styling
UX: emoji styling user status
UX: autocomplete emoji
* DEV: Move hashtag tag counts into new secondary_text prop
* FIX: Add is-online style to mention users via chat
UX: make is-online avatar styling globally available
* DEV: Fix specs
* DEV: Test fix
Co-authored-by: Martin Brennan <martin@discourse.org>
Use `Chat::ChatChannelFetcher.secured_public_channel_search` directly
checking for existence instead of running through
`Chat::ChatChannelFetcher.secured_public_channels` which executes 7 more
DB queries.
Follow up to a review in #18937, this commit changes the HashtagAutocompleteService to no longer use class variables to register hashtag data sources or types in context priority order. This is to address multisite concerns, where one site could e.g. have chat disabled and another might not. The filtered plugin registers I added will not be included if the plugin is disabled.
In both ChatMessage#rebake! and in ChatMessageProcessor
when we were calling ChatMessage.cook we were missing the
user_id to cook with, which causes missed hashtag cooks
because of missing permissions.
Previously, restricted category chat channel was available for all groups - even `readonly`. From now on, only user who belong to group with `create_post` or `full` permissions can access that chat channel.
This introduces another "section" of queries to the
hashtag autocomplete search, which returns results for
each type that start with the search term. So now results
will be in this order, and within these sections ordered
by the types in priority order:
1. Exact matches sorted by type
2. "starts with" sorted by type
3. Everything else sorted by type then name within type
Martin Brennan
Roman Rizzi
Natalie Tay
Joffrey JAFFEUX
Discourse Translator Bot
Jarek Radosz
Kris
Alan Guo Xiang Tan
David Taylor
Krzysztof Kotlarek
Rafael dos Santos Silva
Bianca Nenciu
Daniel Waterworth
chapoi
dependabot[bot]
Andrei Prigorshnev