Why this change?
Some of the tests in `spec/system/table_builder_spec.rb` are flaky when
we are asserting that clicking the cancel button will close the modal.
This change attempts to fix it by using the `click_button` method
instead of `find` then `click` which is more reliable.
Why this change?
The two tests being updated in question has been flaky on CI. However,
when using `be_forbidden`, the error message does not indicate what the
actual response code was making it hard for us to debug.
What does this change do?
Assert for the exact response status code we are expecting.
`window.deprecationWorkflow` does not exist in the server-side pretty-text environment. This commit fixes the check and adds a general spec for deprecations triggered inside pretty-text
Categories will no longer be preloaded when `lazy_load_categories` is
enabled through PreloadStore.
Instead, the list of site categories will continue to be populated
by `Site.updateCategory` as more and more categories are being loaded
from different sources (topic lists, category selectors, etc).
Previously we hand no tests for `include_raw` which some consumers may
depend on.
Specifically, Discourse AI uses it to get raw markdown for a set of posts
on a topic.
Also cleans up tests so they lint with default ruby
Why this change?
We have been running into flaky tests which seems to be related to
AR transaction problems. However, we are not able to reproduce this
locally and do not have sufficient information on our builds now to
debug the problem.
What does this change do?
Noe the following changes only applies when `ENV["GITHUB_ACTIONS"]` is
present.
This change introduces an RSpec around hook when `capture_log: true` has
been set for a test. The responsibility of the hook is to capture the
ActiveRecord debug logs and print them out.
These tests are still flaky (order dependence) just that now it doesn't fail the test, instead it creates an infinite loop. Skipping these for now. We know they work because they pass, but they leak into other tests. I think we can re-enable locally and either fix or remove this once TL migration is done.
We're changing the implementation of trust levels to use groups. Part of this is to have site settings that reference trust levels use groups instead. It converts the min_trust_to_allow_self_wiki site setting to self_wiki_allowed_groups.
Nothing of note here. This is used in exactly one place, and there's no fallout.
There's a leaky test that breaks some controller tests if run first, creating an order-dependent flake.
This change fixes that, but in doing so also skips a low-value test that breaks from the fix. (Verified manually that it's working.)
Why this change?
On CI, we have been seeing the "handles job concurrency" job timing out
on CI after 45 seconds. Upon closer inspection of `Jobs::Base#perform`
when cluster concurrency has been set, we see that a thread is spun up
to extend the expiring of a redis key by 120 seconds every 60 seconds
while the job is still being executed. The thread looks like this before
the fix:
```
keepalive_thread =
Thread.new do
while parent_thread.alive? && !finished
Discourse.redis.without_namespace.expire(cluster_concurrency_redis_key, 120)
sleep 60
end
end
```
In an ensure block of `Jobs::Base#perform`, the thread is stop by doing
something like this:
```
finished = true
keepalive_thread.wakeup
keepalive_thread.join
```
If the thread is sleeping, `keepalive_thread.wakeup` will stop the
`sleep` method and run the next iteration causing the thread to
complete. However, there is a timing issue at play here. If
`keepalive_thread.wakeup` is called at a time when the thread is not
sleeping, it will have no effect and the thread may end up sleeping for
60 seconds which is longer than our timeout on CI of 45 seconds.
What does this change do?
1. Change `sleep 60` to sleep in intervals of 1 second checking if the
job has been finished each time.
2. Add `use_redis_snapshotting` to `Jobs::Base` spec since Redis is
involved in scheduling and we want to ensure we don't leak Redis
keys.
3. Add `ConcurrentJob.stop!` and `thread.join` to `ensure` block in "handles job concurrency"
test since a failing expectation will cause us to not clean up the
thread we created in the test.
When setting an old TL based site setting in the console e.g.:
SiteSetting.min_trust_level_to_allow_ignore = TrustLevel[3]
We will silently convert this to the corresponding Group::AUTO_GROUP. And vice-versa, when we read the value on the old setting, we will automatically get the lowest trust level corresponding to the lowest auto group for the new setting in the database.
Why this change?
When running system tests on our CI, we have been occasionally seeing
server errors like:
```
Error encountered while proccessing /stylesheets/desktop_e58cf7f686aab173f9b778797f241913c2833c39.css
NoMethodError: undefined method `+' for nil:NilClass
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/path/pattern.rb:139:in `[]'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:127:in `block (2 levels) in find_routes'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:126:in `each'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:126:in `each_with_index'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:126:in `block in find_routes'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:123:in `map!'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:123:in `find_routes'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:32:in `serve'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/routing/route_set.rb:852:in `call'
```
While looking through various Rails issues related to the error above, I
came across https://github.com/rails/rails/pull/27647 which is a fix to
fully initialize routes before the first request is handled. However,
the routes are only fully initialize only if `config.eager_load` is set
to `true`. There is no reason why `config.eager_load` shouldn't be `true` in the
CI environment and this is what a new Rails 7.1 app is generated with.
What does this change do?
Enable `config.eager_load` when `env["CI"]` is present
A bug that allowed TL1 to convert other's posts to wiki.
The issue was introduced in this PR: https://github.com/discourse/discourse/pull/24999/files
The wiki can be created if a user is TL3 and it is their own post - default 3 for setting `SiteSetting.min_trust_to_allow_self_wiki`
In addition, a wiki can be created by staff and TL4 users for any post.
Follow-up to f5ca96528d
Why this change?
`RSpec.current_example.metadata[:extra_failure_lines]` can be `nil` and
calling `<<` on `nil` is not a good idea.
What does this change do?
Set `RSpec.current_example.metadata[:extra_failure_lines]` to `""` as
long as there are exceptions.
Why this change?
When running system tests with all official plugins installed, we have
encountered instances where the system tests will hang. When dumping the
backtraces of the threads, we can see that the main thread running the
tests is stuck in a deadlock with the puma thread while serving a
request.
The deadlock happens when the main thread acquires the `ActiveSupport::Concurrency::LoadInterlockAwareMonitor`
lock first in `ActiveRecord::ConnectionAdapters::AbstractAdapter` before acquring another `Monitor` lock in
`ActiveRecord::ModelSchema`. In the Puma thread, it acquires the
`Monitor` lock in `ActiveRecord::ModelSchema` first before acquring the
`ActiveSupport::Concurrency::LoadInterlockAwareMonitor` lock.
What does this change do?
To workaround this problem, we will preload all model schema cache
before running system tests such that the `Monitor` lock in `ActiveRecord::ModelSchema`
will not be acquired.
This changes the Plugins link in the admin sidebar to
be a section instead, which then shows all enabled plugin
admin routes (which are custom routes some plugins e.g.
chat define).
This is done via adding some special preloaded data for
all controllers based on AdminController, and also specifically
on Admin::PluginsController, to have the routes loaded without
additional requests on page load.
We just use a cog for all the route icons for now...we don't
have anything better.
Meta topic: https://meta.discourse.org/t/reseting-robots-txt-override-doesnt-seem-to-work-as-expected/287880?u=osama
Discourse provides a default version for `/robots.txt` which can be customized by admins in `/admin/customize/robots`. In that page, there's a button to reset back to the default version that Discourse provides. However, there's currently a bug with the reset button where the content appears to change to some HTML document instead of the default `robots.txt` version when clicking the button. Refreshing the page shows the true/correct content of `robots.txt` which is the default version, so the reset button actually works but there's a display problem.
What causes this display problem is that we use Rails' `render_to_string` method to generate the default content for `robots.txt` from the template, and what we get from that method is the `robots.txt` content wrapped in the application layout. To fix this issue, we need to pass `layout: false` to the `render_to_string` method so that it renders the template without any layouts.
(extracted from #23678)
* Move Wizard back into main app, remove Wizard addon
* Remove Wizard-related resolver or build hacks
* Install and enable `@embroider/router`
* Add "wizard" to `splitAtRoutes`
In a fully optimized Embroider app, route-based code splitting more
or less Just Work™ – install `@embroider/router`, subclass from it,
configure which routes you want to split and that's about it.
However, our app is not "fully optimized", by which I mean we are
not able to turn on all the `static*` flags.
In Embroider, "static" means "statically analyzable". Specifically
it means that all inter-dependencies between modules (files) are
explicitly expressed as `import`s, as opposed to `{{i18n ...}}`
magically means "look for the default export in app/helpers/i18n.js"
or something even more dynamic with the resolver.
Without turning on those flags, Embroider behaves conservatively,
slurps up all `app` files eagerly into the primary bundle/chunks.
So, while you _could_ turn on route-based code splitting, there
won't be much to split.
The commits leading up to this involves a bunch of refactors and
cleanups that 1) works perfectly fine in the classic build, 2) are
good and useful in their own right, but also 3) re-arranged things
such that most dependencies are now explicit.
With those in place, I was able to move all the wizard code into
the "app/static" folder. Embroider does not eagerly pull things from
this folder into any bundle, unless something explicitly "asks" for
them via `imports`. Conversely, things from this folder are not
registered with the resolver and are not added to the `loader.js`
registry.
In conjunction with route-based code splitting, we now have the
ability to split out islands of on-demand functionalities from the
main app bundle.
When you split a route in Embroider, it automatically creates a
bundle/entrypoint with the relevant routes/templates/controllers
matching that route prefix. Anything they import will be added to
the bundle as well, assuming they are not already in the main app
bundle, which is where the "app/static" folder comes into play.
The "app/static" folder name is not special. It is configured in
ember-cli-build.js. Alternatively, we could have left everything
in their normal locations, and add more fine-grained paths to the
`staticAppPaths` array. I just thought it would be easy to manage
and scale, and less error-prone to do it this way.
Note that putting things in `app/static` does not guarantee that
it would not be part of the main app bundle. For example, if we
were to add an `import ... from "app/static/wizard/...";` in a
main bundle file (say, `app.js`), then that chunk of the module
graph would be pulled in. (Consider using `await import(...)`?)
Overtime, we can build better tooling (e.g. lint rules and babel
macros to make things less repetitive) as we expand the use of
this pattern, but this is a start.
Co-authored-by: Godfrey Chan <godfreykfc@gmail.com>
Why this change?
Previously, we were attaching any server exception to the RSpec
example's `Exception#cause` by doing `example.exception.cause =
RspecErrorTracker.last_exception`. However, this is problematic because
it relies on RSpec internal implementation details where RSpec will
print out the exception's cause. The other problem is that when RSpec
prints out the exception cause, it only includes a single line of
backtrace which isn't very helpful sometimes.
While this change of tracking the last exception works OK for request
specs, it doesn't not work for system specs where multiple requests can
be triggered in an example potentially leading to multiple exceptions.
Knowing all the exceptions which happened in the request is important
for us when it comes to debugging system test failures.
What does this change do?
`RspecErrorTracker` now tracks all exceptions that occurs during an
RSpec example run. All the exceptions including the fullback trace of
each exception is printed out as part of the example's `extra_failure_lines` metadata.
Example:
```
Failures:
1) Shortcuts | mark all read when chat is open when pressing shift+esc marks all channels read
Failure/Error: expect(page).to have_content("all read messagasd")
expected to find text "all read messagasd" in "Topics\nMy Posts\nReview\nAdmin\nMore\nCategories\nAmazing Category 0\nAmazing Category 1\nAmazing Category 2\nUncategorized\nAll categories\nConfigure defaults\nMessages\nInbox\nMy threads\nChannels\nKino Buffs 2\nMusic Lodge 0\nMusic Lodge 1\nPersonal chat\nMusic Lodge 1\nChat settings have been set to retain channel messages for 90 days.\nToday\nbruce6\n2:46 pm\nall read message 0\nbruce7\n2:46 pm\nall read message 1\nbruce8\n2:46 pm\nall read message 2\nbruce9\n2:46 pm\nall read message 3\nbruce10\n2:46 pm\nall read message 4\nbruce11\n2:46 pm\nall read message 5\nbruce12\n2:46 pm\nall read message 6\nbruce13\n2:46 pm\nall read message 7\nbruce14\n2:46 pm\nall read message 8\nbruce15\n2:46 pm\nall read message 9\nShowing all messages"
[Screenshot Image]: /home/tgxworld/work/discourse/tmp/capybara/failures_r_spec_example_groups_shortcuts_mark_all_read_when_chat_is_open_when_pressing_shift_esc_marks_all_channels_read_236.png
~~~~~~~ SERVER EXCEPTIONS ~~~~~~~
Error encountered while proccessing /stylesheets/desktop_theme_1_5dba82f48b7d6e4a9d54ffd915712811591356b7.css
RuntimeError: boom
/home/tgxworld/work/discourse/app/controllers/application_controller.rb:996:in `set_cross_origin_opener_policy_header'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:400:in `block in make_lambda'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:236:in `block in halting_and_conditional'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:599:in `block in invoke_after'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:599:in `each'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:599:in `invoke_after'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:133:in `block in run_callbacks'
/home/tgxworld/work/discourse/app/controllers/application_controller.rb:423:in `block in with_resolved_locale'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/i18n-1.14.1/lib/i18n.rb:322:in `with_locale'
/home/tgxworld/work/discourse/app/controllers/application_controller.rb:423:in `with_resolved_locale'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:127:in `block in run_callbacks'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:138:in `run_callbacks'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/abstract_controller/callbacks.rb:233:in `process_action'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_controller/metal/rescue.rb:23:in `process_action'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_controller/metal/instrumentation.rb:67:in `block in process_action'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/notifications.rb:206:in `block in instrument'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/notifications/instrumenter.rb:24:in `instrument'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/notifications.rb:206:in `instrument'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_controller/metal/instrumentation.rb:66:in `process_action'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_controller/metal/params_wrapper.rb:259:in `process_action'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activerecord-7.0.7/lib/active_record/railties/controller_runtime.rb:27:in `process_action'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/abstract_controller/base.rb:151:in `process'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionview-7.0.7/lib/action_view/rendering.rb:39:in `process'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_controller/metal.rb:188:in `dispatch'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_controller/metal.rb:251:in `dispatch'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/routing/route_set.rb:49:in `dispatch'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/routing/route_set.rb:32:in `serve'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:50:in `block in serve'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:32:in `each'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:32:in `serve'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/routing/route_set.rb:852:in `call'
/home/tgxworld/work/discourse/lib/middleware/omniauth_bypass_middleware.rb:64:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/tempfile_reaper.rb:15:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/conditional_get.rb:27:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/head.rb:12:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/http/permissions_policy.rb:38:in `call'
/home/tgxworld/work/discourse/lib/content_security_policy/middleware.rb:12:in `call'
/home/tgxworld/work/discourse/lib/middleware/anonymous_cache.rb:351:in `call'
/home/tgxworld/work/discourse/lib/middleware/gtm_script_nonce_injector.rb:10:in `call'
/home/tgxworld/work/discourse/spec/rails_helper.rb:47:in `call'
/home/tgxworld/work/discourse/config/initializers/008-rack-cors.rb:14:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/session/abstract/id.rb:266:in `context'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/session/abstract/id.rb:260:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/cookies.rb:704:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/callbacks.rb:27:in `block in call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:99:in `run_callbacks'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/callbacks.rb:26:in `call'
/home/tgxworld/work/discourse/plugins/discourse-geoblocking/lib/geoblocking_middleware.rb:24:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/debug_exceptions.rb:28:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/show_exceptions.rb:29:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/railties-7.0.7/lib/rails/rack/logger.rb:40:in `call_app'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/railties-7.0.7/lib/rails/rack/logger.rb:27:in `call'
/home/tgxworld/work/discourse/config/initializers/100-quiet_logger.rb:20:in `call'
/home/tgxworld/work/discourse/config/initializers/100-silence_logger.rb:29:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/remote_ip.rb:93:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/request_id.rb:26:in `call'
/home/tgxworld/work/discourse/lib/middleware/enforce_hostname.rb:24:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/method_override.rb:24:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/executor.rb:14:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/static.rb:23:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/sendfile.rb:110:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/host_authorization.rb:131:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/message_bus-4.3.8/lib/message_bus/rack/middleware.rb:60:in `call'
/home/tgxworld/work/discourse/lib/middleware/request_tracker.rb:233:in `call'
/home/tgxworld/work/discourse/config/initializers/200-first_middlewares.rb:27:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/railties-7.0.7/lib/rails/engine.rb:530:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/urlmap.rb:74:in `block in call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/urlmap.rb:58:in `each'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/urlmap.rb:58:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/builder.rb:244:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/capybara-3.39.2/lib/capybara/server/animation_disabler.rb:25:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/capybara-3.39.2/lib/capybara/server/middleware.rb:60:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/puma-6.4.0/lib/puma/configuration.rb:272:in `call'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/puma-6.4.0/lib/puma/request.rb💯in `block in handle_request'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/puma-6.4.0/lib/puma/thread_pool.rb:378:in `with_force_shutdown'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/puma-6.4.0/lib/puma/request.rb:99:in `handle_request'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/puma-6.4.0/lib/puma/server.rb:443:in `process_client'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/puma-6.4.0/lib/puma/server.rb:241:in `block in run'
/home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/puma-6.4.0/lib/puma/thread_pool.rb:155:in `block in spawn_thread'
~~~~~~~ END SERVER EXCEPTIONS ~~~~~~~
~~~~~~~ JS LOGS ~~~~~~~
http://localhost:31337/stylesheets/desktop_theme_1_5dba82f48b7d6e4a9d54ffd915712811591356b7.css?__ws=localhost - Failed to load resource: the server responded with a status of 500 (Internal Server Error)
~~~~~ END JS LOGS ~~~~~
```
Why this change?
This is what `Capybara::Session#quit` does:
```
def quit
@driver.quit if @driver.respond_to? :quit
@document = @driver = nil
@touched = false
@server&.reset_error!
end
```
One notable thing is that it resets server errors which means that any
server errors encountered by a session is cleared. That is not what we
want since it hides errors even though `Capybara.raise_server_errors`
has been set to `true`.
Why this change?
This is part of our efforts to harden the security of the Discourse
application. Setting the `CROSS_ORIGIN_OPENER_POLICY` header to `same-origin-allow-popups`
by default makes the application safer. We have opted to make this a
hidden site setting because most admins will never have to care about
this setting so we're are opting not to show it. If they do have to
change it, they can still do so by setting the
`DISCOURSE_CROSS_ORIGIN_OPENER_POLICY` env.
Adds an API scope for accessing Logster's routes. This one is a bit
different than routes from core because it is mounted like
```
mount Logster::Web => "/logs"
```
and doesn't have all the route info a traditional rails app/engine does.
Ability to automatically generate migration when site setting is changed from trust level to groups.
Example usage:
rails generate site_setting_move_to_groups_migration min_trust_to_create_topic create_topic_allowed_groups
Settings that are using the new `file_size_restriction` types like the
`max_image_size_kb` setting need to have their values saved as integers.
This was a recent regression in 00209f03e6
that caused these values to be saved as strings.
This change also removes negatives from the validation regex because
file sizes can't be negative anyways.
Bug report: https://meta.discourse.org/t/289037
This commit refactor CategoryList to remove usage of EmberObject,
hopefully make the code more readable and fixes various edge cases with
lazy loaded categories (third level subcategories not being visible,
subcategories not being visible on category page, requesting for more
pages even if the last one did not return any results, etc).
The problems have always been here, but were not visible because a lot
of the processing was handled by the server and then the result was
serialized. With more of these being moved to the client side for the
lazy category loading, the problems became more obvious.
We're changing the implementation of trust levels to use groups. Part of this is to have site settings that reference trust levels use groups instead. It converts the min_trust_level_to_allow_ignore site setting to ignore_allowed_groups.
This PR maintains backwards compatibility until we can update plugins and themes using this.
We're changing the implementation of trust levels to use groups. Part of this is to have site settings that reference trust levels use groups instead. It converts the min_trust_level_to_allow_invite site setting to invite_allowed_groups.
Nothing much of note. This is used in one place and there's no fallout.
Why this change?
By default, `Capybara.default_max_wait_time` is set to `2`. However,
this is not a high enough default for Discourse as certain requests like
creating a post can take upwards of 2 seconds even on a high end desktop
CPU like the Ryzen 5950x. Therefore, we have decided to double the default max wait time.
Using min_trust_to_create_topic and create_topic_allowed_groups together was part of #24740
Now, when plugins specs are fixed, we can safely remove that part of logic.
This is v0 of admin sidebar navigation, which moves
all of the top-level admin nav from the top of the page
into a sidebar. This is hidden behind a enable_admin_sidebar_navigation
site setting, and is opt-in for now.
This sidebar is dynamically shown whenever the user enters an
admin route in the UI, and is hidden and replaced with either
the:
* Main forum sidebar
* Chat sidebar
Depending on where they navigate to. For now, custom sections
are not supported in the admin sidebar.
This commit removes the experimental admin sidebar generation rake
task but keeps the experimental sidebar UI for now for further
testing; it just uses the real nav as the default now.
Before, when needed to get stats in a plugin, we called Core classes directly.
Introducing plugin API will decouple plugins from Core and give as more freedom
in refactoring stats in Core. Without this API, I wasn't able to do all refactorings
I wanted when working on d91456f.
We're changing the implementation of trust levels to use groups. Part of this is to have site settings that reference trust levels use groups instead. It converts the min_trust_level_to_allow_user_card_background site setting to user_card_background_allowed_groups.
Nothing of note here. This is used in exactly one place, and there's no fallout.
This validator is used for site settings where one or more groups are to be input.
At the moment this validator just checks that the value isn't blank. This PR adds a validation for the existence of the groups passed in.
We're changing the implementation of trust levels to use groups. Part of this is to have site settings that reference trust levels use groups instead. It converts the tl4_delete_posts_and_topics site setting to delete_all_posts_and_topics_allowed_groups.
This one is a bit different from previous ones, as it's a boolean flag, and the default should be no group. Pay special attention to the migration during review.
* FEATURE: core code, tests for feature to allow backups to removed based on a time window
* FEATURE: getting tests working for time-based backup
* FEATURE: getting tests running
* FEATURE: linting
We're changing the implementation of trust levels to use groups. Part of this is to have site settings that reference trust levels use groups instead. It converts the min_trust_to_flag_posts site setting to flag_post_allowed_groups.
Note: In the original setting, "posts" is plural. I have changed this to "post" singular in the new setting to match others.
We're changing the implementation of trust levels to use groups. Part of this is to have site settings that reference trust levels use groups instead. It converts the min_trust_to_edit_post site setting to edit_post_allowed_groups.
The old implementation will co-exist for a short period while I update any references in plugins and themes.
This change converts the min_trust_to_create_topic site setting to
create_topic_allowed_groups.
See: https://meta.discourse.org/t/283408
- Hides the old setting
- Adds the new site setting
- Add a deprecation warning
- Updates to use the new setting
- Adds a migration to fill in the new setting if the old setting was
changed
- Adds an entry to the site_setting.keywords section
- Updates tests to account for the new change
- After a couple of months, we will remove the min_trust_to_create_topicsetting entirely.
Internal ref: /t/117248
This change converts the allow_uploaded_avatars site setting to uploaded_avatars_allowed_groups.
See: https://meta.discourse.org/t/283408
Hides the old setting
Adds the new site setting
Adds a deprecation warning
Updates to use the new setting
Adds a migration to fill in the new setting if the old setting was changed
Adds an entry to the site_setting.keywords section
Updates tests to account for the new change
After a couple of months, we will remove the allow_uploaded_avatars setting entirely.
Internal ref: /t/117248
What motivated this change?
Our builds on Github actions have been extremely flaky mostly due to system tests. This has led to a drop in confidence
in our test suite where our developers tend to assume that a failed job is due to a flaky system test. As a result, we
have had occurrences where changes that resulted in legitimate test failures are merged into the `main` branch because developers
assumed it was a flaky test.
What does this change do?
This change seeks to reduce the flakiness of our builds on Github Actions by automatically re-running RSpec tests once when
they fail. If a failed test passes subsequently in the re-run, we mark the test as flaky by logging it into a file on disk
which is then uploaded as an artifact of the Github workflow run. We understand that automatically re-runs will lead to
lower accuracy of our tests but we accept this as an acceptable trade-off since a fragile build has a much greater impact
on our developers' time. Internally, the Discourse development team will be running a service to fetch the flaky tests
which have been logged for internal monitoring.
How is the change implemented?
1. A `--retry-and-log-flaky-tests` CLI flag is added to the `bin/turbo_rspec` CLI which will then initialize `TurboTests::Runner`
with the `retry_and_log_flaky_tests` kwarg set to `true`.
2. When the `retry_and_log_flaky_tests` kwarg is set to `true` for `TurboTests::Runner`, we will register an additional
formatter `Flaky::FailuresLoggerFormatter` to the `TurboTests::Reporter` in the `TurboTests::Runner#run` method.
The `Flaky::FailuresLoggerFormatter` has a simple job of logging all failed examples to a file on disk when running all the
tests. The details of the failed example which are logged can be found in `TurboTests::Flaky::FailedExample.to_h`.
3. Once all the tests have been run once, we check the result for any failed examples and if there are, we read the file on
disk to fetch the `location_rerun_location` of the failed examples which is then used to run the tests in a new RSpec process.
In the rerun, we configure a `TurboTests::Flaky::FlakyDetectorFormatter` with RSpec which removes all failed examples from the log file on disk since those examples are not flaky tests. Note that if there are too many failed examples on the first run, we will deem the failures to likely not be due to flaky tests and not re-run the test failures. As of writing, the threshold of failed examples is set to 10. If there are more than 10 failed examples, we will not re-run the failures.
Ability to automatically generate migration when site setting name is changed.
Example usage: `rails generate site_setting_rename_migration site_description contact_email`
Applies the embed_unlisted site setting consistently across topic embeds, including those created via the WP Discourse plugin. Relatedly, adds a embed exception to can_create_unlisted_topic? check. Users creating embedded topics are not always staff.
Why this change?
The code changes introduced in 5b91dc1844
resulted in errors being raised when `session.quit` is called when using
multiple sessions. From my debugging, this seems to be attributed to the
fact that the change introduced resulted in multiple sessions sharing
the same instance of `Selenium::WebDriver::Remote::Http::Default`. While
sharing the same instance in theory should be fine, but the problem is
that `Selenium::WebDriver::Driver` will mutate the `server_url` of the
client in `Selenium::WebDriver::Remote::Bridge`. This is problematic
because each session created by capbyara relies on a different server
URL and this mutation causes all sorts of weird errors to occur.
To reproduce the problem, run `LOAD_PLUGINS=1 rspec plugins/chat/spec/system/send_message_spec.rb:76`
locally while excluding the changes in this commit.
This change converts the min_trust_to_edit_wiki_post site setting to edit_wiki_post_allowed_groups.
See: https://meta.discourse.org/t/283408
Hides the old setting
Adds the new site setting
Add a deprecation warning
Updates to use the new setting
Adds a migration to fill in the new setting if the old setting was changed
Adds an entry to the site_setting.keywords section
Updates tests to account for the new change
After a couple of months, we will remove the email_in_min_trust setting entirely.
Internal ref: /t/117248
A lot of work has been put in the select kits used for selecting
categories: CategorySelector, CategoryChooser, CategoryDrop, however
they still do not work as expected when these selectors already have
values set, because the category were still looked up in the list of
categories stored on the client-side Categrories.list().
This PR fixes that by looking up the categories when the selector is
initialized. This required altering the /categories/find.json endpoint
to accept a list of IDs that need to be looked up. The API is called
using Category.asyncFindByIds on the client-side.
CategorySelector was also updated to receive a list of category IDs as
attribute, instead of the list of categories, because the list of
categories may have not been loaded.
During this development, I noticed that SiteCategorySerializer did not
serializer all fields (such as permission and notification_level)
which are not a property of category, but a property of the relationship
between users and categories. To make this more efficient, the
preload_user_fields! method was implemented that can be used to
preload these attributes for a user and a list of categories.
When rebaking and in various other places for posts, we
run through the uploads and call `update_secure_status` on
each of them.
However, if the secure status didn't change, we were still
calling S3 to change the ACL, which would have been a noop
in many cases and takes ~1 second per call, slowing things
down a lot.
Also, we didn't account for the s3_acls_enabled site setting
being false here, and in the specs doing an assertion
that `Discourse.store.update_ACL` is not called doesn't
work; `Discourse.store` isn't a singleton, it re-initializes
`FileStore::S3Store.new` every single time.
Why this change?
The tags modal loads more tags via infinite loading based on when the last tag in the
given page appears in the viewport for the user. When it comes in to
view, a request is then triggered to fetch additional tags. To ensure
that we are only loading a single page of tags each time the modal is
opened, we previously set a max height on the modal's body to ensure
that the last tag which appears in the modal will be outside of the view
port in the initial load. However, this has regressed recently due to
unknown reasons and resulted in multiple pages of tags being loaded
immediately from the server as the modal's height was not restricted.
This regression was caught by an existing test but was unfortunately
determined as flaky.
What does this change do?
This change restores the max height on the edit navigation menu tags
modal on dekstop.
I took the wrong approach here, need to rethink.
* Revert "FIX: Use Guardian.basic_user instead of new (anon) (#24705)"
This reverts commit 9057272ee2.
* Revert "DEV: Remove unnecessary method_missing from GuardianUser (#24735)"
This reverts commit a5d4bf6dd2.
* Revert "DEV: Improve Guardian devex (#24706)"
This reverts commit 77b6a038ba.
* Revert "FIX: Introduce Guardian::BasicUser for oneboxing checks (#24681)"
This reverts commit de983796e1.
c.f. de983796e1
There will soon be additional login_required checks
for Guardian, and the intent of many checks by automated
systems is better fulfilled by using BasicUser, which
simulates a logged in TL0 forum user, rather than an
anon user.
In some cases the use of anon still makes sense (e.g.
anonymous_cache), and in that case the more explicit
`Guardian.anon_user` is used
The modified test used to be the same as the test above. The bad test
was introduced in commit 77d4c4d8dc,
during a refactoring.
This was not a serious problem because the same behavior was still
tested partially by the other tests below.
Through internal discussion, it has become clear that
we need a conceptual Guardian user that bridges the
gap between anon users and a logged in forum user with
an absolute baseline level of access to public topics,
which can be used in cases where:
1. Automated systems are running which shouldn't see any
private data
1. A baseline level of user access is needed
In this case we are fixing the latter; when oneboxing a local
topic, and we are linking to a topic in another category from
the current one, we need to operate off a baseline level of
access, since not all users have access to the same categories,
and we don't want e.g. editing a post with an internal link to
expose sensitive internal information.
We add `Access-Control-Allow-Origin: *` to all asset requests which are requested via a configured CDN. This is particularly important now that we're using browser-native `import()` to load the highlightjs bundle. Unfortunately, user-configurable 'cors_origins' site setting was overriding the wldcard value on CDN assets and causing CORS errors.
This commit updates the logic to give the `*` value precedence, and adds a spec for the situation. It also invalidates the cache of hljs assets (because CDNs will have cached the bad Access-Control-Allow-Origin header).
The rack-cors middleware is also slightly tweaked so that it is always inserted. This makes things easier to test and more consistent.
Why was the problem?
ActiveRecord's query cache for the connection pool wasn't disabled after the
`with a fake provider runs 'other_phase' for enabled auth methods` test
in `omniauth_callbacks_controller_spec.rb` was run. This was because the
Rack response body in `FakeAuthenticator::Strategy::other_phase` did not
adhere to the expected Rack body format which is "typically an Array of
String instances". Because this expectation was broken, it cascaded the
problem down where it resulted in the ActiveRecord's query cache for the
connection pool not being disabled as it normally should when the
response body is closed.
When the query cache is left enabled, common assertions pattern in RSpec
like `expect { something }.to change { Group.count }` will fail since
the query cache is enabled and the call first call to `Group.count` will
cache the result to be reused later on.
To see the bug in action, one can run the following command:
`bundle exec rspec --seed 44747
spec/requests/omniauth_callbacks_controller_spec.rb:1150
spec/models/group_spec.rb:283`
Followup e37fb3042d
* Automatically remove the prefix `Discourse ` from all the plugin titles to avoid repetition
* Remove the :discourse_dev: icon from the author. Consider a "By Discourse" with no labels as official
* We add a `label` metadata to plugin.rb
* Only plugins made by us in `discourse` and `discourse-org` GitHub organizations will show these in the list
* Make the plugin author font size a little smaller
* Make the commit sha look like a link so it's more obvious it goes to the code
Also I added some validation and truncation for plugin metadata
parsing since currently you can put absolutely anything in there
and it will show on the plugin list.
The category drop was rerendered after every category async change
because it updated the categories list. This is not necessary and
categories can be referenced indirectly by ID instead.
This change refactors the check `user.groups.any?` and instead uses
`user.staged?` to check if the user is staged or not.
Also fixes several tests to ensure the users have their auto trust level
groups created.
Follow up to:
- 8a45f84277
- 447d9b2105
- c89edd9e86
Why this change?
Asserting against records of the database in system tests can be flaky
because those assertions can run against the database before the server
has actually saved the necessary changes to the database.
What does this change do?
While the assertion is not ideal, we are working around this as a
temporary fix by using `try_until_success` which will retry the
assertion up till the default capybara timeout.
This value is included when generating static asset URLs. Updating the value will allow site operators to invalidate all asset urls to recover from configuration issues which may have been cached by CDNs/browsers.
When sending SMTP for group SMTP functionality, we
are running into timeouts for both read and open
when sending mail occassionally, which can cause issues
like the email only being sent to _some_ of the recipients
or to fail altogether.
The defaults of 5s are too low, so bumping them up to
the defaults of the `net-smtp` gem.
When we check upload security, one of the checks is to
run `access_control_post.with_secure_uploads?`. The problem
here is that the `topic` for the post could be deleted,
which would make the check return `nil` sometimes instead
of false because of safe navigation. We just need to be
more explicit.
Admin can add tag description up to 1000 characters.
Full description is displayed on tag page, however on topic list it is truncated to 80 characters.
Why this change?
In the `invites_controller_spec.rb` file, we had several tests that were
checking for assets path in the response's body to determine which
layout has been rendered. However, those test fails if `bin/ember-cli
--build` has been run locally.
What does this change do?
Instead of checking for asset paths to determine the layout that has
been rendered, this change relies on the fact that the `no_ember` layout
has a `no-ember` class on the `body` element. This is more deterministic
as compared to relying on the different asset paths that are rendered in
the response.
Followup to 2443446e62
We introduced video placeholders which prevent preloading
metadata for videos in posts. The structure looks like this
in HTML when the post is cooked:
```
<div class="video-placeholder-container" data-video-src="http://some-url.com/video.mp4" dir="ltr" style="cursor: pointer;">
<div class="video-placeholder-wrapper">
<div class="video-placeholder-overlay">
<svg class="fa d-icon d-icon-play svg-icon svg-string" xmlns="http://www.w3.org/2000/svg">
<use href="#play"></use>
</svg>
</div>
</div>
</div>
```
However, we did not update the code that links post uploads
to the post via UploadReference, so any videos uploaded since
this change are essentially dangling and liable to be deleted.
This also causes some uploads to be marked secure when they
shouldn't be, because they are not picked up and analysed in the
CookedPostProcessor flow.
Followup to e37fb3042d,
in some cases we cannot get git information for the
plugin folder (e.g. permission issues), so we need
to only try and get information about it if
commit_hash is present.
Reverts
- DEV: maxmind license checking failing tests #24534
- UX: Show if MaxMind key is missing on IP lookup #18993
These changes are leading to surprising results, our logs are now filling up with warnings on dev environments
We need the change to be redone
This improves the implementation of #18993
1. Error message displayed to user is clearer
2. open_db will also be called, even if license key is blank, as it was previously
3. This in turn means no need to keep stubbing 'maxmind_license_key'
The parent category needs to be serialized before the child category
because they are parsed in order. Otherwise the client will not build
the parent-child relationship correctly.
This change converts the `email_in_min_trust` site setting to
`email_in_allowed_groups`.
See: https://meta.discourse.org/t/283408
- Hides the old setting
- Adds the new site setting
- Add a deprecation warning
- Updates to use the new setting
- Adds a migration to fill in the new setting if the old setting was
changed
- Adds an entry to the site_setting.keywords section
- Updates tests to account for the new change
After a couple of months we will remove the
`email_in_min_trust` setting entirely.
Internal ref: /t/115696
Followup to e37fb3042d
Some plugins like discourse-ai and discourse-saml do not
nicely change from kebab-case to Title Case (e.g. Ai, Saml),
and anyway this method of getting the plugin name is not
translated either.
Better to use the plugin setting category if it exists,
since that is written by a human and is translated.
* DEV: Convert approve_new_topics_unless_trust_level to groups
This change converts the `approve_new_topics_unless_trust_level` site
setting to `approve_new_topics_unless_allowed_groups`.
See: https://meta.discourse.org/t/283408
- Hides the old setting
- Adds the new site setting
- Add a deprecation warning
- Updates to use the new setting
- Adds a migration to fill in the new setting if the old setting was
changed
- Adds an entry to the site_setting.keywords section
- Updates tests to account for the new change
After a couple of months we will remove the
`approve_new_topics_unless_trust_level` setting entirely.
Internal ref: /t/115696
* add missing translation
* Add keyword entry
* Add migration
This commit extracts the storage part of the route-scroll-manager into a dedicated service. This provides a key/value store which will reset for each navigation, and restore previous values when the user uses the back/forward buttons in their browser.
This gives us a reliable replacement for the old `DiscourseRoute.isPoppedState` function, which would not work under all situations.
Previously reverted in e6370decfd. This version has been significantly refactored, and includes an additional system spec for the issue we identified.
We were throwing ArgumentError in UrlHelper.normalised_encode,
but it was incorrect -- we were passing ArgumentError.new
2 arguments which is not supported. Fix this and have a hint
of which URL is causing the issue for debugging.
This change converts the `approve_unless_trust_level` site setting to
`approve_unless_allowed_groups`.
See: https://meta.discourse.org/t/283408
- Adds the new site setting
- Adds a deprecation warning
- Updates core to use the new settings.
- Adds a migration to fill in the new setting of the old setting was
changed
- Adds an entry to the site_setting.keywords section
- Updates many tests to account for the new change
After a couple of months we will remove the `approve_unless_trust_level`
setting entirely.
Internal ref: /t/115696
Why this change?
The test was randomly failing in
https://github.com/discourse/discourse/actions/runs/6936264158/job/18868087113
with the following failure:
```
expect do user.update_ip_address!("127.0.0.1") end.to change {
UserIpAddressHistory.where(user_id: user.id).count
}.by(1)
expected `UserIpAddressHistory.where(user_id: user.id).count` to have changed by 1, but was changed by 0
```
This is due to the fact that ActiveRecord will actually cache the result
of `UserIpAddressHistory.where(user_id: user.id).count`. However,
`User.update_ip_address!` relies on mini_sql and does not go through
ActiveRecord. As a result, the query cache is not cleared and hence the
flakiness.
What does this change do?
This change uses the `uncached` method provided by ActiveRecord when
we are fetching the count.
* Remove checkmark for official plugins
* Add author for plugin, which is By Discourse for all discourse
and discourse-org github plugins
* Link to meta topic instead of github repo
* Add experimental flag for plugin metadata and show this as a
badge on the plugin list if present
---------
Co-authored-by: chapoi <101828855+chapoi@users.noreply.github.com>
In the long term we should aim to modernize these places, but for now this change will make them compatible with Ember 5.x (while maintaining compatibility with Ember 3.28)
This commit adds a new `search_default_sort_order` site setting,
set to "relevance" by default, that controls the default sort order
for the full page /search route.
If the user changes the order in the dropdown on that page, we remember
their preference automatically, and it takes precedence over the site
setting as a default from then on. This way people who prefer e.g.
Latest Post as their default can make it so.
Currently to use a limit in the notifications index, you have to also pass recent: true as a param.
This PR:
Adds optional limit param to be used in the notifications query, regardless of the presence of recent
Raises the max limit of the response with recent present from 50 -> 60. It is super weird we have a hard-limit of 50 before with recent param, and 60 without the param.
config.after(:suite) which stops minio server is called every time one
of the groups of parallel tests complete. This works fine most of the
time with parallel spec runs, but sometimes one of these
MinioRunner.stop calls happens while a spec is running in another
process that expects the minio server to be running.
Skipping these tests to avoid flakys for now.
Why this change?
The test became flaky due to d208396c5c.
In that commit, we introduced `page.has_no_css?("div.menu-panel.animating")` to `PageObjects::Components::NavigationMenu::Sidebar#open_on_mobile` but
it did not work as intended because `page.has_no_css?("div.menu-panel.animating")` can return `true` immediately as the `animating` class has not been added
to the element.
What does this change do?
Switch to the `wait_for_animation` system helper to ensure that all
animations have ended on the element.
When we started using NumberField for integer site settings
in e113eff663, we did not end up
passing down a min/max value for the integer to the field, which
meant that for some fields where negative numbers were allowed
we were not accepting that as valid input.
This commit passes down the min/max options from the server for
integer settings then in turn passes them down to NumberField.
c.f. https://meta.discourse.org/t/delete-user-self-max-post-count-not-accepting-1-to-disable/285162
This PR refactors the following:
* leaving all the CSS applied to the old `modal-body` classes in their respective files
* made new clean styling for `.d-modal` and refactored the template to use the new BEM classes
* `inner-`, `middle-`, `outer-` container classes are gone and replaced with simplified `wrapper` and `container` classes
* use standardised max-sizes with modifiers `-large` and `-max`
* lighter backdrop,
* min-width to prevent puny modals
* other styling changes regarding padding, close button,…
* pulled out all modal overrides into a general `modal-overrides` file + cleanup of outdated CSS
* pulled out login and create account modal styling into their own file, cause it's such a big override
* removed old general login.scss file for mobile & desktop
* only kept some remainders I don't want to touch in `app/assets/stylesheets/common/base/login.scss`
Previously we would only recompile a theme locale when its own data changes. However, the output also includes fallback data from other locales, so we need to invalidate all locales when fallback locale data is changed. Building a list of dependent locales is tricky, so let's just invalidate them all.
We ask users to confirm their session if they are making a sensitive
action, such as adding/updating second factors or passkeys. This
commit adds the ability to confirm sessions with passkeys as an option
to the password confirmation.
The `src` of js files is now dependent on the ember-cli/webpack build, so it's not a good thing to check in specs. In CI it passes because the ember-cli build is not run. But locally it would fail if you had a build in `app/assets/javascripts/discourse/dist`.
This commit updates the specs to check for the presence of a stable data attribute instead.
- Remove vendored copy
- Update Rails implementation to look for language definitions in node_modules
- Use webpack-based dynamic import for hljs core
- Use browser-native dynamic import for site-specific language bundle (and fallback to webpack-based dynamic import in tests)
- Simplify markdown implementation to allow all languages into the `lang-{blah}` className
- Now that all languages are passed through, resolve aliases at runtime to avoid the need for the pre-built `highlightjs-aliases` index
Previously, the app HTML served by the Ember-CLI proxy was generated based on a 'bootstrap json' payload generated by Rails. This inevitably leads to differences between the Rails HTML and the Ember-CLI HTML.
This commit overhauls our proxying strategy. Now, we totally ignore the ember-cli `index.html` file. Instead, we take the full HTML from Rails and surgically replace script URLs based on a `data-discourse-entrypoint` attribute. This should be faster (only one request to Rails), more robust, and less confusing for developers.
The most common thing that we do with fab! is:
fab!(:thing) { Fabricate(:thing) }
This commit adds a shorthand for this which is just simply:
fab!(:thing)
i.e. If you omit the block, then, by default, you'll get a `Fabricate`d object using the fabricator of the same name.
This adds the ability to collect stats without exposing them
among other stats via API.
The most important thing I wanted to achieve is to provide
an API where stats are not exposed by default, and a developer
has to explicitly specify that they should be
exposed (`expose_via_api: true`). Implementing an opposite
solution would be simpler, but that's less safe in terms of
potential security issues.
When working on this, I had to refactor the current solution.
I would go even further with the refactoring, but the next steps
seem to be going too far in changing the solution we have,
and that would also take more time. Two things that can be
improved in the future:
1. Data structures for holding stats can be further improved
2. Core stats are hard-coded in the About template (it's hard
to fix it without correcting data structures first, see point 1):
63a0700d45/app/views/about/index.html.erb (L61-L101)
The most significant refactorings are:
1. Introducing the `Stat` model
2. Aligning the way the core and the plugin stats' are registered
There was a registry for preloaded site categories and a new one has
been introduced recently for categories serialized through a
CategoryList.
Having two registries created a lot of friction for developers and this
commit merges them into a single one, providing a unified API.
There is an edge case where the following occurs:
1. The user sets a bookmark reminder on a post/topic
2. The post/topic is changed to a PM before or after the reminder
fires, and the notification remains unread by the user
3. The user opens their bookmark reminder notification list
and they can still see the notification even though they cannot
access the topic anymore
There is a very low chance for information leaking here, since
the only thing that could be exposed is the topic title if it
changes to something sensitive.
This commit filters the bookmark unread notifications by using
the bookmarkable can_see? methods and also prevents sending
reminder notifications for bookmarks the user can no longer see.
When quoting a chat message in a post, if that message contains a mention,
that mention should be ignored. But we've been detecting them and sending
notifications to users. This PR fixes the problem. Since this fix is for
the chat plugin, I had to introduce a new API for plugins:
# We strip posts before detecting mentions, oneboxes, attachments etc.
# We strip those elements that shouldn't be detected. For example,
# a mention inside a quote should be ignored, so we strip it off.
# Using this API plugins can register their own post strippers.
def register_post_stripper(&block)
end
This commit adds an /admin/customize/theme-components route,
that opens the theme page with the components tab pre-selected,
so people can navigate to that directly.
Switches to using a dialog to confirm a session (i.e. sudo mode for
account changes where we want to be extra sure the current user is who
they say they are) to match what we do with passkeys.
Previously, we were parsing webpack JS chunk filenames from the HTML files which ember-cli generates. This worked ok for simple entrypoints, but falls apart once we start using async imports(), which are not included in the HTML.
This commit uses the stats plugin to generate an assets.json file, and updates Rails to parse it instead of the HTML. Caching on the Rails side is also improved to avoid reading from the filesystem multiple times per request in develoment.
Co-authored-by: Godfrey Chan <godfreykfc@gmail.com>
This commit fixes an issue where when some actions were done
(deleting/recovering post, moving posts) we updated the
topic_users.bookmarked column to the wrong value. This was happening
because the SyncTopicUserBookmarked job was not taking into account
Topic level bookmarks, so if there was a Topic bookmark and no
Post bookmarks for a user in the topic, they would have
topic_users.bookmarked set to false, which meant the bookmark would
no longer show in the /bookmarks list.
To reproduce before the fix:
* Bookmark a topic and don’t bookmark any posts within
* Delete or recover any post in the topic
c.f. https://meta.discourse.org/t/disappearing-bookmarks-and-expected-behavior-of-bookmarks/264670/36
This commit adds an `enable_s3_transfer_acceleration` site setting,
which is hidden to begin with. We are adding this because in certain
regions, using https://aws.amazon.com/s3/transfer-acceleration/ can
drastically speed up uploads, sometimes as much as 70% in certain
regions depending on the target bucket region. This is important for
us because we have direct S3 multipart uploads enabled everywhere
on our hosting.
To start, we only want this on the uploads bucket, not the backup one.
Also, this will accelerate both uploads **and** downloads, depending
on whether a presigned URL is used for downloading. This is the case
when secure uploads is enabled, not anywhere else at this time. To
enable the S3 acceleration on downloads more generally would be a
more in-depth change, since we currently store S3 Upload record URLs
like this:
```
url: "//test.s3.dualstack.us-east-2.amazonaws.com/original/2X/6/123456.png"
```
For acceleration, `s3.dualstack` would need to be changed to `s3-accelerate.dualstack`
here.
Note that for this to have any effect, Transfer Acceleration must be enabled
on the S3 bucket used for uploads per https://docs.aws.amazon.com/AmazonS3/latest/userguide/transfer-acceleration-examples.html.
When Discourse first introduced brotli support, reverse-proxy/CDN support for passing through the accept-encoding header to our NGINX server was very poor. Therefore, a separate `/brotli_assets/...` path was introduced to serve the brotli assets. This worked well, but introduces additional complexity and inconsistencies.
Nowadays, Brotli encoding is well supported, so we don't need the separate paths any more. Requests can be routed to the asset `.js` URLs, and NGINX will serve the brotli/gzip version of the asset automatically.
For deprecated site settings, we log out a warning when
the old setting is used. However when we convert all the client
settings to JSON, we are creating a lot of log noise like this:
> Deprecation notice: `SiteSetting.anonymous_posting_min_trust_level` has been deprecated.
We don't need to do this because we are just dumping the JSON.
We updated scheduled admin checks to run concurrently in their own jobs. The main reason for this was so that we can implement re-check functionality for especially flaky checks (e.g. group e-mail credentials check.)
This works in the following way:
1. The check declares its retry policy using class methods.
2. A block can be yielded to if there are problems, but before they are committed to Redis.
3. The job uses this block to either a) schedule a retry if there are any remaining or b) do nothing and let the check commit.
When submitting files through the form template upload field, we were having an issue where, although a validation error message was being presented to the user, the upload was still coming through, because `PickFilesButton`'s validation happens **after** the Uppy mixin finished the upload and hit `uploadDone`.
This PR adds a new overridable method to the Uppy mixin and overrides it with the custom validation, which now happens before the file is sent.
Additionally, we're now also using `uploadingOrProcessing` as the source of truth to show the upload/uploading label, which seems more reliable.
This PR does some preparatory refactoring of scheduled admin checks in order for us to be able to do custom retry strategies for some of them.
Instead of running all checks in sequence inside a single, scheduled job, the scheduled job spawns one new job per check.
In order to be concurrency-safe, we need to change the existing Redis data structure from a string (of serialized JSON) to a list of strings (of serialized JSON).
This commit introduces a new feature that allows theme developers to manage the transformation of theme settings over time. Similar to Rails migrations, the theme settings migration system enables developers to write and execute migrations for theme settings, ensuring a smooth transition when changes are required in the format or structure of setting values.
Example use cases for the theme settings migration system:
1. Renaming a theme setting.
2. Changing the data type of a theme setting (e.g., transforming a string setting containing comma-separated values into a proper list setting).
3. Altering the format of data stored in a theme setting.
All of these use cases and more are now possible while preserving theme setting values for sites that have already modified their theme settings.
Usage:
1. Create a top-level directory called `migrations` in your theme/component, and then within the `migrations` directory create another directory called `settings`.
2. Inside the `migrations/settings` directory, create a JavaScript file using the format `XXXX-some-name.js`, where `XXXX` is a unique 4-digit number, and `some-name` is a descriptor of your choice that describes the migration.
3. Within the JavaScript file, define and export (as the default) a function called `migrate`. This function will receive a `Map` object and must also return a `Map` object (it's acceptable to return the same `Map` object that the function received).
4. The `Map` object received by the `migrate` function will include settings that have been overridden or changed by site administrators. Settings that have never been changed from the default will not be included.
5. The keys and values contained in the `Map` object that the `migrate` function returns will replace all the currently changed settings of the theme.
6. Migrations are executed in numerical order based on the XXXX segment in the migration filenames. For instance, `0001-some-migration.js` will be executed before `0002-another-migration.js`.
Here's a complete example migration script that renames a setting from `setting_with_old_name` to `setting_with_new_name`:
```js
// File name: 0001-rename-setting.js
export default function migrate(settings) {
if (settings.has("setting_with_old_name")) {
settings.set("setting_with_new_name", settings.get("setting_with_old_name"));
}
return settings;
}
```
Internal topic: t/109980
NOTE: Most of this is experimental and will be removed at a later
time, which is why things like translations have not been added.
The new /admin-revamp UI uses a sidebar for admin nav. This initial
step adds a script to generate a map of all the current admin nav
into a format the sidebar to read. Then, people can experiment
with different changes to this structure.
The structure can then be edited from `/admin-revamp/config/sidebar-experiment`,
and it is saved to local storage so people can visually experiment with different ways
of showing the admin sidebar links.
Two changes were introduced:
1. Reorder links on sidebar section is removed. Clicking and holding the mouse for 250ms was unintuitive;
2. Fixed bugs when reorder is done in edit modal.
