By using real XML responses developers will be able to understand better
how the integration works (the data flow), and the correspondency between
`remote_census` settings and their place at a real XML response.
As `stubbed_responses` methods were removed from the model layer now the
stubbing part should be managed from the test environment code so also
added a new helper module `RemoteCensusSetup` that can be used anywhere
where we need to call the web service.
Co-Authored-By: Javi Martín <javim@elretirao.net>
Since the interface to select this date uses the classic multi-field
interface for day, month and year, we're transforming it into a date in
the Office::Residence initializer.
However, the factory to build an office residence does not assign the
paremeters in the initializer but using the `date_of_birth=` method, so
when doing so we need to use a date instead of a string.
By simplyfing the responses the configuration for specs can be simpler too.
We're also using more generic terms instead of the ones used in Madrid's
Census API.
Co-Authored-By: Javi Martín <javim@elretirao.net>
Otherwise the variants returned for document_type="1" and
document_number="" will be
`["0", "00", "000", "0000", "00000", "000000", "0000000", "00000000"]`
which seems to be useless.
Probably this case is not real for production environments where those
arguments will always be fullfilled but seems to be interesting for
testing environment where this method is being called when those
paremeters where empty.
Ruby 2.6 introduces `Enumerable#filter` as an alias to
`Enumerable#select`, and so our Filterable.filter method will not work
with Ruby 2.6.
So we're renaming the method to `filter_by`, which is similar to
`find_by`. We could also change the `filter` method so if a block is
given it delegates to `Enumerable#filter`, the same way ActiveRecord
handles the `select` method, but IMHO this is easier to follow.
Now we can use the checkbox label text directly as locator at spec,
also default web_sections are created before every spec by loading
`db/seeds.rb` file so we were duplicating "Proposals" WebSection.
We were allowing users to check/uncheck the "Visible to valuators"
checkbox even when the budget is finished and so the investments cannot
be edited. So users were still able to check/uncheck this attribute, but
the server was silently rejecting these changes.
We've considered removing the column in this case but decided to keep it
since users can already control which columns they'd like to display.
We added the background role to the production and preproduction
environments in commit d0b0782c4, but forgot to add it to the staging
environment as well.
After upgrading to Turbolinks 5, redirects are followed on AJAX
requests, so we were accidentally redirecting the user after they mark
an investment as visible to valuators.
There was already a system spec failing due to this issue ("Admin budget
investments Mark as visible to valuators Keeps the valuation tags");
however, it only failed in some cases, so we're adding additional tests.
Ideally we would write a system test to check what happens when users
click on the checkbox. However, from the user's point of view, nothing
happens when they do so, and so testing it is hard. There's a usability
issue here (no feedback is provided to the user indicating the
investment is actually updated when they click on the checkbox and so
they might look for a button to send the form), which also results in a
feature which is difficult to test.
So we're writing two tests instead: one checking the controller does not
redirect when using a JSON request, and one checking the form submits a
JSON request.
I've chosen JSON over AJAX because usually requests to the update action
come from the edit form, and we might change the edit form to send an
AJAX request (and, in this case, Turbolinks would handle the redirect as
mentioned above).
Another option would be to send an AJAX request to a different action,
like it's done for the toggle selection action. I don't have a strong
preference for either option, so I'm leaving it the way it was. At some
point we should change the user interface, though; right now in the same
row there are two actions doing basically the same thing (toggling
valuator visibility and toggling selection) but with very different user
interfaces (one is a checkbox and the other one a link changing its
style depending on the state), resulting in a confusing interface.
We removed it in commit d639cd58 because it recommended using `uniq`
where `distinct` was more appropriate. This has been fixed in
rubocop-rails 2.6.0.
This is something we should do everywhere because concurrent requests
might bypass Rails uniqueness validations. However, since there are
places where we aren't applying this rule and adding a unique index
means we also need to destroy any hypothetical duplicate records, it's
something we aren't going to solve right now. Therefore we use the
"refactor" severity so existing offenses don't get in our way.
In any case, we're adding the rule so we don't make the same mistake in
the future.
This rule was added in rubocop-rspec 1.39.0. The `visible: false` option
is equivalent to `visible: :all`, but we generally use it meaning
`visible: :hidden` for positive expectations and `visible: :all` for
negative expectations.
The only exceptations are tests where we count the number of map icons
present. I'm assuming in this case we care about the number of map icons
independently on whether they are currently shown in the map, so I'm
keeping the `visible: :all` behavior in this case.
By default, Capybara only finds visible elements, so adding the
`visible: true` option is usually redundant.
We were using it sometimes to make it an obvious contrast with another
test using `visible: false`. However, from the user's perspective, we
don't care whether the element has been removed from the DOM or has been
hidden, so we can just test that the visible selector can't be found.
Besides, using `visible: false` means the test will also pass if the
element is present and visible. However, we want the test to fail if the
element is visible. That's why a couple of JavaScript-dependant tests
were passing even when JavaScript was disabled.
These tests were supposed to check the link to vote is hidden when users
don't have permission to vote. However, they aren't testing that, since
the `visible: false` option also matches visible elements. The links are
actually considered visible since they're displayed by the browser;
there's just another element on top of them.
Using `obscured: true` instead of `visible: false` solves the issue.
However, while the `obscured` option is true when the element is hidden
by another element, it's also true when the element is not currently
visible in the browser window, so in some cases we need to scroll so the
condition is effective.
This rule was added in Rubocop 0.91.0. A similar rule named
LeakyConstantDeclaration was added in rubocop-rspec 1.34.0.
Note using the FILENAMES constant did not result in an offense using the
ConstantDefinitionInBlock rule but did result in an offense using the
LeakyConstantDeclaration rule. I've simplified the code to get rid of
the constant; not sure why we were adding a constant with `||=` in the
middle of a spec.
This rule was added in Rubocop 0.89.0. However, there are some false
positives when we don't use interpolation but simply concatenate in
order to avoid long lines. Even if there weren't false positives, there
are places where we concatenate to emphasize the point that we're adding
a certain character to a text.
We might reconsider this rule in the future, since we generally prefer
interpolation over concatenation.
The original devise_security_extension gem has not been maintained for
years. Its last release was version 0.10.0, and wasn't compatible with
Rails 5, and so we were using its master branch.
Since the gem was unmaintained, it was forked as devise-security and the
aforementioned master branch was released as version 0.10.1. This
version wasn't published in Rubygems, though, so we're now using the
first version that was published in Rubygems and had a release
announment [1].
Dependabot will probably open a pull request to upgrade to the latest
version, but for now I'm trying to keep the devise-security gem as
similar as the version we were using to make sure they're compatible,
particularly considering we're monkey-patching some of the modules
provided by this gem.
[1] https://github.com/devise-security/devise-security/releases/tag/v0.11.1
