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.
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.
In the Management section when creating an investment we were not passing the
images attributes, so we were never able to associate images.
Make the nested_imageable 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.
We were submitting the form without checking the AJAX request to attach
the image had finished, so sometimes two requests were executed at the
same time. Sometimes this made InvisibleCaptcha to go crazy and report
the form was submitted too quickly.
Checking the first AJAX request has finished before submitting the form
solves the problem.
