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.
We can change the code a bit so the useless assignment is either part of
the setup (where only another variable was present) or isolated in the
"action" part of the test.
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.
The limit parameter wasn't specified in the test but in the default
value in the database, making the test hard to read.
Since now we've moved the other processes to separate tests, now we can
create four processes using `times` and keep the test simple.
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.
Assigning a variable to each budget we declare results in useless
assignments. We could just delete the three useless assignments and
leave the fourth one, but I find the code easier to read if we use the
name of the budgets to differenciate between them. This way we also keep
the code vertically aligned.
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.
We were only using it in a few places.
I've left the current `final: true` statement in a few places where
using a trait would break vertical alignment, just in case.
Note we usually cannot make it simple because officer assignments are
usually assigned to both a poll and a booth, and on a certain date.
However, in the few cases where the booth nor the date don't matter, we
can make the code a bit easier to read.
While in theory we wouldn't need to use the `transient` nor the
`after(:create)` because there's already a `has_many :through`
association with followers, Factory Bot / ActiveRecord don't
automatically associate the followable, resulting in an invalid record
exception.
When we already have tests checking which records are included, in the
tests testing records are not included we can just generate records
which will not be included and test against an empty array.
We were already using this approach in some tests. This way we also
avoid useless assignments.
We're using `eq` and `match_array` in most places, but there were a few
places where we were still checking each element is included in the
array. This is a bit dangerous, because the array could have duplicate
elements, and we wouldn't detect them with `include`.
The test or the draft phase legislation filter was using an entirely
different set of records, but was still creating the records used in the
open and past filter as well.
This made it hard to test the filter, since it returned records from
both sets.
Grouping the past and open filters together guarantees their records
won't be created in the phase draft test, and so we can test the exact
contents of the array.
We were using `to_sentence` to check the order, while it's easier to
just check the exact contents of the array.
Furthermore, using `to_sentence` checked the order of the array, so it
was a potentially flaky spec since the method doesn't specify an order
and PostgreSQL doesn't guarantee the order of the records.
The group is automatically assigned when we assign the heading. The
budget isn't needed either, except for a special case related to the
reason to be rejected.
These tests are only checking which proposals are not included in the
recommendations, so we don't need to sort the included ones, just like
we don't use the cached votes up attribute in the tests preceeding these
ones.
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`.
