While testing for accessibility issues (in a development branch), we're
removing Turbolinks and monkey-patching the behavior of the `click_link`
method to check the page for accessibility issues after each request.
However, we were getting false positives when clicking links that act
like buttons.
So, for the reasons mentioned in commit 5311daadf, we're replacing the
link to delete a document with a button.
The test "request a translation of an already translated text" was
failing sometimes on our CI since August 29, maybe due to a change in
GitHub Actions since the test had been passing for a year and a half and
we didn't change any code around that time (we were updating the
documentation). While the root cause is unknown, debugging shows that
sometimes (usually the first time this test is executed on our CI, and
only the first time, since running it 600 tests in a row also resulted
in only one failure) the request done by clicking on "Traducir página"
is done with a user session where the locale is in English.
This doesn't make much sense, since both user sessions are already in
Spanish (and we had either explicit or implicit expectations to confirm
that), and debugging shows that the session is indeed in Spanish during
the previous request.
In any case, we're solving it by never using English during the test,
since it wasn't necessary; it was only done that way because all the
tests on this file used the language selector to get to the Spanish
pages. We're simplifying some of the other tests the same way.
The test failure was:
```
Failure/Error: expect(page).to have_content "Se han solicitado
correctamente las traducciones"
expected to find text "Se han solicitado correctamente las traducciones"
in
"Idioma: \n
\nEnglish\nDeutsch\nEspañol\nFrançais\nNederlands\nPortuguês
brasileiro\n中文\n
Entrar\nRegistrarse\nDebates\nPropuestas\nVotaciones\nLegislación
colaborativa\nPresupuestos participativos\nODS\nAyuda\n×\nTranslations
have been correctly requested.\nPropuestas más activas\nAhora mismo no
hay propuestas\nDebates más activos\nndfrrqufrp\nVer todos los
debates\nProcesos abiertos\nAhora mismo no hay procesos
abiertos\nGobierno abierto\nEste portal usa la aplicación CONSUL
DEMOCRACY que es software de código abierto.\nParticipación\nDecide cómo
debe ser la ciudad que quieres.\nCONSUL DEMOCRACY, 2024 Política de
privacidad Condiciones de uso Accesibilidad"
```
Note that most of the text is in Spanish (as expected) but the flash
message itself is in English.
In the past, having this kind of expectations after the process running
the browser has started has resulted in flaky issues with the database
connection.
In one case, we're removing the test because there are controller tests
covering the same scenario and a system test checking what happens from
the user perspective.
In the other case, we're replacing the expectations with expectations
from the user's point of view.
We were using the same setup in these tests, and we were only changing
the expectations or adding an extra step.
Note we're also using `refresh` to simplify the code and because we were
using `select "Español", from: "Idioma:"` when that language was already
selected.
After changing the language, we were checking that certain content isn't
there.
However, the content wasn't there before changing the language either,
so the test will pass even if the request to change the language hasn't
finished.
Although this is probably OK because we aren't changing the language
using an AJAX request, and so Capybara will correctly wait until the
request is finished before finishing the test, confirming that the page
has changed after a request is something we try to do in every test.
Having a class named `Poll::Question::Answer` and another class named
`Poll::Answer` was so confusing that no developer working on the project
has ever been capable of remembering which is which for more than a few
seconds.
Furthermore, we're planning to add open answers to polls, and we might
add a reference from the `poll_answers` table to the
`poll_question_answers` table to property differentiate between open
answers and closed answers. Having yet another thing named answer would
be more than what our brains can handle (we know it because we did this
once in a prototype).
So we're renaming `Poll::Question::Answer` to `Poll::Question::Option`.
Hopefully that'll make it easier to remember. The name is also (more or
less) consistent with the `Legislation::QuestionOption` class, which is
similar.
We aren't changing the table or columns names for now in order to avoid
possible issues when upgrading (old code running with the new database
tables/columns after running the migrations but before deployment has
finished, for instance). We might do it in the future.
I've tried not to change the internationalization keys either so
existing translations would still be valid. However, since we have to
change the keys in `activerecord.yml` so methods like
`human_attribute_name` keep working, I'm also changing them in places
where similar keys were used (like `poll_question_answer` or
`poll/question/answer`).
Note that it isn't clear whether we should use `option` or
`question_option` in some cases. In order to keep things simple, we're
using `option` where we were using `answer` and `question_option` where
we were using `question_answer`.
Also note we're adding tests for the admin menu component, since at
first I forgot to change the `answers` reference there and all tests
passed.
This rule was added in rubocop-capybara 2.19.0. We were following it
about 85% of the time.
Now we won't have to check both have_css and have_selector when
searching the code.
Quoting usability experts Jakob Nielsen and Anna Kaley [1]:
> [Opening PDF files in new tabs] is problematic, because it assumes
> users will always do the exact same things with certain file formats,
> which isn’t always the case.
There are many examples of this situation. For example, some people
(myself included) configure their browser so it downloads PDF files
instead of opening them in the browser. In this situation, a new tab is
opened, a blank page is displayed, the file is downloaded, and then
either the tab is closed or the blank page needs to be manually closed.
The end result is really annoying.
Other situations include people who use a mobile phone browser, where
navigating through tabs is generally much harder than doing so on a
desktop browser.
But IMHO the most important point is: every browser already provides a
way to open "regular" links in a new tab, so people can choose what to
do, but if we decide to open the link in a new tab, we take control away
from them, and people who'd like to open the link in the same tab might
feel frustrated.
In these cases, the links either say "download" or include the word
"PDF", so people know in advance that they're going to download/open a
PDF file, and so we're giving them information and, by removing the
`target` attribute, we're giving them control over their browser so they
can choose what's convenient for them.
[1] https://www.nngroup.com/articles/new-browser-windows-and-tabs
We were displaying documents in five places, and in five different ways.
Sometimes with the metadata in parenthesis after the title, sometimes
with the metadata below the title, sometimes without metadata, sometimes
with an icon in front of the document, and sometimes with a separate
link to download the file.
So we're now displaying the same thing everywhere. Not sure whether this
is the best solution, but at least it's consistent.
We aren't unifying the way we display a list of documents, though, since
different sections look pretty different and I'm not sure whether the
same style would look well everywhere.
Note that we're renaming the `document` HTML class in the documents
table to `document-row` so the styles for the `document` class don't
apply here.
While in unit tests it's great to have different tests for different
expectations, system tests are slow, and so it's better to have just one
test for all the expectations related to the same actions.
Using the `document` or `documents` classes meant styles defined for the
public list of documents conflict with these ones.
So now we're using HTML classes that match the name of the Ruby
component classes, as we usually do.
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.
