Having exceptions is better than having silent bugs.
There are a few methods I've kept the same way they were.
The `RelatedContentScore#score_with_opposite` method is a bit peculiar:
it creates scores for both itself and the opposite related content,
which means the opposite related content will try to create the same
scores as well.
We've already got a test to check `Budget::Ballot#add_investment` when
creating a line fails ("Edge case voting a non-elegible investment").
Finally, the method `User#send_oauth_confirmation_instructions` doesn't
update the record when the email address isn't already present, leading
to the test "Try to register with the email of an already existing user,
when an unconfirmed email was provided by oauth" fo fail if we raise an
exception for an invalid user. That's because updating a user's email
doesn't update the database automatically, but instead a confirmation
email is sent.
There are also a few false positives for classes which don't have bang
methods (like the GraphQL classes) or destroying attachments.
For these reasons, I'm adding the rule with a "Refactor" severity,
meaning it's a rule we can break if necessary.
This one is a bit different than our usual scenario, since we create
three annotations and we only use two of them in the specs (because we
visit the path to that annotation). So there are probably better options
than the combination of `let!` and `before` I've chosen.
Having two questions, each of them with two comments, made the code hard
to follow.
Grouping the comments inside the block creating the questions makes it
easier to know which comment belongs to which question, even if the code
is still not 100% readable.
We also remove instance variables, which by the way used the same
variable name for two different things.
We couldn't declare them inside the block because they would be
considered local variables and its value would be lost when the block
was finished. So we were using instance variables instead.
However, with instance variables we don't get any warnings when we
misspell their names. We can avoid them by declaring the local variables
before the block starts.
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.
We're getting a failure on Travis in one of these tests. Debugging shows
the AJAX request rendering the first page (after clicking the "Previous"
link) takes too long and sometimes it exceeds Capybara's timeout.
After running the test thousands of times, the only way I've found to
clearly reduce the number of times the test fails is to reduce the
number of records shown on the first page. Other experiments, like
adding an `includes(:author)` to the query getting the proposals in the
controller, or adding `author: user` to the `create_list` part of the
test (so only one author needs to be fetched when rendering the
proposals) show inconsistent results regarding performance.
Note we still need at least 10 proposals for the test for several users,
to guarantee two users will never get the same records during the test
(or at least the probability they get the same records is one in
millions).
The `type: :feature` is automatically detected by RSpec because these
tests are inside the `spec/features` folder. Using `feature` re-adds a
`type: :feature` to these files, which will result in a conflict when we
upgrade to Rails 5.1's system tests.
Because of this change, we also need to change `background` to `before`
or else these tests will fail.
We're also adding a rubocop rule so we dont' accidentally add these
keywords again.
The `type: :feature` is automatically detected by RSpec because these
tests are inside the `spec/features` folder. Using `feature` re-adds a
`type: :feature` to these files, which will result in a conflict when we
upgrade to Rails 5.1's system tests.
Because of this change, we also need to change `background` to `before`
or else these tests will fail.
Eventhough some of us sentimentals still like the syntax `to_not` the current trend is to move to the new syntax `not_to`.
In this commit we are updating the references of expectations that used `to_not` to `not_to`.
We make the tests considerably faster, we make them more robust against
changes in the number of records shown per page, and we generate enough
records so the chance of randomly getting the same results twice in a
row is extremely low.
When we were visiting a page showing the content of a record which uses
globalize and our locale was the default one and there was no
translation for the default locale, the application was crashing in some
places because there are no fallbacks for the default locale.
For example, when visiting a legislation process, the line with
`CGI.escape(title)` was crashing because `title` was `nil` for the
default locale.
We've decided to solve this issue by using any available translations as
globalize fallbacks instead of showing a 404 error or a translation
missing error because these solutions would (we thinkg) either require
modifying many places in the application or making the translatable
logic even more complex.
Initially we tried to add this solution to an initializer, but it must
be called after initializing the application so I18n.fallbacks[locale]
gets the value defined in config.i18n.fallbacks.
Also note the line:
fallbacks[locale] = I18n.fallbacks[locale] + I18n.available_locales
Doesn't mention `I18n.default_locale` because the method
`I18n.fallbacks[locale]` automatically adds the default locale.
