Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump rails 6.0 to to 6.1 #3753

Merged
merged 23 commits into from
Jun 22, 2021
Merged

Bump rails 6.0 to to 6.1 #3753

merged 23 commits into from
Jun 22, 2021

Conversation

jsugarman
Copy link
Contributor

@jsugarman jsugarman commented Feb 12, 2021

What

Bump rails 6.0 to to 6.1

Why

Maintenance and support

TODO:

  • all tests passing
  • handle deprecation warnings in app
  • handle deprecation warnings in gov_uk_date_fields gem
  • run through this upgrade guide
  • check Gemfile.lock on RailsBump
  • manual test of external user flows
  • manual test of case worker user flows

@jsugarman jsugarman added the WIP label Feb 12, 2021
@jrmhaig jrmhaig force-pushed the bump-rails-6.0-to-6.1 branch 3 times, most recently from d952097 to d953ec9 Compare May 12, 2021 16:12
@jsugarman jsugarman force-pushed the bump-rails-6.0-to-6.1 branch 6 times, most recently from f5fb2da to dab4a18 Compare June 14, 2021 21:57
@jsugarman jsugarman force-pushed the bump-rails-6.0-to-6.1 branch 3 times, most recently from 761fb0f to ed262ed Compare June 21, 2021 12:48
Gemfile Outdated Show resolved Hide resolved
@jsugarman jsugarman force-pushed the bump-rails-6.0-to-6.1 branch 2 times, most recently from e7b6e87 to cc3b2d8 Compare June 22, 2021 07:19
add_foreign_key "claims", "case_stages", name: "fk_claims_case_stage_id"
add_foreign_key "injection_attempts", "claims"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added in this #3940 and I do not know why.

jsugarman and others added 5 commits June 22, 2021 11:33
1) TypeError: module attributes should be defined directly on class, not
   singleton
  * cattr_accessor must be outside of a `class << self` block
2) NameError: uninitialized constant Rack::NoAnimations
  * rack/no_animations and rack/no_popups need to be explicitly included in
    config/environments/test.rb
When running in CircleCI it fails as it is not able to create tmp/pids/server.pid
jrmhaig and others added 18 commits June 22, 2021 11:33
Model errors have changed in Rails 6.1 and new errors need to be added with:

```ruby
errors.add(:attribute, 'Message')
```

instead of:

```ruby
errors[:attribute] << 'Message'
```

However, `Allocation#rollback_all_allocations!` prepends a message to an
existing list of errors and this cannot be done with `.add` so some behaviour
has to be changed.
After fixing `ErrorPresenter.generate_messages` to use Active Model errors
correctly for Rails 6.1 it triggered a Metrics/AbcSize Rubocop offense. I have
made a small refactor that clears this but more could be done.

When iterating over `@errors` there is now just one loop variable representing
each error instead of the field name and message, as there was before. This is
now passed to `populate_messages` as a single argument instead of three,
including an instance of `ErrorMessageTranslator`.

Further work could be done to move the logic inside `populate_messages` into
`ErrorMessageTranslator` and update `ErrorDetails` so that its initializer
can take an instance of `ErrorMessageTranslator` as an argument. This is
(probably) outside of the scope of this PR (upgrading to Rails 6.1).
This scenario should not happen and now it
raises a `ActiveSupport::MessageVerifier::InvalidSignature`
error.

Could be related to this new feature in 6.1
https://blog.saeloun.com/2020/05/20/rails-6-1-adds-support-for-signed-ids-to-active-record.html
rails/rails#39313

And various issues that were supposedly fixed
rails/rails#41233
The previous method of mocking an error
on the fee object had ceased to work. It is
unclear what rails 6.1 change caused the issue.

The fix involves actually validating the
fee which therefore is more of an integration
test.
The first two of these appears to simply be the
an ordering of columns issue that may reflect changes
in activerecord 6.1

