Add support for Rails 6 #3236

aldesantis · 2019-06-21T12:47:45Z

Description

Adds Rails 6 support. 🍾

Progress:

Before we can merge this, the following needs to be addressed:

solidus_auth_devise needs to use secret_key_base instead of secret_token (see #160).
Ransack will only support Rails 6 after a stable version is released (see their readme). In the meantime, I've forked and fixed Ransack so we can work on Solidus.
Awesome Nested Set needs a new version released for Rails 6 support.

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)

core/config/initializers/inflections.rb

core/app/models/spree/variant/scopes.rb

core/app/models/spree/product/scopes.rb

aldesantis · 2019-06-27T08:54:01Z

On @spaghetticode's advice, I have removed the commit that converted update_attributes to update, so that the PR is less likely to cause conflicts as new work is merged into master. Also took the chance to rebase against the latest master.

kennyadsl · 2019-06-27T20:57:33Z

@aldesantis makes sense, are you going to open a new PR with the update_attribute change?

aldesantis · 2019-06-28T10:13:37Z

@kennyadsl yes, probably after we merge this one.

kennyadsl · 2019-06-28T11:35:14Z

@aldesantis isn't better to start using update before merging the Rails 6 support instead? This way we won't have the deprecation warning at all, right?

aldesantis · 2019-06-28T12:20:58Z

@kennyadsl definitely, what I meant was that instead of opening a PR now and then having to constantly keep it up to date with master, I will just wait for this PR to be merged, pull the latest master and then make the change.

skukx · 2019-08-09T22:46:56Z

@aldesantis Can you expand on rspec 3.8 and rails 6 support? I've not seen any issues on any of my rails 6 projects. I'm not saying there aren't issues, but I am curious what those could be that would require solidus to await rspec 4 before upgrading to Rails 6

aldesantis · 2019-08-12T08:40:15Z

Hi @skukx! 👋 The issue I have encountered specifically is this, which was resolved in 4-0-dev. Let me know if I missed something though!

aldesantis · 2019-08-22T14:38:54Z

We're almost there guys, we're just waiting for RSpec 4 at this point!

tvdeyen · 2019-08-22T14:40:51Z

We're almost there guys, we're just waiting for RSpec 4 at this point!

@aldesantis you can use gem 'rspec-rails', '>= 4.0.0.beta2'

Gemfile

tvdeyen · 2019-08-22T14:43:51Z

core/solidus_core.gemspec

-  s.add_dependency 'awesome_nested_set', '~> 3.0', '>= 3.0.1'
-  s.add_dependency 'cancancan', '~> 2.2'
+  s.add_dependency 'awesome_nested_set', '~> 3.2', '>= 3.0.1'
+  s.add_dependency 'cancancan', '~> 3.0'


There is no need to use 3 and above. It also works with ['>= 2.1', '< 4.0']
We want to keep our dependencies always as low as possible to avoid conflicts with other gems.

@tvdeyen I tried with >= 2.1, < 4.0, but I get tons of these:

111) Taxonomies show should display existing taxonomies Failure/Error: @search = Spree::Order.accessible_by(current_ability, :index).ransack(params[:q]) CanCan::Error: The accessible_by call cannot be used with a block ‘can’ definition.The SQL cannot be determined for :index Spree::Order

Interesting. This does not seem to be Rails 6 related to me. 🤔

I think we need #3174 as well? Maybe loading that code conditionally if Rails >= 6

With CanCanCan 3, everything seems to work without #3174. Not sure if #3174 would allow us to continue using CanCanCan 2? I don't know much about its internals, so let me know what you think the best course of action is.

I commented on #3174 that we should try to allow 2 as well. It should work as it only removes lots of quirks.

Even with the patch from #3174, I keep seeing lots of test failures when using CanCanCan 2. I think an upgrade to 3 is required, but again, let me know if you find a way to make the suite pass.

I would like to see some of these errors. I ask myself what a Rails update should change in terms of these errors from cancancan. The issue is that we try to use the same ability rule for scopes as well for single records. Then cancancan is not able to generate the correct sql for the scope. Very interesting.

