and its relation with the SDG goal model.
Add comparable module be able to sort collections of targets
by code attribute.
Co-Authored-By: Javi Martín <35156+javierm@users.noreply.github.com>
Since data for this model (title and description) is not generated in
CONSUL but by the United Nations, we aren't storing it in the database
but in our YAML translation files.
The reasoning is as follows. Suppose that, a few months after CONSUL
gets SDG support, a new language is added to CONSUL.
With YAML files, getting the texts in the new language would mean
updating CONSUL to include the new language.
But if we store these texts in the database, it means we have to update
the databases of all existing CONSUL installations, either each
installation by themselves (duplicating efforts) or running a rake task
(which we would have to write each time).
So we believe using translations works better in this case.
We're still storing records in the database with the code, so they can
be easily referenced via `has_many` or `has_many :through` associations.
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.
After merge "Bump paperclip from 5.2.1 to 6.1.0 #3905" as indicated in
the release_notes columns type for attachments are changed from integer
to bigint.
For this reason we edit old migrations so that they continue to
generate integer as before.
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`.
These columns were causing Rails 5.2 to throw a warning when ordering by
them, as if they weren't valid column names:
DEPRECATION WARNING: Dangerous query method (method whose arguments are
used as raw SQL) called with non-attribute argument(s):
:"budget/investments_count". 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 change also makes their names consistent with the rest of our
tables and columns.
There are two reasons for this change:
1. Past migrations depending on models will not work once a model is
removed, and they won't work if we remove Globalize either
2. We were getting a conflict in the schema file; when run under Rails
5.0, these migrations were generating a different schema than in Rails
5.1, due to the way the `create_translation_table!` method handles the
`id: :serial` attribute.
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.
Rails 5.1 doesn't align the columns in the schema file anymore, so
there aren't unrelated changes when we add or remove columns to a table:
https://github.com/rails/rails/pull/25675
We're sorry for developers who are really concerned about code
alignment.
There's a conflict between the data migrations Globalize uses and the
audited configuration. Since Globalize uses the model to run the
migrations, it might try to run code that wasn't available when the
migration was created.
In this case, when migrating data it tries to audit the translations
table for budget investments. However, the migration creating the table
for audits hasn't been run at this point, since it was added after the
migration to add translations to investments was.
On the other hand, this data migration isn't really a change in the
model attibutes, so it shouldn't be audited anyway.
So we're disabling auditing during the migration in order to avoid these
issues.
Globalize does not support having translatable columns with the same
name in the original table and the translations table. We were planning
to migrate to Mobility, but we aren't doing so before releasing version
1.1.
We've also found a gotcha regarding having both columns: if we use the
`update_column` method, which we use in rake tasks to speed up the
process and in tests where we want to skip validations and callbacks, we
update the column in the original table and no exception is raised. If
we remove the column in the original table, we get an exception, which
is what we want since our intention is to update the column in the
translations table.
With this change we're following the advice given by the Mobility lead
developer: "If you don't need the columns, I think it would make sense
to just remove them to avoid any edge case issues."
This commit reverts commit 251326ea.
The new CSV report was more configurable and could work on proposals,
processes and comments. However, it had several issues.
In the public area, by default it generated a blank file.
In the admin section, the report was hard to configure and it generated
a file with less quality than the old system.
So until we improve this system, we're bringing back the old investment
CSV exporter.
This commit reverts most of commit 9d1ca3bf.
Our manual implementation had a few issues. In particular, it didn't
track changes related to associations, which became more of an issue
when we made investments translatable.
Using audited gives us more functionality while at the same time
simplifies our code. However, it adds one more external dependency to
our project.
The reason for choosing audited over paper trail is audited seems to
make it easier to handle associations.
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.
The current tracking section had a few issues:
* When browsing as an admin, this section becomes useless since no
investments are shown
* Browsing investments in the admin section, you're suddenly redirected
to the tracking section, making navigation confusing
* One test related to the officing dashboard failed due to these changes
and had been commented
* Several views and controller methods were copied from other sections,
leading to duplication and making the code harder to maintain
* Tracking routes were defined for proposals and legislation processes,
but in the tracking section only investments were shown
* Probably many more things, since these issues were detected after only
an hour reviewing and testing the code
So we're removing this untested section before releasing version 1.1. We
might add it back afterwards.
Tags and help links can be edited, but aren't used anywhere. Since we
don't know what the intended behavior was, I'm removing them for now.
My best guess is tags were supposed to be used so investments for a
budget can only be assigned tags present in the budget. Achieving that
behavior wouldn't be a trivial task.
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.
While I don't use this feature, there are developers who do. It's useful
when running migrations and changing branches.
I'm raising an `ActiveRecord::IrreversibleMigration` exception in every
`drop_table` migration because these migrations were all done before
version 1.0.0, and so making all of them reversible would be too much
work for little benefit.
We were using the `timestamps` method most of the time, but sometimes we
were creating the columns manually.
Note editing past migrations if fine as long as the SQL they generate
remains identical, which is the case here.
There are some rules which only affect migration files, and we cannot
enable them if we're excluding those files from being inspected.
We're also changing migrations related to the Rails/TimeZone rule
slightly because these fields were already changed afterwards, so we
aren't changing the schema.
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 inconsistent on this one. I consider it particularly useful when
a method starts with a `return` statement.
In other cases, we probably shouldn't have a guard rule in the middle of
a method in any case, but that's a different refactoring.
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
We were already using `find_by` most of the time.
Since there are false positives related to our `find_by_slug_or_id!` and
`find_by_manger_login` methods, which cannot be replaced with `find_by`,
I'm adding it indicating the "refactor" severity.
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.
