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.
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 `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.
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.
Before we disabled featured proposals by default, there were many tests
creating them because they were needed in order to create non-featured
proposals.
But now these tests don't need to create featured proposals anymore.
We had a case where we created 5 extra records in pagination and checked
2 records were present, because the other 3 were automatically
considered featured proposals.
Explicitely creating featured proposals let us create 2 extra records
and check 2 records are present, which is far more intuitive.
The "name" attribute is automatically generated by the budget heading
factory. And the "price" attribute is out of context and not needed
since this test doesn't create investments.
One test was testing regular users can't access results, and another one
was testing neither regular users nor managers can. So the second test
can just test the admin scenario, and we're still covering everything.
The factory creating assignments automatically assigns a poll to it, so
we don't use the poll for anything else, there's no need to explicitely
create it.
