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.
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.
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.
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 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.
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.
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).
We were hiding the file input and styling the label as a button instead.
Since clicking on a label has the same effect as clicking on the input,
the input worked properly for mouse and touch screen users.
However, hiding the input makes it inaccessible for keyboard users,
since labels don't get keyboard focus, but inputs do.
So we must not hide the input but make it invisible instead. But we
still need to hide the input (alongside the label) after a file has been
attached.
We could add some extra JavaScript to hide the input when we hide the
label. Since the JavaScript is already quite complex and my first few
attempts at changing it failed, I've opted to assume that the input (and
its label) must be hidden whenever there's already a file name, and
implement that rule with CSS.
Note we're using the `:focus-within` pseudoclass to style a label when
focus is on the input. This rule (at the time of writing) is only
supported by 93.5% of the browsers. Keyboard users without a screen
reader and using the other 6.5% of the browsers will still be able to
focus on the field but might not notice the field has received focus.
Since the percentage of affected users will decrease over time and until
now 100% of keyboard users were completely unable to focus on these
fields, for now we think this is a good-enough solution.
Using `dom_id` means generating `new_document` as ID for new documents.
Since there might be more than one new document in the form, that means
duplicate IDs, which is invalid HTML.
Even though this issue doesn't affect image fields (because we don't
have many images on the same form), we're removing the ID there as well
for consistency.
In the Management section when creating an investment we were not passing the
document attributes, so we were never able to associate documents.
Make the nested_documentable spec compatible with the Management section.
JavaScript is used by about 98% of web users, so by testing without it
enabled, we're only testing that the application works for a very
reduced number of users.
We proceeded this way in the past because CONSUL started using Rails 4.2
and truncating the database between JavaScript tests with database
cleaner, which made these tests terribly slow.
When we upgraded to Rails 5.1 and introduced system tests, we started
using database transactions in JavaScript tests, making these tests much
faster. So now we can use JavaScript tests everywhere without critically
slowing down our test suite.
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.
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.
We were checking `expect_document_has_title(0, "My Title")`, which was
already true before the AJAX request generated by `attach_file` had
finished.
That meant the AJAX request sometimes was handled after this test had
finished, affecting the following test and causing it to fail because
its cookie was overwritten and so `current_user` was set to `nil`.
In the test checking the filename is present, a similar scenario was
taking place: we were updating the `.file-name` element in the `change`
event of `fileupload` (using `App.Documentable.setFilename`); that is,
when the AJAX request started. And so the test passed before the request
was finished, causing the same issue.