I really would like to not raise a commonly used dependency like cancancan if we find a way to not need it. I know this might be disappointing and we all want to land Rails 6 support, but we also have responsibility for not breaking extensions and stores that might use dependencies that are not supporting cancancan 3 yet.

kennyadsl

Thanks @aldesantis, this PR is great! I left some preliminary comments, let me know your thoughts!

bin/build-ci

core/app/models/spree/product/scopes.rb

Gemfile

core/solidus_core.gemspec

core/spec/lib/search/base_spec.rb

core/spec/lib/spree/event_spec.rb

awesome_nested_set 3.2.0 is the first release to provide Rails 6 compatibility.

aldesantis · 2019-08-26T15:52:35Z

@kennyadsl I have rebased with more granular and detailed commits. Let me know what you think!

Gemfile

tvdeyen

In the commit message for the cancancan bump you say that 3.0 is the first version that supports Rails 6. From their gemspec this does not seem to be true. 2.x should also work. Do you have evidence for that claim? If it’s not possible without that bump, fine. But I would like us to try without the bump.

I also think we do not need to wait for rspec 4 as well.

core/solidus_core.gemspec

core/spec/lib/spree/event_spec.rb

tvdeyen · 2019-08-28T17:19:45Z

core/solidus_core.gemspec

-  s.add_dependency 'awesome_nested_set', '~> 3.0', '>= 3.0.1'
-  s.add_dependency 'cancancan', '~> 2.2'
+  s.add_dependency 'awesome_nested_set', '~> 3.2', '>= 3.0.1'
+  s.add_dependency 'cancancan', '~> 3.0'


I would like to see some of these errors. I ask myself what a Rails update should change in terms of these errors from cancancan. The issue is that we try to use the same ability rule for scopes as well for single records. Then cancancan is not able to generate the correct sql for the scope. Very interesting.

I really would like to not raise a commonly used dependency like cancancan if we find a way to not need it. I know this might be disappointing and we all want to land Rails 6 support, but we also have responsibility for not breaking extensions and stores that might use dependencies that are not supporting cancancan 3 yet.

core/lib/spree/permission_sets/default_customer.rb

aldesantis · 2019-08-29T09:07:12Z

@tvdeyen you were right on CanCanCan, I went with ['>= 2.1', '< 4.0'] and it all seems to work. Sorry about the long discussion there, I incorrectly assumed you wanted to stay on ~> 2.2 which is why I didn't see it working. Thanks!

By the way, should we use >= 2.2 instead of >= 2.1, since the previous constraint was ~> 2.2?

tvdeyen · 2019-08-29T09:41:11Z

you were right on CanCanCan, I went with ['>= 2.1', '< 4.0'] and it all seems to work. Sorry about the long discussion there, I incorrectly assumed you wanted to stay on ~> 2.2 which is why I didn't see it working. Thanks!

@aldesantis No worries. You did an amazing job!

By the way, should we use >= 2.2 instead of >= 2.1, since the previous constraint was ~> 2.2?

Yes, sorry, I just copyied it over from another project ;)

tvdeyen

Great job 🍨

Lets merge this and let people try master with Rails 6. Release the new version after everything stabilized.

aldesantis · 2019-08-29T09:46:31Z

Bumped CanCanCan's lower bound to 2.2!

aldesantis · 2019-08-29T15:20:36Z

I have also made Rails 6 the default in CircleCI and in the Gemfile, as per @kennyadsl's request.

kennyadsl

Thanks @aldesantis, this work is amazing. When we'll merge this, I'll try to release a 2.10.0.rc or alpha version so we can start having some store try Solidus with Rails 6 easily!

ericsaupe · 2019-08-29T22:44:29Z

Are we waiting for a full release of rspec-rails?

spaghetticode

Can't wait to see Solidus on Rails 6.0, thank you @aldesantis 🎉 💯 💥 🚀 ❤️

core/solidus_core.gemspec

CanCanCan 3.0.0 is the first release with Rails 6 compatibility.

