We were always displaying the event names in English.
Note we're changing the `user_supported_budgets` key because it didn't
make much sense; the investments are supported, and not the budgets.
We're also adding "created" to most of the event names in order to make
the texts more explicit, since not all the events refer to created data.
Note we're delegating the `t` method because i18n-tasks doesn't detect
code like `ApplicationController.helpers.t` and so reports we aren't
using the `admin.stats.graph` translations.
We were tracking some events with Ahoy, but in an inconsistent way. For
example, we were tracking when a debate was created, but (probably
accidentally) we were only tracking proposals when they were created
from the management section. For budget investments and their supports,
we weren't using Ahoy events but checking their database tables instead.
And we were only using ahoy events for the charts; for the other stats,
we were using the real data.
While we could actually fix these issues and start tracking events
correctly, existing production data would remain broken because we
didn't track a certain event when it happened. And, besides, why should
we bother, for instance, to track when a debate is created, when we can
instead access that information in the debates table?
There are probably some features related to tracking an event and their
visits, but we weren't using them, and we were storing more user data
than we needed to.
So we're removing the track events, allowing us to simplify the code and
make it more consistent. We aren't removing the `ahoy_events` table in
case existing Consul Democracy installations use it, but we'll remove it
after releasing version 2.2.0 and adding a warning in the release notes.
This change fixes the proposal created chart, since we were only
tracking proposals created in the management section, and opens the
possibility to add more charts in the future using data we didn't track
with Ahoy.
Also note the "Level 2 user Graph" test wasn't testing the graph, so
we're changing it in order to test it. We're also moving it next to the
other graphs test and, since we were tracking the event when we were
confirming the phone, we're renaming to "Level 3 users".
Finally, note that, since we were tracking events when something was
created, we're including the `with_hidden` scope. This is also
consistent with the other stats shown in the admin section as well as
the public stats.
We moved this file to `app/lib/` in commit cb477149c so it would be in
the autoload_paths. However, this class is loaded by ActiveStorage, with
the following method:
```
def resolve(class_name)
require "active_storage/service/#{class_name.to_s.underscore}_service"
ActiveStorage::Service.const_get(:"#{class_name.camelize}Service")
rescue LoadError
raise "Missing service adapter for #{class_name.inspect}"
end
``
So this file needs to be in the $LOAD_PATH, or else ActiveStorage won't
be able to load it when we disable the `add_autoload_paths_to_load_path`
option, which is the default in Rails 7.1 [1].
Moving it to the `lib` folder solves the issue; as mentioned in the
guide to upgrade to Rails 7.1 [2]:
> The lib directory is not affected by this flag, it is added to
> $LOAD_PATH always.
However, we were also referencing this class in the `Tenant` model,
meaning we needed to autoload it as well somehow. So, instead of
directly referencing this class, we're using `respond_to?` in the Tenant
model.
We're changing the test so it fails when the code calls
`is_a?(ActiveStorage::Service::TenantDiskService)`. We need to change
the active storage configurations in the test because, otherwise, the
moment `ActiveStorage::Blob` is loaded, the `TenantDiskService` class is
also loaded, meaning the test will pass when using `is_a?`.
Note that, since this class isn't in the autoload paths anymore, we need
to add a `require` in the tests. We could add an initializer to require
it; we're not doing it in order to be consistent with what ActiveStorage
does: it only loads the service that's going to be used in the current
Rails environment. If somebody changed their production environment in
order to use (for example), S3, and we added an initializer to require
the TenantDiskService, we would still load the TenantDiskService even if
it isn't going to be used.
[1] https://guides.rubyonrails.org/v7.1/configuring.html#config-add-autoload-paths-to-load-path
[2] https://guides.rubyonrails.org/v7.1/upgrading_ruby_on_rails.html#autoloaded-paths-are-no-longer-in-$load-path
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
Byebug hasn't been maintained for years, and it isn't fully compatible
with Zeitwerk [1]. On the other hand, Ruby includes the debug gem since
version 3.1.0. We tried to start using at after commit e74eff217, but
couldn't do so because our CI was hanging forever in a test related to
machine learning, with the message:
> DEBUGGER: Attaching after process X fork to child process Y
(Note this message appeared with debug 1.6.3 but not with the version
we're currently using.)
So we're changing the debug gem fork mode in the test so it doesn't hang
anymore when running our CI. We tried to change the test so it wouldn't
call `Process.fork`, but this required changing the code, and since
there are no tests checking machine learning behavior with real scripts,
we aren't sure whether these script would keep working after changing
the code.
[1] Issue 564 in https://github.com/deivid-rodriguez/byebug
Note that the `budget` parameter was added to the `delete_path` method
so it works in the tests; on production, it worked because this
component is only rendered on pages which already have the `budget`
parameter.
Co-authored-by: Javi Martín <javim@elretirao.net>
Note that we keep :created_at order as complement to new :order field.
We do this so that current installations will not notice any change in the
sorting of their cards when upgrading, as the default "order" field will always
be 1, so it will continue to sort by the "created_at".
Before this change, two important things depend on the format of each key,
where to render it in the administration panel and which kind of interface
to use for each setting. Following this strategy led us to a very complex
code, very difficult to maintain or modify. So, we do not want to depend
on the setting key structure anymore to decide how or where to render each
setting.
With this commit, we get rid of the key format-based rules. Now we render
each setting explicitly passing to it the type and the tab where it belongs.
This way only verified users will be able to access this page, which
shows the username of the receiver of the direct message. With this,
it's no longer possible for unverified users to browse direct message
URLs in order to collect usernames from every user.
We were already opening them in the same window because we were
accidentall sanitizing the `target` attribute, but now we're making the
point more explicit.
As far as possible I think the code is clearer if we use CRUD actions
rather than custom actions. This will make it easier to add the action
to remove votes in the next commit.
Note that we are adding this line as we need to validate it that a vote
can be created on a comment by the current user:
```authorize! :create, Vote.new(voter: current_user, votable: @comment)```
We have done it this way and not with the following code as you might
expect, as this way two votes are created instead of one.
```load_and_authorize_resource through: :comment, through_association: :votes_for```
This line tries to load the resource @comment and through the association
"votes_for" it tries to create a new vote associated to that debate.
Therefore a vote is created when trying to authorise the resource and
then another one in the create action, when calling @comment.vote.
As far as possible I think the code is clearer if we use CRUD actions
rather than custom actions. This will make it easier to add the action
to remove votes in the next commit.
Note that we are adding this line as we need to validate it that a vote
can be created on a debate by the current user:
```authorize! :create, Vote.new(voter: current_user, votable: @debate)```
We have done it this way and not with the following code as you might
expect, as this way two votes are created instead of one.
```load_and_authorize_resource through: :debate, through_association: :votes_for```
This line tries to load the resource @debate and through the association
"votes_for" it tries to create a new vote associated to that debate.
Therefore a vote is created when trying to authorise the resource and
then another one in the create action, when calling @debate.vote_by (which
is called by @debate.register_vote).
We originally added the `cached_votes_up > 0` in commit 4ce95e273
because back then `cached_votes_up` was used in the denominator. That's
no longer the case, and it doesn't make sense to mark a debate with 1
vote and 10 flags as conflictive but not doing it when the debate has no
votes and 1000 flags.
We're fixing the bug right now because we're about to change the
affected line in order to apply a new rubocop rule.
This rule was added in rubocop-rspec 2.11.0. We aren't adding it
because, out of 3 offenses, this cop can only correct 2 automatically.
Not sure how to correct the other one since it uses `.and change`.
This rule was added in rubocop-rspec 2.9.0.
We were using `be_nil` 50% of the time, and `be nil` the rest of the
time. No strong preference for either one, but IMHO we don't lose
anything be being consistent.
Note we're excluding a few files:
* Configuration files that weren't generated by us
* Migration files that weren't generated by us
* The Gemfile, since it includes an important comment that must be on
the same line as the gem declaration
* The Budget::Stats class, since the heading statistics are a mess and
having shorter lines would require a lot of refactoring
