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.
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.
So now we test in depth at the model level, and can be a bit more
relaxed about integration tests for translations.
Note we're defining some extra factories to make sure all translatable
attributes with presence validation rules are mandatory. This way we can
simplify the way we obtain required fields, using `required_attribute?`.
Otherwise, fields having an `unless` condition in their presence
validation rules would count as mandatory even when they're not.
This is a mistake I made in commit f2ef27d3. Back then I thought we
needed to keep Globalize.locale and I18n.locale in sync, but the truth
is it automatically happens when setting Globalize.locale to nil.
So now we can use I18n.with_locale (at least in the tests) and forget
about Globalize, which will make it easier to switch to Mobility in the
future.
We don't need `before` blocks because we've removed the `after` blocks,
and we don't need to define available locales because we already do so
in the test environment file.
Now factories define default headings for investments, so there's no
need to create a group and a heading to create an investment.
Likewise, in order to create a heading it isn't necessary to specify a
group anymore; specifying the budget is enough.
It's possible that there are more similar cases we haven't simplified
yet; I'm only addressing the obvious ones.
We already configure `I18n.locale` and we reset Globalize's fallbacks
before every test.
On the other hand, RSpec automatically resets anything which is stub
with `allow`, so there's no need to use `and_call_original` in an
`after` block.
Settings are stored in the database, and so any changes to the settings
done during the tests are automatically rolled back between one test and
the next one.
There were also a few places where we weren't using an `after` block but
changing the setting at the end of the test.
Investments can be reclassified to a different heading during the participatory budget process.
Whilst we are recording this change of heading in the `previous_heading_id` attribute, we are only keeping the _last_ heading. If there are multiple reclassifications we lose this chain of reclassifications.
In this commit we are adding an `original_heading_id` attribute, that will only be set once, when creating the investment, and will not get lost with multiple reclassificaitons of an investment.
The tests in the `spec/lib/graphql_spec.rb` failed sometimes because
creating a record with a tag list of ["health"] when both "health" and
"Health" tags exist might assign either one of them. These tests usually
pass because we create two records and just by chance usually one of the
records gets one tag and the other one gets the other tag. However, the
test was written as if we expected the first record to get the first tag
and the second record to get the second tag, while very often the tests
were passing because the first record got the second tag and the second
record got the first tag. And when both records get the same tag, the
tests fail.
So I've changed these tests to tags are assigned directly and, since we
want to test the `tag_list` method, I've also added some tests to the
Tag model, which reflect the current behaviour: a random tag is assigned
when several tags with the same case-insensitive name exist.
Another option to assign the right tag to the record we're creating
would be to add `ActsAsTaggableOn.strict_case_match = true` to an
initializer. However, that would also create new tags on the database
when we accidentally assign a tag like "hEaLth" (like in the test we add
in this commit). Ideally we would have a strict case match for existing
tags and a non-strict case match for new tags, but I haven't found a way
to do it.
