Spree::Payment::Processing refactor#4823
Merged
Conversation
github-actions
bot
added
the
changelog:solidus_core
Codecov Report
@@ Coverage Diff @@
## master #4823 +/- ##
=======================================
Coverage 86.23% 86.23%
=======================================
Files 578 578
Lines 14674 14671 -3
=======================================
- Hits 12654 12652 -2
+ Misses 2020 2019 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
jarednorman
approved these changes
Jan 3, 2023
adammathys
approved these changes
Jan 4, 2023
tvdeyen
approved these changes
Jan 4, 2023
Member
tvdeyen
left a comment
tvdeyen
left a comment
There was a problem hiding this comment.
Thanks for clearing this up! 🙏🏻
kennyadsl
reviewed
Jan 5, 2023
Member
kennyadsl
left a comment
kennyadsl
left a comment
There was a problem hiding this comment.
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.
elia
force-pushed
the
elia/payment-processing-refactor
branch
2 times, most recently
from
894fb65 to
d00ddb9
Compare
kennyadsl
reviewed
Jan 5, 2023
elia
force-pushed
the
elia/payment-processing-refactor
branch
2 times, most recently
from
68acd75 to
609ed5f
Compare
Member
|
@elia I think you pushed the wrong branch here. Can you please check? |
Member
Author
|
@kennyadsl 🤦♂️ on it thanks |
elia
force-pushed
the
elia/payment-processing-refactor
branch
from
609ed5f to
4882cc6
Compare
Member
Author
|
@kennyadsl done 👍 |
Member
|
@elia please rebase! |
This turns a deeply nested set of `if`s into a list of guard clauses.
elia
force-pushed
the
elia/payment-processing-refactor
branch
from
4882cc6 to
5a34895
Compare
waiting-for-dev
approved these changes
Jan 20, 2023
Contributor
waiting-for-dev
left a comment
waiting-for-dev
left a comment
There was a problem hiding this comment.
Very nice job ❤️
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
force-pushed
the
elia/payment-processing-refactor
branch
from
5a34895 to
a5ec43b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
The following are not always needed (
cross them outif 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.