This way we avoid possible database corruption in our CI, which has
happened in the past when the process running the test accesses the
database after the process running the browser has started.
The original implementation (which was never merged) had a `<select>`
field for the switch, which offered accessibility issues. So I came up
with a very bad idea, which was emulating the look and feel of a select
field while making it more accessible for keyboard users.
This approach is inconvenient because we were using a bunch of ARIA
roles to do the same thing that can be done with a list of links, going
against the first rule of ARIA, which is:
> "Don’t use ARIA if you can achieve the same semantics with a native
> HTML element or attribute
Not only that, but the control was confusing for people using mobile
phones (select fields don't behave the same way), and we were using
*invalid* ARIA roles in this situation, leading Axe to report a critical
accessibility error:
```
aria-required-children: Certain ARIA roles must contain particular
children (critical)
https://dequeuniversity.com/rules/axe/4.10/aria-required-children?application=axeAPI
The following 1 node violate this rule:
Selector: ul[data-dropdown-menu="edw1i2-dropdown-menu"]
HTML: <ul class="dropdown menu" wnenu="edw1i2-dropdown-menu"
data-disable-hover="true" op="true" role="menubar">
Fix any of the following:
- Element has children which are not allowed: button[tabindex]
```
So, at least for now, we're using a simple list of links. We might style
it in the future if we find ways to make usability improvements, but,
for now, it does the job, and it does it better than the custom control
we were using.
We were getting an exception in this case, which was OK I guess since
this shouldn't happen if the application is used in a normal way, but we
can simplify the code a little bit if we make the `recipients` code
return an empty list of users.
Note that the behavior of the `AdminNotification#list_of_recipients` and
`Newsletter#list_of_recipient_emails` methods is now slightly different;
previously they returned `nil` when given an invalid segment recipient,
while now they return an empty array. I haven't found a place where this
change is relevant. For example, in both of these models, the `deliver`
method used to raise an exception when given an invalid segment while
now it doesn't, but we always check the user segment is valid before
calling the `deliver` method anyway, so it doesn't really affect the
application.
While we've already got a system test for the same purpose, we're going
to add another controller test for this action, so we're now writing a
test for the "happy path" scenario.
We were getting a warning by CodeQL regarding a possible code injection
in the `send(segment)` code.
In practice, this wasn't a big deal because the `self.recipients` method
is only called in the admin section, meaning only admin users could try
to take advantage of the code injection, and because this code is rarely
called with an invalid segment due to several conditions in the code
checking that the user segment is valid, with the only exception being
the `generate_csv` action in the `Admin::EmailsDownloadController`.
In any case, now we're checking that the segment is valid before calling
the `send` method. Since now we're making sure that the segment is valid
in the `recipients` method itself, we can remove this check from methods
calling it.
This way we can move some system tests to component tests and stop
creating records after starting the browser with a `visit`.
We could also split the system test in two, but since these tests
aren't checking any user interactions, moving the to component tests we
check the same things while making the tests faster.
Since the partial was using an instance variable, we're passing it to
the component. We're naming it `voter_user` instead of `user` because
passing something named `user` could make us think that we're passing
the `current_user`. I wasn't sure about naming it `voter` because it's a
`User` record and not a `Poll::Voter` record, but naming it `voter`
would definitely be an option.
In some places, we were accidentally creating records after the browser
started because we weren't executing a `let` block before starting the
browser with a `visit`, but were executing the `let` block after that.
In the officing tests, we were accessing `admin.user` after starting the
browser with a `visit`. However, since `admin` is a method with a `let`
block, the administrator isn't created in the database until we
referenced the variable, meaning we were creating the database record in
the middle of the test. Referencing the variable at the beginning of the
test solves the issue.
We were also creating records to call the `login_as` method in the users
tests, so we're moving the code to create them before the first call to
`visit`.
In the notifiable test, we were doing a loop consisting of "create()" ->
"visit" -> "create()" -> "visit!" -> (...), meaning we were creating the
second user after the first `visit`. Creating every user before the
first `visit` solves the issue.
As mentioned in commits like a586ba806, a7664ad81, 006128da5, b41fbfa52
and c480cdd91, accessing the database after starting the browser with
the `visit` method sometimes results in database corruption and failing
tests on our CI due to the process running the test accessing the
database after the process running the browser has started.
In this case, we were hiding a proposal after starting the process
running the browser to check what happens when accessing a notification
for a hidden proposal. We can avoid database access in the middle of the
test by hidding a proposal before starting the browser. The process to
create a notification using the browser is already tested in other
specs, so we don't need to do it here as well.
Note that, to simplify the test, we're extracting the `notify_users`
method. I wonder whether this method should be called in an
`after_create` callback instead... That's a topic for another time,
though.
As mentioned in commits like a586ba806, a7664ad81, 006128da5, b41fbfa52
and c480cdd91, accessing the database after starting the browser with
the `visit` method sometimes results in database corruption and failing
tests on our CI due to the process running the test accessing the
database after the process running the browser has started.
IMHO this is also a bad practice for system tests, since these tests
should be checking what users experience.
In this case, however, I haven't found a way to destroy the VerifiedUser
record using the interface, so we're stubbing `VerifiedUser` instead.
Note we can't stub it since the beginning of the test because we're
testing what happens when clicking the "Send code" button doesn't work,
and we wouldn't be able to access the page containing the "Send code" if
we stubbed `VerifiedUser` since the beginning of the test.
We've already got model tests for that, and we were modifying the
database after a `visit`, since we were doing a loop consisting of
"update!" -> "visit" -> "update!" -> "visit!" -> (...).
Besides, note that the `.kind_or_later("publishing_prices").each` block
wasn't modifying the budget, so it always visited the page under the
same conditions.
So we're simply randomly checking one phase to test that the user
interface works as expected.
As mentioned in commits like a586ba806, a7664ad81, 006128da5, b41fbfa52
and c480cdd91, accessing the database after starting the browser with
the `visit` method sometimes results in database corruption and failing
tests on our CI due to the process running the test accessing the
database after the process running the browser has started.
IMHO this is also a bad practice for system tests, since these tests
should be checking what users experience.
So, just like we did a few commits ago with tests that reloaded records,
we're modifying the tests to check the results of user interactions from
the point of view of the users.
Also note we aren't changing tests with the `:no_js` tag, since these
tests don't run a real browser in a separate process. In the future, we
should also change most of these tests so they don't access the database
and they use a real browser.
Finally, note that one of the tests we're changing in the shared
`notifiable_in_app` file did not check the database content, but we're
also changing it for consistency.
As mentioned in commits like a586ba806, a7664ad81, 006128da5, b41fbfa52
and c480cdd91, accessing the database after starting the browser with
the `visit` method sometimes results in database corruption and failing
tests on our CI due to the process running the test accessing the
database after the process running the browser has started.
In these cases, there's no need to check the database; we're already
checking the application behavior that shows that the voters have been
correctly created.
We can reuse the `officing_verify_residence` method to make the test
easier to read.
We're also removing the final visit to final_officing_polls_path because
it doesn't really add anything to the test.
We were checking that a field exists before interacting with it.
However, the methods that interact with fields already check whether the
field exists, so there's no need for us to check that.
We're removing the word "email" from the method name because the method
was accepting either an email or a username, and we're using the name of
the label to fill in fields, which is better because it checks that the
label is correctly associated with the field , as shown for instance in
commit 431ebeda8.
We were always passing `officer.user` to this method, so we might as
well pass the officer (since the "officer" is in the name of the method)
and call `officer.user` inside the method.
We're also calling `login_through_form_as` in order to remove the
duplication between these two methods.
As mentioned in commits like a586ba806, a7664ad81, 006128da5, b41fbfa52
and c480cdd91, accessing the database after starting the browser with
the `visit` method sometimes results in database corruption and failing
tests on our CI due to the process running the test accessing the
database after the process running the browser has started.
In this case, we're avoiding the usage of `user.subscriptions_token` and
`Comment.last`. In the future, we should probably simplify these tests
by moving most of the checks to a mailer test.
As mentioned in commits like a586ba806, a7664ad81, 006128da5, b41fbfa52
and c480cdd91, accessing the database after starting the browser with
the `visit` method sometimes results in database corruption and failing
tests on our CI due to the process running the test accessing the
database after the process running the browser has started.
Note that, in this case, in order to make the tests more readable, we're
adding a bit of duplication. We should probably simplify these tests by
moving most of the checks to mailer tests and then we could remove the
duplication. The alternative would be to make the
`create_direct_message` method way more complex than it is right now.
As mentioned in commits like a586ba806, a7664ad81, 006128da5, b41fbfa52
and c480cdd91, accessing the database after starting the browser with
the `visit` method sometimes results in database corruption and failing
tests on our CI due to the process running the test accessing the
database after the process running the browser has started.
For example, one of these tests has recently failed on our CI:
```
3) Users Create a level 3 user with email from scratch
Failure/Error: expect(user.reload).to be_confirmed
expected `#<User id: 2060, email: "pepe@gmail.com", created_at:
"2025-03-12 19:51:03.688867000 +0100", updated_...d_debates: true,
recommended_proposals: true, subscriptions_token: nil,
registering_from_web: false>.confirmed?` to be truthy, got false
```
IMHO this is also a bad practice for system tests, since these tests
should be checking what users experience.
So we're modifying the tests to check the results of users interaction
from the point of view of the users. For example, instead of checking
that a user is now level 3 verified in the database, we're checking that
the user interface states that the user is level 3 verified.
Note we're adding an offset when editing the map marker by clicking on
`map-location` with `.click(x: 30, y: 30)`. This way we make sure that
both the latitude and longitude change from the original values; we used
to clicking in the middle (no offset), which didn't change the longitude
and changed the latitude just by coincidence.
Also note we aren't changing tests with the `:no_js` tag, since these
tests don't run a real browser in a separate process. In the future, we
should also change most of these tests so they don't access the database
and they use a real browser.
We already have a test in the `level_three_verification_spec` file that
checks the exact same things and then the letter verification. And, for
the SMS step, we also have a test in the `sms_spec` file.
As mentioned in the previous commit, checking the database after
starting the browser with the `visit` method sometimes results in
database corruption and failing tests on our CI due to the process
running the test accessing the database after the process running the
browser has started.
IMHO this is also a bad practice for system tests, since these tests
should be checking what users experience.
In this case, however, I haven't been able to test the user
experience since it looks like booths and officer assignments for voters
aren't shown anywhere.
So, since the purpose of the test was to check the database, and there
are other tests checking what happens after clicking the "Confirm vote"
button in the user interface, we're converting this test into a
controller test.
As mentioned in commits like a586ba806, a7664ad81, 006128da5, b41fbfa52
and c480cdd91, accessing the database after starting the browser with
the `visit` method sometimes results in database corruption and failing
tests on our CI due to the process running the test accessing the
database after the process running the browser has started.
IMHO this is also a bad practice for system tests, since these tests
should be checking what users experience.
In these cases, however, I haven't been able to test the user
experience. For example, it looks like failed census calls for
unregistered users aren't displayed anywhere and can only be accessed by
manually checking the database. Similarly, there's no interface showing
that all the options from a poll have been deleted (which makes sense,
since we only display options in the context of their poll) or a place
showing the responsible name for a proposal.
So we're splitting the tests in two, with the controller test running
the database checks.
These blocks are no longer used:
* `allowed_phase_list` isn't used since commit 04605d5d5
* `level_two_user` isn't used since commit 26d14cbd0
* `heading` in `budgets/stats_spec` was added in c2457e36a but never
used
* `translatable` was added in 44d137a4c but it's overwritten in all the
contexts.
* `annotation` isn't used since commit 79d00e7b9
* `admin` in `tags/budget_investments_spec` isn't used since 8a2e15980
* `budget` in `welcome_spec` was added in 87be6f302 but never used
I don't think this feature it was ever used. It was introduced in commit
49dec6061 as part of a feature that was removed in commits 1cd47da9d and
c45a0bd8ac.
We removed the link to this page in commit 83e8d6035 because poll
questions don't really make sense without a poll.
However, this page also contained information about successful
proposals, which might be interesting so administrators don't have to
navigate to the public area in order to find and create questions based
on successful proposals.
So we're keeping the part about successful proposals and linking it from
the proposals part of the admin area.
Note we're using translation keys like `successful_proposals_tab`, which
don't make sense anymore, for the successful proposals. We're doing so
because we've already got translations for these keys and, if we renamed
them, we'd lose the existing translations and our translators would have
to add them again.
Also note we're changing one poll question test a little bit so we
create the question from a successful proposal using the new page. There
are other tests checking how to create a question from the
admin/proposals#show action and other tests checking what happens when
accessing a successful proposal in the admin section, so we don't lose
any test coverage by changing an existing test instead of adding a new
one.
Finally, note that we've removing the `search` method in poll question
because we no longer use it. This currently makes the
`author_visible_name` database column useless; we aren't removing it
right now because we don't want to risk a possible data loss in a patch
release (we're about to release version 2.3.1), but we might remove it
in the future.
I accidentally made this mistake while trying to avoid the exact same
issue back in commit 73992b2c8. Accessing the database in a test after
starting the process running the browser has caused database corruption
in our CI multiple times.
This parameter isn't used since commit b4a6f664b.
Note we're changing the tests to use proposals instead of debates
because proposals may have images attached, while debates may not.
This way we know for sure the page has finished refreshing.
Note that, for now, we aren't adding a check after every call to the
`refresh` method because sometimes the page remains identical after
refreshing. Not sure what we should do in these cases.
After a `visit`, we were checking for content or filling in fields that
were already there before the `visit`, so we weren't 100% sure that the
request had finished before the test continued.
In the case of the verification tests, we were clicking the submit
buttons over and over without and then checking or interacting with
elements that were already there. Even though the button was disabled
between requests, meaning there wouldn't be simultaneous requests, it
was possible to interact with a form field before waiting for the
request to finish.
Some of these tests have recently failed on our CI, and it might be
because of that:
```
1) Admin budgets Edit Changing name for current locale will update the
slug if budget is in draft phase
Failure/Error: raise ex, cause: cause
Selenium::WebDriver::Error::UnknownError:
unknown error: unhandled inspector error: {"code":-32000,
"message":"Node with given id does not belong to the document"}
(Session info: chrome=134.0.6998.35)
1) Budgets creation wizard Creation of a multiple-headings budget by
steps
Failure/Error: expect(page).to have_content "Heading created
successfully!"
Selenium::WebDriver::Error::UnknownError:
unknown error: unhandled inspector error: {"code":-32000,
"message":"Node with given id does not belong to the document"}
(Session info: chrome=134.0.6998.35)
1) Custom information texts Show custom texts instead of default ones
Failure/Error: raise ex, cause: cause
Selenium::WebDriver::Error::UnknownError:
unknown error: unhandled inspector error: {"code":-32000,
"message":"Node with given id does not belong to the document"}
(Session info: chrome=134.0.6998.35)
1) Users Regular authentication Sign in Avoid username-email collisions
Failure/Error: raise ex, cause: cause
Selenium::WebDriver::Error::UnknownError:
unknown error: unhandled inspector error: {"code":-32000,
"message":"Node with given id does not belong to the document"}
(Session info: chrome=134.0.6998.35)
2) Verify Letter Code verification 6 tries allowed
Failure/Error: raise ex, cause: cause
Selenium::WebDriver::Error::UnknownError:
unknown error: unhandled inspector error: {"code":-32000,
"message":"Node with given id does not belong to the document"}
(Session info: chrome=134.0.6998.35)
2) Valuation budget investments Valuate Finish valuation
Failure/Error: raise ex, cause: cause
Selenium::WebDriver::Error::UnknownError:
unknown error: unhandled inspector error: {"code":-32000,
"message":"Node with given id does not belong to the document"}
(Session info: chrome=134.0.6998.35)
1) Users Delete a level 2 user account from document verification page
Failure/Error: raise ex, cause: cause
Selenium::WebDriver::Error::UnknownError:
unknown error: unhandled inspector error: {"code":-32000,
"message":"Node with given id does not belong to the document"}
(Session info: chrome=134.0.6998.35)
```
We're already testing the navigation in one test, and in some of these
tests we were checking, for instance, that the title of an investment is
present after a click, but since it is also present before that click,
that could lead to the test finishing before the request does.
This way it's more obvious what's going on.
Note that, in this case, the expectations were **not** true before
visiting the page, so we aren't fixing a flaky test.
This way the code is easier to follow, and we avoid having simultaneous
requests on different browser sessions, which in theory shouldn't affect
the application, but the test might be a bit more robust this way.
This way the tests are slightly faster, and we avoid a possible flaky
spec since in one test we were checking that a certain text was not
present, which didn't guarantee that the request to delete the document
had finished.
After clicking the "Save changes" button, we were sometimes checking
expectations that were already true before clicking the button, so it
was possible that the request generated by that button didn't finish
before the test did.
So now we're checking that the button is no longer present, which is
only true after the request has finished.
The expectation that there isn't a link with the text "Español" wasn't
updated in commit f7417d647, when the tests were updated after switching
the user interface from a list of links to a select field.
We're also adding an additional expectation after the `visit` to make
sure the request has finished before selecting English from the selector
(which probably doesn't make sense because it's the only option, but
changing it is out of the scope of this commit).
We were checking that, after a `visit`, we're redirected to the account
page, but, since we were already in the account page, the page doesn't
change at all. In order to be 100% that the request caused by the call
to `visit` has finished before checking the expectations, we're visiting
a different page first.
We weren't checking whether the request creating the proposal had
finished before checking the document. That's probably why this test has
recently failed on our CI:
```
1) Documents Metadata download document without metadata
Failure/Error: io = URI.parse(
"#{app_host}#{polymorphic_path(
Document.last.attachment)}"
).open
NoMethodError:
undefined method `attachment' for nil:NilClass
```
We weren't checking that the requests had finished before checking the
last sent email. That's probably why one of these tests has recently
failed on our CI:
```
1) System Emails Preview Pending #send_pending
Failure/Error: email = open_last_email
RuntimeError:
No email has been sent!
```
These tests don't use the browser to send emails since commit e21588ec1.
However, note that this commit actually actually decreased our test
coverage somehow; since then, we're no longer testing whether we send an
email to the author after clicking the "Publish comment" button. We
might need to add a test for this in the `spec/system/comments_spec.rb`
file... but that's a story for another time.
Note we need to stub the `deliver_later` method; otherwise,
`open_last_email` would raise an exception since no email would have
been delivered.
We're moving them now because these tests use the `open_last_email`
method, and we're looking for places where using this method might
result in a flaky test when used inside a system test.
These tests were testing mailer methods, but were inside a file for
system tests.
We're moving them now because these tests use the `open_last_email`
method, and we're looking for places where using this method might
result in a flaky test when used inside a system test.
These tests were testing model methods, but were inside files for system
tests.
We're moving them now because these tests use the `open_last_email`
method, and we're looking for places where using this method might
result in a flaky test when used inside a system test.
