Upgrade to Rails 7 by mitchellhenke · Pull Request #5762 · 18F/identity-idp

mitchellhenke · 2021-12-29T16:26:59Z

This is more or less ready to go with the exception of rspec supporting Rails 7:

app/controllers/openid_connect/authorization_controller.rb

.circleci/config.yml

mitchellhenke · 2022-07-20T19:38:55Z

Makefile

 	make lint_analytics_events
 	@echo "--- brakeman ---"
 	bundle exec brakeman
-	@echo "--- zeitwerk check ---"


This is no longer needed since the classic loader no longer exists in Rails 7

mitchellhenke · 2022-07-20T19:40:54Z

spec/controllers/users/webauthn_setup_controller_spec.rb

      stub_analytics
      stub_sign_in(user)
      allow(IdentityConfig.store).to receive(:domain_name).and_return('localhost:3000')
+      request.host = 'localhost:3000'


This is required to ensure that the webauthn domain name does not conflict with the host in the request, otherwise we get (and also because this isn't a request where we want to allow other hosts):

ActionController::Redirecting::UnsafeRedirectError: Unsafe redirect to "http://localhost/account", pass allow_other_host: true to redirect anyway.

mitchellhenke · 2022-07-20T19:42:22Z

spec/helpers/script_helper_spec.rb

          visible: :all,
          text: {
-            'clock.svg' => '/clock.svg',
+            'clock.svg' => 'http://test.host/clock.svg',


This is due to request now existing when the function is called in this spec here:

identity-idp/app/helpers/script_helper.rb

Lines 78 to 80 in f2bc030

elsif request

request.base_url

end

I'm not sure why it would have changed, but I don't think it affects the primary motivation behind the test?

mitchellhenke · 2022-07-20T19:43:19Z

config/application.rb

-    # Generate CSRF tokens that are encoded in URL-safe Base64.
-    #
-    # This change is not backwards compatible with earlier Rails versions.
-    # It's best enabled when your entire app is migrated and stable on 6.1.


We have been stable on 6.1 for some time now 🙂

app/services/request_password_reset.rb

mitchellhenke · 2022-07-20T19:46:46Z

spec/support/matchers/have_actions.rb

-  only_option = if_option_for(action)[0]
-
-  "#{only_option.class}OptionParser".constantize.new(only_option).parse
+  if_option_for(action)[0]


I'll be honest, I don't have confidence in my understanding of this file's metaprogramming, and I'd rather see us test the behaviors around the before_actions via controller specs than ensure a method is called.

omg I didn't realize this file was parsing the source, and not just introspecting the controller in memory. but +1 would rather see something like expect(controller).to receive(:some_action).and_call_original

It's passing around a bunch of stuff and has to call instance_variable_get to get private instance variables, which isn't ideal and it's hard to follow the process. I don't mind it as kind of a simpler check if it was well-supported, but it's unfortunately not the case.

It is used pretty extensively though.

zachmargolis

LGTM!

changelog: Internal, Dependencies, Update to Rails 7

mitchellhenke force-pushed the mitchellhenke/rails-7 branch 3 times, most recently from 1c0dc32 to dc29e91 Compare December 29, 2021 20:27

zachmargolis reviewed Dec 29, 2021

View reviewed changes

app/controllers/openid_connect/authorization_controller.rb Outdated Show resolved Hide resolved

mitchellhenke force-pushed the mitchellhenke/rails-7 branch from dc29e91 to e7b97fc Compare December 30, 2021 15:28

mitchellhenke force-pushed the mitchellhenke/rails-7 branch from e7b97fc to 8d8d294 Compare January 10, 2022 16:24

mitchellhenke force-pushed the mitchellhenke/rails-7 branch 2 times, most recently from c608c4f to 9db6bca Compare February 1, 2022 14:05

mitchellhenke force-pushed the mitchellhenke/rails-7 branch 7 times, most recently from cd552d9 to d6c9a74 Compare February 15, 2022 17:35

mitchellhenke force-pushed the mitchellhenke/rails-7 branch from 830059e to f4762ee Compare March 3, 2022 16:23

zachmargolis reviewed Mar 3, 2022

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

mitchellhenke force-pushed the mitchellhenke/rails-7 branch from ec072da to e60e0c5 Compare March 16, 2022 14:12

mitchellhenke force-pushed the mitchellhenke/rails-7 branch from e60e0c5 to e89e062 Compare March 23, 2022 18:16

mitchellhenke force-pushed the mitchellhenke/rails-7 branch from e89e062 to 1637868 Compare May 4, 2022 16:30

mitchellhenke force-pushed the mitchellhenke/rails-7 branch 3 times, most recently from 2d702a5 to a437d73 Compare May 19, 2022 15:21

mitchellhenke force-pushed the mitchellhenke/rails-7 branch 4 times, most recently from a729d9b to 35aba3d Compare June 2, 2022 01:57

mitchellhenke force-pushed the mitchellhenke/rails-7 branch from 35aba3d to 0568c0f Compare July 20, 2022 19:26

mitchellhenke changed the title ~~WIP: Upgrade to Rails 7~~ Upgrade to Rails 7 Jul 20, 2022

mitchellhenke force-pushed the mitchellhenke/rails-7 branch from 0568c0f to f2bc030 Compare July 20, 2022 19:29

mitchellhenke marked this pull request as ready for review July 20, 2022 19:29

mitchellhenke commented Jul 20, 2022

View reviewed changes

app/services/request_password_reset.rb Outdated Show resolved Hide resolved

mitchellhenke commented Jul 20, 2022

View reviewed changes

mitchellhenke force-pushed the mitchellhenke/rails-7 branch 3 times, most recently from 2b86d9a to a635f27 Compare July 21, 2022 15:25

mitchellhenke requested review from aduth and jmhooper July 22, 2022 13:50

mitchellhenke force-pushed the mitchellhenke/rails-7 branch from a635f27 to 8c2b310 Compare July 22, 2022 13:51

mitchellhenke requested review from jmdembe and mdiarra3 July 22, 2022 16:16

mitchellhenke force-pushed the mitchellhenke/rails-7 branch from 8c2b310 to f2cc0c3 Compare July 22, 2022 16:18

zachmargolis approved these changes Jul 27, 2022

View reviewed changes

Mitchell Henke added 7 commits July 28, 2022 09:54

upgrade to rails 7

07e8baf

fix N+1

7a18b02

allow external redirect to PIVCAC

4f4b384

placeholder

d92af2e

changelog: Internal, Dependencies, Update to Rails 7

hm

e130554

fix spec

c127f97

hm

8a17616

mitchellhenke force-pushed the mitchellhenke/rails-7 branch from f2cc0c3 to 8a17616 Compare July 28, 2022 14:54

mitchellhenke merged commit c6ccbf1 into main Jul 28, 2022

mitchellhenke deleted the mitchellhenke/rails-7 branch July 28, 2022 16:10

solipet mentioned this pull request Aug 9, 2022

Deploy RC 202 to Prod #6688

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Rails 7#5762

Upgrade to Rails 7#5762
mitchellhenke merged 7 commits intomainfrom
mitchellhenke/rails-7

mitchellhenke commented Dec 29, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

mitchellhenke Jul 20, 2022

Uh oh!

mitchellhenke Jul 20, 2022

Uh oh!

mitchellhenke Jul 20, 2022

Uh oh!

mitchellhenke Jul 20, 2022

Uh oh!

Uh oh!

mitchellhenke Jul 20, 2022

Uh oh!

zachmargolis Jul 20, 2022

Uh oh!

mitchellhenke Jul 20, 2022

Uh oh!

zachmargolis left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mitchellhenke commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mitchellhenke Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

mitchellhenke Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

mitchellhenke Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

mitchellhenke Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mitchellhenke Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

zachmargolis Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

mitchellhenke Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mitchellhenke commented Dec 29, 2021 •

edited

Loading