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.
This data is obsolete since commit 9d1ca3bf.
Note we should have probably moved the test to the model, since now the
`to_csv` method isn't tested anywhwere, and we should probably add a
test in the future. The code now uses the new DownloadSetting model,
though, making it hard to test.
These feature tests were taking too long, we can't run them for every
single model.
I'm taking the approach of using one different model for each test, but
in theory only using a few models covering every possible scenario
would be enough.
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.
These settings are enabled by default.
It could be argued explicitely enabling the features makes tests more
consistent, because they'll work if we change the default setting. It
could also be argued that it makes tests more expressive because it
makes the reader realize certain things will only work if a setting is
enabled.
However, we were only doing so in a few tests. The truth is, thousands
of our tests depend on certain features being enabled. So IMHO we should
be consistent and either set them on every test, or not at all. I'm
choosing the latter option for simplicity.
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.
When creating a budget investment with an unverified manager (for
example, a manager who isn't part of the local census), there's a
request to `Budgets::InvestmentsController#suggest`. Since the manager
isn't verified, suggestions can't be obtained.
There are serveral ways to fix this problem:
* Add a `suggest` action to Management::Budgets::InvestmentsController,
doing the same thing the main `suggest` action does.
* Give unverified users permission to access investment suggestions
* Give managers permission to access investment suggestions
I've chosen the last one because I thought it was simple and only
changed existing behaviour for managers, but any other solution would be
as valid. I haven't added the `phase: "accepting"` condition to keep it
simple, since a read-only action like this one in the management portal
isn't gonna create security risks.
While the test should work without this assertion, the intention of the
test is to check what happens if a user selects and investment and, once
it's selected, clicks the "next" link.
When we were inserting a row or replacing an existing one (just like we
do when we click the link to select an investment), we were entering a
row containing all columns, and all of them were displayed even if they
had been excluded using the column selector.
This caused the table to move in a strange way, which sometimes made the
investment selection tests fail.
This way we can reduce the number of locales used in the test
environment as well, while still testing every possible scenario for
remote translations.
We accidentally removed the `count` option in commit 55fb14ac, which
made the translation return a hash.
The test is a bit hacky, which makes me think changing the user
interface would probably be a better solution.
In JavaScript, when there isn't a `break` or `return` statement inside a
`switch` case, the next case will be executed as well.
That wasn't a problem here because CoffeeScript automatically inserts a
`return` statement in this specific situation. However, since we don't
want to return the result of the `hide()` operation, it might be easy to
accidentally remove the `return` statement, causing the code to break.
I've added a test for the scenario where neither `break` nor `return`
statements are present, so we don't run into this error.
