This file only has tests related to tags; if the model doesn't have
tags, we simply wouldn't include `it_behaves_as` in their tests instead
of including it and then skipping it.
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.
We were using custom rules because of some issues with Paperclip. These
rules work fine, but since we're already using the file_validators gem,
we might as well simplify the code a little bit.
We were testing the URL of the image changes to `missing.png`, but
actually that's confusing because the image record is now invalid and so
its changes can't be saved. That means that, when rendered in the
browser, the image won't render the `missing.png` image but will try to
render the destroyed one.
If we want to render the `missing.png` image when the attachment has
been destroyed, we need to remove the attachment presence validation or
change the `url` method so it detects when an attachment is missing.
We were using helper methods inside the model; we might as well include
them in the model and use them from anywhere else.
Note we're using a different logic for images and documents methods.
That's because for images the logic was defined in the helper methods,
but for documents the logic is defined in the Documentable concern. In
the past, different documentable classes allowed different content
types, while imageable classes have always allowed the same content
types.
I'm not sure which method is better; for now, I'm leaving it the way it
was (except for the fact that we're removing the helper methods).
The same way it's done for images.
We were converting the number of megabytes to bytes and then converting
it to megabytes again. Instead, we can leave it as it is and only
convert it to bytes when necessary (only one place).
`dalli_store` is deprecated since dalli 2.7.11.
We can now enable cache_versioning. We didn't enable it when upgrading
to Rails 5.2 because of possible incompatibility with `dalli_store` [1],
even though apparently some the issues were fixed in dalli 2.7.9 and
dalli 2.7.10 [2].
Since using cache versioning makes cache expiration more efficient, and
I'm not sure whether the options we were passing to the dalli store are
valid with memcache store (documentation here is a bit lacking), I'm
just removing the option we used to double the default cache size on
production.
[1] https://www.schneems.com/2018/10/17/cache-invalidation-complexity-rails-52-and-dalli-cache-store
[2] https://github.com/petergoldstein/dalli/blob/master/History.md
Since we're only doing the convertion from bytes to megabytes in one
place, IMHO adding an extra method makes the code harder to read.
This way we don't have do include the DocumentsHelper in the specs
anymore, reducing the risk of possible method naming collisions.
Including them might lead to conflicts since two methods might have the
same name. For example, we're getting some exceptions when taking
screenshots of a failing test, because the method `image_path` from
`ActionView::Helpers::AssetUrlHelper` has the same name as a method used
to save the screenshot.
Besides, we were including all helpers in places were only the `dom_id`
method is used, and in other places where no helper methods were used at
all. So we can just invoke `ActionView::RecordIdentifier.dom_id`
directly.
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.
Sanitizing descriptions before saving a record has a few drawbacks:
1. It makes the application rely on data being safe in the database. If
somehow dangerous data enters the database, the application will be
vulnerable to XSS attacks
2. It makes the code complicated
3. It isn't backwards compatible; if we decide to disallow a certain
HTML tag in the future, we'd need to sanitize existing data.
On the other hand, sanitizing the data in the view means we don't need
to triple-check dangerous HTML has already been stripped when we see the
method `auto_link_already_sanitized_html`, since now every time we use
it we sanitize the text in the same line we call this method.
We could also sanitize the data twice, both when saving to the database
and when displaying values in the view. However, doing so wouldn't make
the application safer, since we sanitize text introduced through
textarea fields but we don't sanitize text introduced through input
fields.
Finally, we could also overwrite the `description` method so it
sanitizes the text. But we're already introducing Globalize which
overwrites that method, and overwriting it again is a bit too confusing
in my humble opinion. It can also lead to hard-to-debug behaviour.
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.
Adapt retire form to include needed translations and move validations
from controller to model.
Also change sanitizable concern to sanitize not marked for destruction
translations.
When editing the map of a proposal or investment(the mappable) the
updated_at attribute of the mappable was not been updated and so the
map still displayed the old location after updating it
