Class variables in Ruby are not the same as instance variables of a
class. They're particularly tricky when it comes to inheritance and
modules.
In the case of the Measurable module, for example, using a class
variable will make all classes including the Measurable module share
the same value. However, that's not what we want; we want the variable
to be different for every class. And that's accomplished using a class
instance variable.
Although in this case it would probably be better to remove the caching
variable. I don't think these methods are called more than once during a
request, and even if they did it's highly unlikely the would become a
bottleneck.
I had mixed feelings about this rule, since I like spaces where
possible.
However, I changed my mind when I realized writing `->(thing) { }` was
similar to defining a method, and we don't have a space before the
parenthesis when defining a method.
This method is ambiguous. Sometimes we use it to set invalid data in
tests (which can usually be done with `update_column`), and other times
we use it instead of `update!`.
I'm removing it because, even if sometimes it could make sense to use
it, it's too similar to `update_attributes` (which is an alias for
`update` and runs validations), making it confusing.
However, there's one case where we're still using it: in the
ActsAsParanoidAliases module, we need to invoke the callbacks, which
`update_column` skips, but tests related to translations fail if we use
`update!`. The reason for this is the tests check what happens if we
restore a record without restoring its translations. But that will make
the record invalid, since there's a validation rule checking it has at
least one translation.
I'm not blacklisting any other method which skips validations because we
know they skip validations and use them anyway (hopefully with care).
Not doing so has a few gotchas when working with relations, particularly
with records which are not stored in the database.
I'm excluding the related content file because it's got a very peculiar
relationship with itself: the `has_one :opposite_related_content` has no
inverse; the relation itself is its inverse. It's a false positive since
the inverse condition is true:
```
content.opposite_related_content.opposite_related_content.object_id ==
content.object_id
```
Usually when we specify a `belongs_to` relations, we also specify its
equivalent `has_many`. That allows us to write, for example:
`topic.user.topics`.
Just like we do in the Budget module, and in some places in the Poll and
Legislation modules, we don't need to specify the class name when the
name of the relation matches the name of a class in the same module.
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.
In Ruby, the Kernel class defined the `open` method, which is available
for (almost) every object. So creating a scope with the name `open`
generates a warning indicating we are overwriting the existing `open`
method.
While this warning is pretty much harmless and we could ignore it, it
generates a lot of noise in the logs. So I'm "undefining" the method
before generating the scope, so we don't get the warning all the time.
Having exceptions is better than having silent bugs.
There are a few methods I've kept the same way they were.
The `RelatedContentScore#score_with_opposite` method is a bit peculiar:
it creates scores for both itself and the opposite related content,
which means the opposite related content will try to create the same
scores as well.
We've already got a test to check `Budget::Ballot#add_investment` when
creating a line fails ("Edge case voting a non-elegible investment").
Finally, the method `User#send_oauth_confirmation_instructions` doesn't
update the record when the email address isn't already present, leading
to the test "Try to register with the email of an already existing user,
when an unconfirmed email was provided by oauth" fo fail if we raise an
exception for an invalid user. That's because updating a user's email
doesn't update the database automatically, but instead a confirmation
email is sent.
There are also a few false positives for classes which don't have bang
methods (like the GraphQL classes) or destroying attachments.
For these reasons, I'm adding the rule with a "Refactor" severity,
meaning it's a rule we can break if necessary.
Usually when we use `try` we actually mean `try!`, which is the same as
the safe navigation operator. However, there are a few cases where we
actually mean to execute a method if the object responds to that method.
In those cases using `try` would actually be OK, but in order to avoid
confusion as to whether we mean to check for `respond_to?` or we mean to
use safe navigation, I'm removing all usages of `try`.
We were converting markdown to HTML every time we saved a record, which
has the same problems as sanitizing HTML before saving it to the
database, particularly because the body of a legislation draft is stored
in a translations table.
Performance-wise this isn't a problem: converting a text with more than
200_000 characters takes about a milisecond on my machine.
Note we need to modify a migration generated by globalize, since the
method `create_translation_table!` would fail now that we don't define
`translates :body_html` in the model.
Sanitizing descriptions before saving a record has a few drawbacks:
1. It makes the application rely on data being safe in the database. If
somehow dangerous data enters the database, the application will be
vulnerable to XSS attacks
2. It makes the code complicated
3. It isn't backwards compatible; if we decide to disallow a certain
HTML tag in the future, we'd need to sanitize existing data.
On the other hand, sanitizing the data in the view means we don't need
to triple-check dangerous HTML has already been stripped when we see the
method `auto_link_already_sanitized_html`, since now every time we use
it we sanitize the text in the same line we call this method.
We could also sanitize the data twice, both when saving to the database
and when displaying values in the view. However, doing so wouldn't make
the application safer, since we sanitize text introduced through
textarea fields but we don't sanitize text introduced through input
fields.
Finally, we could also overwrite the `description` method so it
sanitizes the text. But we're already introducing Globalize which
overwrites that method, and overwriting it again is a bit too confusing
in my humble opinion. It can also lead to hard-to-debug behaviour.
There were some confusing definitions regarding the valuation of budget
investments.
In the controller, `CommentableActions` was included, which includes the
update action.
In the abilities, a valuator was given permission to update an
investment.
However, the action to update an investment didn't work because there is
no route defined to do so.
The ability was defined so valuators could access the "edit" action,
which will not call the "update" action but the "valuate" action. Since
internally "edit" and "update" use the same permission, it worked.
But then we added permission for regular users to update budget
investments, and these permissions were allowing valuators to update
another user's investment.
After this change, everything seems to work properly since we check
authorization in the controller itself instead of using abilities.
Using the `_html` suffix automatically marks texts as HTML safe, so
doing so on sanitized texts is redundant.
Note flash texts are not sanitized the moment they are generated, but
are sanitized when displayed in the view.
We use this method in two different scenarios. In an AJAX request, we
don't want to return every booth if the search is blank. However, in a
normal HTTP GET request, we want to return every record when the search
is empty, as we do everywhere else.
It's possible the behaviour of the AJAX call is unusual, since it
searches all booths, and not just the ones assigned to a poll. If we
changed this behaviour, we could simplify the code and remove the
`quick_search` method.
Some of our team members don't like using `do...end` for scopes, and
some other team members don't like using `{ ... }` for multi-line
blocks, so we've agreed to use class methods instead.
Joining two scopes with `+` does not remove duplicate records. Luckily
now that we've upgraded to Rails 5, we can join scopes using `.or`.
The test was testing for the presence of elements, bud didn't test for
duplicate records. Testing the exact contents of the array revealed this
behaviour.
When `valuator_group` was `nil`, `[valuator_group&.investment_ids]` is
evaluated to `nil`, and so we were adding an extra element to the array.
We could add a `compact` call to the resulting array, but I find it
easier to convert `nil` to an array using `to_a`.
