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 4.1] Upgrade OFN to Rails 4.1 #5386

Closed
luisramos0 opened this issue May 7, 2020 · 5 comments · Fixed by #6297
Closed

[Rails 4.1] Upgrade OFN to Rails 4.1 #5386

luisramos0 opened this issue May 7, 2020 · 5 comments · Fixed by #6297
Labels
epic Group of issues tech debt

Comments

@luisramos0
Copy link
Contributor

luisramos0 commented May 7, 2020

What is the problem we are solving

This issue represents the effort to upgrade rails to 4.1.

This is related to #4826 but not necessarily blocked by it as we can now upgrade rails without upgrading spree.

This discourse thread contains more info about this:
https://community.openfoodnetwork.org/t/bye-bye-spree-or-spree-v2-1-and-beyond/1598/30

I read the rails 4.1 upgrade guide and I estimated this as 3 weeks of dev time. I wonder if the dual boot is worth the effort if these upgrades without spree look so easy for an app like OFN.
Would be good to get a second dev to do this estimate :-)

@luisramos0 luisramos0 added tech debt epic Group of issues labels May 7, 2020
@NickMoignard
Copy link

NickMoignard commented May 8, 2020

Should we stop upgrading at 4.1? Rails 6 + updated gems have fewer security vulnerabilities.
@luisramos0

@luisramos0
Copy link
Contributor Author

luisramos0 commented May 8, 2020

Hello Nick, no! We want to go all the way, this is only the next step. We have been making this top priority for some time now (upgraded to spree 2.0 and we are about to finish upgrade to spree 2.1 which uses rails 4.1, we will then remove spree_core and upgrade rails on our side, this issue).

What do you think? Would be great to get opinions from new contributors!

BTW, the dual boot #4800 work is a good issue to be picked up by experienced new contributors!

@NickMoignard
Copy link

Awesome, I agree with your quote it is quite a massive task, perhaps 4 weeks.

'BTW, the dual boot #4800 work is a good issue to be picked up by experienced new contributors!' awesome i'll have a look now.

@luisramos0
Copy link
Contributor Author

luisramos0 commented May 19, 2020

I think I found a way where we can do this upgrade without having to finish #4826 first. See here: #5456

That means we can start thinking about this upgrade. The official upgrade guidelines are here:
https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#upgrading-from-rails-4-0-to-rails-4-1

From this list, some of the points we may need to address are:

Upgrades (taken from spree 2-3-stable that runs on rails 4.1) that may be needed:

  • s.add_dependency 'acts_as_list', '= 0.3.0'
  • s.add_dependency 'awesome_nested_set', '~> 3.0.0.rc.3'
  • s.add_dependency 'aws-sdk-v1', '1.64.0'
  • s.add_dependency 'cancancan', '~> 1.9.2'
  • s.add_dependency 'font-awesome-rails', '~> 4.0'
  • s.add_dependency 'kaminari', '~> 0.15', '>= 0.15.1'
  • s.add_dependency 'paperclip', '~> 4.2.0'
  • s.add_dependency 'ransack', '~> 1.4.1'

@luisramos0
Copy link
Contributor Author

luisramos0 commented May 19, 2020

The sum method changed in an activerecord relation in rails 4.1. This will be needed (but only if the field, in this case count_on_hand, is a database column, if it's a method in the model & is still needed):
https://api.rubyonrails.org/classes/ActiveRecord/Calculations.html#method-i-sum
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic Group of issues tech debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants