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.
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.
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.
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
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.
We were already saving it as a time, but we didn't offer an interface to
select the time due to lack of decent browser support for this field
back when this feature was added.
However, nowadays all major browsers support this field type and, at the
time of writing, at least 86.5% of the browsers support it [1]. This
percentage could be much higher, since support in 11.25% of the browsers
is unknown.
Note we still need to support the case where this field isn't supported,
and so we offer a fallback and on the server side we don't assume we're
always getting a time. We're doing a strange hack so we set the field
type to text before changing its value; otherwise old Firefox browsers
crashed.
Also note that, until now, we were storing end dates in the database as
a date with 00:00 as its time, but we were considering the poll to be
open until 23:59 that day. So, in order to keep backwards compatibility,
we're adding a task to update the dates of existing polls so we get the
same behavior we had until now.
This also means budget polls are now created so they end at the
beginning of the day when the balloting phase ends. This is consistent
with the dates we display in the budget phases table.
Finally, there's one test where we're using `beginning_of_minute` when
creating a poll. That's because Chrome provides an interface to enter a
time in a `%H:%M` format when the "seconds" value of the provided time
is zero. However, when the "seconds" value isn't zero, Chrome provides
an interface to enter a time in a `%H:%M:%S` format. Since Capybara
doesn't enter the seconds when using `fill_in` with a time, the test
failed when Capybara tried to enter a time in the `%H:%M` format when
Chrome expected a time in the `%H:%M:%S` format.
To solve this last point, an alternative would be to manually provide
the format when using `fill_in` so it includes the seconds.
[1] https://caniuse.com/mdn-html_elements_input_type_datetime-local
People who have already upgraded to version 1.3.0 don't need to execute
them again.
We're not deleting the tasks yet in case some people would like to
upgrade from version 1.2.0 to version 1.3.1. In this case, they'll have
to execute the tasks manually.
Note we're making the validation rule dynamic so it's affected by the
way we stub the constant in the tests to emulate data created in old
applications.
Co-Authored-By: Javi Martín <javim@elretirao.net>
These tasks are not needed for new installations, and in existing
installations they've already been executed when upgrading to version
1.1.
One of them also raises a warning in Rails 5.2:
DEPRECATION WARNING: Dangerous query method (method whose arguments are
used as raw SQL) called with non-attribute argument(s): "MIN(id) as id".
Non-attribute arguments will be disallowed in Rails 6.0. This method
should not be called with user-provided values, such as request
parameters or model attributes. Known-safe values can be passed by
wrapping them in Arel.sql()
If we didn't run this task, investments for existing budgets wouldn't
show their administrator/valuators as an option when we're editing them,
leading to data loss.
We were manually doing the same thing, generating inconsistent results,
since the method `valuation_tag_list` was using the `valuation` context,
when actually the expected behavior would be to use the `valuation_tag`
context.
Although it's already executed when deploying with capistrano, heroku
installations don't use capistrano for deployment, so we're also
executing it when upgrading.
This isn't a one-time task, so it makes sense to have it executed on
every release.
I was thinking of leaving these tasks empty, so in the future we could
use them again if we rename or remove more settings. But since we
haven't renamed nor removed any settings for more than seven months, and
we've only used these tasks once, I'm simply removing the tasks. It's
easy to add them back if we ever need them.
