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.
We've been getting an error in our CI because some the Ubuntu 24.04
image in GitHub Actions doesn't have an updated package database:
```
E: Failed to fetch mirror+file:
pool/main/g/ghostscript/libgs-common_10.02.1%7edfsg1-0ubuntu7.4_all.deb
404 Not Found
E: Failed to fetch mirror+file:
pool/main/g/ghostscript/libgs10-common_10.02.1%7edfsg1-0ubuntu7.4_all.deb
404 Not Found
E: Failed to fetch mirror+file:
pool/main/g/ghostscript/libgs10_10.02.1%7edfsg1-0ubuntu7.4_amd64.deb
404 Not Found
E: Failed to fetch mirror+file:
pool/main/g/ghostscript/ghostscript_10.02.1%7edfsg1-0ubuntu7.4_amd64.deb
404 Not Found
E: Unable to fetch some archives, maybe run apt-get update
or try with --fix-missing?
```
Running `sudo apt-get update` before trying to install a package solves
the issue.
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.
These rules aren't used since commit c5c9efee1.
Note that there's still an `icon-budget` element in the
`_investment_show` partial. The rules for this icon are already defined
inside the rules for the `.budget-investment-show` selector.
The `icon-budget` and `icon-proposals` HTML classes are still used in
the "Following" tab (the code uses the `followable_icon` method), but in
this case the `.budget-investment` or `.proposal` selectors aren't
present, so the properties weren't applied here either.
Similarly, there are elements with the `icon-debates` and
`icon-proposals` HTML classes in the dashboard area, but they aren't
inside elements matching the `.debate` or `.proposal` selectors either.
And there's an element with the `icon-debate` HTML class in the
legislation area. Once again, it isn't inside an element matching the
`.debate` selector.
So can safely remove this code.
This column isn't used since commit 4c0deb0ec because administrators can
associate videos to the answers since commit 5862eea51. The value of
this attribute isn't used in the public area since commit 8277e3cc2.
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.
