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>
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.
We were very inconsistent regarding these rules.
Personally I prefer no empty lines around blocks, clases, etc... as
recommended by the Ruby style guide [1], and they're the default values
in rubocop, so those are the settings I'm applying.
The exception is the `private` access modifier, since we were leaving
empty lines around it most of the time. That's the default rubocop rule
as well. Personally I don't have a strong preference about this one.
[1] https://rubystyle.guide/#empty-lines-around-bodies
Having exceptions is better than having silent bugs.
There are a few methods I've kept the same way they were.
The `RelatedContentScore#score_with_opposite` method is a bit peculiar:
it creates scores for both itself and the opposite related content,
which means the opposite related content will try to create the same
scores as well.
We've already got a test to check `Budget::Ballot#add_investment` when
creating a line fails ("Edge case voting a non-elegible investment").
Finally, the method `User#send_oauth_confirmation_instructions` doesn't
update the record when the email address isn't already present, leading
to the test "Try to register with the email of an already existing user,
when an unconfirmed email was provided by oauth" fo fail if we raise an
exception for an invalid user. That's because updating a user's email
doesn't update the database automatically, but instead a confirmation
email is sent.
There are also a few false positives for classes which don't have bang
methods (like the GraphQL classes) or destroying attachments.
For these reasons, I'm adding the rule with a "Refactor" severity,
meaning it's a rule we can break if necessary.
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.
Settings are stored in the database, and so any changes to the settings
done during the tests are automatically rolled back between one test and
the next one.
There were also a few places where we weren't using an `after` block but
changing the setting at the end of the test.
By default we want this attribute to be the current heading id for existing investments. If there have been reclassifications, this field should be updated accordingly.
The `type: :feature` is automatically detected by RSpec because these
tests are inside the `spec/features` folder. Using `feature` re-adds a
`type: :feature` to these files, which will result in a conflict when we
upgrade to Rails 5.1's system tests.
Because of this change, we also need to change `background` to `before`
or else these tests will fail.
After run the rake to move external_url to description for Proposals and Legislation proposals this spec is failing because external_url field doesn't exists anymore.
This spec is giving some problems related to duplicity of records due to the way rake tasks are loaded.
It will soon become part of seeds anyways. Removing for now.
These tasks dealt with data migrations or stats generations which were
done only once, so we don't need them anymore.
New CONSUL installations don't need these tasks, and existing CONSUL
installations will execute them when upgrading one release at a time.
