When returning a collection of records in the API, we were making sure
we only returned public ones. However, when returning individual
records, we were not checking that.
In practice, this wasn't a big issue, since most `public_for_api`
methods return all records, but it could affect Consul Democracy
installations which might have customized their `public_for_api` method.
The only exception was the `budget` method, since it was returning
budgets that were still in drafting.
Back in commit c984e666f, we reorganized the code related to the GraphQL
API, but we didn't reorganize the tests.
So we're doing it now, since we're going to fix a potential issue and
add some tests for it.
These tests were always passing because they were stubbing the response
of the same method they were testing. For example, we were testing the
result of `Comment.public_for_api` and stubbing it at the same time.
So we're now stubbing the result of the associations; for example, in
order to test `Comment.public_for_api`, we're stubbing the response of
`Debate.public_for_api`. Now the tests fail if, for instance, the
implementation of `Comment.public_for_api` returns all comments.
- added 2 new types
- modified the models to get data through graphQL
- modified the corresponding spec
- also testing that hidden comments do not show up
- modified comments specs bc now it returns comments on budget
investments
The test "request a translation of an already translated text" was
failing sometimes on our CI since August 29, maybe due to a change in
GitHub Actions since the test had been passing for a year and a half and
we didn't change any code around that time (we were updating the
documentation). While the root cause is unknown, debugging shows that
sometimes (usually the first time this test is executed on our CI, and
only the first time, since running it 600 tests in a row also resulted
in only one failure) the request done by clicking on "Traducir página"
is done with a user session where the locale is in English.
This doesn't make much sense, since both user sessions are already in
Spanish (and we had either explicit or implicit expectations to confirm
that), and debugging shows that the session is indeed in Spanish during
the previous request.
In any case, we're solving it by never using English during the test,
since it wasn't necessary; it was only done that way because all the
tests on this file used the language selector to get to the Spanish
pages. We're simplifying some of the other tests the same way.
The test failure was:
```
Failure/Error: expect(page).to have_content "Se han solicitado
correctamente las traducciones"
expected to find text "Se han solicitado correctamente las traducciones"
in
"Idioma: \n
\nEnglish\nDeutsch\nEspañol\nFrançais\nNederlands\nPortuguês
brasileiro\n中文\n
Entrar\nRegistrarse\nDebates\nPropuestas\nVotaciones\nLegislación
colaborativa\nPresupuestos participativos\nODS\nAyuda\n×\nTranslations
have been correctly requested.\nPropuestas más activas\nAhora mismo no
hay propuestas\nDebates más activos\nndfrrqufrp\nVer todos los
debates\nProcesos abiertos\nAhora mismo no hay procesos
abiertos\nGobierno abierto\nEste portal usa la aplicación CONSUL
DEMOCRACY que es software de código abierto.\nParticipación\nDecide cómo
debe ser la ciudad que quieres.\nCONSUL DEMOCRACY, 2024 Política de
privacidad Condiciones de uso Accesibilidad"
```
Note that most of the text is in Spanish (as expected) but the flash
message itself is in English.
In the past, having this kind of expectations after the process running
the browser has started has resulted in flaky issues with the database
connection.
In one case, we're removing the test because there are controller tests
covering the same scenario and a system test checking what happens from
the user perspective.
In the other case, we're replacing the expectations with expectations
from the user's point of view.
We were using the same setup in these tests, and we were only changing
the expectations or adding an extra step.
Note we're also using `refresh` to simplify the code and because we were
using `select "Español", from: "Idioma:"` when that language was already
selected.
After changing the language, we were checking that certain content isn't
there.
However, the content wasn't there before changing the language either,
so the test will pass even if the request to change the language hasn't
finished.
Although this is probably OK because we aren't changing the language
using an AJAX request, and so Capybara will correctly wait until the
request is finished before finishing the test, confirming that the page
has changed after a request is something we try to do in every test.
The resolution of most devices is at least 640 pixels in at least one of
their dimensions. Since we need the width to be smaller than 640 pixels,
having a height of 479px wasn't realistic.
Doing so caused the tests checking the sticky "support proposal" tests
to fail because on 480px-high devices the sticky message covers the
whole screen.
This is a usability issue, but since I'm not sure how many people use
such a small window when browsing the web, for now I'm simply changing
the tests.
We're changing the width to 320px since it's the width required by the
Web Content Accessibility Guidelines 2.2. Quoting these guidelines [1]:
> Content can be presented without loss of information or functionality,
> and without requiring scrolling in two dimensions for:
> * Vertical scrolling content at a width equivalent to 320 CSS pixels;
> * Horizontal scrolling content at a height equivalent to 256 CSS
> pixels.
Note that, using a height of 256px, the sticky message would make the
application unusable, though. IMHO we'll have to get rid of the sticky
message sooner or later.
Since August 29, probably due to a change in the browser used by GitHub
Actions (since branches whose code we didn't change were suddenly
affected that day), many tests related to proposals are failing on
GitHub Actions. Although every time the test suite is run different
tests fail, on each run at least half a dozen tests fail.
Most tests have one thing in common: they click on an element on the
`proposals#show` page, and the click isn't done correctly.
One possible explanation is that the video included on the page causes
the page to scroll at the exact same time that Capybara is clicking on a
link, which results in a misclick.
I haven't been able to reproduce the issue on my machine, so I'm not
sure whether giving the video element a fixed height using CSS (so the
page doesn't scroll when the iframe is loaded) could solve the issue.
But, after using proposals without videos in the tests (except the tests
testing the videos themselves), all these tests are passing in the test
suite.
So, for now, we're simply removing the videos in the proposal factories.
Note this issue wasn't caused by the "no cookie" changes done in commit
ee64efe659, since running the tests in Cosul Democracy 2.1.1 (which
didn't contain those changes) also fails on GitHub Actions.
It was a bit strange to leave the end date blank and have a message
associated with the start date, so we're using presence validations
instead.
For the range validation, we're using the comparison validator included
in Rails 7.0.
Note: to avoid confusion, "answer" will mean a row in the poll_answers
table and "choice" will mean whatever is in the "answer" column of that
table (I'm applying the same convention in the code of the task).
In order make this task perform reasonably on installations with
millions of votes, we're using `update_all` to update all the answers
with the same choice at once. In order to do that, we first need to
check the existing choices and what are the possible option_ids for
those choices.
Note that, in order for this task to work, we need to remote the
duplicate answers first. Otherwise, we will run into a RecordNotUnique
exception when trying to add the same option_id to two duplicate
answers.
So we're making this task depend on the one that removes duplicate
answers. That means we no longer need to specify the task to remove
duplicate answers in the release tasks; it will automatically be
executed when running the task to add an option_id.
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.
The routes for poll questions were accidentally deleted in commit
5bb831e959 when deleting the `:show` action, and restored in commit
9871503c5e. However, the deleted code was:
```
resources :questions, only: [:show], controller: 'polls/questions' (...)
```
While the restored code was:
```
resources :questions, controller: 'polls/questions' (...)
```
Meaning we forgot to add the `only: []` option when restoring the
routes.
We also forgot to remove the `before_action` code when deleting the
`:show` action, so we're removing it now.
Note that, since poll answers belong to a user and not to a voter, we
aren't doing anything regarding poll answers. This is a separate topic
that might be dealt with in a separate pull request.
Also note that, since there are no records belonging to poll voters, and
poll voters don't use `acts_as_paranoia` and don't have any callbacks on
destroy, it doesn't really matter whether we call `destroy!` or
`delete`. We're using `delete` so there are no unintended side-effects
that might affect voters with the same `user_id` and `poll_id` on
Consul Democracy installations customizing this behavior.
Note that, when taking votes from an erased user, since poll answers
don't belong to poll voters, we were not migrating them in the
`take_votes_from` method (and we aren't migrating them now either).
We were checking we didn't have more votes than allowed in the case of
questions with multiple answers, but we weren't checking it in the case
of questions with a single answer. This made it possible to create more
than one answer to the same question. This could happen because the
method `find_or_initialize_user_answer` might initialize two answers in
different threads, due to a race condition.
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.
We were using `authorize_resource`, passing it an unnamed parameter.
When that happens, CanCanCan only checks permissions to read that
resource. But, in this case, we want to check the permission to update
that resource before the `update` action.
Most of the time, it doesn't really matter, but, for example, in our
demo we're going to restrict the locales configuration so locales cannot
be updated on the main tenant (but they can be updated on other
tenants).
We were doing it most of the time, but in some cases we were clicking
the "Sign out" link instead. These actions aren't the same, just like
using `login_as` is not the same as visiting the sign in page and
submitting the form.
Some of these tests failed sometimes because the user wasn't signed in
after using `login_as`. One possible cause could be that we weren't
adding an expectation after clicking the "Sign out" link.
So using `logout` adds consistency, simplifies the code, and might
reduce the chance of these tests failing in the future (although they
might still fail in the future because some of these tests check the
database after a `visit` call).
In this test, we were signing out after confirming a vote, but we
weren't waiting for the request to finish before using the `login_as`
method. Sometimes this test failed with a screenshot showing that the
user wasn't signed in after the `login_as` call, so this might have been
the cause.
We could use the `logout` method instead, but since we don't have any
other tests that click the "Sign out" link from the officing area, we're
leaving it as it was; we might change it to `logout` in the future if
the test keeps failing once in a while.
Note we're also replacing a `visit` call which my guess is was there
because the "Sign out" link wasn't visible due to the notice covering
it. So we're closing the notice instead.
We use caching in our application in two different ways:
1. Rails.cache.fetch in models/controllers/libraries
2. Fragment caching in the views
When using Rails.cache.fetch, we usually set an expiration date or
provide a precise way to invalidate it. If the code changes and the
information stored in the cache is different from what the new code
would return, it's usually not a big deal because the cache will expire
in an hour or a day. Until commit a4461a1a5, the statistics were an
exception to this rule, but that's no longer the case. The actual
exception to this rule are the i18n translations, but the code caching
them is very simple and hasn't changed for more than three years (when
it was written for the first time).
When using fragment caching, on the other hand, Rails automatically
invalidates the cache when the associated _view code_ changes. That is,
if a view contains cache, the view renders a partial, and the partial
changes, the cache is correctly invalidated. The cache isn't invalidated
when the code in helpers, models or controllers change, though, which
the Rails team considers a compromise solution.
However, we've been moving view partials (and even views) to components,
and the cache isn't invalidated when a component changes (it doesn't
matter whether the component Ruby file or the component ERB file
changes). That means that the cache will keep rendering the HTML
generated by the old code.
So, now, we're clearing the cache when upgrading to a new version of
Consul Democracy, as part of the release tasks. That way, institutions
upgrading to a new version don't have to worry about possible issues
with the cache due to the new code not being executed.
I was thinking of adding it to a Capistrano task, but that would mean
that every time people deploy new code, even if it's a hotfix that
doesn't affect the cache at all, the cache would be cleared, which could
be inconvenient. Doing it in a release, that usually changes thousands
of lines of code, sounds like a good compromise.
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/
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.
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.
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.
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.
