Adds rake task "polls:remove_duplicate_partial_results" to delete duplicated
rows in "poll_partial_results" made before the DB was strict about duplicates.
Duplicates are considered only for records without "option_id", grouping by:
(question_id, booth_assignment_id, date, answer). We keep the first one and
delete the rest, per tenant.
The controller use:
Poll::PartialResult.find_or_initialize_by(booth_assignment_id, date, question_id, answer)
which is not a strong protection against race conditions. Without a unique
index at the DB level, duplicates could be created. This task cleans up any
existing duplicates.
Similar to what we did in PR "Avoid duplicate records in poll answers" 5539,
specifically in commit 503369166, we want to stop relying on the plain text
"answer" and start using "option_id" to avoid issues with counts across
translations and to add consistency to the poll_partial_results table.
Note that we also moved the `possible_answers` method from Poll::Question to
Poll::Question::Option, since the list of valid answers really comes from the
options of a question and not from the question itself. Tests were updated
to validate answers against the translations of the assigned option.
Additionally, we renamed lambda parameters in validations to improve clarity.
Note that Rails 7.1 changes `find_or_create_by!` so it calls
`create_or_find_by!` when no record is found, meaning we'll rarely get
`RecordNotUnique` exceptions when using this method during a race
condition.
Adding this index means we need to remove the uniqueness validation.
According to the `create_or_find_by` documentation [1]:
> Columns with unique database constraints should not have uniqueness
> validations defined, otherwise create will fail due to validation
> errors and find_by will never be called.
We're adding a test that checks what happens when using
`create_or_find_by!`.
Note that we're creating voters combining `create_with` with
`find_or_create_by!`. Using `find_or_create_by!(...)` with all
attributes (including non-key ones like `origin`) fails when a voter
already exists with different values, e.g. an existing `origin: "web"`
and an incoming `"booth"`. In this situation the existing record is not
matched and the unique index raises an exception.
`create_with(...).find_or_create_by!(user: ..., poll: ...)` searches by
the unique key only and applies the extra attributes only on creation.
Existing voters are returned unchanged, which is the intended behavior.
For the `take_votes_from` method, we're handling a (highly unlikely, but
theoretically possible) scenario where a user votes at the same time as
taking voters from another user. For that, we're doing something similar
to what `create_or_find_by!` does: we're updating the `user_id` column
inside a new transaction (using a new transactions avoids a
`PG::InFailedSqlTransaction` exception when there are duplicate
records), and deleting the existing voter when we get a
`RecordNotUnique` exception.
On `Poll::WebVote` we're simply raising an exception when there's
already a user who's voted via booth, because the `Poll::WebVote#update`
method should never be called in this case.
We still need to use `with_lock` in `Poll::WebVote`, but not due to
duplicate voters (`find_or_create_by!` method will now handle the unique
record scenario, even in the case of simultaneous transactions), but
because we use a uniqueness validation in `Poll::Answer`; this
validation would cause an error in simultaneous transactions.
[1] https://api.rubyonrails.org/v7.1/classes/ActiveRecord/Relation.html#method-i-create_or_find_by
Note that, while we're no longer including them as part of the
`execute_release_2.2.0_tasks` task, we're keeping the tasks to remove
duplicate poll voters and poll options just in case there are some
unexpected issues when adding a unique database index while upgrading to
version 2.3.0. We'll remove them in version 2.4.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.
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
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.
We were already applying these rules in most cases.
Note we aren't enabling the `MultilineArrayLineBreaks` rule because
we've got places with many elements whire it isn't clear whether
having one element per line would make the code more readable.
We've noticed the following warning while testing the upgrade to
Ruby 3.0:
warning: File.exists? is deprecated; use File.exist? instead
We're adding a Rubocop rule so we don't call the deprecated method
in the future.
On my machine, seeding a tenant takes about one second, so skipping this
action when it isn't necessary makes tests creating tenants faster
(although creating a tenant still takes about 3-4 seconds on my
machine).
We had some of the logic in the ApplicationMailer. Since we're going to
use this logic in more places, we're moving it to the Tenant model,
which is the model handling everything related to hosts.
Note that the `sitemap:refresh` task only pings search engines at the
end, so it only does so for the `Sitemap.default_host` defined last. So
we're using the `sitemap:refresh:no_ping` task instead and pinging
search engines after creating each sitemap.
Note we're pinging search engines in staging and preproduction
environments. I'm leaving it that way because that's what we've done
until now, but I wonder whether we should only do so on production.
Since we're creating a new method to get the current url_options, we're
also using it in the dev_seeds.
Testing that the sitemap is valid (which we test in the following test)
also checks that the sitemap has been generated. The test will also fail
with different errors depending on whether no file was generated or the
generated file is invalid.
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 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 ;).
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
All the code in the `bin/` and the `config/` folders has been generated
running `rake app:update`. The only exception is the code in
`config/application.rb` where we've excluded the engines that Rails 6.0
has added, since we don't use them.
There are a few changes in Active Storage which aren't compatible with
the code we were using until now.
Since the method to assign an attachment in ActiveStorage has changed
and is incompatible with the hack we used to allow assigning `nil`
attachments, and since ActiveStorage now supports assigning `nil`
attachments, we're removing the mentioned hack. This makes the
HasAttachment module redundant, so we're removing it.
Another change in ActiveStorage is files are no longer saved before
saving the `ActiveStorage::Attachment` record. This means we need to
manually upload the file when using direct uploads. We also have to
change the width and height validations we used for images; however,
doing so results in very complex code, and we currently have to write
that code for both images and site customization images.
So, for now, we're just uploading the file before checking its
dimensions. Not ideal, though. We might use active_storage_validations
in the future to fix this issue (when they support a proc/lambda, as
mentioned in commit 600f5c35e).
We also need to update a couple of tests due to a small change in
response headers. Now the content disposition returns something like:
```
attachment; filename="budget_investments.csv"; filename*=UTF-8''budget_investments.csv
```
So we're updating regular expression we use to check the filename.
Finally, Rails 6.0.1 changed the way the host is set in integration
tests [1] and so both `Capybara.app_host` and `Capybara.default_host`
were ignored when generating URLs in the relationable examples. The only
way I've found to make it work is to explicitely assign the host to the
integration session. Rails 6.1 will change this setup again, so maybe
then we can remove this hack.
[1] https://github.com/rails/rails/pull/36283/commits/fe00711e9
We were using this hack in order to allow `File.new` attachments in
tests files. However, we can use the `fixture_file_upload` helper
instead.
Just like it happened with `file_fixture`, this helper method doesn't
work in fixtures, so in this case we're using `Rack::Test::UploadedFile`
instead.
This way we don't have to write `"spec/fixtures/files"` every time.
Note this method isn't included in factories. We could include it like
so:
```
FactoryBot::SyntaxRunner.class_eval do
include ActiveSupport::Testing::FileFixtures
self.file_fixture_path = RSpec.configuration.file_fixture_path
end
```
However, I'm not sure about the possible side effects, and since we only
use attachments in a few factories, there isn't much gain in applying
the monkey-patch.
This fixes a few issues we've had for years.
First, when attaching an image and then sending a form with validation
errors, the image preview would not be rendered when the form was
displayed once again. Now it's rendered as expected.
Second, when attaching an image, removing it, and attaching a new
one, browsers were displaying the image preview of the first one. That's
because Paperclip generated the same URL from both files (as they both
had the same hash data and prefix). Browsers usually cache images and
render the cached image when getting the same URL.
Since now we're storing each image in a different Blob, the images have
different URLs and so the preview of the second one is correctly
displayed.
Finally, when users downloaded a document, they were getting files with
a very long hexadecimal hash as filename. Now they get the original
filename.
There could be inconsistencies in the database and an attachment might
have a `record_id` pointing to a record which no longer exist. We were
getting an exception in this case.
Just like we add the `storage_` prefix for new records so we can use
both Active Storage and Paperclip at the same time.
Now the migration actually works, at least for basic cases.
Our previous system to delete cached attachments didn't work for
documents because the `custom_hash_data` is different for files created
from a file and files created from cached attachments.
When creating a document attachment, the name of the file is taken into
account to calculate the hash. Let's say the original file name is
"logo.pdf", and the generated hash is "123456". The cached attachment
will be "123456.pdf", so the generated hash using the cached attachment
will be something different, like "28af3". So the file that will be
removed will be "28af3.pdf", and not "123456.pdf", which will still be
present.
Furthermore, there are times where users choose a file and then they
close the browser or go to a different page. In those cases, we weren't
deleting the cached attachments either.
So we're adding a rake task to delete these files once a day. This way
we can simplify the logic we were using to destroy cached attachments.
Note there's related a bug in documents: when editing a record (for
example, a proposal), if the title of the document changes, its hash
changes, and so it will be impossible to generate a link to that
document. Changing the way this hash is generated is not an option
because it would break links to existing files. We'll try to fix it when
moving to Active Storage.
When we assigned `I18n.available_locales = default_locales` in the
`ensure` block, `I18n.locale` was set to `:zh-TW`, which is not one of
the default locales.
In some cases this resulted in tests failing:
```
I18n::InvalidLocale:
:"zh-TW" is not a valid locale
```
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>
