Just as we mentioned in the previous commit, there are places where we
aren't sure whether explicit consent is strictly required. So, when the
"require consent" setting is enabled, we're taking the safe approach.
This means that, in this case, we're only displaying a user's activity
if they've given explicit consent.
The GDPR is open for interpretation, and it isn't clear whether showing
users recommended proposals and debates while browsing the site is
considered a notification that needs to be explicitly accepted.
Since we aren't sure whether this is necessary, we're taking the safe
approach and disabling recommendations by default.
Ensure GDPR compliance by default (Article 25 GDPR – privacy by design
and by default). Under GDPR, consent must be freely given, specific,
informed and unambiguous [1]. We were subscribing users without
explicity consent, which goes against the "No pre-ticked boxes"
principle.
For compatibility with existing installations, we're using a setting,
disabled by default. Once we release version 2.4.0 we will enable it by
default, which won't affect existing installations but only new ones.
[1] https://gdprinfo.eu/best-gdpr-newsletter-consent-examples-a-complete-guide-to-compliant-email-marketing
Similar to what we did in PR "Avoid duplicate records in poll answers" 5539,
specifically in commit 503369166, we want to stop relying on the plain text
"answer" and start using "option_id" to avoid issues with counts across
translations and to add consistency to the poll_partial_results table.
Note that we also moved the `possible_answers` method from Poll::Question to
Poll::Question::Option, since the list of valid answers really comes from the
options of a question and not from the question itself. Tests were updated
to validate answers against the translations of the assigned option.
Additionally, we renamed lambda parameters in validations to improve clarity.
Note that Rails 7.1 changes `find_or_create_by!` so it calls
`create_or_find_by!` when no record is found, meaning we'll rarely get
`RecordNotUnique` exceptions when using this method during a race
condition.
Adding this index means we need to remove the uniqueness validation.
According to the `create_or_find_by` documentation [1]:
> Columns with unique database constraints should not have uniqueness
> validations defined, otherwise create will fail due to validation
> errors and find_by will never be called.
We're adding a test that checks what happens when using
`create_or_find_by!`.
Note that we're creating voters combining `create_with` with
`find_or_create_by!`. Using `find_or_create_by!(...)` with all
attributes (including non-key ones like `origin`) fails when a voter
already exists with different values, e.g. an existing `origin: "web"`
and an incoming `"booth"`. In this situation the existing record is not
matched and the unique index raises an exception.
`create_with(...).find_or_create_by!(user: ..., poll: ...)` searches by
the unique key only and applies the extra attributes only on creation.
Existing voters are returned unchanged, which is the intended behavior.
For the `take_votes_from` method, we're handling a (highly unlikely, but
theoretically possible) scenario where a user votes at the same time as
taking voters from another user. For that, we're doing something similar
to what `create_or_find_by!` does: we're updating the `user_id` column
inside a new transaction (using a new transactions avoids a
`PG::InFailedSqlTransaction` exception when there are duplicate
records), and deleting the existing voter when we get a
`RecordNotUnique` exception.
On `Poll::WebVote` we're simply raising an exception when there's
already a user who's voted via booth, because the `Poll::WebVote#update`
method should never be called in this case.
We still need to use `with_lock` in `Poll::WebVote`, but not due to
duplicate voters (`find_or_create_by!` method will now handle the unique
record scenario, even in the case of simultaneous transactions), but
because we use a uniqueness validation in `Poll::Answer`; this
validation would cause an error in simultaneous transactions.
[1] https://api.rubyonrails.org/v7.1/classes/ActiveRecord/Relation.html#method-i-create_or_find_by
We're disabling `action_controller.raise_on_missing_callback_actions`
because sometimes we include `before_action :something, only: actions`
in concerns, and we include these concerns in controllers that don't
have all these actions.
Note that Rails 7.1 logs to STDOUT by default [1]; we're re-adding the
condition `if ENV["RAILS_LOG_TO_STDOUT"].present?` because we're still
using files and we're rotating the logs to avoid running out of space.
Also note that the GraphQL controller test (which is actually a request
test, since it's got `type: :request`) that was raising an exception no
longer does it thanks to the new default value for the
`config.action_dispatch.show_exceptions` configuration option. So we're
updating the test accordingly. This option doesn't affect regular
controller tests (without the `type: :request` option), so in other
tests we're still checking exceptions.
[1] Pull request 47138 in https://github.com/rails/rails
I don't think this feature it was ever used. It was introduced in commit
49dec6061 as part of a feature that was removed in commits 1cd47da9d and
c45a0bd8ac.
This column isn't used since commit 4c0deb0ec because administrators can
associate videos to the answers since commit 5862eea51. The value of
this attribute isn't used in the public area since commit 8277e3cc2.
In commit 96ae69fe9, we stopped using cookies to track Ahoy visits and
started using a combination of the IP and the browser agent instead.
However, since we're still using the legacy logic from Ahoy 1.x to track
visits (which we had to add in commit b5220effd), this way of tracking
visits doesn't work and counts every page visited by a user as an
independent visit.
Maybe we could migrate existing data, which uses the `visitor_id` column
so it uses the new `visit_token` and `visitor_token` columns, but
there's no mention in the Ahoy documentation regarding how to do so.
While deciding what to do about this, we found something interesting.
For two years, we've been seeing random failures in the
`system/admin/tenants_spec.rb` tests, with messages like:
```
1) Tenants Create Tenant with subdomain
Failure/Error:
raise TenantNotFound, <<~EXCEPTION_MESSAGE
Could not set search path to schemas, they may be invalid:
"#{tenant}" #{full_search_path}.
Original error: #{exception.class}: #{exception}
EXCEPTION_MESSAGE
Apartment::TenantNotFound:
Could not set search path to schemas, they may be invalid:
"earth" "public", "shared_extensions".
Original error:
ActiveRecord::StatementInvalid: Could not find schema earth
```
And we've found one of the causes: the AJAX requests done by Ahoy to
track visits. Sometimes a test that creates or updates a tenant finishes
but the Ahoy AJAX request to, say, `earth.lvh.me/ahoy/visits`, is
handled by the next test, when the `earth` schema no longer exists, thus
raising an `Apartment::TenantNotFound` exception.
So by disabling these AJAX requests and tracking the visits in the
server instead, we're killing two birds in one stone: we're fixing the
bug regarding the visits count and we're reducing the flakiness in our
test suite. It looks like we're also removing the "phantom ahoy cookie"
we were getting since the mentioned commit b5220effd: an ahoy cookie was
quickly set and unset in the browser.
Note that, even though we aren't migrating any data, we're still adding
the new fields, because some tests started to fail because, when
tracking visits in the server without cookies, Ahoy expects the Visit
model to have a `visit_token` field.
The search_keyword column was removed for new installations in Ahoy
2.1.0 and stopped being used completely in Ahoy 3.0.0.
There are other columns that we use and are no longer generated by
default by Ahoy, which are: screen_height, screen_width and postal_code.
Apparently they're still used in Ahoy if they're available, though, so
we aren't removing them, at least for now.
Until now, we've stored the text of the answer somebody replied to. The
idea was to handle the scenarios where the user voters for an option but
then that option is deleted and restored, or the texts of the options
are accidentally edited and so the option "Yes" is now "Now" and vice
versa.
However, since commit 3a6e99cb8, options can no longer be edited once
the poll starts, so there's no risk of the option changing once somebody
has voted.
This means we can now store the ID of the option that has been voted.
That'll also help us deal with a bug introduced int 673ec075e, since
answers in different locales are not counted as the same answer. Note we
aren't dealing with this bug right now.
We're still keeping (and storing) the answer as well. There are two
reasons for that.
First, we might add an "open answer" type of questions in the future and
use this column for it.
Second, we've still got logic depending on the answer, and we need to be
careful when changing it because there are existing installations where
the answer is present but the option_id is not.
Note that we're using `dependent: nullify`. The reasoning is that, since
we're storing both the option_id and the answer text, we can still use
the answer text when removing the option. In practice, this won't matter
much, though, since we've got a validation rule that makes it impossible
to destroy options once the poll has started.
Also note we're still allowing duplicate records when the option is nil.
We need to do that until we've removed every duplicate record in the
database.
When we first started caching the stats, generating them was a process
that took several minutes, so we never expired the cache.
However, there have been cases where we run into issues where the stats
shown on the screen were outdated. That's why we introduced a task to
manually expire the cache.
But now, generating the stats only takes a few seconds, so we can
automatically expire them every day, remove all the logic needed to
manually expire them, and get rid of most of the issues related to the
cache being outdated.
We're expiring them every day because it's the same day we were doing in
public stats (which we removed in commit 631b48f58), only we're using
`expires_at:` to set the expiration time, in order to simplify the code.
Note that, in the test, we're using `travel_to(time)` so the test passes
even when it starts an instant before midnight. We aren't using
`:with_frozen_time` because, in similar cases (although not in this
case, but I'm not sure whether that's intentional), `travel_to` shows
this error:
> Calling `travel_to` with a block, when we have previously already made
> a call to `travel_to`, can lead to confusing time stubbing.
As mentioned in Ahoy's README [1]:
> Ahoy provides a number of options to help with GDPR compliance.
> Update config/initializers/ahoy.rb with:
>
> class Ahoy::Store < Ahoy::DatabaseStore
> def authenticate(data)
> # disables automatic linking of visits and users
> end
> end
>
> Ahoy.mask_ips = true
> Ahoy.cookies = :none
As also mentioned in the README:
> If Ahoy was installed before v5, add an index before making this
> change.
> (...)
> For Active Record, create a migration with:
> add_index :ahoy_visits, [:visitor_token, :started_at]
However, the `visitor_token` doesn't exist in our table, since we
generated the `visits` table when Ahoy used the `visitor_id` column. So
we're using this column for the index.
Note we also need to change the `visit` method, since otherwise we get
an exception [2]. As mentioned on the issue reporting the exception:
> you'll need to copy the latest version of that method and adapt it to
> your model. I believe you'll want to replace:
>
> where(visit_token: ahoy.visit_token) with
> where(id: ensure_uuid(ahoy.visit_token))
>
> where(visitor_token: ahoy.visitor_token) with
> where(visitor_id: ensure_uuid(ahoy.visitor_token))
So we're copying the latest version of that method and changing it
accordingly.
[1] https://github.com/ankane/ahoy/blob/v5.0.2/README.md
[2] Issue 549 in https://github.com/ankane/ahoy
This was added in commit 02f19aa4b, before we started tracking events.
I don't think we ever used it; in any case, we now use the `Ahoy::Chart`
class to deal with the stats Ahoy used to generate.
The config.file_watcher option still exists but it's no longer included
in the default environtment file. Since we don't use it, we're removing
it.
The config.assets.assets.debug option is no longer true by default [1],
so it isn't included anymore.
The config.active_support.deprecation option is now omitted on
production in favor of config.active_support.report_deprecations, which
is false by default. I think it's OK to keep it this way, since we check
deprecations in the development and test environments but never on
production environments.
As mentioned in the Rails upgrade guide, sprockets-rails is no longer a
rails dependency and we need to explicitly include it in our Gemfile.
The behavior of queries trying to find an invalid enum value has changed
[2], so we're updating the tests accordingly.
The `favicon_link_tag` method has removed the deprecated `shortcut`
link type [3], so we're updating the tests accordingly.
The method `raw_filter` in ActiveSupport callbacks has been renamed to
`filter` [4], so we're updating the code accordingly.
[1] https://github.com/rails/rails/commit/adec7e7ba87e3
[2] https://github.com/rails/rails/commit/b68f0954
[3] Pull request 43850 in https://github.com/rails/rails
[4] Pull request 41598 in https://github.com/rails/rails
In order to the display a warn text on the last attempt
before the account is locked, we need update
config.paranoid to false as the devise documentation
explains.
Adding "config.paranoid: false" implies further changes
to the code, so for now we unncomment the default value
"config.last_attempt_warning = true" and update it to false.
Note that `Capybara.app_host` now returns `nil` by default and that
breaks tests using `lvh.me` or our custom `app_host` method, so we're
setting `Capybara.app_host` to the value it had in earlier versions of
Rails. I also haven't found a way to remove the code to set the
integration session host in relationable tests which I mentioned in
commit ffc14e499.
Also note that we now filter more parameters, and that they match
regular expressions, so filtering `:passw` means we're filtering
`passwd`, `password`, ...
Note that in the budgets wizard test we now create district with no
associated geozone, so the text "all city" will appear in the districts
table too, meaning we can't use `within "section", text: "All city" do`
anymore since it would result in an ambiguous match.
Co-Authored-By: Julian Herrero <microweb10@gmail.com>
Co-Authored-By: Javi Martín <javim@elretirao.net>
In the PR #1140 this field was added in commit ad697cd2c1, but in this same
PR in commit 28d12fe55 all the related functionality that had been added was
removed but the field was not removed.
To get the heading where a user voted, we were relying on the
`balloted_heading_id` field.
Our guess is this was done so the total number of users is the same as
the sum of users who voted on a heading. That is, if 2000 people voted
just on the "All city" heading, 1000 voted just on the "North district"
heading, and 500 people voted on both, instead of showing "3500 people
voted in total, 2500 voted in all city, 1500 voted in north district",
we show something like "3500 people voted in total, 2250 voted in all
city, and 1250 voted in north district".
However, this approach has some disadvantages.
The first disadvantage is, the stats aren't correct. In the case above,
2500 voted on the "All city heading", so the statistics for this heading
don't show reality.
The second one is we weren't considering the last heading where users
voted inside the budget being displayed, but the last heading where
users voted, period. That means that, if all the people above voted on a
later budget, the stats for the budget above would become "3500 people
voted in total, 0 voted in all city, and 0 voted in north district".
That also means we were including headings from previous budgets in the
statistics for more recent budgets when people hadn't voted on the
recent ones.
So we're removing the `balloted_heading_id` since its data is lost once
people vote on a new budget. And, in order to show the right stats and
simplify the code, we're no longer trying to add votes just to one
heading when users vote on several headings.
Co-Authored-By: Julian Nicolas Herrero <microweb10@gmail.com>
Note we could use `acts_as_paranoid` with the `without_default_scope`
option, but we aren't doing so because it isn't possible to consider
deleted records in uniqueness validations with the paranoia gem [1].
I've added tests for these cases so we don't accidentally add
`acts_as_paranoid` in the future.
Also note we're extracting a `RowComponent` because, when
enabling/disabling a tenant, we're also enabling/disabling the link
pointing to its URL, and so we need to update the URL column after the
AJAX call.
[1] See issues 285 and 319 in https://github.com/rubysherpas/paranoia/
Some institutions using CONSUL have expressed interest in this feature
since some of their tenants might already have their own domains.
We've considered many options for the user interface to select whether
we're using a subdomain or a domain, like having two separate fields,
using a check box, ... In the end we've chosen radio buttons because
they make it easier to follow a logical sequence: first you decide
whether you're introducing a domain or subdomain, and then you enter it.
We've also considered hiding this option and assuming "if it's got a
dot, it's a domain". However, this wouldn't work with nested subdomains
and it wouldn't work with domains which are simply machine names.
Note that a group of radio buttons (or check boxes) is difficult to
style when the text of the label might expand over more than one line
(as is the case here on small screens); in this case, most solutions
result in the second line of the label appearing immediately under the
radio button, instead of being aligned with the first line of the label.
That's why I've added a container for the input+label combination.
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
Giving any user a direct link to edit another user's account settings doesn't seem like a
great idea. Instead we'll generate a random secure hash string to help keep things
more secure. We'll store these hashes on each user so that we have a way to find
them during this public query. To do this we need to add a column to the user table.
This column wasn't used in any released Consul version since it was only
used during development. For the same reason, the task to migrate the
information in the `link` column to the `links` table isn't needed
either.
The `column` method in ActiveRecord::ConnectionAdapters::TableDefinition
supports adding the `index:` option. The documentation says:
> Instantiates a new column for the table. See connection.add_column for
> available options.
>
> Additional options are:
>
> :index - Create an index for the column. Can be either true or an
> options hash.
So basically the `connection.add_column` method silently ignores the
`index:` option, and whenever we intended to create an index this way,
we didn't.
We're creating a new migration where we properly add the indexes that
weren't added when we intended to.
Thanks to the rubocop-rails team, who added this cop in version 2.11.0
and helped us notice this bug.
- Allow to define a link (text and url) on budget form for render on the budget
header.
- Improve styles
Co-authored-by: Senén Rodero Rodríguez <senenrodero@gmail.com>
Previously the draft mode was a phase of the PB, but that had some
limitations.
Now the phase drafting disappears and therefore the PB can have the
status published or not published (in draft mode).
That will give more flexibility in order to navigate through the
different phases and see how it looks for administrators before
publishing the PB and everybody can see.
By default, the PB is always created in draft mode, so it gives you
the flexibility to adjust and modify anything before publishing it.
This is similar to what we do with investments, which belong to a heading
but also belong to a budget. In our case, the reason is we've been asked
to add local targets which belong to a goal but are not related to any
existing target.
Even though we're not implementing that case right now, we're adding the
relation so we don't have to add data migrations in the future.
and its relation with relatables
Note about sdg_review factory: Cannot use the constantize method on
the relatable_type as long as the relatable classes will be loaded and
this will throw an exception because the database is not available at
factiry definition time.
So now we'll be able to add them to other sections.
We're also adding a `dependent: :destroy` relation to models having
cards since it doesn't make sense to have cards around when their page
has been destroyed.
