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.
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.
Using <h3> headings for the links had two disadvantages.
First, it was the wrong heading level to use, since there was no <h2>
tag before it.
Second, headings are supposed to be followed by content associated to
that heading; here, we had no content following the headings.
So we're using a list of links and giving it a heading. We're adding
styles so the page still looks like it used to, although these styles
are certainly asking for improvements.
In commit b3f570512, we changed the key generator hash digest class, and
we wrote:
> Since we haven't seen any Consul Democracy applications using
> encrypted messages and these messages become invalid with this change
> (...)
We didn't realize that ActiveStorage also used the old hash digest class
to generated the signed URLs used to access an image. This doesn't
affect us when we generate images using `image.variant`, because that
generates a new URL on the fly using the new hash digest class. However,
URLs referencing the images generated using the old hash digest class,
like the ones in the HTML content generated with CKEditor, would result
in 404 errors.
So we're rotating the signed IDs generated by earlier versions of
ActiveStorage. This way both new and old images will be correctly
displayed.
Note that, unlike cookies, which will keep working once rotated even if
we delete the code to rotate them, old ActiveStorage URLs will always
need the code rotating them in order to keep working.
Note that we used to have the link to delete images inside the same
<form> tag as the button to update the image. However, using a button
means we're adding a new <form> tag for the action to delete the image.
This isn't valid HTML and, in some browsers, might result in the button
sending the request to the wrong URL.
As explained in commit 5311daadf, to avoid this, we'd need to replace
`button_to` with `button_tag` in the action in order to generate a
button without a form. Then, we could add either a `form` or a
`formaction` attribute to the button.
However, I thik it's easier to move the delete button outside the update
button <form> tag. On the minus side, since the buttons no longer share
a parent, they're harder to style. So we're using a mix of nested flex
layouts with one of the nested elements using a container unit as width.
Since we're at it, we're also improving the styles on small and medium
screens by making sure the "Update" button wraps before the "Delete"
button does (using a container query), by giving enough width to the
column containing this actions on small screens as well (removing
`small-12` and giving it two-thirds of the width on all screen sizes)
and by having a gap between elements.
Note that, at the time of writing, container queries are only supported
by about 91%-93% of the browsers, meaning that some administrators will
see all from controls displayed vertically, one on top of the other, on
all screen sizes. We think this is acceptable, and the page remains
fully functional in this case.
As mentioned in commits 5311daadf and bb958daf0, using links combined
with JavaScript to generate POST (or, in this case, DELETE) requests to
the server has a few issues.
Note that the AJAX response stopped working after replacing the link
with a button. Not sure about the reason, but, since this is one of the
very few places where we use AJAX calls to delete content, the easiest
solution is to stop using AJAX and be consistent with what we do in the
rest of the admin section.
As mentioned in commits 5311daadf and bb958daf0, using links combined
with JavaScript to generate POST (or, in this case, DELETE) requests to
the server has a few issues.
We're keeping the old `apply_stylesheet_media_default` option behavior
because removing `media="screen"` from our stylesheets would completely
break our `print` stylesheet, which would now load the default the
styles defined in `application.css`.
We're also keeping the old `:mini_magick` option to process images so
existing installations don't have to install libvips on their server. We
might change it in the future.
This option was deprecated in Rails 7.0 and removed in Rails 7.1 [1]. It
doesn't really affect us because we weren't using `to_s` with a
parameter anywhere in the application.
The Rubocop rule Rails/ToSWithArgument can be used to detect these cases
but, since we've never used them, and adding them now would cause the
application to crash and so it'll be obvious we've done something wrong,
I don't think it's necessary to add the rule.
[1] https://github.com/rails/rails/commit/e420c3380
This doesn't really affect us because we don'thave any multiple file
inputs in the application, but we're enabling it because it's the new
default configuration option.
Setting it to `true` was deprecated in Rails 7.0 and the option was was
removed in Rails 7.1, so in Rails 7.1 applications it isn't possible to
set it to `true` [1]. So we're setting it to `false` now.
[1] https://github.com/rails/rails/commit/689b27773
According to the Rails configuration guide [1], with this format, Rails
serializes cache entries more efficiently. Most importantly:
> All formats are backward and forward compatible, meaning cache entries
> written in one format can be read when using another format. This
> behavior makes it easy to migrate between formats without invalidating
> the entire cache.
[1] https://guides.rubyonrails.org/v7.1/configuring.html#config-active-support-cache-format-version
The only change between these headers and the ones sent by Rails 6.1
application is that now the `X-XSS-Protection` header is set to zero. As
mentioned in the pull request introducing the change [1]:
> This header has been deprecated and the XSS auditor it triggered has
> been removed from all major modern browsers (in favour of Content
> Security Policy) that implemented this header to begin with (Firefox
> never did).
[1] Pull request 41769 in https://github.com/rails/rails
This configuration option disappeared in Rails 7.1 [1] (meaning it isn't
possible to set it to `false` in a Rails 7.1 application). Since it's
going to be our only option when upgrading to Rails 7.1, we're already
activating it now.
[1] https://github.com/rails/rails/commit/7b4affc78
As mentioned in the Rails configuration documentation [1] (note the link
points to the configuration guide for Rails 7.1, but only because the
documentation for this option wasn't as good in the configuration guide
for Rails 7.0; the behavior hasn't changed between these two versions),
this was done in the `wrap_parameters` initializer but now it can be
done using a new default configuration option.
[1] https://guides.rubyonrails.org/v7.1/configuring.html#config-action-controller-wrap-parameters-by-default
This way we'll add an extra layer of protection from attacks that might
cause our application to redirect to an external host.
There's one place where we're allowing redirects to external hosts,
though: administrators can link external resources in notifications, and
we're redirecting to them after marking the notification as read.
Since the tests for the remote translations controller were
(accidentally) using an external redirect, we're updating them to use a
relative URL.
As mentioned in the Rails pull request [1], the main reason for partial
inserts is no longer relevant thanks to the `ignored_columns` method
(which we haven't even needed so far).
I don't have a preference regarding this setting; we're enabling it in
order to reduce the number of settings we customize.
[1] Pull request 42769 in https://github.com/rails/rails
It looks like we can't really benefit from this rule because usually we
need to specify the option anyway (maybe `user has_many :comments` is
one of the few exceptions). We might make some changes in the code when
Rubocop changes its Rails/InverseOf rule so it doesn't report this case
when using Rails 7, but, until then, we aren't changing anything so we
don't have to deal with false positives in Rubocop.
Before Rails 7.0 was released, neither the Mail gem or Rails were
providing a default timeout for SMTP, so there was a risk of processes
being stuck while sending emails.
That's no longer the case, though; we're using version 2.8.x of the Mail
gem, which already provides a default timeout [2].
Since the default timeout provided by the Mail gem is the same as the
default timeout provided by Rails 7.0, it doesn't matter whether we
enable this option. We're enabling because it's easier to just use the
default 7.0 configuration.
[1] Issue 41244 in https://github.com/rails/rails
[2] Pull request 1427 in https://github.com/mikel/mail
Not sure whether this affects us since we use RSpec; in any case, if it
affects us, it seems like a good idea, although we'll have to watch
whether some tests start failing more often.
We aren't getting any warnings when running our test suite, which means
that gems that depended on this method (like graphql [1]) have already
added compatibility for this case.
[1] Pull request 3774 in https://github.com/rmosolgo/graphql-ruby/
This is similar to what we did in commit 00a5dc921 when upgrading to
Rails 5.2. Quoting from that commit:
> Note this change will cause all fragment caching to expire. We
> consider it acceptable considering the page where caching is most
> important (stats) is barely affected by this change, since this change
> only affects the view, and the time-consuming operations are cached in
> the model.
>
> Comments are actually affected, though, and pages with thousands of
> comments might take a few extra seconds to load the first time they're
> accessed after this change. We don't think this is going to be an
> issue on existing CONSUL installations.
Note that enabling this options means all encrypted messages and cookies
generated the application become invalid, so we're adding a cookie
rotator in order to keep sessions from expiring when upgrading the
application, as recommended in the "Upgrading Ruby on Rails" guideline
[1].
Since we haven't seen any Consul Democracy applications using encrypted
messages and these messages become invalid with this change, we're also
removing the pre-Rails 5.2 encryption to authenticate messages
(AES-256-CBC) and switching to the default one since Rails 5.2
(AES-256-GCM). Since the configured encryption is used by the cookie
rotator initializer (through the ActiveSupport::MessageEncryptor.key_len
method), at first I thought this might affect the cookie rotator, but it
doesn't: upgrading works as expected, and existing sessions are still
active.
I'm adding a comment to remove the initializer once all cookies have
been migrated. I've added "Rails 7.1" in the comment because we usually
check for these comments when upgrading Rails, but we rarely check for
them when after releasing new versions of Consul Democracy.
[1] https://guides.rubyonrails.org/v7.0/upgrading_ruby_on_rails.html#key-generator-digest-class-changing-to-use-sha256
In Rails 6.1 and earlier, `button_to` generated a <button> tag when it
received the content as a block, but an <input> tag when receiving
the content as the first parameter.
That's why we were using blocks with `button_to` most of the time; for
starters, <button> tags accept pseudocontent and so are easier to style.
In Rails 7.0, `button_to` always generates a <button> tag [1], so we're
simplifying the code what uses `button_to`, passing the content as a
first parameter instead of passing it as a block.
[1] https://guides.rubyonrails.org/v7.1/configuring.html#config-action-view-button-to-generates-button-tag
The config.file_watcher option still exists but it's no longer included
in the default environtment file. Since we don't use it, we're removing
it.
The config.assets.assets.debug option is no longer true by default [1],
so it isn't included anymore.
The config.active_support.deprecation option is now omitted on
production in favor of config.active_support.report_deprecations, which
is false by default. I think it's OK to keep it this way, since we check
deprecations in the development and test environments but never on
production environments.
As mentioned in the Rails upgrade guide, sprockets-rails is no longer a
rails dependency and we need to explicitly include it in our Gemfile.
The behavior of queries trying to find an invalid enum value has changed
[2], so we're updating the tests accordingly.
The `favicon_link_tag` method has removed the deprecated `shortcut`
link type [3], so we're updating the tests accordingly.
The method `raw_filter` in ActiveSupport callbacks has been renamed to
`filter` [4], so we're updating the code accordingly.
[1] https://github.com/rails/rails/commit/adec7e7ba87e3
[2] https://github.com/rails/rails/commit/b68f0954
[3] Pull request 43850 in https://github.com/rails/rails
[4] Pull request 41598 in https://github.com/rails/rails
This task was necessary when updating to version 2.1.0, when we
integrated Puma with Systemd. Now that all Consul Democracy
installations are using Systemd, we no longer need it.
The `database:` argument from the `connected_to` method was deprecated
in Rails 6.1 [1] and removed in Rails 7.0 [2]. The ros-apartement gem
has already introduced fixes for this issue [3][4], but there has been
no release including these fixes.
So we're applying the fix in our code.
Note that, since Apartment already overwrites the `connected_to` method
by creating a `connected_to_with_tenant` method, we're calling the
`connected_to_without_tenant` method inside the patch, which is
equivalent to ActiveRecord's original `connected_to`.
[1] Pull request 37874 in https://github.com/rails/rails
[2] Pull request 40530 in https://github.com/rails/rails/pull
[3] Pull request 194 in https://github.com/rails-on-services/apartment
[4] Pull request 243 in https://github.com/rails-on-services/apartment
These warnings appear in the logs in the development environment, and,
with Rails 7, the application will crash. When running the tests, they
would appear in the standard error ouput if we set `config.cache_classes
= false` in the test environment but, since that isn't the case, they
don't.
To reproduce these warnings (or the lack of them), start a Rails console
in development and check the log/development.log file.
Quoting from pull request 43508 in the Rails repository [1]:
> When you are running test locally, most of the time you run only a
> subset, so it's better to load as little code as possible to have a
> faster time to first test result.
>
> But when you are on CI, it's usually much preferable to eager load the
> whole application because you will likely need all the code anyway,
> and even if the test suite is split across runners, it's preferable to
> load all the code to ensure any codefile that may have side effects is
> loaded.
>
> This also ensure that if some autoloaded constants are not properly
> tested on CI, at least they'll be loaded and obvious errors (e.g.
> SyntaxError) will be caught on CI rather than during deploy.
[1] https://github.com/rails/rails/commit/db0ee287eed
In Rails 6.1, the classic autoloader is deprecated.
We were getting an error because we were using `autoload` in the
ActiveStorage plugin for CKEditor:
expected file app/lib/ckeditor/backend/active_storage.rb to define
constant Ckeditor::Backend::ActiveStorage
So we're removing the line causing the error.
Finally, we can now restore all the tests that that failed sometimes
with the classic autoloader and that we modified in commits 2af1fc72f
and 8ba37b295.
Since we're already setting `wordpress_oauth2` using the `option :name`
command in the `OmniAuth::Strategies::Wordpress` class, Devise can
automatically find the strategy. However, it wasn't working because we
were passing a string instead of a symbol.
Even though we don't load this file with Zeitwerk, we're doing it for
consistency.
If we tried to load this file using Zeitwerk, without this change we'd
get an error:
```
NameError: uninitialized constant OmniauthWordpress
```
We were getting a few errors when trying out Zeitwerk:
```
expected file lib/sms_api.rb to define constant SmsApi
expected file app/components/layout/common_html_attributes_component.rb
to define constant Layout::CommonHtmlAttributesComponent
```
In these cases, we aren't using an inflection because we also define the
`Verification::SmsController` and a few migrations containing `Html` in
their class name, and none of them would work if we defined the
inflection.
We were also getting an error regarding classes containing WYSIWYG in
its name:
```
NameError: uninitialized constant WYSIWYGSanitizer
Did you mean? WysiwygSanitizer
```
In this case, adding the acronym is easier, since we never use "Wysiwyg"
in the code but we use "WYSIWYG" in many places.
The purpose of the lib folder is to have code that doesn't necessary
belong in the application but can be shared with other applications.
However, we don't have other applications and, if we did, the way to
share code between them would be using a gem or even a git submodule.
So having both the `app/` and the `lib/` folders is confusing IMHO, and
it causes unnecessary problems with autoloading.
So we're moving the `lib/` folder to `app/lib/`. Originally, some of
these files were in the `app/services/` folder and then they were moved
to the `lib/` folder. We're using `app/lib/` instead of `app/services/`
so the upgrade is less confusing.
There's an exception, though. The `OmniAuth::Strategies::Wordpress`
class needs to be available in the Devise initializer. Since this is an
initializer and trying to autoload a class here will be problematic when
switching to Zeitwerk, we'll keep the `require` clause on top of the
Devise initializer in order to load the file and so it will be loaded
even if it isn't in the autoload paths anymore.
We're getting one warning when compiling the assets due to the code we
use from font-awesome-sass, and a lot of warnings due to the code we use
from foundation.
Since these warnings are very annoying, and we get them both when
deploying and every time we change an SCSS file in development, we're
silencing them.
I haven't found the way to pass the `quiet_deps` option to the Sprockets
processor, so I'm monkey-patching the Sass engine instead.
