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.1 support #5359

Merged
merged 24 commits into from
Dec 21, 2023
Merged

Rails 7.1 support #5359

merged 24 commits into from
Dec 21, 2023

Conversation

peterberkenbosch
Copy link
Contributor

@peterberkenbosch peterberkenbosch commented Aug 26, 2023

Summary

Preparation for supporting Rails 7.1. Very much work in progress, although it looks like the changes needed are minimal so far. The sandbox is running successfully, there are some failing specs that need to be addressed.

One of the current RSpec errors are related to this rails/rails#49591 and it looks that this will be fixed in the upcoming Rails 7.1.2 release.

Ransack is pain still. The custom predicates with formatters is failing hard currently:

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

# The :variants_option_values ransacker filters Spree::Product based on
    # variant option values ids.
    #
    # Usage:
    # Spree::Product.ransack(
    #   variants_option_values_in: [option_value_id1, option_value_id2]
    # ).result
    ransacker :variants_option_values, formatter: proc { |v|
      if OptionValue.exists?(v)
        joins(variants_including_master: :option_values)
          .where(spree_option_values: { id: v })
          .distinct
          .select(:id).arel
      end
    } do |parent|
      parent.table[:id]
    end
1) Spree::Product ransacker :variants_option_values filters products based on option values of their variants
     Failure/Error: result = Spree::Product.ransack(variants_option_values_in: [option_value_1.id]).result
     
     NoMethodError:
       undefined method `value' for #<Arel::SelectManager:0x00000001160b22c0 @ast=#<Ar

Figuring out where this value method call used to be.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • πŸ“– I have updated the README to account for my changes.
  • πŸ“‘ I have documented new code with YARD.
  • πŸ›£οΈ I have opened a PR to update the guides.
  • βœ… I have added automated tests to cover my changes.
  • πŸ“Έ I have attached screenshots to demo visual changes.

@github-actions github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Aug 26, 2023
@codecov
Copy link

codecov bot commented Aug 26, 2023

Codecov Report

Merging #5359 (42fac2a) into main (d095e38) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 42fac2a differs from pull request most recent head bf51cab. Consider uploading reports for the commit bf51cab to get more accurate results

@@           Coverage Diff           @@
##             main    #5359   +/-   ##
=======================================
  Coverage   88.71%   88.71%           
=======================================
  Files         563      563           
  Lines       13896    13896           
=======================================
  Hits        12328    12328           
  Misses       1568     1568           
Files Changed Coverage Ξ”
core/app/models/spree/preference.rb 100.00% <100.00%> (ΓΈ)
core/app/models/spree/return_item.rb 91.40% <100.00%> (ΓΈ)
core/lib/spree/preferences/persistable.rb 100.00% <100.00%> (ΓΈ)

πŸ“£ We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions github-actions bot added the changelog:solidus_backend Changes to the solidus_backend gem label Aug 26, 2023
@github-actions github-actions bot added the changelog:repository Changes to the repository not within any gem label Aug 26, 2023
@peterberkenbosch
Copy link
Contributor Author

specs are almost passing. Figuring out why we now get different apostrophe's back from our api..

Took me a few to figure out why this was not matching:

expected: "can't be blank"
            got: "can’t be blank"

' vs ’

@peterberkenbosch
Copy link
Contributor Author

Intentional Rails update:

*   Improve typography of user facing error messages. In English contractions,
    the Unicode APOSTROPHE (`U+0027`) is now RIGHT SINGLE QUOTATION MARK
    (`U+2019`). For example, "can't be blank" is now "can’t be blank".

    *Jon Dufresne*

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Left an initial set of comments; thanks a ton for this great contribution, @peterberkenbosch! ❀️

@@ -4,8 +4,8 @@ module Spree
module Admin
class OrdersController < Spree::Admin::BaseController
before_action :initialize_order_events
before_action :load_order, only: [:edit, :update, :complete, :advance, :cancel, :resume, :approve, :resend, :unfinalize_adjustments, :finalize_adjustments, :cart, :confirm]
Copy link
Member

Choose a reason for hiding this comment

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

Good finding! Did you check if we also have a (unused) route for this action?

@@ -347,4 +347,7 @@ workflows:
- test_solidus:
name: *name
matrix: { parameters: { rails: ['7.0'], ruby: ['3.2'], database: ['sqlite'], paperclip: [false] } }
- test_solidus:
Copy link
Member

Choose a reason for hiding this comment

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

It's a good idea to always test against the latest Rails development branch. We should probably also make this check not mandatory in the GH actions report on PRs.

Copy link
Member

Choose a reason for hiding this comment

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

I'm moving this to a different PR so we can focus on 7.1

core/app/models/spree/shipping_rate.rb Outdated Show resolved Hide resolved
core/lib/solidus_core.rb Outdated Show resolved Hide resolved
@kennyadsl
Copy link
Member

@peterberkenbosch Hey Peter, did you have any chance to keep up with this? I'd volunteer to continue the work if you want, just let me know please.

@peterberkenbosch
Copy link
Contributor Author

@kennyadsl feel free to pick up. I haven't had the proper focus time to jump on this. Would be nice to have this done.

@kennyadsl
Copy link
Member

Made another pass to fix other deprecation warnings and errors. As far as I can see the biggest thing left are ransack and previews, which require more time.

@github-actions github-actions bot added the changelog:solidus_sample Changes to the solidus_sample gem label Dec 15, 2023
@elia elia changed the title Rails edge (7.1) support Rails 7.1 support Dec 20, 2023
@elia elia self-assigned this Dec 20, 2023
@elia elia force-pushed the rails-edge branch 2 times, most recently from 359448b to 7d91dc7 Compare December 20, 2023 11:25
@elia elia marked this pull request as ready for review December 20, 2023 21:15
@elia elia requested a review from a team as a code owner December 20, 2023 21:15
@elia elia marked this pull request as draft December 20, 2023 21:17
@elia elia marked this pull request as ready for review December 21, 2023 09:59
@elia elia requested a review from rainerdema December 21, 2023 09:59
peterberkenbosch and others added 18 commits December 21, 2023 11:38
This commit removes the following deprecations:

DEPRECATION WARNING: Spree::Order model aliases `bill_address`, but `bill_address` is not an attribute. Starting in Rails 7.2, alias_attribute with non-attribute targets will raise. Use `alias_method :billing_address, :bill_address` or define the method manually.

DEPRECATION WARNING: Spree::Order model aliases `ship_address`, but `ship_address` is not an attribute. Starting in Rails 7.2, alias_attribute with non-attribute targets will raise. Use `alias_method :shipping_address, :ship_address` or define the method manually.

DEPRECATION WARNING: Spree::CreditCard model aliases `cc_type` and has a method called `cc_type=` defined. Starting in Rails 7.2 `brand=` will not be calling `cc_type=` anymore. You may want to additionally define `brand=` to preserve the current behavior.
Apparently now Rails returns the host without the initial .
…cord::Base

DEPRECATION WARNING: SchemaMigration no longer inherits from ActiveRecord::Base. If you want to use the default connection, remove this argument. If you want to use a specific connection, instantiate MigrationContext with the connection's schema migration, for example `MigrationContext.new(path, Dog.connection.schema_migration)`. (called from new at /Users/andrea/nebulab/solidus/core/lib/spree/testing_support/dummy_app/rake_tasks.rb:41)
Due to deprecations, with a previous commit we replaced `alias_attribute`
calls for these two attributes with `alias_method`, but the latter doesn't
fully cover the previous behavior.

When `shipping_address` or `billing_address` are passed as attributes,
the error `ActiveModel::UnknownAttributeError` is now raised. For this
reason, we're starting to replace them with the right association name.

We may want to revisit the possibility to still use `billing_address` and
`shipping_address` as providing these aliases doesn't seem to be fully
viable anymore.
Starting from Rails 7.1, `ActionView::TestCase#output_buffer` uses
`ActionView::OutputBuffer` rather than `ActiveSupport::SafeBuffer`.

Due to the slightly different behavior, before comparing it to a
string we we need first to convert it.

See rails/rails@532e39a
In Rails 7.1 AS will check the global configuration when the macro is
used.
Adding the previews path after setting the autoload ensures the file
is not autoloaded by Zeitwerk.

Co-Authored-By: andrea longhi <[email protected]>
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks for completing the work here, @elia @spaghetticode!

Copy link
Contributor

@rainerdema rainerdema left a comment

Choose a reason for hiding this comment

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

πŸ‘πŸ‘πŸ‘

@elia elia merged commit 993fb59 into solidusio:main Dec 21, 2023
11 checks passed
@tvdeyen
Copy link
Member

tvdeyen commented Dec 21, 2023

Great work! Thanks πŸ™πŸ»

@peterberkenbosch
Copy link
Contributor Author

great!! thanks all for pushing this over the finish line! πŸŽ‰

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:repository Changes to the repository not within any gem changelog:solidus_admin changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem changelog:solidus_sample Changes to the solidus_sample gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants