Instead of having to add `beginning_of_minute` to deal with an issue
with Capybara filling datetime fields as mentioned in commit 5a0fde4048,
we can travel to the beginning of the minute so we don't have to take
the seconds into account.
The extra check to see the voter count has increased was redundant; we
already check the request has finished inside the
`vote_for_poll_via_web` method and we check all three voters are created
in the results table.
Updating the poll so it's in the past after starting the browser might
result in database inconsistencies while running the tests, so we're
using `travel_to` instead.
We were already saving it as a time, but we didn't offer an interface to
select the time due to lack of decent browser support for this field
back when this feature was added.
However, nowadays all major browsers support this field type and, at the
time of writing, at least 86.5% of the browsers support it [1]. This
percentage could be much higher, since support in 11.25% of the browsers
is unknown.
Note we still need to support the case where this field isn't supported,
and so we offer a fallback and on the server side we don't assume we're
always getting a time. We're doing a strange hack so we set the field
type to text before changing its value; otherwise old Firefox browsers
crashed.
Also note that, until now, we were storing end dates in the database as
a date with 00:00 as its time, but we were considering the poll to be
open until 23:59 that day. So, in order to keep backwards compatibility,
we're adding a task to update the dates of existing polls so we get the
same behavior we had until now.
This also means budget polls are now created so they end at the
beginning of the day when the balloting phase ends. This is consistent
with the dates we display in the budget phases table.
Finally, there's one test where we're using `beginning_of_minute` when
creating a poll. That's because Chrome provides an interface to enter a
time in a `%H:%M` format when the "seconds" value of the provided time
is zero. However, when the "seconds" value isn't zero, Chrome provides
an interface to enter a time in a `%H:%M:%S` format. Since Capybara
doesn't enter the seconds when using `fill_in` with a time, the test
failed when Capybara tried to enter a time in the `%H:%M` format when
Chrome expected a time in the `%H:%M:%S` format.
To solve this last point, an alternative would be to manually provide
the format when using `fill_in` so it includes the seconds.
[1] https://caniuse.com/mdn-html_elements_input_type_datetime-local
This is consistent with the way we show the duration of a budget and its
phases. Since budgets are the section with the most recent changes in
the admin area, we're using it as a reference.
Note that, unlike budgets (which are shown to finish at the beginning of
their ending day), a poll has always been considered to finish at the
end of their ending day, so we're showing it this way.
We're also solving a minor usability issue. While it's pretty intuitive
that a poll starting on a certain date will start at the beginning of
the day, a poll ending on a certain date isn't clear about when it
finishes exactly: is it at the beginning of the day, or at the end of
the day?
So now we're making this point clear.
In some cases (e.g. after editing or creating a resource
with errors) the default back_link did not redirect to the
expected page.
Now we force the back_links to the index pages, so we
always get the desired redirect.
Until now, in order to edit an answer, we had to click on its title on
the table and then on the "Edit answer" link.
That was tedious and different from what we usually do in the admin
section. Furthermore, the code for the answers table was written twice
and when we modified it we forgot to update the one in the `show`
action, meaning the table here provided less information than the
information present in the answers tables.
Co-Authored-By: Javi Martín <javim@elretirao.net>
After removing a question from a poll it makes more sense to redirect to
your own poll show page in order to manage their questions.
Currently it is redirecting to the questions index page where all the
questions from all the polls are displayed and takes you completely out
of the context of the poll you are in.
In the future we will remove this index question page.
We were checking that the price of the investment isn't present on the
page. However, the text "200" could be present in either the title of
the budget or the title of the heading ("Heading 200"), for instance. So
now we're making sure we look for the text on the relevant part of the
page.
We were getting some failures because sometimes the titles which are
generated automatically (like "Budget Investment 200 title") contained
the price of the investment, and so, when testing the presence of the
price (200, in this case), the test was failing.
So we're adding a custom title to the investments we create during the
tests.
We were testing that the `calculate_winners` method does not take the
price into account; we can do the same in a model test. We already have
a different system test to check that the price isn't displayed in the
view.
The test was always passing because incompatible investments are only
shown after clicking on the "Show all" link. So now we're clicking the
link before checking the "incompatibles" table isn't shown.
I don't think "incompatibles" is a valid English word; for now, I'm
leaving it like this until a native English speaker reviews it.
We've had some tests fail after this test was executed in our CI, and
one possible reason could be that sometimes this test finished before
all its requests had finished. This could be the case with the following
code:
```
visit sdg_management_proposals_path(filter: "pending_sdg_review")
click_button "Search"
expect(page).to have_css "li.is-active h2", exact_text: "Pending"
```
Before clicking the "Search" button, the expectation is already true, so
there's a chance that the expectation is evaluated as true before the
request has finished. That might result in requests and session data
leaking between tests.
So we're adding more expectations in order to make sure that the
requests have finished before evaluating the expectations associated to
them.
All the code in the `bin/` and the `config/` folders has been generated
running `rake app:update`. The only exception is the code in
`config/application.rb` where we've excluded the engines that Rails 6.0
has added, since we don't use them.
There are a few changes in Active Storage which aren't compatible with
the code we were using until now.
Since the method to assign an attachment in ActiveStorage has changed
and is incompatible with the hack we used to allow assigning `nil`
attachments, and since ActiveStorage now supports assigning `nil`
attachments, we're removing the mentioned hack. This makes the
HasAttachment module redundant, so we're removing it.
Another change in ActiveStorage is files are no longer saved before
saving the `ActiveStorage::Attachment` record. This means we need to
manually upload the file when using direct uploads. We also have to
change the width and height validations we used for images; however,
doing so results in very complex code, and we currently have to write
that code for both images and site customization images.
So, for now, we're just uploading the file before checking its
dimensions. Not ideal, though. We might use active_storage_validations
in the future to fix this issue (when they support a proc/lambda, as
mentioned in commit 600f5c35e).
We also need to update a couple of tests due to a small change in
response headers. Now the content disposition returns something like:
```
attachment; filename="budget_investments.csv"; filename*=UTF-8''budget_investments.csv
```
So we're updating regular expression we use to check the filename.
Finally, Rails 6.0.1 changed the way the host is set in integration
tests [1] and so both `Capybara.app_host` and `Capybara.default_host`
were ignored when generating URLs in the relationable examples. The only
way I've found to make it work is to explicitely assign the host to the
integration session. Rails 6.1 will change this setup again, so maybe
then we can remove this hack.
[1] https://github.com/rails/rails/pull/36283/commits/fe00711e9
While this bug was already present in the general admin search, the
combination of both search and filters was very uncommon. I've only
found this combinations in the users section, where you've got the
"erased" filter, but in this case searching for erased users doesn't
really make sense since their username and email have been deleted and
so there's nothing to find.
So the hidden content seemed to be the only affected section. However,
we're adding the field to every section so we don't have to make sure we
add it when we need it (like we did in the SDGManagement section).
Rubocop was complaining about a Layout/ExtraSpacing in a couple of
places.
These issues weren't detected by Pronto because they didn't affect lines
changed in the pull request. These lines were fine until we removed the
lines next to them in commits 4b42a68b6 and 00f0c4410.
This test has been failing many times because it checks the database
after starting the process running the browser.
We're disabling JavaScript like we do in every other test using the
`create_proposal_notification` method. This way, Capybara will use the
rack driver and there'll be no risk of errors that currently might take
place if both the process running the browser and the process running the
test access the database.
When we perform database queries in tests after the process running the
browser has started, we sometimes get failures in our test suite due to
both the tests and the browser accessing the database at the same time.
Furthermore, using `Poll.all` results in a database query, and doing so
after the process running the browser has started might result in
failures when running our test suite.
We were clicking on the "Sign in" link right after clicking on the "Sign
out" link, which might result in simultaneous requests and exceptions
when running our test suite.
So we're adding an expectation to make sure the first request has
finished before starting the following one.
We were missing a notice in this case. Not only this caused
inconsistencies in the user experience, but it also made it hard to add
an expectation in the test checking the request had finished before
making a new one. Simultaneous requests sometimes cause failures in our
test suite.
Currently with both seeds and dev_seeds, not only was this email not
displayed from the system emails section, but it also caused an error in
the application.
@email_to had an empty value and in the view we tried to access
@email_to.name which caused the error. We kept the same logic but
added the current_user to make sure it always has a valid value. We add
the current_user because the current_user is always present in this controller..
"Participate in the final voting" was a concept which was hard to
understand since many people would think it was related to the
voting/polls section and that somehow there was going to be a "final"
poll.
So we use "Vote for budget projects" instead.
Thanks Pomerange for the suggestion.
We had the same texts four times, with slight variations in the case of
the management section.
We're unifying them under the "verification" i18n namespace, since the
texts are about actions which can be done depending on whether users are
verified or not.
Note the names of the i18n keys aren't very consistent, since we use
"debates" in plural but "proposal" in singular. We're leaving it like
this so existing translations aren't affected.
