This PR primarily fixes this case:
- USER A message
- USER B message
- USER B reply to USER A message <-- not showing user info when it should
Moreover, this commit also improves the spec to correctly test more cases.
This commit is a major overhaul of how chat message actions work, to make it so they are reusable between the main chat channel and the chat thread panel, as well as many improvements and fixes for the thread panel.
There are now several new classes and concepts:
* ChatMessageInteractor - This is initialized from the ChatMessage, ChatMessageActionsDesktop, and ChatMessageActionsMobile components. This handles permissions about what actions can be done for each
message based on the context (thread or channel), handles the actions themselves (e.g. copyLink, delete, edit),
and interacts with the pane of the current context to modify the UI
* ChatChannelThreadPane and ChatChannelPane services - This represents the UI context which contains the
messages, and are mostly used for state management for things like message selection.
* ChatChannelThreadComposer and ChatChannelComposer - This handles interaction between the pane, the
message actions, and the composer, dealing with reply and edit message state.
* Scrolling logic for the messages has now been moved to a helper so it can be shared between the main channel pane and the thread pane
* Various improvements with the emoji picker on both mobile and desktop. The DOM node of each component is now located outside of the message which prevents a large range of issues.
The thread panel now also works in the chat drawer, and the thread messages have less
actions than the main panel, since some do not make sense there (e.g. moving messages to
a different channel). The thread panel title, excerpt, and message sender have also been removed
for now to save space.
This gives us a solid base to keep expanding on and fixing up threads. Subsequent PRs will
make the thread MessageBus subscriptions work and disable echo mode
for the initial release of threads.
Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
This commit adds a system to generate CSS variables and classes for categories
and hashtags, which will be used in an effort to remove baked icons for hashtags
and add color to those icons.
This is in two parts. First I added an initializer generate a category color CSS
variable style tag in the head tag that looks like this:
```css
:root {
--category-1-color: #0088CC;
--category-2-color: #808281;
--category-3-color: #E45735;
--category-4-color: #A461EF;
--category-5-color: #ee56c9;
--category-6-color: #da28c2;
--category-7-color: #ab8b0a;
--category-8-color: #45da37;
...
}
```
The number is the category ID. This only generates CSS variables for categories
the user can access based on `site.categories`. If you need the parent color variable
you can just use the `category.parentCategory.id` to get it.
Then, I added an initializer to generate a hashtag CSS style tag using these variables.
Only the category and channel hashtags need this, the category one generates the
background-gradient needed for the swatch, and the channel just generates a color
for the icon. This is done in an extendable way using the new `api.registerHashtagType`
JS plugin API:
```css
hashtag-color--category-1 {
background: linear-gradient(90deg, var(--category-1-color) 50%, var(--category-1-color) 50%);
}
hashtag-color--category-2 {
background: linear-gradient(90deg, var(--category-2-color) 50%, var(--category-2-color) 50%);
}
hashtag-color--category-5 {
background: linear-gradient(90deg, var(--category-5-color) 50%, var(--category-4-color) 50%);
}
...
.hashtag-color--channel-4 {
color: var(--category-12-color);
}
.hashtag-color--channel-92 {
color: var(--category-24-color);
}
```
Note if a category has a parent, its color is used in the gradient correctly. The numbers
here are again IDs (e.g. channel ID, category ID) and the channel’s chatable ID is used
to find the category color variable.
The translation key is built using the name of the reviewable as it was
defined in Ruby. The chat plugin uses the `Chat` namespace and defines
`Chat::ReviewableMessage`. This was then transformed to
`chat::reviewable_message`, but it should be `chat_reviewable_message`
to resemble the other translation keys.
When a chat message is trashed and the message is used
for someone's UserChatChannelMembership#last_read_message_id,
the user would end up with some read state issues until
someone posted a new message in the channel, since we didn't
clear it like we did on bulk message delete.
This commit fixes the issue, and also takes the opportunity
to start a MessagesController in the API namespace, and move
the trash message functionality into the new service format.
Followup to 0924f874bd,
we migrated Chat::Upload records to UploadReference records
there and have not been making new Chat::Upload records
for some time, we can now delete the model and table.
- Raises the scroll distance to 250px instead of 100px to show the arrow down button
- Always have a margin on drawer when showing channel list, removes this margin when the scrollbar is apparent
- Makes all scrollbar used in chat look the same through the chat-scrollbar mixin
- Ensures hover state is not persistent on channel row in mobile
- Makes the channel row full width on mobile
We keep getting this failure on the spec but I
cannot reproduce locally, add this extra log line
to see if it helps:
```
> Chat::Api::ChatablesController#index with chat permissions does not return DM channels for users who are not in the chat allowed group
> Failure/Error: example.run
>
> expected: 200
> got: 500
>
> (compared using ==)
> # ./plugins/chat/spec/requests/chat/api/chatables_controller_spec.rb:158:in `block (4 levels) in <main>'
> # ./spec/rails_helper.rb:358:in `block (2 levels) in <top (required)>'
> # ./vendor/bundle/ruby/3.2.0/gems/webmock-3.18.1/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'
> # ------------------
> # --- Caused by: ---
> #
> # expected: 200
> # got: 500
> #
> # (compared using ==)
> # ./plugins/chat/spec/requests/chat/api/chatables_controller_spec.rb:158:in `block (4 levels) in <main>'
```
This adds specs to the mentioned serializers to catch regressions
with MessageBus last_ids and to ensure the correct ones are being
returned and passed down to the ChannelSerializer.
Followup to d8ad5c3
Followup to 3ea8df4b06,
I forgot to wrap the call to Chat::Publisher.root_message_bus_channel(object.chat_channel.id)
in MessageBus.last_id, so the channel_message_bus_last_id key was
ending up as e.g. "/chat/58" instead of the last ID value.
We noticed via profiling that chat was doing N redis calls
per channel. Part of this was from the kick_message_bus_last_id
from 520d4f504b being incorrectly
passed down for DM channels rather that public channels, and the
other part was from the root MessageBus channel last_id
being fetched in ChannelSerializer for every single channel.
This commit fixes both issues, for me going from 134 redis calls
on page load to 20 locally.
Also deletes an old file missed in 12a18d4d55
Followup cab4b2cfba,
this was causing client JS errors because the old version
of the client was expecting the old keys, but the new
ruby version of the app was sending different keys via
the MessageBus payload. We can remove this in a couple
of weeks.
This commit introduces a Chat::Publisher and MessageBus endpoint
that allows for updating a user's channel tracking state in bulk for
multiple channels, rather than having to do it for one channel
at a time.
This also required an improvement to ChannelUnreadsQuery -- now
multiple channel IDs can be passed to this to get the unread counts
and mention counts for those channels for a user, also increasing
efficiency rather than having to do a query for every individual
channel.
Followup to #20802
Instead of just marking the state read in JS for each channel
after the AJAX call, we can instead just rely on the MessageBus
user-tracking-state chat channel, and publish the state to all
the channels affected in MarkAllUserChannelsRead. This will make
it so the blue dots for the channels are cleared across all tabs.
This manual set was happening only after the request so was not much faster than just waiting on message bus update. It's not by itself changing any behavior or fixing any bug but it makes reasoning about the whole state easier as it happens in only one central place.
Followup to a0381157e9, we just
need to make sure we set currentUserMembership.last_read_message_id
to the last_read_message_id from the updated memberships after marking
all channels read, otherwise we do not scroll to the bottom and still
show the "last visit" separators in channels that have been
marked read.
This commit adds a keyboard shortcut (Shift+ESC) for chat which marks all
of the chat channels that the user is currently a following member of as read,
updating their `last_read_message_id`. This is done via a new service.
It also includes some refactors and controller changes:
* The old mark message read route from `ChatController` is now supplanted
by the `Chat::Api::ReadsController#update` route.
* The new controller can handle either marking a single or all messages read,
and uses the correct service based on the route and params.
* The `UpdateUserLastRead` service is now used (it wasn't before), and has been slightly
updated to just use the guardian user ID.
These were added in 7dd317b875
but are now consistently failing with described_class.chat_summary(user, {})
returning nil. Skipping for now because they are holding up other
things.
To reproduce failure run:
RSPEC_SEED=46586 bundle exec rake plugin:spec
There are many situations that may cause users to lose permission to
send messages in a chat channel. Until now we have relied on security
checks in `Chat::ChatChannelFetcher` to remove channels which the
user may have a `UserChatChannelMembership` record for but which
they do not have access to.
This commit takes a more proactive approach. Now any of these following
`DiscourseEvent` triggers may cause `UserChatChannelMembership`
records to be deleted:
* `category_updated` - Permissions of the category changed
(i.e. CategoryGroup records changed)
* `user_removed_from_group` - Means the user may not be able to access the
channel based on `GroupUser` or also `chat_allowed_groups`
* `site_setting_changed` - The `chat_allowed_groups` was updated, some
users may no longer be in groups that can access chat.
* `group_destroyed` - Means the user may not be able to access the
channel based on `GroupUser` or also `chat_allowed_groups`
All of these are handled in a distinct service run in a background
job. Users removed are logged via `StaffActionLog` and then we
publish messages on a per-channel basis to users who had their
memberships deleted.
When the user has a channel they are kicked from open, we show
a dialog saying "You no longer have access to this channel".
When they click OK we redirect them either:
* To their first other public channel, if they have any followed
* The chat browse page if they don't
This is to save on tons of requests from kicked out users getting messages
from other channels.
When the user does not have the kicked channel open, we can just
silently yoink it out of their sidebar and turn off subscriptions.
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>
Prior to this fix, we wouldn't display reactions done on different tabs. So, if user A was reacting on tab 1, tab 2 wouldn't display this reaction.
Since few weeks ago we now have the guarantee to have uniq reactions on a message which should prevent any duplicate.
This commit also removes various skipped tests related to reactions and makes `sign_in` explicit at the beginning of each test.
<!-- 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. -->
This commit takes advantage of the `ResizeObserver` to know when dates should be re-computed, it works like this:
```
scrollable-div
-- child-enclosing-div with resize observer
---- message 1
---- message 2
---- message x
```
It also switches to bottom/height for date separators sizing, instead of bottom/top, it prevents a bug where setting the top of the first item (at the top) would cause scrollbar to move to top.
<!-- 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. -->
This commit does a couple of things:
* Adds the ability to load messages in the chat thread panel when it is open. This just loads the most recent N messages, same as a channel, and does nothing more, no scrolling or anything like that.
* Displays the messages in an extremely simple unordered list with no additional features.
* Allows posting new messages to the thread, and echoes them into the main channel, but does not respond to any sort of MessageBus events.
I've moved messages/clearMessages/addMessages/findMessage code out of the `ChatChannel` model
and into a new `ChatMessagesManager` class, which is instantiated in both the `ChatChannel` model
and the `ChatThread` model. This allows both to manage messages in the same way via the
`TrackedArray` pattern.
This is all hidden behind experimental flags, there is no way to make this not completely broken
in a single commit. Much more work and refactoring needs to be done first.
Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
This behavior is hard to test as it's mostly fixing a race condition: User A sends a message at the same time than User B, which as a result doesn't cause a scroll for the second message and we don't update last read unless we do a small up and down scroll.
`updateLastRead` is debounced so it has no direct consequences to call it slightly more often than what should ideally be needed.
Prior to this fix, the upload was removed from DOM when collapsed and not decorated again on expand, which was causing lightbox to not get reapplied. The fix is reverting to previous state where content was not removed from DOM.
Prior to this change `registered_bookmarkable` would return `nil` as `type` in `Bookmark.registered_bookmarkable_from_type(type)` would be `ChatMessage` and we registered a `Chat::Message` class.
This commit will now properly rely on each model `polymorphic_class_for(name)` to help us infer the proper type from a a `bookmarkable_type`.
Tests have also been added to ensure that creating/destroying chat message bookmarks is working correctly.
---
Longer explanation
Currently when you save a bookmark in the database, it's associated to another object through a polymorphic relationship, which will is represented by two columns: `bookmarkable_id` and `bookmarkable_type`. The `bookmarkable_id` contains the id of the relationship (a post ID for example) and the `bookmarkable_type` contains the type of the object as a string by default, (`"Post"` for example).
Chat plugin just started namespacing objects, as a result a model named `ChatMessage` is now named `Chat::Message`, to avoid complex and risky migrations we rely on methods provided by rails to alter the `bookmarkable_type` when we save it: we want to still save it as `"ChatMessage"` and not `"Chat::Message"`. And, to retrieve the correct model when we load the bookmark from the database: we want `"ChatMessage"` to load the `Chat::Message` model and not the `ChatMessage`model which doesn't exist anymore.
On top of this the bookmark codepath is allowing plugins to register types and will check against these types, so we alter this code path to be able to do a similar ChatMessage <-> Chat::Message dance and allow to check the type is valid. In the specific case of this commit, we were retrieving a `"ChatMessage"` bookmarkable_type from the DB and looking for it in the registered bookmarkable types which contain `Chat::Message` and not `ChatMessage`.
This commit main goal was to comply with Zeitwerk and properly rely on autoloading. To achieve this, most resources have been namespaced under the `Chat` module.
- Given all models are now namespaced with `Chat::` and would change the stored types in DB when using polymorphism or STI (single table inheritance), this commit uses various Rails methods to ensure proper class is loaded and the stored name in DB is unchanged, eg: `Chat::Message` model will be stored as `"ChatMessage"`, and `"ChatMessage"` will correctly load `Chat::Message` model.
- Jobs are now using constants only, eg: `Jobs::Chat::Foo` and should only be enqueued this way
Notes:
- This commit also used this opportunity to limit the number of registered css files in plugin.rb
- `discourse_dev` support has been removed within this commit and will be reintroduced later
<!-- 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. -->
Mentions are now displayed as using the non-cooked message which fixes
the problem. This is not ideal. I think we might want to rework how
these excerpts are created and rendered in the near future.
Co-authored-by: Jan Cernik <jancernik12@gmail.com>
Non-markdown tags weren't being escaped in chat excerpts. This could be
triggered by editing a chat message containing a tag (self XSS), or by
replying to a chat message with a tag (XSS).
Co-authored-by: Jan Cernik <jancernik12@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.
Usage:
```javascript
api.addChatDrawerStateCallback(({ isDrawerActive, isDrawerExpanded }) => {
// do something
});
```
Note this commit also uses this new API to add a css class (chat-drawer-active) on the body when the drawer is active.
This fix uses direct `scrollTop` manipulation instead of `scrollIntoView` when we are certain we actually want the bottom of the screen. This avoids a range of issues especially in safari but also chrome where the scroll position was not correct at the end of `scrollIntoView`, especially due to images.
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.
* DEV: Change sidebar header dropdown to use wait_for_animation
Introduced in 54351e1b8a, this
helper should remove the need to have to add the .animated
CSS class in JS for the sidebar.
* DEV: Revert spec change
`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).
- Will consider a message read only one the bottom of the message has been read
- Will allow to mark a message bigger than the view port as read
- Code should be more performant as the scroll is doing less (albeit more often)
- Gives us a very precise scroll state. Problem with throttling scroll is that you could end up never getting the even where scrollTop is at 0, opening a whole range of edge cases to handle
1. Restore the left margin on both (which reflects the right margin of the scroll bar space)
2. Fix the center alignment of scroll-to-bottom icon
3. Fix the spacing of the `-` character between a date label and "last visit" label
4. Fix the incorrect display the border on date label when at the bottom of viewport
When we introduced `existingUploads` as an arg to the
ChatComposerUploads component, we also introduced a bug where
if multiple uploads were being done at once, and the draft
was saved, then because of didReceiveAttrs we would cancel
the currently uploading files because the draft uploads became
the existingUploads.
To work around this, since we do want to keep this on didReceiveAttrs
for cases when the user opens a draft or edits another message,
the easiest thing to do is to just not save uploads into the chat
draft if there are still uploads in progress. That way only when
all uploads are complete do we make them a part of the draft.
There is a small risk that the user could do something to lose
their uploads in the draft, but it's a better gamble to have
that happen rather than in progress uploads to be cancelled
while the user is waiting for them to be done because of the
draft.
Also changes the uploads system spec back to the old way of
attaching multiple files since that is why it was failing.
This is used when calling click_message_action_mobile to wait
for the message actions menu to finish animating up before
attempting to click on it using capybara. Without this, in
the time between capybara getting the x,y position of a menu
item to click on and the click being fired, the animating menu
can move that item out of the way.
With the new helper, we constantly compare x,y client rect positions
for the animating element and wait for them to stabilise. Once they
do, it means the animation is done, and it is safe to click on
anything within the element.
Re-enables mobile system specs for chat that were ignored because
of this.
Similar spirit to e195e6f614,
this moves the Bookmarkable registration to DiscoursePluginRegistry
so plugins which are not enabled do not register additional
bookmarkable classes.
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.
- group writes when computing separators positions
- shows skeleton only on initial load
- forces date separator to be pinned when first message to prevent a pinned - not pinned - pinned sequence when loading more in past
- relies on `message.visible` property instead of checking `isElementInViewport`
- attempts to load next/prev messages earlier
- do not scroll to on fetch more
- hides `last visit` text while pinned
Note this test my prove to be flakey, so I might have to remove it or find a different solution. It's extremely complicated to test for something which shouldn't appear in a period of time and is not a present at T=0
This was possible due to specific events which are hard to represent in a test. The provided test is as close as possible to what was happening in production: a message bus event was played on a channel which has just loaded its state with the existing reaction.
On require login sites, the `site` is not setup and as a result `hashtag_configurations` was blank and causing an error when attempting to access `["chat-composer"]` on it.
This PR is introducing glimmer usage in the chat-live-pane, for components but also for models. RestModel usage has been dropped in favor of native classes.
Other changes/additions in this PR:
sticky dates, scrolling will now keep the date separator of the current section at the top of the screen
better unread management, marking a channel as unread will correctly mark the correct message and not mark the whole channel as read. Tracking state will also now correctly return unread count and unread mentions.
adds an animation on bottom arrow
better scrolling behavior, we should now always correctly keep the scroll position while loading more
reactions are now more reactive, and will update their tooltip without needed to close/reopen it
skeleton has been improved with placeholder images and reactions
when making a reaction on the desktop message actions, the menu won't move anymore
simplify logic and stop maintaining a list of unloaded messages
This PR is introducing glimmer usage in the chat-live-pane, for components but also for models. RestModel usage has been dropped in favor of native classes.
Other changes/additions in this PR:
- sticky dates, scrolling will now keep the date separator of the current section at the top of the screen
- better unread management, marking a channel as unread will correctly mark the correct message and not mark the whole channel as read. Tracking state will also now correctly return unread count and unread mentions.
- adds an animation on bottom arrow
- better scrolling behavior, we should now always correctly keep the scroll position while loading more
- reactions are now more reactive, and will update their tooltip without needed to close/reopen it
- skeleton has been improved with placeholder images and reactions
- when making a reaction on the desktop message actions, the menu won't move anymore
- simplify logic and stop maintaining a list of unloaded messages
* UX: add type tag and design update
* UX: clarify status copy in reviewQ
* DEV: switch to selectKit
* UX: color approve/reject buttons in RQ
* DEV: regroup actions
* UX: add type tag and design update
* UX: clarify status copy in reviewQ
* Join questions for flagged post with "or" with new I18n function
* Move ReviewableScores component out of context
* Add CSS classes to reviewable-item based on human type
* UX: add table header for scoring
* UX: don't display % score
* UX: prefix modifier class with dash
* UX: reviewQ flag table styling
* UX: consistent use of ignore icon
* DEV: only show context question on pending status
* UX: only show table headers on pending status
* DEV: reviewQ regroup actions for hidden posts
* UX: reviewQ > approve/reject buttons
* UX: reviewQ add fadeout
* UX: reviewQ styling
* DEV: move scores back into component
* UX: reviewQ mobile styling
* UX: score table on mobile
* UX: reviewQ > move meta info outside table
* UX: reviewQ > score layout fixes
* DEV: readd `agree_and_keep` and fix the spec tests.
* Fix the spec tests
* fix the quint test
* DEV: readd deleting replies
* UX: reviewQ copy tweaks
* DEV: readd test for ignore + delete replies
* Remove old
* FIX: Add perform_ignore back in for backwards compat
* DEV: add an action alias `ignore` for `ignore_and_do_nothing`.
---------
Co-authored-by: Martin Brennan <martin@discourse.org>
Co-authored-by: Vinoth Kannan <svkn.87@gmail.com>
This PR is introducing glimmer usage in the chat-live-pane, for components but also for models. RestModel usage has been dropped in favor of native classes.
Other changes/additions in this PR:
- sticky dates, scrolling will now keep the date separator of the current section at the top of the screen
- better unread management, marking a channel as unread will correctly mark the correct message and not mark the whole channel as read. Tracking state will also now correctly return unread count and unread mentions.
- adds an animation on bottom arrow
- better scrolling behavior, we should now always correctly keep the scroll position while loading more
- reactions are now more reactive, and will update their tooltip without needed to close/reopen it
- skeleton has been improved with placeholder images and reactions
- when making a reaction on the desktop message actions, the menu won't move anymore
- simplify logic and stop maintaining a list of unloaded messages
This commit allows the user to set their preference vis-a-vis
the chat icon in the header of the page. There are three options:
- All New (default) - This maintains the existing behaviour where
all new messages in the channel show a blue dot on the icon
- Direct Messages and Mentions - Only show the green dot on the
icon when you are directly messaged or mentioned, the blue dot
is never shown
- Never - Never show any dot on the chat icon, for those who
want tractor-beam-laser-focus
Joffrey JAFFEUX
Daniel Waterworth
Martin Brennan
Discourse Translator Bot
Bianca Nenciu
chapoi
Jarek Radosz
Jan Cernik
Penar Musaraj
Alan Guo Xiang Tan
Andrei Prigorshnev
Kris
Blake Erickson