The fix use the SelectKit component in the spec and improves reliability of SelectKit component to ensure expanded/collapsed state effectively set/present.
Multiple lines have also been removed as they are not necessary, eg:
- check button is present
- click button
The check is un-necesssary as we won't find the button on click if not present. This kind of checks are only necessary when another element has to be present before the button is show, eg:
- check modal is displayed
- click button
FInally this commit changes the way the SelectKit component initializes component and now uses a css selector instead of a finder, it ensures we are always getting fresh object and allows to build complete selectors: ".context[data-id=1].foo:enabled"
Watching screenshots of failures it appears that sometimes the reply was stuck at: `this is a #` due to the autocomplete showing, given we only need to send a reply here? I think fill_in + click send should be more reliable here.
Note I also tweaked `send_reply` to accept a content param and use it to fill composer when given.
Usage:
```
CHROME_DEV_TOOLS=bottom bundle exec rspec /path/to/system/spec
```
This commit also regroups common chrome options under `apply_base_chrome_options`, and removes the size of the mobile window which was incorrect. browser_log param is also passed to mobile chrome options.
Page settings can be very long and the setting can be way out of viewport. This should ensure the setting can be found and clicked.
This was for example causing this flakey in discourse-topic-voting:
```
Failures:
1) Voting enables voting in category topics and votes
Failure/Error: find(".edit-category-tab .#{setting} label.checkbox-label", text: text).click
Capybara::ElementNotFound:
Unable to find css ".edit-category-tab .enable-topic-voting label.checkbox-label"
[Screenshot Image]: /__w/discourse/discourse/tmp/capybara/failures_r_spec_example_groups_voting_enables_voting_in_category_topics_and_votes_873.png
~~~~~~~ JS LOGS ~~~~~~~
http://localhost:31341/favicon.ico - Failed to load resource: the server responded with a status of 404 (Not Found)
~~~~~ END JS LOGS ~~~~~
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/node/finders.rb:312:in `block in synced_resolve'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/node/base.rb:84:in `synchronize'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/node/finders.rb:301:in `synced_resolve'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/node/finders.rb:60:in `find'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/session.rb:773:in `find'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/dsl.rb:52:in `call'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/dsl.rb:52:in `find'
# ./spec/system/page_objects/pages/category.rb:34:in `toggle_setting'
# ./plugins/discourse-topic-voting/spec/system/voting_spec.rb:39:in `block (2 levels) in <main>'
# ./spec/rails_helper.rb:368:in `block (3 levels) in <top (required)>'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:189:in `block in timeout'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:36:in `block in catch'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:36:in `catch'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:36:in `catch'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:198:in `timeout'
# ./spec/rails_helper.rb:364:in `block (2 levels) in <top (required)>'
# ./spec/rails_helper.rb:356: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)>'
Finished in 7 minutes 18 seconds (files took 0 seconds to load)
445 examples, 1 failure, 17 pending, 1 error occurred outside of examples
Failed examples:
rspec ./plugins/discourse-topic-voting/spec/system/voting_spec.rb:27 # Voting enables voting in category topics and votes
```
In tests we could attempt to click the preview button when it was not enabled yet, causing a noop and a future failure. This fix ensures we try to find (and wait) for an enabled button.
This should help with this specific flakey:
```
Failures:
1) Admin Customize Form Templates when visiting the page to create a new form template should render all the input field types in the preview
Failure/Error: find(".form-template-field__#{type}").present?
Capybara::ElementNotFound:
Unable to find css ".form-template-field__input"
[Screenshot Image]: /__w/discourse/discourse/tmp/capybara/failures_r_spec_example_groups_admin_customize_form_templates_when_visiting_the_page_to_create_a_new_form_template_should_render_all_the_input_field_types_in_the_preview_277.png
~~~~~~~ JS LOGS ~~~~~~~
http://localhost:31339/favicon.ico - Failed to load resource: the server responded with a status of 404 (Not Found)
~~~~~ END JS LOGS ~~~~~
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/node/finders.rb:312:in `block in synced_resolve'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/node/base.rb:84:in `synchronize'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/node/finders.rb:301:in `synced_resolve'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/node/finders.rb:60:in `find'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/session.rb:773:in `find'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/dsl.rb:52:in `call'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/dsl.rb:52:in `find'
# ./spec/system/page_objects/pages/form_template.rb:55:in `has_input_field?'
# ./spec/system/admin_customize_form_templates_spec.rb:114:in `block (3 levels) in <main>'
# ./spec/rails_helper.rb:368:in `block (3 levels) in <top (required)>'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:189:in `block in timeout'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:36:in `block in catch'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:36:in `catch'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:36:in `catch'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:198:in `timeout'
# ./spec/rails_helper.rb:364:in `block (2 levels) in <top (required)>'
# ./spec/rails_helper.rb:356: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)>'
```
In tests we could attempt to click the button before it was enabled and it could also not be visible on screen. This commits attempts to solve both problems.
It should solve this failure:
```
Failures:
1) Admin Customize Form Templates when visiting the page to create a new form template should allow admin to create a new form template
Failure/Error: find(".form-templates__table tbody tr td", text: name).present?
Capybara::ElementNotFound:
Unable to find css ".form-templates__table tbody tr td"
[Screenshot Image]: /__w/discourse/discourse/tmp/capybara/failures_r_spec_example_groups_admin_customize_form_templates_when_visiting_the_page_to_create_a_new_form_template_should_allow_admin_to_create_a_new_form_template_911.png
~~~~~~~ JS LOGS ~~~~~~~
http://localhost:31339/favicon.ico - Failed to load resource: the server responded with a status of 404 (Not Found)
~~~~~ END JS LOGS ~~~~~
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/node/finders.rb:312:in `block in synced_resolve'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/node/base.rb:84:in `synchronize'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/node/finders.rb:301:in `synced_resolve'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/node/finders.rb:60:in `find'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/session.rb:773:in `find'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/dsl.rb:52:in `call'
# ./vendor/bundle/ruby/3.2.0/gems/capybara-3.39.1/lib/capybara/dsl.rb:52:in `find'
# ./spec/system/page_objects/pages/form_template.rb:21:in `has_form_template?'
# ./spec/system/admin_customize_form_templates_spec.rb:71:in `block (3 levels) in <main>'
# ./spec/rails_helper.rb:381:in `block (3 levels) in <top (required)>'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:189:in `block in timeout'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:36:in `block in catch'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:36:in `catch'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:36:in `catch'
# ./vendor/bundle/ruby/3.2.0/gems/timeout-0.3.2/lib/timeout.rb:198:in `timeout'
# ./spec/rails_helper.rb:377:in `block (2 levels) in <top (required)>'
# ./spec/rails_helper.rb:369: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)>'
```
In some cases activeChannel can be null so this will error,
also it is limiting to have this code in chatApi. Instead
move to the threads manager, and also lean on channelsManager.find
to get the channel from the cache instead, which will not error.
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.
Followup to eae47d82e2,
we removed some specificity from the hashtag color
CSS classes, but now the color is being overridden
by the base hashtag-cooked.d-icon color. This color
is no longer needed, so we just remove that and
the specificity.
New headless shares the same implementation as the chrome browser
instead of being a separate implementation of its own.
See https://developer.chrome.com/articles/new-headless/ for more
details
Co-authored-by: Rafael dos Santos Silva <xfalcox@gmail.com>
#### 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>
Why are we making this change?
Currently, we are displaying the value of the `short_site_description`
site setting in the sidebar only for anonymous user. However, the
display of the description seems out of place in both the `sidebar` and
`header dropdown` navigation menu and do not think the sidebar is the
right place to display it anymore.
This commit introduces a new `within_user_updater_transaction` event that's triggered inside the transaction that saves user updates in `UserUpdater`. Plugins can hook into the transaction using the event to include custom changes in the transaction. Callbacks for this event receive 2 arguments:
1. the user being saved
2. the changed attributes that are passed to `UserUpdater`.
There's also new modifier in this commit called `users_controller_update_user_params` to allow plugins to allowlist custom params in the `UsersController` which eventually end up getting passed as attributes to the `UserUpdater` and the new `within_user_updater_transaction` event where they can be used to perform additional updates using the custom params.
-----
New API is used in https://github.com/discourse/discourse-mailinglist-integration/pull/1.
When a user chooses to move a topic/message to an existing topic/message, they can now opt to merge the posts chronologically (using a checkbox in the UI).
The field more_topic_url is already included in the response preloaded in categories#index
However this field was missing if a request was subsequently made to update the page using
the end-points /categories_and_latest or /categories_and_top. This could lead the client
app to display incorrect information if it relied on this information to update the UI.
This brings the behaviour in line with our other widget-related APIs like `decorateWidget` and `reopenWidget`. This commit also adds a theme/plugin prefix to the console messages.
For now, state is still stored in the modal controller. Eventually the controller will be replaced with a component, and the state will be stored in the service.
(extracted from https://github.com/discourse/discourse/pull/21304)
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 commit regroups 3 changes
- serializes channel ID and json draft when calling the debouncer instead of inside the debounced function, it seems very unlikely to happen, but in a case where the debouncing wouldn't be canceled and the new message not set yet, we could save the draft on an invalid channel
- cancel persist draft handler when changing channel
- ensures we exit early when channel is not set
Very fast or specific mouse moves could allow to leave a message actions menu without reseting the active message. This commit should ensure we correctly catch this event.
No test as it's hard and not reliable to reproduce these in a test.
This commit deprecates the `modify_user_params` method in `UsersController` in favor of a new modifier that replaces that method whose entire purpose is to allow plugins to monkey-patch it to permit custom params in the controller. We now have the "modifier" system which can achieve the same results but in a safer and easier way. The modifier that replaces the deprecated method is included in PR https://github.com/discourse/discourse/pull/21737.
We're running on pretty crappy hardware on Github's CI and this has an
impact on the stability of our system tests on CI. Therefore, we are
bumping `CABPYARA_DEFAULT_MAX_WAIT_TIME` to 10 seconds to account for
the less than ideal hardware we're running the system tests on.
This change trades off speed for stability but speed is already bad on
CI so stability is more important for our case.
What is this change required?
I noticed that actions in `SidebarSectionsController` resulted in
lots of N+1 queries problem and I wanted a solution to
prevent such problems without having to write N+1 queries tests. I have
also used strict loading for `SidebarSection` queries in performance
sensitive spots.
Note that in this commit, I have also set `config.active_record.action_on_strict_loading_violation = :log`
for the production environment so that we have more visibility of
potential N+1 queries problem in the logs. In development and test
environment, we're sticking with the default of raising an error.
What is the problem?
In the test environement, we were calling `SiteSetting.setting` directly
to introduce new site settings. However, this leads to changes in state of the SiteSettings
hash that is stored in memory as test runs. Changing or leaking states
when running tests is one of the major contributors of test flakiness.
An example of how this resulted in test flakiness is our `spec/integrity/i18n_spec.rb` spec file which
had a test case that would fail because a new "plugin_setting" site
setting was registered in another test case but the site setting did not
have translations for the site setting set.
What is the fix?
There are a couple of changes being introduced in this commit:
1. Make `SiteSetting.setting` a private method as it is not safe to be
exposed as a public method of the `SiteSetting` class
2. Change test cases to use existing site settings in Discourse instead
of creating custom site settings. Existing site settings are not
removed often so we don't really need to dynamically add new site
settings in test cases. Even if the site settings being used in test
cases are removed, updating the test cases to rely on other site
settings is a very easy change.
3. Set up a plugin instance in the test environment as a "fixture"
instead of having each test create its own plugin instance.
Joffrey JAFFEUX
Martin Brennan
Jarek Radosz
Alan Guo Xiang Tan
dependabot[bot]
Osama Sayegh
Kris
Renato Atilio
Sérgio Saquetim
David Taylor
Andrei Prigorshnev
Loïc Guitaut