Why this change?
We have been chasing a problem with our flaky system test where the user
is logged out when it should never be.
What does this change do?
1. Logs the request path when lookup a user auth token.
2. Logs the request path and also the current thread's object id in
ActiveRecord query logs.
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.
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
What is this change required?
I noticed that actions in `SidebarSectionsController` resulted in
lots of N+1 queries problem and I wanted a solution to
prevent such problems without having to write N+1 queries tests. I have
also used strict loading for `SidebarSection` queries in performance
sensitive spots.
Note that in this commit, I have also set `config.active_record.action_on_strict_loading_violation = :log`
for the production environment so that we have more visibility of
potential N+1 queries problem in the logs. In development and test
environment, we're sticking with the default of raising an error.
In production, `eager_load=true`. This sometimes leads to boot errors which are not present in dev/test environments. Running `zeitwerk:check` in CI will help us to pick up on any errors early.
This commit also introduces a `DISCOURSE_ZEITWERK_EAGER_LOAD` environment variable to make it easier to toggle the behaviour when developing locally.
This comment has nothing to do with the `eager_load` configuration. It must be left over from some historical refactoring. Removing to avoid confusion.
Rails introduced a environment variables RAILS_LOG_TO_STDOUT in the
template for new projects here: https://github.com/rails/rails/pull/23734
This commit adds the same code to the discourse production environment,
making it easy to configure logging to stdout in production for
environments which already support log collection via stdout.
We now use Ember CLI (core/plugins) and DiscourseJSProcessor (themes) for all Ember and template compilation. This commit removes the remnants of the legacy Sprockets-based Ember compilation system.
Sprockets, and its DiscourseJSProcess-based Babel transformations, is still in use for a few assets. Ideally that will be removed/replaced in the near future.
* FEATURE: Make General the default category
* Set general as the default category in the composer model instead
* use semicolon
* Enable allow_uncategorized_topics in create_post spec helper for now
* Check if general_category_id is set
* Enable allow_uncategorized_topics for test env
* Provide an option to the create_post helper to not set allow_uncategorized_topics
* Add tests to check that category… is not present and that General is selected automatically
Adds DISCOURSE_SMTP_OPEN_TIMEOUT and DISCOURSE_SMTP_READ_TIMEOUT GlobalSettings that allow site admins to configure the SMTP connection behavior to match the needs of their SMTP server.
Following the Rails 7 upgrade, the `DISCOURSE_SMTP_ENABLE_START_TLS`
setting doesn’t work anymore. This is because Rails upgraded the
`net-smtp` gem to the 0.3.1 version which enables `starttls` by default.
The `mail` gem doesn’t support this new behavior yet and doesn’t know
how to disable TLS. This should be fixed in an upcoming release.
Meanwhile applying this patch allows us to get back the previous
behavior which is expected by many.
There are 2 changes in this PR:
1) Add a new environment variable called `DISCOURSE_SKIP_CSS_WATCHER` to disable our stylesheet watcher, and make the `qunit:test` rake task set this variable on the Unicorn/Rails server it spins up to disable our stylesheet watcher when running the tests because it doesn't really need it.
2) Print more Chrome logs (such as network/security errors) to the console.
Over the years we accrued many spelling mistakes in the code base.
This PR attempts to fix spelling mistakes and typos in all areas of the code that are extremely safe to change
- comments
- test descriptions
- other low risk areas
In development we regularly restart/reload Rails, which wipes out the schema cache. This then has to be regenerated using DDL queries on the database.
Instead, we can make use of the `rake db:schema:cache:dump` command. This will dump the schema cache to a YAML file, and then load it when needed. This is significantly faster than rebuilding the cache from DDL queries every time.
On every request, Rails checks to see whether any ruby code has been changed on the filesystem. The default FileUpdateChecker does this by iterating over every file on the autoload_paths and comparing its modified-time.
In Discourse, our autoload path of `/app` includes the `/app/assets` directory, and therefore thousands of non-ruby files (e.g. node_modules). This makes the `Dir["/app"]` call very slow (>100ms in my case). On my machine, every Rails-handled request spends around 150-200ms in the FileUpdateChecker. This commit introduces a couple of changes to completely eliminate this wasted time:
- The `/app/assets` directory is excluded from the file watchers. For me, this cut the time spent in the file_watcher to around 50-100ms
- Switches our development config to use the `EventedFileUpdateChecker`, which makes use of the `listen` gem to subscribe to os-specific notifications of changes. This completely removes the `FileUpdateChecker` from the critical path
On my machine, topic_list requests now return in around 80ms (previously >200ms). Live code reload still works as it did before
It's important that we don't perform pg_dumps against databases
running behind pgbouncer.
We had an old monkey-patch to prevent this, but following some [recent
internal rails refactoring](5488686851),
the patch no longer works. Instead, we can use the official
`config.active_record.dump_schema_after_migration` option.
Setting this to false in production is recommended by Rails, and is the
default for newly generated Rails applications.
Background: RFC 8314 3.3 asks that:
clients and servers SHOULD implement both STARTTLS on
port 587 and Implicit TLS on port 465
Discourse currently cannot be configured this way.
With this patch, it's possible to set
DISCOURSE_SMTP_FORCE_TLS=true to use implicit TLS on port 465
If RAILS_DISABLE_ACTIVERECORD_LOGS=1 is passed when starting Rails, none of the query log output will show. This is very useful for debugging with breakpoints because logs are not flooding in constantly.
This is where they should be as far as ember is concerned. Note this is
a huge commit and we should be really careful everything continues to
work properly.
byebug, ruby-prof, better_errors and rbtrace are very MRI specific, flag
them as such
This helps move forward on potential jruby and truffleruby experiments
The env var `RAILS_ENABLE_TEST_LOG` didn't seem to do anything if enabled. This now sets the logger to STDOUT if `RAILS_ENABLE_TEST_LOG` is enabled and also introduces `RAILS_TEST_LOG_LEVEL` so the level of the logging in the console can be provided (default info).
Note: I am not sure if the original behaviour is expected. I can add an additional env var to enable the STDOUT logging if required
- Refactor source_url to avoid using eval in development
- Precompile handlebars in development
- Include template compilers when running qunit
- Remove unsafe-eval in development CSP
- Include unsafe-eval only for qunit routes in development
From the better_errors README:
> Better Errors works by leaving a lot of context in server process memory. If you're using a web server that runs multiple "workers" it's likely that a second request (as happens when you click on a stack frame) will hit a different worker. That worker won't have the necessary context in memory, and you'll see a Session Expired message.
After a small conversation, we decided that we can set `public_file_server.enabled` to false in the `test` environment to have the same value as `production`.
This renames the DISCOURSE_ENV_HOST var @eviltrout introduced in 95a9a544
to DISCOURSE_ENV_HOSTS and allows for a comma delimited list of hosts
This is useful for testing plugins and customized host names
Rails 6 seems to introduce a whitelist of allowed hosts. I personally
use `dev.local` for development and this no longer works.
This introduces a new ENV variable, `DISCOURSE_DEV_HOST`. If present,
it will whitelist that host for development mode.
This reduces chances of errors where consumers of strings mutate inputs
and reduces memory usage of the app.
Test suite passes now, but there may be some stuff left, so we will run
a few sites on a branch prior to merging
