We were rendering one label and many textarea fields for that label.
This meant that, when switching to a different language, the label
wasn't correctly associated with the textarea.
So we're now rendering one label for each textarea. We could use
`aria-label` or `aria-labelledby` instead, but using a label offers some
advantages like the fact that clicking on the label makes the textarea
take the focus.
The absence of labels in these controls made them hard to use,
particularly for people who use screen readers.
Note we're removing the "Choose language" prompt, since we always
automatically choose a language and not choosing a language doesn't
really make sense. The only scenario where the prompt was used took
place when all languages had been removed but, in that case, the "Choose
language" prompt was misleading because there were no languages to
choose from.
We were using `check "Valuation finished"` everywhere except here.
This way it's easier to realize, while reading the test, that we're
interacting with a checkbox and not a link or a button.
As mentioned in commit 5311daadf, there are several reasons to use
buttons in these situations. And, as mentioned in the previous commits,
using buttons instead of links for actions requiring confirmation will
help us test for accessibility issues.
As mentioned in commit 5311daadf, there are several reasons to use
buttons in these situations. And, as mentioned in the previous commits,
using buttons instead of links for actions requiring confirmation will
help us test for accessibility issues.
As mentioned in commit 5311daadf, there are several reasons to use
buttons in these situations. And, as mentioned in the previous commits,
using buttons instead of links for actions requiring confirmation will
help us test for accessibility issues.
As mentioned in commit 5311daadf, there are several reasons to use
buttons in these situations. And, as mentioned in the previous commits,
using buttons instead of links for actions requiring confirmation will
help us test for accessibility issues.
Since we're adding styles for this button, we're also adding the
`font-size` property instead of using the `small` class. We'll deal with
the `float-right` property in the next commit.
This way it'll be easier for people using screen readers to know which
link/button they're about to click.
Note that, at least for now, we aren't reusing the code en
`Admin::ActionComponent`. We might do so in the future if we implement
similar code in more parts of the public area.
Note that, since the button now generates a `form` tag, we need to
adjust the styles of this section.
As mentioned in commit 5311daadf, there are several reasons to use
buttons in these situations. And, as mentioned in the previous commits,
using buttons instead of links for actions requiring confirmation will
help us test for accessibility issues.
Note we're simplifying the `table .button` margin rules because the
`.button` class already defines `0` for all its margins except the
bottom margin. Otherwise, the margins defined by the `flex-with-gap`
mixin would be overwritten by the margins defined in the `table .button`
class.
As mentioned in commit 5311daadf, there are several reasons to use
buttons in these situations. And, as mentioned in the previous commit,
using buttons instead of links for actions requiring confirmation will
help us test for accessibility issues.
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.
We forgot to do so in commit f0ab7bcfc. We're still leaving one system
spec to test that these images are rendered when browsing the site.
We're also changing the existing component tests to check the `alt`
attribute, just like the remaining system test does.
Just like we did in commits like f2e32b44b, a8537f7e1 and be9fc2265,
we're replacing a buggy JavaScript solution with one using just CSS.
Besides, we've had a failure in our test suite caused by an image not
being displayed on the page, with the message:
```
Failures:
1) Executions Images renders last milestone's image if investment has multiple milestones
with images associated
Failure/Error: expect(page).to have_css("img[alt='Second image']")
expected to find visible css "img[alt='Second image']" but there were no matches.
Also found "", which matched the selector but not all filters.
# ./spec/system/budgets/executions_spec.rb:135:in `block (3 levels) in <top (required)>'
```
The text "matched the selector but not all filters" means that the
element was present on the page but wasn't visible. One possible cause
is that the equalizer was adjusting the height of the element containing
the image before the image was loaded.
Note that, after these changes, all investments on the same row will
have the same height but, unlike with Foundation's equalizer,
investments on different rows might have different heights.
Note that we're changing the component so it uses `polymorphic_path`;
that way we don't have to pass the `@budget` variable to the component.
We could also use `budget_investment_path investment.budget, investment`
instead.
In commit 96ae69fe9, we stopped using cookies to track Ahoy visits and
started using a combination of the IP and the browser agent instead.
However, since we're still using the legacy logic from Ahoy 1.x to track
visits (which we had to add in commit b5220effd), this way of tracking
visits doesn't work and counts every page visited by a user as an
independent visit.
Maybe we could migrate existing data, which uses the `visitor_id` column
so it uses the new `visit_token` and `visitor_token` columns, but
there's no mention in the Ahoy documentation regarding how to do so.
While deciding what to do about this, we found something interesting.
For two years, we've been seeing random failures in the
`system/admin/tenants_spec.rb` tests, with messages like:
```
1) Tenants Create Tenant with subdomain
Failure/Error:
raise TenantNotFound, <<~EXCEPTION_MESSAGE
Could not set search path to schemas, they may be invalid:
"#{tenant}" #{full_search_path}.
Original error: #{exception.class}: #{exception}
EXCEPTION_MESSAGE
Apartment::TenantNotFound:
Could not set search path to schemas, they may be invalid:
"earth" "public", "shared_extensions".
Original error:
ActiveRecord::StatementInvalid: Could not find schema earth
```
And we've found one of the causes: the AJAX requests done by Ahoy to
track visits. Sometimes a test that creates or updates a tenant finishes
but the Ahoy AJAX request to, say, `earth.lvh.me/ahoy/visits`, is
handled by the next test, when the `earth` schema no longer exists, thus
raising an `Apartment::TenantNotFound` exception.
So by disabling these AJAX requests and tracking the visits in the
server instead, we're killing two birds in one stone: we're fixing the
bug regarding the visits count and we're reducing the flakiness in our
test suite. It looks like we're also removing the "phantom ahoy cookie"
we were getting since the mentioned commit b5220effd: an ahoy cookie was
quickly set and unset in the browser.
Note that, even though we aren't migrating any data, we're still adding
the new fields, because some tests started to fail because, when
tracking visits in the server without cookies, Ahoy expects the Visit
model to have a `visit_token` field.
The search_keyword column was removed for new installations in Ahoy
2.1.0 and stopped being used completely in Ahoy 3.0.0.
There are other columns that we use and are no longer generated by
default by Ahoy, which are: screen_height, screen_width and postal_code.
Apparently they're still used in Ahoy if they're available, though, so
we aren't removing them, at least for now.
For reasons that might or might not affect production installations, the
test checking simultaneous requests to create poll voters in the
officing voters controller wasn't behaving as expected.
The expected behavior, since commit 9a8bfac5b, is that the second
request reaching the `with_lock` part of the code waits for the first
request to finish and so this second request raises an
`ActiveRecord::RecordInvalid` exception when trying to save a voter with
the same poll and the same user as the first one.
However, 95% of the time that wasn't the case. Instead, when entering
the `@user.with_lock` block, the second request would replace its
`@voter` object with the `@voter` object saved in the same request, so
the second call to `save!` would succeed as it would simply update the
existing record.
This is a behavior that we could accept if it were consistent and
happened 100% of the time, but that isn't the case. 5% of the time, we
do get the `ActiveRecord::RecordInvalid` exception. So 5% of the time we
got a failure in the test:
```
1) Officing::VotersController POST create does not create two records
with two simultaneous requests
Failure/Error: @user.with_lock { @voter.save! }
ActiveRecord::RecordInvalid:
Validation failed: User User has already voted
# ./app/controllers/officing/voters_controller.rb:25:in `block in create'
# ./app/controllers/officing/voters_controller.rb:25:in `create'
# ./app/controllers/application_controller.rb:50:in `switch_locale'
# ./spec/controllers/officing/voters_controller_spec.rb:15:in `block (5 levels) in <top (required)>'
```
So we're changing the `with_lock` block so it includes the
initialization of the object. This way, we get the
`ActiveRecord::RecordInvalid` exception 100% of the time.
Note that in commit 9a8bfac5b we also rescued the
`ActionDispatch::IllegalStateError` exceptions. I'm not why we were
getting those exceptions when running the tests, and I'm not sure
whether we keep getting after these changes, but it doesn't really
matter. The reason is that in Consul Democracy 2.3.0 we're going to add
a unique index to the `poll_voters` table, which (according to the tests
done in the past) will make both the `@user.lock` block and rescuing the
`ActionDispatch::IllegalStateError` unnecessary.
So, in other words, these changes will never make it to production
because this part of the code will be changed again before releasing
version 2.3.0.
One of these tests has failed in our CI with the following message:
```
1) Users Public activity user can hide public page
Failure/Error: expect(page).to have_content("activity list private")
expected to find text "activity list private" in "Language: \n
\nEnglish\nDeutsch\nEspañol\nFrançais\nNederlands\nPortuguês
brasileiro\n中文\n Notifications\nYou are in\nMy content\nMy
account\nSign out\nDebates\nProposals\nVoting\nCollaborative
legislation\nParticipatory budgeting\nSDG\nHelp\nM\nManuela124\nUser has
no public activity\nOpen government\nThis portal uses the CONSUL
DEMOCRACY application which is open-source
software.\nParticipation\nDecide how to shape the city you want to live
in.\nCONSUL DEMOCRACY, 2024 Privacy Policy Terms and conditions of use
Accessibility"
```
Note how the text "User has no public activity" is present, which is a
message that appears when the user's activity is public.
A possible explanation is that we didn't check that the request done by
the "Save changes" button had finished before continuing with the tests.
Back when we wrote this test, submitting a form in a test would always
wait for the request to be finished before continuing, but a lot has
changed since then.
So we're adding an expectation to make sure the the changes have been
saved before making a new request.
We're also rearraging the blank lines in these tests and removing the
parenthesis in `have_content` expectations to be consistent with the
expectations we're adding.
After the previous commit, we were using `double` in many places but
were only using `instance_double` in one file. So, for consistency,
we're using `double` in this file as well.
Lately (not sure since when), from time to time we've been getting these
failures in our CI:
```
Failures:
1) CommentsHelper#comment_author_class returns is-author if author is the commenting user
Failure/Error: comment = instance_double(Comment, user_id: author_id)
the Comment class does not implement the instance method: user_id
# ./spec/helpers/comments_helper_spec.rb:48:in `block (3 levels) in <top (required)>'
# ./spec/spec_helper.rb:40:in `block (3 levels) in <top (required)>'
# ./spec/spec_helper.rb:39:in `block (2 levels) in <top (required)>'
2) CommentsHelper#comment_author_class returns an empty string if commenter is not the author
Failure/Error: comment = instance_double(Comment, user_id: author_id - 1)
the Comment class does not implement the instance method: user_id
# ./spec/helpers/comments_helper_spec.rb:55:in `block (3 levels) in <top (required)>'
# ./spec/spec_helper.rb:40:in `block (3 levels) in <top (required)>'
# ./spec/spec_helper.rb:39:in `block (2 levels) in <top (required)>'
```
It might be related to the upgrade of rspec-rails done in commit
6fe222148 or maybe due to a change in github actions that caused some
tests to fail, as described in commits bedcb5bca and 3e44eeaee.
What might be causing the issue is the usage of `instance_double`
stubbing different methods in different tests (not sure this is the
cause, though).
We've seen that somebody got a similar error [1] (although it might not
have been for the same reason) and one of the maintainers of rspec-mocks
replied:
> I would recommend switching to double (as you mentioned) or
> refactoring to use something more defined.
So we're simply using `double`, which is what we usually use when
stubbing objects in the tests. Doing so is faster than further
investigating why the `instance_double` isn't reliable 100% of the time.
[1] See issue 1587 in https://github.com/rspec/rspec-mocks/
