Some institutions using CONSUL have expressed interest in this feature
since some of their tenants might already have their own domains.
We've considered many options for the user interface to select whether
we're using a subdomain or a domain, like having two separate fields,
using a check box, ... In the end we've chosen radio buttons because
they make it easier to follow a logical sequence: first you decide
whether you're introducing a domain or subdomain, and then you enter it.
We've also considered hiding this option and assuming "if it's got a
dot, it's a domain". However, this wouldn't work with nested subdomains
and it wouldn't work with domains which are simply machine names.
Note that a group of radio buttons (or check boxes) is difficult to
style when the text of the label might expand over more than one line
(as is the case here on small screens); in this case, most solutions
result in the second line of the label appearing immediately under the
radio button, instead of being aligned with the first line of the label.
That's why I've added a container for the input+label combination.
They were added in Rubocop 1.24.0.
Even if we were already applying FileRead everywhere, this is something
we've manually fixed in the past. Another reason to add it is that these
rules are deeply related.
This rule was added in Rubocop 1.18.0, but we didn't add it back then.
Since we're applying it most of the time, we might as well be consistent
and apply it everywhere.
This rule was added in rubocop-rails 2.12.0. It doesn't catch the case
we've seen the most in the past, though: using `travel_back` to finish
the test will not raise an offense.
However, it does detect a useless `travel_back` call in `after` blocks,
so I guess it's better than nothing.
This cop scans only the tests files by default, but we prefer to scan all
application Ruby files, so when a developer uses the class method
`I18n.locale=`, the cop will embrace using the method
`I18n.with_locale` instead. By doing this way, the cop will help
developers to avoid unexpected translation errors.
Quoting the Rails 6 guides:
> I18n.locale can leak into subsequent requests served by the same
thread/process if it is not consistently set in every controller. For
example executing I18n.locale = :es in one POST requests will have
effects for all later requests to controllers that don't set the locale,
but only in that particular thread/process. For that reason, instead of
I18n.locale = you can use I18n.with_locale which does not have this
leak issue.
Now we enabled the cop for all application Ruby files; we have to
remove the assignments at the controller level to set the request
locale. As Rails 6 guides suggest [1], we can use the `around_action`
controller callback to set each request locale without breaking the
rule.
This cop will warn CONSUL developers when using `I18n.locale`
assignment embracing them to use the `I18n.with_locale`instead.
[1] https://guides.rubyonrails.org/i18n.html#managing-the-locale-across-requests
The `open` method can be used to open files or URLs and it's deprecated
in Ruby 2.7. In this case, it's clear we're dealing with a URL, so we
can use `URI.parse`.
The code was a bit strange, since it returned a value and had a side
effect: opening the URL. I'm not sure about the intention of the code;
my best guess is we wanted to test the URL exists and was accessible
before returning it (and, if that's the case, IMHO the code should be a
bit more explicit in order to show the intention behind it), but it
could also be an unintended side effect which was there by accident.
Now the URL is no longer opened; if the URL isn't accessible, we'll find
out when trying to connect to it with the Savon client.
ActiveStorage support was added to CKEditor in version 5.1.0. However,
we can't upgrade to version 5.0.0 or later because it only supports
loading CKEditor via CDN.
So we're copying the code related to ActiveStorage instead. I tried to
move it to the `vendor/` folder, but couldn't find a way to load it from
there and if I found one I wouldn't be sure it'd work on production
environments.
We forgot to use it in one place, and we've found out other institutions
using CONSUL whose developers aren't so familiar with Ruby also break
this rule, so it might be better to add it explicitly.
The `column` method in ActiveRecord::ConnectionAdapters::TableDefinition
supports adding the `index:` option. The documentation says:
> Instantiates a new column for the table. See connection.add_column for
> available options.
>
> Additional options are:
>
> :index - Create an index for the column. Can be either true or an
> options hash.
So basically the `connection.add_column` method silently ignores the
`index:` option, and whenever we intended to create an index this way,
we didn't.
We're creating a new migration where we properly add the indexes that
weren't added when we intended to.
Thanks to the rubocop-rails team, who added this cop in version 2.11.0
and helped us notice this bug.
This rule was added in rubocop-rails 2.11.0.
Although we prevent I18n locale leaking between tests by setting it
before each test, the `with_locale` method makes the scope of the locale
change more obvious.
We were already using it in most places. Since rubocop-rails 2.11.0,
this rule also detects offenses when using the `head` method, which we
were using with a plain `404`.
We don't need to use `block_given?` since we specifically pass the block
parameter, particularly since we added the Style/ExplicitBlockArgument
rule in commit a102f3f0a.
We removed it in commit d639cd58 because it recommended using `uniq`
where `distinct` was more appropriate. This has been fixed in
rubocop-rails 2.6.0.
This is something we should do everywhere because concurrent requests
might bypass Rails uniqueness validations. However, since there are
places where we aren't applying this rule and adding a unique index
means we also need to destroy any hypothetical duplicate records, it's
something we aren't going to solve right now. Therefore we use the
"refactor" severity so existing offenses don't get in our way.
In any case, we're adding the rule so we don't make the same mistake in
the future.
This rule was added in rubocop-rspec 1.39.0. The `visible: false` option
is equivalent to `visible: :all`, but we generally use it meaning
`visible: :hidden` for positive expectations and `visible: :all` for
negative expectations.
The only exceptations are tests where we count the number of map icons
present. I'm assuming in this case we care about the number of map icons
independently on whether they are currently shown in the map, so I'm
keeping the `visible: :all` behavior in this case.
