Since we upgraded to Rails 7.0 in commmit 8596f1539, the screenshots
started to be stored in `tmp/capybara`, so the step uploading
screenshots wasn't doing 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.
We've been ignoring what the Bullet gem reports for at least 6 years
(maybe more), but we were still updating the gem and maintaining the
code in `config/environments/` (which caused conflicts every time we run
`rails app:update` to upgrade to a new Rails version). Maintaining it
isn't a huge effort, but it's infinitely bigger than the benefits we get
from it, which are zero.
Adding `includes` everywhere we query for records would be a huge
maintenance effort and would make the code less readable, so I don't
think it's worth it. We might do it occasionally if we detect a
performance bottleneck.
We could also use a gem to automatically avoid the N+1 queries problem,
like Goldiloader [1], ArLazyPreload [2] or JitPreload [3]. Benchmarks
show that the performance improvements obtained by using these gems is
about less than 10% (it depends a lot on the page being loaded, though),
which IMHO doesn't justify adding yet another gem that patches
ActiveRecord and that could be incompatible with other gems doing so.
There are a couple of open pull requests (at the time of writing,
they've been open for about two years) in the Rails repository [4][5] to
automatically avoid N+1 queries as well. For now, we'll hope something
similar is integrated in Rails itself in the future.
[1] https://github.com/salsify/goldiloader
[2] https://github.com/DmitryTsepelev/ar_lazy_preload
[3] https://github.com/clio/jit_preloader/
[4] Pull request 45231 in https://github.com/rails/rails/
[5] Pull request 45413 in https://github.com/rails/rails/
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).
We added the code indicating the table in commit 673ec075e because back
then we had a `title` column in both the `poll_question_answers` table
and the `poll_question_answer_translations` table.
Since that's no longer the case since commit 7a7877656, we can simplify
the code.
Not sure about when we stopped needing them, but all usages of
require_dependency definitely become obsolete after we started using
Zeitwerk in commit 5f24ee912.
We're also going to rename `Poll::Question::Answer` to
`Poll::Question::Option`, so the conflict of having two `Answer`
classes, that made us add this code in the first place, will not even
exist.
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.
The text wasn't gramatically correct in English, since it had
the verb before the subject.
Since I'm not a native English speaker, I'm not sure about the
correct way to mention the Census part. I'm using "residents
in" since it sounds OK to me, but I might be wrong.
Many developers run `bin/rspec` and then are (reasonably) confused when
running the tests takes too long.
So we're clarifying that running the whole test suite should be done
using a CI, and only relevant tests should be run while developing on
your machine.
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.
The rows in these tables were using the styles from the `.poll`
selector, and the `position: relative` property defined there caused the
inner borders to disappear in some browsers (like Firefox).
So we're adding the `public` class to the selector; this way, it doesn't
affect elements in the admin section.
Even though it's only necessary to add the `.public` prefix to the
`.poll` selector in one place in order to fix this issue, we're doing it
everywhere for consistency.
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/
Now we're also testing that there's an iframe with the URL; before this
change, the test would pass even if the JavaScript generating the iframe
wouldn't work.
We were using the same code, and the same regular expressions, in two
places. To do so, we were including a helper inside a model, which is
something we don't usually do.
We had an inconsistency where most stylesheets associated to a component
would have the same relative path as their component, so if we had a
component in `app/components/admin/whatever`, its associated stylesheet
would be in `app/assets/stylesheets/admin/whatever`.
There was one exception to this rule: stylesheets for components in
`app/components/shared/` were placed in `app/assets/stylesheets/`. The
reason was that we thought "well... if they're in the root folder,
they're shared". However, this is confusing because in the root folder
there are also stylesheets that aren't associated to a component.
So we're creating the `app/assets/stylesheets/shared/` folder. This also
means we don't have to manually add every stylesheet in this folder the
the `application.scss` file.
We aren't the same for JavaScript files because with JavaScript we still
don't have a clear association between JavaScript files and components.
Only a couple of them (`advanced_search.js` and `check_all_none.js`)
would be good candidates, and the one for the advanced search form
doesn't even use the `.advanced-search-form` selector that we use in the
CSS file.
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.
These buttons only work without JavaScript, so we shouldn't show them in
this case.
I was wondering whether we should use the `hidden` HTML attribute so
these buttons don't show up when stylesheets haven't loaded either. Not
doing so because we already have a stylesheet for the <noscript>
scenario. We might change our minds regarding how to handle these styles
in the future.
The float property was removed in commit b71c61e40, but then it was
added again in commit 4a6313fed.
It might have been necessary to do so back then because we had a
`select` field instead of the links to set the order, but now, instead
of making them float on the left and then make the next element clear
the floats, we can do nothing and obtain the same results.
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.
This code using the legislation-categories HTML class was removed in
commits d679c1eb7 and ff66909cd. We've noticed is while dealing with the
`.menu.simple` selectors in the previous commit.
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.
Since we were on the "Pending review" filter, and there were no records
pending review, the code checking all checkboxes were checked/unchecked
didn't test anything because there were no checkboxes on the page.
So we're clicking on the "All" filter first.
