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 ;).
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
The current consul GraphQL API has two problems.
1) It uses some unnecessary complicated magic to automatically create
the GraphQL types and querys using an `api.yml` file. This approach
is over-engineered, complex and has no benefits. It's just harder to
understand the code for people which are not familiar with the
project (like me, lol).
2) It uses a deprecated DSL [1] that is soon going to be removed from
`graphql-ruby` completely. We are already seeing deprecation warning
because of this (see References).
There was one problem. I wanted to create the API so that it is fully
backwards compatible with the old one, BUT the old one uses field names
which are directly derived from the ruby code, which results in
snake_case field names - not the GraphQL way. When I'm using the
graphql-ruby Class-based syntax, it automatically creates the fields in
camelCase, which breaks backwards-compatibility.
So I've added deprecated snake_case field names to keep it
backwards-compatible.
[1] https://graphql-ruby.org/schema/class_based_api.html
We are use a display: block style for labels containing check boxes inside
them, and the label has a width of 100%.
This means that clicking on the blank space on the right of the label text
will check/uncheck the checkbox. To avoid this behaviour we modify the
"display" attribute of the labels.
In order to prevent unexpected behaviour in terms_of_service form labels,
we add specific css for this case when define a checkbox within the
.actions class.
The application crashed when we generated hints to attributes with
interpolation arguments in their `human_attribute_name`.
When generating the hint, we used the `custom_label` method to generate
a label and get the `for` attribute and, since we weren't passing a
text, it used the default human attribute name for the field. However,
it crashes if the default attribute name requires an interpolation
argument.
So now, since we were only using the `custom_label` method in order to
get the `for` attribute, we're simply passing an arbitrary text to the
method.
The code is based on what's generated using CKEditor's code generator.
We're doing one minor change to the `Ckeditor::Backend::ActiveStorage`
module; we're assigning the data in a `before_validation` instead of a
`before_save` callback. Validations with `file_validations` didn't work
otherwise; it looks like this backend was written with
`active_storage_validations` in mind [1].
Note we don't need to update the `name` column in the attachments table
because, when using Active Storage, CKEditor uses both `data` (as
attribute accessor) and `storage_data` (as attachment attribute).
[1] https://github.com/galetahub/ckeditor/blob/f9e48420ccb6dc/lib/generators/ckeditor/templates/active_record/active_storage/ckeditor/picture.rb#L4
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.
The `parameterize` method uses the `I18n.transliterate` method, whose
documentation says:
```
I18n.transliterate("Ærøskøbing")
=> "AEroskobing"
I18n.transliterate("日本語")
=> "???"
```
That means we can't use it for dictionaries where characters don't have
a transliteration to the latin alphabet.
So we're changing the code in order to only transliterate characters
with a transliteration to the latin alphabet.
Note the first example ("Česká republika") already worked with the
previous code; the test has been added to make sure accented characters
are handled properly.
This way we don't have to use the `send` method in other places, like
the AdminNotification class, and we can change the internal
implementation at any point.
We're going to add geozones as user segments, so it's handy to have the
method in the UserSegments class.
We're also changing the `user_segment_emails` parameter name for
consistency and simplicity.
We're going to make it dynamic using the geozones. Besides, class
methods can be overwritten using custom models, while constants can't be
overwritten without getting a warning [1].
Makes the definition of segments with geozones a little cleaner. I
think it’s worth it, compared to the slight memory gain of using a
constant [2].
[1] warning: already initialized constant UserSegments::SEGMENTS
[2] https://stackoverflow.com/questions/15903835/class-method-vs-constant-in-ruby-rails#answer-15903970
The `open` method can be used to open files or URLs and it's deprecated
in Ruby 2.7. In this case, it's clear we're dealing with a URL, so we
can use `URI.parse`.
The code was a bit strange, since it returned a value and had a side
effect: opening the URL. I'm not sure about the intention of the code;
my best guess is we wanted to test the URL exists and was accessible
before returning it (and, if that's the case, IMHO the code should be a
bit more explicit in order to show the intention behind it), but it
could also be an unintended side effect which was there by accident.
Now the URL is no longer opened; if the URL isn't accessible, we'll find
out when trying to connect to it with the Savon client.
In commit 5a4921a1a we replaced `URI.parse` with `URI.open` due to some
issues during our tests with S3.
However, there are some security issues with `URI.open` [1], since it
might allow some users to execute code on the server.
So we're using `URI.parse#open` instead.
[1] https://docs.rubocop.org/rubocop/cops_security.html#securityopen
This column wasn't used in any released Consul version since it was only
used during development. For the same reason, the task to migrate the
information in the `link` column to the `links` table isn't needed
either.
It was never used and its data is based on Madrid's requirements for
successful proposals. It was written during the development of the
dashboard, probably for testing purposes.
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.
We were getting a duplicate prepared statement error when trying to
execute more than one test using this task. So we're checking whether
the prepared statement exists before defining it.
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.
This model is added by the devise-security gem, but we don't use it. We
were getting an error while migrating:
ActiveRecord::StatementInvalid: PG::UndefinedTable: ERROR: relation "old_passwords" does not exist
LINE 8: WHERE a.attrelid = '"old_passwords"'::regclas...
^
SELECT a.attname, format_type(a.atttypid, a.atttypmod),
pg_get_expr(d.adbin, d.adrelid), a.attnotnull, a.atttypid, a.atttypmod,
c.collname, col_description(a.attrelid, a.attnum) AS comment
FROM pg_attribute a
LEFT JOIN pg_attrdef d ON a.attrelid = d.adrelid AND a.attnum = d.adnum
LEFT JOIN pg_type t ON a.atttypid = t.oid
LEFT JOIN pg_collation c ON a.attcollation = c.oid AND a.attcollation <> t.typcollation
WHERE a.attrelid = '"old_passwords"'::regclass
AND a.attnum > 0 AND NOT a.attisdropped
ORDER BY a.attnum
lib/tasks/active_storage.rake:27:in `block (4 levels) in <top (required)>'
lib/tasks/active_storage.rake:26:in `each'
lib/tasks/active_storage.rake:26:in `block (3 levels) in <top (required)>'
lib/tasks/active_storage.rake:25:in `block (2 levels) in <top (required)>'
Caused by:
PG::UndefinedTable: ERROR: relation "old_passwords" does not exist
We can use the `ActiveStorage::Blob` class to find where the file is
supposed to be stored instead of manually setting the path. This is also
more robust because it works with Active Storage configurations which
don't store files in the default folder.
In order to migrate existing files from Paperclip to ActiveStorage, we
need Paperclip to find out the files associated to existing database
records. So we can't simply replace Paperclip with ActiveStorage.
That's why it's usually recommended [1] to first run the migration and
then replace Paperclip with ActiveStorage using two consecutive
deployments.
However, in our case we can't rely on two consecutive deployments
because we have to make an easy process so existing CONSUL installations
don't run into any issues. We can't just release version 1.4.0 and 1.5.0
and day and ask everyone to upgrade twice on the same day.
Instead, we're following a different plan:
* We're going to provide a Rake task (which will require Paperclip) to
migrate existing files
* We still use Paperclip to generate link and image tags
* New files are handled using both Paperclip and ActiveStorage; that
way, when we make the switch, we won't have to migrate them, and in
the meantime they'll be accessible thanks to Paperclip
* After we make the switch, we'll update the `name` column in the active
storage attachments tables in order to remove the `storage_` prefix
Regarding our handling of new files, the exception are cached
attachments. Since those attachments are temporary files used while
submitting a form and we have to delete them afterwards, we're only
handling them with Paperclip. We'll handle these ones in version 1.5.0.
Note the task creating the dev seeds was failing after these changes
with an `ActiveStorage::IntegrityError` exception because we were
opening some files without closing them. If the same file was attached
twice, it failed the second time.
We're solving it by closing the files with `File.open` and a block. Even
though we didn't get any errors, we're doing the same thing in the
`Attachable` concern because it's a good practice to close files after
we're done with them.
Also note we have to change the CKEditor Active Storage code so it's
compatible with Paperclip. In this case, I haven't been able to write a
test to confirm the attachment exists; I was getting the same
`ActiveStorage::IntegrityError` mentioned above.
Finally, we're updating the site customization image controller to use
`update` so the image and the attachment are updated within the same
transaction. This is also what we do in most controllers.
[1] https://www.youtube.com/watch?v=tZ_WNUytO9o
ActiveStorage support was added to CKEditor in version 5.1.0. However,
we can't upgrade to version 5.0.0 or later because it only supports
loading CKEditor via CDN.
So we're copying the code related to ActiveStorage instead. I tried to
move it to the `vendor/` folder, but couldn't find a way to load it from
there and if I found one I wouldn't be sure it'd work on production
environments.
We're not adding the rule because it would apply the current line length
rule of 110 characters per line. We still haven't decided whether we'll
keep that rule or make lines shorter so they're easier to read,
particularly when vertically splitting the editor window.
So, for now, I'm applying the rule to lines which are about 90
characters long.
