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.
When we first started caching the stats, generating them was a process
that took several minutes, so we never expired the cache.
However, there have been cases where we run into issues where the stats
shown on the screen were outdated. That's why we introduced a task to
manually expire the cache.
But now, generating the stats only takes a few seconds, so we can
automatically expire them every day, remove all the logic needed to
manually expire them, and get rid of most of the issues related to the
cache being outdated.
We're expiring them every day because it's the same day we were doing in
public stats (which we removed in commit 631b48f58), only we're using
`expires_at:` to set the expiration time, in order to simplify the code.
Note that, in the test, we're using `travel_to(time)` so the test passes
even when it starts an instant before midnight. We aren't using
`:with_frozen_time` because, in similar cases (although not in this
case, but I'm not sure whether that's intentional), `travel_to` shows
this error:
> Calling `travel_to` with a block, when we have previously already made
> a call to `travel_to`, can lead to confusing time stubbing.
Since now generating stats (assuming the results aren't in the cache)
only takes a few seconds even when there are a hundred thousand
participants, as opposed to the several minutes it took to generate them
when we introduced the Cron job, we can simply generate the stats during
the first request to the stats page.
Note that, in order to avoid creating a temporary table when the stats
are cached, we're making sure we only create this table when we need to.
Otherwise, we could spend up to 1 second on every request to the stats
page creating a table that isn't going to be used.
Also note we're using an instance variable to check whether we're
creating a table; I tried to use `table_exists?`, but it didn't work. I
wonder whether `table_exists?` doesn't detect temporary tables.
According to the README [1]:
> To mask previously collected IPs, use:
> Ahoy::Visit.find_each do |visit|
> visit.update_column :ip, Ahoy.mask_ip(visit.ip)
> end
We're adapting the code with our version, since we use the `Visit` model
instead of the `Ahoy::Visit` model.
[1] https://github.com/ankane/ahoy/blob/v5.0.2/README.md#ip-masking
We moved this file to `app/lib/` in commit cb477149c so it would be in
the autoload_paths. However, this class is loaded by ActiveStorage, with
the following method:
```
def resolve(class_name)
require "active_storage/service/#{class_name.to_s.underscore}_service"
ActiveStorage::Service.const_get(:"#{class_name.camelize}Service")
rescue LoadError
raise "Missing service adapter for #{class_name.inspect}"
end
``
So this file needs to be in the $LOAD_PATH, or else ActiveStorage won't
be able to load it when we disable the `add_autoload_paths_to_load_path`
option, which is the default in Rails 7.1 [1].
Moving it to the `lib` folder solves the issue; as mentioned in the
guide to upgrade to Rails 7.1 [2]:
> The lib directory is not affected by this flag, it is added to
> $LOAD_PATH always.
However, we were also referencing this class in the `Tenant` model,
meaning we needed to autoload it as well somehow. So, instead of
directly referencing this class, we're using `respond_to?` in the Tenant
model.
We're changing the test so it fails when the code calls
`is_a?(ActiveStorage::Service::TenantDiskService)`. We need to change
the active storage configurations in the test because, otherwise, the
moment `ActiveStorage::Blob` is loaded, the `TenantDiskService` class is
also loaded, meaning the test will pass when using `is_a?`.
Note that, since this class isn't in the autoload paths anymore, we need
to add a `require` in the tests. We could add an initializer to require
it; we're not doing it in order to be consistent with what ActiveStorage
does: it only loads the service that's going to be used in the current
Rails environment. If somebody changed their production environment in
order to use (for example), S3, and we added an initializer to require
the TenantDiskService, we would still load the TenantDiskService even if
it isn't going to be used.
[1] https://guides.rubyonrails.org/v7.1/configuring.html#config-add-autoload-paths-to-load-path
[2] https://guides.rubyonrails.org/v7.1/upgrading_ruby_on_rails.html#autoloaded-paths-are-no-longer-in-$load-path
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
```
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.
This monkey-patch doesn't seem to be working with Zeitwerk, and we were
only using it in one place, so the easiest way to solve the problem is
to remove it.
Note that, in the process, we're changing the operation so `* 100`
appears before the division, so it's consistent with other places where
we do similar things (like the `supports_percentage` method in the
proposals helper).
We use vendor/assets and app/assets; the purpose of lib/assets isn't
that clear, though. According to the Rails guides:
> lib/assets is for your own libraries' code that doesn't really fit
> into the scope of the application or those libraries which are shared
> across applications.
So it must be something for companies having several Rails applications,
which isn't our case. Furthermore, this text has been removed from the
Rails guides for version 7.1, so this folder might be a legacy folder.
Bumps [rubocop-rails](https://github.com/rubocop/rubocop-rails) from 2.20.2 to 2.21.2.
- [Release notes](https://github.com/rubocop/rubocop-rails/releases)
- [Changelog](https://github.com/rubocop/rubocop-rails/blob/master/CHANGELOG.md)
- [Commits](https://github.com/rubocop/rubocop-rails/compare/v2.20.2...v2.21.2)
---
updated-dependencies:
- dependency-name: rubocop-rails
dependency-type: direct:development
update-type: version-update:semver-minor
...
Note version 2.21.0 relaxes the default `Include` path for
`Rails/FindEach`, and so this version can find and correct offenses
outside the `app/models/` folder [1].
Also note this version replaces `unless something.include?` with `if
something.exclude?`; since we don't use the `exclude?` method anywhere,
we're removing the `include?` method from the list of methods checked by
this cop.
Finally, the Rails/HttpStatus method now returns a false positive when
rendering a dashboard partial and passing the `status` variable. In
order to avoid this issue, we could change the name of the local
variable or move the partial to a component, but for now we're simply
excluding these files for this cop.
[1] https://github.com/rubocop/rubocop-rails/pull/1059/commits/0066b3505
Signed-off-by: dependabot[bot] <support@github.com>
We were already opening them in the same window because we were
accidentall sanitizing the `target` attribute, but now we're making the
point more explicit.
We used to open these links in new tabs, but accidentally stopped doing
so in commit 75a28fafc.
While, in general, automatically opening a link in a new tab/window is a
bad idea, the exception comes when people are filling in a form and
there are links to pages that contain information which will help them
fill in a form.
There are mainly two advantages of this approach. First, it makes less
likely for people to accidentally lose the information they were filling
in. And, second, having both the form and a help page open at the same
time can make it easier to fill in the form.
However, opening these links in new tabs also has disadvantages, like
taking control away from people or making it harder to navigate through
pages when using a mobile phone.
So this is a compromise solution.
This syntax has been added in Ruby 3.1.
Not using a variable name might not be very descriptive, but it's just
as descriptive as using "block" as a variable name. Using just `&` we
get the same amount of information than using `&block`: that we're
passing a block.
We're still using `&action` in `around_action` methods because here we
aren't using a generic name for the variable, so (at least for now) we
aren't running this cop on controllers using `around_action`.
This rule was added in rubocop 1.44.0. It's useful to avoid accidental
`unless !condition` clauses.
Note we aren't replacing `unless zero?` with `if nonzero?` because we
never use `nonzero?`; using it sounds like `if !zero?`.
Replacing `unless any?` with `if none?` is only consistent if we also replace
`unless present?` with `if blank?`, so we're also adding this case. For
consistency, we're also replacing `unless blank?` with `if present?`.
We're also simplifying code dealing with `> 0` conditions in order to
make the code (hopefully) easier to understand.
Also for consistency, we're enabling the `Style/InverseMethods` rule,
which follows a similar idea.
Note we're excluding a few files:
* Configuration files that weren't generated by us
* Migration files that weren't generated by us
* The Gemfile, since it includes an important comment that must be on
the same line as the gem declaration
* The Budget::Stats class, since the heading statistics are a mess and
having shorter lines would require a lot of refactoring
For the HashAlignment rule, we're using the default `key` style (keys
are aligned and values aren't) instead of the `table` style (both keys
and values are aligned) because, even if we used both in the
application, we used the `key` style a lot more. Furthermore, the
`table` style looks strange in places where there are both very long and
very short keys and sometimes we weren't even consistent with the
`table` style, aligning some keys without aligning other keys.
Ideally we could align hashes to "either key or table", so developers
can decide whether keeping the symmetry of the code is worth it in a
case-per-case basis, but Rubocop doesn't allow this option.
* Add Tables option to Redcarpet in Legislation draft
* Allow table tags in Admin Legislation Sanitizer
* Add Test to render markdown tables in Legislation drafts
* Add Test for Admin Legislation Sanitizer
We include test for image, table and h1 to h6 tags and additional tests to strengthen the allowed and disallowed parameters
* Add Table from markdown test in System and Factories
* Add test to render tables for admin user
* Remove comment line about Redcarpet options
* Edit custom css for legislation draft table to make it responsive
It has been detected that for the :pt-BR, :zh-CN and :zh-TW locales,
the translate button was being displayed, but when requesting the
translation, the remote translation validation failed due to:
'''
validates :locale, inclusion: { in: ->(_) {
RemoteTranslations::Microsoft::AvailableLocales.available_locales }}
'''
That available_locales method did not contemplate these 3 languages
in the format used by the application.
To solve this problem the api response is mapped to return all
locales in the format expected by the application.
Add remote translation model test to ensure that a remote translation
is valid when its locale is pt-BR.
Co-Authored-By: Javi Martín <35156+javierm@users.noreply.github.com>
TranslatorText isn't compatible with Ruby 3, so we need to use a
different gem.
The 'translator-text' gem response was an array of one or more objects.
Now with the 'bing_translator' gem the response is an array with one or
several translated texts.
We remove the concept of object in the code. And we also remove the
"create_response" method from the specs since it is no longer necessary
to emulate that object and we can simply use arrays with texts to emulate
the new response.
It was added in rubocop-performance 1.13.0. We were already applying it
in most places.
We aren't adding it for performance reasons but in order to make the
code more consistent.
We were using the same logic in many different places, so we're
simplifying the code. I'm not convinced about the method names, though,
so we might change them in the future.
Note using this method for the default tenant in the `TenantDiskService`
class resulted in a `//` in the path, which is probably harmless but
very ugly and it also generates a different key than the one we got
until now. I've added an extra test to make sure that isn't the case.
Note we don't need to update the tests; the tests themselves help us
confirm that `Rails.application.secrets` and `Tenant.current_secrets`
return the same object on single-tenant applications.
While this is not strictly necessary, it can help moving the data of one
tenant to a different server or removing it.
Note we're using subfolders inside the `tenants` subfolder. If we simply
used subfolders with the schema names, if the schema names were, for
instance, language codes like `es`, `en`, `it`, ... they would conflict
with the default subfolders used by Active Storage.
Some tasks don't have to run on every tenant. The task to calculate the
TSV is only done for records which were present before we added the TSV
column, and that isn't going to happen in any tenants because we added
the TSV column before adding the tenants table. Similarly, the migration
needed for existing polls isn't necessary because there weren't any
tenants before we allowed to set the starting/ending time to polls.
We aren't adding any tests for these tasks because tests for rake tasks
are slow and tests creating tenants are also slow, making the
combination of the two even slower, particularly if we add tests for
every single task we're changing. We're adding tests for the
`.run_on_each` method instead.
The `budgets📧selected` and `budgets📧unselected` tasks are
supposed to be run manually because they only make sense at a specific
point during the life of a budget.
However, they would only run on the default tenant, and it was
impossible to run them on a different tenant.
So we're introducing an argument in the rake task accepting the name of
the tenant whose users we want to send emails to.
We were using `Budget.last`, but the last budget might not be published
yet.
I must admit I don't know whether these tasks are useful, but I'm not
removing them because I'm not sure that won't harm any CONSUL
installations.
Until now, running `db:dev_seed` created development data for the
default tenant but it was impossible to create this data for other
tenants.
Now the tenant can be provided as a parameter.
Note that, in order to be able to execute this task twice while running
the tests, we need to use `load` instead of `require_relative`, since
`require_relative` doesn't load the file again if it's already loaded.
Also note that having two optional parameters in a rake task results in
a cumbersome syntax to execute it. To avoid this, we're simply removing
the `print_log` parameter, which was used mainly for the test
environment. Now we use a different logic to get the same result.
From now on it won't be possible to pass the option to avoid the log in
the development environment. I don't know a developer who's ever used
this option, though, and it can always be done using `> /dev/null`.
This way all tenants will be able to access them instead of just the
default one.
The apartment gem recommends using a rake task instead of a migration,
but that's a solution which is primarily meant for new installations.
Migrations are easier to execute on existing installations.
However, since this migration doesn't affect the `schema.rb` file, we
still need to make sure the shared schema is created in tasks which do
not execute migrations, like `db:schema:load` or `db:test:prepare`, just
like the apartment gem recommends. That's why we're enhancing these
tasks so they execute this migration.
Note that there might be cases where the database user isn't a superuser
(as it's usually the case on production environments), meaning commands
to create, alter or drop extensions will fail. There's also the case
where users don't have permissions to create schemas, which is needed in
order to create the shared extensions schema and the schemas used by the
tenants. For these reasons, we're minimizing the number of commands, and
so we only alter or create extensions when it is really necessary.
When users don't have permission, we aren't running the commands but
showing a warning with the steps needed to run the migration manually.
This is only necessary on installations which are going to use
multitenancy; single-tenant applications upgrading don't need to run
this migration, and that's why we aren't raising exceptions when we
can't run it.
For new installations, we'll change the CONSUL installer so extensions
are automatically created in the shared schema.
Also note the plpgsql extension is not handled here. This is a special
extension which must be installed on the pg_catalog schema, which is
always in the search path and so is shared by all tenants.
Finally, we also need to change the `database.yml` file in order to
search for shared extensions while running migrations or model tests,
since none of our enabled extensions are executed during migrations;
we're also adding a rake task for existing installations. Quoting the
apartment documentation:
> your database.yml file must mimic what you've set for your default and
> persistent schemas in Apartment. When you run migrations with Rails,
> it won't know about the extensions schema because Apartment isn't
> injected into the default connection, it's done on a per-request
> basis.
This task was added in commit 2b9b78e38. According to the notes in the
pull request introducing the change:
> If you have had [map validations] in production, you might want to run
> the rake task votes:reset_vote_counter to reset the cached votes
> counter. Otherwise, until there is another vote for a proposal, the
> counter will not be updated, and there might be some votes which are
> not yet displayed
In short, this was a task that was introduced for a specific release and
had to be run manually. So we can remove it now.
This task was "temporarily" removed in commit 7b6619528. Since that was
done three and a half years ago, right after the dashboard was
introduced, I think it's time to make this "temporary" measure a bit
more permanent ;).