The last deletion of a FK is odd?!
```
 - add_foreign_key "injection_attempts", "claims"
```
Handle
```
DEPRECATION WARNING: Enumerating ActiveModel::Errors as a hash has been deprecated.
In Rails 6.1, `errors` is an array of Error objects,
therefore it should be accessed by a block with a single block
parameter like this:

person.errors.each do |error|
  attribute = error.attribute
  message = error.message
end

You are passing a block expecting two parameters,
so the old hash behavior is simulated. As this is deprecated,
this will result in an ArgumentError in Rails 6.2.
(called from copy_errors_to_base_record at /home/circleci/repo/app/validators/base_sub_model_validator.rb:54)
```

...and

```
DEPRECATION WARNING: Calling `<<` to an ActiveModel::Errors message array in order to add an error is deprecated.
Please call `ActiveModel::Errors#add` instead.
(called from block in copy_errors_to_base_record at /home/circleci/repo/app/validators/base_sub_model_validator.rb:57)
```
Fixes
```
DEPRECATION WARNING: Calling `clear` to an ActiveModel::Errors message array in order to delete all errors is deprecated.
Please call `ActiveModel::Errors#delete` instead.
(called from clear_pre_existing_error at app/validators/base_validator.rb:85)
```
prevent output which IMO is intended to warn
devs that there is a problem with the test rather
than whether it is applicable or not.
Fixes
```
DEPRECATION WARNING: ActiveModel::Errors#keys is deprecated and will be removed in Rails 6.2.
To achieve the same use:
  errors.attribute_names (called from block (4 levels) in <top (required)> at spec/validators/fee/base_fee_validator_spec.rb:237)
```
Fixes:
```
DEPRECATION WARNING: Calling `<<` to an ActiveModel::Errors message array in order to add an error is deprecated.
Please call `ActiveModel::Errors#add` instead.
(called from offence_class_xor_offence_band at /Users/jsugarman/rails_projects/ds_work/cccd/Claim-for-Crown-Court-Defence/app/models/offence.rb:58)
(called from roles_valid at /Users/jsugarman/rails_projects/ds_work/cccd/Claim-for-Crown-Court-Defence/app/models/concerns/roles.rb:52)
(called from expenses_valid at /Users/jsugarman/rails_projects/ds_work/cccd/Claim-for-Crown-Court-Defence/app/models/determination.rb:73)
(called from expenses_valid at /Users/jsugarman/rails_projects/ds_work/cccd/Claim-for-Crown-Court-Defence/app/models/determination.rb:77)
(called from expenses_valid at /Users/jsugarman/rails_projects/ds_work/cccd/Claim-for-Crown-Court-Defence/app/models/determination.rb:81)
(called from block (3 levels) in <top (required)> at /Users/jsugarman/rails_projects/ds_work/cccd/Claim-for-Crown-Court-Defence/spec/models/claim/transfer_detail_spec.rb:56)
```
Follows our official release of 3.2.0 with
6.1 support.
Additional submodel errors were appearing on the
model instances and then the presenter was falling
back to displaying them in the summary.

These errors are duplicates of other errors already
being handled by the custom error handler and, in
addition, do not result in a working links to the
relevant (or any) field.

I have widdened the deletion to delete any error
with an attribute indicating it is a nested error.

This should be safe to do as such errors are copied
to the parent/base record in a specifci custom format
for handling by the custom error presenter.

I have sanity tested its impact on a final claim for fixed fees
Use delegation and aliases instead of defined
methods that simply call the behaviour of the
delegated object.

Since we now alias `errors_for?` as `keys?` to
match the behaviour of `ActiveModel::Errors.keys?`
we can use this in the adp_text_field following
duck-typing principles.
Copy link
Contributor

@jrmhaig jrmhaig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot see any blockers to this. I will test it on dev-lgfs.

@jsugarman jsugarman merged commit e72f698 into master Jun 22, 2021
@jsugarman jsugarman deleted the bump-rails-6.0-to-6.1 branch June 22, 2021 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants