Rails 5.2 is raising a warning in some places:
DEPRECATION WARNING: Dangerous query method (method whose arguments are
used as raw SQL) called with non-attribute argument(s). 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().
IMHO this warning is simply wrong, since we're using known PostgreSQL
functions like LOWER() or RANDOM(). AFAIK this code works without warnings
in Rails 6.0 [1][2]
However, since the warning is annoying, we need to take measures so our
logs are clean.
[1] https://github.com/rails/rails/commit/6c82b6c99d
[2] https://github.com/rails/rails/commit/64d8c54e16
When skipping verification, we cannot apply the validation rule saying
the document number and document type must be unique, because they'll be
`nil` in many cases. So we were skipping the rule, but that makes it
possible for the same user to vote several times (for instance, once in
a booth and once via web).
So we're changing the scope of the uniqueness rule: instead of being
unique per document number, voters are unique per user. The reason we
made them unique per document number was that back in commit 900563e3
(when we added the rule), we hadn't added the relation between users and
poll voters yet.
Up until now, we were assuming the voter was valid, but were not raising
an exception if it wasn't. And in the user interface everything seemed
to be working properly.
We were having this issue when skipping verification, when there could
be voters without a document number, which would be considered invalid.
Raising an exception when failing to save the voter and making sure the
answer and the voter are saved inside a transaction solves the problem.
This method is deprecated in Rails 5.1 because its behavior will be
different in `before` and `after` callbacks.
Here we're replacing the deprecated `attribute_changed?` and
`attribute_was` with `will_save_change_to_attribute?` and
`attribute_in_database` during `before_save` callbacks.
https://github.com/rails/rails/pull/32835/
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.
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
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.
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.
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.
Avoid to raise an exception `Module::DelegationError' when trying to
show the name and/or email of a poll officer whose user account has
been deleted.
We'll show a message "User deleted" and "Email deleted" instead.
As mentioned in the Rails console:
DEPRECATION WARNING: Passing a class to the `class_name` is deprecated
and will raise an ArgumentError in Rails 5.2. It eagerloads more classes
than necessary and potentially creates circular dependencies. Please
pass the class name as a string.
Due to technical issues, sometimes users voted in booths and their vote
couldn't be added to the database. So we're including them in the users
with no demographic data.
Using SQL's `select` instead of converting the records to a ruby array
increases performance dramatically when there are thousands of records.
For a poll with 200000 voters, calculating stats took more than 7
minutes, and now it takes less than 2 minutes.
These methods are only used while stats are being generated; once stats
are generated, they aren't used anymore. So there's no need to store
them using the Dalli cache.
Furthermore, there are polls (and even budgets) with hundreds of
thousands of participants. Calculating stats for them takes a very long
time because we can't store all those records in the Dalli cache.
However, since these records aren't used once the stats are generated,
we can store them in an instance variable while we generate the stats,
speeding up the process.
We need a way to manually expire the cache for a budget or poll without
expiring the cache of every budget or poll.
Using the `updated_at` column would be dangerous because most of the
times we update a budget or a poll, we don't need to regenerate their
stats.
We've considered adding a `stats_updated_at` column to each of these
tables. However, in that case we would also need to add a similar column
in the future to every process type whose stats we want to generate.
