The method `available_locales` in
`RemoteTranslations::Microsoft::AvailableLocales` needs to execute a
request to an external server in order to work, meaning it will fail if
the machine its executed on doesn't have an internet connection.
So we're stubbing the method in the tests using it.
We were evaluating its value when the server starts. On production
enviroments, that could mean weeks or months before the available
locales are checked again, and so it would be possible to use a list
which is no longer in sync with the list provided by microsoft.
This method is deprecated in Rails 5.1 because its behavior will be
different in `before` and `after` callbacks.
We're replacing the deprecated `attribute_changed?` and `attribute_was`
methods with `saved_change_to_attribute?` and
`attribute_before_last_save` during `after_save` callbacks.
https://github.com/rails/rails/pull/32835/
Rails 5.1 introduced certain changes in the way a record is touched when
the counter cache option is enabled in a belongs to association.
We need to upgrade acts-as-taggable-on so it keeps changing the
`updated_at` attribute when a new tag is added to a record.
Note we now need to reload the records in some cases to get the
`context_tag_list` method to return what we expect. Methods like
`context_tags` however work properly with no need to reload the record.
Implementation tries to be open for further extensions, such as deciding on
search dictionary based on configuration option or by locale set for
given user.
When an investment had been assigned a user tag and a valuation tag with
the same name, it appeared twice when filtering by tag.
This is because by design, in order to provide compatibility with scopes
using "select" or "distinct", the method `tagged_with` doesn't select
unique records.
Forcing the query to return unique records solves the issue.
- Validate that locale is a valid locale for RemoteTranslation Client.
- RemoteTranslation can only be created for resources that do not have the requested
language translated
These filters were only returning investments with valuation open, but
we don't want to do that since the time we changed the interface in
order to allow users to apply several filters at the same time.
Generating a random title with `Faker::Lorem.sentence` sometimes caused
validation errors in tests because the generated sentence was more than
80 characters long.
The link to show stats for these polls is nowhere to be seen in the
application, and these stats are included in the budget stats, so it
makes sense to restrict access to them.
When defining abilities, scopes cover more cases because they can be
used to check permissions for a record and to filter a collection. Ruby
blocks can only be used to check permissions for a record.
Note the `Budget::Phase.kind_or_later` name sounds funny, probably
because we use the word "phase" for both an an attribute in the budgets
table and an object associated with the budget, and so naming methods
for a budget phase is a bit tricky.
There's no reason to allow administrators to check stats and results for
a poll when it isn't finished or when results and stats are not enabled.
Now admins have the same permissions as everyone else.
* Add custom message for inclusion validation to include the allowed values.
* Force user to choose document_type from select lik the one shown at verification form.
* Convert stored document_type to a human readable text
Although we weren't showing links in the views to execute certain
actions, forms could be still sent using a PUT/PATCH pull request to the
controller actions.
We were adding the condition to show the form in the view. However, that
doesn't prevent users from sending a POST/PUT request to the controller
action.
We could add the condition to the controller as well, but since the
`valuate` permission is only used in one place, it's easier to restrict
that permission to valuators who can edit the dossier.
Since budgets now have milestone tags, the name of this method was
confusing and will conflict with the name generated by acts_as_taggable.
Note the new name could be improved too.
Unfortunately this feature wasn't properly reviewed and tested, and it
had many bugs, some of them critical and hard to fix, like validations
being skipped in concurrent requests.
So we're removing it before releasing version 1.1. We might add it back
in the future if we manage to solve the critical issues.
This commit reverts commit 836f9ba7.
This method is ambiguous. Sometimes we use it to set invalid data in
tests (which can usually be done with `update_column`), and other times
we use it instead of `update!`.
I'm removing it because, even if sometimes it could make sense to use
it, it's too similar to `update_attributes` (which is an alias for
`update` and runs validations), making it confusing.
However, there's one case where we're still using it: in the
ActsAsParanoidAliases module, we need to invoke the callbacks, which
`update_column` skips, but tests related to translations fail if we use
`update!`. The reason for this is the tests check what happens if we
restore a record without restoring its translations. But that will make
the record invalid, since there's a validation rule checking it has at
least one translation.
I'm not blacklisting any other method which skips validations because we
know they skip validations and use them anyway (hopefully with care).
This comment isn't necessary since Ruby 2.0, where UTF-8 became the
default encoding.
I've found this issue thanks to the EmptyLineAfterMagicComment rubocop
rule.
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.
We were converting markdown to HTML every time we saved a record, which
has the same problems as sanitizing HTML before saving it to the
database, particularly because the body of a legislation draft is stored
in a translations table.
Performance-wise this isn't a problem: converting a text with more than
200_000 characters takes about a milisecond on my machine.
Note we need to modify a migration generated by globalize, since the
method `create_translation_table!` would fail now that we don't define
`translates :body_html` in the model.
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.
