Note that Rails 7.1 changes `find_or_create_by!` so it calls
`create_or_find_by!` when no record is found, meaning we'll rarely get
`RecordNotUnique` exceptions when using this method during a race
condition.
Adding this index means we need to remove the uniqueness validation.
According to the `create_or_find_by` documentation [1]:
> Columns with unique database constraints should not have uniqueness
> validations defined, otherwise create will fail due to validation
> errors and find_by will never be called.
We're adding a test that checks what happens when using
`create_or_find_by!`.
Note that we're creating voters combining `create_with` with
`find_or_create_by!`. Using `find_or_create_by!(...)` with all
attributes (including non-key ones like `origin`) fails when a voter
already exists with different values, e.g. an existing `origin: "web"`
and an incoming `"booth"`. In this situation the existing record is not
matched and the unique index raises an exception.
`create_with(...).find_or_create_by!(user: ..., poll: ...)` searches by
the unique key only and applies the extra attributes only on creation.
Existing voters are returned unchanged, which is the intended behavior.
For the `take_votes_from` method, we're handling a (highly unlikely, but
theoretically possible) scenario where a user votes at the same time as
taking voters from another user. For that, we're doing something similar
to what `create_or_find_by!` does: we're updating the `user_id` column
inside a new transaction (using a new transactions avoids a
`PG::InFailedSqlTransaction` exception when there are duplicate
records), and deleting the existing voter when we get a
`RecordNotUnique` exception.
On `Poll::WebVote` we're simply raising an exception when there's
already a user who's voted via booth, because the `Poll::WebVote#update`
method should never be called in this case.
We still need to use `with_lock` in `Poll::WebVote`, but not due to
duplicate voters (`find_or_create_by!` method will now handle the unique
record scenario, even in the case of simultaneous transactions), but
because we use a uniqueness validation in `Poll::Answer`; this
validation would cause an error in simultaneous transactions.
[1] https://api.rubyonrails.org/v7.1/classes/ActiveRecord/Relation.html#method-i-create_or_find_by
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.
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.
These variables can be considered a block, and so removing them doesn't
make the test much harder to undestand.
Sometimes these variables formed the setup, sometimes they formed an
isolated part of the setup, and sometimes they were the part of the test
that made the test different from other tests.
In the scenario where we want to test scopes and use `match_array`, we
usually declare variables we never use, which raises a warning in the
Ruby interpreter (since the main cause for an unused variable is a
typo).
So I've decided to just split the tests into cases where every record is
returned and cases were no records are returned, just like we do in
other places.
There are several other options we've considered:
1. Don't declare unused variables, but declare the ones we use
2. Prefix unused variables with un underscore
3. Declare just one variable being an array containing all elements, and
access the elements using Array#[]
4. Don't declare any variables, and compare results against attributes
such as titles
None of these options was met with enthusiasm.
Unless we're using the booth assignment for something else in the test,
when creating a voter from booth, specifying the poll and the booth is
enough to create a booth assignment.
