We've been using the `url` Setting for a long time, but since then we've
added a few references to `root_url` to this file, so we're now adding
consistency. We're also removing a now unnecessary condition.
We were using `Setting["url"]` to verify the content belonged to the
application URL, but we can use `root_url` instead.
Note that means we need to include the port when filling in forms in the
tests, since in tests URL helpers like `polymorphic_url` don't include
the port, but a port is automatically added when actually making the
request.
This task was "temporarily" removed in commit 7b6619528. Since that was
done three and a half years ago, right after the dashboard was
introduced, I think it's time to make this "temporary" measure a bit
more permanent ;).
We were rendering the `new` action, but that action doesn't exist.
Before commit ec861ca8e, we were rendering the `edit` action of an
answer, which was confusing as well.
Note that, when adding an invalid document, `@answer.documents` contains
that invalid document (which is not present in the database). Since
we're rendering the index, this new document would appear in the list of
the documents that can be deleted; to avoid that, we're kind of
"reloading" the answer object in the component by finding the record in
the database. We aren't using `@answer.reload` because doing so would
remove the validation errors.
We were using the same logic in four different places, so we're creating
a new class to handle that logic.
Note that I didn't find a way to delegate the `content` method to a
`Admin::TableActionsComponent`, so we're delegating the `action` method
instead. That means we need to create a method returning an
`Admin::TableActionsComponent`. We also need to cache this object;
otherwise we were getting an error when calling `actions.action` from
the `Admin::Poll::Questions::TableActionsComponent`.
Note that the `create` action doesn't create an image but updates an
answer instead. We're removing the references to `:create` in the
abilities since it isn't used.
In the future we might change the form to add an image to an answer
because it's been broken for ages since it shows all the attached
images.
Adding, modifiying, and/or deleting questions for an already started
poll is far away from being democratic and can lead to unwanted side
effects like missing votes in the results or stats.
So, from now on, only modifiying questions will be possible only if
the poll has not started yet.
We need to update a couple of tests because a poll is created in the
tests with a timestamp that includes nanoseconds and in the form to edit
the time of the poll the nanoseconds are not sent, meaning it was
detected as a change.
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
The reason why we were displaying the ending date as "one second before
the actual ending" was that, when seeing that a phase ends at a date
like "2000-12-31 00:00", we might end up thinking that the phase will
finished at the midnight between December 31st and January the 1st,
while it actually ends at the midnight between December the 30th and
December the 31st.
This is particularly important because we use a date field to select the
date of a phase and if select December the 31st, it will be stored in
the database as "2000-12-31 00:00". So, instead, in this case we display
"2000-12-30 23:59", which is less confusing.
But now we're going to add support for setting a time on polls, which
means a certain poll might end at 15:30. In this case, displaying that
it ends at 15:29 doesn't make much sense.
We were displaying dates in two different formats in the same component,
leading to strange hacks like manually calling the `call` method or not
being able to use `render_inline` in the tests.
Since we're going to reuse one of these formats outside the budgets
section, we're splitting the component. We're also removing the
mentioned hacks.
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.
This method was introduced in Rails 6.0. It can be used to take an array
and create a hash where the elements of the array are the indexes of the
hash.
Performance tests show both methods of truncating the database take
about the same time, so we can remove one dependency and we don't lose
anything in the process.
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.
