Allow selecting the time when a poll starts/ends #4989
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
javierm
force-pushed
the
add_time_to_polls
branch
9 times, most recently
from
b9b1c77
to
f80bcc4
Compare
taitus
approved these changes
Sep 14, 2022
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.
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.
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're also using the convention of using a `.` to describe class methods.
We didn't have model tests for some of these scopes, and neither scope tests nor instance method tests covered edge cases like the current date.
On the day when a poll ends, it was considered both current and recounting.
This was the only place where we used the `? <= field` format; we use the `field >= ?` format everywhere else. We're also adding a variable to make it easier to understand that the value is the same in both cases and to make the line shorter ;).
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
When we create a poll with "create(:poll)" it is already a current poll.
javierm
force-pushed
the
add_time_to_polls
branch
from
f921255
to
d0359d5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Objectives
Visual Changes
Before these changes:
On Chrome:
On Firefox:
On Safari:
On Chrome for Android:
After these changes:
On Chrome:
On Firefox:
On Safari:
On Chrome for Android:
Notes
At the time of writing, this feature works on at least 86.5% of the browsers. However, among the other 13.5%, 11.25% are versions of mobile browsers with unknown support for this feature, so we think browser support is good enough to include it.
For browsers not supporting this feature, we provide a fallback so a date field is displayed. Note that, in this case, the poll with end at the beginning of the specified date, instead of at the end of the specified date, which was the behavior we had until now. For this reason, we're adding a rake task to update existing data.