Previously the condition was needed because _without it_ the Admin::Poll::Questions::Answers::ImagesController would have resulted in settings? evaluating to true. This was undesired because that controller was scoped under Polls, so only polls? should have evaluated to true. Now that we have moved the images link to the customization menu, this check is not necessary anymore.
There were already some menu items to customization pages under the "Site content" menu. It therefore makes sense to move "Custom images" and "Custom content blocks" (which were previously
located under "Settings") to "Site content" as well.
This syntax has been added in Ruby 3.1.
Not using a variable name might not be very descriptive, but it's just
as descriptive as using "block" as a variable name. Using just `&` we
get the same amount of information than using `&block`: that we're
passing a block.
We're still using `&action` in `around_action` methods because here we
aren't using a generic name for the variable, so (at least for now) we
aren't running this cop on controllers using `around_action`.
We were getting a warning since upgrading to Rails 6.1:
DEPRECATION WARNING: Calling `delete` to an ActiveModel::Errors messages
hash is deprecated. Please call `ActiveModel::Errors#delete` instead.
So we're deleting the error instead of deleting the message.
We were getting a warning in one of the tests:
DEPRECATION WARNING: Rendering actions with '.' in the name is
deprecated: application/nonExistentJavaScript.js
I haven't found a case where the behavior on production environments is
different due to this change; the application seems to behave the same
way as it used to. So I'm not adding tests for this change.
Internet Explorer 8 was released in 2009 and people using it already
know that most web pages look broken on it, so we don't need to warn
them.
Removing it makes our application layout file much easier to read and
modify.
This rule was added in rubocop 1.44.0. It's useful to avoid accidental
`unless !condition` clauses.
Note we aren't replacing `unless zero?` with `if nonzero?` because we
never use `nonzero?`; using it sounds like `if !zero?`.
Replacing `unless any?` with `if none?` is only consistent if we also replace
`unless present?` with `if blank?`, so we're also adding this case. For
consistency, we're also replacing `unless blank?` with `if present?`.
We're also simplifying code dealing with `> 0` conditions in order to
make the code (hopefully) easier to understand.
Also for consistency, we're enabling the `Style/InverseMethods` rule,
which follows a similar idea.
We originally added the `cached_votes_up > 0` in commit 4ce95e273
because back then `cached_votes_up` was used in the denominator. That's
no longer the case, and it doesn't make sense to mark a debate with 1
vote and 10 flags as conflictive but not doing it when the debate has no
votes and 1000 flags.
We're fixing the bug right now because we're about to change the
affected line in order to apply a new rubocop rule.
Note we're excluding a few files:
* Configuration files that weren't generated by us
* Migration files that weren't generated by us
* The Gemfile, since it includes an important comment that must be on
the same line as the gem declaration
* The Budget::Stats class, since the heading statistics are a mess and
having shorter lines would require a lot of refactoring
The `id:` condition works with a list of proposals that might include
nil items, so there's no need to compact it and manually get the IDs.
Note this scope is probably inefficient, since it instantiates proposal
objects (with the `find_voted_items` method) for something that could be
done with a database query. We might change it in he future.
We were using `prepend: true`, but it doesn't seem to be necessary.
We were also loading the draft versions twice in the index, so we can
remove the line loading them a second time.
We forgot to remove it in commit 8066b96fe. We're removing it now
because these methods didn't follow the Layout/BlockAlignment rubocop
rule, which we're about to add.
For the HashAlignment rule, we're using the default `key` style (keys
are aligned and values aren't) instead of the `table` style (both keys
and values are aligned) because, even if we used both in the
application, we used the `key` style a lot more. Furthermore, the
`table` style looks strange in places where there are both very long and
very short keys and sometimes we weren't even consistent with the
`table` style, aligning some keys without aligning other keys.
Ideally we could align hashes to "either key or table", so developers
can decide whether keeping the symmetry of the code is worth it in a
case-per-case basis, but Rubocop doesn't allow this option.
We were already applying these rules in most cases.
Note we aren't enabling the `MultilineArrayLineBreaks` rule because
we've got places with many elements whire it isn't clear whether
having one element per line would make the code more readable.
Since IRB has improved its support for multiline, the main argument
towars using a trailing dot no longer affects most people.
It still affects me, though, since I use Pry :), but I agree
leading dots are more readable, so I'm enabling the rule anyway.
We added the `user_id` rule in commit edaf420f5. To be honest, I'm not
sure what we meant, since I haven't found URLs containing the user id.
So we're treating it as if it was a typo and we wanted to do the same
thing we did with other parameters.
In the `i18n_translation` initializer, we're overwriting the `t` helper
so calling it uses custom translations if they're available.
However, ViewComponent doesn't use the `t` helper but implements its own
`t` method. So, when calling the `t` method in a component, we weren't
using our implementation of the `t` helper, and so we weren't loading
custom translations.
Using the `t` helper in components solves the issue.
There was a test where we were directly testing a method in a component,
and that method uses the `t` helper. This caused an error when running
the test:
ViewComponent::Base::ViewContextCalledBeforeRenderError:
`#helpers` can't be used during initialization, as it depends on the
view context that only exists once a ViewComponent is passed to the
Rails render pipeline.
Using `render_inline` in the test and testing the generated HTML, as
recommended in the ViewComponent documentation, solves the issue.
