When a poll is created, and any of the questions for that poll doesn't
have any answer created, the following exception was raised when
trying to see the results:
Failure/Error: question_answers.max_by {|answer| answer.total_votes }.id
ActionView::Template::Error:
undefined method `id' for nil:NilClass
./app/models/poll/question.rb:66:in `most_voted_answer_id'
Unfortunately this feature wasn't properly reviewed and tested, and it
had many bugs, some of them critical and hard to fix, like validations
being skipped in concurrent requests.
So we're removing it before releasing version 1.1. We might add it back
in the future if we manage to solve the critical issues.
This commit reverts commit 836f9ba7.
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.
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
```
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.
A bit of history in order to understand this change.
A year ago we introduced Hound so it would review our pull requests and
warn contributors when they weren't following our coding style.
However, back then we had many rules we hadn't reviewed yet, and we
weren't sure we wanted to ask contributors to follow them.
So we decided to split these files: .rubocop_basic.yml would contain
rules we had already agreed on and wanted contributors to respect, and
.rubocop.yml would contain rules we still had to review.
Now we've finally gone through all these rules. We've removed some of
them, kept some of them, added new ones, and applied them.
Now all rules with a severity level of "convention" or higher return no
offenses. The rules with "severity: refactor" return some offenses,
though:
* Metrics/LineLenght can only be properly accomplished when we define
better multi-line indentation standards, while in some cases long lines
indicate we need to refactor the code
* Rails/DynamicFindBy returns a few false positives
* Rails/HasManyOrHasOneDependent should by all means be implemented,
although it will not be a trivial task
* Rails/SaveBang returns a few false positives and there are a couple of
places where we skip it on purpose
There are also rules excluding some files:
* Rails/InverseOf returns a false positive
* Rails/OutputSafety is ignored on purpose when we add auto-links to a
text we trust
* RSpec/InstanceVariable returns false positives in two files
Other than that, everything works as expected.
This is actually a hack. We want Hound to warn us about this rule;
however, we don't want to be notified about our many existing offenses.
Ideally we would add the `dependent` option to all existing models.
However, this is extra tricky because adding this option would change
existing behavior.
As mentioned in commit 9d566a22, I've kept these performance cops but
not for performance reasons but because they make the code easier to
read.
As for the security cops, we've never had problems with any of them, but
since we recently added an `eval` call by accident, there's a chance we
could do the same with JSONLoad and YAMLLoad.
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.
