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

Remove Deprecation Warning in ActiveModel Errors #3946

Merged

Conversation

Azeem838
Copy link
Contributor

@Azeem838 Azeem838 commented Feb 19, 2021

Description

This PR is a solution for #3924

Enumerating ActiveModel::Errors as a hash has been deprecated. In Rails 6.1, errors is an array of Error objects, therefore it should be accessed by a block with a single block parameter.

This resulted in deprecation warnings in the payment and product models for the validate_source and validate_master methods respectively.

Since this is deprecated, it will result in an ArgumentError in Rails 6.2.

Other Deprecation Warning
There were warnings relating to the shovel operator being used in various model validators. As with the previous deprecation warning, the use of the shovel operator within this context has been deprecated. The affected methods were changed to use ruby's add method instead.
This PR takes care of this issue as well

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)
  • I have attached screenshots to this PR for visual changes (if needed)

@Azeem838 Azeem838 marked this pull request as draft February 19, 2021 07:09
Copy link
Member

@aldesantis aldesantis left a comment

Choose a reason for hiding this comment

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

@Azeem838 thanks for the work here!

In order to make the code work with Rails 5.2 and 6.0 as well, you can simply check the current Rails version in an if/else branch.

@Azeem838 Azeem838 marked this pull request as ready for review February 20, 2021 12:02
@Azeem838 Azeem838 marked this pull request as draft February 20, 2021 12:03
@Azeem838 Azeem838 force-pushed the refactor/remove-deprecation-warning branch from b57fc98 to b4e9333 Compare February 20, 2021 12:54
@Azeem838 Azeem838 marked this pull request as ready for review February 20, 2021 12:54
@Azeem838 Azeem838 marked this pull request as draft February 20, 2021 13:11
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, I left an improvement for checking the Rails version requirement.

core/app/models/spree/payment.rb Outdated Show resolved Hide resolved
@Azeem838 Azeem838 force-pushed the refactor/remove-deprecation-warning branch from 8409b90 to dc5c975 Compare February 21, 2021 15:52
@Azeem838 Azeem838 marked this pull request as ready for review February 21, 2021 16:11
@kennyadsl
Copy link
Member

Thanks! Can you please just squash commits into a single one and we are good to go! 🙏

Refactor validation methods from using the shovel operator to use the add method.

Refactor validation methods and to accept a single block parameter.
@Azeem838 Azeem838 force-pushed the refactor/remove-deprecation-warning branch from dc5c975 to b570715 Compare February 22, 2021 15:31
@Azeem838
Copy link
Contributor Author

Azeem838 commented Feb 23, 2021

@aldesantis and @kennyadsl, thank you both for your advice on this PR. Please let me know if there are any other changes that needs to be made.

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.

Thank you! 🙏

@kennyadsl kennyadsl added the changelog:solidus_core Changes to the solidus_core gem label Feb 23, 2021
@kennyadsl kennyadsl added this to the 3.1.0 milestone Feb 23, 2021
Copy link
Member

@aldesantis aldesantis left a comment

Choose a reason for hiding this comment

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

Awesome stuff, thanks @Azeem838!

@kennyadsl kennyadsl merged commit f04cd15 into solidusio:master Feb 23, 2021
@kennyadsl kennyadsl modified the milestones: 3.1.0, 2.11 Feb 23, 2021
@kennyadsl
Copy link
Member

I backported this to v2.11 so we remove the warning for users using that Solidus version.

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

Successfully merging this pull request may close these issues.

4 participants