state_machines-activerecord used to have a level constraint on ActiveRecord that prevented us from using it with Rails 6. This was removed[1] in 0.6.0. [1]: state-machines/state_machines-activerecord@e7857a6

Zeitwerk expects file paths to match module names exactly, so we need to use #prepend for files that extend existing modules.

In Rails 6, ActiveRecord will always cast numbers to decimals when comparing them against a decimal column.

In Rails 6, ActiveSupport notifiers use two separate instance variables for string subscribers and object subscribers, so we need to use different setup/teardown logic for the test.

The changes to the CI configuration are needed because teaspoon_env is now[1] searched starting from Rails.root when Rails is available, which breaks our test suite because Rails.root is spec/dummy, while teaspoon_env.rb is in spec. The monkey-patch was removed because Teaspoon now supports passing options to Selenium[2]. [1]: jejacks0n/teaspoon@5b912da [2]: jejacks0n/teaspoon#537

tvdeyen · 2019-09-02T14:47:12Z

🎉 🍾 💯

houndci-bot reviewed Jun 21, 2019

View reviewed changes

aldesantis mentioned this pull request Jun 21, 2019

RFC: Support Rails 6 #3053

Closed

kennyadsl added Needs Work labels Jun 26, 2019

aldesantis changed the title ~~[WIP] Rails 6~~ Rails 6 Jun 27, 2019

tvdeyen reviewed Aug 22, 2019

View reviewed changes

Gemfile Outdated Show resolved Hide resolved

tvdeyen reviewed Aug 22, 2019

View reviewed changes

kennyadsl reviewed Aug 26, 2019

View reviewed changes

aldesantis added 4 commits August 26, 2019 17:45

Add Rails 6 to CircleCI

16eaa52

Allow Rails 6

f8ccaea

Bump rspec-rails to 4.0.0.beta2 for Rails 6 support

220ea02

Bump awesome_nested_set to 3.2.0 for Rails 6 compatibility

9e73b80

awesome_nested_set 3.2.0 is the first release to provide Rails 6 compatibility.

ericsaupe reviewed Aug 26, 2019

View reviewed changes

Gemfile Show resolved Hide resolved

tvdeyen reviewed Aug 28, 2019

View reviewed changes

core/lib/spree/permission_sets/default_customer.rb Show resolved Hide resolved

tvdeyen approved these changes Aug 29, 2019

View reviewed changes

kennyadsl changed the title ~~Rails 6~~ Add support for Rails 6 Aug 29, 2019

kennyadsl approved these changes Aug 29, 2019

View reviewed changes

kennyadsl added Important Change and removed Needs Work labels Aug 29, 2019

spaghetticode approved these changes Aug 30, 2019

View reviewed changes

tvdeyen reviewed Sep 2, 2019

View reviewed changes

core/solidus_core.gemspec Show resolved Hide resolved

aldesantis added 10 commits September 2, 2019 12:30

Allow CanCanCan 3 for Rails 6 compatibility

ee7cf08

CanCanCan 3.0.0 is the first release with Rails 6 compatibility.

Bump state_machines-activerecord for Rails 6 support

cb6f72b

state_machines-activerecord used to have a level constraint on ActiveRecord that prevented us from using it with Rails 6. This was removed[1] in 0.6.0. [1]: state-machines/state_machines-activerecord@e7857a6

Add missing policy scopes to DefaultCustomer

7a731c0

Make code compatible with Zeitwerk

06df181

Zeitwerk expects file paths to match module names exactly, so we need to use #prepend for files that extend existing modules.

Fix Search::Base spec for Rails 6 compatibility

3856d95

In Rails 6, ActiveRecord will always cast numbers to decimals when comparing them against a decimal column.

Fix Event spec for Rails 6 compatibility

f95eb94

In Rails 6, ActiveSupport notifiers use two separate instance variables for string subscribers and object subscribers, so we need to use different setup/teardown logic for the test.

Fix usage of ActiveRecord migration API for Rails 6

7b5f095

Make Rails 6 the default in CircleCI

91daf84

