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?
A system test which was testing our ability to add a form template to a
category was flaky. This turned out to be a client side bug where the
`FormTemplateChooser` dropdown may not display any dropdown options if
the ajax request to fetch the categories form template has not been
completed when the dropdown is opened.
What does this change do?
Make use of select-kit's `triggerSearch` function to fetch the records
which will properly rerender the dropdown options.
Why this change?
The assertion does not make use of Capybara's waiting strategy and is
not really testing anything meaningful by asserting for the src of the
img element.
post action feedback is the mechanism in which we provide visual feedback
to the user when a post action is clicked, in cases where the action is a
background (hidden to user) for example: copying text to the clipboard
Core uses this to share post links, but other plugins (for example: AI) use
this to share post transcripts via the clipboard.
This adds a proper plugin API to consume this functionality
`addPostMenuButton` can provide a builder that specified a function as the action.
This function will be called with an object that has both the current post and a method for showing feedback.
When validating with a dynamic set of values, especially one that might change during runtime, we should use a lambda or a proc to ensure that the validation uses the most up-to-date set of values. This is particularly important when using config.eager_load = true, which can cause some elements to be loaded only once at startup, thus not reflecting changes made at runtime.
This was the root cause of the issues here, as we were adding more ReviewableScore types after initial load through: `register_reviewable_type Chat::ReviewableMessage`
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
- Add plugin outlet to `AdminUserFieldItem`
- Add ability to include custom fields when saving `AdminUserFieldItem`
- Update plugin API with `includeUserFieldPropertiesOnSave` per ☝️
- Add `DiscoursePluginRegistry` to `UserFieldsController` to add custom columns
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).
Small visual improvements for chat header on mobile:
- makes the Back to Forum target size slightly narrower
- makes the text color consistent between header and back button (and d-icon)
- makes the chat heading bold
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
In non secure contexts (HTTP vs HTTPS) which many run in development the
`clipboardCopy` method falls back to and an exec hack.
However, callers expect a promise from this method and the fallback just
returns a boolean.
This change passes down all params to the home logo widget (rather than explicitly setting minimized). This will allow for flexible logo sizing in the Discourse full width theme component.
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.
This change simplifies the layout of our header when chat is open on mobile. The search icon and hamburger menu icons are also hidden and the Discourse logo is replaced by a ← Forum link to make it easier to continue where you left off within the forum (prior to this update the user could only go back to the forum index page).
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.
With certain conditions, this issue does not show up. The easiest way to reproduce this is probably to do either of this
- Use a 3G slow connection or;
- Add a breakpoint to scrolling-post-stream.topRefresh (anon)
- (and optionally lock-on.lock)
This issue is happening because there are multiple areas that set scroll location in the post stream when loading a topic. In our case, sometimes lock-on is triggering and scrolling to post_1, before ?page=2's post_21 is being scrolled to, due to posts above post_21 can finishing loading at different times. This causes some calculations to not add up, as being in the middle of a post stream has different calculations than being at the top of the post stream.
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
Why this change?
Currently we only rerun failing tests to check if they are flaky tests
when there are 10 or less failing tests. When there are more than 10
failing tests in the first run, we assume that the odds of those tests
being flaky are low and do not rerun the tests. However, there was a bug
where we do not clean up the potential flaky tests being logged when
there are too many test failures. This resulted in those test failures
being treated as flaky tests.
What does this change do?
Clean up the flaky tests report when we do not rerun the tests.
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.
The problem:
Removing the options to addEventListener results in events that are properly cleaned up when the search menu is removed. Previously every time you opened the search menu, the listeners would be attached again, and clicking outside even after it was closed would fire the function again and again (N times as you opened the search menu!)
This was made far far worse in this commit c91d053, where I called close() to remove focus from the search input in the event that the search menu is rendered outside the header.
The problem with this was 2-fold. The close function tried to focus the search header button in core here. When the events aren't cleanup up and that happens... you can't do anything in the app.
The solution:
We don't need the event listeners to close the search menu when it's rendered from the header. The widget header handles clicks outside of the header. Sooo
1. Only register them for standalone search menus
2. Remove the passive options to the listeners so that they are properly removed on close
3. Call close() to unfocus input rather than just closing panel
4. Rename passed in are closeSearchMenu -> onClose because it's more accurate. It's really a callback.
