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

Deprecate the Spree::Adjustment.return_authorization scope #5138

Merged

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Jun 9, 2023

Summary

There is no code path that creates adjustments from return authorizations (since a long time).

This line was introduced without a lot of context in 2013, and there is nothing in the current codebase that warrants this scope.

I don't think adding a deprecation notice will do anything for anyone. If anyone tells me this scope currently returns a record in their stores, I'll add one. :)

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.

@mamhoff mamhoff requested a review from a team as a code owner June 9, 2023 13:54
@github-actions github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Jun 9, 2023
@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #5138 (60b4916) into main (b00ae25) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5138   +/-   ##
=======================================
  Coverage   88.76%   88.76%           
=======================================
  Files         564      564           
  Lines       13959    13960    +1     
=======================================
+ Hits        12391    12392    +1     
  Misses       1568     1568           
Files Changed Coverage Ξ”
core/app/models/spree/adjustment.rb 93.84% <100.00%> (+0.09%) ⬆️

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

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

βœ‚οΈ

Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

How long has this been unused in Solidus?

@mamhoff
Copy link
Contributor Author

mamhoff commented Jun 12, 2023

I have not found a single usage in Solidus since v1.0. I've checked in the Spree repo as well, where I could not find usages of this either.

@waiting-for-dev
Copy link
Contributor

Thanks! Given the minimal effort, it won't hurt to add a deprecation message. It wouldn't be unexpected to find something unexpected in some stores πŸ™‚

@elia elia self-assigned this Sep 25, 2023
@elia elia added this to the 4.2 milestone Sep 25, 2023
There is no code path that creates adjustments from return
authorizations (since a long time).

This line was introduced without a lot of context in 2013, and there is
nothing in the current codebase that warrants this scope.
@elia elia force-pushed the remove-adjustment-return-authorizations-scope branch from 5146714 to 60b4916 Compare September 25, 2023 17:01
@elia elia changed the title Remove Spree::Adjustment.return_authorization scope Deprecate the Spree::Adjustment.return_authorization scope Sep 26, 2023
@elia elia merged commit 69ba182 into solidusio:main Sep 26, 2023
11 checks passed
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.

None yet

5 participants