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.
These factories were only used in one place and they even declared ID
attributes. Using the comment factory with the `commentable` attribute
does the same thing.
The instance variable was being evaluated to `nil`, and the budget was
automatically created by the `set_denormalized_ids` method in the budget
investment class.
This is one of the most strange behaviours in ruby: if a variable
doesn't exist, assigning to itself will return `nil`.
So a line like:
mdmkdfm = ooops if mdmkdfm.respond_to?(:uiqpior)
Surprisingly will not raise any errors: the nonexistent `mdmkdfm`
variable will be evaluated to `nil`, `mdmkdfm.respond_to?(:uiqpior)`
will evaluate to `nil.respond_to?(:uiqpior)`, which will return `false`,
and then the line will be evaluated as `mdmkdfm = ooops if false`, which
will return `nil`.
Maybe in the future Ruby will change this behaviour. We hope CONSUL is
now in better shape if that ever happens :).
The `create_proposal_notification` method won't create a new
notification when a proposal has no supporters, so in the test
`notification3` was actually the same as `notification2`, related to
`proposal2`.
The attribute made sense before we changed it in commit ba1a6b4c. Since
then, all milestones have the same date, so the attribute doesn't affect
the test at all.
The valuation comment doesn't show up in the comment show action because
it's not a child of the parent comment. With a regular comment, the test
passes as well.
However, if we make the valuation comment a child of the parent comment,
it shows up both in the index and show actions. That's because the
method `root_descendants` in the `CommentTree` class doesn't filter
valuation comments. I'm not sure whether it's a bug or the intended
behaviour.
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`.
We were creating the investments with more ballot counts first in every
test, so the tests would pass if we ordered the investments by creation
date instead of ordering them by the number of ballot lines.
While there are other variables in these tests, they're not part of the
setup of the test, and so these ones can be removed while keeping the
code easy to read.
While it could be argued we're hiding the real way we've defined
associations in our models, the tests are so much easier to read when we
don't have so many lines just creating data.
Furthermore, developers who care about vertically aligning the code will
be glad to see some variables disrupting this alignment are now gone.
