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.

Sign up for GitHub

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

Jump to bottom

Upgrade to Rails 7.0 #10440

Merged
merged 7 commits into from
Mar 23, 2023
Merged

Upgrade to Rails 7.0 #10440

merged 7 commits into from
Mar 23, 2023

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented Feb 14, 2023 •
edited
Loading

What? Why?

Using the latest version of Rails gives us the latest tools and best compatibility. It speeds up development and unblocks our transition of the asset pipeline.

What should we test?

  • Log in before deploy and make sure that you are still logged in after.
  • Test uploading images including error messages when uploading an unsupported image.
  • Test checkout.

Release notes

Changelog Category: Technical changes

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

Follow-up issues to create:

@mkllnk mkllnk self-assigned this Feb 14, 2023
@mkllnk mkllnk mentioned this pull request Feb 28, 2023
Merged
@mkllnk mkllnk mentioned this pull request Mar 5, 2023
Merged
@filipefurtad0
Copy link
Contributor

I had a look at the failing system spec. It's on the sales tax by producers report:

rspec ./spec/system/admin/reports/sales_tax/sales_tax_totals_by_producer_spec.rb:344

The filters producer, order cycle and customer seem not be appearing on the page - pic taken from the artifacts from GH Actions:
image

In master this appears correctly - pic taken from staging:

image

So, this one is not really spec related, perhaps a code change is needed. Any ideas @abdellani ? (pinging you as you were working on this recently)

@filipefurtad0 filipefurtad0 mentioned this pull request Mar 6, 2023
Closed
@mkllnk mkllnk force-pushed the rails7 branch 2 times, most recently from ffdc175 to d2b208c Compare March 7, 2023 00:46
@abdellani

This comment was marked as resolved.

Sign in to view
@mkllnk
Copy link
Member Author

mkllnk commented Mar 7, 2023

I switched to this branch and run bundle before running rails s.

It's always good to run yarn as well.

@mkllnk mkllnk mentioned this pull request Mar 7, 2023
Merged
@abdellani
Copy link
Member

It's always good to run yarn as well.

@mkllnk I fixed the issue
I was running ./bin/webpack-dev-server on another project.

abdellani
abdellani reviewed Mar 8, 2023
View reviewed changes
Copy link
Member

@abdellani abdellani left a comment

Choose a reason for hiding this comment

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

The problem was on the "lookup_context.exists?".
I just needed to remove the last "/" from the partial path.

@mkllnk
Copy link
Member Author

mkllnk commented Mar 9, 2023

Legend, thanks @abdellani! Once the included PRs are merged I'll rebase this one and we can do some testing.

@mkllnk mkllnk force-pushed the rails7 branch 3 times, most recently from be1876a to 50068fb Compare March 15, 2023 02:07
mkllnk and others added 7 commits March 20, 2023 10:38
@mkllnk
Omit Rails version restriction
4f44d50
@mkllnk
Bump rails from 6.1.7 to 7.0.4
61259bc 
Result of:

    bundle update --conservative rails railties\
     actioncable actionmailbox actionmailer actionpack actiontext\
     actionview activejob activemodel activerecord activestorage activesupport
@mkllnk
Update I18n spec to Rails 7
84daa04
@mkllnk
Remove obsolete setting of Zeitwerk
90a9242 
It's the default now.
https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#the-setter-config-autoloader-has-been-deleted
@mkllnk
Support old cookie encryption
bd20e29 
https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#key-generator-digest-class-changing-to-use-sha256
@mkllnk
Include Rails version in db schema
2e51d9b 
So Rails knows which defaults to apply.
https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#rails-version-is-now-included-in-the-active-record-schema-dump
@abdellani @mkllnk
remove "/" from the partial path to load sub-report filters
008b12a
@mkllnk mkllnk marked this pull request as ready for review March 19, 2023 23:43
dacook
dacook approved these changes Mar 20, 2023
View reviewed changes
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Yay, that seemed pretty painless!

jibees
jibees approved these changes Mar 20, 2023
View reviewed changes
Copy link
Contributor

@jibees jibees left a comment

Choose a reason for hiding this comment

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

Almost there!

@filipefurtad0 filipefurtad0 added the pr-staged-au staging.openfoodnetwork.org.au label Mar 23, 2023
@filipefurtad0 filipefurtad0 self-assigned this Mar 23, 2023
@filipefurtad0 filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-au staging.openfoodnetwork.org.au labels Mar 23, 2023
@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Mar 23, 2023 •
edited
Loading

Hey @mkllnk and everyone chipping in on this ❤️

Performed the suggested tests:

  • Log in before deploy and make sure that you are still logged in after 🟢

  • Test uploading images including error messages when uploading an unsupported image 🟢

    • Image upload worked fine, both product as enterprise profile images -> this was tested in a server using AWS3 (staging-AU) and ActiveStorage (staging-UK)
    • Error message was triggered as expected, by attempting to upload a PDF file:
      image

  • Test checkout: worked as before, using Stripe 🟢

This feels almost too simple compared to other Rails upgrades.
Maybe we can move it to ready to go, and include it in the next release? Meaning, merge it after the release has been prepared. This would be perhaps a bit more conservative approach.

I'll do just that, but feel free to merge if others feel otherwise.

Ready to go 🚀

@filipefurtad0 filipefurtad0 added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-uk staging.openfoodnetwork.org.uk pr-staged-au staging.openfoodnetwork.org.au labels Mar 23, 2023
@filipefurtad0
Copy link
Contributor

Actually, this was tested and will be re-tested on release testing - so I see no reason to hold this back. Merging.

@filipefurtad0 filipefurtad0 merged commit 31ffeab into openfoodfoundation:master Mar 23, 2023
@sigmundpetersen
Copy link
Contributor

🎉 I wonder if this should bump us to an OFN v4.3 for next release?

@mkllnk
Copy link
Member Author

mkllnk commented Mar 24, 2023

Wow, Rails has become a lot more mature. But I still have that "too easy" feeling as well. We'll see...

@mkllnk mkllnk deleted the rails7 branch March 24, 2023 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abdellani abdellani abdellani left review comments

@dacook dacook dacook approved these changes

@jibees jibees jibees approved these changes

Assignees

@mkllnk mkllnk

@filipefurtad0 filipefurtad0

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mkllnk @filipefurtad0 @abdellani @sigmundpetersen @jibees @dacook