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

Rails 7 Upgrade #1138

Closed
wants to merge 7 commits into from
Closed

Rails 7 Upgrade #1138

wants to merge 7 commits into from

Conversation

archonic
Copy link
Contributor

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 use to_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 a www 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

  • I have the time and interest to make additional changes to this PR based on feedback.

@archonic archonic changed the title Rails-7 Rails 7 Upgrade Sep 30, 2023
@jim jim self-requested a review October 3, 2023 01:10
Copy link
Member

@jim jim left a 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.

@phinze
Copy link
Contributor

phinze commented Oct 14, 2023

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!

@archonic
Copy link
Contributor Author

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.

@phinze
Copy link
Contributor

phinze commented Oct 31, 2023

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!

@archonic
Copy link
Contributor Author

archonic commented Nov 5, 2023

@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!

@phinze
Copy link
Contributor

phinze commented Nov 6, 2023

No problem @archonic I'll take it from here - thanks for all the work on it so far!

@phinze phinze self-assigned this Nov 6, 2023
@phinze phinze mentioned this pull request Nov 6, 2023
@phinze
Copy link
Contributor

phinze commented Nov 6, 2023

Will finish this up and bring it home in #1178 🚀

@phinze phinze closed this Nov 6, 2023
phinze added a commit that referenced this pull request Nov 10, 2023
# 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to Rails 7
3 participants