This is consistent to what we usually do. Also, we're applying the same
criteria mentioned in commit 72704d776:
> We're also making these actions idempotent, so sending many requests
> to the same action will get the same result, which wasn't the case
> with the `toggle` action. Although it's a low probability case, the
> `toggle` action could result in [selecting a proposal] when trying to
> [deselect] it if someone else has [deselected it] it between the time
> the page loaded and the time the admin clicked on the "[Selected]"
> button.
The button to select/deselect a proposal wasn't very intuitive; for
example, it wasn't obvious that pressing a button saying "selected"
would deselect the proposal.
So we're using a switch control, like we do to enable/disable features
since commit fabe97e50.
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.
Until now, we've stored the text of the answer somebody replied to. The
idea was to handle the scenarios where the user voters for an option but
then that option is deleted and restored, or the texts of the options
are accidentally edited and so the option "Yes" is now "Now" and vice
versa.
However, since commit 3a6e99cb8, options can no longer be edited once
the poll starts, so there's no risk of the option changing once somebody
has voted.
This means we can now store the ID of the option that has been voted.
That'll also help us deal with a bug introduced int 673ec075e, since
answers in different locales are not counted as the same answer. Note we
aren't dealing with this bug right now.
We're still keeping (and storing) the answer as well. There are two
reasons for that.
First, we might add an "open answer" type of questions in the future and
use this column for it.
Second, we've still got logic depending on the answer, and we need to be
careful when changing it because there are existing installations where
the answer is present but the option_id is not.
Note that we're using `dependent: nullify`. The reasoning is that, since
we're storing both the option_id and the answer text, we can still use
the answer text when removing the option. In practice, this won't matter
much, though, since we've got a validation rule that makes it impossible
to destroy options once the poll has started.
Also note we're still allowing duplicate records when the option is nil.
We need to do that until we've removed every duplicate record in the
database.
It was confusing to have the action to create an answer in
`QuestionsController#answer` while the action to destroy it was
`AnswersController#destroy`.
Most existing Consul Democracy installations will have changed their
`config.i18n.available_locales` option so only a few locales are
available. In many cases, only one locale will be available. In these
cases, rendering a form that only offers one option is useless.
We've considered adding a text in this case mentioning that, in order to
enable more languages, they need to configure their
`config.i18n.available_locales`. However, we haven't done it for two
reasons.
First, if they've changed the available locales to just one, there's a
good chance they aren't interested at all in configuring the locales.
And, second, if there's only one available locale, administrators will
learn to ignore the "languages" link, so they won't realize that locales
can be configured if developers change the available locales. If we hide
the link, on the other hand, they will notice that locales can now be
configured once developers change the available locales.
Note we're still allowing access by entering the URL. This is harmless,
though, since people accessing it this way will see a form with only one
possible option and won't be able to modify anything.
Having a class named `Poll::Question::Answer` and another class named
`Poll::Answer` was so confusing that no developer working on the project
has ever been capable of remembering which is which for more than a few
seconds.
Furthermore, we're planning to add open answers to polls, and we might
add a reference from the `poll_answers` table to the
`poll_question_answers` table to property differentiate between open
answers and closed answers. Having yet another thing named answer would
be more than what our brains can handle (we know it because we did this
once in a prototype).
So we're renaming `Poll::Question::Answer` to `Poll::Question::Option`.
Hopefully that'll make it easier to remember. The name is also (more or
less) consistent with the `Legislation::QuestionOption` class, which is
similar.
We aren't changing the table or columns names for now in order to avoid
possible issues when upgrading (old code running with the new database
tables/columns after running the migrations but before deployment has
finished, for instance). We might do it in the future.
I've tried not to change the internationalization keys either so
existing translations would still be valid. However, since we have to
change the keys in `activerecord.yml` so methods like
`human_attribute_name` keep working, I'm also changing them in places
where similar keys were used (like `poll_question_answer` or
`poll/question/answer`).
Note that it isn't clear whether we should use `option` or
`question_option` in some cases. In order to keep things simple, we're
using `option` where we were using `answer` and `question_option` where
we were using `question_answer`.
Also note we're adding tests for the admin menu component, since at
first I forgot to change the `answers` reference there and all tests
passed.
Just like we do every else (sometimes even on that very same file), we
use the method instead of the instance variable.
We're doing this change now because we're about to modify one of these
files (the poll question answers documents index component).
This way we remove a bit of duplication.
These changes also affect the way geozones are rendered in a couple of
minor ways, making them more consistent:
* No empty list of geozones is rendered when there are no geozones
(before these changes, an empty list was rendered in the index action
but not in the show action)
* The text clarifying the geozone restriction is always shown (before
these changes, it was shown in the index action but not in the show
action)
We've added tests for these cases.
When this code was added, in commit 1a20a3ce4, we had no validation
rules checking the presence of the start and end dates of a poll. Now we
do, so we don't have to check this condition in the view.
We were using a redundant `elsif` instead of an `else`. We were also
using a negative condition in the main `if`, which made the code a bit
harder to read.
Since we usually use `current_user` instead of `user_signed_in?`, we're
also changing that for consistency.Extract component to render the status of a poll
This is consistent with the way we've got partials to render debates,
proposals and legislation processes on their index pages.
Note that, while adding the tests for the status icon, we're keeping one
system test because it also tests the process of voting. We're adding a
new, similar component test, where the voter is created in the database,
so all possible statuses are tested in the component.
With this parameter, Vimeo no longer uses cookies that identifies users
browsing our site.
They do still store some cookies, though; quoting from Vimeo player
parameters overview:
> When DNT is enabled, Vimeo deploys one essential cookie via the
> embeddable player:
> The __cf_bm cookie, which is part of Cloudflare's Bot Management
> service and helps mitigate risk associated with spam and bot traffic.
Not sure whether this counts as essential cookies in our case; they're
essential for Vimeo, but for us, they're third-party cookies, after all.
[1] https://help.vimeo.com/hc/en-us/articles/12426260232977-Player-parameters-overview
When embedding a video in our site YouTube stores cookies in the user's
computer that aren't necessary to watch the video, so we'd have to make
people accept those cookies before letting them watch the video.
Using a URL that doesn't use cookies, like mentioned in YouTube Help
[1], is easier, though, and respects people's privacy without affecting
the user experience.
That I've found some references saying that youtube does store cookies
once you hit the "play" button even when using the nocookie server [2].
Not sure whether that's an old behavior or I'm doing something wrong,
but I don't see this is the case; even after playing the video, cookies
aren't stored on my browser.
[1] https://support.google.com/youtube/answer/171780#zippy=%2Cturn-on-privacy-enhanced-mode
[2] https://www.cnet.com/news/privacy/youtubes-new-nocookie-feature-continues-to-serve-cookies/
Although most Consul Democracy installations will only have a few
available languages using `config.i18n.available_locales`, there's a
chance some installation will keep every language as available and will
enable the desired ones using the admin interface. In these cases,
enabling (or disabling) every language would be tedious, particularly
when casually experimenting in a staging environment or while using the
official Consul Democracy demo.
So we're adding buttons to simplify the process. Since some
installations might have only a couple of available languages, and in
this case these buttons would be pretty much useless, we're only showing
them when there are many languages available.
Since they're related, we're making them part of the same list. Instead
of finding a way to have the `Select` prefix they had as a label for the
list, we're including the "prefix" they had inside their texts, so the
text of a button doesn't need any additional context.
People using screen readers usually expect links to take them somewhere
else in the page on to a different page, while they expect buttons to
change something on the page.
Since we're in the latter scenario, using a button is more accessible.
It's also more natural; with a button, we don't need to provide `#` as
the URL or stop the default event when the button is clicked. And,
unlike links, buttons can be activated with either the space or the
enter key. Finally, clicking a link pointing to `#` with the middle
mouse button opens a useless new tab, while buttons do nothing in this
case.
Now that we only have one "All" link on the page, we no longer need to
specify which "All" link we're clicking or which "All" link we are
checking, so we're simplifying the code doing so.
Since we don't usually style HTML classes starting with `js-`, we're
renaming it, so it's consistent with the `CheckAllNone` name used in the
`check_all_none.js` file.
This way we can simplify the view, particularly the form. However, we're
still adding some complexity to the form so inputs are inside labels and
so the collection is easier to style with CSS.
We're using different controls depending on the number of available
locales.
When there are only a few locales, the solution is obvious: radio
buttons to select the default language, and checkboxes to select the
available ones are simple and intuitive.
With many languages, showing two consecutive lists of 30 languages could
be confusing, though, particularly on small devices, where scrolling
through both lists could be hard.
So, in this case, we're rendering a <select> to choose the default
language. For selecting the available languages, however, we're sticking
to checkboxes because all the other existing options (like multiple
selects) are hard to use. We think it's OK because the form doesn't have
any additional fields, and there's only one big list of options to
scroll through.
While testing the application, we noticed that if we use the
`admin-fieldset-separator` styles when there's only one fieldset, it's
harder to notice that there's an additional field to select the default
language. So we're only using the `admin-fieldset-separator` styles when
all the fields are grouped in fieldsets.
Regarding the help text for the fieldset, if we leave the help text
outside the <legend> tag, people using screen readers won't hear about
this content. However, if we include it inside the <legend> tag, some
screen readers might read it every time they move to a different
checkbox (or radio button), which can be annoying. Since I don't think
these help messages are really essential, I'm leaving them out of the
<legend> tag. It's also easier to style them if they're outside the
<legend> tag.
Note we're using `display: table` for the labels, for the reasons
mentioned in commit 923c2a7ee.
Also note that, when there's only one available locale, this section is
useless. In this case, we aren't disabling it for now because there's a
chance people see it in the official Consul Democracy demo and then
wonder why it isn't available on their installation. We might disable it
in the future, though.
Note that, currently, we take these settings from the database but we
don't provide a way to edit them through the admin interface, so the
locales must be manually introduced through a Rails console.
While we did consider using a comma-separated list, we're using spaces
in order to be consistent with the way we store the allowed content
types settings.
The `enabled_locales` nomenclature, which contrasts with
`available_locales`, is probably subconsciously based on similar
patterns like the one Nginx uses to enable sites.
Note that we aren't using `Setting.enabled_locales` in the globalize
initializer when setting the fallbacks. This means the following test
(which we could add to the shared globalizable examples) would fail:
```
it "Falls back to an enabled locale if the fallback is not enabled" do
Setting["locales.default"] = "en"
Setting["locales.enabled"] = "fr en"
allow(I18n.fallbacks).to receive(:[]).and_return([:fr, :es])
Globalize.set_fallbacks_to_all_available_locales
I18n.with_locale(:fr) do
expect(record.send(attribute)).to eq "In English"
end
end
```
The reason is that the code making this test pass could be:
```
def Globalize.set_fallbacks_to_all_available_locales
Globalize.fallbacks = I18n.available_locales.index_with do |locale|
((I18n.fallbacks[locale] & Setting.enabled_locales) + Setting.enabled_locales).uniq
end
end
```
However, this would make it impossible to run `rake db:migrate` on new
applications because the initializer would try to load the `Setting`
model but the `settings` table wouldn't exist at that point.
Besides, this is a really rare case that IMHO we don't need to support.
For this scenario, an installation would have to enable a locale, create
records with contents in that locale, then disable that locale and have
that locale as a fallback for a language where content for that record
wasn't created. If that happened, it would be solved by creating content
for that record in every enabled language.
This parameter isn't used since commit b86c0d3c3, which deleted a file
that wasn't used since commit 146c09adb. Further proof that this code
wasn't used is the fact that the `enable_translation_style` method,
which this code called, was removed in commit 5ada97544.
In commit e51e03446, we started using the same code to show stats in the
public area and in the admin area. However, in doing so we introduced a
bug, since stats in the public area are only shown after a certain part
of the process has finished, meaning the stats appearing on the page
never change (in theory), so it's perfectly fine to cache them. However,
in the admin area stats can be accessed while the process is still
ongoing, so caching the stats will lead to the wrong results being
displayed.
We've thought about expiring the cache when new supports or ballot lines
are added; however, that means the methods calculating the stats for the
supporting phase would expire when supports are added/removed but the
methods calculating the stats for the voting phase would expire when
ballot lines are added/removed. It gets even more complex because the
`headings` method calculates stats for both the supporting and the
voting phases.
So, since loading stats in the admin section is fast even without the
cache because they only load very basic statistics, we're taking the
simple approach of disabling the cache in this case, so everything works
the same way it did before commit e51e03446.
Co-authored-by: Javi Martín <javim@elretirao.net>
This page isn't linked from anywhere and most Consul Democracy
installations don't even know it exists, so it's useless for most
people.
If we ever bring it back, we should at least add a link pointing to this
page.
Now that we've moved the logic to generate the events data to a model,
and we've got access to the model in the component rendering the chart,
we can render the data inside the chart instead of doing an extra AJAX
request to get the same data.
Originally, this was problaby done this way so the page wouldn't take
several seconds to load while preparing the data for the chart when
there are thousands of dates being displayed. With an AJAX call, the
page would load as fast as usual, and then the chart would render after
a few seconds. However, we can have an even better performance
improvement in this scenario if we use a Set instead of an Array. The
method `Array#include?`, which we were calling for every date in the
data, is much slower that `Set#merge`. So now both the page and the
chart load as fast as expected.
We could also use something like:
```
def add
(...)
shared_keys.push(*collection.keys)
end
def build
(...)
shared_keys.uniq.each do |k|
(...)
end
def shared_keys
@shared_keys ||= []
end
```
Or other approaches to avoid using `Array#include?`. The performance
would be similar to the one we get when using `Set`. We're using a `Set`
because it makes more obvious that `shared_keys` is supposed to contain
unique elements.
We've had some tests failing in the past due to these AJAX requests
being triggered automatically during the tests and no expectations
checking the requests have finished, so now we're reducing the amount of
flaky tests.
We were always displaying the event names in English.
Note we're changing the `user_supported_budgets` key because it didn't
make much sense; the investments are supported, and not the budgets.
We're also adding "created" to most of the event names in order to make
the texts more explicit, since not all the events refer to created data.
Note we're delegating the `t` method because i18n-tasks doesn't detect
code like `ApplicationController.helpers.t` and so reports we aren't
using the `admin.stats.graph` translations.
We were tracking some events with Ahoy, but in an inconsistent way. For
example, we were tracking when a debate was created, but (probably
accidentally) we were only tracking proposals when they were created
from the management section. For budget investments and their supports,
we weren't using Ahoy events but checking their database tables instead.
And we were only using ahoy events for the charts; for the other stats,
we were using the real data.
While we could actually fix these issues and start tracking events
correctly, existing production data would remain broken because we
didn't track a certain event when it happened. And, besides, why should
we bother, for instance, to track when a debate is created, when we can
instead access that information in the debates table?
There are probably some features related to tracking an event and their
visits, but we weren't using them, and we were storing more user data
than we needed to.
So we're removing the track events, allowing us to simplify the code and
make it more consistent. We aren't removing the `ahoy_events` table in
case existing Consul Democracy installations use it, but we'll remove it
after releasing version 2.2.0 and adding a warning in the release notes.
This change fixes the proposal created chart, since we were only
tracking proposals created in the management section, and opens the
possibility to add more charts in the future using data we didn't track
with Ahoy.
Also note the "Level 2 user Graph" test wasn't testing the graph, so
we're changing it in order to test it. We're also moving it next to the
other graphs test and, since we were tracking the event when we were
confirming the phone, we're renaming to "Level 3 users".
Finally, note that, since we were tracking events when something was
created, we're including the `with_hidden` scope. This is also
consistent with the other stats shown in the admin section as well as
the public stats.
