In order to leave the page using turbolinks and then going back, we were
clicking on the "Help" page link, but this link doesn't have to be
available on every Consul Democracy installation.
So we're using the link to the homepage instead.
Note that `Capybara.app_host` now returns `nil` by default and that
breaks tests using `lvh.me` or our custom `app_host` method, so we're
setting `Capybara.app_host` to the value it had in earlier versions of
Rails. I also haven't found a way to remove the code to set the
integration session host in relationable tests which I mentioned in
commit ffc14e499.
Also note that we now filter more parameters, and that they match
regular expressions, so filtering `:passw` means we're filtering
`passwd`, `password`, ...
We applied the Capybara/SpecificMatcher in commit f52a86b46. However,
this rule doesn't convert methods finding <a> tags to methods finding
links because <a> tags only count as links when they've got the `href`
attribute. For instance, in the `xss_spec.rb` file we check what happens
when clicking on an anchor tag because we're testing that the `href`
attribute has been removed and so we can't use `click_link`.
So, basically, we can't enable a rule to automatically detect when we're
using `have_css` instead of `have_link`, but we should still do it
because `have_link` adds an extra check which affects accessibility
since it makes sure the tag has the `href` attribute and so it's
recognizable as a link by screen readers.
Note we're excluding a few files:
* Configuration files that weren't generated by us
* Migration files that weren't generated by us
* The Gemfile, since it includes an important comment that must be on
the same line as the gem declaration
* The Budget::Stats class, since the heading statistics are a mess and
having shorter lines would require a lot of refactoring
Having expectations related to database operations in system tests after
the process running the browser has started might result in exceptions
while running our test suite.
For the HashAlignment rule, we're using the default `key` style (keys
are aligned and values aren't) instead of the `table` style (both keys
and values are aligned) because, even if we used both in the
application, we used the `key` style a lot more. Furthermore, the
`table` style looks strange in places where there are both very long and
very short keys and sometimes we weren't even consistent with the
`table` style, aligning some keys without aligning other keys.
Ideally we could align hashes to "either key or table", so developers
can decide whether keeping the symmetry of the code is worth it in a
case-per-case basis, but Rubocop doesn't allow this option.
Since IRB has improved its support for multiline, the main argument
towars using a trailing dot no longer affects most people.
It still affects me, though, since I use Pry :), but I agree
leading dots are more readable, so I'm enabling the rule anyway.
Using a button for interactive elements is better, as explained in
commit 5311daadf.
Since buttons with "type=button" do nothing by default, we no longer
need to call `preventDefault()` when clicking it.
We were using `map_location` in one place and
`location-map-remove-marker` in another one. We usually use dashes in
HTML class names, we don't say "location map" anywhere else.
The page was crashing when at least part of the content of the page had
been translated between the request showing the remote translations
button and the moment people pressed the button.
We were getting a warning with Ruby 2.7 due to the change in the way
keyword arguments are handled in Ruby 3.0.
```
ruby/gems/2.7.0/gems/rspec-support-3.11.0/lib/rspec/support/with_keywords_when_needed.rb:18:
warning: Passing the keyword argument as the last hash parameter is
deprecated
```
As hinted by the warning, this code crashes with Ruby 3.0:
```
ArgumentError:
unknown keyword: :budget_id
```
I'm not sure why this is the case, though, since we were already
explicitely passing a hash first before passing the keyword parameters.
I guess there are some cases in this whole keyword VS hash parameters
incompatibility that I haven't completely understood.
It was added in rubocop-performance 1.13.0. We were already applying it
in most places.
We aren't adding it for performance reasons but in order to make the
code more consistent.
We were using `Setting["url"]` to verify the content belonged to the
application URL, but we can use `root_url` instead.
Note that means we need to include the port when filling in forms in the
tests, since in tests URL helpers like `polymorphic_url` don't include
the port, but a port is automatically added when actually making the
request.
All the code in the `bin/` and the `config/` folders has been generated
running `rake app:update`. The only exception is the code in
`config/application.rb` where we've excluded the engines that Rails 6.0
has added, since we don't use them.
There are a few changes in Active Storage which aren't compatible with
the code we were using until now.
Since the method to assign an attachment in ActiveStorage has changed
and is incompatible with the hack we used to allow assigning `nil`
attachments, and since ActiveStorage now supports assigning `nil`
attachments, we're removing the mentioned hack. This makes the
HasAttachment module redundant, so we're removing it.
Another change in ActiveStorage is files are no longer saved before
saving the `ActiveStorage::Attachment` record. This means we need to
manually upload the file when using direct uploads. We also have to
change the width and height validations we used for images; however,
doing so results in very complex code, and we currently have to write
that code for both images and site customization images.
So, for now, we're just uploading the file before checking its
dimensions. Not ideal, though. We might use active_storage_validations
in the future to fix this issue (when they support a proc/lambda, as
mentioned in commit 600f5c35e).
We also need to update a couple of tests due to a small change in
response headers. Now the content disposition returns something like:
```
attachment; filename="budget_investments.csv"; filename*=UTF-8''budget_investments.csv
```
So we're updating regular expression we use to check the filename.
Finally, Rails 6.0.1 changed the way the host is set in integration
tests [1] and so both `Capybara.app_host` and `Capybara.default_host`
were ignored when generating URLs in the relationable examples. The only
way I've found to make it work is to explicitely assign the host to the
integration session. Rails 6.1 will change this setup again, so maybe
then we can remove this hack.
[1] https://github.com/rails/rails/pull/36283/commits/fe00711e9
We were doing a `mappable.map_location` call in an `expect` which might
result in a database queries. Doing database queries in a test after the
process running the browser has started might result in exceptions while
running our test suite.
There were cases where we clicked the button to submit the form and
immediately we visited a different page.
In the past, we've had similar code produce PG::ProtocolViolation errors
in similar situations. Since we've had these errors a few times in the
nested imageable specs, there's a chance they're related to the absence
of the expectation.
Although I'm not even remotely sure this will fix these issues, at least
we now follow the convention of making expectations after every request.
Note we're changing both the nested imageable and nested documentable
specs. Only the nested imageable would need to be changed because it's
the one where there's a `visit` inside the
`imageable_redirected_to_resource_show_or_navigate_to` method. I'm
changing both for consistency.
Since we were creating a new answer in the form, we weren't getting the
errors associated to the answer the administrator was trying to create,
and so we were skipping the test.
Using the answer which contains the information about validation errors
fixes the issue and so we don't have to skip the tests.
This way the tests won't appear as "pending" when running the test
suite, and so we get rid of a lot of noise in the test results. There
doesn't seem to be a way to call `skip` without the test being marked as
"pending".
Note that in the globalizable tests we need to build a factory before
deciding whether an atribute is required or not (particularly for the
milestone factory, since milestone attributes are required depending on
the presence of other attributes). This isn't possible before we're
inside the test, so we can't add an `if:` condition to the test. So
we're adding the condition inside the test instead. A minor
inconvenience of this method is the test still runs even when the
condition is `false`.
This file only has tests related to tags; if the model doesn't have
tags, we simply wouldn't include `it_behaves_as` in their tests instead
of including it and then skipping it.
We were using this hack in order to allow `File.new` attachments in
tests files. However, we can use the `fixture_file_upload` helper
instead.
Just like it happened with `file_fixture`, this helper method doesn't
work in fixtures, so in this case we're using `Rack::Test::UploadedFile`
instead.
This way we don't have to write `"spec/fixtures/files"` every time.
Note this method isn't included in factories. We could include it like
so:
```
FactoryBot::SyntaxRunner.class_eval do
include ActiveSupport::Testing::FileFixtures
self.file_fixture_path = RSpec.configuration.file_fixture_path
end
```
However, I'm not sure about the possible side effects, and since we only
use attachments in a few factories, there isn't much gain in applying
the monkey-patch.
We were using custom rules because of some issues with Paperclip. These
rules work fine, but since we're already using the file_validators gem,
we might as well simplify the code a little bit.
This fixes a few issues we've had for years.
First, when attaching an image and then sending a form with validation
errors, the image preview would not be rendered when the form was
displayed once again. Now it's rendered as expected.
Second, when attaching an image, removing it, and attaching a new
one, browsers were displaying the image preview of the first one. That's
because Paperclip generated the same URL from both files (as they both
had the same hash data and prefix). Browsers usually cache images and
render the cached image when getting the same URL.
Since now we're storing each image in a different Blob, the images have
different URLs and so the preview of the second one is correctly
displayed.
Finally, when users downloaded a document, they were getting files with
a very long hexadecimal hash as filename. Now they get the original
filename.
This way we fix a bug we mentioned in commit 930bb753c which caused
links to documents to be broken when editing their title because the
title was used to generate the URL of the document.
Note we're still using Paperclip to render cached attachments because
this is the only case where we store files with just Paperclip and not
Active Storage.
With Active Storage, we render attachments just like any other resource,
using `polymorphic_path`. Paperclip included the `url` method in the
model; since the model doesn't have access to the request parameters
(like the host), this was inconvenient because it wasn't possible to
generate absolute URLs with Paperclip.
In order to simplify the code and make it similar to the way we used
Paperclip, we're adding a `variant` method accepting the name of a
variant and returning the variant.
We can use `polymorphic_url` instead of manually setting the domain
every time.
This is a bit of a hack in order to comply with the validation rule
which says related content must start with the URL defined in
`Setting["url"]`.
We were testing the URL of the image changes to `missing.png`, but
actually that's confusing because the image record is now invalid and so
its changes can't be saved. That means that, when rendered in the
browser, the image won't render the `missing.png` image but will try to
render the destroyed one.
If we want to render the `missing.png` image when the attachment has
been destroyed, we need to remove the attachment presence validation or
change the `url` method so it detects when an attachment is missing.
Sometimes tests were hanging indefinitely. Debugging shows that in some
cases it's due to submitting a form before the AJAX request to get
proposals, debates or investments suggestions is finished, since having
an AJAX and a non-AJAX request at the same time when running the test
sometimes leads to unexpected results.
In our case, we were having many timeouts in Github Actions in the
branches where we use both ActiveStorage and Paperclip to store files
(based on pull request 4598). I can reproduce it in those branches
running the following test ("Should show new image after successful
creation with one uploaded file"), although only when my laptop isn't
plugged (!!):
```
rspec './spec/system/proposals_spec.rb[1:33:1:14]'
```
Since we didn't have a proper way to know the AJAX request had finished,
we're adding a `suggest-success` class to the element showing the
suggestions when that happens. Then in the tests we can look for that
class after filling in the title of a proposal, debate or investments.
Just for clarity's sake, we're also adding the `suggest-loading` class
when the suggestions are loading.
In order not to have expectations everywhere about the suggestions,
we're extracting methods to fill in those titles in the tests. Note we
aren't using these methods in the "edit" actions (suggestions are not
showing when editing) or in tests with the `no_js` tag (since
suggestions only work with JavaScript).
Links acting like buttons have a few disadvantages.
First, screen readers will announce them as "links". Screen reader users
usually associate links with "things that get you somewhere" and buttons
with "things that perform an action". So when something like "Delete,
link" is announced, they'll probably think this is a link which will
take them to another page where they can delete a record.
Furthermore, the URL of the link for the "destroy" action might be the
same as the URL for the "show" action (only one is accessed with a
DELETE request and the other one with a GET request). That means screen
readers could announce the link like "Delete, visited link", which is
very confusing.
They also won't work when opening links in a new tab, since opening
links in a new tab always results in a GET request to the URL the link
points to.
Finally, submit buttons work without JavaScript enabled, so they'll work
even if the JavaScript in the page hasn't loaded (for whatever reason).
For all these reasons (and probably many more), using a button to send
forms is IMHO superior to using links.
There's one disadvantage, though. Using `button_to` we create a <form>
tag, which means we'll generate invalid HTML if the table is inside
another form. If we run into this issue, we need to use `button_tag`
with a `form` attribute and then generate a form somewhere else inside
the HTML (with `content_for`).
Note we're using `button_to` with a block so it generates a <button>
tag. Using it in a different way the text would result in an <input />
tag, and input elements can't have pseudocontent added via CSS.
The following code could be a starting point to use the `button_tag`
with a `form` attribute. One advantage of this approach is screen
readers wouldn't announce "leaving form" while navigating through these
buttons. However, it doesn't work in Internet Explorer.
```
ERB:
<% content_for(:hidden_content, form_tag(path, form_options) {}) %>
<%= button_tag text, button_options %>
Ruby:
def form_id
path.gsub("/", "_")
end
def form_options
{ id: form_id, method: options[:method] }
end
def button_options
html_options.except(:method).merge(form: form_id)
end
Layout:
<%= content_for :hidden_content %> # Right before the `</body>`
```
We were using helper methods inside the model; we might as well include
them in the model and use them from anywhere else.
Note we're using a different logic for images and documents methods.
That's because for images the logic was defined in the helper methods,
but for documents the logic is defined in the Documentable concern. In
the past, different documentable classes allowed different content
types, while imageable classes have always allowed the same content
types.
I'm not sure which method is better; for now, I'm leaving it the way it
was (except for the fact that we're removing the helper methods).
The same way it's done for images.
We were converting the number of megabytes to bytes and then converting
it to megabytes again. Instead, we can leave it as it is and only
convert it to bytes when necessary (only one place).
`dalli_store` is deprecated since dalli 2.7.11.
We can now enable cache_versioning. We didn't enable it when upgrading
to Rails 5.2 because of possible incompatibility with `dalli_store` [1],
even though apparently some the issues were fixed in dalli 2.7.9 and
dalli 2.7.10 [2].
Since using cache versioning makes cache expiration more efficient, and
I'm not sure whether the options we were passing to the dalli store are
valid with memcache store (documentation here is a bit lacking), I'm
just removing the option we used to double the default cache size on
production.
[1] https://www.schneems.com/2018/10/17/cache-invalidation-complexity-rails-52-and-dalli-cache-store
[2] https://github.com/petergoldstein/dalli/blob/master/History.md
Since version 2.0 introduced many breaking changes, we're upgrading to
it first.
The changes have been done by installing the rubocop-faker gem and
running:
```
rubocop \
--require rubocop-faker \
--only Faker/DeprecatedArguments \
--auto-correct
```
We introduced a bug in commit acbd1b023.
When editing a record and removing an existing image, we don't remove
the HTML fields associated with that image but simply hide them, and
then we add fields to create a new image when clicking on "Add image".
This is standard cocoon behavior. However, since in the case of images
there's a `has_one` relation, cocoon doesn't add unique identifiers to
the new fields, generating duplicate IDs, which is invalid HTML.
Since there's a duplicate file input ID, clicking on the "Choose image"
label we aren't clicking on the new input but on the old one. This means
we aren't correctly attaching an image. The tests passed because
Capybara uses the equivalent of a keyboard to select the field, and in
this case everything worked properly.
So we need to delete the existing elements before inserting new ones.
We're adding a test to check there aren't duplicate IDs.
