This commit attempts to have a bullet proof solution to the following case:
- long press on message (finger is still pressed)
- menu appears
- a button is now at finger location
- user releases finger
- a click is triggered on the button
Classic event canceling solution won't work here for performance reasons as we need the event to be passive in a scroll list.
This fixes an issue where a user could send an empty
string as a chat message .e.g ' ' and the message would
be posted. We don't want this, we need to strip the message
first before validating for length etc.
This commit contains multiple changes to improve the composer behavior especially in the context of a thread:
- Generally rename anything of the form `chatChannelThread...` to `chatThread...``
- Moves the textarea interactor instance inside the composer server
- Improves the focus state and closing of panel related to the use of the Escape shortcut
- Creates `Chat::ThreadList` as a component instead of having `Chat::Thread::ListItem` and others which could imply they were children of a the `Chat::Thread` component
One user can create a post or chat message with a hashtag they
have permission to use, but then when other users look at that
post they will see an empty space next to the hashtag because they
do not have the permission to load the colors in CSS classes for
the related category.
This fixes the issue by adding a default color with a special
CSS class if the user doesn't have permission to see the linked
channel/category on the hashtag.
What is the problem?
We were calling out to methods that calls `has_css?` or `has_selector?`
which returns a boolean. Since we are not using the return value, it
means the methods can be deemed unnecessary. However, we do want those
checks and this commit adds the necessarily assertions to make use of
the return values.
This reverts commit ddf4ecba04.
Causing a flaky test to appear:
```
main $ LOAD_PLUGINS=1 rspec plugins/chat/spec/system/chat/composer/shortcuts/channel_spec.rb
Randomized with seed 17765
.....F..
Failures:
1) Chat | composer | shortcuts | channel when using ArrowUp when last message is staged does not edit a message
Failure/Error: channel_page.send_message
expected `#<PageObjects::Components::Chat::Messages:0x00007fe823ac1710 @context=".chat-channel">.has_message?({:persisted=>true, :text=>"2"})` to be truthy, got false
[Screenshot Image]: /home/tgxworld/work/discourse/tmp/capybara/failures_r_spec_example_groups_chat_composer_shortcuts_channel_when_using_arrow_up_when_last_message_is_staged_does_not_edit_a_message_148.png
```
What is the problem?
We were calling out to methods that calls `has_css?` or `has_selector?`
which returns a boolean. Since we are not using the return value, it
means the methods can be deemed unnecessary. However, we do want those
checks and this commit adds the necessarily assertions to make use of
the return values.
If we're asserting that something is missing, we want to use
`has_no_css?` instead of `!has_css?` since `has_css?` will wait the full
capybara default wait time before return if the selector is not present.
* FEATURE: reduce avatar sizes to 6 from 20
This PR introduces 3 changes:
1. SiteSetting.avatar_sizes, now does what is says on the tin.
previously it would introduce a large number of extra sizes, to allow for
various DPIs. Instead we now trust the admin with the size list.
2. When `avatar_sizes` changes, we ensure consistency and remove resized
avatars that are not longer allowed per site setting. This happens on the
12 hourly job and limited out of the box to 20k cleanups per cycle, given
this may reach out to AWS 20k times to remove things.
3.Our default avatar sizes are now "24|48|72|96|144|288" these sizes were
very specifically picked to limit amount of bluriness introduced by webkit.
Our avatars are already blurry due to 1px border, so this corrects old blur.
This change heavily reduces storage required by forums which simplifies
site moves and more.
Co-authored-by: David Taylor <david@taylorhq.com>
- Made the emoji btn blue when composer is focused
- Moved everything chat-composer-button to its own file and BEM-ified it and making the choice to only work with our own is-disabled definition instead of with the attribute :disabled, for consistency
This should fix this failure:
```
Failures:
1) Thread list in side panel | full page when there are no threads that the user is participating in shows a message
Failure/Error: measurement = Benchmark.measure { example.run }
expected to find text "You are not participating in any threads in this channel." in "Community\nEverything\nMy Posts\nMore\nMessages\nInbox\nChannels\nRandom 25\nPersonal chat\nRandom 25\nShowing all messages\nOngoing discussions"
```
The screenshot failure was clearly showing the spinner still being present.
Rescuing them still makes timing-out tests fail but doesn't break `after` spec cleanup (which could trigger more errors) Using custom error class to avoid any other possible timeout-catching code.
Also:
* remove an unnecessary `.select { |x| x.size > 0 }`
* fix a typo in a test title
We were calling reset without the proper params which was causing errors in the console. This commit does the following changes:
- ensures `composer.cancel()` is the only way to cancel editing/reply
- adds a `draftSaved` property to chat message to allow for better tests
- writes a spec to ensure the flow is correct
- adds more page objects for better tests
- homogenize the default state of objects on chat message
Co-authored-by: Martin Brennan <martin@discourse.org>
Editing a message to an empty string and sending it, will delete it.
This commit also refactors a lot of channel/thread composer shortcuts specs.
---
This commit also includes various spec fixes which have been flakey while finishing this pull request.
These specs were disabled in 786f7503. While investigating this, I found out that at some point `:user_membership` got deleted. It's hard to tell why exactly without investing more time, but it seems using `let!` instead of `fab!` solves the issue.
If in the future we decide to investigate why these tests were flaky with `fab!` to reproduce the failure run:
LOAD_PLUGINS=1 rspec --seed 46586 plugins/chat/spec/mailers/user_notifications_spec.rb
This changes the thread header positioning of the
unread indicator to match the designs based on the route:
1. When the channel is open, show the indicator of # unread
threads with the icon
2. When the threads list is open, show no indicator since
you are on the list and can see which threads are unread
3. When a single thread is open, show the unread threads
indicator along with a left < back button, with a label
to show that this goes back to ongoing discussions
Drawer changes to come in another PR.
Why is this change required?
In the `PageObjects::Components::Chat::Messages#has_no_message?` method,
it ended up calling `has_selector` when trying to assert that the
selector is not present. This is an anti-pattern which results in us
waiting the full Capybara default wait time
What is this change required?
In the `chat/spec/system/transcript_spec.rb` test, there is a helper
method that uses `page.has_css?` in a conditional but it do not
specify a wait time and hence the default Capybara default max wait
time is used. However, there is no need for us to be waiting here so
we specify the `wait: 0` option.
Followup 55ef2d0698.
In the cases where the user has no last_read_message_id for
a channel, we want to make sure that a page_size is set for
the ChannelViewBuilder + MessagesQuery, otherwise we end up
loading way more messages than needed (the additional message
loading was fixed in the last commit).
This commit introduces a couple of changes:
1. When editing a chat channel's slug, we were using `this.model.set("title", title)` when the `set`
function does not exist. This was actually throwing the error in the
"can edit slug" system test where the modal was not closed after
saving and was flashing an error.
2. Introduce `PageObjects::Pages::ChatChannelAbout` and
`PageObjects::Modals::ChatChannelEdit` page object to encapsulate
logic better.
When a thread is created / a new message is created in the
thread, we want to make sure that the original message user
has a membership for that thread, otherwise they will not
receive unread indicators for messages in the thread.
This commit attempts to fix the case where the messages loaded initially don't fill the screen. It would prevent user to scroll and as a result to load more.
There are multiple fixes in this commit:
- the main fix is removing this code which was preventing the actual fill:
```javascript
// prevents an edge case where user clicks bottom arrow
// just after scrolling to top
if (loadingPast && this.#isAtBottom()) {
return;
}
```
- ensures we always give a page site to the `chatApi.channel(...)` call if we have one, in the current state when `fetchFromLastRead` was `true` we would not set `args.page_size`
- ensures the `query_paginated_messages` is having a valid page size, which is not nil and not > `MAX_PAGE_SIZE`
- write a spec for the autofill, it was a challenging spec to write but it should give us the confidence we need here
Since 5cce829 and the new
channel view builder, we have no need of these obsolete
routes which have way too much logic in the controller, which
has been superseded by the view builder anyway.
Remove the routes and update the channel message loading to use it.
* Moved the settings cog from thread list to thread and
put it in a new header component
* Remove thread original message component, no longer needed
and the list item and thread indicator styles/content
will be quite different
* Start adding content (unread indicator etc.) to the thread
list item and changing structure to be more like designs
* Serialize the last thread reply when opening the thread index,
show in list and update with message bus
The current behavior is to close drawer when pressing escape inside the input.
After this change, first escape will blur the input, and second escape will close the drawer.
This commit also refactors the whole shortcuts for drawer system spec.
Followup to d4a5b79592,
this introduced an N1 because every message in the list
we had to query users for the mentions and then the user's
status too. Instead we can just include both in Chat::MessagesQuery.
#### FIX: Do not use client lastReadMessageId when fetching channel messages
We had an issue where the following happened:
1. User opened channel and saw the last message, and we set the
lastReadMessageId on the server and the client
2. User navigated to another channel
3. Another user deleted the message in the original channel
4. The first user navigated back to the original channel before
the MessageBus event for the deleted message arrived, and got
a 404 error because we were sending the deleted lastReadMessageId as
target_message_id to the channel controller.
Instead of this which is a bit flaky and is hard to cover all
the issues for, instead we can pass a fetch_from_last_read boolean
param to the channels controller, and just get the user's
last_read_message_id straight from the database to use for the
target_message_id. This gets rid of any sources of race conditions
or lack of updates from MessageBus.
#### FIX: Include missing memberships for thread tracking publish
When we publish the channel/message tracking state for a
user and that message was a thread reply the publisher
was erroring because we were not telling Chat::TrackingStateReportQuery
to return missing memberships (which have zeroed out unread counts)
as well, which is what we do for the channel tracking state here.
Also just make sure that the TrackingStateReport does not error
when passed an ID it doesn't have data for.
This flakey has been very visible by the new headless chrome. The problem was that after moving a message we automatically redirect to the channel where the message has been moved to. However, we were not explicitly waiting for this transition and a result it could happen that we attempt to check the presence of the message on the channel page before the redirect actually happened.
The various naming changes are due to an early mistake we made in chat specs to use `chat` as the variable name for the page object which prevents to use the automatic path `chat.channel_path(...)`.
Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
This commit follows up b6c5a2da08
by serializing the user's thread memberships in these cases:
1. When we do the initial channel fetch with messages, we get
all threads and all the user's thread memberships for those
messages.
2. When the thread list is fetched, we get all the user's memberships
in that list.
3. When the single thread is fetched, either from opening it from
the list, an OM indicator, or just from doing .find() on the
manager when a new MessageBus message comes in
This will let us track the lastReadMessageId on the client, and
will also let us fix an issue where the unread indicator in the
channel header was incrementing for every thread that got a
new message, regardless of whether the user was a member.
This patch introduces policy objects to chat services. It allows putting
more complex logic in a dedicated class, which will make services
thinner. It also allows providing a reason why the policy failed.
Some change has been made to the service runner too to use more easily
these new policy objects: when matching a failing policy (or any failing
step actually), the result object is now provided to the block. This
way, instead of having to access the reason why the policy failed by
doing `result["result.policy.policy_name"].reason` inside the block,
this one can be simply written like this:
```ruby
on_failed_policy(:policy_name) { |policy| policy.reason }
```
This commit adds the thread index and individual thread
in the index list unread indicators, and wires up the message
bus events to mark the threads as read/unread when:
1. People send a new message in the thread
2. The user marks a thread as read
There are several hacky parts and TODOs to cover before
this is more functional:
1. We need to flesh out the thread scrolling and message
visibility behaviour. Currently if you scroll to the end
of the thread it will just mark the whole thread read
unconditionally.
2. We need to send down the thread current user membership
along with the last read message ID to the client and
update that with read state.
3. We need to handle the sidebar unread dot for when threads
are unread in the channel and clear it based on when the
channel was last viewed.
4. We need to show some indicator of thread unreads on the
thread indicators on original messages.
5. UI improvements to make the experience nicer and more
like the actual design rather than just placeholders.
But, the basic premise around incrementing/decrementing the
thread overview count and showing which thread is unread
in the list is working as intended.
This should also make `message_notifications_with_sidebar_spec.rb` more resilient as we are now checking for `is-persisted` class instead of checking for the absence of `is-staged`.
This commit attempts to correctly change draft when the channel changes. It moves responsibility to the composer instead of the channel.
A new service `chatDraftsManager` is being introduced here to allow finer control and pave the way for future thread draft support.
These changes also now allow an editing message to be stored as a draft.
This PR adds status to mentions in chat and makes those mentions receive live updates.
There are known unfinished part in this implementation: when posting a message, status on mentions on that message appears immediately, but only if a user used autocomplete when typing the message. If user copy and paste a message with mentions into chat composer, those mentions won't have user status on them.
PRs with fixes for both problems are following soon.
Preparations for this PR that were made previously include:
- DEV: correct a relationship – a chat message may have several mentions 0dcfd7ddec
- DEV: extract the logic for extracting and expanding mentions from ChatNotifier 75b81b6854
- DEV: Always create chat mention records fa543cda06
- DEV: better split create_notification! and send_notifications logic e292c45924
- DEV: more tests for mentions when updating chat messages e7292e1682
- DEV: extract updating status on mentions into a lib function e49d338c21
- DEV: Create and update chat message mentions earlier 35a414bb38
- DEV: Create a chat_mention record when self mentioning 2703f2311a
- DEV: When deleting a chat message, do not delete mention records f4fde4e49b
In the ChannelViewBuilder, we introduced a check to see if
the target message exists, which errors if the message has
been trashed. However if the user is the creator of the message
or admin then they are able to see trashed messages, so
we need to take this into account.
Followup to c908eeacc9
Instead of using the latest message ID in the channel, which
could cause issues if you have an earlier last read message ID
that matches the deleted one, instead we use the first non-deleted
message that comes before the deleted message by ID.
Followup ae3231e140, when a
message is trashed we already update the lastReadMessageId of
all users in the channel to the latest non-deleted message on
the server side. However we didn't propagate this to the client,
so in some cases when we did the following:
1. Delete the last message in the channel
2. Switch to another channel
3. Switch back to the original
We would get a 404 error from the target message ID being looked
up still being the old lastReadMessageId (now deleted) for the
user's channel membership.
All we need to do is send the last not-deleted message ID for
the channel (or thread) to all the member users.
It easier to check for presence in this case that to check for something not present, as depending on performance of the machine running the test this could take sometime to be changed and the test would fail.
