We were getting a warning since we upgraded to Rails 7.1:
> Rails 7.1 has deprecated the singular fixture_path in favour of an
> array.You should migrate to plural:
We were getting a warning after upgrading to Rails 7.1:
```
DEPRECATION WARNING: ActionDispatch::IllegalStateError is deprecated
without replacement.
```
We were getting a deprecation warning after upgrading to Rails 7.1:
```
DEPRECATION WARNING: Setting action_dispatch.show_exceptions to true is
deprecated. Set to :all instead.
```
So we're passing `:all`.
I've checked whether we can remove the `:show_exceptions` tag completely
now that the new default option is `:rescuable`. The answer is "no" ;).
Removing the `:show_exceptions` tag makes the tests fail.
We were getting the following warning after upgrading to Rails 7.1:
```
Rendered shared/_social_media_meta_tags.html.erb
DEPRECATION WARNING: Passing the class as positional argument is
deprecated and will be removed in Rails 7.2.
Please pass the class as a keyword argument:
serialize :metadata, type: Object
```
We were getting a ton of deprecation warnings:
```
DEPRECATION WARNING: `Rails.application.secrets` is deprecated in favor
of `Rails.application.credentials` and will be removed in Rails 7.2
```
However, we don't plan to replace the secrets with credentials in the
foreseeable future because it would affect existing Consul Democracy
installations, so for now we're simply silencing the warnings.
This is an error we've only been able to reproduce on one specific
machine and only when using the development environment.
It looks like Rails 7.1.5.1 uses `ActiveStorage::PreviewImageJob` when
we attach a PDF. However, that raises an IntegrityError because we're
removing the metadata from PDFs. That is, the final PDF is no longer the
same PDF that was uploaded.
This issue wasn't present in the original Rails 7.1.0 release, but was
introduced in Rails 7.1.4 [1] and has already been fixed in Rails 7.2.0
[2].
So we're applying the same solution that was applied in Rails 7.2.0:
disabling automatic previews for PDFs when no variants require them by
changing a method in `ActiveStorage::Attachment`.
[1] See commit 6f729dd39 in https://github.com/rails/rails/
[2] See pull request 51351 in https://github.com/rails/rails/
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
We were using a rack browser and testing the
`FeatureFlags::FeatureDisabled` exception was raised. However, we don't
test this exception in any other system tests; we only check it in
controller tests.
So we're using a real browser for consistency and because in Rails 7.1
this test is failing when enabling the new default option
`config.action_dispatch.show_exceptions = :rescuable` in the test
environment.
Apparently, the `ids` method, which originally was implemented as
`pluck(:id)`, sometimes returned duplicate ids [1]. Fixing that issue
generated another issue [2] when combining `.includes` and `order`.
There was an attempt to fix it [3], but it still doesn't fix the case
where the ordering column belongs to an association [4].
This means that, when upgrading to Rails 7.1, calling `.ids` on
`budget.investments.includes(:heading).sort_by_random.for_render`
results in an invalid SQL statement:
```
ActiveRecord::StatementInvalid:
PG::GroupingError: ERROR: column "ids.ordering" must appear in
the GROUP BY clause or be used in an aggregate function
LINE 1: ... = $4 GROUP BY "budget_investments"."id" ORDER BY
ids.orderi...
```
To solve it, we could once again use `.pluck(:id)` instead of `ids`, but
I'm not sure whether there would be a risk to get duplicate IDs in some
cases. We cannot call `.reorder` or `.unscope(:order)` either because we
need the IDs to be ordered randomly.
So we're removing the `includes(:heading)` part when getting the IDs.
Since using `includes` generates a separate query that doesn't affect
the query to get the IDs, removing it makes no difference.
Another option would be to remove the `for_render` call, since we're
including the headings to avoid the N+1 queries problem, but we're doing
so without benchmarks showing that it does actually make a difference.
[1] Issue 46455 in https://github.com/rails/rails
[2] Issue 48080 in https://github.com/rails/rails
[3] Pull request 48101 in https://github.com/rails/rails
[4] Issue 50263 in https://github.com/rails/rails
The "foundation_rails_helper" gem is no longer maintained and is
incompatible with Rails 7.1. To avoid blocking the upgrade, we've vendored
the vendor/foundation_rails_helper/form_builder.rb as a copy of the
original FormBuilder class.
To mantain compatibility with auto_labels and button_class variables, that
are used in the original builder, we are overwriting in the foundation
form builder initializer.
The gem has been removed from the Gemfile and replaced with this vendored
fallback. This workaround is safe to remove once legacy Foundation CSS
support is dropped.
All vendored code retains the original MIT license and attribution.
ros-apartment 3.0.0+ includes official support for connection handling in Rails 7,
so we no longer need to override `ActiveRecord::ConnectionHandling#connected_to`.
References: PR #194 and #243 in ros-apartment
We were using a placeholder, which is way less accessible than a label.
One issue here (which also happened before, but is now more obvious) is
that, when adding several options, there will be many fields with the
same label.
Another issue is that, for some languages, we're using texts like "Add a
closed answer", which might be confusing because we might be editing an
existing answer. The proper solution would probably be using the text
"Option 1", "Option 2", ... I'm not doing so right now because I'm not
sure that's a good option and because changing the text would mean
losing the existing translations.
This way the fields are easier to use, and we can get rid of the
placeholders.
Note we're simplifying the `answer_result_value` in order to make it
easier to return `0` instead of `nil` when the field is empty.
Also note there's a small change of behavior here; previously, these
fields were empty by default, and now their value is `0` by default, so
blindly clicking the "Save" button would send `0` instead of an empty
value. I don't think it's a big deal, though, but we need to keep that
in mind.
Back when we added all the missing labels (changes that we merged in
commit c34cab282), we forgot about fields which had placeholdes, since
Axe doesn't report an error when there are placeholders but there aren't
labels.
In this case, we were using an invalid <label> tag for the question
options, and <h3> tags as labels for the votes.
Using standard labels solves the issue.
Saying that we're supposed to introduce a descriptive title in a field
labelled as "Title" is redundant. Besides, the text of the placeholder
was barely distinguishable, making it harder to fill in the form.
We forgot to apply this change in commit f5f96ba86.
Note that, in this case, executing `proposal_notification.author.email`
in the middle of a test would also result in a database query. For some
reason (probably the same reason why the code that explicitly created
the author was added in this test but not in other moderation tests),
that doesn't seem to happen in other moderation tests, so for now we
aren't changing those ones.
The link to the comments page was an "expand" icon, which was confusing
because it wasn't really expanding the contents of the sidebar but going
to an entirely different page. Furthermore, it didn't have any text for
people using screen readers, which is why Axe was reporting the
following accessibility error:
```
link-name: Links must have discernible text (serious)
https://dequeuniversity.com/rules/axe/4.10/link-name?application=axeAPI
The following 1 node violate this rule:
Selector: #annotation-link > a
HTML: <a href="/legislation/processes/75/draft_versions/30/annotations/8?sub_annotation_ids=">
<span class="icon-expand" aria-hidden="true"></span>
</a>
Fix all of the following:
- Element is in tab order and does not have accessible text
Fix any of the following:
- Element does not have text that is visible to screen readers
- aria-label attribute does not exist or is empty
- aria-labelledby attribute does not exist, references elements that
do not exist or references elements that are empty
- Element has no title attribute
```
So we're removing the icon and turning the "N comments" text into a
link, so it's easier to guess that the link takes us to the page showing
all the comments for this annotation.
This expectations in this test were true both before and after clicking
on the `.icon-expand` link, so it was possible that the test finished
before the request generated by that click did.
So we're adding an extra expectation to make sure we're testing what we
want to test: the content of the page after the request has finished.
Just like we do with the rest of the phases.
The reason why we're making this change right now is that we were
getting an accessibility error with processes with no result publication
date:
```
link-name: Links must have discernible text (serious)
https://dequeuniversity.com/rules/axe/4.10/link-name?application=axeAPI
The following 1 node violate this rule:
Selector: p:nth-child(6) > a
HTML: <a href="/legislation/processes/39/result_publication">
<strong></strong>
</a>
Fix all of the following:
- Element is in tab order and does not have accessible text
Fix any of the following:
- Element does not have text that is visible to screen readers
- aria-label attribute does not exist or is empty
- aria-labelledby attribute does not exist, references elements that
do not exist or references elements that are empty
- Element has no title attribute
```
