-
Notifications
You must be signed in to change notification settings - Fork 67
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
Rails 7 Upgrade #1138
Rails 7 Upgrade #1138
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks for working on this for us. I was worried about this upgrade but it looks to have not been too bad after all.
I read through the changes and everything generally looks good. The Github checks detected a few things to address before we merge this:
- There are a couple test failures. They seem to be related to timezones or date comparisons.
- Standard found some code formatting issues.
- ERBLint flagged a place where a safe operator should be used.
The commands to fix 2 and 3 are in the README- for the most part it's usually a one-liner to resolve them.
Let me know if you I can help at all with resolving these minor issues. This is really close and it'll be so exciting to have this upgrade done.
This is looking really close and would be great to get merged. LMK if you could use any help on the last mile @archonic I'd be happy to pitch in! |
I'm away til the 22nd, but happy to take another swing after that. Or if someone else wants to fix the failures, please do. |
Hope your time away was good @archonic 😄 How are you feeling about coming back around to this? If you don't think you'll have the time in the next week or so, I'll probably take it over the finish line. There are a few things in Rails 7 that I'd like to use in some other work. Let me know either way! |
@phinze Hi, it was great! I thought I'd have more time for this but I won't be able to do anything on it till next weekend. Feel free to take it across the finish line! |
No problem @archonic I'll take it from here - thanks for all the work on it so far! |
Will finish this up and bring it home in #1178 🚀 |
# What it does Upgrades the application to Rails 7. # Why it is important Getting on the latest version of Rails unlocks a few underlying features we can use to implement improvements to the application. # Implementation notes * This builds on and supersedes #1138 * This holds off on switching to `vips` for image processing. We will probably do that as part of follow on work on ActiveStorage variants. * The [upgrade guide](https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#upgrading-from-rails-6-1-to-rails-7-0) suggests there are changes to `button_to`. No system tests are failing but we should audit the behavior to make sure everything still works. --------- Co-authored-by: Joshua Flark <[email protected]>
What it does
Upgrades to Rail 7 and closes #975.
Why it is important
Upgrades are good 👍
Implementation notes
There was one deprecated use of
to_s
which we can now useto_sf
for. A few other deprecations had no real impact according to tests. There was some host redirection issues as well which were fixed in one case by just allowing an external redirect to a payment completion page. Another fix for a host redirect was actually just awww
issue. The fix assumes that the shortlinks controller is not meant to be used for external URLs.Your bandwidth for additional changes to this PR