Update readme to indicate we support Rails 6

614ecbe

kennyadsl merged commit 466ac8e into solidusio:master Sep 2, 2019

aldesantis deleted the feature/rails-6 branch September 2, 2019 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Rails 6 #3236

Add support for Rails 6 #3236

aldesantis commented Jun 21, 2019 •

edited

Loading

aldesantis commented Jun 27, 2019

kennyadsl commented Jun 27, 2019

aldesantis commented Jun 28, 2019

kennyadsl commented Jun 28, 2019

aldesantis commented Jun 28, 2019

skukx commented Aug 9, 2019 •

edited

Loading

aldesantis commented Aug 12, 2019 •

edited

Loading

aldesantis commented Aug 22, 2019 •

edited

Loading

tvdeyen commented Aug 22, 2019

tvdeyen Aug 22, 2019 •

edited

Loading

aldesantis Aug 22, 2019 •

edited

Loading

tvdeyen Aug 22, 2019

kennyadsl Aug 22, 2019

aldesantis Aug 22, 2019 •

edited

Loading

tvdeyen Aug 22, 2019

aldesantis Aug 23, 2019

tvdeyen Aug 28, 2019

kennyadsl left a comment

aldesantis commented Aug 26, 2019

tvdeyen left a comment

tvdeyen Aug 28, 2019

aldesantis commented Aug 29, 2019 •

edited

Loading

tvdeyen commented Aug 29, 2019

tvdeyen left a comment

aldesantis commented Aug 29, 2019

aldesantis commented Aug 29, 2019

kennyadsl left a comment

ericsaupe commented Aug 29, 2019

spaghetticode left a comment

tvdeyen commented Sep 2, 2019

Add support for Rails 6 #3236

Add support for Rails 6 #3236

Conversation

aldesantis commented Jun 21, 2019 • edited Loading

aldesantis commented Jun 27, 2019

kennyadsl commented Jun 27, 2019

aldesantis commented Jun 28, 2019

kennyadsl commented Jun 28, 2019

aldesantis commented Jun 28, 2019

skukx commented Aug 9, 2019 • edited Loading

aldesantis commented Aug 12, 2019 • edited Loading

aldesantis commented Aug 22, 2019 • edited Loading

tvdeyen commented Aug 22, 2019

tvdeyen Aug 22, 2019 • edited Loading

Choose a reason for hiding this comment

aldesantis Aug 22, 2019 • edited Loading

Choose a reason for hiding this comment

tvdeyen Aug 22, 2019

Choose a reason for hiding this comment

kennyadsl Aug 22, 2019

Choose a reason for hiding this comment

aldesantis Aug 22, 2019 • edited Loading

Choose a reason for hiding this comment

tvdeyen Aug 22, 2019

Choose a reason for hiding this comment

aldesantis Aug 23, 2019

Choose a reason for hiding this comment

tvdeyen Aug 28, 2019

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

aldesantis commented Aug 26, 2019

tvdeyen left a comment

Choose a reason for hiding this comment

tvdeyen Aug 28, 2019

Choose a reason for hiding this comment

aldesantis commented Aug 29, 2019 • edited Loading

tvdeyen commented Aug 29, 2019

tvdeyen left a comment

Choose a reason for hiding this comment

aldesantis commented Aug 29, 2019

aldesantis commented Aug 29, 2019

kennyadsl left a comment

Choose a reason for hiding this comment

ericsaupe commented Aug 29, 2019

spaghetticode left a comment

Choose a reason for hiding this comment

tvdeyen commented Sep 2, 2019

aldesantis commented Jun 21, 2019 •

edited

Loading

skukx commented Aug 9, 2019 •

edited

Loading

aldesantis commented Aug 12, 2019 •

edited

Loading

aldesantis commented Aug 22, 2019 •

edited

Loading

tvdeyen Aug 22, 2019 •

edited

Loading

aldesantis Aug 22, 2019 •

edited

Loading

aldesantis Aug 22, 2019 •

edited

Loading

aldesantis commented Aug 29, 2019 •

edited

Loading