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.
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.
We barely use this trait. In the votation type spec we're probably using
it wrong, and in the answer spec we assume one of the answers is going
to be "Yes".
The name `yes_no` is more expressive, since it makes it clear what the
answers are.
These specs were added before we disabled featured proposals by default.
After that, they were passing, but they were not testing the scenario
they were supposed to test.
