Ruby 3 #1260
Ruby 3 #1260
Conversation
…d and railssettings-ui)
…estHelper everywhere
|
We'll probably have to ignore the migration test failure here. It tries to install some old gems (whatever is on main) and that fails. |
Dantemss
force-pushed
the
ruby3
branch
2 times, most recently
from
e756337
to
4f80a30
Compare
|
how do we make the migration test not fail? should we just get rid of it? |
|
I'd say either remove or comment out the file until we merge. It's saved me a few times. For the associated gems, I think I approved all of them, but I only have access to deploy openstax_salesforce (I think) - but I agree, let's get them merged and deployed. |
|
@TomWoodward the migration test is useful when we're not updating ruby versions at least. It just fails during updates. We could fix it so it uses the proper ruby versions, probably. |
|
OK, it was actually pretty easy to fix the migration test to install ruby twice |
…ked compass-rails version
| @@ -0,0 +1,6 @@ | |||
| accounts = ActiveSupport::OrderedOptions.new | |||
| # configure how long a login token is valid for | |||
| accounts.default_login_token_expiration_period = 2.days | |||
There was a problem hiding this comment.
I'm not finding info on this - does it expire the logged in session after two days? or is this a temporary login token thing?
There was a problem hiding this comment.
ah, i see it was just a move from here https://github.com/openstax/accounts/pull/1260/files?diff=split&w=0#diff-c1fd91cb1911a0512578b99f657554526f3e1421decdb9e908712beab57e10f9L30
There was a problem hiding this comment.
yeah sorry, I moved the config in application.rb and some of environment.rb to individual initializers
| @@ -48,24 +46,28 @@ | |||
| # the maximum value specified for Puma. Default is set to 5 threads for minimum | |||
| # and maximum; this matches the default thread size of Active Record. | |||
| # | |||
| max_threads = ENV.fetch('RAILS_MAX_THREADS', 5).to_i | |||
| threads ENV.fetch('RAILS_MIN_THREADS', max_threads).to_i, max_threads | |||
| max_threads_count = ENV.fetch('RAILS_MAX_THREADS') { 5 } | |||
There was a problem hiding this comment.
no need to case this as an integer anymore? looks like it's still happening on line 56
There was a problem hiding this comment.
The default Rails 6.1 puma config is:
max_threads_count = ENV.fetch("RAILS_MAX_THREADS") { 5 }
min_threads_count = ENV.fetch("RAILS_MIN_THREADS") { max_threads_count }
threads min_threads_count, max_threads_count
worker_timeout 3600 if ENV.fetch("RAILS_ENV", "development") == "development"
so I figured threads can handle strings but idk about worker_timeout
| field :send_google_analytics, type: :boolean, default: false | ||
| field :google_analytics_code, type: :string, default: 'UA-73668038-2' | ||
| field :google_tag_manager_code, type: :string, default: 'GTM-W6N7PB' | ||
| field :subjects, type: :hash, default: { |
There was a problem hiding this comment.
I don't think these are used anymore - the books get fetched from the CMS... but we can save removing this for another PR.
| @@ -104,30 +105,32 @@ module Newflow | |||
| end | |||
|
|
|||
| example 'arriving from Tutor (a Doorkeeper app)' do | |||
There was a problem hiding this comment.
Are we leaving Tutor here as an inside joke 😄
There was a problem hiding this comment.
I didn't want to get too much into cleanup... I don't even know if the Message API and records are/were ever used.
There was a problem hiding this comment.
Been there, done that. I agree, let's just get off EOL stuff for now.
Make sure to disable dev routes in prod
| @@ -9,7 +9,7 @@ | |||
| next | |||
| end | |||
|
|
|||
| next if !Rails.application.is_real_production? || current_user.is_administrator? | |||
| next if !is_real_production? || current_user.is_administrator? | |||
There was a problem hiding this comment.
is this working because the initializer is named is_real_production, or can you always access methods from other config files?
There was a problem hiding this comment.
The initializer just defines a global is_real_production? method.
There was a problem hiding this comment.
Huh... how is it reading secrets though? hmm
Also it's only used in 1 place, I might as well move it there
| return true if request.format != :html || request.options? | ||
| redirect_to '/signup/profile' if current_user.is_needs_profile? | ||
| # TODO: uncomment this line after fixing openstax_path_prefixer | ||
| # redirect_to main_app.signup_profile_path if current_user.is_needs_profile? |
There was a problem hiding this comment.
is this still a problem?
actually, i don't even think /signup/profile is a path - so maybe best to leave this can of worms unopened
There was a problem hiding this comment.
It looked like a lot to review, but really just a bunch of cleanup - really nice work!
I think I got PTSD looking through some of it. RIP v14.
I'm going to approve. It seems things are working for me locally, once we feel good about it, we can stick it on dev and see what it does (logs everyone out??)
Most my comments are just for my own edification on how stuff is working, not anything I see needing changed for a merge.
|
There are some tests that sporadically fail on GH actions with a net read timeout, but I'm not sure if it's our code or just GH action having issues. |
|
main is sometimes failing, so it might be something in our code |
|
Maybe: teamcapybara/capybara#2770 Someone mentions there the playwright driver works much better. We use also playwright in rex-web. I'll see how easy it is to switch from Selenium. |
Things you may need:
Changes:
It may be a good idea to get all these gems that are loaded from GitHub merged and released.