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

Spree::Payment::Processing refactor #4823

Merged
merged 5 commits into from
Jan 20, 2023
Merged

Conversation

elia
Copy link
Member

@elia elia commented Dec 29, 2022

Summary

While working on solidus_stripe I had a deeper look at how the processing is handled and took the chance to straight out a few indirections.

Since it's only touching private APIs we shouldn't need any special handling or deprecations.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

  • I have written a thorough PR description.
  • I have kept my commits small and atomic.
  • I have used clear, explanatory commit messages.

The following are not always needed (cross them out if they are not):

- [ ] I have added automated tests to cover my changes. - [ ] I have attached screenshots to demo visual changes. - [ ] I have opened a PR to update the [guides](https://github.com/solidusio/edgeguides). - [ ] I have updated the README to account for my changes.

@elia elia self-assigned this Dec 29, 2022
@github-actions github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Dec 29, 2022
@codecov
Copy link

codecov bot commented Dec 29, 2022

Codecov Report

Merging #4823 (a5ec43b) into master (c6050d7) will increase coverage by 0.00%.
The diff coverage is 94.28%.

@@           Coverage Diff           @@
##           master    #4823   +/-   ##
=======================================
  Coverage   86.23%   86.23%           
=======================================
  Files         578      578           
  Lines       14674    14671    -3     
=======================================
- Hits        12654    12652    -2     
+ Misses       2020     2019    -1     
Impacted Files Coverage Δ
core/app/models/spree/payment/processing.rb 98.03% <94.28%> (+0.89%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@elia elia marked this pull request as ready for review December 29, 2022 14:44
@elia elia requested a review from a team as a code owner December 29, 2022 14:44
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.

Thanks for clearing this up! 🙏🏻

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 some comments here and there but I think that this change is worth it, there was a lot of premature optimization in this class, while we probably need more simplicity.

core/app/models/spree/user_last_url_storer.rb Outdated Show resolved Hide resolved
core/app/models/spree/payment/processing.rb Show resolved Hide resolved
core/app/models/spree/payment/processing.rb Outdated Show resolved Hide resolved
core/app/models/spree/payment/processing.rb Show resolved Hide resolved
@elia elia force-pushed the elia/payment-processing-refactor branch 2 times, most recently from 894fb65 to d00ddb9 Compare January 5, 2023 22:46
@elia elia requested a review from kennyadsl January 5, 2023 22:47
@elia elia force-pushed the elia/payment-processing-refactor branch 2 times, most recently from 68acd75 to 609ed5f Compare January 16, 2023 16:54
@kennyadsl
Copy link
Member

@elia I think you pushed the wrong branch here. Can you please check?

@elia
Copy link
Member Author

elia commented Jan 17, 2023

@kennyadsl 🤦‍♂️ on it thanks

@elia elia force-pushed the elia/payment-processing-refactor branch from 609ed5f to 4882cc6 Compare January 17, 2023 11:22
@elia
Copy link
Member Author

elia commented Jan 17, 2023

@kennyadsl done 👍

@kennyadsl
Copy link
Member

@elia please rebase!

elia added 2 commits January 19, 2023 19:01
This turns a deeply nested set of `if`s into a list of guard clauses.
@elia elia force-pushed the elia/payment-processing-refactor branch from 4882cc6 to 5a34895 Compare January 19, 2023 18:01
Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

Very nice job ❤️

elia added 3 commits January 20, 2023 11:58
The internal helpers were adding more noise and indirection than
clarity. Now the calls to `#purchase` and `#authorize` are clearly
This way we group simple checks and checks that would raise
exceptions in different groups for better readability.
Remove some indirection and unnecessary meta-programming.
Have it return either true or false leaving the "success" state call
as a responsibility of the caller (the "failure_state" was always the
same).

Also remove one nesting level in the implementation.
@elia elia force-pushed the elia/payment-processing-refactor branch from 5a34895 to a5ec43b Compare January 20, 2023 10:58
@elia elia requested a review from kennyadsl January 20, 2023 10:59
@kennyadsl kennyadsl merged commit 36340a8 into master Jan 20, 2023
@kennyadsl kennyadsl deleted the elia/payment-processing-refactor branch January 20, 2023 12:06
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.

6 participants