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.
While running the `dev_seed` twice, as we do in the tests, we were
getting the following warnings:
```
db/dev_seeds/proposals.rb:1: warning: already initialized constant
IMAGE_FILES
db/dev_seeds/budgets.rb:1: warning: already initialized constant
INVESTMENT_IMAGE_FILES
```
So we're extracting a method which allows us to use local variables
while removing duplication.
We had this warning with every version of Ruby, not just Ruby 2.7, but
since we're getting rid of all the warnings, we're taking care of this
one as well.
We were passing a hash when the method definition expected keyword
arguments, and so we were getting warnings in Ruby 2.7:
```
db/dev_seeds/widgets.rb:11: warning: Using the last argument as keyword
parameters is deprecated; maybe ** should be added to the call
db/dev_seeds/widgets.rb:23: warning: Using the last argument as keyword
parameters is deprecated; maybe ** should be added to the call
db/dev_seeds/widgets.rb:35: warning: Using the last argument as keyword
parameters is deprecated; maybe ** should be added to the call
db/dev_seeds/widgets.rb:47: warning: Using the last argument as keyword
parameters is deprecated; maybe ** should be added to the call
db/dev_seeds/admin_notifications.rb:3: warning: Using the last argument
as keyword parameters is deprecated; maybe ** should be added to the
call
db/dev_seeds/admin_notifications.rb:11: warning: Using the last argument
as keyword parameters is deprecated; maybe ** should be added to the
call
db/dev_seeds/admin_notifications.rb:19: warning: Using the last argument
as keyword parameters is deprecated; maybe ** should be added to the
call
db/dev_seeds/admin_notifications.rb:27: warning: Using the last argument
as keyword parameters is deprecated; maybe ** should be added to the
call
```
Converting the hash to keyword arguments solves the issue.
This way we'll avoid having all tenant organizations named "CONSUL" by
default, and we'll also use different email senders per tenant, which
will reduce the change of the emails being marked as spam.
We were using `consul.dev` as default, which might be fine for the
development environment, but it's something that definitely needs to be
changed on production.
Now we're providing a better default setting: using the same domain
which is used to generate URLs in emails. This also makes it easier to
configure new tenants, since the setting will default to whatever host
the new tenant is using.
The `|| "consul.dev"` part might not be needed; I'm keeping it because
I'm not sure production environents would not experience any new issues
if we don't add it, but we might remove it in the future.
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.
This rule was added in Rubocop 1.18.0, but we didn't add it back then.
Since we're applying it most of the time, we might as well be consistent
and apply it everywhere.
In the dev seeds, we were using `Setting["url"]/proposals`, but we can
use `proposals_url` instead, similar to what we do in the rest of the
application.
We can do a similar thing in the sitemap. This way the sitemap will also
work on installations who haven't manually set the "url" setting.
Since we aren't using `Setting["url"]` anywhere anymore, we're removing
it.
This setting was mainly redundant, since we already had the
`server_name` in the secrets. Furthermore, `server_name` is
automatically configured when running the installer, while
`Setting["url"]` had to be manually set in the admin section the
application was installed.
Note we're using `ActionMailer::Base` setting to generate URLs. Sounds a
bit strange, but it's a standard way Rails provides to generate URLs
outside the context of a request.
This method was introduced in Rails 6.0. It can be used to take an array
and create a hash where the elements of the array are the indexes of the
hash.
When we added this setting in commit 9979b5399, we disabled it by
default so it would be compatible with existing installations.
Since then, we've released version 1.4, which adds the settings to
existing databases. That means we can now enable it by default and
existing installations won't be affected.
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 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.
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
Some developers work on CONSUL installations where Spanish and/or
English aren't part of the available locales. In those cases, the
`dev_seed` task was crashing because we were using attributes like
`name_en` and `name_es`.
So we're using attributes for random locales instead.
We're using a proc so we don't have code like this all over the place:
random_locales.map do |locale|
I18n.with_locale(locale) do
phase.name = I18n.t("budgets.phase.#{phase.kind}")
phase.save!
end
end
This would make the code harder to read and would execute a `save!` once
per locale, which would make the task much slower.
We could also avoid the procs writing something like:
def random_locales_attributes(**attribute_names_with_values)
random_locales.each_with_object({}) do |locale, attributes|
I18n.with_locale(locale) do
attribute_names_with_values.each do |attribute_name, (i18n_key, i18n_args)|
value = I18n.t(i18n_key, (i18n_args || {}).merge(language: I18n.t("i18n.language.name")))
attributes["#{attribute_name}_#{locale.to_s.underscore}"] = value
end
end
end
end
And calling the method with with:
random_locales_attributes(name: ["seeds.budgets.name", year: Date.current.year - 1])
However, this code would also be different that what we usually do, we'd
have to apply some magic to pass the `language:` parameter, and the
strings wouldn't be recognized by i18n-tasks, so we aren't sure we're
really gaining anything.
We didn't enable it by default in commit 676adfcb3 so existing
installations didn't suddenly get a new section without expecting it.
But since the setting already exists for installations using version
CONSUL 1.3, now it will only be enabled for new installations.
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
Since version 2.0 introduced many breaking changes, we're upgrading to
it first.
The changes have been done by installing the rubocop-faker gem and
running:
```
rubocop \
--require rubocop-faker \
--only Faker/DeprecatedArguments \
--auto-correct
```
Currently it is not necessary to include the link_url field.
When we display these cards without link_url, they create an empty link that
redirects to the same page. I understand that this is not a desired behavior, so I
think it is better to add a validation in this case and force administrators to add a
link_url when creating a card.
In order to ensure compatibility with existing CONSUL installations, we
disabled all settings related to SDG. However, we also made it much
harder to enable SDG globally on the site, since administrators first
had to enable the SDG feature and then enable it for each process.
Most people will expect SDG is enabled for all processes once they
enable the SDG feature, so that's what we're doing. They can of course
disable specific processes should they wish to do so.
Since this PR (Refactor participatory budgets in draft mode #4369) budgets
have a new field "published" to manage whether they are displayed or not. We
update this field in dev_seeds to be able to display budgets on the public page
budgets.
These cards will be displayed in the SDG homepage.
Note there seems to be a strange behavior in cancancan. If we define
these rules:
can :manage, Widget::Card, page_type: "SDG::Phase"
can :manage, Widget::Card
The expected behavior is the first rule will always be ignored because
the second one overwrites it. However, when creating a new card with
`load_and_authorize_resource` will automatically add `page_type:
"SDG::Phase"`.
Similarly, if we do something like:
can :manage, Widget::Card, id: 3
can :manage, Widget::Card
Then the new card will have `3` as an ID.
Maybe upgrading cancancan solves the issue; we haven't tried it. For now
we're defining a different rule when creating widget cards.
This rule was added in Rubocop 0.89.0. However, there are some false
positives when we don't use interpolation but simply concatenate in
order to avoid long lines. Even if there weren't false positives, there
are places where we concatenate to emphasize the point that we're adding
a certain character to a text.
We might reconsider this rule in the future, since we generally prefer
interpolation over concatenation.
In Ruby 5.2, we get a warning when using the "RANDOM()" function:
DEPRECATION WARNING: Dangerous query method (method whose arguments are
used as raw SQL) called with non-attribute argument(s): "RANDOM()".
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().
This warning doesn't make much sense, though, since RANDOM() is a common
function which is not dangerous at all. However, since the warning is
annoying, we'll probably have to find a way to deal with it.
So I'm extracting all our RANDOM() usages into a method. This way we'll
only have to change one method to avoid this warning.
I've chosen `sample` because it's similar to Ruby's Array#sample, and
because `order_by_random` would be confusing if we consider we already
have a method called `sort_by_random`.
Since we were saving the same budget investments several times, once for
every language, we were creating more than 1000 changelog entries.
Assigning the language attributes when creating the investments
generates less entries, making it easier to work with them.
