In commit 96ae69fe9, we stopped using cookies to track Ahoy visits and
started using a combination of the IP and the browser agent instead.
However, since we're still using the legacy logic from Ahoy 1.x to track
visits (which we had to add in commit b5220effd), this way of tracking
visits doesn't work and counts every page visited by a user as an
independent visit.
Maybe we could migrate existing data, which uses the `visitor_id` column
so it uses the new `visit_token` and `visitor_token` columns, but
there's no mention in the Ahoy documentation regarding how to do so.
While deciding what to do about this, we found something interesting.
For two years, we've been seeing random failures in the
`system/admin/tenants_spec.rb` tests, with messages like:
```
1) Tenants Create Tenant with subdomain
Failure/Error:
raise TenantNotFound, <<~EXCEPTION_MESSAGE
Could not set search path to schemas, they may be invalid:
"#{tenant}" #{full_search_path}.
Original error: #{exception.class}: #{exception}
EXCEPTION_MESSAGE
Apartment::TenantNotFound:
Could not set search path to schemas, they may be invalid:
"earth" "public", "shared_extensions".
Original error:
ActiveRecord::StatementInvalid: Could not find schema earth
```
And we've found one of the causes: the AJAX requests done by Ahoy to
track visits. Sometimes a test that creates or updates a tenant finishes
but the Ahoy AJAX request to, say, `earth.lvh.me/ahoy/visits`, is
handled by the next test, when the `earth` schema no longer exists, thus
raising an `Apartment::TenantNotFound` exception.
So by disabling these AJAX requests and tracking the visits in the
server instead, we're killing two birds in one stone: we're fixing the
bug regarding the visits count and we're reducing the flakiness in our
test suite. It looks like we're also removing the "phantom ahoy cookie"
we were getting since the mentioned commit b5220effd: an ahoy cookie was
quickly set and unset in the browser.
Note that, even though we aren't migrating any data, we're still adding
the new fields, because some tests started to fail because, when
tracking visits in the server without cookies, Ahoy expects the Visit
model to have a `visit_token` field.
Note: Since we update to 1.80.1 deprecation warnings are appear when execute the assets:precompile command.
In order to silence this deprecation, we add silence_deprecation option in sass.rb initializer.
The code has also been updated to remove the deprecation warnings that appeared related to the function
darken(), lighten() and "Using / for division" instead of the function calc().
Bumps [sassc-embedded](https://github.com/sass-contrib/sassc-embedded-shim-ruby) from 1.70.1 to 1.80.1.
- [Commits](https://github.com/sass-contrib/sassc-embedded-shim-ruby/compare/v1.70.1...v1.80.1)
---
updated-dependencies:
- dependency-name: sassc-embedded
dependency-type: direct:production
update-type: version-update:semver-minor
...
Signed-off-by: dependabot[bot] <support@github.com>
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.
As mentioned in Ahoy's README [1]:
> Ahoy provides a number of options to help with GDPR compliance.
> Update config/initializers/ahoy.rb with:
>
> class Ahoy::Store < Ahoy::DatabaseStore
> def authenticate(data)
> # disables automatic linking of visits and users
> end
> end
>
> Ahoy.mask_ips = true
> Ahoy.cookies = :none
As also mentioned in the README:
> If Ahoy was installed before v5, add an index before making this
> change.
> (...)
> For Active Record, create a migration with:
> add_index :ahoy_visits, [:visitor_token, :started_at]
However, the `visitor_token` doesn't exist in our table, since we
generated the `visits` table when Ahoy used the `visitor_id` column. So
we're using this column for the index.
Note we also need to change the `visit` method, since otherwise we get
an exception [2]. As mentioned on the issue reporting the exception:
> you'll need to copy the latest version of that method and adapt it to
> your model. I believe you'll want to replace:
>
> where(visit_token: ahoy.visit_token) with
> where(id: ensure_uuid(ahoy.visit_token))
>
> where(visitor_token: ahoy.visitor_token) with
> where(visitor_id: ensure_uuid(ahoy.visitor_token))
So we're copying the latest version of that method and changing it
accordingly.
[1] https://github.com/ankane/ahoy/blob/v5.0.2/README.md
[2] Issue 549 in https://github.com/ankane/ahoy
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.
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
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.
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.
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.
This is possible since audited 5.4.3, and IMHO it's useful because
audited has added a callback on touch, which could result in strange
behavior since we might use the `touch` method to expire the cache
without doing any changes.
While this doesn't affect our application because we're using the `on:`
option of the `audited` method in the budget investment model, it'll
make it easier to enable audited for other classes in custom
installations without having to worry about the (usually undesired)
touch callbacks.
We haven't updated this initializer for years, so here's the updated
version. The `expire_auth_token_on_timeout` doesn't seem to exist
anymore, and a few more options have been added.
Note that the default Devise initializer configures
`config.responder.error_status` and `config.responder.redirect_status`
so they follow Hotwire/Turbo conventions. For now, I'm commenting these
lines because we currently don't use Hotwire/Turbo.
We haven't been updating this file for a long time so, and this version
uses the `configure` method instead of the `config` hash.
We're also adding a few comments already included in previous versions
of wicked pdf that we hadn't copied to our initializer.
As Rails does with the application log and other tools. We
are going to use the same filtering rules we use in Consul Democracy.
We are renaming the initializer file name `filter_parameter_logging.rb` so
it's loaded before the errbit initializer.
