This feature adds notifications for chat messages that are sent within personal chats (1:1 and personal group chats).
To prevent notification spam we make use of consolidated notifications to combine updated message information in a meaningful way that allows the receiver to quickly jump into the chat to see what they missed.
This update respects muted channels, muted and blocked users. It will only create a new notification when the user has not muted the channel and the notified user is not muting or ignoring the message sender.
Why this change?
We have been bitten by bugs where tests are not catching missing
interpolate argument in our client side code because the JavaScript
tests are also using `I18n.translate` to assert that the right message
is shown. Before this change, `I18n.interpolate` will just replace the
missing interpolation argument in the final translation with some
placeholder. As a result, we ended up comparing a broken translation
with another broken translation in the test environment.
Why does this change do?
This change introduces the `I18n.testing` property which when set to
`true` will cause `I18n.translate` to throw an error when an interpolate
argument is missing. With this commit, we also set `I18n.testing = true`
when running qunit acceptance test.
Second iteration of https://github.com/discourse/discourse/pull/23312 with a fix for embroider not resolving an export file using .gjs extension.
---
This PR introduces three new concepts to Discourse codebase through an addon called "FloatKit":
- menu
- tooltip
- toast
## Tooltips
### Component
Simple cases can be express with an API similar to DButton:
```hbs
<DTooltip
@label={{i18n "foo.bar"}}
@icon="check"
@content="Something"
/>
```
More complex cases can use blocks:
```hbs
<DTooltip>
<:trigger>
{{d-icon "check"}}
<span>{{i18n "foo.bar"}}</span>
</:trigger>
<:content>
Something
</:content>
</DTooltip>
```
### Service
You can manually show a tooltip using the `tooltip` service:
```javascript
const tooltipInstance = await this.tooltip.show(
document.querySelector(".my-span"),
options
)
// and later manual close or destroy it
tooltipInstance.close();
tooltipInstance.destroy();
// you can also just close any open tooltip through the service
this.tooltip.close();
```
The service also allows you to register event listeners on a trigger, it removes the need for you to manage open/close of a tooltip started through the service:
```javascript
const tooltipInstance = this.tooltip.register(
document.querySelector(".my-span"),
options
)
// when done you can destroy the instance to remove the listeners
tooltipInstance.destroy();
```
Note that the service also allows you to use a custom component as content which will receive `@data` and `@close` as args:
```javascript
const tooltipInstance = await this.tooltip.show(
document.querySelector(".my-span"),
{
component: MyComponent,
data: { foo: 1 }
}
)
```
## Menus
Menus are very similar to tooltips and provide the same kind of APIs:
### Component
```hbs
<DMenu @icon="plus" @label={{i18n "foo.bar"}}>
<ul>
<li>Foo</li>
<li>Bat</li>
<li>Baz</li>
</ul>
</DMenu>
```
They also support blocks:
```hbs
<DMenu>
<:trigger>
{{d-icon "plus"}}
<span>{{i18n "foo.bar"}}</span>
</:trigger>
<:content>
<ul>
<li>Foo</li>
<li>Bat</li>
<li>Baz</li>
</ul>
</:content>
</DMenu>
```
### Service
You can manually show a menu using the `menu` service:
```javascript
const menuInstance = await this.menu.show(
document.querySelector(".my-span"),
options
)
// and later manual close or destroy it
menuInstance.close();
menuInstance.destroy();
// you can also just close any open tooltip through the service
this.menu.close();
```
The service also allows you to register event listeners on a trigger, it removes the need for you to manage open/close of a tooltip started through the service:
```javascript
const menuInstance = this.menu.register(
document.querySelector(".my-span"),
options
)
// when done you can destroy the instance to remove the listeners
menuInstance.destroy();
```
Note that the service also allows you to use a custom component as content which will receive `@data` and `@close` as args:
```javascript
const menuInstance = await this.menu.show(
document.querySelector(".my-span"),
{
component: MyComponent,
data: { foo: 1 }
}
)
```
## Toasts
Interacting with toasts is made only through the `toasts` service.
A default component is provided (DDefaultToast) and can be used through dedicated service methods:
- this.toasts.success({ ... });
- this.toasts.warning({ ... });
- this.toasts.info({ ... });
- this.toasts.error({ ... });
- this.toasts.default({ ... });
```javascript
this.toasts.success({
data: {
title: "Foo",
message: "Bar",
actions: [
{
label: "Ok",
class: "btn-primary",
action: (componentArgs) => {
// eslint-disable-next-line no-alert
alert("Closing toast:" + componentArgs.data.title);
componentArgs.close();
},
}
]
},
});
```
You can also provide your own component:
```javascript
this.toasts.show(MyComponent, {
autoClose: false,
class: "foo",
data: { baz: 1 },
})
```
Co-authored-by: Martin Brennan <mjrbrennan@gmail.com>
Co-authored-by: Isaac Janzen <50783505+janzenisaac@users.noreply.github.com>
Co-authored-by: David Taylor <david@taylorhq.com>
Co-authored-by: Jarek Radosz <jradosz@gmail.com>
This PR introduces three new UI elements to Discourse codebase through an addon called "FloatKit":
- menu
- tooltip
- toast
Simple cases can be express with an API similar to DButton:
```hbs
<DTooltip
@label={{i18n "foo.bar"}}
@icon="check"
@content="Something"
/>
```
More complex cases can use blocks:
```hbs
<DTooltip>
<:trigger>
{{d-icon "check"}}
<span>{{i18n "foo.bar"}}</span>
</:trigger>
<:content>
Something
</:content>
</DTooltip>
```
You can manually show a tooltip using the `tooltip` service:
```javascript
const tooltipInstance = await this.tooltip.show(
document.querySelector(".my-span"),
options
)
// and later manually close or destroy it
tooltipInstance.close();
tooltipInstance.destroy();
// you can also just close any open tooltip through the service
this.tooltip.close();
```
The service also allows you to register event listeners on a trigger, it removes the need for you to manage open/close of a tooltip started through the service:
```javascript
const tooltipInstance = this.tooltip.register(
document.querySelector(".my-span"),
options
)
// when done you can destroy the instance to remove the listeners
tooltipInstance.destroy();
```
Note that the service also allows you to use a custom component as content which will receive `@data` and `@close` as args:
```javascript
const tooltipInstance = await this.tooltip.show(
document.querySelector(".my-span"),
{
component: MyComponent,
data: { foo: 1 }
}
)
```
Menus are very similar to tooltips and provide the same kind of APIs:
```hbs
<DMenu @icon="plus" @label={{i18n "foo.bar"}}>
<ul>
<li>Foo</li>
<li>Bat</li>
<li>Baz</li>
</ul>
</DMenu>
```
They also support blocks:
```hbs
<DMenu>
<:trigger>
{{d-icon "plus"}}
<span>{{i18n "foo.bar"}}</span>
</:trigger>
<:content>
<ul>
<li>Foo</li>
<li>Bat</li>
<li>Baz</li>
</ul>
</:content>
</DMenu>
```
You can manually show a menu using the `menu` service:
```javascript
const menuInstance = await this.menu.show(
document.querySelector(".my-span"),
options
)
// and later manually close or destroy it
menuInstance.close();
menuInstance.destroy();
// you can also just close any open tooltip through the service
this.menu.close();
```
The service also allows you to register event listeners on a trigger, it removes the need for you to manage open/close of a tooltip started through the service:
```javascript
const menuInstance = this.menu.register(
document.querySelector(".my-span"),
options
)
// when done you can destroy the instance to remove the listeners
menuInstance.destroy();
```
Note that the service also allows you to use a custom component as content which will receive `@data` and `@close` as args:
```javascript
const menuInstance = await this.menu.show(
document.querySelector(".my-span"),
{
component: MyComponent,
data: { foo: 1 }
}
)
```
Interacting with toasts is made only through the `toasts` service.
A default component is provided (DDefaultToast) and can be used through dedicated service methods:
- this.toasts.success({ ... });
- this.toasts.warning({ ... });
- this.toasts.info({ ... });
- this.toasts.error({ ... });
- this.toasts.default({ ... });
```javascript
this.toasts.success({
data: {
title: "Foo",
message: "Bar",
actions: [
{
label: "Ok",
class: "btn-primary",
action: (componentArgs) => {
// eslint-disable-next-line no-alert
alert("Closing toast:" + componentArgs.data.title);
componentArgs.close();
},
}
]
},
});
```
You can also provide your own component:
```javascript
this.toasts.show(MyComponent, {
autoClose: false,
class: "foo",
data: { baz: 1 },
})
```
Co-authored-by: Martin Brennan <mjrbrennan@gmail.com>
Co-authored-by: Isaac Janzen <50783505+janzenisaac@users.noreply.github.com>
Co-authored-by: David Taylor <david@taylorhq.com>
Co-authored-by: Jarek Radosz <jradosz@gmail.com>
- do not prevent the event (it was a violation anyways as the touchstart is passive)
- waits for at least 25px horizontal move before showing the remove action, it avoids showing the remove while scrolling and moving a little bit horizontally
On mobile swiping a channel row will now show a "Remove" option. Holding this to the end will now remove this row from your list of followed direct message channels.
Co-authored-by: chapoi <101828855+chapoi@users.noreply.github.com>
Introduce max length on text columns for description and slug fields within chat.
At a later date we will probably want to convert these text columns to string/varchar through a migration, but for now this change introduces a limit within the active record model.
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.
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.)
Chat review queue flags were missing the context message above the actions.
This is probably because the (reasonably complex) logic was somewhat hard-coded to posts. After some investigation I concluded we can reuse this logic with some small amendments.
- moves the onebox logic away from `plugin.rb` to a new `onebox_handler` lib
- splits the `discourse_chat_message` template into two: one for channels, and one for messages
- refactors the logic code slightly to send only the necessary arguments to each template
This commit shouldn't change end-user behavior.
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.
The unread(s) will still show in the sidebar, outside of chat and when in drawer mode. This is to prevent the confusion to show an unread count for chat on a button which is going to take the user out of chat.
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.
Prior to this fix we would scroll the emoji picker with the body of the page in drawer mode.
With this fix scrolling inside the drawer or the emoji picker will scroll the drawer or the emoji picker, but, scrolling body will close the chat emoji picker.
1. Use `this.` instead of `{{action}}` where applicable
2. Use `{{fn}}` instead of `@actionParam` where applicable
3. Use non-`@` versions of class/type/tabindex/aria-controls/aria-expanded
4. Remove `btn` class (it's added automatically to all DButtons)
5. Remove `type="button"` (it's the default)
6. Use `concat-class` helper
This is extracted from #22390.
This patch aims to ease the transition to the new message creation
service. (in progress in #22390) Indeed, the new service patch is
breaking some specs from `discourse-ai` and `discourse-templates`
because these plugins are using either `Chat::MessageCreator` or the
`chat_message` fabricator.
This patch addresses theses issues by normalizing how we create a chat
message in specs. To do so, the preferred way is to use
`Fabricate(:chat_message)` with a new `:use_service` option allowing to
call the service under the hood. While this patch will obviously call
`Chat::MessageCreator`, the new service patch will now be able to simply
change the call to `Chat::CreateMessage` without breaking any specs from
other plugins.
Another thing this patch does is to not create chat messages using the
service for specs that aren’t system ones, thus speeding the execution
time a bit in the process.
Most of the core plugins were already hidden, this hides
chat, styleguide, and checklist to avoid potential confusion
for end users.
Also removes respond_to? :hide_plugin, since that API has been
in place for a while now.
This moves the "delete message" action (if it is available) of a flagged chat message under the "ignore" menu. This puts it on par with the menu for flagged posts.
This commit fixes an issue from 2ecc8291e8
where the user sees an ugly plain #hashtag when sending a chat
message. Now, we add a basic placeholder that looks like the
cooked hashtag with a grey square, which is then filled in
once the "sent" message bus event for the message comes back,
and we do decorateCooked on the message to fill in the proper
hashtag details.
When flagging a chat message, and options included both notifying user and notifying staff, the modal was missing the separating text. This was happening because the #staffFlagsAvailable method was based on post flags, and the model for chat flags is slightly different. This fixes that by delegating to the relevant flag target object.
Tries to fix the composer upload spec by making the upload
slow enough to allow clicking the Cancel button, and improves
generally the API for CDP network changes.
Channels and threads are cached as much as possible, as a result the `last_message_bus_id` can become stalled.
It was for example exhibited with the following actions:
- open a channel (A)
- send a message
- navigate to another channel (B)
- come back to channel (A), and you would actually get all the messages replayed since you opened (A) for the first time as the `last_message_bus_id` would only refresh on a full page reload
This was technically not causing known bugs ATM, but was probably the source of few hard to repro bugs and would for sure cause issues in the future.
Co-authored-by: Mark VanLandingham <markvanlan@gmail.com>
Prior to this fix the text would be incorrect when the current user reacted and number of reactions was above 2.
This commit fixes the bug and also makes the following changes:
- separates text computation in a standalone lib to make it easier to test
- increases the number of displayed usernames in reaction text (from 5 to 15)
- adds a full test suite for this new `getReactionText` function
- fixes a bug in reaction fabricator which would prevent to change the count to zero
In #22914 we added a fix to stop creating reviewables in the review queue when flagging a chat message and choosing the "notify user" option. By mistake we also stopped creating it when selecting the "something else" option.
This change makes it so a "something else" flag once again creates a reviewable. (Same behaviour as posts.)
- drop @
- prevents +X (participants) to show on next line
- few spacing/fonts adjustments
Note that this commit is also stripping links from chat excerpts.
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 could happen after you had already change the separation mode and would cause unexpected bugs.
This PR also adds more tests around using switch buttons with chat.
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.
Prior to this fix we would test by visiting the tab which could create a false positive, as the tab could not be present but we could still access the tab, the implementation and tests have been changed to correctly ensure this.
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.
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.
In the past we were only intercepting 429 and 404; it's probably better to surface any error.
There are already tests for the 404 and 429, I consider them enough for now.
This commit ensures we have correct icon and title on mobile for the chat header icon.
It also fixes a bug where the site setting was not correctly used when the user has not yet set the user option.
Both cases are now correctly tested.
This commit was incorrectly removed of https://github.com/discourse/discourse/pull/23078 and would set the state only on entering (or exiting) chat route. The tests were already present in the previous PR.
Our code assumed the content_range interval was inclusive, but they are open-ended due to Postgres' [discrete range types](https://www.postgresql.org/docs/current/rangetypes.html#RANGETYPES-DISCRETE), meaning [1,2] will be represented as [1,3).
It also fixes some flaky tests due to test data not being correctly setup and the registry not being resetted after each test.
Each time a message is created through a webhook, we create we webhook_event associated to this webhook.
When destroying a webhook, we were not destroying the webhook_events which was causing orphans records and more importantly errors in the app expecting to find and associated webhook.
Not sure when we added this but it is no longer necessary,
hashtags are cooked appropriately when sending chat messages
and the mention transform was not used anywhere.
The specs were relying a lot on mock and stubs. I suspect that under certain circumstances it didn't play well with fabricators and we ended up with the stub of another spec causing this kind of error:
```
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:0x39b840>, #<User::ActiveRecord_Relation:0x39b868>)
unsatisfied expectations:
- expected exactly once, invoked never: Chat::Publisher.publish_new_channel(#<Chat::CategoryChannel:0x39b890>, [#<User:0x39b8b8>, #<User:0x39b8e0>])
satisfied expectations:
- allowed any number of times, invoked once: Chat::Action::CreateMembershipsForAutoJoin.call(has_entries({:channel => #<Chat::CategoryChannel:0x39b890>, :contract => instance_of(Chat::AutoJoinChannelBatch::Contract)}))
- allowed any number of times, invoked never: Chat::ChannelMembershipManager.new(#<Chat::CategoryChannel:0x39b890>)
- allowed any number of times, invoked never: #<Mock:0x39b930>.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:412:in `block (2 levels) in <top (required)>'
```
The spec is now simplified and shouldn't have this issue anymore.
Prior to this fix we would output an image with no width/height which would then bypass a large part of `CookedProcessorMixin` and have no aspect ratio. As a result, an image with no size would cause layout shift.
It also removes a fix for oneboxes in chat messages due to this case.
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)>'
```
If a selenium finder takes the full wait duration to resolve, that means it has been written inefficiently. Most likely a matcher has been negated incorrectly.
This commit introduces a patch which will raise an error in this situation so that we can catch the issues while developing specs.
This commit also fixes chat's visit_thread helper. It was spinning on `has_css?(".chat-skeleton")` for the full selenium wait duration, and then returns false. That's because the thread is often already fully loaded before `has_css?` is even called. It's now updated to only look for the final expected state.
This commit removes any logic in the app and in specs around
enable_experimental_hashtag_autocomplete and deletes some
old category hashtag code that is no longer necessary.
It also adds a `slug_ref` category instance method, which
will generate a reference like `parent:child` for a category,
with an optional depth, which hashtags use. Also refactors
PostRevisor which was using CategoryHashtagDataSource directly
which is a no-no.
Deletes the old hashtag markdown rule as well.
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.
We were attempting to fetch from last read but this is actually complicated to get right when you have a lot unread, as we might still have more to load after this but the last unread id is still the same and would make the user end up in a loop.
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.
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 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.
I was only able to get one failure out of 100 tries, this failure didn't get me more info. My best guess ATM is that sometimes, the first session was still loading while receiving the reaction and created some unexpected situation.
The commit attempts to start the "check" session before the session making the reaction hoping that will be enough to prevent this case, if this is the issue.
Sharing a link in chat will create a onebox embed with a source that includes a site icon and title.
This change prevents loading the site icon into lightbox.
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).
Turns out making a html4 fragment and then operating on parts of it using html5 fragments is a bad idea. ;)
This seems to fix the issue with occasionally missing GH icons in oneboxes.
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.
Followup to 07c3782e51
The above incorrectly removed the channel unread count in
the mobile/drawer channel list when the user has mentions
(meaning the unreads are urgent). This commit adds it
back and refactors system specs a little.
Define new concept of panels in sidebar. Panels are wrappers around sidebar sections. In the future, it allows creating full focus mode by switching between panels.
A new API method called addSidebarPanel was added. Default main panel is already registered and by default all API sections are mounted to main.
Why this change?
The test being changed in question has been flaky on our CI. However, we
are unable to view the screenshot of why it failed because
ActionDispatch will only take a screenshot of the default session upon
failure. At the same time, taking screenshot of all sessions
automatically upon failure is not possible via the official Capybara or
Rails APIs at the moment. Therefore, we're changing this system test to
avoid using two custom session and instead have the main assertion use
the default session such that any failures will provide us with a
screenshot.
This commit fixes two issues with the thread list:
1. All threads were being shown regardless of whether the user had
a membership in the thread. This was happening because the list
and the channel share the same thread store, so if the channel
had OMs with threads we would load them and they showed in the list.
2. Threads created by the user from a staged thread would double up.
This is because the _cache in the channel threadsManager would use
the staged thread ID even after we'd replaced the object's ID with
the actual thread from the DB. The answer to this is to remove and
re-add the thread to the local cache with the actual ID.
* DEV: Fix and re-enable chat flakys
The early return in JS was added to prevent an error
from channel being null, and it's better to use known
users for the message fabrications in the specs.
* DEV: Use travel_to in drawer spec for thread tracking
Sometimes in the system test the datetime that is last
viewed for the channel for the user and the datetime for
the last message created_at is only microseconds of difference,
and we do not provide that level of fidelity in the MessageBus
serializer, so unreadThreadsCountSinceLastViewed is not
accurate.
Better to just utilize travel_to and move forward 1 minute in
time before sending the new message to easily differentiate.
When we have subscriptions for new messages in a channel,
we also have special handling for messages in a thread. For
cases like DM channels where threads are made in the background
but not used in the UI, this is causing JS errors because we
are trying to fetch the thread but it returns 404.
We only want to do things with messages in threads if the
channel actually has threading enabled.
We need a nice way to only return some hashtag data
sources based on various site settings. This commit
adds an enabled? method that every hashtag data source
must implement. If this returns false the data source
will not be used at all for hashtag lookups or search.
On tablets like iPad where we allow channel and thread to be on the same screen, it was not possible to resize the panels due to code being thought for mouse events. This commit should now correctly allow for this.
The "resizer" has also been made larger to simplify touching.
No test as it's hard to test on iPad and dragging events are also complex.
On iOS we have a hack to prevent the viewport to move when focusing an input, however this code was targeting the textarea node through a global selector which is working fine on iOS as we only show one composer at a time but was failing on iPad as we show both channel and thread on the same screen. As a result `document.querySelector(".chat-composer__input")` was always targeting the first textarea on the screen which was the channel's composer, making it impossible to focus the thread's one.
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.
Why this change?
The test being changed in question has been flaky on our CI. However, we
are unable to view the screenshot of why it failed because
ActionDispatch will only take a screenshot of the default session upon
failure. At the same time, taking screenshot of all sessions
automatically upon failure is not possible via the official Capybara or
Rails APIs at the moment. Therefore, we're changing this system test to
avoid using two custom session and instead have the main assertion use
the default session such that any failures will provide us with a
screenshot.
Prior to this change you might end up in a loop where removing a channel would redirect you to this channel and as we auto-follow opened direct message channels, you could never unfollow this last direct message channel.
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.
Prior to this commit a long press on the image of a chat message would trigger both the actions menu and the contextual menu. This commit ensures we only show the contextual menu in this case.
No test as it's a quite complex behavior to reproduce (would need android for example).
This was causing this event to cause other touch events down the road. For example click a reaction above the composer when the message action was opened could cause the composer to gain focus after the reaction was made.
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.
Not sure how this was even working previously, since it's trying
to press the reply button on a thread original message, which doesn't
work, you need to click the indicator to open the thread.
Why this change?
The following test is flaky on our CI:
```
1) Navigation when sidebar is configured as the navigation menu when re-opening full page chat after navigating to a channel opens full page chat on correct channel
Failure/Error: measurement = Benchmark.measure { example.run }
expected "/" to equal "/chat/c/random-9/17"
```
The theory here is that system tests is running too fast that we're not
giving the href for the chat header icon a chance to update before
clicking on it. Therefore, we're adding an additional assertion to
assert that the link has the right href before clicking on it.
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
It is now safe to render the message excerpt as HTML since
it is no longer using text_entities: true in the server
PrettyText.excerpt call when creating the message excerpt
from the cooked HTML.
This will fix the issue of things like mentions showing
HTML code instead of the actual mention when replying,
and cannot be used to inject improper HTML like style tags
via XSS.
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.
* 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.
Browser capabilities are inherently unconnected to the lifecycle of our app. Making them formally available outside of the service means that they can safely be used in non-app-linked functions without needing risky hacks like `helperContext()` or `discourse-common/lib/get-owner`.
One example of where the old hacks were problematic is the `translateModKey()` utility function. This is called in the root of the `discourse/components/modal/keyboard-shortcuts-help` es6 module. If anything (e.g. a theme/plugin) caused that es6 module to be `require()`d before the application was booted, a fatal error would occur.
Following this commit, `translateModKey()` can safely import and access `capabilities` without needing to worry about the app lifecycle.
The only potential downside to this approach is that the capabilities data now persists across tests. If any tests need to 'stub' capabilities, they will need to revert their changes at the end of the test (e.g. by using Sinon to stub a property).
This commit also updates some legacy references from `capabilities:main` to `service:capabilities`.
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.
Why this change?
The specs are flaky on CI and we've unable to figure out why so we've
decided to skip them only on CI for now. The tests are still ran in our
internal build so we still have some protection in place.
Trying to fix two issues:
1. Sometimes the publish_new! event for update_thread_original_message
finishes running on the UI before the one for thread_created, in this
case we just want to do nothing because thread_created will fetch the
new thread along with its preview from the server if needed
2. Sometimes the thread GET and /read events were erroring because
last_reply on the thread was nil, this was potentially occuring because
the thread_created event was coming through to the UI before the rest
of MessageCreator was done, so we just move that after the big update
to set thread_id for the new and existing messages in the reply
chain
Why is this change being made?
We've decided that the previous "community" section should look more
like a primary section that holds the most important navigation links
for the site and the word "community" doesn't quite fit that
description. Therefore, we've made the decision to drop the
section heading for the community section.
As part of removing the section heading, the following changes are made
as well:
1. Button to customize the section has been moved to the "footer" of the
"More..." section when `navigation_menu` site setting is set to `sidebar`.
When `navigation_menu` is set to `header dropdown`, a button to customize
the section is shown inline.
2. The section will no longer be collapsable.
3. The title of the section is no longer customisable as it is no longer
displayed. As a technical note, we have not dropped any previous
customisations of the section's title previously in case we have to
bring back the header in the future.
4. The new topic button that was previously present in the header has
been removed alongside the header. Admins can add a custom section
link to the `/new-topic` route if there would like to make it easier for
users to create a new topic in the sidebar.
The `message_bus_channels` given to `MessageBus.last_ids(*message_bus_channels)` is not ordered, as a result the expectation of the tests could fail, this test ensures we check the contain of the input instead of content+order.
This commit also standardize the naming pattern of modals: `<Chat::Modal::FooBar />` and changes css class accordingly.
Co-authored-by: David Taylor <david@taylorhq.com>
When the loading slider is enabled, the rendering of `application.hbs` is slightly delayed compared to the old 'spinner' strategy. This means that if a route tried to render a dialog during its `model()` hook, the dialog wrapper element would not be present and an error would occur.
This commit detects that situation and delays rendering the error until the next runloop iteration. If the element is still not found, we print a useful error to the console.
In the long term, we should ideally convert the dialog service to use a pure-ember rendering strategy instead of leaning on a11y-dialog. But for now, this workaround should resolve the problems identified by the chat system specs.
It's way more common to have presence enabled than disabled, so we should have been making it the default from start.
This commit also changes the namespace of `<ChatUserAvatar />` into `<Chat::UserAvatar />` and refactors tests.
Sadly this function is one of the very hard to test codepaths of the app. We could in the future attempt to extract the content of the function to unit-test it.
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.
Chat drawer was using the `DiscourseURL` hook `afterRouteComplete`. This hook suffer from a very poor implementation which makes it very unreliable:
```javascript
if (typeof opts.afterRouteComplete === "function") {
schedule("afterRender", opts.afterRouteComplete);
}
```
This commit attempts to return the promise from `handleURL` to directly use it and have a very reliable after transition hook.
chapoi
Kris
Jarek Radosz
Joffrey JAFFEUX
Sérgio Saquetim
David Taylor
David Battersby
Alan Guo Xiang Tan
Discourse Translator Bot
Jan Cernik
Andrei Prigorshnev
Loïc Guitaut
Ted Johansson
Mark VanLandingham
Jordan Vidrine
Martin Brennan
Isaac Janzen
Roman Rizzi
Krzysztof Kotlarek
Joe