These columns were causing Rails 5.2 to throw a warning when ordering by
them, as if they weren't valid column names:
DEPRECATION WARNING: Dangerous query method (method whose arguments are
used as raw SQL) called with non-attribute argument(s):
:"budget/investments_count". Non-attribute arguments will be disallowed
in Rails 6.0. This method should not be called with user-provided
values, such as request parameters or model attributes. Known-safe
values can be passed by wrapping them in Arel.sql().
This change also makes their names consistent with the rest of our
tables and columns.
Rails 5.2 crashes in the `db:create` task because it tries to run the
`after_initialize` block before the database is created.
The easiest way to solve it is to move the code out of the initializer
and calculate the API type definitions on demand. Note results are still
cached using a class instance variable (not to be confused with a class
variable), and so once definitions are obtained, they will remain
constant until the application is restarted, even in the development
environment.
Recent versions introduce the `Layout/SpaceAroundMethodCallOperator`,
which we are going to use. We aren't upgrading to the latest rubocop
version because it conflicts with the version of Capybara we're using
and because it isn't supported by Hound.
Some rules have been renamed:
Layout/IndentAssignment is now Layout/AssignmentIndentation
Layout/IndentHeredoc is now Layout/HeredocIndentation
Layout/LeadingBlankLines is now Layout/LeadingEmptyLines
Layout/Tab is now Layout/IndentationStyle
Layout/TrailingBlankLines is now Layout/TrailingEmptyLines
Lint/StringConversionInInterpolation is now Lint/RedundantStringCoercion
Metrics/LineLength is now Layout/LineLength
Note after upgrading we get a new "offense" in the `StartWith` rule, so
we're changing the code in order to fix it.
These routes are solved in a different way because of an inconsistency:
we define `groups` and `budget_investments`; we should either use the
`budget_` prefix in all places or remove it everywhere.
We can now share code using `polymorphic_path` even with these models.
In the past, we couldn't use `polymorphic_path` in many places. For
instance, `polymorphic_path(budget, investment)` would return
`budget_budget_investment_path`, while in our routes we had defined
`budget_investment_path`.
With the `resolve` method, introduced in Rails 5.1, we can use symbols
to define we want it to use `investment` instead of `budget_investment`.
It also works with nested resources, so now we can write
`polymorphic_path(investment)`.
This makes the code for `resource_hierarchy_for` almost impossible to
understand. I reached this result after having a look at the internals
of the `resolve` method in order to get its results and then remove the
symbols we include.
Note using this method will not make admin routes compatible with
`polymorphic_path`. Quoting from the Rails documentation:
> This custom behavior only applies to simple polymorphic URLs where a
> single model instance is passed and not more complicated forms, e.g:
> [example showing admin routes won't work]
Also note that now the `admin_polymorphic_path` method will not work for
every model due to inconsistencies in our admin routes. For instance, we
define `groups` and `budget_investments`; we should either use the
`budget_` prefix in all places or remove it everywhere. Right now the
code only works for items with the prefix; it isn't a big deal because
we never call it with an item without the prefix.
Finally, for unknown reasons some routing tests fail if we use
`polymorphic_path`, so we need to redefine that method in those tests
and force the `only_path: true` option.
This plugin provides more control over tables and solves a JS error thrown
when user clicks on "Cell properties" ckeditor feature.
https://ckeditor.com/cke4/addon/tabletools
All of these plugins are not used anywhere.
Change introduced at ckeditor initializer will ommit unneeded
precompilation of plugins assets on production environments.
Change introduced at ckeditor config file adresses the problem with assets
pipeline fallback on testing environments described here: #2711. Now plugins
that are explicitly disabled will not be precomiled when running ckeditor
javascript enabled feature specs.
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.
