This is to provide diagnostic information for some flaky system tests
which we are currently investigating. The previous attempt in
6f92f42eb83fea61b2712aaaefcc124e4939365d only dumped threads that
contains the `puma` gem in the backtrace but that is not enough. Since I
am now printing all thread backtraces, I added the
`dump_threads_on_failure` metadata
which will be used to determine if all the thread backtraces should be
printed out in the spec's failure output.
This patch adds a new shared example to be used as a smoke test in
plugins and themes.
A `skip_examples` argument is available to easily opt-out from a
category of tests.
Example:
```rb
RSpec.describe "Testing core features", type: :system do
it_behaves_like "having working core features", skip_examples: %i[search login]
end
```
When a Capybara helper times out after hitting
`Capybara.default_max_wait_time`, we will now
dump the puma server threads backtraces. This information will help us
diagnose test failures due to requests not completing within
`Capybara.default_max_wait_time`.
Example test failure output:
```
Failures:
1) User resetting password when desktop when user has multi-factor authentication configured when user has TOTP, security key and backup codes configured should allow a user to toggle from security key to TOTP and between TOTP and backup codes
Failure/Error: expect(page).to have_current_path("/u/#{user.username}/preferences/second-factor")
expected `#<Capybara::Session>.has_current_path?("/u/john/preferences/second-factor")` to be truthy, got false
[Screenshot Image]: /Users/tgxworld/work/discourse/tmp/capybara/failures_r_spec_example_groups_user_resetting_password_when_desktop_when_user_has_multi_factor_authentication_configured_when_user_has_totp_security_key_and_backup_codes_configured_should_allow_a_user_to_toggl_874.png
~~~~~~~ SERVER THREADS BACKTRACES ~~~~~~~
/Users/tgxworld/work/discourse/app/controllers/users_controller.rb:2216:in 'Kernel#sleep'
/Users/tgxworld/work/discourse/app/controllers/users_controller.rb:2216:in 'UsersController#confirm_secure_session'
/Users/tgxworld/work/discourse/app/controllers/users_controller.rb:1543:in 'UsersController#confirm_session'
/Users/tgxworld/work/discourse/app/controllers/application_controller.rb:427:in 'block in ApplicationController#with_resolved_locale'
/Users/tgxworld/work/discourse/app/controllers/application_controller.rb:427:in 'ApplicationController#with_resolved_locale'
/Users/tgxworld/work/discourse/lib/middleware/omniauth_bypass_middleware.rb:35:in 'Middleware::OmniauthBypassMiddleware#call'
/Users/tgxworld/work/discourse/lib/content_security_policy/middleware.rb:12:in 'ContentSecurityPolicy::Middleware#call'
/Users/tgxworld/work/discourse/lib/middleware/anonymous_cache.rb:368:in 'Middleware::AnonymousCache#call'
/Users/tgxworld/work/discourse/lib/middleware/csp_script_nonce_injector.rb:12:in 'Middleware::CspScriptNonceInjector#call'
/Users/tgxworld/work/discourse/spec/rails_helper.rb:48:in 'RspecErrorTracker#call'
/Users/tgxworld/work/discourse/config/initializers/008-rack-cors.rb:14:in 'Discourse::Cors#call'
/Users/tgxworld/work/discourse/lib/middleware/default_headers.rb:13:in 'Middleware::DefaultHeaders#call'
/Users/tgxworld/work/discourse/config/initializers/100-quiet_logger.rb:20:in 'DiscourseRackQuietAssetsLogger#call'
/Users/tgxworld/work/discourse/config/initializers/100-silence_logger.rb:29:in 'SilenceLogger#call'
/Users/tgxworld/work/discourse/lib/middleware/enforce_hostname.rb:24:in 'Middleware::EnforceHostname#call'
/Users/tgxworld/work/discourse/lib/middleware/processing_request.rb:12:in 'Middleware::ProcessingRequest#call'
/Users/tgxworld/work/discourse/lib/middleware/request_tracker.rb:385:in 'Middleware::RequestTracker#call'
/Users/tgxworld/work/discourse/config/initializers/200-first_middlewares.rb:30:in 'TestMultisiteMiddleware#call'
/Users/tgxworld/work/discourse/config/initializers/200-first_middlewares.rb:77:in 'BlockRequestsMiddleware#call'
~~~~~~~ END SERVER THREADS BACKTRACES ~~~~~~~
~~~~~~~ JS LOGS ~~~~~~~
(no logs)
~~~~~ END JS LOGS ~~~~~
```
### Reviewer notes
In this [flaky test
example](https://github.com/discourse/discourse/actions/runs/14087382392/job/39454911943),
the test failed because a capybara assertion did not pass within
`Capybara.default_max_wait_time`. From the screenshot below, we see that
the submit button is still loading which indicates that the request is
still being processed by the server. However, we need to know why the
server is taking more than 10 seconds to generate a response.
```
Failure/Error: expect(page).to have_current_path("/u/#{user.username}/preferences/second-factor")
expected `#<Capybara::Session>.has_current_path?("/u/john/preferences/second-factor")` to be truthy, got false
[Screenshot Image]: /__w/discourse/discourse/tmp/capybara/failures_r_spec_example_groups_user_resetting_password_when_desktop_when_user_has_multi_factor_authentication_configured_when_user_only_has_security_key_configured_should_allow_a_user_to_reset_password_with_a__75.png
~~~~~~~ JS LOGS ~~~~~~~
(no logs)
~~~~~ END JS LOGS ~~~~~
Shared Example Group: "forgot password scenarios" called from ./spec/system/forgot_password_spec.rb:213
./spec/system/page_objects/pages/user_preferences_security.rb:15:in `visit_second_factor'
./spec/system/forgot_password_spec.rb:30:in `create_user_security_key'
./spec/system/forgot_password_spec.rb:79:in `block (4 levels) in <main>'
./spec/rails_helper.rb:619:in `block (3 levels) in <top (required)>'
/var/www/discourse/vendor/bundle/ruby/3.3.0/gems/benchmark-0.4.0/lib/benchmark.rb:304:in `measure'
./spec/rails_helper.rb:619:in `block (2 levels) in <top (required)>'
./spec/rails_helper.rb:580:in `block (3 levels) in <top (required)>'
/var/www/discourse/vendor/bundle/ruby/3.3.0/gems/timeout-0.4.3/lib/timeout.rb:185:in `block in timeout'
/var/www/discourse/vendor/bundle/ruby/3.3.0/gems/timeout-0.4.3/lib/timeout.rb:192:in `timeout'
./spec/rails_helper.rb:570:in `block (2 levels) in <top (required)>'
./spec/rails_helper.rb:527:in `block (2 levels) in <top (required)>'
/var/www/discourse/vendor/bundle/ruby/3.3.0/gems/webmock-3.25.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
```
This commit improves said method to ensure that user is redirected to
the right page before returning.
### Reviewer notes
Example of test flakiness:
https://github.com/discourse/discourse/actions/runs/14081653020/job/39435797236
```
Failure/Error: raise capybara_timeout_error
CapybaraTimeoutExtension::CapybaraTimedOut:
This spec passed, but capybara waited for the full wait duration (10s) at least once. This will slow down the test suite. Beware of negating the result of selenium's RSpec matchers.
[Screenshot Image]: /__w/discourse/discourse/tmp/capybara/failures_r_spec_example_groups_user_resetting_password_when_desktop_when_user_has_multi_factor_authentication_configured_when_user_has_security_key_and_backup_codes_configured_should_allow_a_user_to_reset_pass_261.png
~~~~~~~ JS LOGS ~~~~~~~
~~~~~ END JS LOGS ~~~~~
Shared Example Group: "forgot password scenarios" called from ./spec/system/forgot_password_spec.rb:213
./spec/rails_helper.rb:426:in `block (3 levels) in <top (required)>'
./spec/rails_helper.rb:619:in `block (3 levels) in <top (required)>'
/var/www/discourse/vendor/bundle/ruby/3.3.0/gems/benchmark-0.4.0/lib/benchmark.rb:304:in `measure'
./spec/rails_helper.rb:619:in `block (2 levels) in <top (required)>'
./spec/rails_helper.rb:580:in `block (3 levels) in <top (required)>'
/var/www/discourse/vendor/bundle/ruby/3.3.0/gems/timeout-0.4.3/lib/timeout.rb:185:in `block in timeout'
/var/www/discourse/vendor/bundle/ruby/3.3.0/gems/timeout-0.4.3/lib/timeout.rb:192:in `timeout'
./spec/rails_helper.rb:570:in `block (2 levels) in <top (required)>'
./spec/rails_helper.rb:527:in `block (2 levels) in <top (required)>'
/var/www/discourse/vendor/bundle/ruby/3.3.0/gems/webmock-3.25.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
```
b1924c35 switched our compiler to use `@import` internally for scss
entrypoints. This logic also applied to `.css` files, but unfortunately
sass doesn't do anything with `@import` of CSS files, so they'll be left
intact all the way to the browser. Continue using the old concatenation
approach for them in the compiler.
Followup to b1924c352487ab2c85ae50af45c5b3e098589014
Previously we would prepend extra content to developer-authored files,
which means adding `@use` in some files would throw an error because
`@use` must be at the top of any compiled file.
Instead, we can ensure any developer-authored files are on the load
path, and then `@import` them into the synthetic entrypoint.
Plugin color_definitions stylesheets are an edge case here, and will
need to be handled separately (or... wait until we move to native css
relative-color syntax, then we can drop color-definition stylesheets
altogether)
This PR moves the logic that checks if a site setting we're trying to update has been deprecated from one of the controllers into a policy of the SiteSetting::Update service.
It also gives us the opportunity to shift the error message into a locale file.
This commit adds a loading state to the confirm button in
`confirm-session` dialog.
This also unskips the flaky system tests in
`spec/system/forgot_password_spec.rb` as this change will allow us to
gather more information about why the test is flaky. The screenshots
which we have gathered when the test flakes does not allow us to know if
the button has been clicked or not before the test times out.
This switches the signup/login UI to the full page experience by
default. This has been in use by many sites for multiple months and we
have ironed out many fixes in the meantime.
The `full_page_login` setting is also marked for removal in about
1.5mths, by the end of April 2025.
This commit unskips 3 flaky system tests and gives up on asserting that
redirecting is done correctly. This is because we have invested
considerable effort into this and cannot figure it out. The redirect is
tested by the client side anyway so there is still some test coverage.
We currently have `user_color_schemes` serialized on the preloaded
`Site` object, but it's very lightweight, only having an ID and name.
It would be useful for some themes and also the user Interface
preferences tab to be able to access the colors for the color scheme,
as well as the theme it belongs to.
This commit expands the serializer to include these extra attributes.
This `user_color_schemes` key is already cached as a fragment, so this
shouldn't be too much extra burden to send to the client.
Tight lists are lists that look like this:
* Item 1
* Item 2
* Item 3
Loose lists look like this:
* Item 1
* Item 2
* Item 3
There is a place for the latter, but the former is more
common default behaviour for writing apps and widgets, so
we are overriding the prosemirror default to use tight lists.
Eventually we will have a shortcut or other special behaviour
to switch between the list styles.
When editing a color palette via the new page introduced in
https://github.com/discourse/discourse/pull/31742, it should apply the
color changes for the admin making the change automatically upon save.
Internal topic: t/148628/12.
The URLs returned by DiscourseConnect for the user's avatar, profile and
card backgrounds were not always correctly handling CDN.
This make use of the `GlobalPath.full_cdn_url` helper method which has
been battle-tested.
Ref - https://meta.discourse.org/t/-/356599
`/categories` sometimes returns accompanying topics under certain site
settings. The `CategoryList` currently allows preloading for topic
custom fields via `preloaded_topic_custom_fields`, but not for topics
themselves.
This addition is required for
https://github.com/discourse/discourse-solved/pull/342.
When multiple admins are working in the review queue, it's quite easy for two people to try and handle the same reviewable at the same time. This change addresses the two major situations where this can occur.
The `ReviewableClaimedTopic` model has been extended to allow the system to mark a reviewable as claimed as soon as the first moderator starts handling the reviewable, even when the `reviewable_claiming` setting is disabled. This ensures that reviewable actions with client-site activity (for example, `agree_and_suspend`) will lock the reviewable before another moderator starts working on it.
When someone handles handles a reviewable, we now use `MessageBus` to inform other moderators that it's changed. If any of the other moderator have that reviewable open (either individually, or on the list screen), it will automatically refresh that data.
The tests being fixed in this commit are not waiting for the
asynchronous behaviour of bulk closing topic to complete before running
some of the assertions. This commit updates the tests to check for the
asynchronous behaviour to complete so that the following assertions will
pass with more reliability.
We didn't have support in the #problem constructor for multiple targets, forcing developers to manually construct a Problem instance. This involves a lot of details and is error prone.
This PR supports passing an optional target argument to the constructor. This will be passed on to the translation_data method to generate the correct message as well.
Currently, the light version of mobile logo falls back to the desktop
version if the mobile version isn't set. It makes sense to have the same
fallback rule for the dark version as well, i.e. if there's no dark
mobile logo, use the dark desktop logo.
Internal topic: t/150316.
This commit removes an assertion for the redirect after 2FA
authentication is success message because the message is flashed briefly
before a route transition happens. A proper fix would require us to
redesign when/how the flash message which we can address in the future.
This PR renames a couple of settings related to anonymous mode:
1. `allow_anonymous_posting` → `allow_anonymous_mode`. This setting is
used as a switch for the entire anonymous mode feature, so it makes
sense to give it a generic name that better reflects what the setting
does.
2. `allow_anonymous_likes` → `allow_likes_in_anonymous_mode`. The new
name is clearer and will match a new setting that we'll add to allow
anonymous users to post in chat.
Internal topic: t/148088.
Meta:
https://meta.discourse.org/t/cant-erase-the-bookmark-search-input/357861/4
> When we fill in the bookmark search input and send the request, then
we can’t delete the input’s content.
After removing the last character, the `searchTerm` getter is called. At
this point, `_searchTerm` is empty.
However, `this._searchTerm || this.q` will treat the empty string as
_falsy_, and the `q` value is displayed instead.
It makes it impossible to clear the input.
To fix this, we check specifically on the initial state of
`_searchTerm,` which is _undefined_, to include an empty string as a
valid value.
Note: because of `@computed`, the issue is not triggered when the
content is selected and deleted.
This commit updates the 2FA token submission page to disable the submit
button when the 2FA token is not valid and to also set the submit button
to be in the loading state after the submit button has been clicked.
The UX issues were discovered while I was investigating a flaky test
which has been unskipped in this commit as well. I am not sure if this
will completely resolve the flakiness but we have to unskip it to know
if it continues to be flaky.
Setting Rails.logger after the application has been initialized does not
seem to be safe anymore and can lead to flaky tests. This commit
disallows the reassignment of `Rails.logger` going forward and updates
the affected test.
### Reviewer notes
Reassigning `Rails.logger` from within RSpec tests is causing tests
which uses `Rails.logger.broadcast_to(FakeLogger.new)` to flake.
Example:
https://github.com/discourse/discourse/actions/runs/13951116847/job/39050616967
```
1) invalid requests handles NotFound with invalid json body
Failure/Error: expect(fake_logger.errors).to have_attributes(size: 1)
expected [] to have attributes {:size => 1} but had attributes {:size => 0}
Diff:
@@ -1 +1 @@
-:size => 1,
+:size => 0,
# ./spec/integration/invalid_request_spec.rb:18:in `block (2 levels) in <main>'
# ./spec/rails_helper.rb:619:in `block (3 levels) in <top (required)>'
# /var/www/discourse/vendor/bundle/ruby/3.3.0/gems/benchmark-0.4.0/lib/benchmark.rb:304:in `measure'
# ./spec/rails_helper.rb:619:in `block (2 levels) in <top (required)>'
# ./spec/rails_helper.rb:580:in `block (3 levels) in <top (required)>'
# /var/www/discourse/vendor/bundle/ruby/3.3.0/gems/timeout-0.4.3/lib/timeout.rb:185:in `block in timeout'
# /var/www/discourse/vendor/bundle/ruby/3.3.0/gems/timeout-0.4.3/lib/timeout.rb:192:in `timeout'
# ./spec/rails_helper.rb:570:in `block (2 levels) in <top (required)>'
# ./spec/rails_helper.rb:527:in `block (2 levels) in <top (required)>'
# /var/www/discourse/vendor/bundle/ruby/3.3.0/gems/webmock-3.25.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
```
Now we have the search input showing in a few
different configurations:
* Welcome banner
* Header field
* Header icon
And we can get to the search with both `/` and
`Ctrl+F` shortcuts. These configurations can
be used together, and we need to focus on the right
search input at the right time.
This commit fixes the shortcuts not working
or showing the wrong thing in some cases,
and adds a comprehensive system spec for all
the variants.
Commit f1700ca58929bcbfad23565861d1d3084ae1b3f8 ensures that categories
are loaded lazily, in pages, if the number of visible categories is over
1000. This affected the list of subcategories on the category page too.
The logic has been changed to only paginate if the number of categories
that would have been returned is grater than 1000. For example, if there
is a parent category filter, pagination will only be enforced if the
number of subcategories is over 1000.
Fixes an incorrect logic on `transformPasted` that was causing a
`TypeError: Cannot read properties of null (reading 'marks')` when only
part of a paragraph was selected.
Adds tests
Follow-up to https://github.com/discourse/discourse/pull/30953
This PR is a partial revert of the linked PR — it changes the "Themes
and components" link in the admin sidebar back to the legacy
`/admin/customize/themes` page and adds the themes list/sidebar back to
the left hand side of the page. The new `/admin/config/customize/` route
is still available, but it's not linked from anywhere. When accessing
the new page and then navigating to a theme, the old components (e.g.
the themes list) of the page are hidden. This allows us to iterate on
the new page and improve until we're more ready to make it available
more widely.
Both tests being unskipped here failed previosly with the following
error:
```
Failure/Error: expect(page).to have_current_path("/u/#{user.username}/preferences/account")
expected "/u/confirm-new-email/f42a416fcbca40d66788d65a8837ad49" to equal "/u/bruce306/preferences/account"
./spec/system/email_change_spec.rb:49:in `block (2 levels) in <main>'
```
The error indicates that the transition was not successful and I
suspect that it may be due to the use of the `/my` route prefix which
is just a nice to have and not necessary.
When hard deleting a first post by passing force_destroy: true as an option to PostDestroyer, the post/topic is correctly deleted, and the staff record is created, but the app then errors out.
This only happens on sites with a topic_destroyed webhook setup.
After deleting the record, we pass the topic's ID to TopicView, which then raises an error because it can not load it from the DB.
TopicView supports being initialized with either an ID or an already instantiated record. Since we still have the record in memory after deleting, we can pass that to TopicView.
We currently limit the number of characters in the bar-separated list of auto-membership e-mail domains. We want to make this configurable through site settings.
After this change, we limit the length of each individual domain, and enable the number of domains to be configured through a hidden site setting.
The original limit is there to prevent DoS, since a TEXT column can take up to 1Gb. With this new limit we're still at a maximum of around 10kb.
The change made in #31854 introduced a regression when editing groups, preventing saving when no auto membership e-mail domains are entered.
This change fixes that and adds a system test.
Before this commit,
`PageObjects::Components::SelectKit#expanded_component` might not expand
the component if `is_collapsed?` returns true which can return true if
the method was called before the select-kit component appears on the
screen. When that happens, we end up not expanding the `select-kit`
because we think it is collapsed when in fact the select-kit component
is not rendered yet. This lead to system test failures with errors like:
```
Capybara::ElementNotFound:
Unable to find css "#add-synonyms.is-expanded"
```
This commit updates `PageObjects::Components::SelectKit#is_collapsed?`
to check that the select-kit component has rendered first before
checking if the component is not expanded.
### Reviewer notes
Instances of test flakiness due to this bug:
1.
https://github.com/discourse/discourse/actions/runs/13905226478/job/38906569777
2.
https://github.com/discourse/discourse/actions/runs/13848357836/job/38751122333
Follow up to e4401e587e98ac4020c2c4fd965e227146cf33d4.
In the previous change, when serialising a Reviewable post, we were appending the title to the post content whilst checking for watched word matches. This append action was acting upon the object itself, rather than just a copy of the string, causing the UI to display the title appended twice to the content.
Fortunately, since it was only happening in the serialiser, the incorrect data was never stored in the database, it was only happening when viewing the review queue.
When performing an action in the review queue, this change makes two improvements:
- The buttons on the reviewable item are disabled, so you can't accidentally multi-click.
- A toast is displayed when the action is complete, as a success indication.
Fixes the custom homepage crawler output to include links to the site's
top menu.
This also provides a way to override that content via a plugin. Example
usage in a `plugin.rb` file:
```
register_html_builder("server:custom-homepage-crawler-view") do |c|
"<div>override</div>"
end
```
