-
Notifications
You must be signed in to change notification settings - Fork 23
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
Support solidus 3.0 #122
Support solidus 3.0 #122
Conversation
68e1de4
to
05a4876
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @DanielePalombo , leaving a question, hope it helps
|
||
it { expect { to_json }.not_to raise_error } | ||
|
||
if Spree.solidus_gem_version >= Gem::Version.new(2.11) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we use two contexts with different Spree.solidus_gem_version
?
This way the whole functionality would always be tested, and not dependently of the bundle when running the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I had made two contexts would mean that I have had stub the Spree.solidus_gem_version, in that way I can test the method as BlackBox.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits and questions
05a4876
to
b51bfc7
Compare
It seems this PR meets all the requirements now, I hope it will be approved soon enough. Support for solidus 3.0 is necessary to work with this extension. Keep on the good work! |
b51bfc7
to
63fe8f9
Compare
Hi @DanielePalombo 👋 But from this commit, if the Solidus version is greater than 2.10, then the |
@RyanofWoods nice catch, I don't see any spec around this behavior so I assume this is just an error. |
On the following line, the billing address is given to the name method. https://github.com/solidusio-contrib/solidus_paypal_commerce_platform/blob/64be7ddb4967a93155c035430101d4bbfce83313/app/models/solidus_paypal_commerce_platform/paypal_order.rb#L40 The following commit accidentally started using the order's ship_address instead of the given billing address when Solidus' version is >= 2.11. solidusio@63fe8f9 Reference: solidusio#122 (comment)
No description provided.