In commit f638e5017 we introduced some methods to avoid race conditions
in tests that created debates, proposals or investments.
However, since we don't have a way to effectively make sure we use these
methods in new code, we forgot to do so when adding tests in commits
c483c6036 and 84b88c0ec.
So we're using them now.
There's a chance that this is what was causing multitenancy tests to
fail sometimes; if we don't wait for the request to get the suggestions
to finish, the application might still be dealing with this request when
we make another request to a different subdomain, or when the test has
finished and the tenant has already been deleted.
On my machine, the test "Creating content in one tenant doesn't affect
other tenants" failed about 5% of the time without these changes, and I
haven't been able to reproduce this failure after applying them. Having
said that, it's possible that this is a coincidence and that this test
will fail for a different reason in the future (like `login_as` not
working properly with subdomains).
Even after disabling the turbolinks previews in the previous commit
(which is still necessary, even with the changes in this commit), these
tests were still finishing before the "Go back" requests did. To
reproduce an issue caused by this behavior, run:
```
rspec spec/system/budgets/ballots_spec.rb:425 spec/system/users_auth_spec.rb:701 --seed 40358
```
Apparently, a `have_current_path` expectation isn't enough to check that
the request has finished and it only checks that the request to that
path has started or it's being processed.
Adding an additional expectation to check that the content of the page
has changed solves the issue.
When clicking the browser's back button, browsers usually don't reload
the page but show a cached version of the page.
Turbolinks takes this one step further. When clicking on a link to a
page that's already cached, turbolinks displays the cached version of
the page and then it reloads it.
I don't really like this behavior but, since it affects the whole
application and we're about to release a patch version :), for now we're
keeping it this way in the development and production environments.
In the test environment, however, we're disabling these previews because
they might lead to requests leaking between tests.
For example, a test that visits the investments index, then goes to
"check my votes", then clicks on "Go back" and finishes by checking some
content on this page will result in those checks being done against the
cached version of the page. If these checks pass before turbolinks
reloads the page, the "Go back" request will finish during the test that
runs immediately after this one, resulting in unpredictable results.
Disabling the previews solves the issue.
Since the PR "Do not use third-party cookies in embedded videos #5548", the logic from
"embed_videos_helper" was extracted to the "embedded_video_component" and the
"videoable" model concern.
However, during this refactor, the "regex" method, which uses record.class:: to handle
video embeds, was left inaccessible for Legislation Proposals.
This commit fixes the issue by including the concern in the Legislation Proposal model.
Using ubuntu-latest might result in incompatibilities when this image
changes to a different version of Ubuntu. For example, the Ubuntu 24.04
image no longer includes imagemagick, meaning that we'll have to install
it manually when using Ubuntu 24.04.
We had a trait called `:admin_request` for actions that are requests to
administrators, but the default factories were also requests to
administrators.
The tests checking that the "Request" button is not present, which
shouldn't pass with the wrong default factories, were passing by
coincidence. The issue was that we weren't checking whether that the
request had finished before checking that the "Request" button wasn't
present. That meant that we were checking that the "Request" button
wasn't there right at the moment we pressed the link, before the request
was finished.
So we're now checking that the request is finished before checking that
the button isn't there.
On the other hand, the tests checking for the "Request resource" link
being present were checking a behavior that's no longer there since
commit 9d85b3935, when we changed the conditions affecting that link.
In rubocop-rails 2.26.0, the Rails/CompactBlank rule was modified to handle
cases where select(&:present?) is used. After identifying three occurrences
in our code, we've decided to apply this rule as it encourages the use of the
more efficient and clearer method, compact_blank.
By using compact_blank, we improve code clarity and performance, as this method performs the same operation but in a more optimized way.
In rubocop-rails 2.26.0, support was added for Rails 7 syntax in the
Rails/EnumHash rule. We took this opportunity to ensure consistency
by converting all enums to hash with integer values. This format minimizes
the risk of data consistency issues in the database when adding new values.
This rule was added in rubocop-rails 2.26.0. Applying it allows
us to anticipate the deprecation of the current enum syntax
using keyword arguments, which is set to be removed in Rails
8.0, as mentioned in the rule's own documentation:
https://docs.rubocop.org/rubocop-rails/cops_rails.html#railsenumsyntax
This rule was introduced in RuboCop 0.67.2, but now after seeing a fix in version 1.65.1,
we have decided to add it. The reason for adding it is to ensure consistency in how we
reference exceptions throughout the project, by following a standard naming convention
for exception variables.
This rule was introduced in RuboCop 1.53.0. After adding the
Style/RedundantRegexpCharacterClass rule in the previous commit,
RuboCop started detecting redundant regular expression arguments.
Therefore, we apply this rule to remove them and prevent future
occurrences.
This rule was introduced in RuboCop 0.93.0, but now after seeing a fix in version 1.65,
we have decided to add it. The reason for adding it is to simplify our regular
expressions. This enforcement will help us maintain better regular expression
practices across the project.
We were checking the proposal video URL, but its value was `nil` since
commit bedcb5bca2. This resulted in a warning:
```
Checking for expected text of nil is confusing and/or pointless since it
will always match. Please specify a string or regexp instead.
spec/system/admin/hidden_proposals_spec.rb:14
```
We had three files that were almost identical, and we can use
environment variables to specify the differences.
Note we're using the `PGUSER` and `PGPASSWORD` variables, since these
variables will automatically be used by the PostgreSQL client when we
have a blank `username` and `password` keys in the `database.yml` file
(which we did until now). The difference between these variables and the
`POSTGRES_USER` and `POSTGRES_PASSWORD` variables is that the `PG`
variables are used by the client connecting to the database, while the
`POSTGRES_` variables are used by the Docker postgresql image when
creating the database superuser.
For consistency with the code in our github workflows (and everywhere
else in the postgres world), we're respecting this double standard. The
fact that there are two different names for what's basically the same
thing makes the code confusing, though, particularly when running the
docker-compose commands, since we get the password from an environment
variable but we have to assign two different environment variables with
it.
So we're accepting both `PGPASSWORD` and `POSTGRES_PASSWORD` variables
in the database configuration file. This way, developers using
docker-compose can use `POSTGRES_PASSWORD` for everything and it'll work
fine. We're also making `PGPASSWORD` default to `POSTGRES_PASSWORD` so
we don't get a warning if we only set `POSTGRES_PASSWORD`:
```
WARN[0000] The "PGPASSWORD" variable is not set. Defaulting to a blank
string.
```
Also note we're using `DB_HOST` instead of `PGHOST` because that's the
variable Rails currently uses by default for new applications [1].
Finally, note we're using `.presence` in the `ENV` calls in the
database.yml file. The `PGPASSWORD` variable was set to an empty string
when running docker-compose, so using `ENV["PGPASSWORD"] ||` wouldn't
work.
[1] https://github.com/rails/rails/blob/c90a8701e5/railties/lib/rails/generators/rails/app/templates/config/databases/postgresql.yml.tt#L22
