While this is not a secret and in theory should be in a file under
version control, currently the CONSUL installer disables delayed jobs by
default, meaning we were keeping two versions of the delayed jobs
configuration file, and some existing configurations have their settings
defined in a file in capistrano's `shared` folder.
So we're moving existing settings to the secrets file.
The new version of CKEditor loads the balloonpanel and balloontoolbar
plugins. Even if we don't need them, I haven't found a way to prevent
them from loading, meaning we have to precompile them.
This version solves a security issue:
https://ckeditor.com/cke4/release/CKEditor-4.11.0
Note this version adds a `ckeditor/samples` folder, which is
automatically added to the application's assets manifest even if we
remove all CKEditor references in our application. One of the files in
that folder makes ExecJS raise a syntax error, causing every page to
raise a 500 error.
The `and` and `or` keywords are not equivalent to `&&` and `||` and its
use is counterintuitive. Here's an example
```
good = true && false # good if false
bad = true and false # bad is true
```
The reason is `and` and `or` are control flow operators. So the code:
```
bad = true and false
```
Is equivalent to:
```
if bad = true
false
end
```
We were inconsistent on this one. I consider it particularly useful when
a method starts with a `return` statement.
In other cases, we probably shouldn't have a guard rule in the middle of
a method in any case, but that's a different refactoring.
We were very inconsistent regarding these rules.
Personally I prefer no empty lines around blocks, clases, etc... as
recommended by the Ruby style guide [1], and they're the default values
in rubocop, so those are the settings I'm applying.
The exception is the `private` access modifier, since we were leaving
empty lines around it most of the time. That's the default rubocop rule
as well. Personally I don't have a strong preference about this one.
[1] https://rubystyle.guide/#empty-lines-around-bodies
We were already using `find_by` most of the time.
Since there are false positives related to our `find_by_slug_or_id!` and
`find_by_manger_login` methods, which cannot be replaced with `find_by`,
I'm adding it indicating the "refactor" severity.
This will be the default behaviour in Rails 5.1, and it's a much better
approach.
I've checked the code and luckily there doesn't seem to be a single
place where we could accidentally stop the callback chain by returning
false in (for example) a `before_save` callback.
We were monkey-patching FoundationRailsHelper::Formbuilder, which made
form customization difficult. We can inherit from it, which is the
standard way of extending what an existing class does, and make our form
the default one.
Internet Explorer 9 was released eight years ago. Besides that, we don't
really support IE8 anyway, since we show a popup to IE8 users saying
we don't support it, we haven't maintained the IE8-specific CSS file for
years, and we don't test our JavaScript against IE8.
We're reading the value from the database, but the
`ApplicationMailer.default` method is evaluated when the application is
started. So if we don't use a Proc, we'll need to restart the server
every time we change the value in the database, or else the old value
will still be used.
Using a Proc makes sure the mailer from address is evaluated at runtime,
so emails are sent using the from address currently defined in the
database.
The same situation took place using the devise mailer. Now we don't need
to check for the settings table being present because the Proc in the
devise initializer won't be evaluated before the settings table is
created and populated.
Proposal, Debate and Comment "globalize_accessors" class method were
loaded before application available locales initialization because of
graphql initializer. This will cause unexpected translation errors at
any translatable classes declared at graphql api definition (api.yml).
Doing GraphQL initialization after application initialization should
solve this issue.
Proposal, Debate and Comment "globalize_accessors" class method were
loaded before application available locales initialization because of
graphql initializer. This will cause unexpected translation errors at
any translatable classes declared at graphql api definition (api.yml).
Doing GraphQL initialization after application initialization should
solve this issue.
Rails 5 changed the initialization order, and now our initializers were
running before the `append_assets_path` initializer for each engine,
which prepended application assets to the custom assets we prepended in
the initializer.
Moving the code to the `config.after_initialize` code didn't work
either, since the paths added there were ignored by the application.
Adding another initializer to the Rails Engine is a hack, but solves the
problem.
Metrics/LineLength: Line is too long.
RSpec/InstanceVariable: Use let instead of an instance variable.
Layout/TrailingBlankLines: Final newline missing.
Style/StringLiterals: Prefer double-quoted strings.
DEPRECATION WARNING: #table_exists? currently checks both tables and
views. This behavior is deprecated and will be changed with Rails 5.1
to only check tables. Use #data_source_exists? instead.
