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.
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.
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.
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 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
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.
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.
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.
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`.
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've got quite a messy hack to sign in managers: they need to visit a
specific URL (management root path).
That means tests signing in managers start the browser to sign them in,
which might cause issues if we setup the database after that.
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.
So tests won't fail when an institution changes the default organization
name.
The tests are also easier to understand now, since it's more obvious
where the "CONSUL" text is coming from.
When a user recovers a page from browser history where placed a
marker in different map pane (visible map layer) marker was
successfully added to the map but the map center is the one
defined at Settings map properties so the marker was not visible
to the user.
Now when map_location form has valid coordinates we use them
instead of default map center settings. This will avoid the user to
have to rellocate the marker (or find the correct pane where the
marker was added) if already placed.
When using an editable map is better to load marker latitude, longitude and
map zoom from form fields so we can show the marker at latest position defined
by user when the page was restored from browser history.
To reproduce this behavior:
0. Undo this commit
1. Go to new proposal page
2. Place the proposal map marker
3. Go away to any other page
4. Restore new proposal page from browser history.
At this point you should not see the recently placed marker.
The same thing happens when editing a proposal.
If we do not do this a map could be initialized twice times or more
when restoring a page with a map causing weird UI effects and
loading some map layers also twice times or more.
Need to add a maps array to be able to store all initialized
(visible) maps so we can destroy them when needed. Notice that
we are destroying maps also when admin settings tabs changes
(only visible ones), this is again to avoid to re-initialize map more
than once when users navigate through settings tabs, another
option to the settings issue could be to detect if the map was
already initialized to skip uneeded initialization.
